[PATCH] libata-core: Don't have screaming fits over DF/ERR combinations

[PATCH] libata-core: Don't have screaming fits over DF/ERR combinations

[Alan Cox]

Some hardware seems to get this wrong in a non-harmful way, and there are
some devices that seem to do it deliberately for various reasons.

Just take it as a device error not a catastrophic state machine
explosion. 

Signed-off-by: Alan Cox <alan@redhat.com>

diff -u --exclude-from /usr/src/exclude --new-file --recursive linux.vanilla-2.6.23-mm1/drivers/ata/libata-core.c linux-2.6.23-mm1/drivers/ata/libata-core.c
--- linux.vanilla-2.6.23-mm1/drivers/ata/libata-core.c	2007-10-15 15:03:26.000000000 +0100
+++ linux-2.6.23-mm1/drivers/ata/libata-core.c	2007-10-15 15:13:49.000000000 +0100
@@ -5258,7 +5319,15 @@
 		if (unlikely(status & (ATA_ERR | ATA_DF))) {
 			ata_port_printk(ap, KERN_WARNING, "DRQ=1 with device "
 					"error, dev_stat 0x%X\n", status);
-			qc->err_mask |= AC_ERR_HSM;
+			/* Some devices muck this up. Some follow an ATA
+			   non-standard that permits the damaged sector to
+			   be retrieved at this point. The ATA spec says
+			   we should jump up and down on DRQ + ERR, reality
+			   says we should be a little more relaxed.
+
+			   Rather than an HSM error, take it as a device
+			   error */
+			qc->err_mask |= AC_ERR_DEV;
 			ap->hsm_task_state = HSM_ST_ERR;
 			goto fsm_start;
 		}

Re: [PATCH] libata-core: Don't have screaming fits over DF/ERR combinations

[Sergei Shtylyov]

Alan Cox wrote:

> Some hardware seems to get this wrong in a non-harmful way, and there are
> some devices that seem to do it deliberately for various reasons.

> Just take it as a device error not a catastrophic state machine
> explosion. 

> Signed-off-by: Alan Cox <alan@redhat.com>
> 
> diff -u --exclude-from /usr/src/exclude --new-file --recursive linux.vanilla-2.6.23-mm1/drivers/ata/libata-core.c linux-2.6.23-mm1/drivers/ata/libata-core.c
> --- linux.vanilla-2.6.23-mm1/drivers/ata/libata-core.c	2007-10-15 15:03:26.000000000 +0100
> +++ linux-2.6.23-mm1/drivers/ata/libata-core.c	2007-10-15 15:13:49.000000000 +0100
> @@ -5258,7 +5319,15 @@
>  		if (unlikely(status & (ATA_ERR | ATA_DF))) {
>  			ata_port_printk(ap, KERN_WARNING, "DRQ=1 with device "
>  					"error, dev_stat 0x%X\n", status);
> -			qc->err_mask |= AC_ERR_HSM;
> +			/* Some devices muck this up. Some follow an ATA
> +			   non-standard that permits the damaged sector to
> +			   be retrieved at this point. The ATA spec says
> +			   we should jump up and down on DRQ + ERR, reality

    I've always thought that setting both DRQ and ERR is perfectly valid 
(well, maybe it's become invalid since ATAPI-4 where all these state 
transition flow charts have made its first appearance, to be quickly replaced 
by the state diagrams :-) -- I'm too lazy to check now... :-)

> +			   says we should be a little more relaxed.
> +
> +			   Rather than an HSM error, take it as a device
> +			   error */

    I'm not sure it's an error in the first place.

> +			qc->err_mask |= AC_ERR_DEV;
>  			ap->hsm_task_state = HSM_ST_ERR;
>  			goto fsm_start;
>  		}

MBR, Sergei

Re: [PATCH] libata-core: Don't have screaming fits over DF/ERR combinations

[Jeff Garzik]

