Uhhuh. That's READY_STAT | SRV_STAT. No error, no DRQ, no nothing.And I think this also explains why your bisect found that old commit4d977e43d8ae758434e603cf2455d955f71c77c4 to be problematic.

The thing is, that commit - and the taskfile code that it then all got replaced with - use this totally bogus check:

OK_STAT(stat, DRQ_STAT, BAD_R_STAT) ...

which basically says that the only OK situation is that no error bits are set, and DRQ _has_ to be set.

Before that, the code did the right thing in any _combination_ of bits: if DRQ wasn't set, it would just say "oh, ok, the command is done", but the taskfile code and the 4d977e43d commit code thinks that DRQ not being set is an error, and then it gets confused when the actual error bits aren't set!

I think that whole way of writing code is totally horribly bad! It's not only trying to tie together bits that are independent, but it actually gets a lot harder to understand too, because now you have *four* different cases ("no error, no drq", "error, no drq", "no error, drq", "error, drq") but you use one test to try to find the one you expect, and then the code easily messes up on all the other three cases!

I personally think the code should be written more like the suggested appended patch, which checks those error and drq bits _separately_, and doesn't try to mix them up, because they really are independent.

Anders, does this patch change any behaviour?

It basically does:

- if the error bits are set, we just error out (and expect ide_error() to flush the data fifo if necessary)

- if the DRQ bit is *not* set, something unexpected happened, and we go out of line to handle that.

- otherwise we just do the data in phase and see if we're all done.

Then, in the unexpected case, we try to see what is actually going on, and that's where we do the same thing we always used to do (ie the thing that commit 4d977e43d messed up): if the drive tells us it's all done, we just end the command.

IOW, if we have no errors, no drq, and the drive says "ready and not busy", we just finish the command!

I think this is much more readable, and also less fragile, than using that OK_STAT() macro to mix up DRQ and errors in odd ways. Keep them separate events.

- /* If it was the last datablock check status and finish transfer. */+ /* Are we done? Check status and finish transfer. */ if (!hwif->nleft) { stat = wait_drive_not_busy(drive); if (!OK_STAT(stat, 0, BAD_STAT))