* [PATCH] ata: cleanup SAT error translation
@ 2013-06-14 20:48 Gwendal Grignou
2013-06-17 19:48 ` Tejun Heo
0 siblings, 1 reply; 4+ messages in thread
From: Gwendal Grignou @ 2013-06-14 20:48 UTC (permalink / raw)
To: tj; +Cc: linux-ide, Gwendal Grignou
Remove duplicate Medium Error Entry.
Remove the ABRT entry: it is too broad: when only ABRT is set
we should look at status for more information.
Only if status does not provide information we set ABORTED_COMMAND
sense key.
Tested: When a disk fails, it sets
Status = 0x71 [DRDY DF ERR] , Error = 0x4 [ABRT]
Ensure the sense key is HARDWARE_ERROR.
When there is a simple command syntax error:
Status = 0x51 [DRDY ERR] , Error = 0x4 [ABRT]
Ensure the sense key is ABORTED_COMMAND.
Signed-off-by: Gwendal Grignou <gwendal@google.com>
---
drivers/ata/libata-scsi.c | 4 ----
1 file changed, 4 deletions(-)
diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
index dd310b27..1724189 100644
--- a/drivers/ata/libata-scsi.c
+++ b/drivers/ata/libata-scsi.c
@@ -850,14 +850,10 @@ static void ata_to_sense_error(unsigned id, u8 drv_stat, u8 drv_err, u8 *sk,
{0x01, MEDIUM_ERROR, 0x13, 0x00}, // Address mark not found Address mark not found for data field
/* TRK0 */
{0x02, HARDWARE_ERROR, 0x00, 0x00}, // Track 0 not found Hardware error
- /* Abort & !ICRC */
- {0x04, ABORTED_COMMAND, 0x00, 0x00}, // Aborted command Aborted command
/* Media change request */
{0x08, NOT_READY, 0x04, 0x00}, // Media change request FIXME: faking offline
/* SRV */
{0x10, ABORTED_COMMAND, 0x14, 0x00}, // ID not found Recorded entity not found
- /* Media change */
- {0x08, NOT_READY, 0x04, 0x00}, // Media change FIXME: faking offline
/* ECC */
{0x40, MEDIUM_ERROR, 0x11, 0x04}, // Uncorrectable ECC error Unrecovered read error
/* BBD - block marked bad */
--
1.8.3
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] ata: cleanup SAT error translation
2013-06-14 20:48 [PATCH] ata: cleanup SAT error translation Gwendal Grignou
@ 2013-06-17 19:48 ` Tejun Heo
2013-06-18 17:54 ` [PATCH v2] " Gwendal Grignou
0 siblings, 1 reply; 4+ messages in thread
From: Tejun Heo @ 2013-06-17 19:48 UTC (permalink / raw)
To: Gwendal Grignou; +Cc: linux-ide
Hello, Gwendal.
On Fri, Jun 14, 2013 at 01:48:33PM -0700, Gwendal Grignou wrote:
> Remove duplicate Medium Error Entry.
> Remove the ABRT entry: it is too broad: when only ABRT is set
> we should look at status for more information.
> Only if status does not provide information we set ABORTED_COMMAND
> sense key.
>
> Tested: When a disk fails, it sets
> Status = 0x71 [DRDY DF ERR] , Error = 0x4 [ABRT]
> Ensure the sense key is HARDWARE_ERROR.
> When there is a simple command syntax error:
> Status = 0x51 [DRDY ERR] , Error = 0x4 [ABRT]
> Ensure the sense key is ABORTED_COMMAND.
But this would trigger "no sense translation" warning. The
translation in that function is pretty simplistic anyway. Can you
please update the patch so that there's a comment noting why we aren't
mapping 0x04 and drop the warning?
Thanks.
--
tejun
^ permalink raw reply [flat|nested] 4+ messages in thread
* [PATCH v2] ata: cleanup SAT error translation
2013-06-17 19:48 ` Tejun Heo
@ 2013-06-18 17:54 ` Gwendal Grignou
2013-06-18 18:37 ` Tejun Heo
0 siblings, 1 reply; 4+ messages in thread
From: Gwendal Grignou @ 2013-06-18 17:54 UTC (permalink / raw)
To: tj; +Cc: linux-ide, Gwendal Grignou
Remove duplicate Medium Error Entry.
Fix translations to match SAT2 translation table.
Remove warning messages when translation is not found
when decoding error or status register.
Goes through status register decoding when only ABRT bit
is set in error register.
Tested: When a disk fails, it sets
Status = 0x71 [DRDY DF ERR] , Error = 0x4 [ABRT]
Ensure the sense key is HARDWARE_ERROR.
When there is a simple command syntax error:
Status = 0x51 [DRDY ERR] , Error = 0x4 [ABRT]
Ensure the sense key is ABORTED_COMMAND.
Signed-off-by: Gwendal Grignou <gwendal@google.com>
---
drivers/ata/libata-scsi.c | 33 ++++++++++++++-------------------
1 file changed, 14 insertions(+), 19 deletions(-)
diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
index dd310b27..9db1174 100644
--- a/drivers/ata/libata-scsi.c
+++ b/drivers/ata/libata-scsi.c
@@ -849,25 +849,24 @@ static void ata_to_sense_error(unsigned id, u8 drv_stat, u8 drv_err, u8 *sk,
/* Bad address mark */
{0x01, MEDIUM_ERROR, 0x13, 0x00}, // Address mark not found Address mark not found for data field
/* TRK0 */
- {0x02, HARDWARE_ERROR, 0x00, 0x00}, // Track 0 not found Hardware error
- /* Abort & !ICRC */
- {0x04, ABORTED_COMMAND, 0x00, 0x00}, // Aborted command Aborted command
+ {0x02, HARDWARE_ERROR, 0x00, 0x00}, // Track 0 not found Hardware error
+ /* Abort: 0x04 is not translated here */
/* Media change request */
{0x08, NOT_READY, 0x04, 0x00}, // Media change request FIXME: faking offline
- /* SRV */
- {0x10, ABORTED_COMMAND, 0x14, 0x00}, // ID not found Recorded entity not found
- /* Media change */
- {0x08, NOT_READY, 0x04, 0x00}, // Media change FIXME: faking offline
+ /* SRV/IDNF */
+ {0x10, ILLEGAL_REQUEST, 0x21, 0x00}, // ID not found Logical address out of range
+ /* MC */
+ {0x20, UNIT_ATTENTION, 0x28, 0x00}, // Media Changed Not ready to ready change, medium may have changed
/* ECC */
{0x40, MEDIUM_ERROR, 0x11, 0x04}, // Uncorrectable ECC error Unrecovered read error
/* BBD - block marked bad */
- {0x80, MEDIUM_ERROR, 0x11, 0x04}, // Block marked bad Medium error, unrecovered read error
+ {0x80, MEDIUM_ERROR, 0x11, 0x04}, // Block marked bad Medium error, unrecovered read error
{0xFF, 0xFF, 0xFF, 0xFF}, // END mark
};
static const unsigned char stat_table[][4] = {
/* Must be first because BUSY means no other bits valid */
{0x80, ABORTED_COMMAND, 0x47, 0x00}, // Busy, fake parity for now
- {0x20, HARDWARE_ERROR, 0x00, 0x00}, // Device fault
+ {0x20, HARDWARE_ERROR, 0x44, 0x00}, // Device fault, internal target failure
{0x08, ABORTED_COMMAND, 0x47, 0x00}, // Timed out in xfer, fake parity for now
{0x04, RECOVERED_ERROR, 0x11, 0x00}, // Recovered ECC error Medium error, recovered
{0xFF, 0xFF, 0xFF, 0xFF}, // END mark
@@ -892,13 +891,12 @@ static void ata_to_sense_error(unsigned id, u8 drv_stat, u8 drv_err, u8 *sk,
goto translate_done;
}
}
- /* No immediate match */
- if (verbose)
- printk(KERN_WARNING "ata%u: no sense translation for "
- "error 0x%02x\n", id, drv_err);
}
- /* Fall back to interpreting status bits */
+ /* Fall back to interpreting status bits
+ * Note that if the drv_err has only the ABRT bit set, we decode drv_stat.
+ * ABRT by itself is not descriptive enough
+ */
for (i = 0; stat_table[i][0] != 0xFF; i++) {
if (stat_table[i][0] & drv_stat) {
*sk = stat_table[i][1];
@@ -907,13 +905,10 @@ static void ata_to_sense_error(unsigned id, u8 drv_stat, u8 drv_err, u8 *sk,
goto translate_done;
}
}
- /* No error? Undecoded? */
- if (verbose)
- printk(KERN_WARNING "ata%u: no sense translation for "
- "status: 0x%02x\n", id, drv_stat);
/* We need a sensible error return here, which is tricky, and one
- that won't cause people to do things like return a disk wrongly */
+ * that won't cause people to do things like return a disk wrongly
+ */
*sk = ABORTED_COMMAND;
*asc = 0x00;
*ascq = 0x00;
--
1.8.3
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH v2] ata: cleanup SAT error translation
2013-06-18 17:54 ` [PATCH v2] " Gwendal Grignou
@ 2013-06-18 18:37 ` Tejun Heo
0 siblings, 0 replies; 4+ messages in thread
From: Tejun Heo @ 2013-06-18 18:37 UTC (permalink / raw)
To: Gwendal Grignou; +Cc: linux-ide
Applied to libata/for-3.11 with some description / comment updates.
Thanks!
>From 78062c50d15d6a0adfa09f6e35a6c52abcc9a32d Mon Sep 17 00:00:00 2001
From: Gwendal Grignou <gwendal@google.com>
Date: Tue, 18 Jun 2013 10:54:48 -0700
Subject: [PATCH] libata: cleanup SAT error translation
- Remove duplicate Medium Error Entry.
- Fix translations to match SAT2 translation table.
- Remove warning messages when translation is not found when decoding
error or status register.
- Goes through status register decoding when only ABRT bit is set in
error register.
Tested: When a disk fails, it sets
Status = 0x71 [DRDY DF ERR] , Error = 0x4 [ABRT]
This patch will make the sense key HARDWARE_ERROR instead.
When there is a simple command syntax error:
Status = 0x51 [DRDY ERR] , Error = 0x4 [ABRT]
The sense key remains ABORTED_COMMAND.
tj: Some updates to the description and comments.
Signed-off-by: Gwendal Grignou <gwendal@google.com>
Signed-off-by: Tejun Heo <tj@kernel.org>
---
drivers/ata/libata-scsi.c | 37 +++++++++++++++++--------------------
1 file changed, 17 insertions(+), 20 deletions(-)
diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
index dd310b27..006f1bf 100644
--- a/drivers/ata/libata-scsi.c
+++ b/drivers/ata/libata-scsi.c
@@ -849,25 +849,24 @@ static void ata_to_sense_error(unsigned id, u8 drv_stat, u8 drv_err, u8 *sk,
/* Bad address mark */
{0x01, MEDIUM_ERROR, 0x13, 0x00}, // Address mark not found Address mark not found for data field
/* TRK0 */
- {0x02, HARDWARE_ERROR, 0x00, 0x00}, // Track 0 not found Hardware error
- /* Abort & !ICRC */
- {0x04, ABORTED_COMMAND, 0x00, 0x00}, // Aborted command Aborted command
+ {0x02, HARDWARE_ERROR, 0x00, 0x00}, // Track 0 not found Hardware error
+ /* Abort: 0x04 is not translated here, see below */
/* Media change request */
{0x08, NOT_READY, 0x04, 0x00}, // Media change request FIXME: faking offline
- /* SRV */
- {0x10, ABORTED_COMMAND, 0x14, 0x00}, // ID not found Recorded entity not found
- /* Media change */
- {0x08, NOT_READY, 0x04, 0x00}, // Media change FIXME: faking offline
+ /* SRV/IDNF */
+ {0x10, ILLEGAL_REQUEST, 0x21, 0x00}, // ID not found Logical address out of range
+ /* MC */
+ {0x20, UNIT_ATTENTION, 0x28, 0x00}, // Media Changed Not ready to ready change, medium may have changed
/* ECC */
{0x40, MEDIUM_ERROR, 0x11, 0x04}, // Uncorrectable ECC error Unrecovered read error
/* BBD - block marked bad */
- {0x80, MEDIUM_ERROR, 0x11, 0x04}, // Block marked bad Medium error, unrecovered read error
+ {0x80, MEDIUM_ERROR, 0x11, 0x04}, // Block marked bad Medium error, unrecovered read error
{0xFF, 0xFF, 0xFF, 0xFF}, // END mark
};
static const unsigned char stat_table[][4] = {
/* Must be first because BUSY means no other bits valid */
{0x80, ABORTED_COMMAND, 0x47, 0x00}, // Busy, fake parity for now
- {0x20, HARDWARE_ERROR, 0x00, 0x00}, // Device fault
+ {0x20, HARDWARE_ERROR, 0x44, 0x00}, // Device fault, internal target failure
{0x08, ABORTED_COMMAND, 0x47, 0x00}, // Timed out in xfer, fake parity for now
{0x04, RECOVERED_ERROR, 0x11, 0x00}, // Recovered ECC error Medium error, recovered
{0xFF, 0xFF, 0xFF, 0xFF}, // END mark
@@ -892,13 +891,13 @@ static void ata_to_sense_error(unsigned id, u8 drv_stat, u8 drv_err, u8 *sk,
goto translate_done;
}
}
- /* No immediate match */
- if (verbose)
- printk(KERN_WARNING "ata%u: no sense translation for "
- "error 0x%02x\n", id, drv_err);
}
- /* Fall back to interpreting status bits */
+ /*
+ * Fall back to interpreting status bits. Note that if the drv_err
+ * has only the ABRT bit set, we decode drv_stat. ABRT by itself
+ * is not descriptive enough.
+ */
for (i = 0; stat_table[i][0] != 0xFF; i++) {
if (stat_table[i][0] & drv_stat) {
*sk = stat_table[i][1];
@@ -907,13 +906,11 @@ static void ata_to_sense_error(unsigned id, u8 drv_stat, u8 drv_err, u8 *sk,
goto translate_done;
}
}
- /* No error? Undecoded? */
- if (verbose)
- printk(KERN_WARNING "ata%u: no sense translation for "
- "status: 0x%02x\n", id, drv_stat);
- /* We need a sensible error return here, which is tricky, and one
- that won't cause people to do things like return a disk wrongly */
+ /*
+ * We need a sensible error return here, which is tricky, and one
+ * that won't cause people to do things like return a disk wrongly.
+ */
*sk = ABORTED_COMMAND;
*asc = 0x00;
*ascq = 0x00;
--
1.8.2.1
^ permalink raw reply related [flat|nested] 4+ messages in thread
end of thread, other threads:[~2013-06-18 18:37 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2013-06-14 20:48 [PATCH] ata: cleanup SAT error translation Gwendal Grignou
2013-06-17 19:48 ` Tejun Heo
2013-06-18 17:54 ` [PATCH v2] " Gwendal Grignou
2013-06-18 18:37 ` Tejun Heo
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).