Sergei Shtylyov wrote:
> Alan Cox wrote:
> 
>> Some hardware seems to get this wrong in a non-harmful way, and there are
>> some devices that seem to do it deliberately for various reasons.
> 
>> Just take it as a device error not a catastrophic state machine
>> explosion. 
> 
>> Signed-off-by: Alan Cox <alan@redhat.com>
>>
>> diff -u --exclude-from /usr/src/exclude --new-file --recursive 
>> linux.vanilla-2.6.23-mm1/drivers/ata/libata-core.c 
>> linux-2.6.23-mm1/drivers/ata/libata-core.c
>> --- linux.vanilla-2.6.23-mm1/drivers/ata/libata-core.c    2007-10-15 
>> 15:03:26.000000000 +0100
>> +++ linux-2.6.23-mm1/drivers/ata/libata-core.c    2007-10-15 
>> 15:13:49.000000000 +0100
>> @@ -5258,7 +5319,15 @@
>>          if (unlikely(status & (ATA_ERR | ATA_DF))) {
>>              ata_port_printk(ap, KERN_WARNING, "DRQ=1 with device "
>>                      "error, dev_stat 0x%X\n", status);
>> -            qc->err_mask |= AC_ERR_HSM;
>> +            /* Some devices muck this up. Some follow an ATA
>> +               non-standard that permits the damaged sector to
>> +               be retrieved at this point. The ATA spec says
>> +               we should jump up and down on DRQ + ERR, reality
> 
>    I've always thought that setting both DRQ and ERR is perfectly valid 
> (well, maybe it's become invalid since ATAPI-4 where all these state 
> transition flow charts have made its first appearance, to be quickly 
> replaced by the state diagrams :-) -- I'm too lazy to check now... :-)

DRQ+ERR is valid, and SRST (or hard reset) is defined as the method of 
kicking the device out of that state.

	Jeff

Re: [PATCH] libata-core: Don't have screaming fits over DF/ERR combinations

[Sergei Shtylyov]

Jeff Garzik wrote:

>>> Some hardware seems to get this wrong in a non-harmful way, and there 
>>> are
>>> some devices that seem to do it deliberately for various reasons.

>>> Just take it as a device error not a catastrophic state machine
>>> explosion. 

>>> Signed-off-by: Alan Cox <alan@redhat.com>

>>> diff -u --exclude-from /usr/src/exclude --new-file --recursive 
>>> linux.vanilla-2.6.23-mm1/drivers/ata/libata-core.c 
>>> linux-2.6.23-mm1/drivers/ata/libata-core.c
>>> --- linux.vanilla-2.6.23-mm1/drivers/ata/libata-core.c    2007-10-15 
>>> 15:03:26.000000000 +0100
>>> +++ linux-2.6.23-mm1/drivers/ata/libata-core.c    2007-10-15 
>>> 15:13:49.000000000 +0100
>>> @@ -5258,7 +5319,15 @@
>>>          if (unlikely(status & (ATA_ERR | ATA_DF))) {
>>>              ata_port_printk(ap, KERN_WARNING, "DRQ=1 with device "
>>>                      "error, dev_stat 0x%X\n", status);
>>> -            qc->err_mask |= AC_ERR_HSM;
>>> +            /* Some devices muck this up. Some follow an ATA
>>> +               non-standard that permits the damaged sector to
>>> +               be retrieved at this point. The ATA spec says
>>> +               we should jump up and down on DRQ + ERR, reality

>>    I've always thought that setting both DRQ and ERR is perfectly 
>> valid (well, maybe it's become invalid since ATAPI-4 where all these 
>> state transition flow charts have made its first appearance, to be 
>> quickly replaced by the state diagrams :-) -- I'm too lazy to check 
>> now... :-)

> DRQ+ERR is valid, and SRST (or hard reset) is defined as the method of 
> kicking the device out of that state.

    Well, in the times of old, reading a sector (or group of them for multiple 
mode) was valid for that purpose, wasn't it?

>     Jeff

MBR, Sergei

Re: [PATCH] libata-core: Don't have screaming fits over DF/ERR combinations

[Jeff Garzik]

Sergei Shtylyov wrote:
> Jeff Garzik wrote:
> 
>>>> Some hardware seems to get this wrong in a non-harmful way, and 
>>>> there are
>>>> some devices that seem to do it deliberately for various reasons.
> 
>>>> Just take it as a device error not a catastrophic state machine
>>>> explosion. 
> 
>>>> Signed-off-by: Alan Cox <alan@redhat.com>
> 
>>>> diff -u --exclude-from /usr/src/exclude --new-file --recursive 
>>>> linux.vanilla-2.6.23-mm1/drivers/ata/libata-core.c 
>>>> linux-2.6.23-mm1/drivers/ata/libata-core.c
>>>> --- linux.vanilla-2.6.23-mm1/drivers/ata/libata-core.c    2007-10-15 
>>>> 15:03:26.000000000 +0100
>>>> +++ linux-2.6.23-mm1/drivers/ata/libata-core.c    2007-10-15 
>>>> 15:13:49.000000000 +0100
>>>> @@ -5258,7 +5319,15 @@
>>>>          if (unlikely(status & (ATA_ERR | ATA_DF))) {
>>>>              ata_port_printk(ap, KERN_WARNING, "DRQ=1 with device "
>>>>                      "error, dev_stat 0x%X\n", status);
>>>> -            qc->err_mask |= AC_ERR_HSM;
>>>> +            /* Some devices muck this up. Some follow an ATA
>>>> +               non-standard that permits the damaged sector to
>>>> +               be retrieved at this point. The ATA spec says
>>>> +               we should jump up and down on DRQ + ERR, reality
> 
>>>    I've always thought that setting both DRQ and ERR is perfectly 
>>> valid (well, maybe it's become invalid since ATAPI-4 where all these 
>>> state transition flow charts have made its first appearance, to be 
>>> quickly replaced by the state diagrams :-) -- I'm too lazy to check 
>>> now... :-)
> 
>> DRQ+ERR is valid, and SRST (or hard reset) is defined as the method of 
>> kicking the device out of that state.
> 
>    Well, in the times of old, reading a sector (or group of them for 
> multiple mode) was valid for that purpose, wasn't it?

Yes, hence the recent FIFO drain patches.

But that doesn't make sense for SATA devices, where the FIFO is really 
emulated, and it works on older PATA devices.

	Jeff

Re: [PATCH] libata-core: Don't have screaming fits over DF/ERR combinations

[Alan Cox]

> Yes, hence the recent FIFO drain patches.
> 
> But that doesn't make sense for SATA devices, where the FIFO is really 
> emulated, and it works on older PATA devices.

The FIFO is emulated on PATA. Basically speaking the device prefetches
data and buffers the irq arrival until the right moment. Hence the need
to drain it on some chips. Others reset themselves in this case and post
reset a data read on some promise chips hangs the bus solid

Re: [PATCH] libata-core: Don't have screaming fits over DF/ERR combinations

[Jeff Garzik]

Alan Cox wrote:
> Some hardware seems to get this wrong in a non-harmful way, and there are
> some devices that seem to do it deliberately for various reasons.
> 
> Just take it as a device error not a catastrophic state machine
> explosion. 
> 
> Signed-off-by: Alan Cox <alan@redhat.com>

applied to #for-testing branch, let me know when you think its suitable 
for upstream