sata_nv: disable hardreset for generic

RFC PATCH In review Tejun Heo 4 participants 9 messages Aug 3, 2008 – Oct 2, 2008 libata

lore

Share & tools

lore

Tejun Heo

author

2008-08-03 05:23 UTC #0 lore raw

RFC PATCH 1/1 sata_nv: disable hardreset for generic

Quoted 1 line

From 2.6.26, libata is defaulting to hardreset for various reasons one

of them being unifying probing, hotplug and EH reset paths uniform. Previously, broken hardreset could go unnoticed as it wasn't used during probing but when something goes wrong or after hotplug the problem will surface and bite hard.

OSDL bug 11195 reports that sata_nv generic flavor falls into this category. Hardreset itself succeeds but PHY stays offline after hardreset. I tried longer debounce timing but the result was the same.

http://bugzilla.kernel.org/show_bug.cgi?id=11195

So, it seems we'll have to drop hardreset from the generic flavor. Peer Chen and Kuan, does anything ring a bell? ie. Certain subset are unaffected, etc...

Thanks.

PLEASE DON'T APPLY JUST YET

---
 drivers/ata/sata_nv.c |   19 +------------------
 1 file changed, 1 insertion(+), 18 deletions(-)

drivers/ata/sata_nv.c +1 ~~-18~~

index 858f706..1e1f3f3 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);

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

@@ -1588,21 +1586,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;

Peer Chen

2008-08-04 09:27 UTC #1 lore raw

Hard reset should work for MCP61 and 2.6.26 work fine on my MCP61 board, I believe the failure is specific case.
Does this board work fine under windows? Switch the HDD or connecting the HDD to other ports are also worth trying.

BRs
Peer Chen

Quoted 4 lines

-----Original Message-----
From: Tejun Heo [mailto:[email protected]]
Sent: Sunday, August 03, 2008 1:22 PM
To: [email protected]; IDE/ATA development list; Jeff Garzik; Peer Chen;

Kuan Luo

Quoted 1 line

Subject: [RFC PATCH #upstream-fixes] sata_nv: disable hardreset for

generic

Quoted 59 lines

From 2.6.26, libata is defaulting to hardreset for various reasons one of them being unifying probing, hotplug and EH reset paths uniform. Previously, broken hardreset could go unnoticed as it wasn't used during probing but when something goes wrong or after hotplug the problem will surface and bite hard.
OSDL bug 11195 reports that sata_nv generic flavor falls into this category. Hardreset itself succeeds but PHY stays offline after hardreset. I tried longer debounce timing but the result was the same.
http://bugzilla.kernel.org/show_bug.cgi?id=11195
So, it seems we'll have to drop hardreset from the generic flavor. Peer Chen and Kuan, does anything ring a bell? ie. Certain subset are unaffected, etc...
Thanks.
PLEASE DON'T APPLY JUST YET
---
 drivers/ata/sata_nv.c |   19 +------------------
 1 file changed, 1 insertion(+), 18 deletions(-)
drivers/ata/sata_nv.c +1 ~~-11~~
index 858f706..1e1f3f3 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);
@@ -407,7 +405,7 @@ static struct scsi_host_template nv_swncq_sht = {
 
 static struct ata_port_operations nv_generic_ops = {
 	.inherits		= &ata_bmdma_port_ops,
-	.hardreset		= nv_hardreset,
+	.hardreset		= ATA_OP_NULL,
 	.scr_read		= nv_scr_read,
 	.scr_write		= nv_scr_write,
 };
@@ -1588,21 +1586,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

drivers/ata/sata_nv.c static void nv_mcp55_thaw(struct ata_port *ap) · line 1591

Quoted 10 lines

drivers/ata/sata_nv.c +0 -7

-	 */
-	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;

-----------------------------------------------------------------------------------
This email message is for the sole use of the intended recipient(s) and may contain confidential information. Any unauthorized review, use, disclosure or distribution is prohibited. If you are not the intended recipient, please contact the sender by reply email and destroy all copies of the original message.
-----------------------------------------------------------------------------------

Tejun Heo

author

2008-08-13 06:54 UTC #2 lore raw

Quoted 4 lines · Peer Chen

Hard reset should work for MCP61 and 2.6.26 work fine on my MCP61 board, I believe the failure is specific case.
Does this board work fine under windows? Switch the HDD or connecting the HDD to other ports are also worth trying.

Sorry about the delay. Can you please try to test one? This one is rather serious regression as devices fail detection and it's far better to disable hardreset on all MCP61s if we can't determine which specific ones are problematic.

Thank you.

Signature · 1 line

tejun

Tejun Heo

author

2008-08-29 14:16 UTC #3 lore raw

PATCH 1/1 sata_nv: disable hardreset for generic

In reply to Tejun Heo

Quoted 1 line

From 2.6.26, libata is defaulting to hardreset for various reasons one

http://bugzilla.kernel.org/show_bug.cgi?id=11195

So, it seems we'll have to drop hardreset from the generic flavor.

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

Cc Peer Chen <[email protected]>

---
There have been more than three reports of this problem and there just isn't good reason to defer this workaround anymore. Once this gets into #upstream-fixes, I think this should go to -stable too.

Thanks.

 drivers/ata/sata_nv.c |   19 +------------------
 1 file changed, 1 insertion(+), 18 deletions(-)

drivers/ata/sata_nv.c +1 ~~-18~~

index 858f706..1e1f3f3 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);

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

