libata: add human-readable error value decoding

PATCH In review Robert Hancock 7 participants 14 messages May 10, 2007 – May 11, 2007 libata

Share & tools

Robert Hancock

author

2007-05-10 02:19 UTC #0 lore raw

PATCH 1/1 libata: add human-readable error value decoding

This adds human-readable decoding of the ATA status and error registers (similar to what drivers/ide does) as well as the SATA Serror register to libata error handling output. This prevents the need to pore through standards documents to figure out the meaning of the bits in these registers when looking at error reports. Some bits that drivers/ide decoded are not decoded here, since the bits are either command-dependent or obsolete, and properly parsing them would add too much complexity.

Signed-off-by Robert Hancock <[email protected]>

drivers/ata/libata-eh.c +0 -0

@@ -1523,6 +1523,27 @@ static void ata_eh_report(struct ata_por

			ata_port_printk(ap, KERN_ERR, "(%s)\n", desc);
	}

+ if (ehc->i.serror)
+ ata_port_printk(ap, 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 ? "RecovDataErr " : "",
+		  ehc->i.serror & SERR_COMM_RECOVERED ? "RecovCommErr " : "",
+		  ehc->i.serror & SERR_DATA ? "UnrecovDataErr " : "",
+		  ehc->i.serror & SERR_PERSISTENT ? "PersistErr " : "",
+		  ehc->i.serror & SERR_PROTOCOL ? "ProtocolErr " : "",
+		  ehc->i.serror & SERR_INTERNAL ? "HostInternalErr " : "",
+		  ehc->i.serror & SERR_PHYRDY_CHG ? "PHYRdyChg " : "",
+		  ehc->i.serror & SERR_PHY_INT_ERR ? "PHYInternalErr " : "",
+		  ehc->i.serror & SERR_COMM_WAKE ? "CommWake " : "",
+		  ehc->i.serror & SERR_10B_8B_ERR ? "10B8BErr " : "",
+		  ehc->i.serror & SERR_DISPARITY ? "Disparity " : "",
+		  ehc->i.serror & SERR_CRC ? "CRCErr " : "",
+		  ehc->i.serror & SERR_HANDSHAKE ? "HandshakeErr " : "",
+		  ehc->i.serror & SERR_LINK_SEQ_ERR ? "LinkSeqErr " : "",
+		  ehc->i.serror & SERR_TRANS_ST_ERROR ? "TransStatTransErr " : "",
+		  ehc->i.serror & SERR_UNRECOG_FIS ? "UnrecogFIS " : "",
+		  ehc->i.serror & SERR_DEV_XCHG ? "DevExchanged " : "" );
+
	for (tag = 0; tag < ATA_MAX_QUEUE; tag++) {
		static const char *dma_str[] = {
			[DMA_BIDIRECTIONAL]	= "bidi",
@@ -1552,6 +1573,29 @@ static void ata_eh_report(struct ata_por
			res->hob_feature, res->hob_nsect,
			res->hob_lbal, res->hob_lbam, res->hob_lbah,
			res->device, qc->err_mask, ata_err_string(qc->err_mask));

+
+ if (res->command & (ATA_BUSY | ATA_DRDY | ATA_DF | ATA_DRQ |
+ ATA_ERR) ) {
+ if (res->command & ATA_BUSY)
+ ata_dev_printk(qc->dev, KERN_ERR,
+ "status: {Busy}\n" );
+ else
+ ata_dev_printk(qc->dev, KERN_ERR,

+				  "status: {%s%s%s%s}\n",
+				  res->command & ATA_DRDY ? "DRDY " : "",
+				  res->command & ATA_DF ? "DF " : "",
+				  res->command & ATA_DRQ ? "DRQ " : "",
+				  res->command & ATA_ERR ? "ERR " : "" );

+ }
+
+ if (cmd->command != ATA_CMD_PACKET &&
+ (res->feature & (ATA_ICRC | ATA_UNC | ATA_IDNF | ATA_ABORTED)))
+ ata_dev_printk(qc->dev, KERN_ERR,

+			  "error: {%s%s%s%s}\n",
+			  res->feature & ATA_ICRC ? "ICRC " : "",
+			  res->feature & ATA_UNC ? "UNC " : "",
+			  res->feature & ATA_IDNF ? "IDNF " : "",
+			  res->feature & ATA_ABORTED ? "ABRT " : "" );
	}
}

