[PATCH] libata-eh copy result_tf flags from outgoing tf

[PATCH] libata-eh copy result_tf flags from outgoing tf

[Mark Lord]

Fix mis-reporting of NCQ errors by copying tf->flags to result_tf->flags
in libata-eh.  This allows ata_gen_ata_sense() to report the failed
block number correctly to SCSI after a media error during NCQ.

This patch may also be a candidate for backporting to earlier kernels.
Without this fix, SCSI will fail I/O on the entire request rather
than just the bad sector.  That can be bad for a request that was
merged from many independent read reads from different tasks.

Signed-off-by: Mark Lord <mlord@pobox.com>

--- upstream/drivers/ata/libata-eh.c	2008-04-18 09:31:15.000000000 -0400
+++ linux/drivers/ata/libata-eh.c	2008-04-23 16:25:38.000000000 -0400
@@ -1402,6 +1402,7 @@
 	/* we've got the perpetrator, condemn it */
 	qc = __ata_qc_from_tag(ap, tag);
 	memcpy(&qc->result_tf, &tf, sizeof(tf));
+	qc->result_tf.flags = qc->tf.flags;
 	qc->err_mask |= AC_ERR_DEV | AC_ERR_NCQ;
 	ehc->i.err_mask &= ~AC_ERR_DEV;
 }

Re: [PATCH] libata-eh copy result_tf flags from outgoing tf

[Tejun Heo]

Mark Lord wrote:
> Fix mis-reporting of NCQ errors by copying tf->flags to result_tf->flags
> in libata-eh.  This allows ata_gen_ata_sense() to report the failed
> block number correctly to SCSI after a media error during NCQ.
> 
> This patch may also be a candidate for backporting to earlier kernels.
> Without this fix, SCSI will fail I/O on the entire request rather
> than just the bad sector.  That can be bad for a request that was
> merged from many independent read reads from different tasks.
> 
> Signed-off-by: Mark Lord <mlord@pobox.com>
> 
> --- upstream/drivers/ata/libata-eh.c    2008-04-18 09:31:15.000000000 -0400
> +++ linux/drivers/ata/libata-eh.c    2008-04-23 16:25:38.000000000 -0400
> @@ -1402,6 +1402,7 @@
>     /* we've got the perpetrator, condemn it */
>     qc = __ata_qc_from_tag(ap, tag);
>     memcpy(&qc->result_tf, &tf, sizeof(tf));
> +    qc->result_tf.flags = qc->tf.flags;

Maybe it's better to set ATA_TFLAG_LBA48 | ATA_TFLAG_ISADDR | 
ATA_TFLAG_DEVICE?  After all, all fields are available after that memcpy().

Thanks for fixing this.

-- 
tejun

Re: [PATCH] libata-eh copy result_tf flags from outgoing tf

[Mark Lord]

Tejun Heo wrote:
> Mark Lord wrote:
>> Fix mis-reporting of NCQ errors by copying tf->flags to result_tf->flags
>> in libata-eh.  This allows ata_gen_ata_sense() to report the failed
>> block number correctly to SCSI after a media error during NCQ.
>>
>> This patch may also be a candidate for backporting to earlier kernels.
>> Without this fix, SCSI will fail I/O on the entire request rather
>> than just the bad sector.  That can be bad for a request that was
>> merged from many independent read reads from different tasks.
>>
>> Signed-off-by: Mark Lord <mlord@pobox.com>
>>
>> --- upstream/drivers/ata/libata-eh.c    2008-04-18 09:31:15.000000000 
>> -0400
>> +++ linux/drivers/ata/libata-eh.c    2008-04-23 16:25:38.000000000 -0400
>> @@ -1402,6 +1402,7 @@
>>     /* we've got the perpetrator, condemn it */
>>     qc = __ata_qc_from_tag(ap, tag);
>>     memcpy(&qc->result_tf, &tf, sizeof(tf));
>> +    qc->result_tf.flags = qc->tf.flags;
> 
> Maybe it's better to set ATA_TFLAG_LBA48 | ATA_TFLAG_ISADDR | 
> ATA_TFLAG_DEVICE?  After all, all fields are available after that memcpy().
..

