libata: implement dump_id force param

PATCH Applied Tejun Heo 3 participants 5 messages May 23, 2010 – Jul 1, 2014 1 message not in the archive

Subsystems (2)

Documentation libata

lore

Share & tools

lore

Tejun Heo

author

2010-05-23 10:59 UTC #0 lore raw

PATCH 1/1 libata: implement dump_id force param

Add dump_id libata.force parameter. If specified, libata dumps full IDENTIFY data during device configuration. This is to aid debugging.

Signed-off-by Tejun Heo <[email protected]>

Cc Larry Baker <[email protected]>

---
 Documentation/kernel-parameters.txt |    2 ++
 drivers/ata/libata-core.c           |    9 +++++++++
 include/linux/libata.h              |    1 +
 3 files changed, 12 insertions(+)

drivers/ata/libata-core.c +9 -0

Index: ata/drivers/ata/libata-core.c
===================================================================
@@ -2122,6 +2122,14 @@ retry:
 		goto err_out;
 	}
 
+	if (dev->horkage & ATA_HORKAGE_DUMP_ID) {
+		ata_dev_printk(dev, KERN_DEBUG, "dumping IDENTIFY data, "
+			       "class=%d may_fallback=%d tried_spinup=%d\n",
+			       class, may_fallback, tried_spinup);
+		print_hex_dump(KERN_DEBUG, "", DUMP_PREFIX_OFFSET,
+			       16, 2, id, ATA_ID_WORDS * sizeof(*id), true);
+	}
+
 	/* Falling back doesn't make sense if ID data was read
 	 * successfully at least once.
 	 */