include/linux/ata.h +0 -0

@@ -223,6 +223,15 @@ enum {

	SERR_PROTOCOL		= (1 << 10), /* protocol violation */
	SERR_INTERNAL		= (1 << 11), /* host internal error */
	SERR_PHYRDY_CHG		= (1 << 16), /* PHY RDY changed */

+ SERR_PHY_INT_ERR = (1 << 17), /* PHY internal error */
+ SERR_COMM_WAKE = (1 << 18), /* Comm wake */
+ SERR_10B_8B_ERR = (1 << 19), /* 10b to 8b decode error */
+ SERR_DISPARITY = (1 << 20), /* Disparity */
+ SERR_CRC = (1 << 21), /* CRC error */
+ SERR_HANDSHAKE = (1 << 22), /* Handshake error */
+ SERR_LINK_SEQ_ERR = (1 << 23), /* Link sequence error */
+ SERR_TRANS_ST_ERROR = (1 << 24), /* Transport state transition error */
+ SERR_UNRECOG_FIS = (1 << 25), /* Unrecognized FIS */

	SERR_DEV_XCHG		= (1 << 26), /* device exchanged */

	/* struct ata_taskfile flags */

Tejun Heo

2007-05-10 09:43 UTC #1 lore raw

Quoted 33 lines · Robert Hancock

This adds human-readable decoding of the ATA status and error registers (similar
to what drivers/ide does) as well as the SATA Serror register to libata error
handling output. This prevents the need to pore through standards documents to figure out the meaning of the bits in these registers when looking at error
reports. Some bits that drivers/ide decoded are not decoded here, since the bits
are either command-dependent or obsolete, and properly parsing them would add
too much complexity.
Signed-off-by Robert Hancock <[email protected]>
--- linux-2.6.21.1/drivers/ata/libata-eh.c 2007-04-27
15:49:26.000000000 -0600
+++ linux-2.6.21.1edit/drivers/ata/libata-eh.c 2007-05-09
19:47:53.000000000 -0600
(quoted diff) +10 -0
@@ -1523,6 +1523,27 @@ static void ata_eh_report(struct ata_por
            ata_port_printk(ap, KERN_ERR, "(%s)\n", desc);
    }
 
+    if (ehc->i.serror)
+        ata_port_printk(ap, 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 ? "RecovDataErr " : "",
+          ehc->i.serror & SERR_COMM_RECOVERED ? "RecovCommErr " : "",
+          ehc->i.serror & SERR_DATA ? "UnrecovDataErr " : "",
+          ehc->i.serror & SERR_PERSISTENT ? "PersistErr " : "",
+          ehc->i.serror & SERR_PROTOCOL ? "ProtocolErr " : "",
+          ehc->i.serror & SERR_INTERNAL ? "HostInternalErr " : "",
+          ehc->i.serror & SERR_PHYRDY_CHG ? "PHYRdyChg " : "",

Review static void ata_eh_report(struct ata_por · line 1545

Quoted 10 lines

(quoted diff) +10 -0

+          ehc->i.serror & SERR_PHY_INT_ERR ? "PHYInternalErr " : "",
+          ehc->i.serror & SERR_COMM_WAKE ? "CommWake " : "",
+          ehc->i.serror & SERR_10B_8B_ERR ? "10B8BErr " : "",
+          ehc->i.serror & SERR_DISPARITY ? "Disparity " : "",
+          ehc->i.serror & SERR_CRC ? "CRCErr " : "",
+          ehc->i.serror & SERR_HANDSHAKE ? "HandshakeErr " : "",
+          ehc->i.serror & SERR_LINK_SEQ_ERR ? "LinkSeqErr " : "",
+          ehc->i.serror & SERR_TRANS_ST_ERROR ? "TransStatTransErr " : "",
+          ehc->i.serror & SERR_UNRECOG_FIS ? "UnrecogFIS " : "",
+          ehc->i.serror & SERR_DEV_XCHG ? "DevExchanged " : "" );

I'm not really convinced whether this is necessary. The human readable form is also a bit cryptic and can get quite long. So, mild NACK from me.

Signature · 1 line

tejun

Mark Lord

2007-05-10 13:24 UTC #2 lore raw

Quoted 4 lines · Tejun Heo

Quoted 3 lines · Robert Hancock
This adds human-readable decoding of the ATA status and error registers (similar
to what drivers/ide does) as well as the SATA Serror register to libata

...

Quoted 2 lines

I'm not really convinced whether this is necessary. The human readable form is also a bit cryptic and can get quite long. So, mild NACK from me.

Same here, but I would like to see it in there under a CONFIG_DEBUG_LIBATA kernel build option or something. Kind of like the "FANCY_STATUS_DUMPS" flag that drivers/ide used to have for this kind of stuff.

Cheers

Jeff Garzik

2007-05-10 16:40 UTC #3 lore raw

Quoted 3 lines · Mark Lord

Same here, but I would like to see it in there under a CONFIG_DEBUG_LIBATA kernel build option or something. Kind of like the "FANCY_STATUS_DUMPS" flag that drivers/ide used to have for this kind of stuff.

The long term goal is to enable verbose output with a module option and/or sysfs knob, rather than a compile-time switch.

That's what ata_msg_xxx is for. We just don't have the knob yet. Patches welcome...

Jeff

Mark Lord

2007-05-10 21:33 UTC #4 lore raw

Quoted 7 lines · Jeff Garzik

Quoted 3 lines · Mark Lord
Same here, but I would like to see it in there under a CONFIG_DEBUG_LIBATA kernel build option or something. Kind of like the "FANCY_STATUS_DUMPS" flag that drivers/ide used to have for this kind of stuff.
The long term goal is to enable verbose output with a module option and/or sysfs knob, rather than a compile-time switch.

If we're compiling the messages into the kernel regardless, then it doesn't really make much sense to NOT show all of them on the error paths.

This stuff (fancy status dumps) is mostly just for the error paths, where more information is always a good thing.

Controlling the rest of the ata_msg_xxx stuff from sysctl is fine for non-error paths.

Cheers

Jeff Garzik

2007-05-10 21:42 UTC #5 lore raw

Quoted 3 lines · Mark Lord

If we're compiling the messages into the kernel regardless, then it doesn't really make much sense to NOT show all of them on the error paths.

Not true. Uncontrolled message spewage inevitably results in critical information scrolling off the screen, before a user can take a digital photo of the output... Or of users being confused by subsequent error fallout (i.e. multiple oopses reporting problem).

Moderation and restraint still have roles to play... :)

Jeff

Robert Hancock

author

2007-05-10 23:32 UTC #6 lore raw

Quoted 11 lines · Jeff Garzik

Quoted 3 lines · Mark Lord
If we're compiling the messages into the kernel regardless, then it doesn't really make much sense to NOT show all of them on the error paths.
Not true. Uncontrolled message spewage inevitably results in critical information scrolling off the screen, before a user can take a digital photo of the output... Or of users being confused by subsequent error fallout (i.e. multiple oopses reporting problem).
Moderation and restraint still have roles to play... :)
Jeff

