My AMD IDE driver, v2.7

Quoted 5 lines

You may happen to have the numbers, though - that should be enough.
Btw, I have a CMD640B based PCI card lying around here, but never managed to get it generate any interrupts, though the rest seems to be working.

Attach it to the timer interrupt -- that should do it for testing. Simplest way is to make ide timeouts HZ/100 and killing "lost interrupt" msg ;-).

Pavel

Signature · 2 lines

Philips Velo 1: 1"x4"x8", 300gram, 60, 12MB, 40bogomips, linux, mutt,
details at http://atrey.karlin.mff.cuni.cz/~pavel/velo/index.html.

Vojtech Pavlik

author

2002-03-11 22:46 UTC #7 lore raw

Quoted 16 lines · Linus Torvalds

Quoted 2 lines · Alan Cox
Its quite different to other goings on
Alan, did you actually look at the diffs that Martin sent out, or are you just reacting to the description?
I think you read more into the description than was actually in the patch itself.
Rule #1: always read the patch.
Right now, that patch definitely needs to learn to use "yield()" instead of "schedule()" etc details, but I really don't understand why all the brouhaha over Martins patches.

Quoted 7 lines

Am I really the only one who actually reads the actual _changes_ instead of arguing over personal issues?
Now, I've long had this theory that IDE coding is bad for your mental health (you won't ever see _me_ going close to the dang thing - I'll use it, but I won't start writing code for it), but that theory used to be a _joke_, for chrissake! Don't make it appear a truism.

Are you, by any chance, confusing my AMD IDE changes with Pavel Machek's first attempt at IDE wake/suspend code?

Gee, I think we all need some calming down and sleep ...

Signature · 2 lines

Vojtech Pavlik
SuSE Labs

Linus Torvalds

2002-03-11 22:55 UTC #8 lore raw

Quoted 3 lines · Vojtech Pavlik

Are you, by any chance, confusing my AMD IDE changes with Pavel Machek's first attempt at IDE wake/suspend code?

No, I'm just reacting to alan's email where he mentions the "other goings on" - as opposed to your patch which even Alan agreed with.

Note that I've actually tried to read all patches, and right now they are all in my tree (of course, the "all" is the Linus kind of "all", which only includes the ones I actually _noticed_.

That includes your AMD IDE driver change _and_ the oh-so-much-discussed patches by Martin (which in turn included Pavel's change, which - together with his changelog entry - seems to be the one that triggered the "lively discussions").

Linus

Bill Davidsen

2002-03-12 00:16 UTC #9 lore raw

Quoted 8 lines · Linus Torvalds

Note that I've actually tried to read all patches, and right now they are all in my tree (of course, the "all" is the Linus kind of "all", which only includes the ones I actually _noticed_.
That includes your AMD IDE driver change _and_ the oh-so-much-discussed patches by Martin (which in turn included Pavel's change, which - together with his changelog entry - seems to be the one that triggered the "lively discussions").

I think you might want to offer an opinion (or edict, mandate, whatever) WRT taskfile. While I think that some protective editing of commands is desirable, useful, etc, I'm damn sure that some form of raw access is required long term to allow diagmostics. I wouldn't argue if that interface was required for low level format and other really drastic operations as well.

I think "future new commands" is total FUD, the idea that some new command would come along and be so instantly popular, useful and incompatible that all Linux boxes would require it before the next kernel or driver update is silly at best, and I'm working hard to keep this on a civil plane. Linux moves a hell of a lot faster than Windows, and I can't see anything coming out which requires "instant attention" because it is totally incompatible with existing drivers. Most changes will be only added functionality, like 48 bit addressing, and will be a superset of current drivers. In other words, not everyone will wnat or need the capability, and no one is going to feel that a month or so wait from driver TESTING would cause them to give up computing and become a stripper in a gay bar.

In any case, taskfile is here, and unless there is a good reason to drop lightly edited commands, or raw commands, or go to some totally different approach, I wish you would express a clear opinion, at least WRT 2.4, before developers become nasty and stop limiting themselves to accusations of stupidity, incompetence, inreliability and prevarication.

Signature · 3 lines

bill davidsen <[email protected]>
  CTO, TMR Associates, Inc
Doing interesting things with little computers since 1979.

Jeff Garzik

2002-03-12 00:35 UTC #10 lore raw

Quoted 10 lines · Bill Davidsen

Quoted 7 lines · Linus Torvalds
Note that I've actually tried to read all patches, and right now they are all in my tree (of course, the "all" is the Linus kind of "all", which only includes the ones I actually _noticed_.
That includes your AMD IDE driver change _and_ the oh-so-much-discussed patches by Martin (which in turn included Pavel's change, which - together with his changelog entry - seems to be the one that triggered the "lively discussions").

Quoted 7 lines

I think you might want to offer an opinion (or edict, mandate, whatever) WRT taskfile. While I think that some protective editing of commands is desirable, useful, etc, I'm damn sure that some form of raw access is required long term to allow diagmostics. I wouldn't argue if that interface was required for low level format and other really drastic operations as well.

One sticking point seems to be the question of whether there needs to be some amount of validation performed on the commands passed through the taskfile interface. I don't think Andre buys into the argument that most other kernel hackers understand, which is, "if userspace is allowed to bang away at random i/o ports, why validate taskfile requests in the kernel?"

And IMO, we should have some basic validation of taskfile requests in the kernel...
Reason 1: Standard kernel convention. In other ioctls, we check basic arguments and return EINVAL when they are wrong, even for privieleged ioctls.
Reason 2: If you have multiple programs issuing ATA commands, you would want a decent amount of synchronization, provided by the kernel, for the multiple user processes and multiple kernel processes issuing requests. Having the userspace commands come down a single spot in the kernel code makes this job a lot easier, if not making the impossible possible :)

Regards,

Jeff

Bill Davidsen

2002-03-12 16:42 UTC #11 lore raw

Quoted 5 lines · Jeff Garzik

And IMO, we should have some basic validation of taskfile requests in the kernel...
Reason 1: Standard kernel convention. In other ioctls, we check basic arguments and return EINVAL when they are wrong, even for privieleged ioctls.

If we assume that this path is for commands the kernel doesn't understand, how do we validate them?

Quoted 5 lines

Reason 2: If you have multiple programs issuing ATA commands, you would want a decent amount of synchronization, provided by the kernel, for the multiple user processes and multiple kernel processes issuing requests.
Having the userspace commands come down a single spot in the kernel
code makes this job a lot easier, if not making the impossible possible :)

Linus addressed this, I agree with his proposed three part implementation. AFAIK he said the same thing in a different way.

Signature · 3 lines

bill davidsen <[email protected]>
  CTO, TMR Associates, Inc
Doing interesting things with little computers since 1979.

Erik Andersen

2002-03-12 00:59 UTC #12 lore raw

Quoted 3 lines · Jeff Garzik

Reason 1: Standard kernel convention. In other ioctls, we check basic arguments and return EINVAL when they are wrong, even for privieleged ioctls.

I have no argument with basic command validation. But take a look at ide_cmd_type_parser(), for example. Do we really need a giant switch statement listing all the allowed commands, just so we can throw back a IDE_DRIVE_TASK_INVALID to user-space if they decide to send down some undocumeted firmware wiping commands? Especially since that giant struct of allowed commands is duplicated in ide_pre_handler_parser() and ide_handler_parser()

-Erik

--
Erik B. Andersen http://codepoet-consulting.com/
--This message was written using 73% post-consumer electrons--

Linus Torvalds

2002-03-12 01:06 UTC #13 lore raw

Quoted 7 lines · Bill Davidsen

I think you might want to offer an opinion (or edict, mandate, whatever) WRT taskfile. While I think that some protective editing of commands is desirable, useful, etc, I'm damn sure that some form of raw access is required long term to allow diagmostics. I wouldn't argue if that interface was required for low level format and other really drastic operations as well.

Quite frankly, my personal opinion is that there's no point in trying to parse the commands too much at all. The _only_ thing the kernel should try to care about is that the buffers passed to the command are valid (ie the actual data in/out area pointer should be a kernel buffer as far as possible, not under user control).

Basically, there are three "modes" of operation here:

- regular kernel access (ie the normal read/write stuff)

parsing: none. The kernel originated the request.

- common ioctl's that have nothing to do with IDE per se (ie all the stuff to open/close/lock removable media, unlocking DVD's, reading sound sectors etc - stuff that really exists elsewhere too, and where the kernel should do the translation from the generic problem space to the specific commands for the bus in quesion)

Parsing: common ioctl turning stuff into common SCSI commands in the "struct request".

- totally IDE-only stuff, where the kernel simply doesn't add any value, and only has a way of passing it down to the drive, and passing back the requests.

Parsing: none. The kernel simply doesn't have anything to do with the request, except to synchronize it with all other requests.

The only common factor here is the "synchronize with other requests" - I feel strongly (much more strongly than any parsing notion) that the raw requests have to be passed down the "struct request" and NOT be done the way they are traditionally done (ie completely outside the request stream, with no synchronization at all with any IO currently in progress).

Quoted 4 lines

I think "future new commands" is total FUD, the idea that some new command would come along and be so instantly popular, useful and incompatible that all Linux boxes would require it before the next kernel or driver update is silly at best, and I'm working hard to keep this on a civil plane.

It has nothing to do with "new" commands, and everything to do with "random vendor-specific commands and the vendor-specific tools". Commands that simply should _never_ be parsed in the kernel, because we do not want to care about 10 different vendors 10 different revisions of their firmware having 10 different small random special commands for that particular drive.

In particular, a user that upgrades his hardware should never _ever_ have to upgrade his kernel just because some random disk diagnostic tool needs support for a disk that is new and has new diagnostics.

Linus

Jeff Garzik

2002-03-12 01:42 UTC #14 lore raw

Quoted 6 lines · Linus Torvalds

The only common factor here is the "synchronize with other requests" - I feel strongly (much more strongly than any parsing notion) that the raw requests have to be passed down the "struct request" and NOT be done the way they are traditionally done (ie completely outside the request stream, with no synchronization at all with any IO currently in progress).

agreed

Quoted 13 lines

Quoted 5 lines
I think "future new commands" is total FUD, the idea that some new command would come along and be so instantly popular, useful and incompatible that all Linux boxes would require it before the next kernel or driver update is silly at best, and I'm working hard to keep this on a civil plane.
It has nothing to do with "new" commands, and everything to do with "random vendor-specific commands and the vendor-specific tools". Commands that simply should _never_ be parsed in the kernel, because we do not want to care about 10 different vendors 10 different revisions of their firmware having 10 different small random special commands for that particular drive.

Quoted 4 lines

In particular, a user that upgrades his hardware should never _ever_ have to upgrade his kernel just because some random disk diagnostic tool needs support for a disk that is new and has new diagnostics.

Are such random vendor-specific commands really that common?

Linus, would it be acceptable to you to include an -optional- filter for ATA commands? There is definitely a segment of users that would like to firewall their devices, and I think (as crazy as it may sound) that notion is a valid one.

Jeff

Linus Torvalds

2002-03-12 01:59 UTC #15 lore raw

Quoted 5 lines · Jeff Garzik

Linus, would it be acceptable to you to include an -optional- filter for ATA commands? There is definitely a segment of users that would like to firewall their devices, and I think (as crazy as it may sound) that notion is a valid one.

I'd rather have the rule that you can turn off this feature altogther: if the command is so esoteric that it's not part of the regular commands we can generate on our own, 99% of all people probably aren't even interested in it.

If the command is useful on its own, and has a generic meaning (ie across a large subset of users), then I think the kernel should support it through a _real_ interface, not through some "pass user data though inscathed" thing. Because that generic meaning probably exists even outside ATA drives.

Linus

Jeff Garzik

2002-03-12 02:23 UTC #16 lore raw

Quoted 9 lines · Linus Torvalds

Quoted 5 lines · Jeff Garzik
Linus, would it be acceptable to you to include an -optional- filter for ATA commands? There is definitely a segment of users that would like to firewall their devices, and I think (as crazy as it may sound) that notion is a valid one.

Quoted 5 lines

I'd rather have the rule that you can turn off this feature altogther: if the command is so esoteric that it's not part of the regular commands we can generate on our own, 99% of all people probably aren't even interested in it.

Are we talking about the same thing or different things?

If filtering is done, I agree the filter feature is disable-able if the kernel builder / sysadmin desires such. Disable the filter by default, if that's your fancy. But let us filter. :)

Quoted 6 lines

If the command is useful on its own, and has a generic meaning (ie across a large subset of users), then I think the kernel should support it through a _real_ interface, not through some "pass user data though inscathed" thing. Because that generic meaning probably exists even outside ATA drives.

Let me first agree on a point, to get that out of the way. I agree there is generic meaning outside of ATA drives,for most of the ATA[PI] commands. Further, I hope you know by now I agree with your taste in kernel interfaces :)

First, for verification of the driver, it's darned useful to be able to issue ATA commands from userspace. But since we're being generic, let's just call them device commands, and imagine SCSI perhaps playing in this playground too.

Second, if you are issuing device commands from userspace, you need to deal with synchronization with the commands being issued from kernel space.

Third, if you have synchronization, that's a good and easy point to add -optional- filtering.

Now... the overall point I am trying to sell is... IFF certain lesser-used device commands are primarily issued from userspace, and IFF it is not reasonable to create a new kernel interface, is it not reasonable to be able to issue raw device commands from userspace? Just the verification case alone seems like a strong argument.

Call it device_command not ata_command if you want, and add SCSI support. Passing in raw device_commands from userspace is useful, IMO. Throw in filtering of those commands, and life is golden for everybody AFAICS. There is no need to be ATA specific with these goals...

Jeff

Linus Torvalds

2002-03-12 02:47 UTC #17 lore raw

Quoted 4 lines · Jeff Garzik

If filtering is done, I agree the filter feature is disable-able if the kernel builder / sysadmin desires such. Disable the filter by default, if that's your fancy. But let us filter. :)

BUT WHAT THE HELL IS THE POINT?

Don't you get that? If the sysadmin can turn the filtering off, so can any root program. And your worry seems to be the CRM kind of disk locking utility which most _definitely_ would do exactly that if it is as evil as you think it is.

And if you hardcode the filtering at compile-time in the kernel, that means that you've now painted yourself into the corner that you already seemed to admit was not a good idea - the same way it's not a good idea for network filtering.

Linus

Jeff Garzik

2002-03-12 02:38 UTC #18 lore raw

Quoted 8 lines · Linus Torvalds

Quoted 4 lines · Jeff Garzik
If filtering is done, I agree the filter feature is disable-able if the kernel builder / sysadmin desires such. Disable the filter by default, if that's your fancy. But let us filter. :)

Quoted 7 lines

BUT WHAT THE HELL IS THE POINT?
Don't you get that? If the sysadmin can turn the filtering off, so can any root program. And your worry seems to be the CRM kind of disk locking utility which most _definitely_ would do exactly that if it is as evil as you think it is.

dynamic filtering sucks, I agree, and I am not arguing for that.

Quoted 5 lines

And if you hardcode the filtering at compile-time in the kernel, that means that you've now painted yourself into the corner that you already seemed to admit was not a good idea - the same way it's not a good idea for network filtering.

No, I think hardcoding at compile time is useful here.

It serves to encourage openness, nobody is forced to use it, and it provides an additional layer of protection for those that choose to use it. That is the point. It's a choice, and you don't have to enable it in your kernel. But there seems to be enough demand that it should be at least an option.

Jeff

Olivier Galibert

2002-03-12 03:34 UTC #19 lore raw

Quoted 3 lines · Jeff Garzik

It serves to encourage openness, nobody is forced to use it, and it provides an additional layer of protection for those that choose to use it. That is the point.

It doesn't provide any meaningful protection, that's the point.

If you're root/have CAP_SYS_RAWIO, you can bit-bang the interface, you can patch out the filter from the kernel binary, you can do whatever pleases you. Don't run evil programs as root in the first place. And if you want to have finer-grained capabilities for specific drive-level actions, create an higher-level interface for them which will guarantee that only safe commands are used because they will be generated by the kernel in the first place.

Broken security is actually worse than no security. With no security you at least know what to expect.

The exact same discussion happened with Andre, please refer to it.

OG.

Jeff Garzik

2002-03-12 04:14 UTC #20 lore raw

Quoted 5 lines · Olivier Galibert

Quoted 2 lines · Jeff Garzik
It serves to encourage openness, nobody is forced to use it, and it provides an additional layer of protection for those that choose to use it. That is the point.

Quoted 9 lines

It doesn't provide any meaningful protection, that's the point.
If you're root/have CAP_SYS_RAWIO, you can bit-bang the interface, you can patch out the filter from the kernel binary, you can do whatever pleases you. Don't run evil programs as root in the first place. And if you want to have finer-grained capabilities for specific drive-level actions, create an higher-level interface for them which will guarantee that only safe commands are used because they will be

Under more restricted domains, root cannot bit-bang the interface. s/CAP_SYS_RAWIO/CAP_DEVICE_CMD/ for the raw cmd ioctl interface. Have the OS trap I/O port accesses using SMM mode if you would like, and that applies to your particular security situation.

The filter is useful for other reasons like correctness, as well.

Jeff

Martin Dalecki

2002-03-12 11:26 UTC #21 lore raw

Quoted 1 line · Jeff Garzik

It serves to encourage openness, nobody is forced to use it, and it provides an additional layer of protection for those that choose to use it. That is the point. It's a choice, and you don't have to enable it in your kernel. But there seems to be enough demand that it should be at least an option.

No there is no real demand out there. There are just people arguing over it again and agian on the abstract, without proper actual usage examples. and most of the time without proper understanding what setuid == 0 is about.

Pavel Machek

2002-03-15 10:59 UTC #22 lore raw

Quoted 2 lines

Under more restricted domains, root cannot bit-bang the interface.
s/CAP_SYS_RAWIO/CAP_DEVICE_CMD/ for the raw cmd ioctl interface. Have

Nobody uses capabilities these days, right?

Quoted 1 line

The filter is useful for other reasons like correctness, as well.

If you want to test if it works, you just disallow that access altogether. It is usually not needed , anyway.
Pavel

Signature · 2 lines

Philips Velo 1: 1"x4"x8", 300gram, 60, 12MB, 40bogomips, linux, mutt,
details at http://atrey.karlin.mff.cuni.cz/~pavel/velo/index.html.

Alan Cox

2002-03-15 14:30 UTC #23 lore raw

Quoted 3 lines

Quoted 1 line
s/CAP_SYS_RAWIO/CAP_DEVICE_CMD/ for the raw cmd ioctl interface. Have
Nobody uses capabilities these days, right?

Wrong. There are quite a few people using them, including large scale projects and actual sold packages. For other reasons CAP_DEVICE_CMD doesn't work out

Jeff Garzik

2002-03-15 11:08 UTC #24 lore raw

Quoted 7 lines · Pavel Machek

Hi
Quoted 2 lines
Under more restricted domains, root cannot bit-bang the interface. s/CAP_SYS_RAWIO/CAP_DEVICE_CMD/ for the raw cmd ioctl interface. Have
Nobody uses capabilities these days, right?

Actually, the NSA and HP secure linux products do, at the very least. And there is some ELF capabilities project out there IIRC, but I dunno if anybody's using it.

Quoted 6 lines

Quoted 2 lines
The filter is useful for other reasons like correctness, as well.
If you want to test if it works, you just disallow that access altogether. It is usually not needed , anyway.

The filter, or directly sending drive commands to userspace?

I agree the filter is of limited usefulness.

Sending drive commands directly from userspace is definitely -very- useful. As we start doing more and more stuff in userspace, I predict this facility will be used more and more. Plus, IBM particularly already has their Drive Fitness Tests, or whatever, sending direct drive commands. With the proper sequencing, you can even do power management of the drives in userspace. You don't want to do system suspend/resume that way, but you can certainly have a userspace policy daemon running, that powers-down and powers-up the drives, etc.

Jeff

Pavel Machek

2002-03-18 19:56 UTC #25 lore raw

Hi!

Quoted 9 lines

Quoted 5 lines
Quoted 3 lines
Under more restricted domains, root cannot bit-bang the interface. s/CAP_SYS_RAWIO/CAP_DEVICE_CMD/ for the raw cmd ioctl interface. Have
Nobody uses capabilities these days, right?
Actually, the NSA and HP secure linux products do, at the very least. And there is some ELF capabilities project out there IIRC, but I dunno if anybody's using it.

I did ELF capabilities ;-). And no, I do not think I had many users.

Quoted 4 lines

commands. With the proper sequencing, you can even do power management of the drives in userspace. You don't want to do system suspend/resume that way, but you can certainly have a userspace policy daemon running, that powers-down and powers-up the drives, etc.

See noflushd, Hdparm is able to powersave disks well, already, and it was in 2.2.X, too.
Pavel

Signature · 2 lines

(about SSSCA) "I don't say this lightly.  However, I really think that the U.S.
no longer is classifiable as a democracy, but rather as a plutocracy." --hpa

Vojtech Pavlik

author

2002-03-19 09:29 UTC #26 lore raw

Quoted 7 lines · Pavel Machek

Quoted 4 lines
commands. With the proper sequencing, you can even do power management of the drives in userspace. You don't want to do system suspend/resume that way, but you can certainly have a userspace policy daemon running, that powers-down and powers-up the drives, etc.
See noflushd, Hdparm is able to powersave disks well, already, and it was in 2.2.X, too.

Not all of them safely, though. Many a drive will corrupt data if it receives a command when not spinned up. You need to issue a wake command first, which hdparm doesn't, it just leaves it to the kernel to issue a read command or whatever to wake the drive ...

Signature · 2 lines

Vojtech Pavlik
SuSE Labs

Pavel Machek

2002-03-19 21:22 UTC #27 lore raw

Hi!

Quoted 5 lines

Quoted 5 lines
Quoted 4 lines
commands. With the proper sequencing, you can even do power management of the drives in userspace. You don't want to do system suspend/resume that way, but you can certainly have a userspace policy daemon running, that powers-down and powers-up the drives, etc.

Quoted 7 lines

Quoted 2 lines
See noflushd, Hdparm is able to powersave disks well, already, and it was in 2.2.X, too.
Not all of them safely, though. Many a drive will corrupt data if it receives a command when not spinned up. You need to issue a wake command first, which hdparm doesn't, it just leaves it to the kernel to issue a read command or whatever to wake the drive ...

Is this common disk bug, or are they permitted to behave like that?

Pavel

Signature · 2 lines

Casualities in World Trade Center: ~3k dead inside the building,
cryptography in U.S.A. and free speech in Czech Republic.

Andre Hedrick

2002-03-19 22:34 UTC #28 lore raw

Pavel,

No, it is called being a "BAD HOST". Things were added before the infrastructure was complete to support feature. Thus the feature is now a BUG. Sorry but that is the simple true, one of the reasons why I asked you to delay but it is not important now.

Regards,

Andre Hedrick
Linux Disk Certification Project Linux ATA Development

Quoted 27 lines · Pavel Machek

