[PATCH #upstream] sata_nv: use hardreset only for post-boot probing

[PATCH #upstream] sata_nv: use hardreset only for post-boot probing

[Tejun Heo]

When I thought it was finally defeated, it came back with vengeance.
The failure cases are ever more convoluted.  Now there is a single
combination which fails boot probing - MCP5x + Intel SSD and there are
two hotplug failure reports on different flavors where softreset fails
to bring up the device.

Through the many bug reports after the switch to hardreset, the
following patterns emerged.

- Softreset during boot always works.

- Hardreset during boot sometimes fails to bring up the link on
  certain comibnations and device signature acquisition is unreliable.

- Hardreset is often necessary after hotplug.

It looks like the old behavior of preferring softreset was somehow
pretty close to the working reset protocol although it could have lost
a device during phy error handling by issuing hardreset.

This patch implements nv_hardreset() which kicks in only for post-boot
(!LOADING) device probing resets.  This should be able to work around
all known problem cases.  This isn't perfect but given the various
hardreset quirks on these controllers, I think this is as good as it
can get.

Tested on mcp5x (swncq), nf3 and ck804 for all both boot, warm and
hot probing cases.

Kudos to all the bug reporters and their painful hours with these damn
controllers.  ;-)

Signed-off-by: Tejun Heo <tj@kernel.org>
Cc: Robert Hancock <hancockr@shaw.ca>
Reported-by: David Lang <david@lang.hm>
Reported-by: Samo Vodopivec <lament.email.si@gmail.com>
---
Jeff, please queue for 2.6.31.  I think this should be pretty safe but
well I've said that before several times for sata_nv already and was
wrong, so...

Thanks.

 drivers/ata/sata_nv.c |  131 ++++++++++++++++++++++++++++++--------------------
 1 file changed, 81 insertions(+), 50 deletions(-)

diff --git a/drivers/ata/sata_nv.c b/drivers/ata/sata_nv.c
index 6cda12b..b2d11f3 100644
--- a/drivers/ata/sata_nv.c
+++ b/drivers/ata/sata_nv.c
@@ -305,8 +305,8 @@ static irqreturn_t nv_ck804_interrupt(int irq, void *dev_instance);
 static int nv_scr_read(struct ata_link *link, unsigned int sc_reg, u32 *val);
 static int nv_scr_write(struct ata_link *link, unsigned int sc_reg, u32 val);
 
-static int nv_noclassify_hardreset(struct ata_link *link, unsigned int *class,
-				   unsigned long deadline);
+static int nv_hardreset(struct ata_link *link, unsigned int *class,
+			unsigned long deadline);
 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);
@@ -406,49 +406,82 @@ static struct scsi_host_template nv_swncq_sht = {
 	.slave_configure	= nv_swncq_slave_config,
 };
 