I don't think this is as big of a deal here as in other cases, like oops output. With libata errors, if they're at the console (which they'd have to be to see these messages), unless something has actually caused a panic the scrollback buffer should still be functional and they'd be able to see the entire output..

Signature · 3 lines

Robert Hancock      Saskatoon, SK, Canada
To email, remove "nospam" from [email protected]
Home Page: http://www.roberthancock.com/

Jeff Garzik

2007-05-10 23:47 UTC #7 lore raw

Quoted 1 line · Robert Hancock

I don't think this is as big of a deal here as in other cases, like oops output. With libata errors, if they're at the console (which they'd have to be to see these messages), unless something has actually caused a panic the scrollback buffer should still be functional and they'd be able to see the entire output..

Scrollback rarely works as planned, for me. Overall, a balance must be found.

More information is more helpful. But.

There are downsides to spewing everything possible, upon error. You cause logging to the possibly problematic disk, you push older messages out of the printk ring buffer, etc., etc.

Jeff

Alan Cox

2007-05-11 00:51 UTC #8 lore raw

Quoted 8 lines

Scrollback rarely works as planned, for me. Overall, a balance must be found.
More information is more helpful. But.
There are downsides to spewing everything possible, upon error. You cause logging to the possibly problematic disk, you push older messages out of the printk ring buffer, etc., etc.

Get yourself a Voodoo5 or similar card cheap off ebay. The firmware on most of them doesn't clear the top 30MB of RAM on a reboot/PCI reset which makes them excellent debug buffers providing you empty the buffer before you run the X server.

Alan

Robert Hancock

author

2007-05-10 23:30 UTC #9 lore raw

In reply to Tejun Heo

Quoted 21 lines · Tejun Heo

Quoted 10 lines

(quoted diff) +10 -0

+    if (ehc->i.serror)
+        ata_port_printk(ap, 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 ? "RecovDataErr " : "",
+          ehc->i.serror & SERR_COMM_RECOVERED ? "RecovCommErr " : "",
+          ehc->i.serror & SERR_DATA ? "UnrecovDataErr " : "",
+          ehc->i.serror & SERR_PERSISTENT ? "PersistErr " : "",
+          ehc->i.serror & SERR_PROTOCOL ? "ProtocolErr " : "",
+          ehc->i.serror & SERR_INTERNAL ? "HostInternalErr " : "",
+          ehc->i.serror & SERR_PHYRDY_CHG ? "PHYRdyChg " : "",

Review

Quoted 10 lines

(quoted diff) +10 -0

+          ehc->i.serror & SERR_PHY_INT_ERR ? "PHYInternalErr " : "",
+          ehc->i.serror & SERR_COMM_WAKE ? "CommWake " : "",
+          ehc->i.serror & SERR_10B_8B_ERR ? "10B8BErr " : "",
+          ehc->i.serror & SERR_DISPARITY ? "Disparity " : "",
+          ehc->i.serror & SERR_CRC ? "CRCErr " : "",
+          ehc->i.serror & SERR_HANDSHAKE ? "HandshakeErr " : "",
+          ehc->i.serror & SERR_LINK_SEQ_ERR ? "LinkSeqErr " : "",
+          ehc->i.serror & SERR_TRANS_ST_ERROR ? "TransStatTransErr " : "",
+          ehc->i.serror & SERR_UNRECOG_FIS ? "UnrecogFIS " : "",
+          ehc->i.serror & SERR_DEV_XCHG ? "DevExchanged " : "" );

Quoted 3 lines

I'm not really convinced whether this is necessary. The human readable form is also a bit cryptic and can get quite long. So, mild NACK from me.

It certainly seems useful when debugging hotplug issues or random SATA problems which end up being caused by communication problems. Without this output, Joe User stands no chance of figuring out what's going on, and neither does Joe libata Developer unless they really care to dig through the spec and count bits to figure out what they mean. At least with this you can see that there was a CRC error, etc. and go from that..

Signature · 3 lines

Robert Hancock      Saskatoon, SK, Canada
To email, remove "nospam" from [email protected]
Home Page: http://www.roberthancock.com/

Chuck Ebbert

2007-05-11 16:48 UTC #10 lore raw

Quoted 10 lines · Robert Hancock

Quoted 9 lines
Review
Quoted 4 lines
+ ehc->i.serror & SERR_TRANS_ST_ERROR ? "TransStatTransErr "
: "",
(quoted diff) +2 -0
+          ehc->i.serror & SERR_UNRECOG_FIS ? "UnrecogFIS " : "",
+          ehc->i.serror & SERR_DEV_XCHG ? "DevExchanged " : "" );
I'm not really convinced whether this is necessary. The human readable form is also a bit cryptic and can get quite long. So, mild NACK from me.

Quoted 7 lines

It certainly seems useful when debugging hotplug issues or random SATA problems which end up being caused by communication problems. Without this output, Joe User stands no chance of figuring out what's going on, and neither does Joe libata Developer unless they really care to dig through the spec and count bits to figure out what they mean. At least with this you can see that there was a CRC error, etc. and go from that..

Why not just document the error messages?

And the scsi ones too, I can't seem to find what the sense codes mean.

Tejun Heo

2007-05-11 17:12 UTC #11 lore raw

Quoted 17 lines · Chuck Ebbert

Quoted 8 lines · Robert Hancock
Quoted 8 lines
Review
Quoted 4 lines
+ ehc->i.serror & SERR_TRANS_ST_ERROR ? "TransStatTransErr "
: "",
(quoted diff) +2 -0
+          ehc->i.serror & SERR_UNRECOG_FIS ? "UnrecogFIS " : "",
+          ehc->i.serror & SERR_DEV_XCHG ? "DevExchanged " : "" );
I'm not really convinced whether this is necessary. The human readable form is also a bit cryptic and can get quite long. So, mild NACK from me.
Quoted 7 lines
It certainly seems useful when debugging hotplug issues or random SATA problems which end up being caused by communication problems. Without this output, Joe User stands no chance of figuring out what's going on, and neither does Joe libata Developer unless they really care to dig through the spec and count bits to figure out what they mean. At least with this you can see that there was a CRC error, etc. and go from that..