Hi!
Quoted 5 lines
Quoted 5 lines
Quoted 4 lines
commands. With the proper sequencing, you can even do power management of the drives in userspace. You don't want to do system suspend/resume that way, but you can certainly have a userspace policy daemon running, that powers-down and powers-up the drives, etc.
Quoted 7 lines
Quoted 2 lines
See noflushd, Hdparm is able to powersave disks well, already, and it was in 2.2.X, too.
Not all of them safely, though. Many a drive will corrupt data if it receives a command when not spinned up. You need to issue a wake command first, which hdparm doesn't, it just leaves it to the kernel to issue a read command or whatever to wake the drive ...
Is this common disk bug, or are they permitted to behave like that?
Pavel
Signature · 7 lines
Casualities in World Trade Center: ~3k dead inside the building,
cryptography in U.S.A. and free speech in Czech Republic.
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Alan Cox

2002-03-20 00:10 UTC #29 lore raw

In reply to Pavel Machek

Quoted 5 lines

Quoted 3 lines
receives a command when not spinned up. You need to issue a wake command first, which hdparm doesn't, it just leaves it to the kernel to issue a read command or whatever to wake the drive ...
Is this common disk bug, or are they permitted to behave like that?

Its not that common, but these sort of things happen sometimes. IDE is very much a defensive driver

Vojtech Pavlik

author

2002-03-19 21:56 UTC #30 lore raw

Quoted 10 lines · Pavel Machek

Hi!
Quoted 8 lines
Quoted 7 lines
Quoted 4 lines
commands. With the proper sequencing, you can even do power management of the drives in userspace. You don't want to do system suspend/resume that way, but you can certainly have a userspace policy daemon running, that powers-down and powers-up the drives, etc.
See noflushd, Hdparm is able to powersave disks well, already, and it was in 2.2.X, too.

Quoted 6 lines

Quoted 4 lines
Not all of them safely, though. Many a drive will corrupt data if it receives a command when not spinned up. You need to issue a wake command first, which hdparm doesn't, it just leaves it to the kernel to issue a read command or whatever to wake the drive ...
Is this common disk bug, or are they permitted to behave like that?

This behavior is permitted by the specification, as far as I know - results of commands other than wakeup (and other pm commands) in sleep or suspend mode are undefined ...

Signature · 2 lines

Vojtech Pavlik
SuSE Labs

Daniela Engert

2002-03-20 08:33 UTC #31 lore raw

Quoted 8 lines · Vojtech Pavlik

Quoted 6 lines
Quoted 4 lines
Not all of them safely, though. Many a drive will corrupt data if it receives a command when not spinned up. You need to issue a wake command first, which hdparm doesn't, it just leaves it to the kernel to issue a read command or whatever to wake the drive ...
Is this common disk bug, or are they permitted to behave like that?
This behavior is permitted by the specification, as far as I know -

Actually not. Have a look at page 36 of the current ATA6 specification.

Quoted 2 lines

results of commands other than wakeup (and other pm commands) in sleep or suspend mode are undefined ...

If a disk is in power state PM1:idle or PM2:standby, each ATA command which requires media access will result in a transition to power state PM0:active as well. The driver should be prepared of a long command execution time in this case (due to the spin up delay). This is why a well implemented ATA driver should track the individual power states of each attached unit to modulate its internal command timeout accordingly.

The driver may issue a SET_FEATURES(spin up) command in anticipation of a media access. A "forgiving" implementation might issue this command at proper system state transitions as well .

If a disk has entered power state PM3:sleep, its interface is turned off! You no longer can issue any command except for a DEVICE RESET to this unit. A reset is required to initiate a state transition from PM3:sleep to PM2:standby (there are no other state transitions).

Ciao,
Dani

~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
Daniela Engert, systems engineer at MEDAV GmbH
Gräfenberger Str. 34, 91080 Uttenreuth, Germany
Phone ++49-9131-583-348, Fax ++49-9131-583-11

Bill Davidsen

2002-03-20 18:13 UTC #32 lore raw

Quoted 3 lines · Daniela Engert

Quoted 1 line
This behavior is permitted by the specification, as far as I know -
Actually not. Have a look at page 36 of the current ATA6 specification.

That's probably not the place to look, since drives will conform to the specs in place when they were designed. If older specs did not require automatic spinup, then ATA6 doesn't apply.

In the real world I have seen drives which definitely didn't like commands in spindown, so unless there is some really large penalty for sending that I would suggest having the logic support the existing hardware. We still have 386 and FPE emulation, I would bet there are more older drives than 386s in use (at least by my clients ;-).

Signature · 3 lines

bill davidsen <[email protected]>
  CTO, TMR Associates, Inc
Doing interesting things with little computers since 1979.

Daniela Engert

2002-03-20 18:46 UTC #33 lore raw

Quoted 9 lines · Bill Davidsen

Quoted 3 lines · Daniela Engert
Quoted 1 line
This behavior is permitted by the specification, as far as I know -
Actually not. Have a look at page 36 of the current ATA6 specification.
That's probably not the place to look, since drives will conform to the specs in place when they were designed. If older specs did not require automatic spinup, then ATA6 doesn't apply.

Well, then have a look at page 31 of the ATA3 spec. It tells you basically the same - just a little more terse ;-)

Ciao,
Dani

Pavel Machek

2002-03-20 22:15 UTC #34 lore raw

Hi!

Quoted 11 lines

Quoted 8 lines
Quoted 6 lines
Quoted 4 lines
Not all of them safely, though. Many a drive will corrupt data if it receives a command when not spinned up. You need to issue a wake command first, which hdparm doesn't, it just leaves it to the kernel to issue a read command or whatever to wake the drive ...
Is this common disk bug, or are they permitted to behave like that?
This behavior is permitted by the specification, as far as I know -
Actually not. Have a look at page 36 of the current ATA6 specification.

Good. So noflushd is safe.
Pavel

Quoted 4 lines

If a disk has entered power state PM3:sleep, its interface is turned off! You no longer can issue any command except for a DEVICE RESET to this unit. A reset is required to initiate a state transition from PM3:sleep to PM2:standby (there are no other state transitions).

I do not think we are using PM3:sleep in noflushd, but we even could. AFAICT, ide layer resets interface if it does not respond.

Signature · 2 lines

Casualities in World Trade Center: ~3k dead inside the building,
cryptography in U.S.A. and free speech in Czech Republic.

Daniel Kobras

2002-03-20 23:09 UTC #35 lore raw

Quoted 2 lines · Pavel Machek

I do not think we are using PM3:sleep in noflushd, but we even could. AFAICT, ide layer resets interface if it does not respond.

It does, but only after a rather longish timeout, and a couple of IDE errors in the syslog. Nothing I'd like to scare users with, so noflushd issues standby (PM2).

Regards,

Daniel.

J. Dow

2002-03-12 02:51 UTC #36 lore raw

From: "Jeff Garzik" <[email protected]>

Quoted 4 lines

Quoted 2 lines · Linus Torvalds
On Mon, 11 Mar 2002, Jeff Garzik wrote:

...

Quoted 4 lines

First, for verification of the driver, it's darned useful to be able to issue ATA commands from userspace. But since we're being generic, let's just call them device commands, and imagine SCSI perhaps playing in this playground too.

No fooling! (By way of history I was one of the SCSI device driver mavens for the Amiga. We covered this generic command ground, recovered this ground, and generally ground this ground into the ground.)

Quoted 2 lines

Second, if you are issuing device commands from userspace, you need to deal with synchronization with the commands being issued from kernel space.

Generally speaking, yes.

Quoted 2 lines

Third, if you have synchronization, that's a good and easy point to add -optional- filtering.

And while I see your point I still ask, "But why?" To make this work the way it should work you will need a special table of normally disallowed commands that are allowed for each specific device on the IDE buses within a machine. You also need this to be generic enough to allow some commands to go through except for some specific sets of parameters. Now, I realize that SCSI is more generic than IDE. However, contemplating this sort of filtering within SCSI device drivers fell into such a point of silliness that it became the general concensus within the community that the filtering is inappropriate. In point of fact this proved useful when technology marched on. The facility within SCSI device drivers for directly issueing low level commands became instantly important with the advent of drives that were larger than 2^24 bytes. The normal read/write interface on the device drivers was byte offset and byte count oriented, even though it required the byte offsets to be on block boundaries. The direct SCSI command interface allowed for workaround patches that broke the disk into several virtual disks. These patches would open the low level driver and insert themselves as the "real" device driver for the set of devices on each SCSI bus. This died away fairly quickly as filesystem adaptations became more popular. These adaptations, in the case finally adopted within the latest OS revisions, involved an institutionalized driver patch that presented a proper 64 bit read/write interface to the filesystems. The fact that the device drivers that are no longer under any sort of development are patched using direct SCSI commands is neatly hidden from the users. But it is there and allowed the OS to survive where there was no further maintenance effort taking place.

I note there seem to be more than one device driver for Linux that is no longer maintained or poorly maintained. While I doubt this will be the case for generic IDE, I do suspect that specific IDE interfaces that have "peculiarities" will fall by the wayside as fewer people use them. It might be nice to have the ability to patch around these drivers rather than try to patch then with no test cases handy. It saved a significant amount of grief on the Amiga. There's no reason I can see that it would not save grief on Linux.

Quoted 10 lines

Now... the overall point I am trying to sell is... IFF certain lesser-used device commands are primarily issued from userspace, and IFF it is not reasonable to create a new kernel interface, is it not reasonable to be able to issue raw device commands from userspace? Just the verification case alone seems like a strong argument.
Call it device_command not ata_command if you want, and add SCSI support. Passing in raw device_commands from userspace is useful, IMO.
Throw in filtering of those commands, and life is golden for everybody
AFAICS. There is no need to be ATA specific with these goals...

It might be a good exercise, Jeff, to define the filters in such a way that potentially harmful commands can be adequately filters while other potentially "saving" commands can be allowed even if they differ only in parameters. It is also interesting to note that direct userland ATA and ATAPI reads and writes are fully as dangerous as any exotic level commands. Linux seems to allow this danger. What makes the other commands so dangerous? I'm not as familiar with the Linux SCSI interfaces as with those of the Amiga. (I've sort of moved on to video work.) However, I wonder if the Linux SCSI world filters "mode select" and manufacturer defined commands or not. I suspect they have a longer history with this sort of interface and its abuse than IDE. It might pay to investigate and adopt consonant postures.

{^_^} Joanne Dow, [email protected], did Amiga SCSI device drivers for
a decade or so as a consultant.

Jeff Garzik

2002-03-12 03:11 UTC #37 lore raw

Quoted 6 lines · J. Dow

From: "Jeff Garzik" <[email protected]>
Quoted 2 lines
Second, if you are issuing device commands from userspace, you need to deal with synchronization with the commands being issued from kernel space.
Generally speaking, yes.

cool

Quoted 8 lines

Quoted 3 lines
Third, if you have synchronization, that's a good and easy point to add -optional- filtering.
And while I see your point I still ask, "But why?" To make this work the way it should work you will need a special table of normally disallowed commands that are allowed for each specific device on the IDE buses within a machine.

[...]

Quoted 5 lines

It might be a good exercise, Jeff, to define the filters in such a way that potentially harmful commands can be adequately filters while other potentially "saving" commands can be allowed even if they differ only in parameters. It is also interesting to note that direct userland ATA and

I'm afraid I may have been confusing to the casual reader, because the current thread of discussion mutated from talking about (among other things) implementation details of the existing ATA raw command "filter" and the existing interface for issuing raw ATA commands.

You can, certainly, make a filter that addresses the issues you list. The existing filter is mainly a correctness filter -- it ensures that only known and standard ATA commands are passed down to the actual devices. It filters out vendor-specific commands as well as potentially nefarious future commands like COPYPROTECT or somesuch.

So, to summarize my points on the thread so far (as modified by Linus's responses to me):

1) There should be a raw device command interface (not ATA or SCSI specific)
2) There should be kernel interfaces for the standard cases, so that the raw device command interface is often not needed
3) There should be capability to optionally install filter the raw device command interface. The filter is built into the kernel at compile-time, but can also be disabled at boot time.

Jeff

Linus Torvalds

2002-03-12 03:29 UTC #38 lore raw

Quoted 2 lines · Jeff Garzik

1) There should be a raw device command interface (not ATA or SCSI specific)

Well, since the raw commands themselves will be bus-specific, you can't really avoid THAT part.

The thing I want to be generic is the fact that everybody uses the "request->cmd[]" array, along with the flags in "request->flags" to specify what kind of command it is.

We actually have this infrastructure already, it's just not fully used.

Quoted 2 lines

2) There should be kernel interfaces for the standard cases, so that the raw device command interface is often not needed

Absolutely.

Quoted 3 lines

3) There should be capability to optionally install filter the raw device command interface. The filter is built into the kernel at compile-time, but can also be disabled at boot time.

This is the part I really don't like.

Thinking like a sysadmin, I want to be able to run programs that I would not allow my users to run or want to be run accidentally. And I do _not_ want to reboot my kernel just because one of my mirrored disks died, I hot-replaced it, and I notice that I need to upgrade the firmware on the thing to make it play nice with the other disks in the array.

See? A setup that either allows everything or nothing is fundamentally flawed in this kind of situation - suddenly I as a sysadmin cannot do something without bringing the machine down. Which makes all the hotplug interfaces useless - or then I as a sysadmin just have to leave a kernel in place that allows the kinds of raw command accesses that I am so scared of.

One solution may be to have the whole raw cmd thing as a loadable module, and then I can make sure that it's not even available on the system so that I have to do some work to find it, and somebody elses program won't just know what to do.

But in that case is should be far removed from the IDE driver - it would just be a module that inserts a raw request on the request queue, and NOT inside some subsystem driver that I obviously want to have available all the time.

See?

Linus

Jeff Garzik

2002-03-12 03:47 UTC #39 lore raw

Quoted 5 lines · Linus Torvalds

Quoted 2 lines · Jeff Garzik
1) There should be a raw device command interface (not ATA or SCSI specific)

Quoted 7 lines

Well, since the raw commands themselves will be bus-specific, you can't really avoid THAT part.
The thing I want to be generic is the fact that everybody uses the "request->cmd[]" array, along with the flags in "request->flags" to specify what kind of command it is.
We actually have this infrastructure already, it's just not fully used.

I omitted "for userspace" :)

I was referring to having a userspace conduit for submitting requests, a la the taskfile ioctl now.

Quoted 7 lines

Quoted 2 lines
3) There should be capability to optionally install filter the raw device command interface. The filter is built into the kernel at compile-time, but can also be disabled at boot time.
This is the part I really don't like.
Thinking like a sysadmin, I want to be able to run programs that I would not allow my users to run or want to be run accidentally. And I do _not_ want to reboot my kernel just because one of my mirrored disks died, I hot-replaced it, and I notice that I need to upgrade the firmware on the thing to make it play nice with the other disks in the array.

Quoted 9 lines

See? A setup that either allows everything or nothing is fundamentally
flawed in this kind of situation - suddenly I as a sysadmin cannot do
something without bringing the machine down. Which makes all the hotplug
interfaces useless - or then I as a sysadmin just have to leave a kernel in place that allows the kinds of raw command accesses that I am so scared of.
One solution may be to have the whole raw cmd thing as a loadable module, and then I can make sure that it's not even available on the system so that I have to do some work to find it, and somebody elses program won't just know what to do.
But in that case is should be far removed from the IDE driver - it would just be a module that inserts a raw request on the request queue, and NOT inside some subsystem driver that I obviously want to have available all the time.

I like this solution, it was the one I was thinking of :)

The entire userspace raw cmd ioctl should be a separate module for precisely the issues you outlined. If they choose, people can compile that module into the static kernel image, including filter. Or they can use the module without the filter. Or they can not use the module at all. Etc.

I do understand the problems stemming from the inflexibility of the filter that's being discussed, but I disagree that it's a fundamental flaw. It's an option that some people -shouldn't- choose, for various reasons. But that's an issue for documentation and system integrators.

Jeff

Linus Torvalds

2002-03-12 04:00 UTC #40 lore raw

Quoted 10 lines · Linus Torvalds

One solution may be to have the whole raw cmd thing as a loadable module, and then I can make sure that it's not even available on the system so that I have to do some work to find it, and somebody elses program won't just know what to do.
But in that case is should be far removed from the IDE driver - it would just be a module that inserts a raw request on the request queue, and NOT inside some subsystem driver that I obviously want to have available all the time.

Let me put this proposal in more specific terms and see who hollers..

First, the actual assumptions:
- we should use the request queue, simply because that is the only thing that serializes access to all controllers - if we do not have any "sideband", there is no way to create the kind of confusion that we can create right now.

- we want the approach to be generic, even if the details will end up being IDE/SCSI/xxx-specific.

- I personally believe that we want to be able to do filtering independently of the controller driver, ie the filtering is not part of the driver infrastructure at all, but at a higher level (ie the same way network filtering has _nothing_ to do with any actual network drivers, regardless of what bus those drivers are on)

Thus I would suggest against a filter inside the IDE driver, and instead suggest a loadable module that does

- attach to one or more request queue(s). Notice that you should not have _one_ module that handles all request queues, because the filter module obviously has to be different for an ATA disk than for a SCSI disk, and in fact it might be different for an IBM ATA disk than for a Maxtor ATA disk, for example.

- the module basically acts the way the SCSI generic driver does right now, except it acts on a higher level: instead of generating SCSI requests and feeding them directly to the driver with a scsi_do_req(), it would generate the command requests and feed it to the request queue.

In fact, don't think of it as the ATA thing at all: I'm more thinking along the lines of splitting up "sg.c" into the highlevel command generator (the biggest part) and a very _small_ part in the scsi request loop that understands about the generic command interface.

If we can really do what sg.c does now, while at the _same_ time also have a "ide-generic" module that uses the exact same infrastructure, then I think I'm happy. Yes, the filtering is bus-specific (because the commands are bus-specific), but the general approach is common.

Does anybody find any real downsides to this approach or basically trying to abstract sg.c "upwards" a bit?

Linus

Jeff Garzik

2002-03-12 04:27 UTC #41 lore raw

Your proposal sounds 100% ok to me...

For the details of the userspace interface (for both ATA and SCSI), my idea was to use standard read(2) and write(2).

Any number of programs can open /dev/ata/hda/control or /dev/scsi/sdc/control. write(2) submits requests, read(2) consumes command responses, perhaps buffering a bit so that multiple responses are not lost if userspace is slow.

Maybe it's a cheesy way to avoid ioctl(2), maybe not...

Jeff

Linus Torvalds

2002-03-12 04:51 UTC #42 lore raw

Quoted 3 lines · Jeff Garzik

For the details of the userspace interface (for both ATA and SCSI), my idea was to use standard read(2) and write(2).

I like it, but you have to realize that most, if not all, of the commands really are not a matter of a read or a write, but a "transaction", which is a pair of both (and at least in theory you could have transactions that are more complex than just a "command + result", ie more than just a write("cmd + outdata")+read("status + indata")).

I agree 100% with the notion of using read/write to do this, but I think you need to make it very explicit that we _are_ talking about transactions, and that the file descriptor would act as a "transaction descriptor" when you do this. The file descriptor is the one that matches up the write that started the transaction with the read that gets the status of it - thus allowing multiple concurrent transactions in flight.

And the command has to have some structure, ie it needs to not just be the low-level command, but also have the information about the "format" of the transaction (so that the generic layer can build up the correct BIO data structures for the result, before it actually sees the read itself).

Linus

Linus Torvalds

2002-03-12 04:33 UTC #43 lore raw

Quoted 6 lines · Linus Torvalds

- attach to one or more request queue(s). Notice that you should not have _one_ module that handles all request queues, because the filter module obviously has to be different for an ATA disk than for a SCSI disk, and in fact it might be different for an IBM ATA disk than for a Maxtor ATA disk, for example.

Btw, to tie this back to the other IDE thread, namely the suspend/resume thing, I think things like that should also just push commands down the request list. In particular, instead of waiting until the handler is NULL, it should do something like

- create a "sync" request
- do the equivalent of

	DECLARE_COMPLETION(wait);
	rq->waiting = &wait;
	q->elevator.elevator_add_req_fn(q, rq, queue_head);
	wait_for_completion(&wait);

which automatically synchronizes with any outstanding requests (simply by virtue of the elevator knowing not to re-order/merge special requests, so when the sync command in finished, we know all other commands have finished too).

Note that this should be the same code as for a shutdown as well - we should finish with a flush buffers request and wait for it to have completed.

I'd like a _lot_ of stuff to stop using "do_xxx_command()", and move toa higher layer so that more code can be shared between different subsystems (all of these "sync" issues are really completely generic, and should _not_ be duplicated across drivers or subsystems).

Linus

Jeff Garzik

2002-03-12 05:06 UTC #44 lore raw

Quoted 14 lines · Linus Torvalds

Quoted 6 lines · Linus Torvalds
- attach to one or more request queue(s). Notice that you should not have _one_ module that handles all request queues, because the filter module obviously has to be different for an ATA disk than for a SCSI disk, and in fact it might be different for an IBM ATA disk than for a Maxtor ATA disk, for example.
Btw, to tie this back to the other IDE thread, namely the suspend/resume thing, I think things like that should also just push commands down the request list. In particular, instead of waiting until the handler is NULL, it should do something like
- create a "sync" request
- do the equivalent of

Quoted 7 lines

	DECLARE_COMPLETION(wait);
	rq->waiting = &wait;
	q->elevator.elevator_add_req_fn(q, rq, queue_head);
	wait_for_completion(&wait);
which automatically synchronizes with any outstanding requests (simply by virtue of the elevator knowing not to re-order/merge special requests, so when the sync command in finished, we know all other commands have finished too).

Dumb question, why create a separate request?

Why not just have some way to wait for request X (and flag it for no-merge/barrier treatment, etc.)? bios have end_io callbacks...

Jeff

Linus Torvalds

2002-03-12 05:21 UTC #45 lore raw

Quoted 2 lines · Jeff Garzik

Dumb question, why create a separate request?

Because you need to anyway. Things like shutdown/suspend need to sync the caches, and that's a command that needs to go down the pipe to the disk.

Quoted 2 lines

Why not just have some way to wait for request X (and flag it for no-merge/barrier treatment, etc.)? bios have end_io callbacks...

The bio's are just fragment descriptors, they don't really stand on their own. A bio needs a request in order to move down to the driver.

The request is the place where you find the actual command - the bio just contains the fragment data of the command.

Of course, the "just" is a big simplification. Since a block command can be a chain of hundreds of blocks which each actually have a lifetime of their own, unlike the network later, the fragments are a lot more complicated than a "skb_frag_t".

So bio's are complex entities in themselves, and they have a life of their own. It's just that you cannot send a raw bio to a device - the device wouldn't know what to do with it.

Linus

Martin Dalecki

2002-03-12 11:41 UTC #46 lore raw

Quoted 19 lines · Linus Torvalds

Quoted 6 lines · Linus Torvalds
- attach to one or more request queue(s). Notice that you should not have _one_ module that handles all request queues, because the filter module obviously has to be different for an ATA disk than for a SCSI disk, and in fact it might be different for an IBM ATA disk than for a Maxtor ATA disk, for example.
Btw, to tie this back to the other IDE thread, namely the suspend/resume thing, I think things like that should also just push commands down the request list. In particular, instead of waiting until the handler is NULL, it should do something like
- create a "sync" request
- do the equivalent of
	DECLARE_COMPLETION(wait);
	rq->waiting = &wait;
	q->elevator.elevator_add_req_fn(q, rq, queue_head);
	wait_for_completion(&wait);

