libata: update EH / configuration behavior

PATCH New Tejun Heo 2 participants 8 messages Oct 31, 2007 – Nov 3, 2007 libata

lore

Share & tools

lore

Tejun Heo

author

2007-10-31 01:33 UTC #0 lore raw

PATCH 1/1 libata: update EH / configuration behavior

Hello,

This patchset contains six small patches updating EH / configuration behavior. This series is focused on improving corner case handling. The following behavior changes are made.

* More robust handling classification failures.

* Improve host link speed down handling when PMP is attached.

* When PMP is attached, don't configure downstream links faster than the host link.

Thanks.

--
tejun

Tejun Heo

author

2007-10-31 01:28 UTC #1 lore raw

PATCH 1/6 libata: fix timing computation in ata_eh_reset()

As jiffies changes asynchronously, it needs to be cached if unchanging timestamp is needed. The code in ata_eh_reset() intended to do that with @now but never actually did it. Fix it.

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

---
 drivers/ata/libata-eh.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

drivers/ata/libata-eh.c +1 -1

index 496edaf..f1c0117 100644
@@ -2184,7 +2184,7 @@ int ata_eh_reset(struct ata_link *link, int classify,
 		unsigned long now = jiffies;
 
 		if (time_before(now, deadline)) {
-			unsigned long delta = deadline - jiffies;
+			unsigned long delta = deadline - now;
 
 			ata_link_printk(link, KERN_WARNING, "reset failed "
 				"(errno=%d), retrying in %u secs\n",

Signature · 1 line

1.5.2.4

Tejun Heo

author

2007-10-31 01:33 UTC #3 lore raw

PATCH 2/6 libata: cosmetic clean up / reorganization of ata_eh_reset()

In reply to Tejun Heo

Clean up and reorganize ata_eh_reset() to ease further changes.

* Cache ARRAY_SIZE(ata_eh_reset_timeouts) in @max_tries.
* Cache link->flags in @lflags.
* Move failure handling block to the end of the function and unnest both success and failure handling blocks.

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

---
 drivers/ata/libata-eh.c |  112 +++++++++++++++++++++++-----------------------
 1 files changed, 56 insertions(+), 56 deletions(-)

drivers/ata/libata-eh.c +56 ~~-56~~

index f1c0117..e0de689 100644
@@ -2064,16 +2064,19 @@ int ata_eh_reset(struct ata_link *link, int classify,
 		 ata_prereset_fn_t prereset, ata_reset_fn_t softreset,
 		 ata_reset_fn_t hardreset, ata_postreset_fn_t postreset)
 {
+	const int max_tries = ARRAY_SIZE(ata_eh_reset_timeouts);
 	struct ata_port *ap = link->ap;
 	struct ata_eh_context *ehc = &link->eh_context;
 	unsigned int *classes = ehc->classes;
+	unsigned int lflags = link->flags;
 	int verbose = !(ehc->i.flags & ATA_EHI_QUIET);
 	int try = 0;
 	struct ata_device *dev;
-	unsigned long deadline;
+	unsigned long deadline, now;
 	unsigned int tmp_action;
 	ata_reset_fn_t reset;
 	unsigned long flags;
+	u32 sstatus;
 	int rc;
 
 	/* about to reset */

@@ -2086,7 +2089,7 @@ int ata_eh_reset(struct ata_link *link, int classify,
 	/* Determine which reset to use and record in ehc->i.action.
 	 * prereset() may examine and modify it.
 	 */
-	if (softreset && (!hardreset || (!(link->flags & ATA_LFLAG_NO_SRST) &&
+	if (softreset && (!hardreset || (!(lflags & ATA_LFLAG_NO_SRST) &&
 					 !sata_set_spd_needed(link) &&
 					 !(ehc->i.action & ATA_EH_HARDRESET))))
 		tmp_action = ATA_EH_SOFTRESET;

@@ -2168,7 +2171,7 @@ int ata_eh_reset(struct ata_link *link, int classify,
 		rc = ata_do_reset(link, reset, classes, deadline);
 
 		if (rc == 0 && classify && classes[0] == ATA_DEV_UNKNOWN &&
-		    !(link->flags & ATA_LFLAG_ASSUME_CLASS)) {
+		    !(lflags & ATA_LFLAG_ASSUME_CLASS)) {
 			ata_link_printk(link, KERN_ERR,
 					"classification failed\n");
 			rc = -EINVAL;

@@ -2176,67 +2179,42 @@ int ata_eh_reset(struct ata_link *link, int classify,
 		}
 	}
 
-	/* if we skipped follow-up srst, clear rc */
-	if (rc == -EAGAIN)
-		rc = 0;
-
-	if (rc && rc != -ERESTART && try < ARRAY_SIZE(ata_eh_reset_timeouts)) {
-		unsigned long now = jiffies;
+	/* -EAGAIN can happen if we skipped followup SRST */
+	if (rc && rc != -EAGAIN)
+		goto fail;
 
-		if (time_before(now, deadline)) {
-			unsigned long delta = deadline - now;
-
-			ata_link_printk(link, KERN_WARNING, "reset failed "
-				"(errno=%d), retrying in %u secs\n",
-				rc, (jiffies_to_msecs(delta) + 999) / 1000);
+	ata_link_for_each_dev(dev, link) {
+		/* After the reset, the device state is PIO 0 and the
+		 * controller state is undefined.  Reset also wakes up
+		 * drives from sleeping mode.
+		 */
+		dev->pio_mode = XFER_PIO_0;
+		dev->flags &= ~ATA_DFLAG_SLEEPING;
 
-			while (delta)
-				delta = schedule_timeout_uninterruptible(delta);
-		}
+		if (ata_link_offline(link))
+			continue;
 
-		if (rc == -EPIPE ||
-		    try == ARRAY_SIZE(ata_eh_reset_timeouts) - 1)
-			sata_down_spd_limit(link);
-		if (hardreset)
-			reset = hardreset;
-		goto retry;
+		/* apply class override and convert UNKNOWN to NONE */
+		if (lflags & ATA_LFLAG_ASSUME_ATA)
+			classes[dev->devno] = ATA_DEV_ATA;
+		else if (lflags & ATA_LFLAG_ASSUME_SEMB)
+			classes[dev->devno] = ATA_DEV_SEMB_UNSUP; /* not yet */
+		else if (classes[dev->devno] == ATA_DEV_UNKNOWN)
+			classes[dev->devno] = ATA_DEV_NONE;
 	}
 
-	if (rc == 0) {
-		u32 sstatus;
+	/* record current link speed */
+	if (sata_scr_read(link, SCR_STATUS, &sstatus) == 0)
+		link->sata_spd = (sstatus >> 4) & 0xf;
 
-		ata_link_for_each_dev(dev, link) {
-			/* After the reset, the device state is PIO 0
-			 * and the controller state is undefined.
-			 * Reset also wakes up drives from sleeping
-			 * mode.
-			 */
-			dev->pio_mode = XFER_PIO_0;
-			dev->flags &= ~ATA_DFLAG_SLEEPING;
+	if (postreset)
+		postreset(link, classes);
 
-			if (ata_link_offline(link))
-				continue;
+	/* reset successful, schedule revalidation */
+	ata_eh_done(link, NULL, ehc->i.action & ATA_EH_RESET_MASK);
+	ehc->i.action |= ATA_EH_REVALIDATE;
 
-			/* apply class override and convert UNKNOWN to NONE */
-			if (link->flags & ATA_LFLAG_ASSUME_ATA)
-				classes[dev->devno] = ATA_DEV_ATA;
-			else if (link->flags & ATA_LFLAG_ASSUME_SEMB)
-				classes[dev->devno] = ATA_DEV_SEMB_UNSUP; /* not yet */
-			else if (classes[dev->devno] == ATA_DEV_UNKNOWN)
-				classes[dev->devno] = ATA_DEV_NONE;
-		}
-
-		/* record current link speed */
-		if (sata_scr_read(link, SCR_STATUS, &sstatus) == 0)
-			link->sata_spd = (sstatus >> 4) & 0xf;
-
-		if (postreset)
-			postreset(link, classes);
-
-		/* reset successful, schedule revalidation */
-		ata_eh_done(link, NULL, ehc->i.action & ATA_EH_RESET_MASK);
-		ehc->i.action |= ATA_EH_REVALIDATE;
-	}
+	rc = 0;
  out:
 	/* clear hotplug flag */
 	ehc->i.flags &= ~ATA_EHI_HOTPLUGGED;

@@ -2246,6 +2224,28 @@ int ata_eh_reset(struct ata_link *link, int classify,
 	spin_unlock_irqrestore(ap->lock, flags);
 
 	return rc;
+
+ fail:
+	if (rc == -ERESTART || try >= max_tries)
+		goto out;
+
+	now = jiffies;
+	if (time_before(now, deadline)) {
+		unsigned long delta = deadline - now;
+
+		ata_link_printk(link, KERN_WARNING, "reset failed "
+				"(errno=%d), retrying in %u secs\n",
+				rc, (jiffies_to_msecs(delta) + 999) / 1000);
+
+		while (delta)
+			delta = schedule_timeout_uninterruptible(delta);
+	}
+
+	if (rc == -EPIPE || try == max_tries - 1)
+		sata_down_spd_limit(link);
+	if (hardreset)
+		reset = hardreset;
+	goto retry;
 }
 
 static int ata_eh_revalidate_and_attach(struct ata_link *link,

Signature · 1 line

1.5.2.4

Tejun Heo

author

2007-10-31 01:33 UTC #4 lore raw

PATCH 3/6 libata: more robust reset failure handling

In reply to Tejun Heo

Reset failure is a critical error. It results in disabling the link requiring user intervention to re-enable it. Make reset failure handling more robust such that libata EH doesn't give up too early.

* Temporary glitches during hardreset may lead to classification failure when there's no softreset available. Retry instead of giving up.

* Initial softreset or follow up softreset may fail classification. Move classification error handling block out of followup softreset block such that both cases are handled and retry instead of giving up. Also, on the last try, give ATA class a blind shot.

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

---
 drivers/ata/libata-eh.c |   25 ++++++++++++++++---------
 1 files changed, 16 insertions(+), 9 deletions(-)

drivers/ata/libata-eh.c +16 -9

index e0de689..10af1a8 100644
@@ -2164,25 +2164,32 @@ int ata_eh_reset(struct ata_link *link, int classify,
 					"follow-up softreset required "
 					"but no softreset avaliable\n");
 			rc = -EINVAL;
-			goto out;
+			goto fail;
 		}
 
 		ata_eh_about_to_do(link, NULL, ATA_EH_RESET_MASK);
 		rc = ata_do_reset(link, reset, classes, deadline);
-
-		if (rc == 0 && classify && classes[0] == ATA_DEV_UNKNOWN &&
-		    !(lflags & ATA_LFLAG_ASSUME_CLASS)) {
-			ata_link_printk(link, KERN_ERR,
-					"classification failed\n");
-			rc = -EINVAL;
-			goto out;
-		}
 	}
 
 	/* -EAGAIN can happen if we skipped followup SRST */
 	if (rc && rc != -EAGAIN)
 		goto fail;
 
+	/* was classification successful? */
+	if (classify && classes[0] == ATA_DEV_UNKNOWN &&
+	    !(lflags & ATA_LFLAG_ASSUME_CLASS)) {
+		if (try < max_tries) {
+			ata_link_printk(link, KERN_WARNING,
+					"classification failed\n");
+			rc = -EINVAL;
+			goto fail;
+		}
+
+		ata_link_printk(link, KERN_WARNING,
+				"classfication failed, assuming ATA\n");
+		lflags |= ATA_LFLAG_ASSUME_ATA;
+	}
+
 	ata_link_for_each_dev(dev, link) {
 		/* After the reset, the device state is PIO 0 and the
 		 * controller state is undefined.  Reset also wakes up

Signature · 1 line

1.5.2.4

Tejun Heo

author

2007-10-31 01:33 UTC #5 lore raw

PATCH 4/6 libata: consider errors not associated with commands for speed down

In reply to Tejun Heo

libata EH used to ignore errors not associated with commands when determining whether speed down is necessary or not. This leads to the following problems.

* Errors not associated with commands can occur indefinitely without libata EH taking corrective actions.

* Upstream link errors don't trigger speed down when PMP is attached to it and commands issued to downstream device trigger errors on the upstream link.

This patch makes ata_eh_link_autopsy() consider errors not associated with command for speed down.

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

---
 drivers/ata/libata-eh.c |   19 +++++++++++++------
 1 files changed, 13 insertions(+), 6 deletions(-)

drivers/ata/libata-eh.c +13 -6

index 10af1a8..ded44bd 100644
@@ -1747,6 +1747,7 @@ static void ata_eh_link_autopsy(struct ata_link *link)
 {
 	struct ata_port *ap = link->ap;
 	struct ata_eh_context *ehc = &link->eh_context;
+	struct ata_device *dev;
 	unsigned int all_err_mask = 0;
 	int tag, is_io = 0;
 	u32 serror;

@@ -1819,18 +1820,24 @@ static void ata_eh_link_autopsy(struct ata_link *link)
 	else if (all_err_mask)
 		ehc->i.action |= ATA_EH_REVALIDATE;
 
-	/* if we have offending qcs and the associated failed device */
+	/* If we have offending qcs and the associated failed device,
+	 * perform per-dev EH action only on the offending device.
+	 */
 	if (ehc->i.dev) {
-		/* speed down */
-		ehc->i.action |= ata_eh_speed_down(ehc->i.dev, is_io,
-						   all_err_mask);
-
-		/* perform per-dev EH action only on the offending device */
 		ehc->i.dev_action[ehc->i.dev->devno] |=
 			ehc->i.action & ATA_EH_PERDEV_MASK;
 		ehc->i.action &= ~ATA_EH_PERDEV_MASK;
 	}
 
+	/* consider speeding down */
+	dev = ehc->i.dev;
+	if (!dev && ata_link_max_devices(link) == 1 &&
+	    ata_dev_enabled(link->device))
+		dev = link->device;
+
+	if (dev)
+		ehc->i.action |= ata_eh_speed_down(dev, is_io, all_err_mask);
+
 	DPRINTK("EXIT\n");
 }

Signature · 1 line

1.5.2.4

Tejun Heo

author

2007-10-31 01:28 UTC #6 lore raw

PATCH 5/6 libata: request PHY speed configuration on SControl access failure

In reply to Tejun Heo

In sata_set_spd_needed(), if SControl read failed, it returned 0 and skipped PHY speed configuration. However, if SControl access fails, it's far more logical to request PHY speed configuration. Reverse the logic.

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

---
 drivers/ata/libata-core.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

drivers/ata/libata-core.c +1 -1

index 8ee56e5..fd46606 100644
@@ -2578,7 +2578,7 @@ int sata_set_spd_needed(struct ata_link *link)
 	u32 scontrol;
 
 	if (sata_scr_read(link, SCR_CONTROL, &scontrol))
-		return 0;
+		return 1;
 
 	return __sata_set_spd_needed(link, &scontrol);
 }

Signature · 1 line

1.5.2.4

Tejun Heo

author

2007-10-31 01:28 UTC #7 lore raw

PATCH 6/6 libata: don't configure downstream links faster than the upstream link

In reply to Tejun Heo

There's nothing to be gained by configuring downstream links faster than the upstream link and such configurations cause problems on certain PMPs. Limit downstream link speed by the upstream link speed.

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

---
 drivers/ata/libata-core.c |   22 ++++++++++++++++------
 1 files changed, 16 insertions(+), 6 deletions(-)

drivers/ata/libata-core.c +16 -6

index fd46606..9b6e7b8 100644
@@ -2545,17 +2545,27 @@ int sata_down_spd_limit(struct ata_link *link)
 
 static int __sata_set_spd_needed(struct ata_link *link, u32 *scontrol)
 {
-	u32 spd, limit;
+	struct ata_link *host_link = &link->ap->link;
+	u32 limit, target, spd;
 
-	if (link->sata_spd_limit == UINT_MAX)
-		limit = 0;
+	limit = link->sata_spd_limit;
+
+	/* Don't configure downstream link faster than upstream link.
+	 * It doesn't speed up anything and some PMPs choke on such
+	 * configuration.
+	 */
+	if (!ata_is_host_link(link) && host_link->sata_spd)
+		limit &= (1 << host_link->sata_spd) - 1;
+
+	if (limit == UINT_MAX)
+		target = 0;
 	else
-		limit = fls(link->sata_spd_limit);
+		target = fls(limit);
 
 	spd = (*scontrol >> 4) & 0xf;
-	*scontrol = (*scontrol & ~0xf0) | ((limit & 0xf) << 4);
+	*scontrol = (*scontrol & ~0xf0) | ((target & 0xf) << 4);
 
-	return spd != limit;
+	return spd != target;
 }
 
 /**

Signature · 1 line

1.5.2.4