Quoted 3 lines

Why not just document the error messages?
And the scsi ones too, I can't seem to find what the sense codes mean.

They are well documented elsewhere - the standard documents. For sense codes, t10.org. For SError bits, t13.org. You can get drafts free of charge.

Signature · 1 line

tejun

Robert Hancock

author

2007-05-11 23:10 UTC #12 lore raw

Quoted 17 lines · Tejun Heo

Quoted 16 lines · Chuck Ebbert
Quoted 15 lines · Robert Hancock
Quoted 8 lines
Review
Quoted 4 lines
+ ehc->i.serror & SERR_TRANS_ST_ERROR ? "TransStatTransErr "
: "",
(quoted diff) +2 -0
+          ehc->i.serror & SERR_UNRECOG_FIS ? "UnrecogFIS " : "",
+          ehc->i.serror & SERR_DEV_XCHG ? "DevExchanged " : "" );
I'm not really convinced whether this is necessary. The human readable form is also a bit cryptic and can get quite long. So, mild NACK from me.
It certainly seems useful when debugging hotplug issues or random SATA problems which end up being caused by communication problems. Without this output, Joe User stands no chance of figuring out what's going on, and neither does Joe libata Developer unless they really care to dig through the spec and count bits to figure out what they mean. At least with this you can see that there was a CRC error, etc. and go from that..

Quoted 7 lines

Quoted 3 lines
Why not just document the error messages?
And the scsi ones too, I can't seem to find what the sense codes mean.
They are well documented elsewhere - the standard documents. For sense codes, t10.org. For SError bits, t13.org. You can get drafts free of charge.

The ATA ones are more of a pain in that regard than SCSI though - SCSI has all distinct error codes for different errors, whereas ATA has bitmasks for everything..

Signature · 3 lines

Robert Hancock      Saskatoon, SK, Canada
To email, remove "nospam" from [email protected]
Home Page: http://www.roberthancock.com/

Jeff Garzik

2007-05-11 23:22 UTC #13 lore raw

Quoted 1 line · Robert Hancock

The ATA ones are more of a pain in that regard than SCSI though - SCSI has all distinct error codes for different errors, whereas ATA has bitmasks for everything..

That should not affect implementation. Either way, a table-driven approach can easily work.

I favor decoding the SError status bits, but your names were far too long. "ProtocolErr" should be "Proto". "10B8BErr" should be "10b8b". HostInternalErr to HostInt. PHYInternalErr to PHYInt. etc.

Jeff