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

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

[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.

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?

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

[Jeff Garzik]

Mark Lord wrote:
> 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).

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

	Jeff

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

[Mark Lord]

Jeff Garzik wrote:
> Mark Lord wrote:
>> 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).
> 
> 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 <mlord@pobox.com>
---
--- linux/drivers/scsi/libata-scsi.c.orig       2006-06-19 10:37:03.000000000 -0400
+++ linux/drivers/scsi/libata-scsi.c    2006-07-06 16:46:42.000000000 -0400
@@ -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.
         */

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

[Mark Lord]

On Thursday 06 July 2006 16:51, Mark Lord wrote:
>..
> 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 <mlord@pobox.com>
---
--- 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
@@ -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.
 	 */

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

[Jeff Garzik]

Mark Lord wrote:
> Enable libata-scsi to report correct sense data on errors.
> 
> Signed-off-by:  Mark Lord <mlord@pobox.com>
> ---
> --- 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
> @@ -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


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

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

[Ric Wheeler]

Jeff Garzik wrote:

> Mark Lord wrote:
>
>> Enable libata-scsi to report correct sense data on errors.
>>
>> Signed-off-by:  Mark Lord <mlord@pobox.com>
>> ---
>> --- 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
>> @@ -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
>
>
>
> 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
>
>
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

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

[Mark Lord]

Jeff Garzik wrote:
> Mark Lord wrote:
>> Enable libata-scsi to report correct sense data on errors.
>>
>> Signed-off-by:  Mark Lord <mlord@pobox.com>
>> ---
>> --- 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
>> @@ -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
> 
> 
> 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.

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

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

[Tejun Heo]

Hello, Mark Lord.

Mark Lord wrote:
[--snip--]
> 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.

> 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.  :-(

-- 
tejun