@@ -6372,6 +6380,7 @@ static int __init ata_parse_force_one(ch
 		{ "3.0Gbps",	.spd_limit	= 2 },
 		{ "noncq",	.horkage_on	= ATA_HORKAGE_NONCQ },
 		{ "ncq",	.horkage_off	= ATA_HORKAGE_NONCQ },
+		{ "dump_id",	.horkage_on	= ATA_HORKAGE_DUMP_ID },
 		{ "pio0",	.xfer_mask	= 1 << (ATA_SHIFT_PIO + 0) },
 		{ "pio1",	.xfer_mask	= 1 << (ATA_SHIFT_PIO + 1) },
 		{ "pio2",	.xfer_mask	= 1 << (ATA_SHIFT_PIO + 2) },

include/linux/libata.h +1 -0

Index: ata/include/linux/libata.h
===================================================================
@@ -386,6 +386,7 @@ enum {
 	ATA_HORKAGE_1_5_GBPS	= (1 << 13),	/* force 1.5 Gbps */
 	ATA_HORKAGE_NOSETXFER	= (1 << 14),	/* skip SETXFER, SATA only */
 	ATA_HORKAGE_BROKEN_FPDMA_AA	= (1 << 15),	/* skip AA */
+	ATA_HORKAGE_DUMP_ID	= (1 << 16),	/* dump IDENTIFY data */
 
 	 /* DMA mask for user DMA control: User visible values; DO NOT
 	    renumber */

Documentation/kernel-parameters.txt +2 -0

Index: ata/Documentation/kernel-parameters.txt
===================================================================
@@ -1227,6 +1227,8 @@ and is between 256 and 4096 characters.
 			* nohrst, nosrst, norst: suppress hard, soft
                           and both resets.
 
+			* dump_id: dump IDENTIFY data.
+
 			If there are multiple matching configurations changing
 			the same attribute, the last one is used.

Jeff Garzik

Applied

2010-05-25 23:42 UTC #1 lore raw

Quoted 9 lines · Tejun Heo

Add dump_id libata.force parameter. If specified, libata dumps full IDENTIFY data during device configuration. This is to aid debugging.
Signed-off-by Tejun Heo<[email protected]>
Cc Larry Baker<[email protected]>
---
  Documentation/kernel-parameters.txt |    2 ++
  drivers/ata/libata-core.c           |    9 +++++++++
  include/linux/libata.h              |    1 +

applied

A message that is not in the archive (sent off-list or not yet received): [email protected]

Larry Baker

2014-07-01 18:57 UTC #3 lore raw

Jeff and Tejun,

I see what happened. Tejun sent me two sets of patches: one to implement the driver dump_id kernel parameter and the other to fix the problem with the buggy CF card. (It lies that it supports ATA MULTI -- which is actually required by the standard!) Jeff only saw the first set, which is what he applied. They are in the current Linux source tree. What are missing are the patches to recover from multi-block transfer failures by falling back to use single-block transfers. Here's Tejun's e-mail to me with those patches (ready to go?).

Thank you again,

Larry Baker
US Geological Survey
650-329-5608
[email protected]

Quoted 111 lines · Tejun Heo

Quoted 10 lines · Tejun Heo

Quoted 10 lines
If the failed command is one of the READ/WRITE MULTI commands (entries 0-4 in ata_rw_cmds[]), and the device response is ABORT, use your HORKAGE mechanism to set a flag like ATA_HORKAGE_BROKEN_MULTI to skip setting dev->multi_count in libata-core.c (the code in 2., above), i.e., disable multi-sector transfers, then reconfigure the device to disable multi-sector transfers (call ata_dev_configure(), I guess, so the new configuration gets printed out in dmesg) and retry the transfer. Any future calls to ata_dev_configure() for that device will not attempt to enable multi-sector transfers because the ATA_HORKAGE_BROKEN_MULTI is set:

Quoted 5 lines

Review
Quoted 2 lines
(quoted diff) +2 -0
+        /* don't try to enable R/W multi if it is broken */
+        if (!(dev->horkage & ATA_HORKAGE_BROKEN_MULTI))
So, turn off MULTI on ABORT.... Oh yeah, I can add that to the existing speed down logic. Would you be available to test patches?

Something like the following. Please test whether it works as expected. It should turn off muti after two device errors during partition scan.

Thanks.

drivers/ata/libata-core.c +2 -1

index c47373f..1f97189 100644
@@ -2416,7 +2416,8 @@ int ata_dev_configure(struct ata_device *dev)
 		dev->n_sectors = ata_id_n_sectors(id);
 
 		/* get current R/W Multiple count setting */
-		if ((dev->id[47] >> 8) == 0x80 && (dev->id[59] & 0x100)) {
+		if (!(dev->flags & ATA_DFLAG_MULTI_OFF) &&
+		    (dev->id[47] >> 8) == 0x80 && (dev->id[59] & 0x100)) {
 			unsigned int max = dev->id[47] & 0xff;
 			unsigned int cnt = dev->id[59] & 0xff;
 			/* only recognize/allow powers of two here */

drivers/ata/libata-eh.c +14 -4

index f77a673..60b352f 100644
@@ -53,6 +53,7 @@ enum {
 	ATA_EH_SPDN_SPEED_DOWN		= (1 << 1),
 	ATA_EH_SPDN_FALLBACK_TO_PIO	= (1 << 2),
 	ATA_EH_SPDN_KEEP_ERRORS		= (1 << 3),
+	ATA_EH_SPDN_MULTI_OFF		= (1 << 4),
 
 	/* error flags */
 	ATA_EFLAG_IS_IO			= (1 << 0),

@@ -1805,13 +1806,13 @@ static int speed_down_verdict_cb(struct ata_ering_entry *ent, void *void_arg)
 *	   occurred during last 5 mins, SPEED_DOWN and FALLBACK_TO_PIO.
 *
 *	2. If more than one DUBIOUS_TOUT_HSM or DUBIOUS_UNK_DEV errors
- *	   occurred during last 5 mins, NCQ_OFF.
+ *	   occurred during last 5 mins, NCQ_OFF and MULTI_OFF.
 *
 *	3. If more than 8 ATA_BUS, TOUT_HSM or UNK_DEV errors
 *	   ocurred during last 5 mins, FALLBACK_TO_PIO
 *
 *	4. If more than 3 TOUT_HSM or UNK_DEV errors occurred
- *	   during last 10 mins, NCQ_OFF.
+ *	   during last 10 mins, NCQ_OFF and MULTI_OFF.
 *
 *	5. If more than 3 ATA_BUS or TOUT_HSM errors, or more than 6
 *	   UNK_DEV errors occurred during last 10 mins, SPEED_DOWN.

@@ -1841,7 +1842,8 @@ static unsigned int ata_eh_speed_down_verdict(struct ata_device *dev)
 
 	if (arg.nr_errors[ATA_ECAT_DUBIOUS_TOUT_HSM] +
 	    arg.nr_errors[ATA_ECAT_DUBIOUS_UNK_DEV] > 1)
-		verdict |= ATA_EH_SPDN_NCQ_OFF | ATA_EH_SPDN_KEEP_ERRORS;
+		verdict |= ATA_EH_SPDN_NCQ_OFF | ATA_EH_SPDN_MULTI_OFF |
+			ATA_EH_SPDN_KEEP_ERRORS;
 
 	if (arg.nr_errors[ATA_ECAT_ATA_BUS] +
 	    arg.nr_errors[ATA_ECAT_TOUT_HSM] +

@@ -1855,7 +1857,7 @@ static unsigned int ata_eh_speed_down_verdict(struct ata_device *dev)
 
 	if (arg.nr_errors[ATA_ECAT_TOUT_HSM] +
 	    arg.nr_errors[ATA_ECAT_UNK_DEV] > 3)
-		verdict |= ATA_EH_SPDN_NCQ_OFF;
+		verdict |= ATA_EH_SPDN_NCQ_OFF | ATA_EH_SPDN_MULTI_OFF;
 
 	if (arg.nr_errors[ATA_ECAT_ATA_BUS] +
 	    arg.nr_errors[ATA_ECAT_TOUT_HSM] > 3 ||

@@ -1908,6 +1910,14 @@ static unsigned int ata_eh_speed_down(struct ata_device *dev,
 		goto done;
 	}
 
+	/* turn off multi? */
+	if ((verdict & ATA_EH_SPDN_MULTI_OFF) && (dev->flags & ATA_DFLAG_PIO)) {
+		dev->flags |= ATA_DFLAG_MULTI_OFF;
+		ata_dev_printk(dev, KERN_WARNING,
+			       "PIO multi disabled due to excessive errors\n");
+		goto done;
+	}
+
 	/* speed down? */
 	if (verdict & ATA_EH_SPDN_SPEED_DOWN) {
 		/* speed down SATA link speed if possible */

include/linux/libata.h +1 -0

index ee84e7e..a145d80 100644
@@ -147,6 +147,7 @@ enum {
 	ATA_DFLAG_DUBIOUS_XFER	= (1 << 16), /* data transfer not verified */
 	ATA_DFLAG_NO_UNLOAD	= (1 << 17), /* device doesn't support unload */
 	ATA_DFLAG_UNLOCK_HPA	= (1 << 18), /* unlock HPA */
+	ATA_DFLAG_MULTI_OFF	= (1 << 19), /* turn off PIO multi mode */
 	ATA_DFLAG_INIT_MASK	= (1 << 24) - 1,
 
 	ATA_DFLAG_DETACH	= (1 << 24),

Quoted 30 lines · Larry Baker

Jeff and Tejun,
We had some exchanges in 2010 regarding CF card problems. Tejun wrote patches which we use on our desktop playback systems and Jeff said he applied them (below). However, they seem to not be in any of the kernel source trees when i browse http://lxr.free-electrons.com/source/drivers/ata/. I think we may be running into similar problems with the CF cards used in some embedded Linux data acquisition systems we bought that are running a 2.6 kernel. I was going to advise them to look for Tejun's work in a later kernel. But, alas, his work is not there. Any idea what happened to Jeff's "applied"? I can forward our e-mail conversations, which include the patches Tejun sent to me.
Thank you,
Larry Baker
US Geological Survey
650-329-5608
[email protected]
Quoted 14 lines · Jeff Garzik
Quoted 9 lines · Tejun Heo
Add dump_id libata.force parameter. If specified, libata dumps full IDENTIFY data during device configuration. This is to aid debugging.
Signed-off-by Tejun Heo<[email protected]>
Cc Larry Baker<[email protected]>
---
 Documentation/kernel-parameters.txt |    2 ++
 drivers/ata/libata-core.c           |    9 +++++++++
 include/linux/libata.h              |    1 +
applied

Tejun Heo

author

2014-07-01 20:51 UTC #4 lore raw

Hello, Larry.

Quoted 9 lines · Larry Baker

I see what happened. Tejun sent me two sets of patches: one to implement the driver dump_id kernel parameter and the other to fix the problem with the buggy CF card. (It lies that it supports ATA MULTI -- which is actually required by the standard!) Jeff only saw the first set, which is what he applied. They are in the current Linux source tree. What are missing are the patches to recover from multi-block transfer failures by falling back to use single-block transfers. Here's Tejun's e-mail to me with those patches (ready to go?).

I dug out the original patch. It looks like I was waiting for Tested-by. Has anyone tested the patch at the time?

Thanks.

Signature · 1 line

tejun

Larry Baker

2014-07-01 21:08 UTC #5 lore raw

Tejun,

I understand. We have moved buildings since 2010 and I do not know whether I can find the CF cards that had the problem. I have a deadline in 3 weeks that I must work on. I will follow up with you after that.

Thank you,

Larry Baker
US Geological Survey
650-329-5608
[email protected]

Quoted 20 lines · Tejun Heo

Hello, Larry.
Quoted 9 lines · Larry Baker
I see what happened. Tejun sent me two sets of patches: one to implement the driver dump_id kernel parameter and the other to fix the problem with the buggy CF card. (It lies that it supports ATA MULTI -- which is actually required by the standard!) Jeff only saw the first set, which is what he applied. They are in the current Linux source tree. What are missing are the patches to recover from multi-block transfer failures by falling back to use single-block transfers. Here's Tejun's e-mail to me with those patches (ready to go?).
I dug out the original patch. It looks like I was waiting for Tested-by. Has anyone tested the patch at the time?
Thanks.
Signature · 1 line
tejun