Re: libata-eh/pmp command sequence on NCQ media error

[Mark Lord <liml@rtr.ca>]

Mark Lord wrote:
> Mark Lord wrote:
>> Tejun Heo wrote:
> ..
>>> w00t w00t I thought about that when I wrote NCQ EH.  All you have to 
>>> do is to export ata_eh_analyze_ncq_error() can call it right after 
>>> error_handler starts and put the controller into a working state (so 
>>> that SCR accesses work again).  After it finishes, call the generic 
>>> handler.  The second time around ata_eh_analyze_ncq_error() will be 
>>> no-op and you should get what you want.
>> ..
>>
>> Tejun, I've implemented this, and it sort of works now.
>>
>> But libata-eh is interfering in a different way.
>> It reads the SCR_ERROR register for the PMP link,
>> notices that the SERR_COMM_RECOVERED bit is set,
>> and then decides to perform a full reset.
>>
>> This bit seems to be always set whenever sata_mv tries
>> to report device-errors (media errors) in NCQ for a pmp.
>>
>> I'll have to dig some more, but it's probably just leftover
>> from the initial probe/reset time or something.
>> This is a Marvell PM.
>>
>> My understanding was, that this particular bit is supposed
>> to be for information purposes only, letting us know that
>> the hardware has automatically recovered from a soft error.
>>
>> So why are we taking a hammer to things there?
> ..
> 
> FWIW, this patch fixes it for me (and fixes a misleading printk).
> Or I could just clear that bit from sata_mv before invoking EH.
..

Mmmm.. my first attempts to clear it from within sata_mv didn't work.
But the patch below is still fine.  (?)

> ----------------- SNIP ------------------
> 
> Prevent libata-eh from taking too drastic an action in response
> to a simple SATA "Recovered Communication Error".
> Fix a misleading (port vs. link) printk while we're at it.
> 
> Signed-off-by: Mark Lord <mlord@pobox.com>
> 
> --- upstream/drivers/ata/libata-eh.c    2008-04-30 17:35:36.000000000 -0400
> +++ linux/drivers/ata/libata-eh.c    2008-04-30 17:35:45.000000000 -0400
> @@ -1312,8 +1312,7 @@
>         err_mask |= AC_ERR_ATA_BUS;
>         action |= ATA_EH_RESET;
>     }
> -    if (serror &
> -        (SERR_DATA_RECOVERED | SERR_COMM_RECOVERED | SERR_DATA)) {
> +    if (serror & (SERR_DATA_RECOVERED | SERR_DATA)) {
>         err_mask |= AC_ERR_ATA_BUS;
>         action |= ATA_EH_RESET;
>     }
> @@ -1924,7 +1923,7 @@
>     }
> 
>     if (ehc->i.serror)
> -        ata_port_printk(ap, KERN_ERR,
> +        ata_link_printk(link, KERN_ERR,
>           "SError: { %s%s%s%s%s%s%s%s%s%s%s%s%s%s%s%s%s}\n",
>           ehc->i.serror & SERR_DATA_RECOVERED ? "RecovData " : "",
>           ehc->i.serror & SERR_COMM_RECOVERED ? "RecovComm " : "",
> -- 
> To unsubscribe from this list: send the line "unsubscribe linux-ide" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html