Quoted 8 lines

which automatically synchronizes with any outstanding requests (simply by virtue of the elevator knowing not to re-order/merge special requests, so when the sync command in finished, we know all other commands have finished too).
Note that this should be the same code as for a shutdown as well - we should finish with a flush buffers request and wait for it to have completed.
I'd like a _lot_ of stuff to stop using "do_xxx_command()", and move toa higher layer so that more code can be shared between different subsystems (all of these "sync" issues are really completely generic, and should _not_ be duplicated across drivers or subsystems).

"Amen Brother" to this. Getting device id strings and others are in the same gang.

BTW.> Would you dare to care to splow out a 2.5.7-pre1 just to allow synchronization without bitkeeper?

J. Dow

2002-03-12 06:27 UTC #47 lore raw

From: "Jeff Garzik" <[email protected]>

Quoted 11 lines

Your proposal sounds 100% ok to me...
For the details of the userspace interface (for both ATA and SCSI), my idea was to use standard read(2) and write(2).
Any number of programs can open /dev/ata/hda/control or /dev/scsi/sdc/control. write(2) submits requests, read(2) consumes command responses, perhaps buffering a bit so that multiple responses are not lost if userspace is slow.
Maybe it's a cheesy way to avoid ioctl(2), maybe not...

Jeff, from a security aspect would it perhaps be better to have the filter always in place and load rule sets through a rigidly controlled interface? This gives a control hook for non-Unixoid security model control over the interface filtering. The filter module would have the lower level interfaces all opened exclusively so there would be no paths around the filter. I propose that the rule sets, for each device's instance of the filter interface, could be changed to include anything from a null set to forbidding anything past fully controlled read and write with no raw IO. One specific entity could be allowed to make the changes and no others. This gives a single interface for verifying signatures on filter data sets, as well.

{^_^}

Martin Dalecki

2002-03-12 11:38 UTC #48 lore raw

Quoted 7 lines · Linus Torvalds

Quoted 4 lines · Linus Torvalds
One solution may be to have the whole raw cmd thing as a loadable module, and then I can make sure that it's not even available on the system so that I have to do some work to find it, and somebody elses program won't just know what to do.
But in that case is should be far removed from the IDE driver - it would just be a module that inserts a raw request on the request queue, and NOT inside some subsystem driver that I obviously want to have available all the time.

Quoted 7 lines

Let me put this proposal in more specific terms and see who hollers..
First, the actual assumptions:
- we should use the request queue, simply because that is the only thing that serializes access to all controllers - if we do not have any "sideband", there is no way to create the kind of confusion that we can create right now.

Amen to this.

Quoted 2 lines

- we want the approach to be generic, even if the details will end up being IDE/SCSI/xxx-specific.

Could be done but seems to be very optimistic about the actual detail. However stuff like flush/power up/power down/operate silent/operate fast should be indeed generic.

Quoted 20 lines

- I personally believe that we want to be able to do filtering independently of the controller driver, ie the filtering is not part of the driver infrastructure at all, but at a higher level (ie the same way network filtering has _nothing_ to do with any actual network drivers, regardless of what bus those drivers are on)
Thus I would suggest against a filter inside the IDE driver, and instead suggest a loadable module that does
- attach to one or more request queue(s). Notice that you should not have _one_ module that handles all request queues, because the filter module obviously has to be different for an ATA disk than for a SCSI disk, and in fact it might be different for an IBM ATA disk than for a Maxtor ATA disk, for example.
- the module basically acts the way the SCSI generic driver does right now, except it acts on a higher level: instead of generating SCSI requests and feeding them directly to the driver with a scsi_do_req(), it would generate the command requests and feed it to the request queue.

Quoted 4 lines

In fact, don't think of it as the ATA thing at all: I'm more thinking along the lines of splitting up "sg.c" into the highlevel command generator (the biggest part) and a very _small_ part in the scsi request loop that understands about the generic command interface.

The last part could make the few above possible. But the layering problems in ide are currently much preventig the most trivial kind of validation. See for example problems in asynchornous commands, which turned out recently.

Martin Dalecki

2002-03-12 11:45 UTC #49 lore raw

Quoted 6 lines · J. Dow

From: "Jeff Garzik" <[email protected]>
Quoted 4 lines
Your proposal sounds 100% ok to me...
For the details of the userspace interface (for both ATA and SCSI), my idea was to use standard read(2) and write(2).

Quoted 8 lines

Quoted 4 lines
Any number of programs can open /dev/ata/hda/control or /dev/scsi/sdc/control. write(2) submits requests, read(2) consumes command responses, perhaps buffering a bit so that multiple responses are not lost if userspace is slow.
Maybe it's a cheesy way to avoid ioctl(2), maybe not...
Jeff, from a security aspect would it perhaps be better to have the filter always in place and load rule sets through a rigidly controlled interface?

You are overdesigning by a broad margin. From a security point of view (I mean the paranoid one) the whole raw interface whatever filtered or not should *just not be there*.

Erik Andersen

2002-03-12 04:50 UTC #50 lore raw

Quoted 7 lines · Linus Torvalds

If we can really do what sg.c does now, while at the _same_ time also have a "ide-generic" module that uses the exact same infrastructure, then I think I'm happy. Yes, the filtering is bus-specific (because the commands are bus-specific), but the general approach is common.
Does anybody find any real downsides to this approach or basically trying to abstract sg.c "upwards" a bit?

Essentially, if I understand what you are saying, you are looking for a uniform low-level mass-storage layer that does all the normal low-level drive access stuff. Presumably, if done correctly, this would act as a bus-abstracting foundation upon which the block layer could be built...

-Erik

--
Erik B. Andersen http://codepoet-consulting.com/
--This message was written using 73% post-consumer electrons--

Linus Torvalds

2002-03-12 05:09 UTC #51 lore raw

Quoted 4 lines · Erik Andersen

Essentially, if I understand what you are saying, you are looking for a uniform low-level mass-storage layer that does all the normal low-level drive access stuff.

Well, yes and no.

It's not really low-level - it's at the same level as the current "ll_rw_block()" in that it doesn't per se know any real details of any drivers - it is nothing but a "build this request" layer (with a way of getting the data back, of course).

So from a driver perspective (or even driver subsystem like IDE or SCSI) it's actually a high-level request generator.

Now, let me say outright that I have a very specific reason why I'd like this thing to be done at this level - which really has nothing to do with Jeff's wishes to create a IDE command parser.

The reason I want to see stuff done _above_ the level of the request queue is that I really count on the "struct request" as being the abstraction layer that the current block driver interfaces so sadly lack.

Right now, block drivers are not very well abstracted at all: and this is a large part of the reason for why we even _have_ things like the SCSI midlevel layer or the IDE layer in the first place. But both the SCSI layer and the IDE layer are just fairly messy - with no good way to share code (no common interfaces) and with very inflexible setups (just look at how hard it is to do a high-performance SCSI driver, because if you do a SCSI driver at all you have to do all the interface crap that comes with the job).

I'm really hoping that some day, if the _only_ interface to the block driver is "struct request", you can write truly interchangable block drivers - the same way you can write truly independent network drivers today. None of this "IDE vs SCSI midlevel layer" crap.

Yes, "struct request" is slightly more complex than just being handed a packet to send out, but I think that is unavoidable - block devices just _are_ more complex than sending out a packet on a network. So while it's not a really simple interface, at least it is a clear border between drivers. And a SCSI driver would be nothing more than a driver that accepts at least a subset of the kinds of requests that some scsi module can push to it.

None of this complex function call interface with common complex structures that are imposed by hundreds of different drivers that each have slightly different issues. Just a queue.

"Everything is a stream of bytes" - except in this case the stream just happens to have enough structure that you can re-order the packets and try to sort them while they haven't been committed yet.

Linus

J. Dow

2002-03-12 06:05 UTC #52 lore raw

From: "Linus Torvalds" <[email protected]>

Quoted 12 lines

Quoted 3 lines
3) There should be capability to optionally install filter the raw device command interface. The filter is built into the kernel at compile-time, but can also be disabled at boot time.
This is the part I really don't like.
Thinking like a sysadmin, I want to be able to run programs that I would not allow my users to run or want to be run accidentally. And I do _not_ want to reboot my kernel just because one of my mirrored disks died, I hot-replaced it, and I notice that I need to upgrade the firmware on the thing to make it play nice with the other disks in the array.

Quoted 6 lines

See? A setup that either allows everything or nothing is fundamentally flawed in this kind of situation - suddenly I as a sysadmin cannot do something without bringing the machine down. Which makes all the hotplug interfaces useless - or then I as a sysadmin just have to leave a kernel in place that allows the kinds of raw command accesses that I am so scared of.

Good example. (I even implemented something of that sort for downloading new firmware for Maxtor SCSI drives back when the 1G Panther was a brandy spanky new thing. I never distributed it and have no plans of doing so. Amigans as a rule are not good candidates for having such code on hand. The average Linux SysAdmin should be a few dB smarter and more careful. I don't think it would be a good thing to force multiple reboots in the situation you cite. And it does happen.)

{^_^} Joanne Dow

J. Dow

2002-03-12 06:11 UTC #53 lore raw

From: "Jeff Garzik" <[email protected]>

Quoted 1 line

Linus Torvalds wrote:

...

Quoted 11 lines

Quoted 10 lines
One solution may be to have the whole raw cmd thing as a loadable module, and then I can make sure that it's not even available on the system so that I have to do some work to find it, and somebody elses program won't just know what to do.
But in that case is should be far removed from the IDE driver - it would just be a module that inserts a raw request on the request queue, and NOT inside some subsystem driver that I obviously want to have available all the time.
I like this solution, it was the one I was thinking of :)

It leaves me bemused. You speak of a module to install a raw userspace IO capability. If that module exists the module interface exists. Would it have to be the module compiled into the kernel that gets run on that interface? It looks as wide open as ever, to me.

Quoted 5 lines

The entire userspace raw cmd ioctl should be a separate module for precisely the issues you outlined. If they choose, people can compile that module into the static kernel image, including filter. Or they can use the module without the filter. Or they can not use the module at all. Etc.

Of course, this seems to be one of those compromises between high availability and high security. It should be made clear that this is the compromise involved when the raw io filter is compiled in or not.

I'm not sure you can build an unfiltered raw IO capability into the kernel SCSI, IDE, or anything else and maintain system security.

{^_^} Joanne

Erik Andersen

2002-03-12 04:41 UTC #54 lore raw

Quoted 1 line · Jeff Garzik

1) There should be a raw device command interface (not ATA or SCSI specific)

Hmm. If such a generic low-level raw device layer were to be implemented (presumably as the foundation for the block layer), I expect the interface would be somthing like the cdrom layer, and would abstract out all the normal things that raw mass-storage devices can do.

But the minute such a layer is in place, people will begin going straight to the sub-low-level raw device layers so they can use all the exciting new extended features of their XP370000 quantum storage array which needs the special frob-electrons command to make it work...

-Erik

--
Erik B. Andersen http://codepoet-consulting.com/
--This message was written using 73% post-consumer electrons--

Bill Davidsen

2002-03-12 16:38 UTC #55 lore raw

Quoted 9 lines · Erik Andersen

Quoted 1 line · Jeff Garzik
1) There should be a raw device command interface (not ATA or SCSI specific)
Hmm. If such a generic low-level raw device layer were to be implemented (presumably as the foundation for the block layer), I expect the interface would be somthing like the cdrom layer, and would abstract out all the normal things that raw mass-storage devices can do.

Quoted 5 lines

But the minute such a layer is in place, people will begin going straight to the sub-low-level raw device layers so they can use all the exciting new extended features of their XP370000 quantum storage array which needs the special frob-electrons command to make it work...

Given that this has not happened with sg, nor with people using the sg+ide-scsi, I think you would have to provide a good reason why this would happen. The most likely path would be a separate driver or enhanced IDE driver patch. User programs generally do read, write and seek, not ioctl level hacking.

Signature · 3 lines

bill davidsen <[email protected]>
  CTO, TMR Associates, Inc
Doing interesting things with little computers since 1979.

Jeff Garzik

2002-03-12 04:49 UTC #56 lore raw

Quoted 6 lines · Erik Andersen

But the minute such a layer is in place, people will begin going straight to the sub-low-level raw device layers so they can use all the exciting new extended features of their XP370000 quantum storage array which needs the special frob-electrons command to make it work...

SCSI generic has existed for a while now :)

So this is really just catching up. And WRT the filtering stuff, people are free to use the raw cmd without any filtering at all. Your choice.

If you mean bit-banging, see my reply to Oliver (Olivier?).

Anyway, Linus's current proposal seems both sane and flexible enough. And the basic infrastructure already exists to shove raw commands onto the request queue.

Jeff

J. Dow

2002-03-12 06:30 UTC #57 lore raw

From: "Erik Andersen" <[email protected]>

Quoted 9 lines · Jeff Garzik

Quoted 1 line
1) There should be a raw device command interface (not ATA or SCSI specific)
Hmm. If such a generic low-level raw device layer were to be implemented (presumably as the foundation for the block layer), I expect the interface would be somthing like the cdrom layer, and would abstract out all the normal things that raw mass-storage devices can do.

Quoted 5 lines

But the minute such a layer is in place, people will begin going straight to the sub-low-level raw device layers so they can use all the exciting new extended features of their XP370000 quantum storage array which needs the special frob-electrons command to make it work...

Erik, that is precisely the point on which the Amiga discussions of the Direct SCSI command capability in scsi device drivers fell down. (Due to historical oddities even IDE device drivers feature the direct SCSI interface via emulation. It seems like an ultimate horror. However it has enabled some marvelously generic tools.)

{^_^}

J. Dow

2002-03-12 06:30 UTC #58 lore raw

From: "Jeff Garzik" <[email protected]>

Quoted 1 line

SCSI generic has existed for a while now :)

Now we get to the point. What is the SCSI experience with the generic interface and filtering, if any?

{^_^}

Alan Cox

2002-03-12 02:45 UTC #59 lore raw

Quoted 1 line

Are such random vendor-specific commands really that common?

Not very

Quoted 4 lines

Linus, would it be acceptable to you to include an -optional- filter for ATA commands? There is definitely a segment of users that would like to firewall their devices, and I think (as crazy as it may sound) that notion is a valid one.

Jeff -I like the idea of the filters - but if the ATA command raw stuff is CAP_SYS_RAWIO then its the same right set as inb/outb. Beyond that its a job for the NSA and RSBAC stuff ?

Jeff Garzik

2002-03-12 02:49 UTC #60 lore raw

Quoted 7 lines · Alan Cox

Quoted 2 lines
Linus, would it be acceptable to you to include an -optional- filter for ATA commands? There is definitely a segment of users that would like to firewall their devices, and I think (as crazy as it may sound) that notion is a valid one.
Jeff -I like the idea of the filters - but if the ATA command raw stuff is CAP_SYS_RAWIO then its the same right set as inb/outb. Beyond that its a job for the NSA and RSBAC stuff ?

Yeah, you can still bit-bang with the current implementation, on that capability. Couldn't that be cured with s/CAP_SYS_RAWIO/some new CAP_DEVICE_CMD/ for the raw device command interface?

The current implementation needs to be changed anyway :) From "ATA raw command" to "device raw command" at the very least.

Jeff

Alan Cox

2002-03-12 11:06 UTC #61 lore raw

Quoted 3 lines

Yeah, you can still bit-bang with the current implementation, on that capability. Couldn't that be cured with s/CAP_SYS_RAWIO/some new CAP_DEVICE_CMD/ for the raw device command interface?

By uploading new firmware you can make the drive return compromised pages from swap which make an existing RAWIO application do what you want

- so CAP_DEVICE_CMD in the IDE case at least is CAP_SYS_RAWIO

Quoted 2 lines

The current implementation needs to be changed anyway :) From "ATA raw command" to "device raw command" at the very least.

That assumes a totally generic interface is good - ATA is rather different to ATAPI/SCSI so it may be best the device interface reflects that.

bert hubert

2002-03-13 08:14 UTC #62 lore raw

ide filters / 'ide dump' / 'bio dump'

Quoted 2 lines · Jeff Garzik

The current implementation needs to be changed anyway :) From "ATA raw command" to "device raw command" at the very least.

Regarding proposed ATA filtering, this touches somewhat on something I've been thinking about for a while: biodump.

Basically, biodump is to a block device what tcpdump is to a network adaptor.

So we would be able to do this:

# biodump /dev/hda
09:09:33.023 READ block 12345 [10 blocks]
09:09:33.024 READ block 12355 [10 blocks]
09:09:33.025 READ block 12365 [10 blocks]
09:09:34.000 WRITE block 12345 [1 block]

Or somewhat more fancy, and only useful for non-growing files:

# biodump /var/db/bigdb/tablefile
file on /dev/hda, getting blockmap:
09:09:33.023 READ logical block 12345 [10 blocks]
09:09:33.024 READ logical block 12355 [10 blocks]
09:09:33.025 READ logical block 12365 [10 blocks]
09:09:34.000 WRITE logical block 12345 [1 block]

Because right now, we have no idea why that disk light is blinking. If somebody implements a filter, it would be a natural place to also provide a dumping hook.

biodump might in fact turn out to become atadump and scsidump (which is rather sad actually).

regards,

bert

Signature · 3 lines

http://www.PowerDNS.com          Versatile DNS Software & Services
http://www.tk                              the dot in .tk
http://lartc.org           Linux Advanced Routing & Traffic Control HOWTO

Jeff Garzik

2002-03-13 10:11 UTC #63 lore raw

Re: ide filters / 'ide dump' / 'bio dump'

Quoted 6 lines · bert hubert

# biodump /dev/hda
09:09:33.023 READ block 12345 [10 blocks]
09:09:33.024 READ block 12355 [10 blocks]
09:09:33.025 READ block 12365 [10 blocks]
09:09:34.000 WRITE block 12345 [1 block]

Definitely an interesting idea... With this new stuff Linus talked about in his proposal and what I'm thinking about, it shouldn't be too hard to do.

Jeff

Linus Torvalds

2002-03-13 17:31 UTC #64 lore raw

Re: ide filters / 'ide dump' / 'bio dump'

Quoted 11 lines · Jeff Garzik

Quoted 5 lines · bert hubert
# biodump /dev/hda
09:09:33.023 READ block 12345 [10 blocks]
09:09:33.024 READ block 12355 [10 blocks]
09:09:33.025 READ block 12365 [10 blocks]
09:09:34.000 WRITE block 12345 [1 block]
Definitely an interesting idea... With this new stuff Linus talked
about in his proposal and what I'm thinking about, it shouldn't be too hard to do.

Note that I actually think we're talking about two different things.

There's the notion of _feeding_ special requests onto the request queue through some interface that also can refuse to feed certain kinds of requests. That is needed for the special commands, and is "above" the requests queue layer.

That interface doesn't really support filtering of requests that other people (notably the regular kernel itself) is also feeding to the request queue.

Note that one of the big issues with the request queue is that it acts as a funnel: it (very much by design) can, and does, take requests from different places, and nobody needs to "own" the request queue. But the kind of "feed this raw request down" module that has been talked about would have absolutely _zero_ visibility into what others are feeding into the request queue.

If you actually want to filter other peoples requests, then you have to do something completely different, namely set up a request queue of your own, then exporting _your_ request queue as the request queue for <major,minor> and then taking the requests off that queue internally, and moving them to the original queue after you've done filtering.

Basically a simplified "loopback" thing that doesn't even need to do any remapping.

The two are totally independent, they work on requests queues at different levels (one feeds some random request queue, the other changes how we look up a request queue and inserts its own queue in between).

Linus

Malcolm Beattie

2002-03-13 12:03 UTC #65 lore raw

Re: ide filters / 'ide dump' / 'bio dump'

Quoted 10 lines · Jeff Garzik

Quoted 6 lines · bert hubert

# biodump /dev/hda
09:09:33.023 READ block 12345 [10 blocks]
09:09:33.024 READ block 12355 [10 blocks]
09:09:33.025 READ block 12365 [10 blocks]
09:09:34.000 WRITE block 12345 [1 block]

Quoted 3 lines

Definitely an interesting idea... With this new stuff Linus talked
about in his proposal and what I'm thinking about, it shouldn't be too hard to do.

I wrote a basic version of that (reqlog) a couple of years ago (with the writing of the rw/block/size info inline in the kernel rather than via a nice separate filter). The use I put it to was for migrating block devices live (i.e. while being read/written) without having to quiesce the readers/writers except for a few seconds at the end. The idea is that you set up (as an ordinary userland process) a bitmap in memory for all the blocks on the device. You start with them all marked dirty. You then do two loops concurrently (select() or threads):
while (1) {
read next block/size touched
mark corresponding bits dirty in bitmap
}
and
while (1) {
while (find_next_dirty_bit_in_bitmap()) {
mark bit clean
read corresponding block from device
send blocknumber, blockdata to remote peer
}
go back to start of bitmap
}

The remote peer is a daemon which just reads the (blocknumber,data) pairs and writes the data to the snapshot-to-be device at its end. Eventually, the bitmap gets mostly clean and the migrating process has "caught up" and is just siphoning newly dirtied data off to the remote end in real time. At that point, you quiesce the writing applications nicely for a few seconds (database, serving daemons or whatever is writing to the device) and let the migrator clean the bitmap fully (the last few blocks) and at that point you have a point-in-time safe snapshot at the remote end.

