libata-eh copy result_tf flags from outgoing tf

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 <[email protected]>

Quoted 11 lines · 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 <[email protected]>

Review line 1405

Quoted 8 lines

--- 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

(quoted diff) +1 -0

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

Quoted 1 line

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.

Quoted 8 lines · Mark Lord

Quoted 1 line

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.

Quoted 1 line

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.

Quoted 8 lines · Mark Lord

Quoted 1 line

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.

Quoted 8 lines · Tejun Heo

Quoted 7 lines · Mark Lord

Quoted 1 line

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.

Quoted 3 lines

Quoted 1 line

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.

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 <[email protected]> ---

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

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 <[email protected]> ---

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

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.

Mark Lord wrote:

Quoted 9 lines

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.

Quoted 3 lines

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.

Mark Lord wrote:

Quoted 8 lines

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.

Quoted 3 lines

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 Heo wrote:

Quoted 1 line

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?

Quoted 1 line · Mark Lord

Tejun Heo wrote:

..

Quoted 21 lines

Quoted 1 line

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.

Quoted 1 line

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 <[email protected]>

drivers/ata/libata-eh.c 2008-04-30 17:35:45.000000000 -0400 renamed from drivers/ata/libata-eh.c 2008-04-30 17:35:36.000000000 -0400 +2 -3

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

Signature · 3 lines

To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Quoted 1 line

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

Quoted 1 line · Mark Lord

Quoted 1 line

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.

Quoted 3 lines

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?

Quoted 5 lines · Tejun Heo

Quoted 1 line · Mark Lord

Quoted 1 line

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.

Quoted 5 lines

Quoted 3 lines

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.

Quoted 10 lines · Mark Lord

Quoted 9 lines · Tejun Heo

Quoted 1 line · Mark Lord

Quoted 1 line

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.

Quoted 3 lines

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

Quoted 4 lines

Quoted 1 line

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.

Quoted 3 lines · Tejun Heo

Quoted 1 line · Mark Lord

Quoted 1 line

So why are we taking a hammer to things there?

Quoted 7 lines

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.

Quoted 3 lines

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.

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.

Quoted 2 lines

Quoted 1 line

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:

Quoted 14 lines · Mark Lord

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

Quoted 3 lines

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.

Mark Lord wrote:

Quoted 6 lines

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?

Quoted 1 line · Tejun Heo

Mark Lord wrote:

..

Quoted 7 lines

Quoted 6 lines

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.

Quoted 1 line

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.. :)

Quoted 1 line · Mark Lord

Quoted 1 line

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.