I just did it the same way as the standard version in libata-core does it.
In this particular case, we know it was NCQ, so we know that the original
tf->flags have (ATA_TFLAG_LBA48|ATA_TFLAG_ISADDR|ATA_TFLAG_DEVICE) set.

I figured better to copy than just overwrite, because new flags might come along.

Cheers

Re: [PATCH] libata-eh copy result_tf flags from outgoing tf

[Tejun Heo]

Mark Lord wrote:
>> Maybe it's better to set ATA_TFLAG_LBA48 | ATA_TFLAG_ISADDR | 
>> ATA_TFLAG_DEVICE?  After all, all fields are available after that 
>> memcpy().
> ..
> 
> I just did it the same way as the standard version in libata-core does it.
> In this particular case, we know it was NCQ, so we know that the original
> tf->flags have (ATA_TFLAG_LBA48|ATA_TFLAG_ISADDR|ATA_TFLAG_DEVICE) set.
> 
> I figured better to copy than just overwrite, because new flags might 
> come along.

Those flags are to tell which parts of the TF are valid or should be 
read in case of TFLAG_LBA48.  So, a TF after tf_read should always have 
TFLAG_ISADDR and TFLAG_DEVICE.  TFLAG_LBA48 works both ways.  If it was 
set when tf_read is called the higher part of LBA is read and the 
resulting tf should have TFLAG_LBA48 set.  But yeah the original code 
isn't like that.  If you can fix it up together with the NCQ path, it 
would be great.

Thanks.

-- 
tejun

Re: [PATCH] libata-eh copy result_tf flags from outgoing tf

[Mark Lord]

Tejun Heo wrote:
> Mark Lord wrote:
>>> Maybe it's better to set ATA_TFLAG_LBA48 | ATA_TFLAG_ISADDR | 
>>> ATA_TFLAG_DEVICE?  After all, all fields are available after that 
>>> memcpy().
>> ..
>>
>> I just did it the same way as the standard version in libata-core does 
>> it.
>> In this particular case, we know it was NCQ, so we know that the original
>> tf->flags have (ATA_TFLAG_LBA48|ATA_TFLAG_ISADDR|ATA_TFLAG_DEVICE) set.
>>
>> I figured better to copy than just overwrite, because new flags might 
>> come along.
> 
> Those flags are to tell which parts of the TF are valid or should be 
> read in case of TFLAG_LBA48.  So, a TF after tf_read should always have 
> TFLAG_ISADDR and TFLAG_DEVICE.  TFLAG_LBA48 works both ways.  If it was 
> set when tf_read is called the higher part of LBA is read and the 
> resulting tf should have TFLAG_LBA48 set.  But yeah the original code 
> isn't like that.  If you can fix it up together with the NCQ path, it 
> would be great.
..

No, I'm not going to touch the mainline path.  tf_read is used to read
the taskfile, but it doesn't always contain an LBA address, so I'm not so
sure that forcing TFLAG_ISADDR is always correct.

And since I don't know that code as well as you, I don't want to end up
having to help debug something that might break as a result.

This may warrant two patches.. one to fix the NCQ bug (already posted),
and perhaps another from you or Jeff if you want to change the behaviour.

Cheers

Re: [PATCH] libata-eh copy result_tf flags from outgoing tf

[Tejun Heo]

Mark Lord wrote:
>> Those flags are to tell which parts of the TF are valid or should be 
>> read in case of TFLAG_LBA48.  So, a TF after tf_read should always 
>> have TFLAG_ISADDR and TFLAG_DEVICE.  TFLAG_LBA48 works both ways.  If 
>> it was set when tf_read is called the higher part of LBA is read and 
>> the resulting tf should have TFLAG_LBA48 set.  But yeah the original 
>> code isn't like that.  If you can fix it up together with the NCQ 
>> path, it would be great.
> ..
> 
> No, I'm not going to touch the mainline path.  tf_read is used to read
> the taskfile, but it doesn't always contain an LBA address, so I'm not so
> sure that forcing TFLAG_ISADDR is always correct.
> 
> And since I don't know that code as well as you, I don't want to end up
> having to help debug something that might break as a result.
> 
> This may warrant two patches.. one to fix the NCQ bug (already posted),
> and perhaps another from you or Jeff if you want to change the behaviour.