With filters available (presumably as modules) for the bio stuff, this will become possible without having to patch the kernel (I think I submitted the reqlog and bufflink patches for inclusion but didn't care enough to keep trying).

See
http://www.clueful.co.uk/mbeattie/linux-kernel.html#reqlog
and bmigrate and bufflink on the same page for the "old" way. It's basic stuff and new APIs mean a rewrite will be much easier but I thought you may be interested in another application of such logging.

--Malcolm

Signature · 4 lines

Malcolm Beattie <[email protected]>
Linux Technical Consultant
IBM EMEA Enterprise Server Group...
...from home, speaking only for myself

Martin Dalecki

2002-03-12 11:13 UTC #66 lore raw

Quoted 23 lines · Jeff Garzik

Quoted 6 lines · Linus Torvalds
The only common factor here is the "synchronize with other requests" - I feel strongly (much more strongly than any parsing notion) that the raw requests have to be passed down the "struct request" and NOT be done the way they are traditionally done (ie completely outside the request stream, with no synchronization at all with any IO currently in progress).
agreed
Quoted 13 lines
Quoted 5 lines
I think "future new commands" is total FUD, the idea that some new command would come along and be so instantly popular, useful and incompatible that all Linux boxes would require it before the next kernel or driver update is silly at best, and I'm working hard to keep this on a civil plane.
It has nothing to do with "new" commands, and everything to do with "random vendor-specific commands and the vendor-specific tools". Commands that simply should _never_ be parsed in the kernel, because we do not want to care about 10 different vendors 10 different revisions of their firmware having 10 different small random special commands for that particular drive.

Quoted 7 lines

Quoted 4 lines
In particular, a user that upgrades his hardware should never _ever_ have to upgrade his kernel just because some random disk diagnostic tool needs support for a disk that is new and has new diagnostics.
Are such random vendor-specific commands really that common?
Linus, would it be acceptable to you to include an -optional- filter for ATA commands? There is definitely a segment of users that would like to firewall their devices, and I think (as crazy as it may sound) that notion is a valid one.

If you are *trully paranoid* and want to *fire wall* your device then the proper way of doing this is to DISABLE those ioctl entierly. It simple like that. They are not required for regular operation by concept. Other then this I see no argument here.

[email protected]

2002-03-12 11:50 UTC #67 lore raw

Quoted 9 lines

Quoted 4 lines
Linus, would it be acceptable to you to include an -optional- filter for ATA commands? There is definitely a segment of users that would like to firewall their devices, and I think (as crazy as it may sound) that notion is a valid one.
If you are *trully paranoid* and want to *fire wall* your device then the proper way of doing this is to DISABLE those ioctl entierly. It simple like that. They are not required for regular operation by concept. Other then this I see no argument here.

Well; I don't agree. Firewall via a loadable module can make sense if the actual module does a kind of "filter all but a specified known set of safe commands" for example, like retreiving SMART infos, or such things.

Ben.

Bill Davidsen

2002-03-12 16:35 UTC #68 lore raw

Quoted 23 lines · Linus Torvalds

Quoted 7 lines · Bill Davidsen
I think you might want to offer an opinion (or edict, mandate, whatever) WRT taskfile. While I think that some protective editing of commands is desirable, useful, etc, I'm damn sure that some form of raw access is required long term to allow diagmostics. I wouldn't argue if that interface was required for low level format and other really drastic operations as well.
Quite frankly, my personal opinion is that there's no point in trying to parse the commands too much at all. The _only_ thing the kernel should try to care about is that the buffers passed to the command are valid (ie the actual data in/out area pointer should be a kernel buffer as far as possible, not under user control).
Basically, there are three "modes" of operation here:
- regular kernel access (ie the normal read/write stuff)
parsing: none. The kernel originated the request.

Quoted 8 lines

- common ioctl's that have nothing to do with IDE per se (ie all the stuff to open/close/lock removable media, unlocking DVD's, reading sound sectors etc - stuff that really exists elsewhere too, and where the kernel should do the translation from the generic problem space to the specific commands for the bus in quesion)
Parsing: common ioctl turning stuff into common SCSI commands in the "struct request".

This next part is the one I believe should be a compile/boot option, since it is the one which provides full access to the device.

Quoted 6 lines

- totally IDE-only stuff, where the kernel simply doesn't add any value, and only has a way of passing it down to the drive, and passing back the requests.
Parsing: none. The kernel simply doesn't have anything to do with the request, except to synchronize it with all other requests.

[observations on sync and queue issues understood and snipped]

Signature · 3 lines

bill davidsen <[email protected]>
  CTO, TMR Associates, Inc
Doing interesting things with little computers since 1979.

Gunther Mayer

2002-03-12 20:21 UTC #69 lore raw

Quoted 4 lines · Jeff Garzik

Linus, would it be acceptable to you to include an -optional- filter for ATA commands? There is definitely a segment of users that would like to firewall their devices, and I think (as crazy as it may sound) that notion is a valid one.

A perfect filter exists already: it's called "non-root user" :-)

Jeff Garzik

2002-03-12 01:34 UTC #70 lore raw

Quoted 5 lines · Erik Andersen

Quoted 2 lines · Jeff Garzik
Reason 1: Standard kernel convention. In other ioctls, we check basic arguments and return EINVAL when they are wrong, even for privieleged ioctls.

Quoted 8 lines

I have no argument with basic command validation. But take a look at ide_cmd_type_parser(), for example. Do we really need a giant switch statement listing all the allowed commands, just so we can throw back a IDE_DRIVE_TASK_INVALID to user-space if they decide to send down some undocumeted firmware wiping commands? Especially since that giant struct of allowed commands is duplicated in ide_pre_handler_parser() and ide_handler_parser()

I agree the implementation could be improved.

Your first question is really philosophical. I think that people should -not- be able to send undocumented commands through the interface... and in this area IMO it pays to be paranoid.

If we wanted to be ultra-super-paranoid, drop the ioctl and taskfile parser, and implement the taskfile checks via SMM mode callbacks from activity on the IDE ports ;-) That way we know the NSA is not doing something sneaky, as well as supporting unlimited SMP bit-banging from userland. Can you say ug and non-portable even to a lot of ia32 platforms. :)

So, the implementation may need improvement, but we do (a) want the taskfile ioctl [and one for scsi too], and (b) want to implement some amount of mininal sanity checks on the requests.

Jeff

Linus Torvalds

2002-03-12 01:56 UTC #71 lore raw

Quoted 4 lines · Jeff Garzik

Your first question is really philosophical. I think that people should -not- be able to send undocumented commands through the interface...
and in this area IMO it pays to be paranoid.

What if the command is perfectly documented, but only for a certain class of IBM disks?

Are you going to create a table of every disk out there, along with every command it can do?

Remember: the kernel driver is a driver for the host controller, yet the command is for the _disk_. It makes no sense to check for disk commands in a host controller driver - they are two different things.

It's like checking for icmp messages in a network driver. Do you seriously propose having network drivers check icmp messages for command validity?

Linus

Jeff Garzik

2002-03-12 01:50 UTC #72 lore raw

Quoted 14 lines · Linus Torvalds

Quoted 4 lines · Jeff Garzik
Your first question is really philosophical. I think that people should -not- be able to send undocumented commands through the interface... and in this area IMO it pays to be paranoid.
What if the command is perfectly documented, but only for a certain class of IBM disks?
Are you going to create a table of every disk out there, along with every command it can do?

Quoted 7 lines

Remember: the kernel driver is a driver for the host controller, yet the command is for the _disk_. It makes no sense to check for disk commands in a host controller driver - they are two different things.
It's like checking for icmp messages in a network driver. Do you seriously propose having network drivers check icmp messages for command validity?

See my other message, and thanks for making this analogy :)

I -do- know the distrinction between hosts and devices. I think there should be -some- way, I don't care how, to filter out those unknown commands (which may be perfectly valid for a small subset of special IBM drives). The net stack lets me do filtering, I want to sell you on the idea of letting the ATA stack do the same thing.

You have convinced me that unconditional filtering is bad. But I still think people should be provided the option to filter if they so desire.

Jeff

[email protected]

2002-03-12 02:20 UTC #73 lore raw

Quoted 10 lines

I -do- know the distrinction between hosts and devices. I think there should be -some- way, I don't care how, to filter out those unknown commands (which may be perfectly valid for a small subset of special IBM drives). The net stack lets me do filtering, I want to sell you on the idea of letting the ATA stack do the same thing.
You have convinced me that unconditional filtering is bad. But I still think people should be provided the option to filter if they so desire.
Jeff

How hard would this be to farm off to an external utility?

Gerhard

--
Gerhard Mack

[email protected]

<>< As a computer I find your faith in technology amusing.

Linus Torvalds

2002-03-12 02:34 UTC #74 lore raw

Quoted 3 lines · Jeff Garzik

You have convinced me that unconditional filtering is bad. But I still think people should be provided the option to filter if they so desire.

Hey, choice is always good, except if it adds complexity.

The problem with conditional filtering is that either it is a boot (or compile time) option, or it is a dynamic filter.

If its a dynamic filter, and you don't trust root, what _are_ you going to trust? The root program you don't trust might as well be turning the filtering off because it wants to be "convenient". And since the only programs you really want to filter are _exactly_ the kinds of programs that want to avoid filtering, you're just hosed.

That's my real beef with this whole idiotic parsing thing. Either it is fixed (bad, if you don't know what the commands are for all disks) or it is trivially overcome in the name of "convenience" (equally bad, since it makes the whole thing pointless).

None of these issues really exist in your networking example.

Trust me, I'm not trying to be difficult, I'm just pointing out the fundamental problems of your approach.

Yeah yeah. You can add additional levels of protection, and we have capabilities.

Add a special password-protected capability, so that only YOU can enable certain hardware access stuff. Where does it end? Is one such capability enough? How do you initialize the default values for the system if you need to be there to type in the password at bootup every time? We're talking about some rather fundamental things here, and these are issues that go _far_ beyond some silly ATA stack layer.

Linus

Jeff Garzik

2002-03-12 02:35 UTC #75 lore raw

Quoted 12 lines · Linus Torvalds

Quoted 3 lines · Jeff Garzik
You have convinced me that unconditional filtering is bad. But I still think people should be provided the option to filter if they so desire.
Hey, choice is always good, except if it adds complexity.
The problem with conditional filtering is that either it is a boot (or compile time) option, or it is a dynamic filter.

heh, I couldn't have given a better argument against a dynamic filter.

I was actually assuming the filter would be a compile-time option, for security's sake. Boot-time option works too.

So, it sounds like you could be sold on an fixed-at-compile-time filter that can be disabled at boot :) I know you don't like fixed-at-compile-time as you mentioned, but it's my argument there is a class of users that definitely do. MandrakeSoft would likely enable the filter in the "secure" kernel and disable it in the "normal" kernel, for example.

Jeff

J. Dow

2002-03-12 02:54 UTC #76 lore raw

From: "Linus Torvalds" <[email protected]>

Quoted 9 lines

Yeah yeah. You can add additional levels of protection, and we have capabilities.
Add a special password-protected capability, so that only YOU can enable certain hardware access stuff. Where does it end? Is one such capability enough? How do you initialize the default values for the system if you need to be there to type in the password at bootup every time? We're talking about some rather fundamental things here, and these are issues that go _far_ beyond some silly ATA stack layer.

Linus, this discussion hung on long enough I decided to start reading it. So I may have missed something. However, I notice you speak of a networking example. I believe it is a strawman. The real comparison is closer to that between IDE and SCSI. Do the SCSI device drivers filter? What and how do they filter if they do at all? Aren't they a better model to adopt for a system consistent interface philosophy?

{^_^} Joanne Dow, [email protected]

Martin Dalecki

2002-03-12 11:24 UTC #77 lore raw

Quoted 11 lines · Jeff Garzik

Quoted 9 lines · Linus Torvalds
Quoted 3 lines · Jeff Garzik
You have convinced me that unconditional filtering is bad. But I still think people should be provided the option to filter if they so desire.
Hey, choice is always good, except if it adds complexity.

Quoted 8 lines

Quoted 3 lines
The problem with conditional filtering is that either it is a boot (or compile time) option, or it is a dynamic filter.
heh, I couldn't have given a better argument against a dynamic filter.
I was actually assuming the filter would be a compile-time option, for security's sake. Boot-time option works too.
So, it sounds like you could be sold on an fixed-at-compile-time filter that can be disabled at boot :) I know you don't like fixed-at-compile-time as you mentioned, but it's my argument there is a class of users that definitely do. MandrakeSoft would likely enable the filter in the "secure" kernel and disable it in the "normal" kernel, for example.

Mandrake Soft should disable the whole sidestepping
ioctl - even the generic one - in a secure kernel. Filtering them all to -EINVAL is the bast one can do for *security*. The stuff which is not should be implemented with generic ioctl with a proper sementics.

Rings a bell?

No matter how you turn it it turns out that the taskfile stuff as it is is just useless - formal verification inside the kernel can be done at coding time. Formal verification for vendor specific stuff can be done in user land. Formal verification for security stuff doens't make sense, becouse the whole interface doesn't make sense for security concerns. Formal verification of the commands for political reasons is just plain naive...

Vojtech Pavlik

author

2002-03-12 06:32 UTC #78 lore raw

Quoted 16 lines · Linus Torvalds

Quoted 3 lines · Jeff Garzik
You have convinced me that unconditional filtering is bad. But I still think people should be provided the option to filter if they so desire.
Hey, choice is always good, except if it adds complexity.
The problem with conditional filtering is that either it is a boot (or compile time) option, or it is a dynamic filter.
If its a dynamic filter, and you don't trust root, what _are_ you going to trust? The root program you don't trust might as well be turning the filtering off because it wants to be "convenient". And since the only programs you really want to filter are _exactly_ the kinds of programs that want to avoid filtering, you're just hosed.

Quoted 4 lines

That's my real beef with this whole idiotic parsing thing. Either it is fixed (bad, if you don't know what the commands are for all disks) or it is trivially overcome in the name of "convenience" (equally bad, since it makes the whole thing pointless).

Well, there are uses for the 'dynamic' filter, and it doesn't add too much complexity. One could be allowing certain commands to be performed on certain devices by normal users - eg. CD-burning or whatever without root privileges (I know we're using ide-scsi for the command access right now ...), and also protecting the oneself from ACPI and the like. Because ACPI can do IDE commands and does that in a way interfaceable to a 'taskfile' kernel ioctl. It'd be nice to know a broken ACPI implementation can't screw up your drive easily through a kernel driver.

Signature · 2 lines

Vojtech Pavlik
SuSE Labs

Horst von Brand

2002-03-13 17:44 UTC #79 lore raw

Jeff Garzik <[email protected]>

[...]

Quoted 5 lines

I -do- know the distrinction between hosts and devices. I think there should be -some- way, I don't care how, to filter out those unknown commands (which may be perfectly valid for a small subset of special IBM drives). The net stack lets me do filtering, I want to sell you on the idea of letting the ATA stack do the same thing.

The net stack does filtering for handling traffic from _untrusted_ external sources, either for local consumtion or as a service for dumb machines downstream, and as a way of limiting outward access to _untrusted_ users. Here we are talking of the ultimate _trusted_ user (root, CAP_SYS_RAWIO, whatever). It makes no sense for the _kernel_ to get in the way. Create a userland proggie for prodding IDE drives, and give it ways to check (as far as terminal paranoia demands, a little, or not at all) as desired. Unix ultimate simplicity is all about giving root enough rope to shoot at his own feet.

Signature · 4 lines

Dr. Horst H. von Brand                   User #22616 counter.li.org
Departamento de Informatica                     Fono: +56 32 654431
Universidad Tecnica Federico Santa Maria              +56 32 654239
Casilla 110-V, Valparaiso, Chile                Fax:  +56 32 797513

Andre Hedrick

2002-03-13 19:26 UTC #80 lore raw

Quoted 10 lines · Horst von Brand

Jeff Garzik <[email protected]>
[...]
Quoted 5 lines
I -do- know the distrinction between hosts and devices. I think there should be -some- way, I don't care how, to filter out those unknown commands (which may be perfectly valid for a small subset of special IBM drives). The net stack lets me do filtering, I want to sell you on the idea of letting the ATA stack do the same thing.

Quoted 9 lines

The net stack does filtering for handling traffic from _untrusted_ external sources, either for local consumtion or as a service for dumb machines downstream, and as a way of limiting outward access to _untrusted_ users. Here we are talking of the ultimate _trusted_ user (root, CAP_SYS_RAWIO, whatever). It makes no sense for the _kernel_ to get in the way. Create a userland proggie for prodding IDE drives, and give it ways to check (as far as terminal paranoia demands, a little, or not at all) as desired. Unix ultimate simplicity is all about giving root enough rope to shoot at his own feet.

Greetings Dr. von Brand,

One of the issues that has been confused in the stack is the command parser and/or sequencer. It is used internally to allow quick and rapid command additions as needed. Most are not clear how the IOCTL works and thinks it runs via the command parser, this is totally wrong. The sad part is the lenght of the rope does not hit the feet, it hits the head.

Regards,

Andre Hedrick

Pavel Machek

2002-03-15 11:03 UTC #81 lore raw

Hi!

Quoted 3 lines

Well, there are uses for the 'dynamic' filter, and it doesn't add too much complexity. One could be allowing certain commands to be performed on certain devices by normal users - eg. CD-burning or whatever without

That should be better done userspace: setuid on cdrecord + its security audit seems like nice solution.

Quoted 5 lines

root privileges (I know we're using ide-scsi for the command access right now ...), and also protecting the oneself from ACPI and the like. Because ACPI can do IDE commands and does that in a way interfaceable to a 'taskfile' kernel ioctl. It'd be nice to know a broken ACPI implementation can't screw up your drive easily through a kernel driver.

This is entirely different can of worms, and should be easy to do at ACPI level.
Pavel

Signature · 2 lines

Philips Velo 1: 1"x4"x8", 300gram, 60, 12MB, 40bogomips, linux, mutt,
details at http://atrey.karlin.mff.cuni.cz/~pavel/velo/index.html.

Vojtech Pavlik

author

2002-03-12 06:26 UTC #82 lore raw

Quoted 17 lines · Jeff Garzik

Quoted 7 lines · Erik Andersen
Quoted 4 lines · Jeff Garzik
Reason 1: Standard kernel convention. In other ioctls, we check basic arguments and return EINVAL when they are wrong, even for privieleged ioctls.
Quoted 8 lines
I have no argument with basic command validation. But take a look at ide_cmd_type_parser(), for example. Do we really need a giant switch statement listing all the allowed commands, just so we can throw back a IDE_DRIVE_TASK_INVALID to user-space if they decide to send down some undocumeted firmware wiping commands? Especially since that giant struct of allowed commands is duplicated in ide_pre_handler_parser() and ide_handler_parser()

Quoted 5 lines

I agree the implementation could be improved.
Your first question is really philosophical. I think that people should -not- be able to send undocumented commands through the interface...
and in this area IMO it pays to be paranoid.

Why? What if I really want to update the firmware is say my CD-RW, and it doesn't use any of the ATA-spec commands for that? Should it fail?

I think filtering bad requests is OK - validating stuff like permissions, concurrent access, pointers, whatever, but the kernel shouldn't have hardcoded tables of commands that are allowed.

The "right solution", if we really need command filtering is an ioctl and an utility that can fill a 'filter table', which says which commands are ok for whom and which are not. Like 'INQUIRY can be used by anyone', 'no, I won't send anything related to CPRM to the drive'. Easy to implement, no huge case statements needed.

Quoted 10 lines

If we wanted to be ultra-super-paranoid, drop the ioctl and taskfile parser, and implement the taskfile checks via SMM mode callbacks from activity on the IDE ports ;-) That way we know the NSA is not doing something sneaky, as well as supporting unlimited SMP bit-banging from userland. Can you say ug and non-portable even to a lot of ia32 platforms. :)
So, the implementation may need improvement, but we do (a) want the taskfile ioctl [and one for scsi too], and (b) want to implement some amount of mininal sanity checks on the requests.

Signature · 2 lines

Vojtech Pavlik
SuSE Labs

Erik Andersen

2002-03-12 07:13 UTC #83 lore raw

Quoted 4 lines · Erik

I have no argument with basic command validation. But take a look at ide_cmd_type_parser(), for example. Do we really need a giant switch statement listing all the allowed commands, just so we can throw back a IDE_DRIVE_TASK_INVALID to user-space if they

In looking closer at the ide driver, it turns out that ide_cmd_type_parser() is _not_ filtering HDIO_DRIVE_TASKFILE ioctl commands at all. My mistake. There is actually no filtering at all HDIO_DRIVE_TASKFILE ioctl.

Strangely enough, ide_cmd_type_parser() is filtering commands issued by the ide driver itself.

-Erik

--
Erik B. Andersen http://codepoet-consulting.com/
--This message was written using 73% post-consumer electrons--

Vojtech Pavlik

author

2002-03-15 11:15 UTC #84 lore raw

Quoted 8 lines · Pavel Machek

Quoted 5 lines
You may happen to have the numbers, though - that should be enough.
Btw, I have a CMD640B based PCI card lying around here, but never managed to get it generate any interrupts, though the rest seems to be working.
Attach it to the timer interrupt -- that should do it for testing. Simplest way is to make ide timeouts HZ/100 and killing "lost interrupt" msg ;-).

Well, it seems like we'll have to something like this anyway. Some chips sometimes forget to assert the IRQ after a transfer due to HW bugs, and some PIIX3s are reported to do it quite often.

Signature · 2 lines

Vojtech Pavlik
SuSE Labs

Martin Dalecki

2002-03-12 11:02 UTC #85 lore raw

Hello Vojtech.

I have noticed that the ide-timings.h and ide_modules.h are running much in aprallel in the purpose they serve. Are the any chances you could dare to care about propagating the fairly nice ide-timings.h stuff in favour of ide_modules.h more.

BTW.> I think some stuff from ide-timings.h just belongs as generic functions intro ide.c, and right now there is nobody who you need to work from behind ;-).

So please feel free to do the changes you apparently desired to do a long time ago...

Pavel Machek

2002-03-18 19:56 UTC #86 lore raw

Hi!

Quoted 6 lines

Quoted 6 lines
Quoted 5 lines
You may happen to have the numbers, though - that should be enough.
Btw, I have a CMD640B based PCI card lying around here, but never managed to get it generate any interrupts, though the rest seems to be working.

Quoted 6 lines

Quoted 2 lines
Attach it to the timer interrupt -- that should do it for testing. Simplest way is to make ide timeouts HZ/100 and killing "lost interrupt" msg ;-).
Well, it seems like we'll have to something like this anyway. Some chips sometimes forget to assert the IRQ after a transfer due to HW bugs, and some PIIX3s are reported to do it quite often.

What is "quite often"? Unless it is more than once in a hour, current code is just okay... (It waits for timeout, which is about 30 sec?, then recovers).
Pavel

Signature · 2 lines

(about SSSCA) "I don't say this lightly.  However, I really think that the U.S.
no longer is classifiable as a democracy, but rather as a plutocracy." --hpa

Vojtech Pavlik

author

2002-03-12 16:00 UTC #87 lore raw

Quoted 8 lines · Martin Dalecki

Hello Vojtech.
I have noticed that the ide-timings.h and ide_modules.h are running much in aprallel in the purpose they serve. Are the any chances you could dare to care about propagating the fairly nice ide-timings.h stuff in favour of ide_modules.h more.

Quoted 6 lines

BTW.> I think some stuff from ide-timings.h just belongs as generic functions intro ide.c, and right now there is nobody who you need to work from behind ;-).
So please feel free to do the changes you apparently desired to do a long time ago...

Hmm, ok. Try this. It shouldn't change any functionality, yet makes a small step towards cleaning the chipset specific drivers.

Reading through them as I was doing the changes, I found out that most of them compute the timings incorrectly. Because of that I also removed the pio blacklist (which is going to come back in a more powerful form, merged together with the DMA blacklist), because that one is based on ancient experiments with the broken CMD640 chip and a driver which doesn't get the timings correct either. The blacklist is plain invalid.

I plan to focus on the most important drivers first, to fix and clean them, working with the authors where possible.

Signature · 2 lines

Vojtech Pavlik
SuSE Labs

drivers/ide/Makefile +1 -1

diff -urN linux-2.5.6/drivers/ide/Makefile linux-2.5.6-timing/drivers/ide/Makefile
@@ -76,7 +76,7 @@
 
 ide-obj-$(CONFIG_PROC_FS)		+= ide-proc.o
 
-ide-mod-objs		:= ide-taskfile.o ide.o ide-probe.o ide-geometry.o ide-features.o $(ide-obj-y)
+ide-mod-objs		:= ide-taskfile.o ide.o ide-probe.o ide-geometry.o ide-features.o ide-timing.o $(ide-obj-y)
 
 include $(TOPDIR)/Rules.make

drivers/ide/aec62xx.c +5 ~~-10~~

