[PATCHSET] libata: add @disable_on_err to ata_set_mode(), take#2

[PATCHSET] libata: add @disable_on_err to ata_set_mode(), take#2

[Tejun Heo]

Hello, all.

This is the second take of add-disable_on_err-to-ata_set_mode
patchset.  Changes from the last take[1] are...

* a new patch check-disable-after-internal-cmd is added.  This makes
  ata_exec_internal() to check whether LLDD disabled the port after an
  internal command and handle the situation.  This change allows upper
  layers to operate under the assumption that lower layer does not
  diddle with port/device enable status.  Note that this is a
  temporary measure, to be removed once all LLDDs are updated to the
  new API.

This patchset is against
current upstream[2]
+ implement-per-dev-xfer-masks patchset, take#2 [3]

Thanks.

--
tejun

[1] http://article.gmane.org/gmane.linux.ide/8754
[2] 2cc432eed0491df66e14b578139bba2c75fb3f9a
[3] http://article.gmane.org/gmane.linux.ide/9001

[PATCH 1/5] libata: check if port is disabled after internal command

[Tejun Heo]

libata core is being changed to disallow port/device disable on lower
layers.  However, some LLDDs (sata_mv) directly disable port on
command failure.  This patch makes ata_exec_internal() check whether a
port got disabled after an internal command.  If it is, AC_ERR_SYSTEM
is added to err_mask and the port gets re-enabled.

As internal command failure results in device disable for drivers
which don't implement newer reset/EH callbacks, this change results in
no behavior change for single device per port controllers.  For
slave-possible LLDDs which disable port on command failure, (1) such
drivers don't exist currently, (2) issuing command to the other device
of once-disabled port shouldn't result in catastrophe even if such
driver exists.  So, this should be enough as a temporary measure.

Signed-off-by: Tejun Heo <htejun@gmail.com>

---

 drivers/scsi/libata-core.c |   16 ++++++++++++++++
 1 files changed, 16 insertions(+), 0 deletions(-)

e236cbb94f12a51962fc38db3e1724b17f738c09
diff --git a/drivers/scsi/libata-core.c b/drivers/scsi/libata-core.c
index 1063928..9cd9053 100644
--- a/drivers/scsi/libata-core.c
+++ b/drivers/scsi/libata-core.c
@@ -1009,6 +1009,22 @@ ata_exec_internal(struct ata_port *ap, s
 
 	ata_qc_free(qc);
 
+	/* XXX - Some LLDDs (sata_mv) disable port on command failure.
+	 * Until those drivers are fixed, we detect the condition
+	 * here, fail the command with AC_ERR_SYSTEM and reenable the
+	 * port.
+	 *
+	 * Note that this doesn't change any behavior as internal
+	 * command failure results in disabling the device in the
+	 * higher layer for LLDDs without new reset/EH callbacks.
+	 *
+	 * Kill the following code as soon as those drivers are fixed.
+	 */
+	if (ap->flags & ATA_FLAG_PORT_DISABLED) {
+		err_mask |= AC_ERR_SYSTEM;
+		ata_port_probe(ap);
+	}
+
 	return err_mask;
 }
 
-- 
1.2.4

Re: [PATCH 1/5] libata: check if port is disabled after internal command

[Jeff Garzik]

Tejun Heo wrote:
> libata core is being changed to disallow port/device disable on lower
> layers.  However, some LLDDs (sata_mv) directly disable port on
> command failure.  This patch makes ata_exec_internal() check whether a
> port got disabled after an internal command.  If it is, AC_ERR_SYSTEM
> is added to err_mask and the port gets re-enabled.
> 
> As internal command failure results in device disable for drivers
> which don't implement newer reset/EH callbacks, this change results in
> no behavior change for single device per port controllers.  For
> slave-possible LLDDs which disable port on command failure, (1) such
> drivers don't exist currently, (2) issuing command to the other device
> of once-disabled port shouldn't result in catastrophe even if such
> driver exists.  So, this should be enough as a temporary measure.
> 
> Signed-off-by: Tejun Heo <htejun@gmail.com>

applied 1-4

[PATCH 3/5] libata: use ata_dev_disable() in ata_bus_probe()

[Tejun Heo]

We may or may not disable a device after ata_dev_configure() fails.
Kill 'not supported, ignoring' message in ata_dev_configure() and use
ata_dev_disable() in ata_bus_probe().

Signed-off-by: Tejun Heo <htejun@gmail.com>

---

 drivers/scsi/libata-core.c |    4 +---
 1 files changed, 1 insertions(+), 3 deletions(-)

cc1c133b0ab1a846987c042f2893d3b35f2e6220
diff --git a/drivers/scsi/libata-core.c b/drivers/scsi/libata-core.c
index d1530b9..b08e79f 100644
--- a/drivers/scsi/libata-core.c
+++ b/drivers/scsi/libata-core.c
@@ -1363,8 +1363,6 @@ static int ata_dev_configure(struct ata_
 	return 0;
 
 err_out_nosup:
-	printk(KERN_WARNING "ata%u: dev %u not supported, ignoring\n",
-	       ap->id, dev->devno);
 	DPRINTK("EXIT, err\n");
 	return rc;
 }
@@ -1431,7 +1429,7 @@ static int ata_bus_probe(struct ata_port
 		}
 
 		if (ata_dev_configure(ap, dev, 1)) {
-			dev->class++;	/* disable device */
+			ata_dev_disable(ap, dev);
 			continue;
 		}
 
-- 
1.2.4

[PATCH 4/5] libata: make ata_set_mode() responsible for failure handling

[Tejun Heo]

Make ata_set_mode() responsible for determining whether to take port
or device offline on failure.  ata_dev_set_xfermode() and
ata_dev_set_mode() indicate error to the caller instead of disabling
port directly on failure.  Also, for consistency, ata_dev_present()
check is done in ata_set_mode() instead of ata_dev_set_mode().

Signed-off-by: Tejun Heo <htejun@gmail.com>

---

 drivers/scsi/libata-core.c |   56 ++++++++++++++++++++++++++++----------------
 1 files changed, 36 insertions(+), 20 deletions(-)

efdc8a26c53fce5e7ca529a0758e565679947366
diff --git a/drivers/scsi/libata-core.c b/drivers/scsi/libata-core.c
index b08e79f..6970f68 100644
--- a/drivers/scsi/libata-core.c
+++ b/drivers/scsi/libata-core.c
@@ -64,7 +64,8 @@
 static unsigned int ata_dev_init_params(struct ata_port *ap,
 					struct ata_device *dev);
 static void ata_set_mode(struct ata_port *ap);
-static void ata_dev_set_xfermode(struct ata_port *ap, struct ata_device *dev);
+static unsigned int ata_dev_set_xfermode(struct ata_port *ap,
+					 struct ata_device *dev);
 static void ata_dev_xfermask(struct ata_port *ap, struct ata_device *dev);
 
 static unsigned int ata_unique_id = 1;
@@ -1742,20 +1743,28 @@ int ata_timing_compute(struct ata_device
 	return 0;
 }
 
