Skip to contentSkip to conversation

Can you please test the attached patch for nv hardreset problem?

PATCH Tejun Heo 2 participants 12 messages Oct 5, 2008 – Oct 21, 2008 libata

lore
Share & tools
lore

Tejun Heo

author
#0 lore raw

Hello,

Please verify whether the attached patch fixes all hardreset / probing related issues. The patch is on top of v2.6.27-rc8. Sorry about all the mess but it was quite a confusing problem. Here's what went down.

* During 2.6.27-rcX, libata switched to prefer hardreset.

* Which broke, generic flavor of sata_nv. Some sata_nv's fail to bring the link up after hardreset.

* A fix patch went in to disable hardreset for generic flavor. But it accidentaly disabled hardreset for all flavors.

* Which broke nf2/3 and CK804, because their hotplug interrupt bits never get cleared. They're apparently cleared by hardreset sequence.

* Another fix patch went in to reinstate nv_hardreset for nf2/3 and CK804.

* Which broke nf2/3, which was because nf2/3's hardreset was broken in different way.

So, to sum up, each generation of sata_nv controllers exhibits different hardreset behaviors.

* generic: doesn't work.

* nf2/3: signature D2H Reg FIS delivery doesn't work.

* ck804: works.

So, all three require different treatments to prefer hardreset and any solution which didn't do that was broken. The attached patch should solve all the problems.

It was messy but this at least fixes hardreset for all flavors of nv controllers, so it will generally improve reliability.

Thanks.

Signature · 1 line
tejun
drivers/ata/sata_nv.c +7 -28
index 14601dc..ce77619 100644
@@ -309,8 +309,6 @@ static void nv_nf2_freeze(struct ata_port *ap);
 static void nv_nf2_thaw(struct ata_port *ap);
 static void nv_ck804_freeze(struct ata_port *ap);
 static void nv_ck804_thaw(struct ata_port *ap);
-static int nv_hardreset(struct ata_link *link, unsigned int *class,
-			unsigned long deadline);
 static int nv_adma_slave_config(struct scsi_device *sdev);
 static int nv_adma_check_atapi_dma(struct ata_queued_cmd *qc);
 static void nv_adma_qc_prep(struct ata_queued_cmd *qc);

@@ -405,17 +403,8 @@ static struct scsi_host_template nv_swncq_sht = {
 	.slave_configure	= nv_swncq_slave_config,
 };
 
-/* OSDL bz3352 reports that some nv controllers can't determine device
- * signature reliably and nv_hardreset is implemented to work around
- * the problem.  This was reported on nf3 and it's unclear whether any
- * other controllers are affected.  However, the workaround has been
- * applied to all variants and there isn't much to gain by trying to
- * find out exactly which ones are affected at this point especially
- * because NV has moved over to ahci for newer controllers.
- */
 static struct ata_port_operations nv_common_ops = {
 	.inherits		= &ata_bmdma_port_ops,
-	.hardreset		= nv_hardreset,
 	.scr_read		= nv_scr_read,
 	.scr_write		= nv_scr_write,
 };

@@ -429,12 +418,17 @@ static struct ata_port_operations nv_generic_ops = {
 	.hardreset		= ATA_OP_NULL,
 };
 
+/* OSDL bz3352 reports that nf2/3 controllers can't determine device
+ * signature reliably.  Use sata_std_hardreset().
+ */
 static struct ata_port_operations nv_nf2_ops = {
 	.inherits		= &nv_common_ops,
 	.freeze			= nv_nf2_freeze,
 	.thaw			= nv_nf2_thaw,
+	.hardreset		= sata_std_hardreset,
 };
 