Alright, fair enough.

-- 
tejun

Re: [PATCH] libata-eh copy result_tf flags from outgoing tf

[Mark Lord]

Tejun Heo wrote:
> Mark Lord wrote:
>>> Maybe it's better to set ATA_TFLAG_LBA48 | ATA_TFLAG_ISADDR | 
>>> ATA_TFLAG_DEVICE?  After all, all fields are available after that 
>>> memcpy().
>> ..
>>
>> I just did it the same way as the standard version in libata-core does 
>> it.
>> In this particular case, we know it was NCQ, so we know that the original
>> tf->flags have (ATA_TFLAG_LBA48|ATA_TFLAG_ISADDR|ATA_TFLAG_DEVICE) set.
>>
>> I figured better to copy than just overwrite, because new flags might 
>> come along.
> 
> Those flags are to tell which parts of the TF are valid or should be 
> read in case of TFLAG_LBA48.  So, a TF after tf_read should always have 
> TFLAG_ISADDR and TFLAG_DEVICE.  TFLAG_LBA48 works both ways.  If it was 
> set when tf_read is called the higher part of LBA is read and the 
> resulting tf should have TFLAG_LBA48 set.  But yeah the original code 
> isn't like that.  If you can fix it up together with the NCQ path, it 
> would be great.
..

Actually, the TF after tf_read in this case should have TFLAG_LBA in there
as well.  And TFLAG_DEVICE looks unnecessary (but harmless).

Copying from the outgoing tf gets all of those.

Re: [PATCH] libata-eh copy result_tf flags from outgoing tf

[Tejun Heo]

Mark Lord wrote:
> Tejun Heo wrote:
>> Mark Lord wrote:
>>>> Maybe it's better to set ATA_TFLAG_LBA48 | ATA_TFLAG_ISADDR | 
>>>> ATA_TFLAG_DEVICE?  After all, all fields are available after that 
>>>> memcpy().
>>> ..
>>>
>>> I just did it the same way as the standard version in libata-core 
>>> does it.
>>> In this particular case, we know it was NCQ, so we know that the 
>>> original
>>> tf->flags have (ATA_TFLAG_LBA48|ATA_TFLAG_ISADDR|ATA_TFLAG_DEVICE) set.
>>>
>>> I figured better to copy than just overwrite, because new flags might 
>>> come along.
>>
>> Those flags are to tell which parts of the TF are valid or should be 
>> read in case of TFLAG_LBA48.  So, a TF after tf_read should always 
>> have TFLAG_ISADDR and TFLAG_DEVICE.  TFLAG_LBA48 works both ways.  If 
>> it was set when tf_read is called the higher part of LBA is read and 
>> the resulting tf should have TFLAG_LBA48 set.  But yeah the original 
>> code isn't like that.  If you can fix it up together with the NCQ 
>> path, it would be great.
> ..
> 
> Actually, the TF after tf_read in this case should have TFLAG_LBA in there
> as well.  And TFLAG_DEVICE looks unnecessary (but harmless).

Yeah.. right.

> Copying from the outgoing tf gets all of those.

Not for all cases tho.  There are commands which don't necessarily use 
the LBA fields but reading it back on failure is still a good idea.

-- 
tejun

[PATCH] libata-eh set tf flags in NCQ EH result_tf

[Mark Lord]

Fix mis-reporting of NCQ errors by ensuring that result_tf->flags
is properly initialized in libata-eh.  This allows ata_gen_ata_sense()
to report the failed block number correctly to SCSI after a media error
during NCQ.

This patch may also be a candidate for backporting to earlier kernels.
Without this fix, SCSI will fail I/O on the entire request rather
than just the bad sector.  That can be bad for a request that was
merged from many independent read reads from different tasks.

Signed-off-by: Mark Lord <mlord@pobox.com> 
---

This is an alternative version of the original patch
which safely copied the outgoing tf.flags to result_tf.flags.
Both work.  Pick one.

--- upstream/drivers/ata/libata-eh.c	2008-04-18 09:31:15.000000000 -0400
+++ linux/drivers/ata/libata-eh.c	2008-04-23 16:25:38.000000000 -0400
@@ -1402,6 +1402,7 @@
 	/* we've got the perpetrator, condemn it */
 	qc = __ata_qc_from_tag(ap, tag);
 	memcpy(&qc->result_tf, &tf, sizeof(tf));
