libata EH appears to be NFG up to 2.6.17 (at least).

DISCUSSION Mark Lord 4 participants 8 messages Jul 6, 2006 – Jul 8, 2006

Subsystems (2)

SCSI libata

lore

Share & tools

lore

Mark Lord

author

2006-07-06 20:37 UTC #0 lore raw

Got your attention now? Good!

I am doing some testing with known-bad drives on 2.6.16 (and 2.6.17).

Libata EH is wretched there, because it does not seem to be careful about reading/saving the bad ata_status value when an error occurs.

The ata_status from a failed/aborted command is first read in the interrupt handler, either by the LLD or by ata_host_intr().

This value is not saved for reuse anywhere, and the next time it is read, the reader will see ATA_ERR==0, and then not do the Right Thing (tm).

Who reads it next, you ask? Well, it gets read *again* from libata-scsi when it is trying to generate meaningful sense data. But at that point, all that is seen is 0x50 -- "success".

So libata-scsi returns incorrect (or no) sense data to the SCSI mid-layer, and the error is mishandled or ignored.

Ugh. The distro folks will probably want to fix this in their 2.6.1[56] based distro kernels. I don't yet see a way to do this without modifying core data structures (eg. adding an ata_status field to the qc).

Any ideas?

Jeff Garzik

2006-07-06 20:47 UTC #1 lore raw

Quoted 17 lines · Mark Lord

Got your attention now? Good!
I am doing some testing with known-bad drives on 2.6.16 (and 2.6.17).
Libata EH is wretched there, because it does not seem to be careful about reading/saving the bad ata_status value when an error occurs.
The ata_status from a failed/aborted command is first read in the interrupt handler, either by the LLD or by ata_host_intr().
This value is not saved for reuse anywhere, and the next time it is read, the reader will see ATA_ERR==0, and then not do the Right Thing (tm).
Who reads it next, you ask? Well, it gets read *again* from libata-scsi when it is trying to generate meaningful sense data. But at that point, all that is seen is 0x50 -- "success".

Quoted 6 lines

So libata-scsi returns incorrect (or no) sense data to the SCSI mid-layer, and the error is mishandled or ignored.
Ugh. The distro folks will probably want to fix this in their 2.6.1[56] based distro kernels. I don't yet see a way to do this without modifying core data structures (eg. adding an ata_status field to the qc).

What driver?
What architecture?
What kernel config?
How does 2.6.18-rc1, with vastly different EH, behave?

Jeff

Mark Lord

author

2006-07-06 20:51 UTC #2 lore raw

Quoted 21 lines · Jeff Garzik

Quoted 20 lines · Mark Lord
Got your attention now? Good!
I am doing some testing with known-bad drives on 2.6.16 (and 2.6.17).
Libata EH is wretched there, because it does not seem to be careful about reading/saving the bad ata_status value when an error occurs.
The ata_status from a failed/aborted command is first read in the interrupt handler, either by the LLD or by ata_host_intr().
This value is not saved for reuse anywhere, and the next time it is read, the reader will see ATA_ERR==0, and then not do the Right Thing (tm).
Who reads it next, you ask? Well, it gets read *again* from libata-scsi when it is trying to generate meaningful sense data. But at that point, all that is seen is 0x50 -- "success".
So libata-scsi returns incorrect (or no) sense data to the SCSI mid-layer, and the error is mishandled or ignored.

Quoted 8 lines

Quoted 3 lines
Ugh. The distro folks will probably want to fix this in their 2.6.1[56] based distro kernels. I don't yet see a way to do this without modifying core data structures (eg. adding an ata_status field to the qc).
What driver?
What architecture?
What kernel config?
How does 2.6.18-rc1, with vastly different EH, behave?

All drivers, all architectures. Dunno about 2.6.18-rc1 yet.

This patch (below) appears to fix it, but I really need your opinion on its correctness. My apologies in advance for the likely bad whitespace, as I don't currently have access to my usual patch-mailer at present.

Enable libata-scsi to report correct sense data on errors.

Signed-off-by Mark Lord <[email protected]>

---

drivers/scsi/libata-scsi.c 2006-07-06 16:46:42.000000000 -0400 renamed from drivers/scsi/libata-scsi.c.orig 2006-06-19 10:37:03.000000000 -0400 +6 -0

