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

[Mark Lord <liml@rtr.ca>]

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.
(??)


----------------- 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 " : "",