Skip to contentSkip to conversation

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

PATCH New Tejun Heo 3 participants 24 messages Mar 24, 2006 – Mar 30, 2006

Subsystems (2)
SCSI libata
lore
Share & tools
lore

Tejun Heo

author
#0 lore raw

PATCH 1/1 libata: add @disable_on_err to ata_set_mode(), take#2

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

author
#1 lore raw

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

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 <[email protected]>

---

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

e236cbb94f12a51962fc38db3e1724b17f738c09

drivers/scsi/libata-core.c +16 -0
index 1063928..9cd9053 100644
@@ -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;
 }
Signature · 1 line
1.2.4
#2 lore raw

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

Quoted 14 lines · 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.

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

applied 1-4

author
#3 lore raw

PATCH 2/5 libata: implement ata_dev_disable()

In reply to 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 <[email protected]>

---

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

e117990623836ac284ecd9f472302b5110e03f82

drivers/scsi/libata-core.c +9 -0
index 9cd9053..d1530b9 100644
@@ -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
Signature · 1 line
1.2.4
author
#4 lore raw

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

In reply to 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 <[email protected]>

---

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

cc1c133b0ab1a846987c042f2893d3b35f2e6220

drivers/scsi/libata-core.c +1 -3
index d1530b9..b08e79f 100644
@@ -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;
 		}
Signature · 1 line
1.2.4
author
#5 lore raw

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

In reply to 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 <[email protected]>

---

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

efdc8a26c53fce5e7ca529a0758e565679947366

drivers/scsi/libata-core.c +36 -20
index b08e79f..6970f68 100644
@@ -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;
 }
 
 /**
Signature · 1 line
1.2.4
author
#6 lore raw

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

In reply to 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 <[email protected]>

---

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

4576ddde674d91702cda2f249d3c83555ea26d77

drivers/scsi/libata-core.c +37 -24
index 6970f68..b60a7dc 100644
@@ -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;
Signature · 1 line
1.2.4
#7 lore raw

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

Quoted 11 lines · 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 <[email protected]>

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

#8 lore raw

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

Quoted 1 line · Jeff Garzik

* set features - xfer mode fails

We should retry here btw but don't

Quoted 4 lines

* 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

Quoted 4 lines

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.

Quoted 5 lines

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.

author
#9 lore raw

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

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 <[email protected]>

---

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(-)
drivers/scsi/libata-core.c +12 -12
index 516b1a6..f3277f8 100644
@@ -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);
drivers/scsi/libata-scsi.c +2 -2
index 628191b..a117f98 100644
@@ -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);
 	}
 }
drivers/scsi/sata_mv.c +1 -1
index 275ed9b..bbe4ddc 100644
@@ -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);
 	}
drivers/scsi/sata_sil.c +1 -1
index 18c296c..d6c7086 100644
@@ -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 */
include/linux/libata.h +21 -1
index 9fcc061..1d16293 100644
@@ -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);
#10 lore raw

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

Quoted 8 lines · 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 <[email protected]>

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

author
#11 lore raw

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

In reply to Alan Cox

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 <[email protected]>

---

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(-)
drivers/scsi/libata-core.c +50 -29
index 3e0c4b1..121c201 100644
@@ -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;
#12 lore raw

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

Quoted 7 lines · 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.

Quoted 8 lines

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 <[email protected]>

ACK, but dropped due to dropping patch #1

Quoted 4 lines

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

author
#13 lore raw

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

Quoted 15 lines · Jeff Garzik
Quoted 14 lines · 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.

Quoted 4 lines
Quoted 1 line
Signed-off-by Tejun Heo <[email protected]>

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.

Quoted 6 lines
Quoted 5 lines

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.

Quoted 6 lines

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.

Quoted 2 lines

* 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).

Quoted 4 lines

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.

Signature · 1 line
tejun
author
#14 lore raw

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

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 <[email protected]>

---

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(-)
drivers/scsi/libata-core.c +12 -12
index 8631056..4cfd6c0 100644
@@ -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);
drivers/scsi/libata-scsi.c +2 -2
index 628191b..a117f98 100644
@@ -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);
 	}
 }
drivers/scsi/sata_mv.c +1 -1
index 275ed9b..bbe4ddc 100644
@@ -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);
 	}
drivers/scsi/sata_sil.c +1 -1
index 18c296c..d6c7086 100644
@@ -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 */
include/linux/libata.h +13 -3
index 9fcc061..e8519c3 100644
@@ -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)
#15 lore raw

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

Quoted 5 lines · 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.

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.

author
#16 lore raw

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

Quoted 7 lines · Alan Cox
Quoted 5 lines · 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.

Quoted 6 lines

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.

Signature · 1 line
tejun
#17 lore raw

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

Quoted 5 lines · Tejun Heo

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

author
#18 lore raw

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

Quoted 11 lines · Alan Cox
Quoted 5 lines · Tejun Heo

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.

Signature · 1 line
tejun
#19 lore raw

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

Quoted 4 lines · Tejun Heo

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

#20 lore raw

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

In reply to Tejun Heo
Quoted 7 lines · 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 <[email protected]>

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

Jeff

author
#21 lore raw

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

Quoted 11 lines · Jeff Garzik
Quoted 7 lines · 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 <[email protected]>

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.

Signature · 1 line
tejun
author
#22 lore raw

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

In reply to 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 <[email protected]>

---

 libata-core.c |   79 ++++++++++++++++++++++++++++++++++++----------------------
 1 file changed, 50 insertions(+), 29 deletions(-)
drivers/scsi/libata-core.c +50 -29
index 4cfd6c0..ea9bf13 100644
@@ -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;
#23 lore raw

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

In reply to Jeff Garzik
Quoted 4 lines · Jeff Garzik

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