diff -urN linux-2.5.6/drivers/ide/aec62xx.c linux-2.5.6-timing/drivers/ide/aec62xx.c
@@ -24,7 +24,7 @@
 #include <asm/io.h>
 #include <asm/irq.h>
 
-#include "ide_modes.h"
+#include "ide-timing.h"
 
 #define DISPLAY_AEC62XX_TIMINGS

@@ -412,16 +412,11 @@
 static void aec62xx_tune_drive (ide_drive_t *drive, byte pio)
 {
 	byte speed;
-	byte new_pio = XFER_PIO_0 + ide_get_best_pio_mode(drive, 255, 5, NULL);
 
-	switch(pio) {
-		case 5:		speed = new_pio; break;
-		case 4:		speed = XFER_PIO_4; break;
-		case 3:		speed = XFER_PIO_3; break;
-		case 2:		speed = XFER_PIO_2; break;
-		case 1:		speed = XFER_PIO_1; break;
-		default:	speed = XFER_PIO_0; break;
-	}
+	if (pio == 255)
+		speed = ide_find_best_mode(drive, XFER_PIO | XFER_EPIO);
+	else
+		speed = XFER_PIO_0 + min_t(byte, pio, 4);
 
 	switch(HWIF(drive)->pci_dev->device) {
 		case PCI_DEVICE_ID_ARTOP_ATP850UF:

drivers/ide/ali14xx.c +12 -9

diff -urN linux-2.5.6/drivers/ide/ali14xx.c linux-2.5.6-timing/drivers/ide/ali14xx.c
@@ -48,7 +48,7 @@
 
 #include <asm/io.h>
 
-#include "ide_modes.h"
+#include "ide-timing.h"
 
 /* port addresses for auto-detection */
 #define ALI_NUM_PORTS 4

@@ -67,8 +67,6 @@
 	{0x35, 0x03}, {0x00, 0x00}
 };
 
-#define ALI_MAX_PIO 4
-
 /* timing parameter registers for each drive */
 static struct { byte reg1, reg2, reg3, reg4; } regTab[4] = {
 	{0x03, 0x26, 0x04, 0x27},     /* drive 0 */

@@ -114,21 +112,26 @@
 	int time1, time2;
 	byte param1, param2, param3, param4;
 	unsigned long flags;
-	ide_pio_data_t d;
+	struct ide_timing *t;
+
+	if (pio == 255)
+		pio = ide_find_best_mode(drive, XFER_PIO | XFER_EPIO);
+	else
+		pio = XFER_PIO_0 + min_t(byte, pio, 4);
 
-	pio = ide_get_best_pio_mode(drive, pio, ALI_MAX_PIO, &d);
+	t = ide_timing_find_mode(pio);
 
 	/* calculate timing, according to PIO mode */
-	time1 = d.cycle_time;
-	time2 = ide_pio_timings[pio].active_time;
+	time1 = t->cycle;
+	time2 = t->active;
 	param3 = param1 = (time2 * system_bus_speed + 999) / 1000;
 	param4 = param2 = (time1 * system_bus_speed + 999) / 1000 - param1;
-	if (pio < 3) {
+	if (pio < XFER_PIO_3) {
 		param3 += 8;
 		param4 += 8;
 	}
 	printk("%s: PIO mode%d, t1=%dns, t2=%dns, cycles = %d+%d, %d+%d\n",
-		drive->name, pio, time1, time2, param1, param2, param3, param4);
+		drive->name, pio - XFER_PIO_0, time1, time2, param1, param2, param3, param4);
 
 	/* stuff timing parameters into controller registers */
 	driveNum = (HWIF(drive)->index << 1) + drive->select.b.unit;

drivers/ide/alim15x3.c +12 ~~-17~~

diff -urN linux-2.5.6/drivers/ide/alim15x3.c linux-2.5.6-timing/drivers/ide/alim15x3.c
@@ -26,7 +26,7 @@
 
 #include <asm/io.h>
 
-#include "ide_modes.h"
+#include "ide-timing.h"
 
 #define DISPLAY_ALI_TIMINGS

@@ -241,7 +241,7 @@
 
 static void ali15x3_tune_drive (ide_drive_t *drive, byte pio)
 {
-	ide_pio_data_t d;
+	struct ide_timing *t;
 	ide_hwif_t *hwif = HWIF(drive);
 	struct pci_dev *dev = hwif->pci_dev;
 	int s_time, a_time, c_time;

@@ -250,15 +250,21 @@
 	int port = hwif->index ? 0x5c : 0x58;
 	int portFIFO = hwif->channel ? 0x55 : 0x54;
 	byte cd_dma_fifo = 0;
+	
+	if (pio == 255)
+		pio = ide_find_best_mode(drive, XFER_PIO | XFER_EPIO);
+	else
+		pio = XFER_PIO_0 + min_t(byte, pio, 4);
 
-	pio = ide_get_best_pio_mode(drive, pio, 5, &d);
-	s_time = ide_pio_timings[pio].setup_time;
-	a_time = ide_pio_timings[pio].active_time;
+	t = ide_timing_find_mode(pio);	
+	
+	s_time = t->setup;
+	a_time = t->active;
 	if ((s_clc = (s_time * system_bus_speed + 999) / 1000) >= 8)
 		s_clc = 0;
 	if ((a_clc = (a_time * system_bus_speed + 999) / 1000) >= 8)
 		a_clc = 0;
-	c_time = ide_pio_timings[pio].cycle_time;
+	c_time = t->cycle;
 
 #if 0
 	if ((r_clc = ((c_time - s_time - a_time) * system_bus_speed + 999) / 1000) >= 16)

@@ -295,17 +301,6 @@
 	pci_write_config_byte(dev, port, s_clc);
 	pci_write_config_byte(dev, port+drive->select.b.unit+2, (a_clc << 4) | r_clc);
 	__restore_flags(flags);
-
-	/*
-	 * setup   active  rec
-	 * { 70,   165,    365 },   PIO Mode 0
-	 * { 50,   125,    208 },   PIO Mode 1
-	 * { 30,   100,    110 },   PIO Mode 2
-	 * { 30,   80,     70  },   PIO Mode 3 with IORDY
-	 * { 25,   70,     25  },   PIO Mode 4 with IORDY  ns
-	 * { 20,   50,     30  }    PIO Mode 5 with IORDY (nonstandard)
-	 */
-
 }
 
 static int ali15x3_tune_chipset (ide_drive_t *drive, byte speed)

drivers/ide/amd74xx.c +2 -2

diff -urN linux-2.5.6/drivers/ide/amd74xx.c linux-2.5.6-timing/drivers/ide/amd74xx.c
@@ -235,7 +235,7 @@
 				drive->dn >> 1, drive->dn & 1);
 
 	T = 1000000000 / amd_clock;
-	UT = T / MIN(MAX(amd_config->flags & AMD_UDMA, 1), 2);
+	UT = T / min_t(int, max_t(int, amd_config->flags & AMD_UDMA, 1), 2);
 
 	ide_timing_compute(drive, speed, &t, T, UT);

@@ -270,7 +270,7 @@
 		return;
 	}
 
-	amd_set_drive(drive, XFER_PIO_0 + MIN(pio, 5));
+	amd_set_drive(drive, XFER_PIO_0 + min_t(byte, pio, 5));
 }
 
 #ifdef CONFIG_BLK_DEV_IDEDMA

drivers/ide/cmd640.c +13 ~~-15~~

diff -urN linux-2.5.6/drivers/ide/cmd640.c linux-2.5.6-timing/drivers/ide/cmd640.c
@@ -112,7 +112,7 @@
 
 #include <asm/io.h>
 
-#include "ide_modes.h"
+#include "ide-timing.h"
 
 /*
  * This flag is set in ide.c by the parameter:  ide0=cmd640_vlb

@@ -589,15 +589,11 @@
 /*
  * Set a specific pio_mode for a drive
  */
