Re: Linux 2.6.25-rc4

[Linus Torvalds <torvalds@linux-foundation.org>]

On Tue, 18 Mar 2008, Anders Eriksson wrote:
> 
> Mar 18 14:18:26 tippex do_rw_taskfile(task->data_phase=1, tf->command=236
> Mar 18 14:18:26 tippex do_rw_taskfile(task->data_phase=0, tf->command=176
> Mar 18 14:18:26 tippex do_rw_taskfile(task->data_phase=1, tf->command=176
> Mar 18 14:18:26 tippex task_in_intr: hda: stat=50

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 commit
4d977e43d8ae758434e603cf2455d955f71c77c4 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.

			Linus

---
 drivers/ide/ide-taskfile.c |   36 +++++++++++++++++++++++++++---------
 1 files changed, 27 insertions(+), 9 deletions(-)

diff --git a/drivers/ide/ide-taskfile.c b/drivers/ide/ide-taskfile.c
index 0518a2e..4c86a8d 100644
--- a/drivers/ide/ide-taskfile.c
+++ b/drivers/ide/ide-taskfile.c
@@ -423,6 +423,25 @@ void task_end_request(ide_drive_t *drive, struct request *rq, u8 stat)
 }
 
 /*
+ * We got an interrupt on a task_in case, but no errors and no DRQ.
+ *
+ * It might be a spurious irq (shared irq), but it might be a
+ * command that had no output.
+ */
+static ide_startstop_t task_in_unexpected(ide_drive_t *drive, struct request *rq, u8 stat)
+{
+	/* Command all done? */
+	if (OK_STAT(stat, READY_STAT, BUSY_STAT)) {
+		task_end_request(drive, rq, stat);
+		return ide_stopped;
+	}
+
+	/* Assume it was a spurious irq */
+	ide_set_handler(drive, &task_in_intr, WAIT_WORSTCASE, NULL);
+	return ide_started;
+}
+
+/*
  * Handler for command with PIO data-in phase (Read/Read Multiple).
  */
 static ide_startstop_t task_in_intr(ide_drive_t *drive)
@@ -431,18 +450,17 @@ static ide_startstop_t task_in_intr(ide_drive_t *drive)
 	struct request *rq = HWGROUP(drive)->rq;
 	u8 stat = ide_read_status(drive);
 
-	/* new way for dealing with premature shared PCI interrupts */
-	if (!OK_STAT(stat, DRQ_STAT, BAD_R_STAT)) {
-		if (stat & (ERR_STAT | DRQ_STAT))
-			return task_error(drive, rq, __FUNCTION__, stat);
-		/* No data yet, so wait for another IRQ. */
-		ide_set_handler(drive, &task_in_intr, WAIT_WORSTCASE, NULL);
-		return ide_started;
-	}
+	/* Error? */
+	if (stat & ERR_STAT)
+		return task_error(drive, rq, __FUNCTION__, stat);
+
+	/* No error, but didn't want any data either? Odd. */
+	if (!(stat & DRQ_STAT))
+		return task_in_unexpected(drive, rq, stat);
 
 	ide_pio_datablock(drive, rq, 0);
 
-	/* 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))