@@ -554,6 +554,12 @@
       qc->ap->ops->tf_read(qc->ap, tf);
 
       /*
+        * Restore the error bit (which got cleared when the
+        * interrupt handler first read the ata_status
+        */
+       if (qc->err_mask)
+               tf->command |= ATA_ERR;
+       /*
        * Use ata_to_sense_error() to map status register bits
        * onto sense key, asc & ascq.
        */

Mark Lord

author

2006-07-06 21:25 UTC #3 lore raw

Quoted 6 lines · Mark Lord

..
All drivers, all architectures. Dunno about 2.6.18-rc1 yet.
This patch (below) appears to fix it, but I really need your opinion on its correctness. My apologies in advance for the likely bad whitespace, as I don't currently have access to my usual patch-mailer at present.

Whoops.. got the wrong function when regenerating the patch. Here it is again, on the correct lines:

Enable libata-scsi to report correct sense data on errors.

Signed-off-by Mark Lord <[email protected]>

---

drivers/scsi/libata-scsi.c renamed from drivers/scsi/libata-scsi.c.orig +7 -0

@@ -667,6 +667,13 @@
 	qc->ap->ops->tf_read(qc->ap, tf);
 
 	/*
+	 * Restore the error bit, which got cleared when the
+	 * interrupt handler first read the ata_status.
+	 */
+	if (qc->err_mask & AC_ERR_DEV)
+		tf->command |= ATA_ERR;
+
+	/*
 	 * Use ata_to_sense_error() to map status register bits
 	 * onto sense key, asc & ascq.
 	 */

Jeff Garzik

2006-07-06 22:09 UTC #4 lore raw

Quoted 8 lines · Mark Lord

Enable libata-scsi to report correct sense data on errors.
Signed-off-by Mark Lord <[email protected]>
---
drivers/scsi/libata-scsi.c renamed from drivers/scsi/libata-scsi.c.orig +0 -0
@@ -667,6 +667,13 @@
 	qc->ap->ops->tf_read(qc->ap, tf);

drivers/scsi/libata-scsi.c line 677

Quoted 10 lines

