Re: Linux 2.6.25-rc4

[Linus Torvalds]

On Wed, 19 Mar 2008, Bartlomiej Zolnierkiewicz wrote:
> 
> Which is with compliance with PIO-in protocol specification and years of
> usage of the task_in_intr() code for fs code paths and HDIO_DRIVE_TASKFILE
> ioctl has proven that real hardware also works this way (please note that
> the changed code was used only for HDIO_DRIVE_CMD ioctl requests).

.. and years of drive_cmd_intr() which is much more widely used is *not* 
in agreement.

> If INTRQ is asserted and "BSY is cleared to zero and DRQ is cleared, then
> the device has completed the command with an error." (thus task_in_intr()
> assumed ERR bit to be set), otherwise the value ERR may not be defined
> (however task_in_intr() still assumed that it is OK to check ERR bit).

First off, the patch I sent out _works_.

Secondly, it's a hell of a lot more robust than yours is, exactly because 
it doesn't get confused if the data direction or size bit disagrees with 
the particular command in question.

> This is SMART command with SMART ENABLE ATTRIBUTE AUTOSAVE sub-command
> (feat == 0xd2, nsect == 0xf1) but it should use no-data protocol instead of
> PIO-in which brings us back to commit 18a056feccabdfa9764016a615121b194828bc72
> ("ide: don't enable local IRQs for PIO-in in driver_cmd_intr() (take 2)"):

.. and what about all the magic special IDE commands that may be 
drive-specific? What are we going to do about them?

In other words, we should not try to create an impossible-to-maintain 
piece of shit code that does the wrong thing if you send a command to the 
drive that the IDE layer doesn't understand (but the sending code 
hopefully does).

We should make the core IDE code *robust*. 

Your "real fix" is nothing of the sort. It's just a workaround for the 
fragility of the code that looks at the drive status. The real fix is to 
be robust in the face of even unexpected drive status codes, *especially* 
for the code that handles commands injected by the user!

In other words, you can talk about protocol specifications for things like 
the regular filesystem READ/WRITE commands. But don't create total crap 
like this that depends on the code knowing all possible outcomes of every 
single possible command.

Your patch is utter crap.

You say (about commit 18a056feccabdfa9764016a615121b194828bc72):

> as can be seen before this patch protocol to use was decided basing on DRQ bit
> being set - what would you call _that_ if taskfile code is horrible, he? ;-)

I would call that *correct*.

And my point is, we used to be better. You made the code buggy and fragile 
with that crap commit, and with the others like it (ie the already 
much-mentioned commit 4d977e43d8ae758434e603cf2455d955f71c77c4).

And that is and was *exactly* my point. The reason I called the taskfile 
code horrible was exactly the fact that it only worked if it thought it 
knew exactly what was going on.

Deciding what to do based on the DRQ bit (and the READY/BUSY/ERROR bits) 
is the *right* thing to do. It's the intelligent approach - actually 
tekign the response of the hardware into account, and being robust. The 
*stupid* and horrible thing to do is to think that you know better than 
what the hardware tells you, and think that you can look up every command 
that the user sends in the spec and use *that* to figure out what to do.

Trust the hardware, not the paper. Don't make the code only work when you 
think you know what is going on. Make the code work _always_.

In short, there is no way in hell I'll apply a workaround patch for crap 
code, when we already know what the robust solution is.

		Linus
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/