+	qc->result_tf.flags = ATA_TFLAG_ISADDR | ATA_TFLAG_LBA | ATA_TFLAG_LBA48;
 	qc->err_mask |= AC_ERR_DEV | AC_ERR_NCQ;
 	ehc->i.err_mask &= ~AC_ERR_DEV;
 }

Re: [PATCH] libata-eh set tf flags in NCQ EH result_tf

[Tejun Heo]

Mark Lord wrote:
> Fix mis-reporting of NCQ errors by ensuring that result_tf->flags
> is properly initialized in libata-eh.  This allows ata_gen_ata_sense()
> to report the failed block number correctly to SCSI after a media error
> during NCQ.
> 
> This patch may also be a candidate for backporting to earlier kernels.
> Without this fix, SCSI will fail I/O on the entire request rather
> than just the bad sector.  That can be bad for a request that was
> merged from many independent read reads from different tasks.
> 
> Signed-off-by: Mark Lord <mlord@pobox.com> ---
> 
> This is an alternative version of the original patch
> which safely copied the outgoing tf.flags to result_tf.flags.
> Both work.  Pick one.

Acked-by: Tejun Heo <htejun@gmail.com>

-- 
tejun

Re: [PATCH] libata-eh set tf flags in NCQ EH result_tf

[Jeff Garzik]

Mark Lord wrote:
> Fix mis-reporting of NCQ errors by ensuring that result_tf->flags
> is properly initialized in libata-eh.  This allows ata_gen_ata_sense()
> to report the failed block number correctly to SCSI after a media error
> during NCQ.
> 
> This patch may also be a candidate for backporting to earlier kernels.
> Without this fix, SCSI will fail I/O on the entire request rather
> than just the bad sector.  That can be bad for a request that was
> merged from many independent read reads from different tasks.
> 
> Signed-off-by: Mark Lord <mlord@pobox.com> ---
> 
> This is an alternative version of the original patch
> which safely copied the outgoing tf.flags to result_tf.flags.
> Both work.  Pick one.
> 
> --- upstream/drivers/ata/libata-eh.c    2008-04-18 09:31:15.000000000 -0400
> +++ linux/drivers/ata/libata-eh.c    2008-04-23 16:25:38.000000000 -0400
> @@ -1402,6 +1402,7 @@
>     /* we've got the perpetrator, condemn it */
>     qc = __ata_qc_from_tag(ap, tag);
>     memcpy(&qc->result_tf, &tf, sizeof(tf));
> +    qc->result_tf.flags = ATA_TFLAG_ISADDR | ATA_TFLAG_LBA | 
> ATA_TFLAG_LBA48;
>     qc->err_mask |= AC_ERR_DEV | AC_ERR_NCQ;

applied this version

libata-eh/pmp command sequence on NCQ media error

[Mark Lord]

Tejun,

During testing of my internal version of sata_mv.c here,
I noticed the following:

With no port-multiplier attached, a media error during NCQ
results in an immediate READ_LOG_EXT_10H to retrieve the
task file for the failed I/O.

With a port-multiplier, there is instead a flurry of sata_pmp_read()
attempts.  I'm guessing that the READ_LOG_EXT_10H would normally
then follow those ?

The problem is, on most of the Marvell chips, non-data commands
cannot succeed after any kind of error (until after the port is reset),
so they fail, and we never then get to the READ_LOG_EXT_10H stage.
Oddly, the READ_LOG_EXT_10H command itself is okay (with some errata
goodness tossed in).

So, for sata_mv at least, I'd kinda like to have libata-eh attempt
the READ_LOG_EXT_10H before it tries to (unsuccessfully) access the
per-port SCRs on the PMP.

For now, I'll try and hack something into my local tree to ensure
that things will actually work that way around without any other
unforseen complications.

But for upstream, I'm thinking maybe a HORKAGE flag or something?
I'm still trying to avoid having to pull a lot of libata-eh/pmp code
into sata_mv for local customizations.

Suggestions?

Cheers

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

[Tejun Heo]