-static struct ata_port_operations nv_common_ops = {
+/*
+ * NV SATA controllers have various different problems with hardreset
+ * protocol depending on the specific controller and device.
+ *
+ * GENERIC:
+ *
+ *  bko11195 reports that link doesn't come online after hardreset on
+ *  generic nv's and there have been several other similar reports on
+ *  linux-ide.
+ *
+ *  bko12351#c23 reports that warmplug on MCP61 doesn't work with
+ *  softreset.
+ *
+ * NF2/3:
+ *
+ *  bko3352 reports nf2/3 controllers can't determine device signature
+ *  reliably after hardreset.  The following thread reports detection
+ *  failure on cold boot with the standard debouncing timing.
+ *
+ *  http://thread.gmane.org/gmane.linux.ide/34098
+ *
+ *  bko12176 reports that hardreset fails to bring up the link during
+ *  boot on nf2.
+ *
+ * CK804:
+ *
+ *  For initial probing after boot and hot plugging, hardreset mostly
+ *  works fine on CK804 but curiously, reprobing on the initial port
+ *  by rescanning or rmmod/insmod fails to acquire the initial D2H Reg
+ *  FIS in somewhat undeterministic way.
+ *
+ * SWNCQ:
+ *
+ *  bko12351 reports that when SWNCQ is enabled, for hotplug to work,
+ *  hardreset should be used and hardreset can't report proper
+ *  signature, which suggests that mcp5x is closer to nf2 as long as
+ *  reset quirkiness is concerned.
+ *
+ *  bko12703 reports that boot probing fails for intel SSD with
+ *  hardreset.  Link fails to come online.  Softreset works fine.
+ *
+ * The failures are varied but the following patterns seem true for
+ * all flavors.
+ *
+ * - Softreset during boot always works.
+ *
+ * - Hardreset during boot sometimes fails to bring up the link on
+ *   certain comibnations and device signature acquisition is
+ *   unreliable.
+ *
+ * - Hardreset is often necessary after hotplug.
+ *
+ * So, preferring softreset for boot probing and error handling (as
+ * hardreset might bring down the link) but using hardreset for
+ * post-boot probing should work around the above issues in most
+ * cases.  Define nv_hardreset() which only kicks in for post-boot
+ * probing and use it for all variants.
+ */
+static struct ata_port_operations nv_generic_ops = {
 	.inherits		= &ata_bmdma_port_ops,
 	.lost_interrupt		= ATA_OP_NULL,
 	.scr_read		= nv_scr_read,
 	.scr_write		= nv_scr_write,
+	.hardreset		= nv_hardreset,
 };
 
-/* OSDL bz11195 reports that link doesn't come online after hardreset
- * on generic nv's and there have been several other similar reports
- * on linux-ide.  Disable hardreset for generic nv's.
- */
-static struct ata_port_operations nv_generic_ops = {
-	.inherits		= &nv_common_ops,
-	.hardreset		= ATA_OP_NULL,
-};
-
-/* nf2 is ripe with hardreset related problems.
- *
- * kernel bz#3352 reports nf2/3 controllers can't determine device
- * signature reliably.  The following thread reports detection failure
- * on cold boot with the standard debouncing timing.
- *
- * http://thread.gmane.org/gmane.linux.ide/34098
- *
- * And bz#12176 reports that hardreset simply doesn't work on nf2.
- * Give up on it and just don't do hardreset.
- */
 static struct ata_port_operations nv_nf2_ops = {
 	.inherits		= &nv_generic_ops,
 	.freeze			= nv_nf2_freeze,
 	.thaw			= nv_nf2_thaw,
 };
 
-/* For initial probing after boot and hot plugging, hardreset mostly
- * works fine on CK804 but curiously, reprobing on the initial port by
- * rescanning or rmmod/insmod fails to acquire the initial D2H Reg FIS
- * in somewhat undeterministic way.  Use noclassify hardreset.
- */
 static struct ata_port_operations nv_ck804_ops = {
-	.inherits		= &nv_common_ops,
+	.inherits		= &nv_generic_ops,
 	.freeze			= nv_ck804_freeze,
 	.thaw			= nv_ck804_thaw,
-	.hardreset		= nv_noclassify_hardreset,
 	.host_stop		= nv_ck804_host_stop,
 };
 
@@ -476,19 +509,8 @@ static struct ata_port_operations nv_adma_ops = {
 	.host_stop		= nv_adma_host_stop,
 };
 
-/* Kernel bz#12351 reports that when SWNCQ is enabled, for hotplug to
- * work, hardreset should be used and hardreset can't report proper
- * signature, which suggests that mcp5x is closer to nf2 as long as
- * reset quirkiness is concerned.  Define separate ops for mcp5x with
- * nv_noclassify_hardreset().
- */
-static struct ata_port_operations nv_mcp5x_ops = {
-	.inherits		= &nv_common_ops,
-	.hardreset		= nv_noclassify_hardreset,
-};
-
 static struct ata_port_operations nv_swncq_ops = {
-	.inherits		= &nv_mcp5x_ops,
+	.inherits		= &nv_generic_ops,
 
 	.qc_defer		= ata_std_qc_defer,
 	.qc_prep		= nv_swncq_qc_prep,
@@ -557,7 +579,7 @@ static const struct ata_port_info nv_port_info[] = {
 		.pio_mask	= NV_PIO_MASK,
 		.mwdma_mask	= NV_MWDMA_MASK,
 		.udma_mask	= NV_UDMA_MASK,
-		.port_ops	= &nv_mcp5x_ops,
+		.port_ops	= &nv_generic_ops,
 		.private_data	= NV_PI_PRIV(nv_generic_interrupt, &nv_sht),
 	},
 	/* SWNCQ */
@@ -1559,15 +1581,24 @@ static int nv_scr_write(struct ata_link *link, unsigned int sc_reg, u32 val)
 	return 0;
 }
 
-static int nv_noclassify_hardreset(struct ata_link *link, unsigned int *class,
-				   unsigned long deadline)
+static int nv_hardreset(struct ata_link *link, unsigned int *class,
+			unsigned long deadline)
 {
-	bool online;
-	int rc;
+	struct ata_eh_context *ehc = &link->eh_context;
 
-	rc = sata_link_hardreset(link, sata_deb_timing_hotplug, deadline,
-				 &online, NULL);
-	return online ? -EAGAIN : rc;
+	/* Do hardreset iff it's post-boot probing, please read the
+	 * comment above port ops for details.
+	 */
+	if (!(link->ap->pflags & ATA_PFLAG_LOADING) &&
+	    !ata_dev_enabled(link->device))
+		sata_link_hardreset(link, sata_deb_timing_hotplug, deadline,
+				    NULL, NULL);
+	else if (!(ehc->i.flags & ATA_EHI_QUIET))
+		ata_link_printk(link, KERN_INFO,
+				"nv: skipping hardreset on occupied port\n");
+
+	/* device signature acquisition is unreliable */
+	return -EAGAIN;
 }
 
 static void nv_nf2_freeze(struct ata_port *ap)

Re: [PATCH #upstream] sata_nv: use hardreset only for post-boot probing

[Jeff Garzik]

Tejun Heo wrote:
> When I thought it was finally defeated, it came back with vengeance.
> The failure cases are ever more convoluted.  Now there is a single
> combination which fails boot probing - MCP5x + Intel SSD and there are
> two hotplug failure reports on different flavors where softreset fails
> to bring up the device.
> 
> Through the many bug reports after the switch to hardreset, the
> following patterns emerged.
> 
> - Softreset during boot always works.
> 
> - Hardreset during boot sometimes fails to bring up the link on
>   certain comibnations and device signature acquisition is unreliable.
> 
> - Hardreset is often necessary after hotplug.
> 
> It looks like the old behavior of preferring softreset was somehow
> pretty close to the working reset protocol although it could have lost
> a device during phy error handling by issuing hardreset.
> 
> This patch implements nv_hardreset() which kicks in only for post-boot
> (!LOADING) device probing resets.  This should be able to work around
> all known problem cases.  This isn't perfect but given the various
> hardreset quirks on these controllers, I think this is as good as it
> can get.
> 
> Tested on mcp5x (swncq), nf3 and ck804 for all both boot, warm and
> hot probing cases.
> 
> Kudos to all the bug reporters and their painful hours with these damn
> controllers.  ;-)
> 
> Signed-off-by: Tejun Heo <tj@kernel.org>
> Cc: Robert Hancock <hancockr@shaw.ca>
> Reported-by: David Lang <david@lang.hm>
> Reported-by: Samo Vodopivec <lament.email.si@gmail.com>
> ---
> Jeff, please queue for 2.6.31.  I think this should be pretty safe but
> well I've said that before several times for sata_nv already and was
> wrong, so...
> 
> Thanks.
> 
>  drivers/ata/sata_nv.c |  131 ++++++++++++++++++++++++++++++--------------------
>  1 file changed, 81 insertions(+), 50 deletions(-)

applied

Re: [PATCH #upstream] sata_nv: use hardreset only for post-boot probing

[david]

this does not apply to 2.6.30, is there a version of this patch that would 
apply to 2.6.30?

also, what testing would need to be done on this before it is a candidate 
for -stable?

David Lang

On Wed, 10 Jun 2009, Tejun Heo wrote:

> Date: Wed, 10 Jun 2009 16:29:07 +0900
> From: Tejun Heo <tj@kernel.org>
> To: Jeff Garzik <jeff@garzik.org>, Robert Hancock <hancockr@shaw.ca>,
>     david@lang.hm, lament.email.si@gmail.com,
>     IDE/ATA development list <linux-ide@vger.kernel.org>
> Subject: [PATCH #upstream] sata_nv: use hardreset only for post-boot probing
> 
> When I thought it was finally defeated, it came back with vengeance.
> The failure cases are ever more convoluted.  Now there is a single
> combination which fails boot probing - MCP5x + Intel SSD and there are
> two hotplug failure reports on different flavors where softreset fails
> to bring up the device.
>
> Through the many bug reports after the switch to hardreset, the
> following patterns emerged.
>
> - Softreset during boot always works.
>
> - Hardreset during boot sometimes fails to bring up the link on
>  certain comibnations and device signature acquisition is unreliable.
>
> - Hardreset is often necessary after hotplug.
>
> It looks like the old behavior of preferring softreset was somehow
> pretty close to the working reset protocol although it could have lost
> a device during phy error handling by issuing hardreset.
>
> This patch implements nv_hardreset() which kicks in only for post-boot
> (!LOADING) device probing resets.  This should be able to work around
> all known problem cases.  This isn't perfect but given the various
> hardreset quirks on these controllers, I think this is as good as it
> can get.
>
> Tested on mcp5x (swncq), nf3 and ck804 for all both boot, warm and
> hot probing cases.
>
> Kudos to all the bug reporters and their painful hours with these damn
> controllers.  ;-)
>
> Signed-off-by: Tejun Heo <tj@kernel.org>
> Cc: Robert Hancock <hancockr@shaw.ca>
> Reported-by: David Lang <david@lang.hm>
> Reported-by: Samo Vodopivec <lament.email.si@gmail.com>
> ---
> Jeff, please queue for 2.6.31.  I think this should be pretty safe but
> well I've said that before several times for sata_nv already and was
> wrong, so...
>
> Thanks.
>
> drivers/ata/sata_nv.c |  131 ++++++++++++++++++++++++++++++--------------------
> 1 file changed, 81 insertions(+), 50 deletions(-)
>
> diff --git a/drivers/ata/sata_nv.c b/drivers/ata/sata_nv.c
> index 6cda12b..b2d11f3 100644
> --- a/drivers/ata/sata_nv.c
> +++ b/drivers/ata/sata_nv.c
> @@ -305,8 +305,8 @@ static irqreturn_t nv_ck804_interrupt(int irq, void *dev_instance);
> static int nv_scr_read(struct ata_link *link, unsigned int sc_reg, u32 *val);
> static int nv_scr_write(struct ata_link *link, unsigned int sc_reg, u32 val);
>
> -static int nv_noclassify_hardreset(struct ata_link *link, unsigned int *class,
> -				   unsigned long deadline);
> +static int nv_hardreset(struct ata_link *link, unsigned int *class,
> +			unsigned long deadline);
> 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);
> @@ -406,49 +406,82 @@ static struct scsi_host_template nv_swncq_sht = {
> 	.slave_configure	= nv_swncq_slave_config,
> };
>
> -static struct ata_port_operations nv_common_ops = {
> +/*
> + * NV SATA controllers have various different problems with hardreset
> + * protocol depending on the specific controller and device.
> + *
> + * GENERIC:
> + *
> + *  bko11195 reports that link doesn't come online after hardreset on
> + *  generic nv's and there have been several other similar reports on
> + *  linux-ide.
> + *
> + *  bko12351#c23 reports that warmplug on MCP61 doesn't work with
> + *  softreset.
> + *
> + * NF2/3:
> + *
> + *  bko3352 reports nf2/3 controllers can't determine device signature
> + *  reliably after hardreset.  The following thread reports detection
> + *  failure on cold boot with the standard debouncing timing.
> + *
> + *  http://thread.gmane.org/gmane.linux.ide/34098
> + *
> + *  bko12176 reports that hardreset fails to bring up the link during
> + *  boot on nf2.
> + *
> + * CK804:
> + *
> + *  For initial probing after boot and hot plugging, hardreset mostly
> + *  works fine on CK804 but curiously, reprobing on the initial port
> + *  by rescanning or rmmod/insmod fails to acquire the initial D2H Reg
> + *  FIS in somewhat undeterministic way.
> + *
> + * SWNCQ:
> + *
> + *  bko12351 reports that when SWNCQ is enabled, for hotplug to work,
> + *  hardreset should be used and hardreset can't report proper
> + *  signature, which suggests that mcp5x is closer to nf2 as long as
> + *  reset quirkiness is concerned.
> + *
> + *  bko12703 reports that boot probing fails for intel SSD with
> + *  hardreset.  Link fails to come online.  Softreset works fine.
> + *
> + * The failures are varied but the following patterns seem true for
> + * all flavors.
> + *
> + * - Softreset during boot always works.
> + *
> + * - Hardreset during boot sometimes fails to bring up the link on
> + *   certain comibnations and device signature acquisition is
> + *   unreliable.
> + *
> + * - Hardreset is often necessary after hotplug.
> + *
> + * So, preferring softreset for boot probing and error handling (as
> + * hardreset might bring down the link) but using hardreset for
> + * post-boot probing should work around the above issues in most
> + * cases.  Define nv_hardreset() which only kicks in for post-boot
> + * probing and use it for all variants.
> + */
> +static struct ata_port_operations nv_generic_ops = {
> 	.inherits		= &ata_bmdma_port_ops,
> 	.lost_interrupt		= ATA_OP_NULL,
> 	.scr_read		= nv_scr_read,
> 	.scr_write		= nv_scr_write,
> +	.hardreset		= nv_hardreset,
> };
>
> -/* OSDL bz11195 reports that link doesn't come online after hardreset
> - * on generic nv's and there have been several other similar reports
> - * on linux-ide.  Disable hardreset for generic nv's.
> - */
> -static struct ata_port_operations nv_generic_ops = {
> -	.inherits		= &nv_common_ops,
> -	.hardreset		= ATA_OP_NULL,
> -};
> -
> -/* nf2 is ripe with hardreset related problems.
> - *
> - * kernel bz#3352 reports nf2/3 controllers can't determine device
> - * signature reliably.  The following thread reports detection failure
> - * on cold boot with the standard debouncing timing.
> - *
> - * http://thread.gmane.org/gmane.linux.ide/34098
> - *
> - * And bz#12176 reports that hardreset simply doesn't work on nf2.
> - * Give up on it and just don't do hardreset.
> - */
> static struct ata_port_operations nv_nf2_ops = {
> 	.inherits		= &nv_generic_ops,
> 	.freeze			= nv_nf2_freeze,
> 	.thaw			= nv_nf2_thaw,
> };
>
> -/* For initial probing after boot and hot plugging, hardreset mostly
> - * works fine on CK804 but curiously, reprobing on the initial port by
> - * rescanning or rmmod/insmod fails to acquire the initial D2H Reg FIS
> - * in somewhat undeterministic way.  Use noclassify hardreset.
> - */
> static struct ata_port_operations nv_ck804_ops = {
> -	.inherits		= &nv_common_ops,
> +	.inherits		= &nv_generic_ops,
> 	.freeze			= nv_ck804_freeze,
> 	.thaw			= nv_ck804_thaw,
> -	.hardreset		= nv_noclassify_hardreset,
> 	.host_stop		= nv_ck804_host_stop,
> };
>
> @@ -476,19 +509,8 @@ static struct ata_port_operations nv_adma_ops = {
> 	.host_stop		= nv_adma_host_stop,
> };
>
> -/* Kernel bz#12351 reports that when SWNCQ is enabled, for hotplug to
> - * work, hardreset should be used and hardreset can't report proper
> - * signature, which suggests that mcp5x is closer to nf2 as long as
> - * reset quirkiness is concerned.  Define separate ops for mcp5x with
> - * nv_noclassify_hardreset().
> - */
> -static struct ata_port_operations nv_mcp5x_ops = {
> -	.inherits		= &nv_common_ops,
> -	.hardreset		= nv_noclassify_hardreset,
> -};
> -
> static struct ata_port_operations nv_swncq_ops = {
> -	.inherits		= &nv_mcp5x_ops,
> +	.inherits		= &nv_generic_ops,
>
> 	.qc_defer		= ata_std_qc_defer,
> 	.qc_prep		= nv_swncq_qc_prep,
> @@ -557,7 +579,7 @@ static const struct ata_port_info nv_port_info[] = {
> 		.pio_mask	= NV_PIO_MASK,
> 		.mwdma_mask	= NV_MWDMA_MASK,
> 		.udma_mask	= NV_UDMA_MASK,
> -		.port_ops	= &nv_mcp5x_ops,
> +		.port_ops	= &nv_generic_ops,
> 		.private_data	= NV_PI_PRIV(nv_generic_interrupt, &nv_sht),
> 	},
> 	/* SWNCQ */
> @@ -1559,15 +1581,24 @@ static int nv_scr_write(struct ata_link *link, unsigned int sc_reg, u32 val)
> 	return 0;
> }
>
> -static int nv_noclassify_hardreset(struct ata_link *link, unsigned int *class,
> -				   unsigned long deadline)
> +static int nv_hardreset(struct ata_link *link, unsigned int *class,
> +			unsigned long deadline)
> {
> -	bool online;
> -	int rc;
> +	struct ata_eh_context *ehc = &link->eh_context;
>
> -	rc = sata_link_hardreset(link, sata_deb_timing_hotplug, deadline,
> -				 &online, NULL);
> -	return online ? -EAGAIN : rc;
> +	/* Do hardreset iff it's post-boot probing, please read the
> +	 * comment above port ops for details.
> +	 */
> +	if (!(link->ap->pflags & ATA_PFLAG_LOADING) &&
> +	    !ata_dev_enabled(link->device))
> +		sata_link_hardreset(link, sata_deb_timing_hotplug, deadline,
> +				    NULL, NULL);
> +	else if (!(ehc->i.flags & ATA_EHI_QUIET))
> +		ata_link_printk(link, KERN_INFO,
> +				"nv: skipping hardreset on occupied port\n");
> +
> +	/* device signature acquisition is unreliable */
> +	return -EAGAIN;
> }
>
> static void nv_nf2_freeze(struct ata_port *ap)
>

Re: [PATCH #upstream] sata_nv: use hardreset only for post-boot probing

[Tejun Heo]

david@lang.hm wrote:
> this does not apply to 2.6.30, is there a version of this patch that
> would apply to 2.6.30?

There isn't one.

> also, what testing would need to be done on this before it is a
> candidate for -stable?

sata_nv saw many disruptions with recent reset protocol update and a
lot of changes which looked perfectly okay caused interesting and
twisted regressions.  I'm not all that confident about pushing this to
-stable.  It's not like a lot of systems are broken on current 2.6.30
after all.

Thanks.

-- 
tejun