@@ -1588,21 +1586,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;

Jeff Garzik

Applied

2008-09-08 16:16 UTC #4 lore raw

Quoted 17 lines · Tejun Heo

Quoted 1 line
From 2.6.26, libata is defaulting to hardreset for various reasons one
of them being unifying probing, hotplug and EH reset paths uniform. Previously, broken hardreset could go unnoticed as it wasn't used during probing but when something goes wrong or after hotplug the problem will surface and bite hard.
OSDL bug 11195 reports that sata_nv generic flavor falls into this category. Hardreset itself succeeds but PHY stays offline after hardreset. I tried longer debounce timing but the result was the same.
http://bugzilla.kernel.org/show_bug.cgi?id=11195
So, it seems we'll have to drop hardreset from the generic flavor.
Signed-off-by Tejun Heo <[email protected]>
Cc Peer Chen <[email protected]>

Quoted 8 lines

---
There have been more than three reports of this problem and there just isn't good reason to defer this workaround anymore. Once this gets into #upstream-fixes, I think this should go to -stable too.
Thanks.
 drivers/ata/sata_nv.c |   19 +------------------

applied

Tejun Heo

author

2008-09-08 19:19 UTC #5 lore raw

In reply to Tejun Heo

Quoted 11 lines · Tejun Heo

From 2.6.26, libata is defaulting to hardreset for various reasons one of them being unifying probing, hotplug and EH reset paths uniform. Previously, broken hardreset could go unnoticed as it wasn't used during probing but when something goes wrong or after hotplug the problem will surface and bite hard.
OSDL bug 11195 reports that sata_nv generic flavor falls into this category. Hardreset itself succeeds but PHY stays offline after hardreset. I tried longer debounce timing but the result was the same.

Quoted 6 lines

http://bugzilla.kernel.org/show_bug.cgi?id=11195
So, it seems we'll have to drop hardreset from the generic flavor.
Signed-off-by Tejun Heo <[email protected]>
Cc Peer Chen <[email protected]>

This one definitely should go into -stable too. The original patch is...

http://article.gmane.org/gmane.linux.ide/34077/raw

Thanks.

Signature · 1 line

tejun

Greg KH

2008-10-01 23:43 UTC #6 lore raw

Quoted 14 lines · Tejun Heo

Quoted 13 lines · Tejun Heo
From 2.6.26, libata is defaulting to hardreset for various reasons one of them being unifying probing, hotplug and EH reset paths uniform. Previously, broken hardreset could go unnoticed as it wasn't used during probing but when something goes wrong or after hotplug the problem will surface and bite hard.
OSDL bug 11195 reports that sata_nv generic flavor falls into this category. Hardreset itself succeeds but PHY stays offline after hardreset. I tried longer debounce timing but the result was the same.
http://bugzilla.kernel.org/show_bug.cgi?id=11195