Mark Lord wrote:
> Tejun,
> 
> During testing of my internal version of sata_mv.c here,
> I noticed the following:
> 
> With no port-multiplier attached, a media error during NCQ
> results in an immediate READ_LOG_EXT_10H to retrieve the
> task file for the failed I/O.
> 
> With a port-multiplier, there is instead a flurry of sata_pmp_read()
> attempts.  I'm guessing that the READ_LOG_EXT_10H would normally
> then follow those ?
> 
> The problem is, on most of the Marvell chips, non-data commands
> cannot succeed after any kind of error (until after the port is reset),
> so they fail, and we never then get to the READ_LOG_EXT_10H stage.
> Oddly, the READ_LOG_EXT_10H command itself is okay (with some errata
> goodness tossed in).
> 
> So, for sata_mv at least, I'd kinda like to have libata-eh attempt
> the READ_LOG_EXT_10H before it tries to (unsuccessfully) access the
> per-port SCRs on the PMP.
> 
> For now, I'll try and hack something into my local tree to ensure
> that things will actually work that way around without any other
> unforseen complications.
> 
> But for upstream, I'm thinking maybe a HORKAGE flag or something?
> I'm still trying to avoid having to pull a lot of libata-eh/pmp code
> into sata_mv for local customizations.

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

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

[Mark Lord]

Tejun Heo wrote:
> Mark Lord wrote:
..
>>
>> So, for sata_mv at least, I'd kinda like to have libata-eh attempt
>> the READ_LOG_EXT_10H before it tries to (unsuccessfully) access the
>> per-port SCRs on the PMP.
>>
>> For now, I'll try and hack something into my local tree to ensure
>> that things will actually work that way around without any other
>> unforseen complications.
>>
>> But for upstream, I'm thinking maybe a HORKAGE flag or something?
>> I'm still trying to avoid having to pull a lot of libata-eh/pmp code
>> into sata_mv for local customizations.
> 
> 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.
..

Really... Mmm...  :)

Thanks, Tejun.  *You are The Man* !

:)

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

[Mark Lord]

Tejun Heo wrote:
> Mark Lord wrote:
..
>> So, for sata_mv at least, I'd kinda like to have libata-eh attempt
>> the READ_LOG_EXT_10H before it tries to (unsuccessfully) access the
>> per-port SCRs on the PMP.
>>
>> For now, I'll try and hack something into my local tree to ensure
>> that things will actually work that way around without any other
>> unforseen complications.
>>
>> But for upstream, I'm thinking maybe a HORKAGE flag or something?
>> I'm still trying to avoid having to pull a lot of libata-eh/pmp code
>> into sata_mv for local customizations.
> 
> 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? 

Cheers

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

[Mark Lord]

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

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

[Mark Lord]

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

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

[Tejun Heo]

Mark Lord wrote:
>> So why are we taking a hammer to things there?

Hmmm... The reset action might be too heavy handed but maybe keeping 
that categorized as ATA bus error is still a good idea so that multiple 
errors w/ that bit set can trigger speed down.

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

Does the bit get set for the host link or pmp fanout links?

-- 
tejun

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

[Mark Lord]

Tejun Heo wrote:
> Mark Lord wrote:
>>> So why are we taking a hammer to things there?
> 
> Hmmm... The reset action might be too heavy handed but maybe keeping 
> that categorized as ATA bus error is still a good idea so that multiple 
> errors w/ that bit set can trigger speed down.
> 
>> 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.
>> (??)
> 
> Does the bit get set for the host link or pmp fanout links?
..

It's only on the pmp fanout link. 
Dunno why it gets set, but it does.

Cheers

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

[Tejun Heo]

Mark Lord wrote:
> Tejun Heo wrote:
>> Mark Lord wrote:
>>>> So why are we taking a hammer to things there?
>>
>> Hmmm... The reset action might be too heavy handed but maybe keeping 
>> that categorized as ATA bus error is still a good idea so that 
>> multiple errors w/ that bit set can trigger speed down.
>>
>>> 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.
>>> (??)
>>
>> Does the bit get set for the host link or pmp fanout links?
> ..
> 
> It's only on the pmp fanout link. Dunno why it gets set, but it does.