-static void cmd640_set_mode (unsigned int index, byte pio_mode, unsigned int cycle_time)
+static void cmd640_set_mode (unsigned int index, byte pio_mode, unsigned int cycle_time, unsigned int active_time, unsigned int setup_time)
 {
-	int setup_time, active_time, recovery_time, clock_time;
+	int recovery_time, clock_time;
 	byte setup_count, active_count, recovery_count, recovery_count2, cycle_count;
 
-	if (pio_mode > 5)
-		pio_mode = 5;
-	setup_time  = ide_pio_timings[pio_mode].setup_time;
-	active_time = ide_pio_timings[pio_mode].active_time;
 	recovery_time = cycle_time - (setup_time + active_time);
 	clock_time = 1000 / system_bus_speed;
 	cycle_count = (cycle_time + clock_time - 1) / clock_time;

@@ -646,7 +642,7 @@
 static void cmd640_tune_drive (ide_drive_t *drive, byte mode_wanted)
 {
 	byte b;
-	ide_pio_data_t  d;
+	struct ide_timing *t;
 	unsigned int index = 0;
 
 	while (drive != cmd_drives[index]) {

@@ -674,14 +670,16 @@
 			return;
 	}
 
-	(void) ide_get_best_pio_mode (drive, mode_wanted, 5, &d);
-	cmd640_set_mode (index, d.pio_mode, d.cycle_time);
+	if (mode_wanted == 255)
+		t = ide_timing_find_mode(ide_find_best_mode(drive, XFER_PIO | XFER_EPIO));
+	else
+		t = ide_timing_find_mode(XFER_PIO_0 + min_t(byte, mode_wanted, 4));
+
+	cmd640_set_mode(index, t->mode - XFER_PIO_0, t->cycle, t->active, t->setup);
+
+	printk ("%s: selected cmd640 PIO mode%d (%dns)",
+		drive->name, t->mode, t->cycle);
 
-	printk ("%s: selected cmd640 PIO mode%d (%dns)%s",
-		drive->name,
-		d.pio_mode,
-		d.cycle_time,
-		d.overridden ? " (overriding vendor mode)" : "");
 	display_clocks(index);
 	return;
 }

drivers/ide/cmd64x.c +14 ~~-21~~

diff -urN linux-2.5.6/drivers/ide/cmd64x.c linux-2.5.6-timing/drivers/ide/cmd64x.c
@@ -23,7 +23,7 @@
 
 #include <asm/io.h>
 
-#include "ide_modes.h"
+#include "ide-timing.h"
 
 #ifndef SPLIT_BYTE
 #define SPLIT_BYTE(B,H,L)	((H)=(B>>4), (L)=(B-((B>>4)<<4)))

@@ -279,10 +279,10 @@
  */
 static void cmd64x_tuneproc (ide_drive_t *drive, byte mode_wanted)
 {
-	int setup_time, active_time, recovery_time, clock_time, pio_mode, cycle_time;
+	int recovery_time, clock_time;
 	byte recovery_count2, cycle_count;
 	int setup_count, active_count, recovery_count;
-	ide_pio_data_t  d;
+	struct ide_timing *t;
 
 	switch (mode_wanted) {
 		case 8: /* set prefetch off */

@@ -291,27 +291,21 @@
 			/*set_prefetch_mode(index, mode_wanted);*/
 			cmdprintk("%s: %sabled cmd640 prefetch\n", drive->name, mode_wanted ? "en" : "dis");
 			return;
+		case 255: mode_wanted = ide_find_best_mode(drive, XFER_PIO | XFER_EPIO);
 	}
 
-	mode_wanted = ide_get_best_pio_mode (drive, mode_wanted, 5, &d);
-	pio_mode = d.pio_mode;
-	cycle_time = d.cycle_time;
+	t = ide_timing_find_mode(XFER_PIO_0 + min_t(byte, mode_wanted, 4));
 
 	/*
 	 * I copied all this complicated stuff from cmd640.c and made a few minor changes.
 	 * For now I am just going to pray that it is correct.
 	 */
-	if (pio_mode > 5)
-		pio_mode = 5;
-	setup_time  = ide_pio_timings[pio_mode].setup_time;
-	active_time = ide_pio_timings[pio_mode].active_time;
-	recovery_time = cycle_time - (setup_time + active_time);
-	clock_time = 1000 / system_bus_speed;
-	cycle_count = (cycle_time + clock_time - 1) / clock_time;
-
-	setup_count = (setup_time + clock_time - 1) / clock_time;
 
-	active_count = (active_time + clock_time - 1) / clock_time;
+	recovery_time = t->cycle - (t->setup + t->active);
+	clock_time = 1000 / system_bus_speed;
+	cycle_count = (t->cycle + clock_time - 1) / clock_time;
+	setup_count = (t->setup + clock_time - 1) / clock_time;
+	active_count = (t->active + clock_time - 1) / clock_time;
 
 	recovery_count = (recovery_time + clock_time - 1) / clock_time;
 	recovery_count2 = cycle_count - (setup_count + active_count);

@@ -334,9 +328,8 @@
 	 */
 	program_drive_counts (drive, setup_count, active_count, recovery_count);
 
-	cmdprintk("%s: selected cmd646 PIO mode%d : %d (%dns)%s, clocks=%d/%d/%d\n",
-		drive->name, pio_mode, mode_wanted, cycle_time,
-		d.overridden ? " (overriding vendor mode)" : "",
+	cmdprintk("%s: selected cmd646 PIO mode%d : %d (%dns), clocks=%d/%d/%d\n",
+		drive->name, t.mode - XFER_PIO_0, mode_wanted, cycle_time,
 		setup_count, active_count, recovery_count);
 }

@@ -391,7 +384,7 @@
 static void config_cmd64x_chipset_for_pio (ide_drive_t *drive, byte set_speed)
 {
 	byte speed	= 0x00;
-	byte set_pio	= ide_get_best_pio_mode(drive, 4, 5, NULL);
+	byte set_pio	= ide_find_best_mode(drive, XFER_PIO | XFER_EPIO) - XFER_PIO_0;
 
 	cmd64x_tuneproc(drive, set_pio);
 	speed = XFER_PIO_0 + set_pio;

@@ -408,7 +401,7 @@
 	u8 speed		= 0x00;
 	u8 mode_pci		= 0x00;
 	u8 channel_timings	= cmd680_taskfile_timing(hwif);
-	u8 set_pio		= ide_get_best_pio_mode(drive, 4, 5, NULL);
+	u8 set_pio		= ide_find_best_mode(drive, XFER_PIO | XFER_EPIO) - XFER_PIO_0;
 
 	pci_read_config_byte(dev, addr_mask, &mode_pci);
 	mode_pci &= ~((unit) ? 0x30 : 0x03);

drivers/ide/cs5530.c +7 -4

diff -urN linux-2.5.6/drivers/ide/cs5530.c linux-2.5.6-timing/drivers/ide/cs5530.c
@@ -27,7 +27,7 @@
 #include <asm/io.h>
 #include <asm/irq.h>
 
-#include "ide_modes.h"
+#include "ide-timing.h"
 
 #define DISPLAY_CS5530_TIMINGS

@@ -114,10 +114,13 @@
 {
 	ide_hwif_t	*hwif = HWIF(drive);
 	unsigned int	format, basereg = CS5530_BASEREG(hwif);
-	static byte	modes[5] = {XFER_PIO_0, XFER_PIO_1, XFER_PIO_2, XFER_PIO_3, XFER_PIO_4};
 
-	pio = ide_get_best_pio_mode(drive, pio, 4, NULL);
-	if (!cs5530_set_xfer_mode(drive, modes[pio])) {
+	if (pio == 255)
+		pio = ide_find_best_mode(drive, XFER_PIO | XFER_EPIO);
+	else
+		pio = XFER_PIO_0 + min_t(byte, pio, 4);
+
+	if (!cs5530_set_xfer_mode(drive, pio)) {
 		format = (inl(basereg+4) >> 31) & 1;
 		outl(cs5530_pio_timings[format][pio], basereg+(drive->select.b.unit<<3));
 	}

drivers/ide/cy82c693.c +9 -8

diff -urN linux-2.5.6/drivers/ide/cy82c693.c linux-2.5.6-timing/drivers/ide/cy82c693.c
@@ -53,7 +53,7 @@
 
 #include <asm/io.h>
 
-#include "ide_modes.h"
+#include "ide-timing.h"
 
 /* the current version */
 #define CY82_VERSION	"CY82C693U driver v0.34 99-13-12 Andreas S. Krebs ([email protected])"

@@ -140,8 +140,11 @@
  */ 
 static void compute_clocks (byte pio, pio_clocks_t *p_pclk)
 {
+	struct ide_timing *t;
 	int clk1, clk2;
 
+	t = ide_timing_find_mode(XFER_PIO_0 + pio);
+
 	/* we don't check against CY82C693's min and max speed,
 	 * so you can play with the idebus=xx parameter
 	 */

@@ -150,15 +153,13 @@
 		pio = CY82C693_MAX_PIO;
 
 	/* let's calc the address setup time clocks */
-	p_pclk->address_time = (byte)calc_clk(ide_pio_timings[pio].setup_time, system_bus_speed);
+	p_pclk->address_time = (byte)calc_clk(t->setup, system_bus_speed);
 
 	/* let's calc the active and recovery time clocks */
-	clk1 = calc_clk(ide_pio_timings[pio].active_time, system_bus_speed);
+	clk1 = calc_clk(t->active, system_bus_speed);
 
 	/* calc recovery timing */
-	clk2 =	ide_pio_timings[pio].cycle_time -
-		ide_pio_timings[pio].active_time -
-		ide_pio_timings[pio].setup_time;
+	clk2 =	t->cycle - t->active - t->setup;
 
 	clk2 = calc_clk(clk2, system_bus_speed);

@@ -166,7 +167,7 @@
 
 	/* note: we use the same values for 16bit IOR and IOW
          *	those are all the same, since I don't have other
-	 *	timings than those from ide_modes.h
+	 *	timings than those from ide-timing.h
 	 */
 
 	p_pclk->time_16r = (byte)clk1;

@@ -321,7 +322,7 @@
 #endif /* CY82C693_DEBUG_LOGS */
 
         /* first let's calc the pio modes */
-	pio = ide_get_best_pio_mode(drive, pio, CY82C693_MAX_PIO, NULL);
+	pio = ide_find_best_mode(drive, XFER_PIO | XFER_EPIO) - XFER_PIO_0;
 
 #if CY82C693_DEBUG_INFO
 	printk (KERN_INFO "%s: Selected PIO mode %d\n", drive->name, pio);

drivers/ide/dtc2278.c +2 -2

diff -urN linux-2.5.6/drivers/ide/dtc2278.c linux-2.5.6-timing/drivers/ide/dtc2278.c
@@ -15,7 +15,7 @@
 
 #include <asm/io.h>
 
-#include "ide_modes.h"
+#include "ide-timing.h"
 
 /*
  * Changing this #undef to #define may solve start up problems in some systems.

@@ -70,7 +70,7 @@
 {
 	unsigned long flags;
 
-	pio = ide_get_best_pio_mode(drive, pio, 4, NULL);
+	pio = ide_find_best_mode(drive, XFER_PIO | XFER_EPIO) - XFER_PIO_0;
 
 	if (pio >= 3) {
 		save_flags(flags);	/* all CPUs */

drivers/ide/hpt34x.c +3 -3

diff -urN linux-2.5.6/drivers/ide/hpt34x.c linux-2.5.6-timing/drivers/ide/hpt34x.c
@@ -40,7 +40,7 @@
 
 #include <asm/io.h>
 #include <asm/irq.h>
-#include "ide_modes.h"
+#include "ide-timing.h"
 
 #ifndef SPLIT_BYTE
 #define SPLIT_BYTE(B,H,L)	((H)=(B>>4), (L)=(B-((B>>4)<<4)))

@@ -152,9 +152,9 @@
 
 	byte	timing, speed, pio;
 
-	pio = ide_get_best_pio_mode(drive, 255, 5, NULL);
+	pio = ide_find_best_mode(drive, XFER_PIO | XFER_EPIO) - XFER_PIO_0;
 
-	if (xfer_pio> 4)
+	if (xfer_pio > 4)
 		xfer_pio = 0;
 
 	if (drive->id->eide_pio_iordy > 0) {

drivers/ide/hpt366.c +3 -3

diff -urN linux-2.5.6/drivers/ide/hpt366.c linux-2.5.6-timing/drivers/ide/hpt366.c
@@ -61,7 +61,7 @@
 #include <asm/io.h>
 #include <asm/irq.h>
 
-#include "ide_modes.h"
+#include "ide-timing.h"
 
 #define DISPLAY_HPT366_TIMINGS

@@ -600,9 +600,9 @@
 	unsigned short xfer_pio = drive->id->eide_pio_modes;
 	byte	timing, speed, pio;
 
-	pio = ide_get_best_pio_mode(drive, 255, 5, NULL);
+	pio = ide_find_best_mode(drive, XFER_PIO | XFER_EPIO) - XFER_PIO_0;
 
-	if (xfer_pio> 4)
+	if (xfer_pio > 4)
 		xfer_pio = 0;
 
 	if (drive->id->eide_pio_iordy > 0) {

drivers/ide/ht6560b.c +12 -8

diff -urN linux-2.5.6/drivers/ide/ht6560b.c linux-2.5.6-timing/drivers/ide/ht6560b.c
@@ -44,7 +44,7 @@
 
 #include <asm/io.h>
 
-#include "ide_modes.h"
+#include "ide-timing.h"
 
 /* #define DEBUG */  /* remove comments for DEBUG messages */

@@ -199,20 +199,23 @@
 {
 	int active_time, recovery_time;
 	int active_cycles, recovery_cycles;
-	ide_pio_data_t d;
+	struct ide_timing *t;
 	
         if (pio) {
-		pio = ide_get_best_pio_mode(drive, pio, 5, &d);
+		if (pio == 255)
+			pio = ide_find_best_mode(drive, XFER_PIO | XFER_EPIO);
+		else
+			pio = XFER_PIO_0 + min_t(byte, pio, 4);
+
+		t = ide_timing_find_mode(pio);
 		
 		/*
 		 *  Just like opti621.c we try to calculate the
 		 *  actual cycle time for recovery and activity
 		 *  according system bus speed.
 		 */
-		active_time = ide_pio_timings[pio].active_time;
-		recovery_time = d.cycle_time 
-			- active_time
-			- ide_pio_timings[pio].setup_time;
+		active_time = t->active;
+		recovery_time = t->cycle - active_time - t->setup;
 		/*
 		 *  Cycle times should be Vesa bus cycles
 		 */

@@ -227,7 +230,8 @@
 		if (recovery_cycles > 15) recovery_cycles = 0;  /* 0==16 */
 		
 #ifdef DEBUG
-		printk("ht6560b: drive %s setting pio=%d recovery=%d (%dns) active=%d (%dns)\n", drive->name, pio, recovery_cycles, recovery_time, active_cycles, active_time);
+		printk("ht6560b: drive %s setting pio=%d recovery=%d (%dns) active=%d (%dns)\n",
+			drive->name, pio - XFER_PIO_0, recovery_cycles, recovery_time, active_cycles, active_time);
 #endif
 		
 		return (byte)((recovery_cycles << 4) | active_cycles);

drivers/ide/icside.c +0 ~~-107~~

131 lines not on this page · raw message

drivers/ide/ide-m8xx.c +10 ~~-13~~

diff -urN linux-2.5.6/drivers/ide/ide-m8xx.c linux-2.5.6-timing/drivers/ide/ide-m8xx.c
@@ -37,7 +37,7 @@
 #include <asm/machdep.h>
 #include <asm/irq.h>
 
-#include "ide_modes.h"
+#include "ide-timing.h"
 static int identify  (volatile unsigned char *p);
 static void print_fixed (volatile unsigned char *p);
 static void print_funcid (int func);

@@ -222,19 +222,19 @@
 		/* Compute clock cycles for PIO timings */
 		for (i=0; i<6; ++i) {
 			bd_t	*binfo = (bd_t *)__res;
+			struct ide_timing *t;
+
+			t = ide_timing_find_mode(i + XFER_PIO_0);
 
 			hold_time[i]   =
 				PCMCIA_MK_CLKS (hold_time[i],
 						binfo->bi_busfreq);
 			ide_pio_clocks[i].setup_time  =
-				PCMCIA_MK_CLKS (ide_pio_timings[i].setup_time,
-						binfo->bi_busfreq);
+				PCMCIA_MK_CLKS (t->setup, binfo->bi_busfreq);
 			ide_pio_clocks[i].active_time =
-				PCMCIA_MK_CLKS (ide_pio_timings[i].active_time,
-						binfo->bi_busfreq);
+				PCMCIA_MK_CLKS (t->active, binfo->bi_busfreq);
 			ide_pio_clocks[i].cycle_time  =
-				PCMCIA_MK_CLKS (ide_pio_timings[i].cycle_time,
-						binfo->bi_busfreq);
+				PCMCIA_MK_CLKS (t->cycle, binfo->bi_busfreq);
 #if 0
 			printk ("PIO mode %d timings: %d/%d/%d => %d/%d/%d\n",
 				i,

@@ -242,10 +242,7 @@
 				ide_pio_clocks[i].active_time,
 				ide_pio_clocks[i].hold_time,
 				ide_pio_clocks[i].cycle_time,
-				ide_pio_timings[i].setup_time,
-				ide_pio_timings[i].active_time,
-				ide_pio_timings[i].hold_time,
-				ide_pio_timings[i].cycle_time);
+				t->setup, t->active, hold_time[i], t->cycle);
 #endif
 		}
 	}

@@ -429,13 +426,13 @@
 static void
 m8xx_ide_tuneproc(ide_drive_t *drive, byte pio)
 {
-	ide_pio_data_t d;
 #if defined(CONFIG_IDE_8xx_PCCARD) || defined(CONFIG_IDE_8xx_DIRECT)
 	volatile pcmconf8xx_t	*pcmp;
 	ulong timing, mask, reg;
 #endif
 
-	pio = ide_get_best_pio_mode(drive, pio, 4, &d);
+	if (pio == 255)
+		pio = ide_find_best_mode(drive, XFER_PIO | XFER_EPIO) - XFER_PIO_0;
 
 #if 1
 	printk("%s[%d] %s: best PIO mode: %d\n",

drivers/ide/ide-pmac.c +14 -8

diff -urN linux-2.5.6/drivers/ide/ide-pmac.c linux-2.5.6-timing/drivers/ide/ide-pmac.c
@@ -36,7 +36,7 @@
 #include <linux/pmu.h>
 #include <asm/irq.h>
 #endif
-#include "ide_modes.h"
+#include "ide-timing.h"
 
 extern char *ide_dmafunc_verbose(ide_dma_action_t dmafunc);

@@ -282,7 +282,7 @@
 static void
 pmac_ide_tuneproc(ide_drive_t *drive, byte pio)
 {
-	ide_pio_data_t d;
+	struct ide_timing *t;
 	int i;
 	u32 *timings;
 	int accessTicks, recTicks;

@@ -290,9 +290,15 @@
 	i = pmac_ide_find(drive);
 	if (i < 0)
 		return;
+	
+	if (pio = 255)	
+		pio = ide_find_best_mode(drive, XFER_PIO | XFER_EPIO);
+	else
+		pio = XFER_PIO_0 + min_t(byte, pio, 4);
 		
-	pio = ide_get_best_pio_mode(drive, pio, 4, &d);
-	accessTicks = SYSCLK_TICKS(ide_pio_timings[pio].active_time);
+	t = ide_timing_find_mode(pio);
+	
+	accessTicks = SYSCLK_TICKS(t->active);
 	if (drive->select.all & 0x10)
 		timings = &pmac_ide[i].timings[1];
 	else

@@ -300,16 +306,16 @@
 	
 	if (pmac_ide[i].kind == controller_kl_ata4) {
 		/* The "ata-4" IDE controller of Core99 machines */
-		accessTicks = SYSCLK_TICKS_UDMA(ide_pio_timings[pio].active_time * 1000);
-		recTicks = SYSCLK_TICKS_UDMA(d.cycle_time * 1000) - accessTicks;
+		accessTicks = SYSCLK_TICKS_UDMA(t->active * 1000);
+		recTicks = SYSCLK_TICKS_UDMA(t->cycle * 1000) - accessTicks;
 
 		*timings = ((*timings) & 0x1FFFFFC00) | accessTicks | (recTicks << 5);
 	} else {
 		/* The old "ata-3" IDE controller */
-		accessTicks = SYSCLK_TICKS(ide_pio_timings[pio].active_time);
+		accessTicks = SYSCLK_TICKS(t->active);
 		if (accessTicks < 4)
 			accessTicks = 4;
-		recTicks = SYSCLK_TICKS(d.cycle_time) - accessTicks - 4;
+		recTicks = SYSCLK_TICKS(t->cycle) - accessTicks - 4;
 		if (recTicks < 1)
 			recTicks = 1;

drivers/ide/ide-timing.c +235 -0

235 lines not on this page · raw message

drivers/ide/ide-timing.h +19 ~~-212~~

267 lines not on this page · raw message

drivers/ide/ide.c +1 ~~-188~~

207 lines not on this page · raw message

drivers/ide/ide_modes.h +0 ~~-42~~

42 lines not on this page · raw message

drivers/ide/it8172.c +7 ~~-21~~

diff -urN linux-2.5.6/drivers/ide/it8172.c linux-2.5.6-timing/drivers/ide/it8172.c
@@ -41,7 +41,7 @@
 #include <asm/io.h>
 #include <asm/it8172/it8172_int.h>
 
-#include "ide_modes.h"
+#include "ide-timing.h"
 
 /*
  * Prototypes

@@ -65,7 +65,11 @@
     int master_port	= 0x40;
     int slave_port      = 0x44;
     
-    pio = ide_get_best_pio_mode(drive, pio, 5, NULL);
+    if (pio == 255)
+	pio = ide_find_best_mode(drive, XFER_PIO | XFER_EPIO) - XFER_PIO_0;
+    else
+       pio = min_t(byte, pio, 4);
+
     pci_read_config_word(HWIF(drive)->pci_dev, master_port, &master_data);
     pci_read_config_dword(HWIF(drive)->pci_dev, slave_port, &slave_data);

@@ -186,25 +190,7 @@
     struct hd_driveid *id = drive->id;
     byte speed;
 
-    if (id->dma_ultra & 0x0010) {
-	speed = XFER_UDMA_2;
-    } else if (id->dma_ultra & 0x0008) {
-	speed = XFER_UDMA_1;
-    } else if (id->dma_ultra & 0x0004) {
-	speed = XFER_UDMA_2;
-    } else if (id->dma_ultra & 0x0002) {
-	speed = XFER_UDMA_1;
-    } else if (id->dma_ultra & 0x0001) {
-	speed = XFER_UDMA_0;
-    } else if (id->dma_mword & 0x0004) {
-	speed = XFER_MW_DMA_2;
-    } else if (id->dma_mword & 0x0002) {
-	speed = XFER_MW_DMA_1;
-    } else if (id->dma_1word & 0x0004) {
-	speed = XFER_SW_DMA_2;
-    } else {
-	speed = XFER_PIO_0 + ide_get_best_pio_mode(drive, 255, 5, NULL);
-    }
+    speed = ide_find_best_mode(drive, XFER_PIO | XFER_EPIO | XFER_SWDMA | XFER_MWDMA | XFER_UDMA);
 
     (void) it8172_tune_chipset(drive, speed);

drivers/ide/opti621.c +7 -3

diff -urN linux-2.5.6/drivers/ide/opti621.c linux-2.5.6-timing/drivers/ide/opti621.c
@@ -97,7 +97,7 @@
 
 #include <asm/io.h>
 
-#include "ide_modes.h"
+#include "ide-timing.h"
 
 #define OPTI621_MAX_PIO 3
 /* In fact, I do not have any PIO 4 drive

@@ -144,12 +144,16 @@
 	int d;
 	ide_hwif_t *hwif = HWIF(drive);
 
-	drive->drive_data = ide_get_best_pio_mode(drive, pio, OPTI621_MAX_PIO, NULL);
+	if (pio == PIO_DONT_KNOW)
+		drive->drive_data = min(ide_find_best_mode(drive, XFER_PIO | XFER_EPIO) - XFER_PIO_0, OPTI621_MAX_PIO);
+	else
+		drive->drive_data = pio;
+		
 	for (d = 0; d < 2; ++d) {
 		drive = &hwif->drives[d];
 		if (drive->present) {
 			if (drive->drive_data == PIO_DONT_KNOW)
-				drive->drive_data = ide_get_best_pio_mode(drive, 255, OPTI621_MAX_PIO, NULL);
+				drive->drive_data = min(ide_find_best_mode(drive, XFER_PIO | XFER_EPIO) - XFER_PIO_0, OPTI621_MAX_PIO);
 #ifdef OPTI621_DEBUG
 			printk("%s: Selected PIO mode %d\n", drive->name, drive->drive_data);
 #endif

drivers/ide/pdc202xx.c +5 -3

diff -urN linux-2.5.6/drivers/ide/pdc202xx.c linux-2.5.6-timing/drivers/ide/pdc202xx.c
@@ -46,7 +46,7 @@
 #include <asm/io.h>
 #include <asm/irq.h>
 
-#include "ide_modes.h"
+#include "ide-timing.h"
 
 #define PDC202XX_DEBUG_DRIVE_INFO		0
 #define PDC202XX_DECODE_REGISTER_INFO		0

@@ -681,8 +681,10 @@
 {
 	byte speed = 0x00;
 
-	pio = (pio == 5) ? 4 : pio;
-	speed = XFER_PIO_0 + ide_get_best_pio_mode(drive, 255, pio, NULL);
+	if (pio == 255)
+		speed = ide_find_best_mode(drive, XFER_PIO | XFER_EPIO);
+	else
+		speed = XFER_PIO_0 + min_t(byte, pio, 4);
         
 	return ((int) pdc202xx_tune_chipset(drive, speed));
 }

drivers/ide/pdcadma.c +1 -1

diff -urN linux-2.5.6/drivers/ide/pdcadma.c linux-2.5.6-timing/drivers/ide/pdcadma.c
@@ -24,7 +24,7 @@
 #include <asm/io.h>
 #include <asm/irq.h>
 
-#include "ide_modes.h"
+#include "ide-timing.h"
 
 #undef DISPLAY_PDCADMA_TIMINGS

drivers/ide/piix.c +10 ~~-24~~

diff -urN linux-2.5.6/drivers/ide/piix.c linux-2.5.6-timing/drivers/ide/piix.c
@@ -65,7 +65,7 @@
 
 #include <asm/io.h>
 
-#include "ide_modes.h"
+#include "ide-timing.h"
 
 #define PIIX_DEBUG_DRIVE_INFO		0

@@ -225,7 +225,11 @@
 				    { 2, 1 },
 				    { 2, 3 }, };
 
-	pio = ide_get_best_pio_mode(drive, pio, 5, NULL);
+	if (pio == 255)
+		pio = ide_find_best_mode(drive, XFER_PIO | XFER_EPIO) - XFER_PIO_0;
+	else
+		pio = min_t(byte, pio, 4);
+
 	pci_read_config_word(HWIF(drive)->pci_dev, master_port, &master_data);
 	if (is_slave) {
 		master_data = master_data | 0x4000;

@@ -352,27 +356,9 @@
 				   (dev->device == PCI_DEVICE_ID_INTEL_82451NX) ||
 				   (dev->device == PCI_DEVICE_ID_INTEL_82801AB_1)) ? 1 : 0;
 
-	if ((id->dma_ultra & 0x0020) && (udma_66) && (ultra100)) {
-		speed = XFER_UDMA_5;
-	} else if ((id->dma_ultra & 0x0010) && (ultra)) {
-		speed = ((udma_66) && (ultra66)) ? XFER_UDMA_4 : XFER_UDMA_2;
-	} else if ((id->dma_ultra & 0x0008) && (ultra)) {
-		speed = ((udma_66) && (ultra66)) ? XFER_UDMA_3 : XFER_UDMA_1;
-	} else if ((id->dma_ultra & 0x0004) && (ultra)) {
-		speed = XFER_UDMA_2;
-	} else if ((id->dma_ultra & 0x0002) && (ultra)) {
-		speed = XFER_UDMA_1;
-	} else if ((id->dma_ultra & 0x0001) && (ultra)) {
-		speed = XFER_UDMA_0;
-	} else if (id->dma_mword & 0x0004) {
-		speed = XFER_MW_DMA_2;
-	} else if (id->dma_mword & 0x0002) {
-		speed = XFER_MW_DMA_1;
-	} else if (id->dma_1word & 0x0004) {
-		speed = XFER_SW_DMA_2;
-        } else {
-		speed = XFER_PIO_0 + ide_get_best_pio_mode(drive, 255, 5, NULL);
-	}
+	speed = ide_find_best_mode(drive, XFER_PIO | XFER_EPIO | XFER_SWDMA | XFER_MWDMA
+					| (ultra ? XFER_UDMA : 0) | ((udma_66 & ultra66) ? XFER_UDMA_66 : 0)
+					| ((udma_66 & ultra100) ? XFER_UDMA_100 : 0));
 
 	(void) piix_tune_chipset(drive, speed);

@@ -385,7 +371,7 @@
 
 static void config_chipset_for_pio (ide_drive_t *drive)
 {
-	piix_tune_drive(drive, ide_get_best_pio_mode(drive, 255, 5, NULL));
+	piix_tune_drive(drive, ide_find_best_mode(drive, XFER_PIO | XFER_EPIO) - XFER_PIO_0);
 }
 
 static int config_drive_xfer_rate (ide_drive_t *drive)

drivers/ide/qd65xx.c +18 ~~-14~~

diff -urN linux-2.5.6/drivers/ide/qd65xx.c linux-2.5.6-timing/drivers/ide/qd65xx.c
@@ -33,7 +33,7 @@
 #include <linux/init.h>
 #include <asm/io.h>
 
-#include "ide_modes.h"
+#include "ide-timing.h"
 #include "qd65xx.h"
 
 /*

@@ -249,42 +249,46 @@
 
 static void qd6580_tune_drive (ide_drive_t *drive, byte pio)
 {
-	ide_pio_data_t d;
+	struct ide_timing *t;
 	int base = HWIF(drive)->select_data;
 	int active_time   = 175;
 	int recovery_time = 415; /* worst case values from the dos driver */
 
 	if (drive->id && !qd_find_disk_type(drive,&active_time,&recovery_time)) {
-		pio = ide_get_best_pio_mode(drive, pio, 255, &d);
-		pio = min(pio,4);
+
+		if (pio == 255)
+			pio = ide_find_best_mode(drive, XFER_PIO | XFER_EPIO);
+		else
+			pio = XFER_PIO_0 + min_t(byte, pio, 4);
+
+		t = ide_timing_find_mode(pio);
 
 		switch (pio) {
 			case 0: break;
 			case 3:
-				if (d.cycle_time >= 110) {
+				if (t->cycle >= 110) {
 					active_time = 86;
-					recovery_time = d.cycle_time-102;
+					recovery_time = t->cycle-102;
 				} else
 					printk(KERN_WARNING "%s: Strange recovery time !\n",drive->name);
 				break;
 			case 4:
-				if (d.cycle_time >= 69) {
+				if (t->cycle >= 69) {
 					active_time = 70;
-					recovery_time = d.cycle_time-61;
+					recovery_time = t->cycle-61;
 				} else
 					printk(KERN_WARNING "%s: Strange recovery time !\n",drive->name);
 				break;
 			default:
-				if (d.cycle_time >= 180) {
+				if (t->cycle >= 180) {
 					active_time = 110;
-					recovery_time = d.cycle_time - 120;
+					recovery_time = t->cycle - 120;
 				} else {
-					active_time = ide_pio_timings[pio].active_time;
-					recovery_time = d.cycle_time
-							-active_time;
+					active_time = t->active;
+					recovery_time = t->cycle - active_time;
 				}
 		}
-		printk(KERN_INFO "%s: PIO mode%d\n",drive->name,pio);
+		printk(KERN_INFO "%s: PIO mode%d\n", drive->name, pio - XFER_PIO_0);
 	}
 
 	if (!HWIF(drive)->channel && drive->type != ATA_DISK) {

drivers/ide/serverworks.c +7 ~~-26~~

diff -urN linux-2.5.6/drivers/ide/serverworks.c linux-2.5.6-timing/drivers/ide/serverworks.c
@@ -91,7 +91,7 @@
 
 #include <asm/io.h>
 
-#include "ide_modes.h"
+#include "ide-timing.h"
 
 #define DISPLAY_SVWKS_TIMINGS	1
 #undef SVWKS_DEBUG_DRIVE_INFO

@@ -263,7 +263,7 @@
 	byte pio_timing		= 0x00;
 	unsigned short csb5_pio	= 0x00;
 
-	byte pio	= ide_get_best_pio_mode(drive, 255, 5, NULL);
+	byte pio	= ide_find_best_mode(drive, XFER_PIO | XFER_EPIO) - XFER_PIO_0;
 
         switch (drive->dn) {
 		case 0: drive_pci = 0x41; drive_pci2 = 0x45; break;

@@ -364,9 +364,9 @@
 	unsigned short xfer_pio = drive->id->eide_pio_modes;
 	byte timing, speed, pio;
 
-	pio = ide_get_best_pio_mode(drive, 255, 5, NULL);
+	pio = ide_find_best_mode(drive, XFER_PIO | XFER_EPIO) - XFER_PIO_0;
 
-	if (xfer_pio> 4)
+	if (xfer_pio > 4)
 		xfer_pio = 0;
 
 	if (drive->id->eide_pio_iordy > 0)

@@ -417,29 +417,10 @@
 	byte udma_66	= eighty_ninty_three(drive);
 	int ultra66	= (dev->device == PCI_DEVICE_ID_SERVERWORKS_CSB5IDE) ? 1 : 0;
 	int ultra100 	= (ultra66 && svwks_revision >= SVWKS_CSB5_REVISION_NEW) ? 1 : 0;
-	byte speed;
 
-	if ((id->dma_ultra & 0x0020) && (udma_66) && (ultra100)) {
-		speed = XFER_UDMA_5;
-	} else if (id->dma_ultra & 0x0010) {
-		speed = ((udma_66) && (ultra66)) ? XFER_UDMA_4 : XFER_UDMA_2;
-	} else if (id->dma_ultra & 0x0008) {
-		speed = ((udma_66) && (ultra66)) ? XFER_UDMA_3 : XFER_UDMA_1;
-	} else if (id->dma_ultra & 0x0004) {
-		speed = XFER_UDMA_2;
-	} else if (id->dma_ultra & 0x0002) {
-		speed = XFER_UDMA_1;
-	} else if (id->dma_ultra & 0x0001) {
-		speed = XFER_UDMA_0;
-	} else if (id->dma_mword & 0x0004) {
-		speed = XFER_MW_DMA_2;
-	} else if (id->dma_mword & 0x0002) {
-		speed = XFER_MW_DMA_1;
-	} else if (id->dma_1word & 0x0004) {
-		speed = XFER_SW_DMA_2;
-	} else {
-		speed = XFER_PIO_0 + ide_get_best_pio_mode(drive, 255, 5, NULL);
-	}
+	byte speed = ide_find_best_mode(drive, XFER_PIO | XFER_EPIO | XFER_SWDMA | XFER_MWDMA | XFER_UDMA
+				| ((udma_66 && ultra66) ? XFER_UDMA_66 : 0)
+				| ((udma_66 && ultra100) ? XFER_UDMA_100 : 0));
 
 	(void) svwks_tune_chipset(drive, speed);

drivers/ide/sis5513.c +4 -2

diff -urN linux-2.5.6/drivers/ide/sis5513.c linux-2.5.6-timing/drivers/ide/sis5513.c
@@ -26,7 +26,7 @@
 #include <asm/io.h>
 #include <asm/irq.h>
 
-#include "ide_modes.h"
+#include "ide-timing.h"
 
 #define DISPLAY_SIS_TIMINGS
 #define SIS5513_DEBUG_DRIVE_INFO	0

@@ -256,7 +256,9 @@
 	unsigned short xfer_pio = drive->id->eide_pio_modes;
 
 	config_drive_art_rwp(drive);
-	pio = ide_get_best_pio_mode(drive, 255, pio, NULL);
+	
+	if (pio == 255)
+		pio = ide_find_best_mode(drive, XFER_PIO | XFER_EPIO) - XFER_PIO_0;
 
 	if (xfer_pio> 4)
 		xfer_pio = 0;

drivers/ide/sl82c105.c +13 ~~-17~~

diff -urN linux-2.5.6/drivers/ide/sl82c105.c linux-2.5.6-timing/drivers/ide/sl82c105.c
@@ -22,7 +22,7 @@
 #include <asm/io.h>
 #include <asm/dma.h>
 
-#include "ide_modes.h"
+#include "ide-timing.h"
 
 extern char *ide_xfer_verbose (byte xfer_rate);

@@ -31,13 +31,13 @@
  * times for the interface.  This has protection against run-away
  * timings.
  */
-static unsigned int get_timing_sl82c105(ide_pio_data_t *p)
+static unsigned int get_timing_sl82c105(struct ide_timing *t)
 {
 	unsigned int cmd_on;
 	unsigned int cmd_off;
 
-	cmd_on = (ide_pio_timings[p->pio_mode].active_time + 29) / 30;
-	cmd_off = (p->cycle_time - 30 * cmd_on + 29) / 30;
+	cmd_on = (t->active + 29) / 30;
+	cmd_off = (t->cycle - 30 * cmd_on + 29) / 30;
 
 	if (cmd_on > 32)
 		cmd_on = 32;

@@ -49,7 +49,7 @@
 	if (cmd_off == 0)
 		cmd_off = 1;
 
-	return (cmd_on - 1) << 8 | (cmd_off - 1) | (p->use_iordy ? 0x40 : 0x00);
+	return (cmd_on - 1) << 8 | (cmd_off - 1) | ((t->mode > XFER_PIO_2) ? 0x40 : 0x00);
 }
 
 /*

@@ -59,25 +59,21 @@
 {
 	ide_hwif_t *hwif = HWIF(drive);
 	struct pci_dev *dev = hwif->pci_dev;
-	ide_pio_data_t p;
+	struct ide_timing *t;
 	unsigned short drv_ctrl = 0x909;
 	unsigned int xfer_mode, reg;
 
 	reg = (hwif->channel ? 0x4c : 0x44) + (drive->select.b.unit ? 4 : 0);
 
-	pio = ide_get_best_pio_mode(drive, pio, 5, &p);
+	if (pio == 255)
+		xfer_mode = ide_find_best_mode(drive, XFER_PIO | XFER_EPIO);
+	else
+		xfer_mode = XFER_PIO_0 + min_t(byte, pio, 4);
 
-	switch (pio) {
-	default:
-	case 0:		xfer_mode = XFER_PIO_0;		break;
-	case 1:		xfer_mode = XFER_PIO_1;		break;
-	case 2:		xfer_mode = XFER_PIO_2;		break;
-	case 3:		xfer_mode = XFER_PIO_3;		break;
-	case 4:		xfer_mode = XFER_PIO_4;		break;
-	}
+	t = ide_timing_find_mode(xfer_mode);
 
 	if (ide_config_drive_speed(drive, xfer_mode) == 0)
-		drv_ctrl = get_timing_sl82c105(&p);
+		drv_ctrl = get_timing_sl82c105(t);
 
 	if (drive->using_dma == 0) {
 		/*

@@ -89,7 +85,7 @@
 
 		if (report) {
 			printk("%s: selected %s (%dns) (%04X)\n", drive->name,
-			       ide_xfer_verbose(xfer_mode), p.cycle_time, drv_ctrl);
+			       ide_xfer_verbose(xfer_mode), t->cycle, drv_ctrl);
 		}
 	}
 }

drivers/ide/slc90e66.c +10 ~~-22~~

diff -urN linux-2.5.6/drivers/ide/slc90e66.c linux-2.5.6-timing/drivers/ide/slc90e66.c
@@ -48,7 +48,7 @@
 
 #include <asm/io.h>
 
-#include "ide_modes.h"
+#include "ide-timing.h"
 
 #define SLC90E66_DEBUG_DRIVE_INFO		0

@@ -202,7 +202,11 @@
 				    { 2, 1 },
 				    { 2, 3 }, };
 
-	pio = ide_get_best_pio_mode(drive, pio, 5, NULL);
+	if (pio == 255)
+		pio = ide_find_best_mode(drive, XFER_PIO | XFER_EPIO) - XFER_PIO_0;
+	else
+		pio = min_t(byte, pio, 4);
+
 	pci_read_config_word(HWIF(drive)->pci_dev, master_port, &master_data);
 	if (is_slave) {
 		master_data = master_data | 0x4000;

@@ -299,30 +303,14 @@
 
 #if 1 /* allow PIO modes */
 	if (!HWIF(drive)->autodma) {
-		speed = XFER_PIO_0 + ide_get_best_pio_mode(drive, 255, 5, NULL);
+		speed = ide_find_best_mode(drive, XFER_PIO | XFER_EPIO);
 		(void) slc90e66_tune_chipset(drive, speed);
 		return ((int) ide_dma_off_quietly);
 	}
 #endif
-	if ((id->dma_ultra & 0x0010) && (ultra)) {
-		speed = (udma_66) ? XFER_UDMA_4 : XFER_UDMA_2;
-	} else if ((id->dma_ultra & 0x0008) && (ultra)) {
-		speed = (udma_66) ? XFER_UDMA_3 : XFER_UDMA_1;
-	} else if ((id->dma_ultra & 0x0004) && (ultra)) {
-		speed = XFER_UDMA_2;
-	} else if ((id->dma_ultra & 0x0002) && (ultra)) {
-		speed = XFER_UDMA_1;
-	} else if ((id->dma_ultra & 0x0001) && (ultra)) {
-		speed = XFER_UDMA_0;
-	} else if (id->dma_mword & 0x0004) {
-		speed = XFER_MW_DMA_2;
-	} else if (id->dma_mword & 0x0002) {
-		speed = XFER_MW_DMA_1;
-	} else if (id->dma_1word & 0x0004) {
-		speed = XFER_SW_DMA_2;
-        } else {
-		speed = XFER_PIO_0 + ide_get_best_pio_mode(drive, 255, 5, NULL);
-	}
+
+	speed = ide_find_best_mode(drive, XFER_PIO | XFER_EPIO | XFER_SWDMA | XFER_MWDMA 
+					| (ultra ? XFER_UDMA : 0) | ((ultra && udma_66) ? XFER_UDMA_66 : 0));
 
 	(void) slc90e66_tune_chipset(drive, speed);

drivers/ide/umc8672.c +6 -2

diff -urN linux-2.5.6/drivers/ide/umc8672.c linux-2.5.6-timing/drivers/ide/umc8672.c
@@ -52,7 +52,7 @@
 
 #include <asm/io.h>
 
-#include "ide_modes.h"
+#include "ide-timing.h"
 
 /*
  * Default speeds.  These can be changed with "auto-tune" and/or hdparm.

@@ -113,7 +113,11 @@
 	unsigned long flags;
 	ide_hwgroup_t *hwgroup = ide_hwifs[HWIF(drive)->index^1].hwgroup;
 
-	pio = ide_get_best_pio_mode(drive, pio, 4, NULL);
+	if (pio == 255)
+		pio = ide_find_best_mode(drive, XFER_PIO | XFER_EPIO) - XFER_PIO_0;
+	else
+		pio = min_t(byte, pio, 4);
+
 	printk("%s: setting umc8672 to PIO mode%d (speed %d)\n", drive->name, pio, pio_to_umc[pio]);
 	save_flags(flags);	/* all CPUs */
 	cli();			/* all CPUs */

drivers/ide/via82cxxx.c +1 -1

diff -urN linux-2.5.6/drivers/ide/via82cxxx.c linux-2.5.6-timing/drivers/ide/via82cxxx.c
@@ -352,7 +352,7 @@
 		return;
 	}
 
-	via_set_drive(drive, XFER_PIO_0 + MIN(pio, 5));
+	via_set_drive(drive, XFER_PIO_0 + min_t(byte, pio, 5));
 }
 
 #ifdef CONFIG_BLK_DEV_IDEDMA

Martin Dalecki

2002-03-12 16:12 UTC #88 lore raw

Quoted 22 lines · Vojtech Pavlik

Quoted 8 lines · Martin Dalecki
Hello Vojtech.
I have noticed that the ide-timings.h and ide_modules.h are running much in aprallel in the purpose they serve. Are the any chances you could dare to care about propagating the fairly nice ide-timings.h stuff in favour of ide_modules.h more.
Quoted 7 lines
BTW.> I think some stuff from ide-timings.h just belongs as generic functions intro ide.c, and right now there is nobody who you need to work from behind ;-).
So please feel free to do the changes you apparently desired to do a long time ago...
Hmm, ok. Try this. It shouldn't change any functionality, yet makes a small step towards cleaning the chipset specific drivers.

Quoted 6 lines

Reading through them as I was doing the changes, I found out that most of them compute the timings incorrectly. Because of that I also removed the pio blacklist (which is going to come back in a more powerful form, merged together with the DMA blacklist), because that one is based on ancient experiments with the broken CMD640 chip and a driver which doesn't get the timings correct either. The blacklist is plain invalid.

Amen to this. May "the force" be with you! (I mean the force in you fingers!)

AS you may know I was once (an eon ago)
during the Marc Lord "era" involved in the initial developement of the cmd640 support. And well we got it working, but after that some friend got to the idea of the black list and my disk went from georgious 5M/sec to only lame 2.8M/sec rates (remember it was a conner 400MB drive then one of those "buggy" Quantums!) for no good reason. I was long time patching every single kernel those time for this. So if anything I very well know that the list found there is both:
obsolete and invalid. Further on my CMD640 code wasn't even trying to compute the timing values in any dynamic ways. I was just using the original tables from CMD directly, but unfortunately the maintainer enjoyed Z/ ring arithmetics too much ;-)

Quoted 2 lines

I plan to focus on the most important drivers first, to fix and clean them, working with the authors where possible.

PIIX na VIA comes to mind first ;-)...

Martin Dalecki

2002-03-12 16:19 UTC #89 lore raw

Quoted 15 lines · Vojtech Pavlik

Quoted 12 lines · Martin Dalecki
Hello Vojtech.
I have noticed that the ide-timings.h and ide_modules.h are running much in aprallel in the purpose they serve. Are the any chances you could dare to care about propagating the fairly nice ide-timings.h stuff in favour of ide_modules.h more.
BTW.> I think some stuff from ide-timings.h just belongs as generic functions intro ide.c, and right now there is nobody who you need to work from behind ;-).

Quoted 6 lines

Quoted 3 lines
So please feel free to do the changes you apparently desired to do a long time ago...
Hmm, ok. Try this. It shouldn't change any functionality, yet makes a small step towards cleaning the chipset specific drivers.

OK the patch looks fine. Taken. Still I have some notes:

1. Let's start calling stuff ATA and not IDE. (AT-Attachment is it and not just Integrated Device Electornics.) OK?

2. I quite don't like the nested #include directives in ide-timing.h. It's cleaner to include the needed headers in front of usage of ide-timing.h. (Just s small note.... not really important...)

3. I wellcome that the MIN MAX macros there are gone. In fact I have yerstoday just done basically the same ;-). (Will just have to revert it now.

Patch swallowed.

Vojtech Pavlik

author

2002-03-12 16:21 UTC #90 lore raw

Quoted 9 lines · Martin Dalecki

Quoted 6 lines
Reading through them as I was doing the changes, I found out that most of them compute the timings incorrectly. Because of that I also removed the pio blacklist (which is going to come back in a more powerful form, merged together with the DMA blacklist), because that one is based on ancient experiments with the broken CMD640 chip and a driver which doesn't get the timings correct either. The blacklist is plain invalid.
Amen to this. May "the force" be with you! (I mean the force in you fingers!)

Quoted 11 lines

AS you may know I was once (an eon ago)
during the Marc Lord "era" involved in the initial developement of the cmd640 support. And well we got it working, but after that some friend got to the idea of the black list and my disk went from georgious 5M/sec to only lame 2.8M/sec rates (remember it was a conner 400MB drive then one of those "buggy" Quantums!) for no good reason. I was long time patching every single kernel those time for this. So if anything I very well know that the list found there is both:
obsolete and invalid. Further on my CMD640 code wasn't even trying to compute the timing values in any dynamic ways. I was just using the original tables from CMD directly, but unfortunately the maintainer enjoyed Z/ ring arithmetics too much ;-)

Well, as much as I'd like to use safe pre-computed register values for the chips, that ain't possible - even when we assumed the system bus (PCI, VLB, whatever) was always 33 MHz, still the drives have various ideas about what DMA and PIO modes should look like, see the tDMA and tPIO entries in hdparm -t.

So, arithmetics has to stay. Hopefully just one instance in ide-timing.c.

Quoted 4 lines

Quoted 2 lines
I plan to focus on the most important drivers first, to fix and clean them, working with the authors where possible.
PIIX na VIA comes to mind first ;-)...

VIA is already OK, well, it has my name in it. :) AMD is now also (well, that one wasn't broken, just ugly), SiS is being revamped by Lionel Bouton (whom I'm trying to help as much as I can), so yes, PIIX would be next.

PIIX and ICH are pretty crazy hardware from the design perspective, very legacy-bound back to the first Intel PIIX chip. And the driver for these in the kernel has similarly evolved following the hardware. However, it doesn't seem to be wrong at the first glance. Nevertheless, I'll take a look at it. Unfortunately, I don't have any Intel hardware at hand to test it with.

Signature · 2 lines

Vojtech Pavlik
SuSE Labs

Sebastian Droege

2002-03-12 16:40 UTC #91 lore raw

Hi,

Quoted 11 lines · On Tue, 12 Mar 2002 17:21:34 +0100 Vojtech Pavlik <[email protected]> wrote:

VIA is already OK, well, it has my name in it. :) AMD is now also (well, that one wasn't broken, just ugly), SiS is being revamped by Lionel Bouton (whom I'm trying to help as much as I can), so yes, PIIX would be next.
PIIX and ICH are pretty crazy hardware from the design perspective, very legacy-bound back to the first Intel PIIX chip. And the driver for these in the kernel has similarly evolved following the hardware. However, it doesn't seem to be wrong at the first glance. Nevertheless, I'll take a look at it. Unfortunately, I don't have any Intel hardware at hand to test it with.

I have one Intel Corp. 82371AB PIIX4 IDE (rev 01) here and I can test it for you. This machine isn't very important so it doesn't matter if some data gets shredded ;) Just send me the patch when you've finished it.

Bye

Bill Davidsen

2002-03-13 19:45 UTC #92 lore raw

Quoted 6 lines · Vojtech Pavlik

PIIX and ICH are pretty crazy hardware from the design perspective, very legacy-bound back to the first Intel PIIX chip. And the driver for these in the kernel has similarly evolved following the hardware. However, it doesn't seem to be wrong at the first glance. Nevertheless, I'll take a look at it. Unfortunately, I don't have any Intel hardware at hand to test it with.

I'm not sure "evolved" is the term you want, allow me to suggest "mutated."

Signature · 3 lines

bill davidsen <[email protected]>
  CTO, TMR Associates, Inc
Doing interesting things with little computers since 1979.

Martin Dalecki

2002-03-12 16:27 UTC #93 lore raw

Quoted 5 lines · Vojtech Pavlik

Well, as much as I'd like to use safe pre-computed register values for the chips, that ain't possible - even when we assumed the system bus (PCI, VLB, whatever) was always 33 MHz, still the drives have various ideas about what DMA and PIO modes should look like, see the tDMA and tPIO entries in hdparm -t.

Yes yes yes of course some of the drivers are confused. And I don't argue that precomputation is adequate right now. It just wasn't for the CMD640 those times... I only wanted to reffer to history and why my timings where different then the computed.

Quoted 11 lines

So, arithmetics has to stay. Hopefully just one instance in ide-timing.c.
Quoted 5 lines
Quoted 3 lines
I plan to focus on the most important drivers first, to fix and clean them, working with the authors where possible.
PIIX na VIA comes to mind first ;-)...
VIA is already OK, well, it has my name in it. :) AMD is now also (well,

Oh of course I was reffering to VIA as important.

Quoted 3 lines

that one wasn't broken, just ugly), SiS is being revamped by Lionel Bouton (whom I'm trying to help as much as I can), so yes, PIIX would be next.

I swallowed his SiS stuff already, since my home main machine is
a SiS735 based board. (Awfoul cheap that thing and quite good price/performance ratio ;-).

Quoted 2 lines

PIIX and ICH are pretty crazy hardware from the design perspective, very legacy-bound back to the first Intel PIIX chip. And the driver for these

Yes I "love" the bound together DMA areas as well ;-).

Quoted 4 lines

in the kernel has similarly evolved following the hardware. However, it doesn't seem to be wrong at the first glance. Nevertheless, I'll take a look at it. Unfortunately, I don't have any Intel hardware at hand to test it with.

Just another hint: dmaproc is silly nomenclature is should be dma_strategy <- this would better reflect it's purpose.

Vojtech Pavlik

author

2002-03-12 16:28 UTC #94 lore raw

Quoted 20 lines · Martin Dalecki

Quoted 19 lines · Vojtech Pavlik
Quoted 15 lines · Martin Dalecki
Hello Vojtech.
I have noticed that the ide-timings.h and ide_modules.h are running much in aprallel in the purpose they serve. Are the any chances you could dare to care about propagating the fairly nice ide-timings.h stuff in favour of ide_modules.h more.
BTW.> I think some stuff from ide-timings.h just belongs as generic functions intro ide.c, and right now there is nobody who you need to work from behind ;-).
So please feel free to do the changes you apparently desired to do a long time ago...

Quoted 8 lines

Quoted 2 lines
Hmm, ok. Try this. It shouldn't change any functionality, yet makes a small step towards cleaning the chipset specific drivers.
OK the patch looks fine. Taken. Still I have some notes:
1. Let's start calling stuff ATA and not IDE. (AT-Attachment is it and not just Integrated Device Electornics.) OK?

Sure. Feel free to rename whatever file/struct/variable you want. In my opinion, it's just not worth caring about whether we call the stuff ATA or IDE, both are TLAs with a long history. (Hmm, ATA probably means Advanced Technology Attachment, right?)

But for new stuff, I'll try to stick with the 'ata' name.

Quoted 3 lines

2. I quite don't like the nested #include directives in ide-timing.h. It's cleaner to include the needed headers in front of usage of ide-timing.h. (Just s small note.... not really important...)

I can change that, or you can. I think the hdreg.h one is reasonable, while the ide.h can probably go without any significant trouble. I think the only file that'll need adding #include <ide.h> will be ide-timing.c

Quoted 5 lines

3. I wellcome that the MIN MAX macros there are gone. In fact I have yerstoday just done basically the same ;-). (Will just have to revert it now.
Patch swallowed.

Signature · 2 lines

Vojtech Pavlik
SuSE Labs

Vojtech Pavlik

author

2002-03-12 16:33 UTC #95 lore raw

Quoted 9 lines · Martin Dalecki

Quoted 5 lines · Vojtech Pavlik
Well, as much as I'd like to use safe pre-computed register values for the chips, that ain't possible - even when we assumed the system bus (PCI, VLB, whatever) was always 33 MHz, still the drives have various ideas about what DMA and PIO modes should look like, see the tDMA and tPIO entries in hdparm -t.

Quoted 4 lines

Yes yes yes of course some of the drivers are confused. And I don't argue that precomputation is adequate right now. It just wasn't for the CMD640 those times... I only wanted to reffer to history and why my timings where different then the computed.

We may want to compare your original timings to what ide-timing.[ch] will compute ...

Quoted 14 lines

Quoted 11 lines
So, arithmetics has to stay. Hopefully just one instance in ide-timing.c.
Quoted 5 lines
Quoted 3 lines
I plan to focus on the most important drivers first, to fix and clean them, working with the authors where possible.
PIIX na VIA comes to mind first ;-)...
VIA is already OK, well, it has my name in it. :) AMD is now also (well,
Oh of course I was reffering to VIA as important.

Quoted 7 lines

Quoted 3 lines
that one wasn't broken, just ugly), SiS is being revamped by Lionel Bouton (whom I'm trying to help as much as I can), so yes, PIIX would be next.
I swallowed his SiS stuff already, since my home main machine is
a SiS735 based board. (Awfoul cheap that thing and quite good price/performance ratio ;-).

As you can guess, I have mostly VIA machines around here. One SiS, too, but that's an embedded board.

Quoted 4 lines

Quoted 2 lines
PIIX and ICH are pretty crazy hardware from the design perspective, very legacy-bound back to the first Intel PIIX chip. And the driver for these
Yes I "love" the bound together DMA areas as well ;-).

And the SIDETIM register is just a kyooot idea. :)