Quoted 8 lines

Quoted 4 lines
So, it seems we'll have to drop hardreset from the generic flavor.
Signed-off-by Tejun Heo <[email protected]>
Cc Peer Chen <[email protected]>
This one definitely should go into -stable too. The original patch is...
http://article.gmane.org/gmane.linux.ide/34077/raw

Now applied, thanks.

greg k-h

Tejun Heo

author

2008-10-02 00:26 UTC #7 lore raw

Quoted 17 lines · Greg KH

Quoted 16 lines · Tejun Heo
Quoted 15 lines · Tejun Heo
From 2.6.26, libata is defaulting to hardreset for various reasons one of them being unifying probing, hotplug and EH reset paths uniform. Previously, broken hardreset could go unnoticed as it wasn't used during probing but when something goes wrong or after hotplug the problem will surface and bite hard.
OSDL bug 11195 reports that sata_nv generic flavor falls into this category. Hardreset itself succeeds but PHY stays offline after hardreset. I tried longer debounce timing but the result was the same.
http://bugzilla.kernel.org/show_bug.cgi?id=11195
So, it seems we'll have to drop hardreset from the generic flavor.

Quoted 7 lines

Quoted 5 lines
Quoted 2 lines
Signed-off-by Tejun Heo <[email protected]>
Cc Peer Chen <[email protected]>
This one definitely should go into -stable too. The original patch is...
http://article.gmane.org/gmane.linux.ide/34077/raw
Now applied, thanks.

Greg, can you please hold a bit before releasing this? This patch accidentally disables hardreset for all flavors of nv controllers which causes problems for ck804. A later patch to only apply it to generic went in but it then broke nf2 flavor, so it seems generic and nf2 require this workaround but ck804 doesn't like the workaround. I've asked a reporter to test to apply the workaround only to generic and nf2 and will forward the third patch as soon as it's confirmed. So, please wait till the third one is ready.

Thanks.

Signature · 1 line

tejun

Greg KH

2008-10-02 20:03 UTC #8 lore raw

Quoted 26 lines · Tejun Heo

Quoted 17 lines · Greg KH
Quoted 16 lines · Tejun Heo
Quoted 15 lines · Tejun Heo
From 2.6.26, libata is defaulting to hardreset for various reasons one of them being unifying probing, hotplug and EH reset paths uniform. Previously, broken hardreset could go unnoticed as it wasn't used during probing but when something goes wrong or after hotplug the problem will surface and bite hard.
OSDL bug 11195 reports that sata_nv generic flavor falls into this category. Hardreset itself succeeds but PHY stays offline after hardreset. I tried longer debounce timing but the result was the same.
http://bugzilla.kernel.org/show_bug.cgi?id=11195
So, it seems we'll have to drop hardreset from the generic flavor.
Quoted 7 lines
Quoted 5 lines
Quoted 2 lines
Signed-off-by Tejun Heo <[email protected]>
Cc Peer Chen <[email protected]>
This one definitely should go into -stable too. The original patch is...
http://article.gmane.org/gmane.linux.ide/34077/raw
Now applied, thanks.

Quoted 8 lines

Greg, can you please hold a bit before releasing this? This patch accidentally disables hardreset for all flavors of nv controllers which causes problems for ck804. A later patch to only apply it to generic went in but it then broke nf2 flavor, so it seems generic and nf2 require this workaround but ck804 doesn't like the workaround. I've asked a reporter to test to apply the workaround only to generic and nf2 and will forward the third patch as soon as it's confirmed. So, please wait till the third one is ready.

Ok, I've dropped:
sata_nv-disable-hardreset-for-generic.patch
from the 2.6.26-stable queue for now.

If you get it all working, please resend all of the patches that you wish me to pick up for a -stable release (well send them to [email protected], not me directly.)

thanks,

greg k-h