Which PMP are you using?  I've never seen something like that.  If it's 
on fan-out port, you'll want to implement a quirk in libata-pmp.c as it 
will probably happen with other controllers too.

Thanks.

-- 
tejun

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

[Mark Lord]

Tejun Heo wrote:
> Mark Lord wrote:
>> Tejun Heo wrote:
>>> Mark Lord wrote:
>>>>> So why are we taking a hammer to things there?
>>>
>>> Hmmm... The reset action might be too heavy handed but maybe keeping 
>>> that categorized as ATA bus error is still a good idea so that 
>>> multiple errors w/ that bit set can trigger speed down.
>>>
>>>> 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.
>>>> (??)
>>>
>>> Does the bit get set for the host link or pmp fanout links?
>> ..
>>
>> It's only on the pmp fanout link. Dunno why it gets set, but it does.
> 
> Which PMP are you using?  I've never seen something like that.  If it's 
> on fan-out port, you'll want to implement a quirk in libata-pmp.c as it 
> will probably happen with other controllers too.
..

Mmmm.. good point.  I'm seeing it with the Marvell PM right now.
If I remember, I'll try it with the Silicon Image PM on Thursday.

Bedtime here.

cheers!

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

[Mark Lord]

Mark Lord wrote:
> Tejun Heo wrote:
>> Mark Lord wrote:
>>>> So why are we taking a hammer to things there?
>>
>> Hmmm... The reset action might be too heavy handed but maybe keeping 
>> that categorized as ATA bus error is still a good idea so that 
>> multiple errors w/ that bit set can trigger speed down.
>>
>>> 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.
>>> (??)
>>
>> Does the bit get set for the host link or pmp fanout links?
> It's only on the pmp fanout link. Dunno why it gets set, but it does.
..

Oh, wait a sec.. I think I know what's going on.
We're back to the original problem in this thread again:

Mark Lord wrote:
> With no port-multiplier attached, a media error during NCQ
> results in an immediate READ_LOG_EXT_10H to retrieve the
> task file for the failed I/O.
> 
> With a port-multiplier, there is instead a flurry of sata_pmp_read()
> attempts.  I'm guessing that the READ_LOG_EXT_10H would normally
> then follow those ?
> 
> The problem is, on most of the Marvell chips, non-data commands
> cannot succeed after any kind of error (until after the port is reset),
> so they fail, and we never then get to the READ_LOG_EXT_10H stage.
> Oddly, the READ_LOG_EXT_10H command itself is okay (with some errata
> goodness tossed in).
> 
> So, for sata_mv at least, I'd kinda like to have libata-eh attempt
> the READ_LOG_EXT_10H before it tries to (unsuccessfully) access the
> per-port SCRs on the PMP.
..

So what is happening now, is that libata-eh is going and attempting
to access the per-port SCRs *after* the READ_LOG_EXT commands.
And those per-port SCRs are not actually accessible:  the shadow
registers are misbehaving -- known errata -- and cannot be accurately
used without a port reset.

Mmm.. gotta figure out a way to mark the port for RESET,
without having that action taint the commands already analyzed.

I suppose I'll have to just clone some code from libata-eh to
do the READ_LOG_EXT and then qc_complete() those commands
before continuing.  Or something.

???

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

[Tejun Heo]

Mark Lord wrote:
>>> Does the bit get set for the host link or pmp fanout links?
>> It's only on the pmp fanout link. Dunno why it gets set, but it does.
> ..
> 
> Oh, wait a sec.. I think I know what's going on.
> We're back to the original problem in this thread again:

Aieee...