-static void ata_dev_set_mode(struct ata_port *ap, struct ata_device *dev)
+static int ata_dev_set_mode(struct ata_port *ap, struct ata_device *dev)
 {
-	if (!ata_dev_present(dev) || (ap->flags & ATA_FLAG_PORT_DISABLED))
-		return;
+	unsigned int err_mask;
+	int rc;
 
 	if (dev->xfer_shift == ATA_SHIFT_PIO)
 		dev->flags |= ATA_DFLAG_PIO;
 
-	ata_dev_set_xfermode(ap, dev);
+	err_mask = ata_dev_set_xfermode(ap, dev);
+	if (err_mask) {
+		printk(KERN_ERR
+		       "ata%u: failed to set xfermode (err_mask=0x%x)\n",
+		       ap->id, err_mask);
+		return -EIO;
+	}
 
-	if (ata_dev_revalidate(ap, dev, 0)) {
-		printk(KERN_ERR "ata%u: failed to revalidate after set "
-		       "xfermode, disabled\n", ap->id);
-		ata_port_disable(ap);
+	rc = ata_dev_revalidate(ap, dev, 0);
+	if (rc) {
+		printk(KERN_ERR
+		       "ata%u: failed to revalidate after set xfermode\n",
+		       ap->id);
+		return rc;
 	}
 
 	DPRINTK("xfer_shift=%u, xfer_mode=0x%x\n",
@@ -1764,6 +1773,7 @@ static void ata_dev_set_mode(struct ata_
 	printk(KERN_INFO "ata%u: dev %u configured for %s\n",
 	       ap->id, dev->devno,
 	       ata_mode_string(ata_xfer_mode2mask(dev->xfer_mode)));
+	return 0;
 }
 
 static int ata_host_set_pio(struct ata_port *ap)
@@ -1847,11 +1857,15 @@ static void ata_set_mode(struct ata_port
 	ata_host_set_dma(ap);
 
 	/* step 4: update devices' xfer mode */
-	for (i = 0; i < ATA_MAX_DEVICES; i++)
-		ata_dev_set_mode(ap, &ap->device[i]);
+	for (i = 0; i < ATA_MAX_DEVICES; i++) {
+		struct ata_device *dev = &ap->device[i];
 
-	if (ap->flags & ATA_FLAG_PORT_DISABLED)
-		return;
+		if (!ata_dev_present(dev))
+			continue;
+
+		if (ata_dev_set_mode(ap, dev))
+			goto err_out;
+	}
 
 	if (ap->ops->post_set_mode)
 		ap->ops->post_set_mode(ap);
@@ -2724,11 +2738,16 @@ static void ata_dev_xfermask(struct ata_
  *
  *	LOCKING:
  *	PCI/etc. bus probe sem.
+ *
+ *	RETURNS:
+ *	0 on success, AC_ERR_* mask otherwise.
  */
 
-static void ata_dev_set_xfermode(struct ata_port *ap, struct ata_device *dev)
+static unsigned int ata_dev_set_xfermode(struct ata_port *ap,
+					 struct ata_device *dev)
 {
 	struct ata_taskfile tf;
+	unsigned int err_mask;
 
 	/* set up set-features taskfile */
 	DPRINTK("set features - xfer mode\n");
@@ -2740,13 +2759,10 @@ static void ata_dev_set_xfermode(struct 
 	tf.protocol = ATA_PROT_NODATA;
 	tf.nsect = dev->xfer_mode;
 
-	if (ata_exec_internal(ap, dev, &tf, DMA_NONE, NULL, 0)) {
-		printk(KERN_ERR "ata%u: failed to set xfermode, disabled\n",
-		       ap->id);
-		ata_port_disable(ap);
-	}
+	err_mask = ata_exec_internal(ap, dev, &tf, DMA_NONE, NULL, 0);
 
-	DPRINTK("EXIT\n");
+	DPRINTK("EXIT, err_mask=%x\n", err_mask);
+	return err_mask;
 }
 
 /**
-- 
1.2.4

[PATCH 5/5] libata: add @disable_on_err argument to ata_set_mode()

[Tejun Heo]

ata_set_mode() used to disable whole port on failure.  This patch adds
@disable_on_err which makes ata_set_mode() disable failing devices
when non-zero, and simply return when zero.  Due to the port-wide
characteristic of ATA xfer mode configuration, ata_mode_set() is the
final place to determine device offlining; thus, the @disable_on_err
mechanism to tell it which action to take on failure.

Now port is disabled only if all devices on the port is disabled.
This behavior change is intentional.

Signed-off-by: Tejun Heo <htejun@gmail.com>

---

 drivers/scsi/libata-core.c |   61 +++++++++++++++++++++++++++-----------------
 1 files changed, 37 insertions(+), 24 deletions(-)

4576ddde674d91702cda2f249d3c83555ea26d77
diff --git a/drivers/scsi/libata-core.c b/drivers/scsi/libata-core.c
index 6970f68..b60a7dc 100644
--- a/drivers/scsi/libata-core.c
+++ b/drivers/scsi/libata-core.c
@@ -63,7 +63,7 @@
 
 static unsigned int ata_dev_init_params(struct ata_port *ap,
 					struct ata_device *dev);
-static void ata_set_mode(struct ata_port *ap);
+static int ata_set_mode(struct ata_port *ap, int disable_on_err);
 static unsigned int ata_dev_set_xfermode(struct ata_port *ap,
 					 struct ata_device *dev);
 static void ata_dev_xfermask(struct ata_port *ap, struct ata_device *dev);
@@ -1437,16 +1437,16 @@ static int ata_bus_probe(struct ata_port
 		found = 1;
 	}
 
-	if (!found)
-		goto err_out_disable;
-
-	ata_set_mode(ap);
-	if (ap->flags & ATA_FLAG_PORT_DISABLED)
-		goto err_out_disable;
-
-	return 0;
+	/* configure transfer mode */
+	if (found) {
+		ata_set_mode(ap, 1);
+		for (i = 0; i < ATA_MAX_DEVICES; i++)
+			if (ata_dev_present(&ap->device[i]))
+				return 0;
+	}
 
-err_out_disable:
+	/* no device present, disable port */
+	ata_port_disable(ap);
 	ap->ops->port_disable(ap);
 	return -1;
 }
@@ -1776,7 +1776,7 @@ static int ata_dev_set_mode(struct ata_p
 	return 0;
 }
 
-static int ata_host_set_pio(struct ata_port *ap)
+static int ata_host_set_pio(struct ata_port *ap, int disable_on_err)
 {
 	int i;
 
@@ -1787,8 +1787,13 @@ static int ata_host_set_pio(struct ata_p
 			continue;
 
 		if (!dev->pio_mode) {
-			printk(KERN_WARNING "ata%u: no PIO support for device %d.\n", ap->id, i);
-			return -1;
+			printk(KERN_WARNING "ata%u: dev %u no PIO support\n",
+			       ap->id, dev->devno);
+			if (disable_on_err) {
+				ata_dev_disable(ap, dev);
+				continue;
+			} else
+				return -EINVAL;
 		}
 
 		dev->xfer_mode = dev->pio_mode;
@@ -1820,13 +1825,19 @@ static void ata_host_set_dma(struct ata_
 /**
  *	ata_set_mode - Program timings and issue SET FEATURES - XFER
  *	@ap: port on which timings will be programmed
+ *	@disable_on_err: disable device on error
  *
- *	Set ATA device disk transfer mode (PIO3, UDMA6, etc.).
+ *	Set ATA device disk transfer mode (PIO3, UDMA6, etc.).  If
+ *	@disable_on_err is non-zero, devices which fail to configure
+ *	are taken offline and this function always succeeds.
  *
  *	LOCKING:
  *	PCI/etc. bus probe sem.
+ *
+ *	RETURNS:
+ *	0 on success, negative errno otherwise
  */
-static void ata_set_mode(struct ata_port *ap)
+static int ata_set_mode(struct ata_port *ap, int disable_on_err)
 {
 	int i, rc;
 
@@ -1849,9 +1860,9 @@ static void ata_set_mode(struct ata_port
 	}
 
 	/* step 2: always set host PIO timings */
-	rc = ata_host_set_pio(ap);
+	rc = ata_host_set_pio(ap, disable_on_err);
 	if (rc)
-		goto err_out;
+		return rc;
 
 	/* step 3: set host DMA timings */
 	ata_host_set_dma(ap);
@@ -1863,17 +1874,19 @@ static void ata_set_mode(struct ata_port
 		if (!ata_dev_present(dev))
 			continue;
 
-		if (ata_dev_set_mode(ap, dev))
-			goto err_out;
+		rc = ata_dev_set_mode(ap, dev);
+		if (rc) {
+			if (disable_on_err)
+				ata_dev_disable(ap, dev);
+			else
+				return rc;
+		}
 	}
 
 	if (ap->ops->post_set_mode)
 		ap->ops->post_set_mode(ap);
 
-	return;
-
-err_out:
-	ata_port_disable(ap);
+	return 0;
 }
 
 /**
@@ -4318,7 +4331,7 @@ int ata_device_resume(struct ata_port *a
 {
 	if (ap->flags & ATA_FLAG_SUSPENDED) {
 		ap->flags &= ~ATA_FLAG_SUSPENDED;
-		ata_set_mode(ap);
+		ata_set_mode(ap, 1);
 	}
 	if (!ata_dev_present(dev))
 		return 0;
-- 
1.2.4

Re: [PATCH 5/5] libata: add @disable_on_err argument to ata_set_mode()

[Jeff Garzik]

Tejun Heo wrote:
> ata_set_mode() used to disable whole port on failure.  This patch adds
> @disable_on_err which makes ata_set_mode() disable failing devices
> when non-zero, and simply return when zero.  Due to the port-wide
> characteristic of ATA xfer mode configuration, ata_mode_set() is the
> final place to determine device offlining; thus, the @disable_on_err
> mechanism to tell it which action to take on failure.
> 
> Now port is disabled only if all devices on the port is disabled.
> This behavior change is intentional.
> 
> Signed-off-by: Tejun Heo <htejun@gmail.com>

What about this scenario:

* set features - xfer mode fails
* we offline failed device
* we talk to another device on the same bus
* now the PATA cable is possibly spewing something the failed device 
won't like

Another scenario:  some of the drivers/ide hardware supported only by 
the 'generic' driver.  Some of the hardware, we can do what the device 
is already programmed to do, and that's it.  It might be in DMA mode, in 
which case we can DMA.  But we can't [re]tune it at all.

Thus if set features - xfer mode fails, we can do the easy thing -- stop 
talking to the port completely -- or the hard thing, recovery.  Recovery 
should involve attempting to see if the device, which by definition 
responded to IDENTIFY DEVICE successfully, will once again respond to 
IDENTIFY DEVICE.  That will tell us what mode the drive is in, tell us 
if its responding, and allow us to decide how safe it is to program the 
OTHER device at a higher speed.

	Jeff

Re: [PATCH 5/5] libata: add @disable_on_err argument to ata_set_mode()

[Alan Cox]

On Gwe, 2006-03-24 at 10:04 -0500, Jeff Garzik wrote:
> * set features - xfer mode fails

We should retry here btw but don't

> * we offline failed device
> * we talk to another device on the same bus
> * now the PATA cable is possibly spewing something the failed device 
> won't like

The only thing that will matter is address setup timings. Since we
compute the acceptable modes and set modes according to the pair of
drives if present I would expect the disabling to risk us setting a
wrong mode. A deferred disable would behave better perhaps

> Another scenario:  some of the drivers/ide hardware supported only by 
> the 'generic' driver.  Some of the hardware, we can do what the device 
> is already programmed to do, and that's it.  It might be in DMA mode, in 
> which case we can DMA.  But we can't [re]tune it at all.

All handled by the PATA patches which don't use the usual mode compute
path at all but their own setup function replacement.

> Thus if set features - xfer mode fails, we can do the easy thing -- stop 
> talking to the port completely -- or the hard thing, recovery.  Recovery 
> should involve attempting to see if the device, which by definition 
> responded to IDENTIFY DEVICE successfully, will once again respond to 
> IDENTIFY DEVICE. 

We don't care what mode the other device is in, we just care that any
shared timing address setup is the slower of the pair on the cable. We
could just assume its in PIO_0 for timing purposes then offline it after
timing set up.

[PATCH 1/2] libata: implement ata_dev_enabled, disabled and present()

[Tejun Heo]

This patch renames ata_dev_present() to ata_dev_enabled() and adds
ata_dev_disabled() and ata_dev_present().  This is to discern the
state where a device is present but disabled from not-present state.
This disctinction is necessary when configuring transfer mode because
device selection timing must not be violated even if a device fails to
configures.

Signed-off-by: Tejun Heo <htejun@gmail.com>

---

More explanation in the next patch.

 drivers/scsi/libata-core.c |   24 ++++++++++++------------
 drivers/scsi/libata-scsi.c |    4 ++--
 drivers/scsi/sata_mv.c     |    2 +-
 drivers/scsi/sata_sil.c    |    2 +-
 include/linux/libata.h     |   22 +++++++++++++++++++++-
 5 files changed, 37 insertions(+), 17 deletions(-)

diff --git a/drivers/scsi/libata-core.c b/drivers/scsi/libata-core.c
index 516b1a6..f3277f8 100644
--- a/drivers/scsi/libata-core.c
+++ b/drivers/scsi/libata-core.c
@@ -397,7 +397,7 @@ static const char *ata_mode_string(unsig
 
 static void ata_dev_disable(struct ata_port *ap, struct ata_device *dev)
 {
-	if (ata_dev_present(dev)) {
+	if (ata_dev_enabled(dev)) {
 		printk(KERN_WARNING "ata%u: dev %u disabled\n",
 		       ap->id, dev->devno);
 		dev->class++;
@@ -1210,7 +1210,7 @@ static int ata_dev_configure(struct ata_
 	unsigned int xfer_mask;
 	int i, rc;
 
-	if (!ata_dev_present(dev)) {
+	if (!ata_dev_enabled(dev)) {
 		DPRINTK("ENTER/EXIT (host %u, dev %u) -- nodev\n",
 			ap->id, dev->devno);
 		return 0;
@@ -1389,7 +1389,7 @@ static int ata_bus_probe(struct ata_port
 
 		dev->class = classes[i];
 
-		if (!ata_dev_present(dev))
+		if (!ata_dev_enabled(dev))
 			continue;
 
 		WARN_ON(dev->id != NULL);
@@ -1556,7 +1556,7 @@ void sata_phy_reset(struct ata_port *ap)
 struct ata_device *ata_dev_pair(struct ata_port *ap, struct ata_device *adev)
 {
 	struct ata_device *pair = &ap->device[1 - adev->devno];
-	if (!ata_dev_present(pair))
+	if (!ata_dev_enabled(pair))
 		return NULL;
 	return pair;
 }
@@ -1769,7 +1769,7 @@ static int ata_host_set_pio(struct ata_p
 	for (i = 0; i < ATA_MAX_DEVICES; i++) {
 		struct ata_device *dev = &ap->device[i];
 
-		if (!ata_dev_present(dev))
+		if (!ata_dev_enabled(dev))
 			continue;
 
 		if (!dev->pio_mode) {
@@ -1793,7 +1793,7 @@ static void ata_host_set_dma(struct ata_
 	for (i = 0; i < ATA_MAX_DEVICES; i++) {
 		struct ata_device *dev = &ap->device[i];
 
-		if (!ata_dev_present(dev) || !dev->dma_mode)
+		if (!ata_dev_enabled(dev) || !dev->dma_mode)
 			continue;
 
 		dev->xfer_mode = dev->dma_mode;
@@ -1821,7 +1821,7 @@ static void ata_set_mode(struct ata_port
 		struct ata_device *dev = &ap->device[i];
 		unsigned int pio_mask, dma_mask;
 
-		if (!ata_dev_present(dev))
+		if (!ata_dev_enabled(dev))
 			continue;
 
 		ata_dev_xfermask(ap, dev);
@@ -1846,7 +1846,7 @@ static void ata_set_mode(struct ata_port
 	for (i = 0; i < ATA_MAX_DEVICES; i++) {
 		struct ata_device *dev = &ap->device[i];
 
-		if (!ata_dev_present(dev))
+		if (!ata_dev_enabled(dev))
 			continue;
 
 		if (ata_dev_set_mode(ap, dev))
@@ -2523,7 +2523,7 @@ int ata_dev_revalidate(struct ata_port *
 	u16 *id;
 	int rc;
 
-	if (!ata_dev_present(dev))
+	if (!ata_dev_enabled(dev))
 		return -ENODEV;
 
 	class = dev->class;
@@ -2647,7 +2647,7 @@ static void ata_dev_xfermask(struct ata_
 	/* use port-wide xfermask for now */
 	for (i = 0; i < ATA_MAX_DEVICES; i++) {
 		struct ata_device *d = &ap->device[i];
-		if (!ata_dev_present(d))
+		if (!ata_dev_enabled(d))
 			continue;
 		xfer_mask &= ata_pack_xfermask(d->pio_mask, d->mwdma_mask,
 					       d->udma_mask);
@@ -4256,7 +4256,7 @@ int ata_device_resume(struct ata_port *a
 		ap->flags &= ~ATA_FLAG_SUSPENDED;
 		ata_set_mode(ap);
 	}
-	if (!ata_dev_present(dev))
+	if (!ata_dev_enabled(dev))
 		return 0;
 	if (dev->class == ATA_DEV_ATA)
 		ata_start_drive(ap, dev);
@@ -4274,7 +4274,7 @@ int ata_device_resume(struct ata_port *a
  */
 int ata_device_suspend(struct ata_port *ap, struct ata_device *dev, pm_message_t state)
 {
-	if (!ata_dev_present(dev))
+	if (!ata_dev_enabled(dev))
 		return 0;
 	if (dev->class == ATA_DEV_ATA)
 		ata_flush_cache(ap, dev);
diff --git a/drivers/scsi/libata-scsi.c b/drivers/scsi/libata-scsi.c
index 628191b..a117f98 100644
--- a/drivers/scsi/libata-scsi.c
+++ b/drivers/scsi/libata-scsi.c
@@ -2353,7 +2353,7 @@ ata_scsi_find_dev(struct ata_port *ap, c
 		     (scsidev->lun != 0)))
 		return NULL;
 
-	if (unlikely(!ata_dev_present(dev)))
+	if (unlikely(!ata_dev_enabled(dev)))
 		return NULL;
 
 	if (!atapi_enabled || (ap->flags & ATA_FLAG_NO_ATAPI)) {
@@ -2747,7 +2747,7 @@ void ata_scsi_scan_host(struct ata_port 
 	for (i = 0; i < ATA_MAX_DEVICES; i++) {
 		dev = &ap->device[i];
 
-		if (ata_dev_present(dev))
+		if (ata_dev_enabled(dev))
 			scsi_scan_target(&ap->host->shost_gendev, 0, i, 0, 0);
 	}
 }
diff --git a/drivers/scsi/sata_mv.c b/drivers/scsi/sata_mv.c
index 275ed9b..bbe4ddc 100644
--- a/drivers/scsi/sata_mv.c
+++ b/drivers/scsi/sata_mv.c
@@ -1985,7 +1985,7 @@ comreset_retry:
 	tf.nsect = readb((void __iomem *) ap->ioaddr.nsect_addr);
 
 	dev->class = ata_dev_classify(&tf);
-	if (!ata_dev_present(dev)) {
+	if (!ata_dev_enabled(dev)) {
 		VPRINTK("Port disabled post-sig: No device present.\n");
 		ata_port_disable(ap);
 	}
diff --git a/drivers/scsi/sata_sil.c b/drivers/scsi/sata_sil.c
index 18c296c..d6c7086 100644
--- a/drivers/scsi/sata_sil.c
+++ b/drivers/scsi/sata_sil.c
@@ -264,7 +264,7 @@ static void sil_post_set_mode (struct at
 
 	for (i = 0; i < 2; i++) {
 		dev = &ap->device[i];
-		if (!ata_dev_present(dev))
+		if (!ata_dev_enabled(dev))
 			dev_mode[i] = 0;	/* PIO0/1/2 */
 		else if (dev->flags & ATA_DFLAG_PIO)
 			dev_mode[i] = 1;	/* PIO3/4 */
diff --git a/include/linux/libata.h b/include/linux/libata.h
index 9fcc061..1d16293 100644
--- a/include/linux/libata.h
+++ b/include/linux/libata.h
@@ -663,11 +663,31 @@ static inline unsigned int ata_tag_valid
 	return (tag < ATA_MAX_QUEUE) ? 1 : 0;
 }
 
-static inline unsigned int ata_class_present(unsigned int class)
+static inline unsigned int ata_class_enabled(unsigned int class)
 {
 	return class == ATA_DEV_ATA || class == ATA_DEV_ATAPI;
 }
 
+static inline unsigned int ata_class_disabled(unsigned int class)
+{
+	return class == ATA_DEV_ATA_UNSUP || class == ATA_DEV_ATAPI_UNSUP;
+}
+
+static inline unsigned int ata_class_present(unsigned int class)
+{
+	return ata_class_enabled(class) || ata_class_disabled(class);
+}
+
+static inline unsigned int ata_dev_enabled(const struct ata_device *dev)
+{
+	return ata_class_enabled(dev->class);
+}
+
+static inline unsigned int ata_dev_disabled(const struct ata_device *dev)
+{
+	return ata_class_disabled(dev->class);
+}
+
 static inline unsigned int ata_dev_present(const struct ata_device *dev)
 {
 	return ata_class_present(dev->class);

Re: [PATCH 1/2] libata: implement ata_dev_enabled, disabled and present()

[Jeff Garzik]

Tejun Heo wrote:
> This patch renames ata_dev_present() to ata_dev_enabled() and adds
> ata_dev_disabled() and ata_dev_present().  This is to discern the
> state where a device is present but disabled from not-present state.
> This disctinction is necessary when configuring transfer mode because
> device selection timing must not be violated even if a device fails to
> configures.
> 
> Signed-off-by: Tejun Heo <htejun@gmail.com>

OK in general, but NAK for transition reasons:  this invites silent 
breakage of code not yet in #upstream, because ata_dev_present() 
implementation changes but the function signature does not.  If a new 
ata_dev_present() is to be introduced, either a different name should be 
chosen, or the function signature should change such that an older user 
of the API will see a compiler warning or error.

	Jeff

[PATCH 2/2] libata: add @disable_on_err argument to ata_set_mode()

[Tejun Heo]

ata_set_mode() used to disable whole port on failure.  This patch adds
@disable_on_err which makes ata_set_mode() disable failing devices
when non-zero, and simply return when zero.  Due to the port-wide
characteristic of ATA xfer mode configuration, ata_mode_set() is the
final place to determine device offlining; thus, the @disable_on_err
mechanism to tell it which action to take on failure.

With this patch, only failing devices are disabled not the whole port.
Transfer mode configuration must consider all devices on the port
regardless of failure status; otherwise, device selection timing can
be violoated resulting in malfunction.  This patch makes
ata_dev_xfermask() consider disabled but present devices such that
device timing selection timing is honored.

Signed-off-by: Tejun Heo <htejun@gmail.com>

---

As said in the above comment, this patch makes sure that present but
disabled devices are taken into account when determining transfer
mode.  If IDENTIFY data is present, it is used; otherwise, PIO0 is
forced.  I think this should be enough.

Thanks.

 libata-core.c |   79 ++++++++++++++++++++++++++++++++++++----------------------
 1 file changed, 50 insertions(+), 29 deletions(-)

diff --git a/drivers/scsi/libata-core.c b/drivers/scsi/libata-core.c
index 3e0c4b1..121c201 100644
--- a/drivers/scsi/libata-core.c
+++ b/drivers/scsi/libata-core.c
@@ -63,7 +63,7 @@
 
 static unsigned int ata_dev_init_params(struct ata_port *ap,
 					struct ata_device *dev);
-static void ata_set_mode(struct ata_port *ap);
+static int ata_set_mode(struct ata_port *ap, int disable_on_err);
 static unsigned int ata_dev_set_xfermode(struct ata_port *ap,
 					 struct ata_device *dev);
 static void ata_dev_xfermask(struct ata_port *ap, struct ata_device *dev);
@@ -1394,7 +1394,7 @@ static int ata_bus_probe(struct ata_port
 
 		WARN_ON(dev->id != NULL);
 		if (ata_dev_read_id(ap, dev, &dev->class, 1, &dev->id)) {
-			dev->class = ATA_DEV_NONE;
+			ata_dev_disable(ap, dev);
 			continue;
 		}
 
@@ -1406,16 +1406,16 @@ static int ata_bus_probe(struct ata_port
 		found = 1;
 	}
 
-	if (!found)
-		goto err_out_disable;
-
-	ata_set_mode(ap);
-	if (ap->flags & ATA_FLAG_PORT_DISABLED)
-		goto err_out_disable;
-
-	return 0;
+	/* configure transfer mode */
+	if (found) {
+		ata_set_mode(ap, 1);
+		for (i = 0; i < ATA_MAX_DEVICES; i++)
+			if (ata_dev_present(&ap->device[i]))
+				return 0;
+	}
 
-err_out_disable:
+	/* no device present, disable port */
+	ata_port_disable(ap);
 	ap->ops->port_disable(ap);
 	return -1;
 }
@@ -1762,7 +1762,7 @@ static int ata_dev_set_mode(struct ata_p
 	return 0;
 }
 
-static int ata_host_set_pio(struct ata_port *ap)
+static int ata_host_set_pio(struct ata_port *ap, int disable_on_err)
 {
 	int i;
 
@@ -1773,8 +1773,13 @@ static int ata_host_set_pio(struct ata_p
 			continue;
 
 		if (!dev->pio_mode) {
-			printk(KERN_WARNING "ata%u: no PIO support for device %d.\n", ap->id, i);
-			return -1;
+			printk(KERN_WARNING "ata%u: dev %u no PIO support\n",
+			       ap->id, dev->devno);
+			if (disable_on_err) {
+				ata_dev_disable(ap, dev);
+				continue;
+			} else
+				return -EINVAL;
 		}
 
 		dev->xfer_mode = dev->pio_mode;
@@ -1806,13 +1811,19 @@ static void ata_host_set_dma(struct ata_
 /**
  *	ata_set_mode - Program timings and issue SET FEATURES - XFER
  *	@ap: port on which timings will be programmed
+ *	@disable_on_err: disable device on error
  *
- *	Set ATA device disk transfer mode (PIO3, UDMA6, etc.).
+ *	Set ATA device disk transfer mode (PIO3, UDMA6, etc.).  If
+ *	@disable_on_err is non-zero, devices which fail to configure
+ *	are taken offline and this function always succeeds.
  *
  *	LOCKING:
  *	PCI/etc. bus probe sem.
+ *
+ *	RETURNS:
+ *	0 on success, negative errno otherwise
  */
-static void ata_set_mode(struct ata_port *ap)
+static int ata_set_mode(struct ata_port *ap, int disable_on_err)
 {
 	int i, rc;
 
@@ -1835,9 +1846,9 @@ static void ata_set_mode(struct ata_port
 	}
 
 	/* step 2: always set host PIO timings */
-	rc = ata_host_set_pio(ap);
+	rc = ata_host_set_pio(ap, disable_on_err);
 	if (rc)
-		goto err_out;
+		return rc;
 
 	/* step 3: set host DMA timings */
 	ata_host_set_dma(ap);
@@ -1849,17 +1860,19 @@ static void ata_set_mode(struct ata_port
 		if (!ata_dev_enabled(dev))
 			continue;
 
-		if (ata_dev_set_mode(ap, dev))
-			goto err_out;
+		rc = ata_dev_set_mode(ap, dev);
+		if (rc) {
+			if (disable_on_err)
+				ata_dev_disable(ap, dev);
+			else
+				return rc;
+		}
 	}
 
 	if (ap->ops->post_set_mode)
 		ap->ops->post_set_mode(ap);
 
-	return;
-
-err_out:
-	ata_port_disable(ap);
+	return 0;
 }
 
 /**
@@ -2647,13 +2660,21 @@ static void ata_dev_xfermask(struct ata_
 	/* use port-wide xfermask for now */
 	for (i = 0; i < ATA_MAX_DEVICES; i++) {
 		struct ata_device *d = &ap->device[i];
-		if (!ata_dev_enabled(d))
+		if (!ata_dev_present(d))
 			continue;
 		xfer_mask &= ata_pack_xfermask(d->pio_mask, d->mwdma_mask,
 					       d->udma_mask);
-		xfer_mask &= ata_id_xfermask(d->id);
-		if (ata_dma_blacklisted(d))
-			xfer_mask &= ~(ATA_MASK_MWDMA | ATA_MASK_UDMA);
+		if (d->id) {
+			xfer_mask &= ata_id_xfermask(d->id);
+			if (ata_dma_blacklisted(d))
+				xfer_mask &= ~(ATA_MASK_MWDMA | ATA_MASK_UDMA);
+		} else {
+			/* We have no idea what this device is capable
+			 * of.  Force PIO0 to avoid violating address
+			 * setup timing.
+			 */
+			xfer_mask &= ata_pack_xfermask(1, UINT_MAX, UINT_MAX);
+		}
 	}
 
 	if (ata_dma_blacklisted(dev))
@@ -4254,7 +4275,7 @@ int ata_device_resume(struct ata_port *a
 {
 	if (ap->flags & ATA_FLAG_SUSPENDED) {
 		ap->flags &= ~ATA_FLAG_SUSPENDED;
-		ata_set_mode(ap);
+		ata_set_mode(ap, 1);
 	}
 	if (!ata_dev_enabled(dev))
 		return 0;

Re: [PATCH 2/2] libata: add @disable_on_err argument to ata_set_mode()

[Jeff Garzik]

Tejun Heo wrote:
> ata_set_mode() used to disable whole port on failure.  This patch adds
> @disable_on_err which makes ata_set_mode() disable failing devices
> when non-zero, and simply return when zero.  Due to the port-wide
> characteristic of ATA xfer mode configuration, ata_mode_set() is the
> final place to determine device offlining; thus, the @disable_on_err
> mechanism to tell it which action to take on failure.
> 
> With this patch, only failing devices are disabled not the whole port.
> Transfer mode configuration must consider all devices on the port
> regardless of failure status; otherwise, device selection timing can
> be violoated resulting in malfunction.  This patch makes
> ata_dev_xfermask() consider disabled but present devices such that
> device timing selection timing is honored.
> 
> Signed-off-by: Tejun Heo <htejun@gmail.com>

ACK, but dropped due to dropping patch #1


> As said in the above comment, this patch makes sure that present but
> disabled devices are taken into account when determining transfer
> mode.  If IDENTIFY data is present, it is used; otherwise, PIO0 is
> forced.  I think this should be enough.

Several follow-up comments:

* Ideally, I think libata should re-read the identify data from the 
device.  This (a) makes sure PIO is working, and (b) tells us for 
certain what mode the device is.  That's fine for a follow-up patch 
though, since few will exercise this code anyway.

* skipping ->post_set_mode() in this error case being discussing is 
probably unwise.

BTW, got any PATA hardware lying about?  Since you're wandering into 
xfer mode territory, it would better to test PATA than SATA, as xfer 
mode matters more in the PATA realm.  Intel PATA should be fairly easy 
to find, covered by ata_piix, and all the docs are on developer.intel.com.

	Jeff

Re: [PATCH 2/2] libata: add @disable_on_err argument to ata_set_mode()

[Tejun Heo]

On Fri, Mar 24, 2006 at 11:03:55PM -0500, Jeff Garzik wrote:
> Tejun Heo wrote:
> >ata_set_mode() used to disable whole port on failure.  This patch adds
> >@disable_on_err which makes ata_set_mode() disable failing devices
> >when non-zero, and simply return when zero.  Due to the port-wide
> >characteristic of ATA xfer mode configuration, ata_mode_set() is the
> >final place to determine device offlining; thus, the @disable_on_err
> >mechanism to tell it which action to take on failure.
> >
> >With this patch, only failing devices are disabled not the whole port.
> >Transfer mode configuration must consider all devices on the port
> >regardless of failure status; otherwise, device selection timing can
> >be violoated resulting in malfunction.  This patch makes
> >ata_dev_xfermask() consider disabled but present devices such that
> >device timing selection timing is honored.
> >
> >Signed-off-by: Tejun Heo <htejun@gmail.com>
> 
> ACK, but dropped due to dropping patch #1
> 

I'll remove ata_dev_present() for the time being.  It's used in only
one place anyway.  We can do with ata_dev_enabled() ||
ata_dev_disabled() for the time being and resurrect ata_dev_present()
after one or two releases, I think.

> 
> >As said in the above comment, this patch makes sure that present but
> >disabled devices are taken into account when determining transfer
> >mode.  If IDENTIFY data is present, it is used; otherwise, PIO0 is
> >forced.  I think this should be enough.
> 
> Several follow-up comments:
> 
> * Ideally, I think libata should re-read the identify data from the 
> device.  This (a) makes sure PIO is working, and (b) tells us for 
> certain what mode the device is.  That's fine for a follow-up patch 
> though, since few will exercise this code anyway.

Agreed, we can revalidate the device and then try to configure the
next lower transfer mode until we succeed.  For the time being, I just
wanna move forward with the current code so that EH changes can be
submitted sooner than later.

> * skipping ->post_set_mode() in this error case being discussing is 
> probably unwise.

We don't skip ->post_set_mode() unless @disable_on_err is zero, in
which case the upper layer is responsible for handling the error
condition.  Upper layer will usually reset and reconfigure the whole
thing again, so omitting ->post_set_mode() should be fine there (we
need to force PIO0 before IDENTIFY'ing though).

> BTW, got any PATA hardware lying about?  Since you're wandering into 
> xfer mode territory, it would better to test PATA than SATA, as xfer 
> mode matters more in the PATA realm.  Intel PATA should be fairly easy 
> to find, covered by ata_piix, and all the docs are on developer.intel.com.

Yeap, my test machine's ICH7 can do ata_piix and my notebook has
ICH6M.

-- 
tejun

[PATCH 1/2] libata: implement ata_dev_enabled and disabled()

[Tejun Heo]

This patch renames ata_dev_present() to ata_dev_enabled() and adds
ata_dev_disabled().  This is to discern the state where a device is
present but disabled from not-present state.  This disctinction is
necessary when configuring transfer mode because device selection
timing must not be violated even if a device fails to configure.

Signed-off-by: Tejun Heo <htejun@gmail.com>

---

This is the same patch without ata_class/dev_present().

 drivers/scsi/libata-core.c |   24 ++++++++++++------------
 drivers/scsi/libata-scsi.c |    4 ++--
 drivers/scsi/sata_mv.c     |    2 +-
 drivers/scsi/sata_sil.c    |    2 +-
 include/linux/libata.h     |   16 +++++++++++++---
 5 files changed, 29 insertions(+), 19 deletions(-)

diff --git a/drivers/scsi/libata-core.c b/drivers/scsi/libata-core.c
index 8631056..4cfd6c0 100644
--- a/drivers/scsi/libata-core.c
+++ b/drivers/scsi/libata-core.c
@@ -397,7 +397,7 @@ static const char *ata_mode_string(unsig
 
 static void ata_dev_disable(struct ata_port *ap, struct ata_device *dev)
 {
-	if (ata_dev_present(dev)) {
+	if (ata_dev_enabled(dev)) {
 		printk(KERN_WARNING "ata%u: dev %u disabled\n",
 		       ap->id, dev->devno);
 		dev->class++;
@@ -1210,7 +1210,7 @@ static int ata_dev_configure(struct ata_
 	unsigned int xfer_mask;
 	int i, rc;
 
-	if (!ata_dev_present(dev)) {
+	if (!ata_dev_enabled(dev)) {
 		DPRINTK("ENTER/EXIT (host %u, dev %u) -- nodev\n",
 			ap->id, dev->devno);
 		return 0;
@@ -1389,7 +1389,7 @@ static int ata_bus_probe(struct ata_port
 
 		dev->class = classes[i];
 
-		if (!ata_dev_present(dev))
+		if (!ata_dev_enabled(dev))
 			continue;
 
 		WARN_ON(dev->id != NULL);
@@ -1556,7 +1556,7 @@ void sata_phy_reset(struct ata_port *ap)
 struct ata_device *ata_dev_pair(struct ata_port *ap, struct ata_device *adev)
 {
 	struct ata_device *pair = &ap->device[1 - adev->devno];
-	if (!ata_dev_present(pair))
+	if (!ata_dev_enabled(pair))
 		return NULL;
 	return pair;
 }
@@ -1769,7 +1769,7 @@ static int ata_host_set_pio(struct ata_p
 	for (i = 0; i < ATA_MAX_DEVICES; i++) {
 		struct ata_device *dev = &ap->device[i];
 
-		if (!ata_dev_present(dev))
+		if (!ata_dev_enabled(dev))
 			continue;
 
 		if (!dev->pio_mode) {
@@ -1793,7 +1793,7 @@ static void ata_host_set_dma(struct ata_
 	for (i = 0; i < ATA_MAX_DEVICES; i++) {
 		struct ata_device *dev = &ap->device[i];
 
-		if (!ata_dev_present(dev) || !dev->dma_mode)
+		if (!ata_dev_enabled(dev) || !dev->dma_mode)
 			continue;
 
 		dev->xfer_mode = dev->dma_mode;
@@ -1821,7 +1821,7 @@ static void ata_set_mode(struct ata_port
 		struct ata_device *dev = &ap->device[i];
 		unsigned int pio_mask, dma_mask;
 
-		if (!ata_dev_present(dev))
+		if (!ata_dev_enabled(dev))
 			continue;
 
 		ata_dev_xfermask(ap, dev);
@@ -1846,7 +1846,7 @@ static void ata_set_mode(struct ata_port
 	for (i = 0; i < ATA_MAX_DEVICES; i++) {
 		struct ata_device *dev = &ap->device[i];
 
-		if (!ata_dev_present(dev))
+		if (!ata_dev_enabled(dev))
 			continue;
 
 		if (ata_dev_set_mode(ap, dev))
@@ -2523,7 +2523,7 @@ int ata_dev_revalidate(struct ata_port *
 	u16 *id;
 	int rc;
 
-	if (!ata_dev_present(dev))
+	if (!ata_dev_enabled(dev))
 		return -ENODEV;
 
 	class = dev->class;
@@ -2651,7 +2651,7 @@ static void ata_dev_xfermask(struct ata_
 	/* use port-wide xfermask for now */
 	for (i = 0; i < ATA_MAX_DEVICES; i++) {
 		struct ata_device *d = &ap->device[i];
-		if (!ata_dev_present(d))
+		if (!ata_dev_enabled(d))
 			continue;
 		xfer_mask &= ata_pack_xfermask(d->pio_mask, d->mwdma_mask,
 					       d->udma_mask);
@@ -4260,7 +4260,7 @@ int ata_device_resume(struct ata_port *a
 		ap->flags &= ~ATA_FLAG_SUSPENDED;
 		ata_set_mode(ap);
 	}
-	if (!ata_dev_present(dev))
+	if (!ata_dev_enabled(dev))
 		return 0;
 	if (dev->class == ATA_DEV_ATA)
 		ata_start_drive(ap, dev);
@@ -4278,7 +4278,7 @@ int ata_device_resume(struct ata_port *a
  */
 int ata_device_suspend(struct ata_port *ap, struct ata_device *dev, pm_message_t state)
 {
-	if (!ata_dev_present(dev))
+	if (!ata_dev_enabled(dev))
 		return 0;
 	if (dev->class == ATA_DEV_ATA)
 		ata_flush_cache(ap, dev);
diff --git a/drivers/scsi/libata-scsi.c b/drivers/scsi/libata-scsi.c
index 628191b..a117f98 100644
--- a/drivers/scsi/libata-scsi.c
+++ b/drivers/scsi/libata-scsi.c
@@ -2353,7 +2353,7 @@ ata_scsi_find_dev(struct ata_port *ap, c
 		     (scsidev->lun != 0)))
 		return NULL;
 
-	if (unlikely(!ata_dev_present(dev)))
+	if (unlikely(!ata_dev_enabled(dev)))
 		return NULL;
 
 	if (!atapi_enabled || (ap->flags & ATA_FLAG_NO_ATAPI)) {
@@ -2747,7 +2747,7 @@ void ata_scsi_scan_host(struct ata_port 
 	for (i = 0; i < ATA_MAX_DEVICES; i++) {
 		dev = &ap->device[i];
 
-		if (ata_dev_present(dev))
+		if (ata_dev_enabled(dev))
 			scsi_scan_target(&ap->host->shost_gendev, 0, i, 0, 0);
 	}
 }
diff --git a/drivers/scsi/sata_mv.c b/drivers/scsi/sata_mv.c
index 275ed9b..bbe4ddc 100644
--- a/drivers/scsi/sata_mv.c
+++ b/drivers/scsi/sata_mv.c
@@ -1985,7 +1985,7 @@ comreset_retry:
 	tf.nsect = readb((void __iomem *) ap->ioaddr.nsect_addr);
 
 	dev->class = ata_dev_classify(&tf);
-	if (!ata_dev_present(dev)) {
+	if (!ata_dev_enabled(dev)) {
 		VPRINTK("Port disabled post-sig: No device present.\n");
 		ata_port_disable(ap);
 	}
diff --git a/drivers/scsi/sata_sil.c b/drivers/scsi/sata_sil.c
index 18c296c..d6c7086 100644
--- a/drivers/scsi/sata_sil.c
+++ b/drivers/scsi/sata_sil.c
@@ -264,7 +264,7 @@ static void sil_post_set_mode (struct at
 
 	for (i = 0; i < 2; i++) {
 		dev = &ap->device[i];
-		if (!ata_dev_present(dev))
+		if (!ata_dev_enabled(dev))
 			dev_mode[i] = 0;	/* PIO0/1/2 */
 		else if (dev->flags & ATA_DFLAG_PIO)
 			dev_mode[i] = 1;	/* PIO3/4 */
diff --git a/include/linux/libata.h b/include/linux/libata.h
index 9fcc061..e8519c3 100644
--- a/include/linux/libata.h
+++ b/include/linux/libata.h
@@ -663,14 +663,24 @@ static inline unsigned int ata_tag_valid
 	return (tag < ATA_MAX_QUEUE) ? 1 : 0;
 }
 
-static inline unsigned int ata_class_present(unsigned int class)
+static inline unsigned int ata_class_enabled(unsigned int class)
 {
 	return class == ATA_DEV_ATA || class == ATA_DEV_ATAPI;
 }
 
-static inline unsigned int ata_dev_present(const struct ata_device *dev)
+static inline unsigned int ata_class_disabled(unsigned int class)
 {
-	return ata_class_present(dev->class);
+	return class == ATA_DEV_ATA_UNSUP || class == ATA_DEV_ATAPI_UNSUP;
+}
+
+static inline unsigned int ata_dev_enabled(const struct ata_device *dev)
+{
+	return ata_class_enabled(dev->class);
+}
+
+static inline unsigned int ata_dev_disabled(const struct ata_device *dev)
+{
+	return ata_class_disabled(dev->class);
 }
 
 static inline u8 ata_chk_status(struct ata_port *ap)

Re: [PATCH 1/2] libata: implement ata_dev_enabled and disabled()

[Alan Cox]

On Sad, 2006-03-25 at 15:12 +0900, Tejun Heo wrote:
> This patch renames ata_dev_present() to ata_dev_enabled() and adds
> ata_dev_disabled().  This is to discern the state where a device is
> present but disabled from not-present state.  This disctinction is
> necessary when configuring transfer mode because device selection
> timing must not be violated even if a device fails to configure.

This breaks the expected behaviour of ata_dev_pair(). I'd rather kept
the existing behaviour of ata_dev_present for the moment until we can
figure out what we need to do for "present but not enabled". That should
also fit the scsi layer expectations so we can correctly "offline" a
device.

Re: [PATCH 1/2] libata: implement ata_dev_enabled and disabled()

[Tejun Heo]

On Sat, Mar 25, 2006 at 11:54:55PM +0000, Alan Cox wrote:
> On Sad, 2006-03-25 at 15:12 +0900, Tejun Heo wrote:
> > This patch renames ata_dev_present() to ata_dev_enabled() and adds
> > ata_dev_disabled().  This is to discern the state where a device is
> > present but disabled from not-present state.  This disctinction is
> > necessary when configuring transfer mode because device selection
> > timing must not be violated even if a device fails to configure.
> 
> This breaks the expected behaviour of ata_dev_pair(). I'd rather kept
> the existing behaviour of ata_dev_present for the moment until we can
> figure out what we need to do for "present but not enabled". That should
> also fit the scsi layer expectations so we can correctly "offline" a
> device.
> 

Hello, Alan.

Can you elaborate how ata_dev_pair() is broken by this patch?
ata_dev_present() is simply renamed to ata_dev_enabled().
ata_dev_disabled() only differenciates the conditions where
ata_dev_present() used to think there was no device.  There should be
no behavior change to existing code. 

Thanks.

-- 
tejun

Re: [PATCH 1/2] libata: implement ata_dev_enabled and disabled()

[Alan Cox]

On Sul, 2006-03-26 at 08:57 +0900, Tejun Heo wrote:
> Can you elaborate how ata_dev_pair() is broken by this patch?
> ata_dev_present() is simply renamed to ata_dev_enabled().
> ata_dev_disabled() only differenciates the conditions where
> ata_dev_present() used to think there was no device.  There should be
> no behavior change to existing code. 

Some of the drivers want one sematic some want the other, and
ata_dev_pair is now confusing. Needs a pair of better names and I'm not
sure what they should be

Re: [PATCH 1/2] libata: implement ata_dev_enabled and disabled()

[Tejun Heo]

On Mon, Mar 27, 2006 at 12:17:37PM +0100, Alan Cox wrote:
> On Sul, 2006-03-26 at 08:57 +0900, Tejun Heo wrote:
> > Can you elaborate how ata_dev_pair() is broken by this patch?
> > ata_dev_present() is simply renamed to ata_dev_enabled().
> > ata_dev_disabled() only differenciates the conditions where
> > ata_dev_present() used to think there was no device.  There should be
> > no behavior change to existing code. 
> 
> Some of the drivers want one sematic some want the other, and
> ata_dev_pair is now confusing. Needs a pair of better names and I'm not
> sure what they should be
> 

Hmm... how about using ata_dev_enable() || ata_dev_disable() for
ata_dev_pair() such that any existent device returned and make the
user responsible for ata_dev_enable() testing if it wants only enabled
device?

And I think the current enabled/disabled model matches SCSI device
state model sufficiently.  SCSI doesn't register devices which fail
INQUIRY and devices which fail after being attached get offlined.
Similar thing will happen for libata devices.  Stuff that fails
initial configuration won't be attached to SCSI while devices which
fail during operation will get disabled and then get SCSI-offlined.

Thanks.

-- 
tejun

Re: [PATCH 1/2] libata: implement ata_dev_enabled and disabled()

[Alan Cox]

On Mer, 2006-03-29 at 15:58 +0900, Tejun Heo wrote:
> Hmm... how about using ata_dev_enable() || ata_dev_disable() for
> ata_dev_pair() such that any existent device returned and make the
> user responsible for ata_dev_enable() testing if it wants only enabled
> device?

That seems sensible to me. Agreed

Re: [PATCH 1/2] libata: implement ata_dev_enabled and disabled()

[Jeff Garzik]

Tejun Heo wrote:
> This patch renames ata_dev_present() to ata_dev_enabled() and adds
> ata_dev_disabled().  This is to discern the state where a device is
> present but disabled from not-present state.  This disctinction is
> necessary when configuring transfer mode because device selection
> timing must not be violated even if a device fails to configure.
> 
> Signed-off-by: Tejun Heo <htejun@gmail.com>

ACK patches 1-2.  can you please rediff against current #upstream and 
resend?

	Jeff

Re: [PATCH 1/2] libata: implement ata_dev_enabled and disabled()

[Tejun Heo]

Jeff Garzik wrote:
> Tejun Heo wrote:
>> This patch renames ata_dev_present() to ata_dev_enabled() and adds
>> ata_dev_disabled().  This is to discern the state where a device is
>> present but disabled from not-present state.  This disctinction is
>> necessary when configuring transfer mode because device selection
>> timing must not be violated even if a device fails to configure.
>>
>> Signed-off-by: Tejun Heo <htejun@gmail.com>
> 
> ACK patches 1-2.  can you please rediff against current #upstream and 
> resend?
> 

Hello, Jeff.

Can you please hold a bit? This patch went through interface change last 
night in my repository to support probing retry. I've been diddling with 
new EH like crazy last few days and it's taking more days than I thought 
but new EH looks really good now. I think it can go out today, well, 
unless I hit another unexpected slab corruption. ;(

Thanks.

-- 
tejun

[PATCH 2/2] libata: add @disable_on_err argument to ata_set_mode()

[Tejun Heo]

ata_set_mode() used to disable whole port on failure.  This patch adds
@disable_on_err which makes ata_set_mode() disable failing devices
when non-zero, and simply return when zero.  Due to the port-wide
characteristic of ATA xfer mode configuration, ata_mode_set() is the
final place to determine device offlining; thus, the @disable_on_err
mechanism to tell it which action to take on failure.

With this patch, only failing devices are disabled not the whole port.
Transfer mode configuration must consider all devices on the port
regardless of failure status; otherwise, device selection timing can
be violoated resulting in malfunction.  This patch makes
ata_dev_xfermask() consider disabled but present devices such that
device timing selection timing is honored.

Signed-off-by: Tejun Heo <htejun@gmail.com>

---

 libata-core.c |   79 ++++++++++++++++++++++++++++++++++++----------------------
 1 file changed, 50 insertions(+), 29 deletions(-)

diff --git a/drivers/scsi/libata-core.c b/drivers/scsi/libata-core.c
index 4cfd6c0..ea9bf13 100644
--- a/drivers/scsi/libata-core.c
+++ b/drivers/scsi/libata-core.c
@@ -63,7 +63,7 @@
 
 static unsigned int ata_dev_init_params(struct ata_port *ap,
 					struct ata_device *dev);
-static void ata_set_mode(struct ata_port *ap);
+static int ata_set_mode(struct ata_port *ap, int disable_on_err);
 static unsigned int ata_dev_set_xfermode(struct ata_port *ap,
 					 struct ata_device *dev);
 static void ata_dev_xfermask(struct ata_port *ap, struct ata_device *dev);
@@ -1394,7 +1394,7 @@ static int ata_bus_probe(struct ata_port
 
 		WARN_ON(dev->id != NULL);
 		if (ata_dev_read_id(ap, dev, &dev->class, 1, &dev->id)) {
-			dev->class = ATA_DEV_NONE;
+			ata_dev_disable(ap, dev);
 			continue;
 		}
 
@@ -1406,16 +1406,16 @@ static int ata_bus_probe(struct ata_port
 		found = 1;
 	}
 
-	if (!found)
-		goto err_out_disable;
-
-	ata_set_mode(ap);
-	if (ap->flags & ATA_FLAG_PORT_DISABLED)
-		goto err_out_disable;
-
-	return 0;
+	/* configure transfer mode */
+	if (found) {
+		ata_set_mode(ap, 1);
+		for (i = 0; i < ATA_MAX_DEVICES; i++)
+			if (ata_dev_enabled(&ap->device[i]))
+				return 0;
+	}
 
-err_out_disable:
+	/* no device present, disable port */
+	ata_port_disable(ap);
 	ap->ops->port_disable(ap);
 	return -1;
 }
@@ -1762,7 +1762,7 @@ static int ata_dev_set_mode(struct ata_p
 	return 0;
 }
 
-static int ata_host_set_pio(struct ata_port *ap)
+static int ata_host_set_pio(struct ata_port *ap, int disable_on_err)
 {
 	int i;
 
@@ -1773,8 +1773,13 @@ static int ata_host_set_pio(struct ata_p
 			continue;
 
 		if (!dev->pio_mode) {
-			printk(KERN_WARNING "ata%u: no PIO support for device %d.\n", ap->id, i);
-			return -1;
+			printk(KERN_WARNING "ata%u: dev %u no PIO support\n",
+			       ap->id, dev->devno);
+			if (disable_on_err) {
+				ata_dev_disable(ap, dev);
+				continue;
+			} else
+				return -EINVAL;
 		}
 
 		dev->xfer_mode = dev->pio_mode;
@@ -1806,13 +1811,19 @@ static void ata_host_set_dma(struct ata_
 /**
  *	ata_set_mode - Program timings and issue SET FEATURES - XFER
  *	@ap: port on which timings will be programmed
+ *	@disable_on_err: disable device on error
  *
- *	Set ATA device disk transfer mode (PIO3, UDMA6, etc.).
+ *	Set ATA device disk transfer mode (PIO3, UDMA6, etc.).  If
+ *	@disable_on_err is non-zero, devices which fail to configure
+ *	are taken offline and this function always succeeds.
  *
  *	LOCKING:
  *	PCI/etc. bus probe sem.
+ *
+ *	RETURNS:
+ *	0 on success, negative errno otherwise
  */
-static void ata_set_mode(struct ata_port *ap)
+static int ata_set_mode(struct ata_port *ap, int disable_on_err)
 {
 	int i, rc;
 
@@ -1835,9 +1846,9 @@ static void ata_set_mode(struct ata_port
 	}
 
 	/* step 2: always set host PIO timings */
-	rc = ata_host_set_pio(ap);
+	rc = ata_host_set_pio(ap, disable_on_err);
 	if (rc)
-		goto err_out;
+		return rc;
 
 	/* step 3: set host DMA timings */
 	ata_host_set_dma(ap);
@@ -1849,17 +1860,19 @@ static void ata_set_mode(struct ata_port
 		if (!ata_dev_enabled(dev))
 			continue;
 
-		if (ata_dev_set_mode(ap, dev))
-			goto err_out;
+		rc = ata_dev_set_mode(ap, dev);
+		if (rc) {
+			if (disable_on_err)
+				ata_dev_disable(ap, dev);
+			else
+				return rc;
+		}
 	}
 
 	if (ap->ops->post_set_mode)
 		ap->ops->post_set_mode(ap);
 
-	return;
-
-err_out:
-	ata_port_disable(ap);
+	return 0;
 }
 
 /**
@@ -2651,13 +2664,21 @@ static void ata_dev_xfermask(struct ata_
 	/* use port-wide xfermask for now */
 	for (i = 0; i < ATA_MAX_DEVICES; i++) {
 		struct ata_device *d = &ap->device[i];
-		if (!ata_dev_enabled(d))
+		if (!ata_dev_enabled(d) && !ata_dev_disabled(d))
 			continue;
 		xfer_mask &= ata_pack_xfermask(d->pio_mask, d->mwdma_mask,
 					       d->udma_mask);
-		xfer_mask &= ata_id_xfermask(d->id);
-		if (ata_dma_blacklisted(d))
-			xfer_mask &= ~(ATA_MASK_MWDMA | ATA_MASK_UDMA);
+		if (d->id) {
+			xfer_mask &= ata_id_xfermask(d->id);
+			if (ata_dma_blacklisted(d))
+				xfer_mask &= ~(ATA_MASK_MWDMA | ATA_MASK_UDMA);
+		} else {
+			/* We have no idea what this device is capable
+			 * of.  Force PIO0 to avoid violating address
+			 * setup timing.
+			 */
+			xfer_mask &= ata_pack_xfermask(1, UINT_MAX, UINT_MAX);
+		}
 	}
 
 	if (ata_dma_blacklisted(dev))
@@ -4258,7 +4279,7 @@ int ata_device_resume(struct ata_port *a
 {
 	if (ap->flags & ATA_FLAG_SUSPENDED) {
 		ap->flags &= ~ATA_FLAG_SUSPENDED;
-		ata_set_mode(ap);
+		ata_set_mode(ap, 1);
 	}
 	if (!ata_dev_enabled(dev))
 		return 0;

Re: [PATCH 2/2] libata: add @disable_on_err argument to ata_set_mode()

[Alan Cox]

On Gwe, 2006-03-24 at 23:03 -0500, Jeff Garzik wrote:
> BTW, got any PATA hardware lying about?  Since you're wandering into 
> xfer mode territory, it would better to test PATA than SATA, as xfer 
> mode matters more in the PATA realm.  Intel PATA should be fairly easy 
> to find, covered by ata_piix, and all the docs are on developer.intel.com.

I've got a fair amount of PATA stuff. If you want to play with failed
devices I'd suggest the best test device to work with is actually
something like the SIL680 with shared address setup. 

I've been watching the changes so far and the overall direction looks
right to me. As far as I can see from the PATA side of things the "we
care about another horked device on the bus" situation is

a) Extremely rare (I've almost never seen it on drivers/ide reports)
b) Usually a controller programming screwup (so wants fixing there)
c) With very few exceptions needs no special handling.

I can find no case where a set mode failure followed by a switch to PIO
0 on the controller for that device (which is right - the device didnt
switch so its in the old mode....), doesn't do the right thing.

Alan

[PATCH 2/5] libata: implement ata_dev_disable()

[Tejun Heo]

This patch implements ata_dev_disable() which prints a warning message
and takes @dev offline.  Currently, this is done by explicitly
incrementing dev->class with case-by-case warning messages.  Giving
user clear indication when libata gives up will be more important as
libata will be doing more retries.

Signed-off-by: Tejun Heo <htejun@gmail.com>

---

 drivers/scsi/libata-core.c |    9 +++++++++
 1 files changed, 9 insertions(+), 0 deletions(-)

e117990623836ac284ecd9f472302b5110e03f82
diff --git a/drivers/scsi/libata-core.c b/drivers/scsi/libata-core.c
index 9cd9053..d1530b9 100644
--- a/drivers/scsi/libata-core.c
+++ b/drivers/scsi/libata-core.c
@@ -394,6 +394,15 @@ static const char *ata_mode_string(unsig
 	return "<n/a>";
 }
 
+static void ata_dev_disable(struct ata_port *ap, struct ata_device *dev)
+{
+	if (ata_dev_present(dev)) {
+		printk(KERN_WARNING "ata%u: dev %u disabled\n",
+		       ap->id, dev->devno);
+		dev->class++;
+	}
+}
+
 /**
  *	ata_pio_devchk - PATA device presence detection
  *	@ap: ATA channel to examine
-- 
1.2.4