Quoted 7 lines

Quoted 4 lines
in the kernel has similarly evolved following the hardware. However, it doesn't seem to be wrong at the first glance. Nevertheless, I'll take a look at it. Unfortunately, I don't have any Intel hardware at hand to test it with.
Just another hint: dmaproc is silly nomenclature is should be dma_strategy <- this would better reflect it's purpose.

Dmaproc? Well that's a do-everything-possible-with-default-fallbacks function. Quite an interesting approach, though I'd be much happier without it.

Btw, do you know that the chipset and drives are tuned twice each boot? First they're configured to PIO and half a second later to DMA (or PIO) again ...

Signature · 2 lines

Vojtech Pavlik
SuSE Labs

Martin Dalecki

2002-03-12 16:34 UTC #96 lore raw

Quoted 10 lines · Vojtech Pavlik

Quoted 5 lines
OK the patch looks fine. Taken. Still I have some notes:
1. Let's start calling stuff ATA and not IDE. (AT-Attachment is it and not just Integrated Device Electornics.) OK?
Sure. Feel free to rename whatever file/struct/variable you want. In my opinion, it's just not worth caring about whether we call the stuff ATA or IDE, both are TLAs with a long history. (Hmm, ATA probably means Advanced Technology Attachment, right?)

Basically I would like to try to follow the FreeBSD nomenclature as much as possible, becouse:

1. I started doing Linux, becouse I like UNIX ;-).
2. It helps (in some distant time) to look at both side by side.
3. It helps to see what has changed "recently".
4. "Learning from the Soviet Union is learning about victory"... just kidding I'm. But you are from .cz so you already know ;-).

Martin Dalecki

2002-03-12 16:43 UTC #97 lore raw

Quoted 66 lines · Vojtech Pavlik

Quoted 10 lines · Martin Dalecki
Quoted 6 lines · Vojtech Pavlik
Well, as much as I'd like to use safe pre-computed register values for the chips, that ain't possible - even when we assumed the system bus (PCI, VLB, whatever) was always 33 MHz, still the drives have various ideas about what DMA and PIO modes should look like, see the tDMA and tPIO entries in hdparm -t.
Quoted 5 lines
Yes yes yes of course some of the drivers are confused. And I don't argue that precomputation is adequate right now. It just wasn't for the CMD640 those times... I only wanted to reffer to history and why my timings where different then the computed.
We may want to compare your original timings to what ide-timing.[ch] will compute ...
Quoted 17 lines
Quoted 5 lines
So, arithmetics has to stay. Hopefully just one instance in ide-timing.c.
Quoted 9 lines
Quoted 7 lines
Quoted 4 lines
I plan to focus on the most important drivers first, to fix and clean them, working with the authors where possible.
PIIX na VIA comes to mind first ;-)...
VIA is already OK, well, it has my name in it. :) AMD is now also (well,
Oh of course I was reffering to VIA as important.
Quoted 8 lines
Quoted 4 lines
that one wasn't broken, just ugly), SiS is being revamped by Lionel Bouton (whom I'm trying to help as much as I can), so yes, PIIX would be next.
I swallowed his SiS stuff already, since my home main machine is
a SiS735 based board. (Awfoul cheap that thing and quite good price/performance ratio ;-).
As you can guess, I have mostly VIA machines around here. One SiS, too, but that's an embedded board.
Quoted 5 lines
Quoted 3 lines
PIIX and ICH are pretty crazy hardware from the design perspective, very legacy-bound back to the first Intel PIIX chip. And the driver for these
Yes I "love" the bound together DMA areas as well ;-).
And the SIDETIM register is just a kyooot idea. :)
Quoted 5 lines
Quoted 5 lines
in the kernel has similarly evolved following the hardware. However, it doesn't seem to be wrong at the first glance. Nevertheless, I'll take a look at it. Unfortunately, I don't have any Intel hardware at hand to test it with.

Quoted 7 lines

Quoted 3 lines
Just another hint: dmaproc is silly nomenclature is should be dma_strategy <- this would better reflect it's purpose.
Dmaproc? Well that's a do-everything-possible-with-default-fallbacks function. Quite an interesting approach, though I'd be much happier without it.

Yeep. As an added bonus it's error prone, if some implementation of this method forgets to cut and paste the fallback code.
You noticed the way I made at least the file_operations dispatcher looking "opaque" recently? Most visible was the vanishment of dome

if (ban->bang == NULL)
bang = default1
if (ban->bang == NULL)
bang = default1
if (ban->bang == NULL)
bang = default1
if (ban->bang == NULL)
bang = default1
if (ban->bang == NULL)
bang = default1
if (ban->bang == NULL)
bang = default1
....

orgy upon "disk" initialization...

Quoted 3 lines

Btw, do you know that the chipset and drives are tuned twice each boot? First they're configured to PIO and half a second later to DMA (or PIO) again ...