> Mark Lord wrote:
>> With no port-multiplier attached, a media error during NCQ
>> results in an immediate READ_LOG_EXT_10H to retrieve the
>> task file for the failed I/O.
>>
>> With a port-multiplier, there is instead a flurry of sata_pmp_read()
>> attempts.  I'm guessing that the READ_LOG_EXT_10H would normally
>> then follow those ?
>>
>> The problem is, on most of the Marvell chips, non-data commands
>> cannot succeed after any kind of error (until after the port is reset),
>> so they fail, and we never then get to the READ_LOG_EXT_10H stage.
>> Oddly, the READ_LOG_EXT_10H command itself is okay (with some errata
>> goodness tossed in).
>>
>> So, for sata_mv at least, I'd kinda like to have libata-eh attempt
>> the READ_LOG_EXT_10H before it tries to (unsuccessfully) access the
>> per-port SCRs on the PMP.
> ..
> 
> So what is happening now, is that libata-eh is going and attempting
> to access the per-port SCRs *after* the READ_LOG_EXT commands.
> And those per-port SCRs are not actually accessible:  the shadow
> registers are misbehaving -- known errata -- and cannot be accurately
> used without a port reset.
> 
> Mmm.. gotta figure out a way to mark the port for RESET,
> without having that action taint the commands already analyzed.
> 
> I suppose I'll have to just clone some code from libata-eh to
> do the READ_LOG_EXT and then qc_complete() those commands
> before continuing.  Or something.

The usual way to handle this is to clear the controller state (not the 
PHY) from ->error_handler() before calling the generic error_handler. 
Many drivers do similar things - ahci restarts the engine, sil24 calls 
sil24_init_port() and so on.  Does mv need PHY reset to get it working 
again?

-- 
tejun

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

[Mark Lord]

Tejun Heo wrote:
>Mark Lord wrote:
..
>> Mmm.. gotta figure out a way to mark the port for RESET,
>> without having that action taint the commands already analyzed.
>>
>> I suppose I'll have to just clone some code from libata-eh to
>> do the READ_LOG_EXT and then qc_complete() those commands
>> before continuing.  Or something.
> 
> The usual way to handle this is to clear the controller state (not the 
> PHY) from ->error_handler() before calling the generic error_handler. 
> Many drivers do similar things - ahci restarts the engine, sil24 calls 
> sil24_init_port() and so on.  Does mv need PHY reset to get it working again?
..

Heh.. the thoughtful chip designers didn't include an engine-only reset mechanism.
If we reset the engine, it resets the PHY.  That's the whole problem here.
Things would be so much simpler if it did the simple thing..  :)

Cheers

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

[Tejun Heo]

Mark Lord wrote:
> Tejun Heo wrote:
>> Mark Lord wrote:
> ..
>>> Mmm.. gotta figure out a way to mark the port for RESET,
>>> without having that action taint the commands already analyzed.
>>>
>>> I suppose I'll have to just clone some code from libata-eh to
>>> do the READ_LOG_EXT and then qc_complete() those commands
>>> before continuing.  Or something.
>>
>> The usual way to handle this is to clear the controller state (not the 
>> PHY) from ->error_handler() before calling the generic error_handler. 
>> Many drivers do similar things - ahci restarts the engine, sil24 calls 
>> sil24_init_port() and so on.  Does mv need PHY reset to get it working 
>> again?
> ..
> 
> Heh.. the thoughtful chip designers didn't include an engine-only reset 
> mechanism.
> If we reset the engine, it resets the PHY.  That's the whole problem here.
> Things would be so much simpler if it did the simple thing..  :)

Eeeee... nice, so you have to reset the host PHY anyway?  If so, just 
freeze the host port.  It will print some nasty messages (which we'll 
need to subdue) but libata EH won't issue any command to fanout ports 
before host port reset.

-- 
tejun

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

[Mark Lord]

Tejun Heo wrote:
> Mark Lord wrote:
>>> So why are we taking a hammer to things there?
> 
> Hmmm... The reset action might be too heavy handed but maybe keeping 
> that categorized as ATA bus error is still a good idea so that multiple 
> errors w/ that bit set can trigger speed down.
..

Like a lot of those bits, I suppose we need to do the old telco tricks
of keeping a rolling average versus time.  Too many counts of them
in too short a period triggers remedial action, otherwise they're tolerated.

??

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

[Tejun Heo]

Mark Lord wrote:
> Like a lot of those bits, I suppose we need to do the old telco tricks
> of keeping a rolling average versus time.  Too many counts of them
> in too short a period triggers remedial action, otherwise they're 
> tolerated.

Yeah, that's what the current EH does.  What I was suggesting was to set 
AC_ERR_ATA_BUS but not set ATA_EH_RESET.  Hmmm... We'll probably need to 
add another class of errors.  AC_ERR_ATA_BUS_SOFT or something.

Thanks.

-- 
tejun