drivers/scsi/libata-scsi.c renamed from drivers/scsi/libata-scsi.c.orig +7 -0

 
 	/*
+	 * Restore the error bit, which got cleared when the
+	 * interrupt handler first read the ata_status.
+	 */
+	if (qc->err_mask & AC_ERR_DEV)
+		tf->command |= ATA_ERR;
+
+	/*
 	 * Use ata_to_sense_error() to map status register bits

Again it's an LLDD issue. Your answer of "all LLDDs" sounds a bit suspicious.

AC_ERR_DEV should not be set unless (a) some code noticed that ATA_ERR was already present in the Status register, or (b) some code set AC_ERR_DEV for some reason other than ATA_ERR presence. Both of those factors depend heavily on LLDD behavior.

ANYWAY, your above patch is not wrong, but it begs the question of where the problem REALLY lies. Which LLDD sets AC_ERR_DEV when ATA_ERR is not present?

Jeff

Ric Wheeler

2006-07-06 23:22 UTC #5 lore raw

Quoted 27 lines · Jeff Garzik

Quoted 10 lines · Mark Lord
Enable libata-scsi to report correct sense data on errors.
Signed-off-by Mark Lord <[email protected]>
---
--- linux/drivers/scsi/libata-scsi.c.orig 2006-07-06
17:09:54.000000000 -0400
+++ linux/drivers/scsi/libata-scsi.c 2006-07-06 17:17:43.000000000
-0400
drivers/scsi/libata-scsi.c renamed from drivers/scsi/libata-scsi.c.orig +0 -0
@@ -667,6 +667,13 @@
     qc->ap->ops->tf_read(qc->ap, tf);
drivers/scsi/libata-scsi.c line 677
Quoted 10 lines
drivers/scsi/libata-scsi.c renamed from drivers/scsi/libata-scsi.c.orig +7 -0
 
     /*
+     * Restore the error bit, which got cleared when the
+     * interrupt handler first read the ata_status.
+     */
+    if (qc->err_mask & AC_ERR_DEV)
+        tf->command |= ATA_ERR;
+
+    /*
      * Use ata_to_sense_error() to map status register bits
Again it's an LLDD issue. Your answer of "all LLDDs" sounds a bit suspicious.

Quoted 7 lines

AC_ERR_DEV should not be set unless (a) some code noticed that ATA_ERR was already present in the Status register, or (b) some code set AC_ERR_DEV for some reason other than ATA_ERR presence. Both of those factors depend heavily on LLDD behavior.
ANYWAY, your above patch is not wrong, but it begs the question of where the problem REALLY lies. Which LLDD sets AC_ERR_DEV when ATA_ERR is not present?
Jeff

I believe that Mark was testing this on an ahci (ich6r) based box.

A quick (and inexpert) look at ahci.c in 2.6.17 shows that ahci_host_intr() does set AC_ERR_DEV - I did not see in my quick look where ATA_ERR was set.

Tejun's updated EH code changed the code path pretty substantially.

ric

Mark Lord

author

2006-07-07 13:03 UTC #6 lore raw

In reply to Jeff Garzik

Quoted 25 lines · Jeff Garzik

Quoted 10 lines · Mark Lord
Enable libata-scsi to report correct sense data on errors.
Signed-off-by Mark Lord <[email protected]>
---
--- linux/drivers/scsi/libata-scsi.c.orig 2006-07-06
17:09:54.000000000 -0400
+++ linux/drivers/scsi/libata-scsi.c 2006-07-06 17:17:43.000000000
-0400
drivers/scsi/libata-scsi.c renamed from drivers/scsi/libata-scsi.c.orig +0 -0
@@ -667,6 +667,13 @@
     qc->ap->ops->tf_read(qc->ap, tf);
drivers/scsi/libata-scsi.c line 677
Quoted 10 lines
drivers/scsi/libata-scsi.c renamed from drivers/scsi/libata-scsi.c.orig +7 -0
 
     /*
+     * Restore the error bit, which got cleared when the
+     * interrupt handler first read the ata_status.
+     */
+    if (qc->err_mask & AC_ERR_DEV)
+        tf->command |= ATA_ERR;
+
+    /*
      * Use ata_to_sense_error() to map status register bits
Again it's an LLDD issue. Your answer of "all LLDDs" sounds a bit suspicious.

Quoted 1 line

AC_ERR_DEV should not be set unless (a) some code noticed that ATA_ERR was already present in the Status register, or (b) some code set AC_ERR_DEV for some reason other than ATA_ERR presence. Both of those factors depend heavily on LLDD behavior.

So that *is* the meaning of AC_ERR_DEV --> "LLDD saw ATA_ERR" ? I'm just not sure whether to check for that one bit, or for any bit set in qc->err_mask.

Of course even doing this is not at all foolproof -- the code in libata-scsi will then go and read the ATA status again (along with all of the others), and the drive/software may well have cleared/changed bits by then.

In fact, you've even had isolated bug reports from the past month with exactly this happening -- where the reported error status was 0x50 by the time libata-scsi got hold of it.

I dunno what the new EH does (yet), but the only good way to deal with this is for the ATA status & error values to be read *and* saved on command completion, and then reused by any later code that needs to do detailed sense reporting or other diagnostics.

With NCQ/TCQ, this can be tough to achieve, possibly requiring the LLDD to drop to legacy mode on error, and capture the values there. Of course, by then it's too late to be perfect, as the host controller has probably already read
and discarded the drive status at least once. (the PDC sata_qstor device
is an exception there -- they save/return failed status during queuing).

In summary, when a command fails, we need to save the failed status and error values that we saw when we decided it fails. Reading them again later is not optimal, because they can change in the interim (asynchronous notifications, or just drives that like to clear ERR after status is read).

I'm out today, but I'll try and fiddle with this some more on the weekend. I have several host controllers here to try this with.

Cheers

Tejun Heo

2006-07-08 16:55 UTC #7 lore raw

Hello, Mark Lord.

Mark Lord wrote:
[--snip--]

Quoted 4 lines

I dunno what the new EH does (yet), but the only good way to deal with this is for the ATA status & error values to be read *and* saved on command completion, and then reused by any later code that needs to do detailed sense reporting or other diagnostics.

Yeap, this is exactly what new EH does. It stores TF in qc->result_tf on failure and EH/SCSI completion only uses the cached TF stored at the time of failure.

Quoted 4 lines

With NCQ/TCQ, this can be tough to achieve, possibly requiring the LLDD to drop to legacy mode on error, and capture the values there. Of course, by then it's too late to be perfect, as the host controller has probably already read
and discarded the drive status at least once. (the PDC sata_qstor device
is an exception there -- they save/return failed status during queuing).

With NCQ, the true TF is retrieved via log page 10. EH retrieves it and stores it in qc->result_tf. The rest is the same.

With PMP, things get more interesting though. :-(

Signature · 1 line

tejun