Yes I know ;-). I have just removed some other twice callers for other stuff. Do you know for example that I'm about 90% sure that cleanup hooks for the device category drivers can be called twice. Or that adding just a single drive reiterates every one on the system?

First I will try to change the way disks get iterated. This is becouse disks are the main object in the whole driver and not the host chip channels... Look for the plenty loops with MAX_HWIFS as upper bound to see what I mean.

"Tune" is a silly name as well BTW. It has nothing to do with attaching of carbon fiber to cars. It's host chip initialization, which is going on.

Martin Dalecki

2002-03-12 16:46 UTC #98 lore raw

Quoted 12 lines · Vojtech Pavlik

Quoted 10 lines · Martin Dalecki
Quoted 6 lines · Vojtech Pavlik
Well, as much as I'd like to use safe pre-computed register values for the chips, that ain't possible - even when we assumed the system bus (PCI, VLB, whatever) was always 33 MHz, still the drives have various ideas about what DMA and PIO modes should look like, see the tDMA and tPIO entries in hdparm -t.

Quoted 8 lines

Quoted 5 lines
Yes yes yes of course some of the drivers are confused. And I don't argue that precomputation is adequate right now. It just wasn't for the CMD640 those times... I only wanted to reffer to history and why my timings where different then the computed.
We may want to compare your original timings to what ide-timing.[ch] will compute ...

Unfortunately there is no chance. I have abondony this board quite happy a long time ago... It was an 486 and I don't keep old shread around. Sorry I just don't have it at hand anylonger.

Vojtech Pavlik

author

2002-03-12 16:51 UTC #99 lore raw

Quoted 19 lines · Martin Dalecki

Quoted 18 lines · Vojtech Pavlik
Quoted 15 lines · Martin Dalecki
Quoted 6 lines · Vojtech Pavlik
Well, as much as I'd like to use safe pre-computed register values for the chips, that ain't possible - even when we assumed the system bus (PCI, VLB, whatever) was always 33 MHz, still the drives have various ideas about what DMA and PIO modes should look like, see the tDMA and tPIO entries in hdparm -t.
Yes yes yes of course some of the drivers are confused. And I don't argue that precomputation is adequate right now. It just wasn't for the CMD640 those times... I only wanted to reffer to history and why my timings where different then the computed.

Quoted 6 lines

Quoted 2 lines
We may want to compare your original timings to what ide-timing.[ch] will compute ...
Unfortunately there is no chance. I have abondony this board quite happy a long time ago... It was an 486 and I don't keep old shread around. Sorry I just don't have it at hand anylonger.

You may happen to have the numbers, though - that should be enough.

Btw, I have a CMD640B based PCI card lying around here, but never managed to get it generate any interrupts, though the rest seems to be working.

Signature · 2 lines

Vojtech Pavlik
SuSE Labs

Martin Dalecki

2002-03-12 16:59 UTC #100 lore raw

Quoted 33 lines · Vojtech Pavlik

Quoted 23 lines · Martin Dalecki
Quoted 21 lines · Vojtech Pavlik
Quoted 18 lines · Martin Dalecki
Quoted 7 lines · Vojtech Pavlik
Well, as much as I'd like to use safe pre-computed register values for the chips, that ain't possible - even when we assumed the system bus (PCI, VLB, whatever) was always 33 MHz, still the drives have various ideas about what DMA and PIO modes should look like, see the tDMA and tPIO entries in hdparm -t.
Yes yes yes of course some of the drivers are confused. And I don't argue that precomputation is adequate right now. It just wasn't for the CMD640 those times... I only wanted to reffer to history and why my timings where different then the computed.
Quoted 7 lines
Quoted 3 lines
We may want to compare your original timings to what ide-timing.[ch] will compute ...
Unfortunately there is no chance. I have abondony this board quite happy a long time ago... It was an 486 and I don't keep old shread around. Sorry I just don't have it at hand anylonger.

Quoted 5 lines

You may happen to have the numbers, though - that should be enough.
Btw, I have a CMD640B based PCI card lying around here, but never managed to get it generate any interrupts, though the rest seems to be working.

I remember that I have found them in some readme attached to the original dos drivers from CMD. Maybe it's still availabe on the net but really I don't have it anylongeranywhere archieved. And BTW.> The CMD640B was the variant which wasn't physically broken, as the CMD640 which had improper bus termination of the secondary channel.

Vojtech Pavlik

author

2002-03-12 20:00 UTC #101 lore raw

PATCH 1/1 PIIX driver rewrite

Quoted 8 lines · Martin Dalecki

Hello Vojtech.
I have noticed that the ide-timings.h and ide_modules.h are running much in aprallel in the purpose they serve. Are the any chances you could dare to care about propagating the fairly nice ide-timings.h stuff in favour of ide_modules.h more.

Quoted 6 lines

BTW.> I think some stuff from ide-timings.h just belongs as generic functions intro ide.c, and right now there is nobody who you need to work from behind ;-).
So please feel free to do the changes you apparently desired to do a long time ago...

Oh, by the way, here goes the PIIX rewrite ... unlike the AMD one, this is completely untested, and may not work at all - I only have the datasheets at hand, no PIIX hardware.

Differences from the previous PIIX driver:

* 82451NX MIOC isn't supported anymore. It's not an ATA controller, anyway ;)
* 82371FB_0 PIIX ISA bridge isn't an ATA controller either.
* 82801CA ICH3 support added. Only ICH3-M is supported by the original driver.
* 82371MX MPIIX is not supported anymore. Too weird beast and doesn't do DMA anyway, better handled by the generic PCI ATA routines.

* Cleaner, converted to ide-timing.[ch]

* May not work. ;)

Signature · 2 lines

Vojtech Pavlik
SuSE Labs

drivers/ide/ide-pci.c +13 ~~-13~~

diff -urN linux-2.5.6-timing/drivers/ide/ide-pci.c linux-2.5.6-piix/drivers/ide/ide-pci.c
@@ -119,6 +119,7 @@
 extern unsigned int pci_init_piix(struct pci_dev *);
 extern unsigned int ata66_piix(ide_hwif_t *);
 extern void ide_init_piix(ide_hwif_t *);
+extern void ide_dmacapable_piix(ide_hwif_t *, unsigned long);
 #endif
 
 #ifdef CONFIG_BLK_DEV_IT8172

@@ -197,19 +198,17 @@
 
 static ide_pci_device_t pci_chipsets[] __initdata = {
 #ifdef CONFIG_BLK_DEV_PIIX
-	{PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_82371FB_0, NULL, NULL, ide_init_piix,	NULL, {{0x41,0x80,0x80}, {0x43,0x80,0x80}}, ON_BOARD, 0, 0 },
-	{PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_82371FB_1, NULL, NULL, ide_init_piix,	NULL, {{0x41,0x80,0x80}, {0x43,0x80,0x80}}, ON_BOARD, 0, 0 },
-	{PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_82371MX, NULL, NULL, ide_init_piix, NULL, {{0x6D,0x80,0x80}, {0x6F,0x80,0x80}}, ON_BOARD, 0, ATA_F_NODMA },
-	{PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_82371SB_1, pci_init_piix, NULL, ide_init_piix, NULL, {{0x41,0x80,0x80}, {0x43,0x80,0x80}}, ON_BOARD, 0, 0 },
-	{PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_82371AB, pci_init_piix, NULL,	ide_init_piix, NULL, {{0x41,0x80,0x80}, {0x43,0x80,0x80}}, ON_BOARD, 0, 0 },
-	{PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_82801AB_1, pci_init_piix, NULL, ide_init_piix, NULL, {{0x41,0x80,0x80}, {0x43,0x80,0x80}}, ON_BOARD, 0, 0 },
-	{PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_82443MX_1, pci_init_piix, NULL, ide_init_piix, NULL, {{0x41,0x80,0x80}, {0x43,0x80,0x80}}, ON_BOARD, 0, 0 },
-	{PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_82801AA_1, pci_init_piix, ata66_piix, ide_init_piix, NULL, {{0x41,0x80,0x80}, {0x43,0x80,0x80}}, ON_BOARD, 0, 0 },
-	{PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_82372FB_1, pci_init_piix, ata66_piix, ide_init_piix, NULL, {{0x41,0x80,0x80}, {0x43,0x80,0x80}}, ON_BOARD, 0, 0 },
-	{PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_82451NX, pci_init_piix, NULL,	ide_init_piix, NULL, {{0x41,0x80,0x80}, {0x43,0x80,0x80}}, ON_BOARD, 0, ATA_F_NOADMA },
-	{PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_82801BA_9, pci_init_piix, ata66_piix,	ide_init_piix, NULL, {{0x41,0x80,0x80}, {0x43,0x80,0x80}}, ON_BOARD, 0, 0 },
-	{PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_82801BA_8, pci_init_piix, ata66_piix,	ide_init_piix, NULL, {{0x41,0x80,0x80}, {0x43,0x80,0x80}}, ON_BOARD, 0, 0 },
-	{PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_82801CA_10, pci_init_piix, ata66_piix, ide_init_piix,	NULL, {{0x41,0x80,0x80}, {0x43,0x80,0x80}}, ON_BOARD, 0, 0 },
+	{PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_82371FB_1, pci_init_piix, ata66_piix, ide_init_piix, ide_dmacapable_piix, {{0x41,0x80,0x80}, {0x43,0x80,0x80}}, ON_BOARD, 0, 0 },
+	{PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_82371SB_1, pci_init_piix, ata66_piix, ide_init_piix, ide_dmacapable_piix, {{0x41,0x80,0x80}, {0x43,0x80,0x80}}, ON_BOARD, 0, 0 },
+	{PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_82371AB, pci_init_piix, ata66_piix, ide_init_piix, ide_dmacapable_piix, {{0x41,0x80,0x80}, {0x43,0x80,0x80}}, ON_BOARD, 0, 0 },
+	{PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_82443MX_1, pci_init_piix, ata66_piix, ide_init_piix, ide_dmacapable_piix, {{0x41,0x80,0x80}, {0x43,0x80,0x80}}, ON_BOARD, 0, 0 },
+	{PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_82372FB_1, pci_init_piix, ata66_piix, ide_init_piix, ide_dmacapable_piix, {{0x41,0x80,0x80}, {0x43,0x80,0x80}}, ON_BOARD, 0, 0 },
+	{PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_82801AA_1, pci_init_piix, ata66_piix, ide_init_piix, ide_dmacapable_piix, {{0x41,0x80,0x80}, {0x43,0x80,0x80}}, ON_BOARD, 0, 0 },
+	{PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_82801AB_1, pci_init_piix, ata66_piix, ide_init_piix, ide_dmacapable_piix, {{0x41,0x80,0x80}, {0x43,0x80,0x80}}, ON_BOARD, 0, 0 },
+	{PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_82801BA_9, pci_init_piix, ata66_piix, ide_init_piix, ide_dmacapable_piix, {{0x41,0x80,0x80}, {0x43,0x80,0x80}}, ON_BOARD, 0, 0 },
+	{PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_82801BA_8, pci_init_piix, ata66_piix, ide_init_piix, ide_dmacapable_piix, {{0x41,0x80,0x80}, {0x43,0x80,0x80}}, ON_BOARD, 0, 0 },
+	{PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_82801CA_10, pci_init_piix, ata66_piix, ide_init_piix, ide_dmacapable_piix, {{0x41,0x80,0x80}, {0x43,0x80,0x80}}, ON_BOARD, 0, 0 },
+	{PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_82801CA_11, pci_init_piix, ata66_piix, ide_init_piix, ide_dmacapable_piix, {{0x41,0x80,0x80}, {0x43,0x80,0x80}}, ON_BOARD, 0, 0 },
 #endif
 #ifdef CONFIG_BLK_DEV_VIA82CXXX
 	{PCI_VENDOR_ID_VIA, PCI_DEVICE_ID_VIA_82C576_1,	pci_init_via82cxxx, ata66_via82cxxx, ide_init_via82cxxx, ide_dmacapable_via82cxxx, {{0x40,0x02,0x02}, {0x40,0x01,0x01}}, ON_BOARD, 0, ATA_F_NOADMA },

@@ -311,6 +310,7 @@
 	{PCI_VENDOR_ID_NS, PCI_DEVICE_ID_NS_87410, NULL, NULL, NULL, NULL, {{0x43,0x08,0x08}, {0x47,0x08,0x08}}, ON_BOARD, 0, 0 },
 	{PCI_VENDOR_ID_HINT, PCI_DEVICE_ID_HINT, NULL, NULL, NULL, NULL, {{0x00,0x00,0x00}, {0x00,0x00,0x00}}, ON_BOARD, 0, 0 },
 	{PCI_VENDOR_ID_HOLTEK, PCI_DEVICE_ID_HOLTEK_6565, NULL, NULL, NULL, NULL, {{0x00,0x00,0x00}, {0x00,0x00,0x00}}, ON_BOARD, 0, 0 },
+	{PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_82371MX, NULL, NULL, NULL, NULL, {{0x6D,0x80,0x80}, {0x00,0x00,0x00}}, ON_BOARD, 0, ATA_F_NODMA },
 	{PCI_VENDOR_ID_UMC, PCI_DEVICE_ID_UMC_UM8673F, NULL, NULL, NULL, NULL, {{0x00,0x00,0x00}, {0x00,0x00,0x00}}, ON_BOARD, 0, ATA_F_FIXIRQ },
 	{PCI_VENDOR_ID_UMC, PCI_DEVICE_ID_UMC_UM8886A, NULL, NULL, NULL, NULL, {{0x00,0x00,0x00}, {0x00,0x00,0x00}}, ON_BOARD, 0, ATA_F_FIXIRQ },
 	{PCI_VENDOR_ID_UMC, PCI_DEVICE_ID_UMC_UM8886BF, NULL, NULL, NULL, NULL, {{0x00,0x00,0x00}, {0x00,0x00,0x00}}, ON_BOARD, 0, ATA_F_FIXIRQ },

drivers/ide/piix.c +435 ~~-424~~

934 lines not on this page · raw message

Sebastian Droege

2002-03-12 20:31 UTC #102 lore raw

Re: [patch] PIIX driver rewrite

Quoted 28 lines · On Tue, 12 Mar 2002 21:00:35 +0100 Vojtech Pavlik <[email protected]> wrote:

Quoted 8 lines · Martin Dalecki
Hello Vojtech.
I have noticed that the ide-timings.h and ide_modules.h are running much in aprallel in the purpose they serve. Are the any chances you could dare to care about propagating the fairly nice ide-timings.h stuff in favour of ide_modules.h more.
Quoted 6 lines
BTW.> I think some stuff from ide-timings.h just belongs as generic functions intro ide.c, and right now there is nobody who you need to work from behind ;-).
So please feel free to do the changes you apparently desired to do a long time ago...
Oh, by the way, here goes the PIIX rewrite ... unlike the AMD one, this is completely untested, and may not work at all - I only have the datasheets at hand, no PIIX hardware.
Differences from the previous PIIX driver:
* 82451NX MIOC isn't supported anymore. It's not an ATA controller, anyway ;)
* 82371FB_0 PIIX ISA bridge isn't an ATA controller either.
* 82801CA ICH3 support added. Only ICH3-M is supported by the original driver.
* 82371MX MPIIX is not supported anymore. Too weird beast and doesn't do DMA anyway, better handled by the generic PCI ATA routines.

Quoted 3 lines

* Cleaner, converted to ide-timing.[ch]
* May not work. ;)

But does work with an Intel Corp. 82371AB PIIX4 IDE (rev 01) IDE controller...
I'll do some more stress testing but it boots, works in DMA and the data transfer rates haven't decreased ;) *playingwithhdparmanddbench* ;)

Bye

Vojtech Pavlik

author

2002-03-12 20:34 UTC #103 lore raw

Re: [patch] PIIX driver rewrite

Quoted 31 lines · Sebastian Droege

Quoted 28 lines · On Tue, 12 Mar 2002 21:00:35 +0100 Vojtech Pavlik <[email protected]> wrote:
Quoted 8 lines · Martin Dalecki
Hello Vojtech.
I have noticed that the ide-timings.h and ide_modules.h are running much in aprallel in the purpose they serve. Are the any chances you could dare to care about propagating the fairly nice ide-timings.h stuff in favour of ide_modules.h more.
Quoted 6 lines
BTW.> I think some stuff from ide-timings.h just belongs as generic functions intro ide.c, and right now there is nobody who you need to work from behind ;-).
So please feel free to do the changes you apparently desired to do a long time ago...
Oh, by the way, here goes the PIIX rewrite ... unlike the AMD one, this is completely untested, and may not work at all - I only have the datasheets at hand, no PIIX hardware.
Differences from the previous PIIX driver:
* 82451NX MIOC isn't supported anymore. It's not an ATA controller, anyway ;)
* 82371FB_0 PIIX ISA bridge isn't an ATA controller either.
* 82801CA ICH3 support added. Only ICH3-M is supported by the original driver.
* 82371MX MPIIX is not supported anymore. Too weird beast and doesn't do DMA anyway, better handled by the generic PCI ATA routines.

Quoted 4 lines

Quoted 3 lines
* Cleaner, converted to ide-timing.[ch]
* May not work. ;)
But does work with an Intel Corp. 82371AB PIIX4 IDE (rev 01) IDE controller...

Thanks a lot for the testing!

Quoted 2 lines

I'll do some more stress testing but it boots, works in DMA and the data transfer rates haven't decreased ;) *playingwithhdparmanddbench* ;)

If you can (in addition to the benchmark numbers) also send me the output of 'cat /proc/ide/piix' and 'dmesg' and 'lspci -vvxxx'both my and the original version ... that'd help a lot too.

Thanks again,

Signature · 2 lines

Vojtech Pavlik
SuSE Labs

Sebastian Droege

2002-03-12 21:05 UTC #104 lore raw

Re: [patch] PIIX driver rewrite

Quoted 37 lines · On Tue, 12 Mar 2002 21:34:28 +0100 Vojtech Pavlik <[email protected]> wrote:

Quoted 31 lines · Sebastian Droege
Quoted 28 lines · On Tue, 12 Mar 2002 21:00:35 +0100 Vojtech Pavlik <[email protected]> wrote:
Quoted 8 lines · Martin Dalecki
Hello Vojtech.
I have noticed that the ide-timings.h and ide_modules.h are running much in aprallel in the purpose they serve. Are the any chances you could dare to care about propagating the fairly nice ide-timings.h stuff in favour of ide_modules.h more.
Quoted 6 lines
BTW.> I think some stuff from ide-timings.h just belongs as generic functions intro ide.c, and right now there is nobody who you need to work from behind ;-).
So please feel free to do the changes you apparently desired to do a long time ago...
Oh, by the way, here goes the PIIX rewrite ... unlike the AMD one, this is completely untested, and may not work at all - I only have the datasheets at hand, no PIIX hardware.
Differences from the previous PIIX driver:
* 82451NX MIOC isn't supported anymore. It's not an ATA controller, anyway ;)
* 82371FB_0 PIIX ISA bridge isn't an ATA controller either.
* 82801CA ICH3 support added. Only ICH3-M is supported by the original driver.
* 82371MX MPIIX is not supported anymore. Too weird beast and doesn't do DMA anyway, better handled by the generic PCI ATA routines.
Quoted 4 lines
Quoted 3 lines
* Cleaner, converted to ide-timing.[ch]
* May not work. ;)
But does work with an Intel Corp. 82371AB PIIX4 IDE (rev 01) IDE controller...

Quoted 8 lines

Thanks a lot for the testing!
Quoted 2 lines
I'll do some more stress testing but it boots, works in DMA and the data transfer rates haven't decreased ;) *playingwithhdparmanddbench* ;)
If you can (in addition to the benchmark numbers) also send me the output of 'cat /proc/ide/piix' and 'dmesg' and 'lspci -vvxxx'both my and the original version ... that'd help a lot too.

OK here they are ;)
The only thing which confuses me is the line
PCI clock: 33333MHz
in /proc/ide/piix
It should be 33 MHz I think
I think the mistake is in line 199 of piix.c:
piix_print("PCI clock: %dMHz", piix_clock);
has to be
piix_print("PCI clock: %dMHz", piix_clock / 1000);

or
the whole piix_clock stuff after line 432 is wrong

BTW: in /proc/ide/piix are sometimes wrong values for transfer rate and cycle time... I think they can't get negative ;)

Benchmark results come tomorrow

Bye

Vojtech Pavlik

author

2002-03-12 21:20 UTC #105 lore raw

Re: [patch] PIIX driver rewrite

Hi!

Thanks a lot, the /proc stuff indeed seems to be wrong. I'll fix that tomorrow.

Quoted 66 lines · Sebastian Droege

Quoted 37 lines · On Tue, 12 Mar 2002 21:34:28 +0100 Vojtech Pavlik <[email protected]> wrote:
Quoted 31 lines · Sebastian Droege
Quoted 28 lines · On Tue, 12 Mar 2002 21:00:35 +0100 Vojtech Pavlik <[email protected]> wrote:
Quoted 8 lines · Martin Dalecki
Hello Vojtech.
I have noticed that the ide-timings.h and ide_modules.h are running much in aprallel in the purpose they serve. Are the any chances you could dare to care about propagating the fairly nice ide-timings.h stuff in favour of ide_modules.h more.
Quoted 6 lines
BTW.> I think some stuff from ide-timings.h just belongs as generic functions intro ide.c, and right now there is nobody who you need to work from behind ;-).
So please feel free to do the changes you apparently desired to do a long time ago...
Oh, by the way, here goes the PIIX rewrite ... unlike the AMD one, this is completely untested, and may not work at all - I only have the datasheets at hand, no PIIX hardware.
Differences from the previous PIIX driver:
* 82451NX MIOC isn't supported anymore. It's not an ATA controller, anyway ;)
* 82371FB_0 PIIX ISA bridge isn't an ATA controller either.
* 82801CA ICH3 support added. Only ICH3-M is supported by the original driver.
* 82371MX MPIIX is not supported anymore. Too weird beast and doesn't do DMA anyway, better handled by the generic PCI ATA routines.
Quoted 4 lines
Quoted 3 lines
* Cleaner, converted to ide-timing.[ch]
* May not work. ;)
But does work with an Intel Corp. 82371AB PIIX4 IDE (rev 01) IDE controller...
Quoted 8 lines
Thanks a lot for the testing!
Quoted 2 lines
I'll do some more stress testing but it boots, works in DMA and the data transfer rates haven't decreased ;) *playingwithhdparmanddbench* ;)
If you can (in addition to the benchmark numbers) also send me the output of 'cat /proc/ide/piix' and 'dmesg' and 'lspci -vvxxx'both my and the original version ... that'd help a lot too.
OK here they are ;)
The only thing which confuses me is the line
PCI clock: 33333MHz
in /proc/ide/piix
It should be 33 MHz I think
I think the mistake is in line 199 of piix.c:
piix_print("PCI clock: %dMHz", piix_clock);
has to be
piix_print("PCI clock: %dMHz", piix_clock / 1000);
or
the whole piix_clock stuff after line 432 is wrong
BTW: in /proc/ide/piix are sometimes wrong values for transfer rate and cycle time... I think they can't get negative ;)
Benchmark results come tomorrow
Bye

Signature · 2 lines

Vojtech Pavlik
SuSE Labs

Russell King

2002-03-13 00:02 UTC #106 lore raw