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

[Ric Wheeler <ric@emc.com>]

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