+/* CK804 finally gets hardreset right */
 static struct ata_port_operations nv_ck804_ops = {
 	.inherits		= &nv_common_ops,
 	.freeze			= nv_ck804_freeze,

@@ -443,7 +437,7 @@ static struct ata_port_operations nv_ck804_ops = {
 };
 
 static struct ata_port_operations nv_adma_ops = {
-	.inherits		= &nv_common_ops,
+	.inherits		= &nv_ck804_ops,
 
 	.check_atapi_dma	= nv_adma_check_atapi_dma,
 	.sff_tf_read		= nv_adma_tf_read,

@@ -467,7 +461,7 @@ static struct ata_port_operations nv_adma_ops = {
 };
 
 static struct ata_port_operations nv_swncq_ops = {
-	.inherits		= &nv_common_ops,
+	.inherits		= &nv_generic_ops,
 
 	.qc_defer		= ata_std_qc_defer,
 	.qc_prep		= nv_swncq_qc_prep,

@@ -1605,21 +1599,6 @@ static void nv_mcp55_thaw(struct ata_port *ap)
 	ata_sff_thaw(ap);
 }
 
-static int nv_hardreset(struct ata_link *link, unsigned int *class,
-			unsigned long deadline)
-{
-	int rc;
-
-	/* SATA hardreset fails to retrieve proper device signature on
-	 * some controllers.  Request follow up SRST.  For more info,
-	 * see http://bugzilla.kernel.org/show_bug.cgi?id=3352
-	 */
-	rc = sata_sff_hardreset(link, class, deadline);
-	if (rc)
-		return rc;
-	return -EAGAIN;
-}
-
 static void nv_adma_error_handler(struct ata_port *ap)
 {
 	struct nv_adma_port_priv *pp = ap->private_data;
#1 lore raw
Quoted 37 lines · Tejun Heo

Hello,

Please verify whether the attached patch fixes all hardreset / probing related issues. The patch is on top of v2.6.27-rc8. Sorry about all the mess but it was quite a confusing problem. Here's what went down.

* During 2.6.27-rcX, libata switched to prefer hardreset.

* Which broke, generic flavor of sata_nv. Some sata_nv's fail to bring the link up after hardreset.

* A fix patch went in to disable hardreset for generic flavor. But it accidentaly disabled hardreset for all flavors.

* Which broke nf2/3 and CK804, because their hotplug interrupt bits never get cleared. They're apparently cleared by hardreset sequence.

* Another fix patch went in to reinstate nv_hardreset for nf2/3 and CK804.

* Which broke nf2/3, which was because nf2/3's hardreset was broken in different way.

So, to sum up, each generation of sata_nv controllers exhibits different hardreset behaviors.

* generic: doesn't work.

* nf2/3: signature D2H Reg FIS delivery doesn't work.

* ck804: works.

So, all three require different treatments to prefer hardreset and any solution which didn't do that was broken. The attached patch should solve all the problems.

Quoted 8 lines

It was messy but this at least fixes hardreset for all flavors of nv controllers, so it will generally improve reliability.

Thanks.

--
tejun

OK, patching the 2.6.26 based on your patch doesn't work properly. It works for sata devices but not sata emulated devices (but these do work with the 2.6.27).

author
#2 lore raw
Quoted 3 lines · gettinther

OK, patching the 2.6.26 based on your patch doesn't work properly. It works for sata devices but not sata emulated devices (but these do work with the 2.6.27).

Eh... what do you mean it doesn't work on 2.6.26? The same change doesn't work on 2.6.26 but it does on 2.6.27? How did you apply the change to 2.6.26? Can you post the diff?

Thanks.

Signature · 1 line
tejun
#3 lore raw
Quoted 5 lines · Tejun Heo
Quoted 3 lines · gettinther

OK, patching the 2.6.26 based on your patch doesn't work properly. It works for sata devices but not sata emulated devices (but these do work with the 2.6.27).

Quoted 9 lines

Eh... what do you mean it doesn't work on 2.6.26? The same change doesn't work on 2.6.26 but it does on 2.6.27? How did you apply the change to 2.6.26? Can you post the diff?

Thanks.

--
tejun

Sorry, replied on the wrong thread

Here's the patch I applied

drivers/ata/sata_nv.c renamed from drivers/ata/sata_nv.c.old +1 -18
@@ -309,8 +309,6 @@
 static void nv_nf2_thaw(struct ata_port *ap);
 static void nv_ck804_freeze(struct ata_port *ap);
 static void nv_ck804_thaw(struct ata_port *ap);
-static int nv_hardreset(struct ata_link *link, unsigned int *class,
-			unsigned long deadline);
 static int nv_adma_slave_config(struct scsi_device *sdev);
 static int nv_adma_check_atapi_dma(struct ata_queued_cmd *qc);
 static void nv_adma_qc_prep(struct ata_queued_cmd *qc);

@@ -407,7 +405,6 @@
 
 static struct ata_port_operations nv_generic_ops = {
 	.inherits		= &ata_bmdma_port_ops,
-	.hardreset		= nv_hardreset,
 	.scr_read		= nv_scr_read,
 	.scr_write		= nv_scr_write,
 };

@@ -416,6 +413,7 @@
 	.inherits		= &nv_generic_ops,
 	.freeze			= nv_nf2_freeze,
 	.thaw			= nv_nf2_thaw,
+	.hardreset		= sata_std_hardreset,
 };
 
 static struct ata_port_operations nv_ck804_ops = {

@@ -1588,21 +1586,6 @@
 	ata_sff_thaw(ap);
 }
 
-static int nv_hardreset(struct ata_link *link, unsigned int *class,
-			unsigned long deadline)
-{
-	int rc;
-
-	/* SATA hardreset fails to retrieve proper device signature on
-	 * some controllers.  Request follow up SRST.  For more info,
-	 * see http://bugzilla.kernel.org/show_bug.cgi?id=3352
-	 */
-	rc = sata_sff_hardreset(link, class, deadline);
-	if (rc)
-		return rc;
-	return -EAGAIN;
-}
-
 static void nv_adma_error_handler(struct ata_port *ap)
 {
 	struct nv_adma_port_priv *pp = ap->private_data;
author
#4 lore raw
Quoted 11 lines · gettinther
Quoted 10 lines · Tejun Heo
Quoted 3 lines · gettinther

OK, patching the 2.6.26 based on your patch doesn't work properly. It works for sata devices but not sata emulated devices (but these do work with the 2.6.27).

Eh... what do you mean it doesn't work on 2.6.26? The same change doesn't work on 2.6.26 but it does on 2.6.27? How did you apply the change to 2.6.26? Can you post the diff?

Thanks.

Quoted 7 lines
Quoted 3 lines

--
tejun

Sorry, replied on the wrong thread

Here's the patch I applied

Hmm... that should work for 2.6.26 for MCP55. Can you post the failing log?

Signature · 1 line
tejun
#5 lore raw
Quoted 16 lines · Tejun Heo
Quoted 15 lines · gettinther
Quoted 13 lines · Tejun Heo
Quoted 3 lines · gettinther

OK, patching the 2.6.26 based on your patch doesn't work properly. It works for sata devices but not sata emulated devices (but these do work with the 2.6.27).

Eh... what do you mean it doesn't work on 2.6.26? The same change doesn't work on 2.6.26 but it does on 2.6.27? How did you apply the change to 2.6.26? Can you post the diff?

Thanks.

--
tejun

Quoted 9 lines
Quoted 3 lines

Sorry, replied on the wrong thread

Here's the patch I applied

Hmm... that should work for 2.6.26 for MCP55. Can you post the failing log?

--
tejun

The log simply ignores the drive altogether.

from the sound coming from the drive, it does seem that the drive initially starts at boot, then stops. After that stage the device detection takes place. as the item is a pata to sata converter (emulated sata), it does not support hot plugging.

I also need to mention that this behavior (start-stop during boot) did not happen on the 2.6.24 and I need to check the 2.6.27. However the device is detected by both of these kernels.

Thanking you,

Cyrille

Oct 14 21:18:12 localhost kernel: scsi0 : sata_nv
Oct 14 21:18:12 localhost kernel: scsi1 : sata_nv
Oct 14 21:18:12 localhost kernel: ata1: SATA max UDMA/133 cmd 0xc480
ctl 0xc400 bmdma 0xbc00 irq 23
Oct 14 21:18:12 localhost kernel: ata2: SATA max UDMA/133 cmd 0xc080
ctl 0xc000 bmdma 0xbc08 irq 23
Oct 14 21:18:12 localhost kernel: ata1: SATA link up 3.0 Gbps (SStatus
123 SControl 300)
Oct 14 21:18:12 localhost kernel: ata1.00: ATA-7: ST3120211AS, 3.AAE,
max UDMA/133
Oct 14 21:18:12 localhost kernel: ata1.00: 234441648 sectors, multi
16: LBA48 NCQ (depth 31/32)
Oct 14 21:18:12 localhost kernel: ata1.00: configured for UDMA/133
Oct 14 21:18:12 localhost kernel: input: ImPS/2 Generic Wheel Mouse as
/class/input/input1
Oct 14 21:18:12 localhost kernel: ata2: SATA link up 1.5 Gbps (SStatus
113 SControl 300)
Oct 14 21:18:12 localhost kernel: scsi 0:0:0:0: Direct-Access     ATA
    ST3120211AS      3.AA PQ: 0 ANSI: 5
Oct 14 21:18:12 localhost kernel: ACPI: PCI Interrupt Link [LSA1]
enabled at IRQ 22
Oct 14 21:18:12 localhost kernel: ACPI: PCI Interrupt 0000:00:05.1[B]
-> Link [LSA1] -> GSI 22 (level, low) -> IRQ 22
Oct 14 21:18:12 localhost kernel: sata_nv 0000:00:05.1: Using SWNCQ mode
author
#6 lore raw
Quoted 10 lines · gettinther

The log simply ignores the drive altogether.

from the sound coming from the drive, it does seem that the drive initially starts at boot, then stops. After that stage the device detection takes place. as the item is a pata to sata converter (emulated sata), it does not support hot plugging.

I also need to mention that this behavior (start-stop during boot) did not happen on the 2.6.24 and I need to check the 2.6.27. However the device is detected by both of these kernels.

Hmmm...

Quoted 2 lines
Oct 14 21:18:12 localhost kernel: ata2: SATA link up 1.5 Gbps (SStatus

113 SControl 300)

Hmmm... Can you please apply the attached patch on top of it and report the boot log?

Signature · 1 line
tejun
drivers/ata/libata-core.c +3 -1
index e97afce..1fc4f80 100644
@@ -1160,6 +1160,8 @@ unsigned int ata_dev_classify(const struct ata_taskfile *tf)
 	 * We follow the current spec and consider that 0x69/0x96
 	 * identifies a port multiplier and 0x3c/0xc3 a SEMB device.
 	 */
+	printk("XXX classify %02x/%02x\n", tf->lbam, tf->lbah);
+
 	if ((tf->lbam == 0) && (tf->lbah == 0)) {
 		DPRINTK("found ATA device by sig\n");
 		return ATA_DEV_ATA;

@@ -5855,7 +5857,7 @@ int ata_host_register(struct ata_host *host, struct scsi_host_template *sht)
 
 			ehi->probe_mask |= ATA_ALL_DEVICES;
 			ehi->action |= ATA_EH_RESET | ATA_EH_LPM;
-			ehi->flags |= ATA_EHI_NO_AUTOPSY | ATA_EHI_QUIET;
+			ehi->flags |= ATA_EHI_NO_AUTOPSY/* | ATA_EHI_QUIET*/;
 
 			ap->pflags &= ~ATA_PFLAG_INITIALIZING;
 			ap->pflags |= ATA_PFLAG_LOADING;
#7 lore raw
Quoted 14 lines · Tejun Heo
Quoted 10 lines · gettinther

The log simply ignores the drive altogether.

from the sound coming from the drive, it does seem that the drive initially starts at boot, then stops. After that stage the device detection takes place. as the item is a pata to sata converter (emulated sata), it does not support hot plugging.

I also need to mention that this behavior (start-stop during boot) did not happen on the 2.6.24 and I need to check the 2.6.27. However the device is detected by both of these kernels.

Hmmm...

Quoted 9 lines
Quoted 2 lines
Oct 14 21:18:12 localhost kernel: ata2: SATA link up 1.5 Gbps (SStatus

113 SControl 300)

Hmmm... Can you please apply the attached patch on top of it and report the boot log?

--
tejun

here it is. Let me know if you need other parts of the logs.

Oct 15 20:00:33 localhost kernel: sata_nv 0000:00:05.0: Using SWNCQ mode
Oct 15 20:00:33 localhost kernel: scsi0 : sata_nv
Oct 15 20:00:33 localhost kernel: scsi1 : sata_nv
Oct 15 20:00:33 localhost kernel: ata1: SATA max UDMA/133 cmd 0xc480
ctl 0xc400 bmdma 0xbc00 irq 23
Oct 15 20:00:33 localhost kernel: ata2: SATA max UDMA/133 cmd 0xc080
ctl 0xc000 bmdma 0xbc08 irq 23
Oct 15 20:00:33 localhost kernel: ata1: hard resetting link
Oct 15 20:00:33 localhost kernel: XXX classify 00/00
Oct 15 20:00:33 localhost kernel: ata1: SATA link up 3.0 Gbps (SStatus
123 SControl 300)
Oct 15 20:00:33 localhost kernel: ata1.00: ATA-7: ST3120211AS, 3.AAE,
max UDMA/133
Oct 15 20:00:33 localhost kernel: ata1.00: 234441648 sectors, multi
16: LBA48 NCQ (depth 31/32)
Oct 15 20:00:33 localhost kernel: ata1.00: configured for UDMA/133
Oct 15 20:00:33 localhost kernel: ata1: EH complete
Oct 15 20:00:33 localhost kernel: ata2: hard resetting link
Oct 15 20:00:33 localhost kernel: input: ImPS/2 Generic Wheel Mouse as
/class/input/input1
Oct 15 20:00:33 localhost kernel: XXX classify 12/00
Oct 15 20:00:33 localhost kernel: ata2: SATA link up 1.5 Gbps (SStatus
113 SControl 300)
Oct 15 20:00:33 localhost kernel: ata2: EH complete
Oct 15 20:00:33 localhost kernel: scsi 0:0:0:0: Direct-Access     ATA
    ST3120211AS      3.AA PQ: 0 ANSI: 5
Oct 15 20:00:33 localhost kernel: ACPI: PCI Interrupt Link [LSA1]
enabled at IRQ 22
Oct 15 20:00:33 localhost kernel: ACPI: PCI Interrupt 0000:00:05.1[B]
-> Link [LSA1] -> GSI 22 (level, low) -> IRQ 22
Oct 15 20:00:33 localhost kernel: sata_nv 0000:00:05.1: Using SWNCQ mode
author
#8 lore raw
Quoted 16 lines · gettinther
Quoted 15 lines · Tejun Heo
Quoted 10 lines · gettinther

The log simply ignores the drive altogether.

from the sound coming from the drive, it does seem that the drive initially starts at boot, then stops. After that stage the device detection takes place. as the item is a pata to sata converter (emulated sata), it does not support hot plugging.

I also need to mention that this behavior (start-stop during boot) did not happen on the 2.6.24 and I need to check the 2.6.27. However the device is detected by both of these kernels.

Hmmm...

Quoted 2 lines
Oct 14 21:18:12 localhost kernel: ata2: SATA link up 1.5 Gbps (SStatus

113 SControl 300)

Quoted 8 lines
Quoted 6 lines

Hmmm... Can you please apply the attached patch on top of it and report the boot log?

--
tejun

here it is. Let me know if you need other parts of the logs.

Ah.. okay, you need to change .hardreset to ATA_OP_NULL instead of sata_std_hardreset.

Signature · 1 line
tejun
#9 lore raw
Quoted 24 lines · Tejun Heo
Quoted 23 lines · gettinther
Quoted 21 lines · Tejun Heo
Quoted 10 lines · gettinther

The log simply ignores the drive altogether.

from the sound coming from the drive, it does seem that the drive initially starts at boot, then stops. After that stage the device detection takes place. as the item is a pata to sata converter (emulated sata), it does not support hot plugging.

I also need to mention that this behavior (start-stop during boot) did not happen on the 2.6.24 and I need to check the 2.6.27. However the device is detected by both of these kernels.

Hmmm...

Quoted 2 lines
Oct 14 21:18:12 localhost kernel: ata2: SATA link up 1.5 Gbps (SStatus

113 SControl 300)

Hmmm... Can you please apply the attached patch on top of it and report the boot log?

--
tejun

Quoted 8 lines
Quoted 1 line

here it is. Let me know if you need other parts of the logs.

Ah.. okay, you need to change .hardreset to ATA_OP_NULL instead of sata_std_hardreset.

--
tejun

On both 2.6.26 and 2.6.27 or just the 2.6.26? The initial patch does work on the 2.6.27

Thanks,

Cyrille

#10 lore raw
Quoted 34 lines · gettinther
Quoted 24 lines · Tejun Heo
Quoted 23 lines · gettinther
Quoted 21 lines · Tejun Heo
Quoted 10 lines · gettinther

The log simply ignores the drive altogether.

from the sound coming from the drive, it does seem that the drive initially starts at boot, then stops. After that stage the device detection takes place. as the item is a pata to sata converter (emulated sata), it does not support hot plugging.

I also need to mention that this behavior (start-stop during boot) did not happen on the 2.6.24 and I need to check the 2.6.27. However the device is detected by both of these kernels.

Hmmm...

Quoted 2 lines
Oct 14 21:18:12 localhost kernel: ata2: SATA link up 1.5 Gbps (SStatus

113 SControl 300)

Hmmm... Can you please apply the attached patch on top of it and report the boot log?

--
tejun

Quoted 8 lines
Quoted 1 line

here it is. Let me know if you need other parts of the logs.

Ah.. okay, you need to change .hardreset to ATA_OP_NULL instead of sata_std_hardreset.

--
tejun

Quoted 7 lines

On both 2.6.26 and 2.6.27 or just the 2.6.26? The initial patch does work on the 2.6.27

Thanks,

Cyrille

Tried replacing sata_std_hardreset with ATA_OP_NULL but it didn't work. I'll add back the debug patch and post the results

#11 lore raw
Quoted 38 lines · gettinther
Quoted 37 lines · gettinther
Quoted 24 lines · Tejun Heo
Quoted 23 lines · gettinther
Quoted 21 lines · Tejun Heo
Quoted 10 lines · gettinther

The log simply ignores the drive altogether.

from the sound coming from the drive, it does seem that the drive initially starts at boot, then stops. After that stage the device detection takes place. as the item is a pata to sata converter (emulated sata), it does not support hot plugging.

I also need to mention that this behavior (start-stop during boot) did not happen on the 2.6.24 and I need to check the 2.6.27. However the device is detected by both of these kernels.

Hmmm...

Quoted 2 lines
Oct 14 21:18:12 localhost kernel: ata2: SATA link up 1.5 Gbps (SStatus

113 SControl 300)

Hmmm... Can you please apply the attached patch on top of it and report the boot log?

--
tejun

Quoted 8 lines
Quoted 1 line

here it is. Let me know if you need other parts of the logs.

Ah.. okay, you need to change .hardreset to ATA_OP_NULL instead of sata_std_hardreset.

--
tejun

On both 2.6.26 and 2.6.27 or just the 2.6.26? The initial patch does work on the 2.6.27

Quoted 8 lines
Quoted 4 lines

Thanks,

Cyrille

Tried replacing sata_std_hardreset with ATA_OP_NULL but it didn't work. I'll add back the debug patch and post the results

Sorry for the delay

Here's the message with debug

Oct 21 07:39:35 dranagh1 kernel: sata_nv 0000:00:05.1: Using SWNCQ mode
Oct 21 07:39:35 dranagh1 kernel: scsi2 : sata_nv
Oct 21 07:39:35 dranagh1 kernel: scsi3 : sata_nv
Oct 21 07:39:35 dranagh1 kernel: ata3: SATA max UDMA/133 cmd 0xb880
ctl 0xb800 bmdma 0xb080 irq 22
Oct 21 07:39:35 dranagh1 kernel: ata4: SATA max UDMA/133 cmd 0xb480
ctl 0xb400 bmdma 0xb088 irq 22
Oct 21 07:39:35 dranagh1 kernel: ata3: hard resetting link
Oct 21 07:39:35 dranagh1 kernel: XXX classify 12/00
Oct 21 07:39:35 dranagh1 kernel: ata3: SATA link up 1.5 Gbps (SStatus
113 SControl 300)
Oct 21 07:39:35 dranagh1 kernel: ata3: EH complete
Oct 21 07:39:35 dranagh1 kernel: ata4: hard resetting link
Oct 21 07:39:35 dranagh1 kernel: ata4: SATA link down (SStatus 0 SControl 300)
Oct 21 07:39:35 dranagh1 kernel: ata4: EH complete
Oct 21 07:39:35 dranagh1 kernel: ACPI: PCI Interrupt Link [LSA2]
enabled at IRQ 21
Oct 21 07:39:35 dranagh1 kernel: ACPI: PCI Interrupt 0000:00:05.2[C]
-> Link [LSA2] -> GSI 21 (level, low) -> IRQ 21
Oct 21 07:39:35 dranagh1 kernel: sata_nv 0000:00:05.2: Using SWNCQ mode
Oct 21 07:39:35 dranagh1 kernel: scsi4 : sata_nv