Skip to contentSkip to conversation

libata: improve timeout handling for EH commands

PATCH In review Tejun Heo 5 participants 20 messages May 19, 2008 – Nov 11, 2008 libata

lore
Share & tools
lore
author
#1 lore raw

PATCH 1/5 libata: kill unused constants

In reply to Tejun Heo

Kill a few unused constants.

Signed-off-by Tejun Heo <[email protected]>
---
 include/linux/libata.h |    3 ---
 1 files changed, 0 insertions(+), 3 deletions(-)
include/linux/libata.h +0 -3
index 95a2000..989088e 100644
@@ -111,13 +111,10 @@ enum {
 	/* various global constants */
 	LIBATA_MAX_PRD		= ATA_MAX_PRD / 2,
 	LIBATA_DUMB_MAX_PRD	= ATA_MAX_PRD / 4,	/* Worst case */
-	ATA_MAX_PORTS		= 8,
 	ATA_DEF_QUEUE		= 1,
 	/* tag ATA_MAX_QUEUE - 1 is reserved for internal commands */
 	ATA_MAX_QUEUE		= 32,
 	ATA_TAG_INTERNAL	= ATA_MAX_QUEUE - 1,
-	ATA_MAX_BUS		= 2,
-	ATA_DEF_BUSY_WAIT	= 10000,
 	ATA_SHORT_PAUSE		= (HZ >> 6) + 1,
 
 	ATAPI_MAX_DRAIN		= 16 << 10,
Signature · 1 line
1.5.2.4

Tejun Heo

author
#0 lore raw

PATCH 1/1 libata: improve timeout handling for EH commands

Hello,

[S]ATA has lots of legacy and not in a too healthy way. The desparate efforts to keep everything compatible made mundane things quite challenging. Probing definitely is one of them. We now span from ancient CF devices to port multipliers and quirky ones are easy to find all along the spectrum.

For example, according to the spec reset protocol and the actual implementations, there's no defined way to wait for the initial D2H Reg FIS after hardresetting a fan-out port. The problem is that the D2H Reg FIS can arrive during later steps of probing is in progress and a rogue FIS at the right time can easily make the controller fail or time out pending operations.

Another problem of the long spectrum is that we can't choose the one correct value as timeout for operations. A timeout value which can cover all the devices is too long and makes EH painfully slow when something goes wrong. Choosing shorter timeout means the ancient and weirdos won't work.

So, we have to resort to smart self adjusting timeouts. Early quickies to detect transient failures quickly and later long ones to cover odd devices. Deadline driven ata_eh_reset() was a step in that direction and this patchset, in addition to other timeout related improvements, applies it to the rest of EH commands.

This patchset contains the following patches.

01-libata-kill-unused-constants.patch 02-libata-consistently-use-msecs-for-time-durations.patch 03-libata-improve-EH-retry-delay-handling.patch 04-libata-use-ULONG_MAX-to-terminate-reset-timeout-tab.patch 05-libata-improve-EH-internal-command-timeout-handling.patch

#01, #02 and #04 are preparations. #02 makes liata use msecs consistently for time durations. #03 makes inter-try or inter-reset delays smarter and allows libata EH to delay less between tries without sacrificing robustness. #05 implements per command class stepped timeouts for EH commands, so that the first IDENTIFY try fails quickly but later on the device can take full 30secs to think about it while restricting maximum timeout for SET_FEATURES to 10secs.

This patchset makes whole EH behave much more swiftly and timeout cases bearable. :-)

This patchset is on top of

#upstream-fixes (005b1f7495e812b99b73de5adbc73afd7a1cbcaf)
+ [1] hotplug-fixes patchset

and available as git tree

http://git.kernel.org/?p=linux/kernel/git/tj/libata-dev.git;a=shortlog;h=improve-timeout
git://git.kernel.org/pub/scm/linux/kernel/git/tj/libata-dev.git improve-timeout

 drivers/ata/libata-core.c |   56 +++++++-----
 drivers/ata/libata-eh.c   |  200 +++++++++++++++++++++++++++++++++++++------
 drivers/ata/libata-pmp.c  |   13 --
 drivers/ata/libata-sff.c  |   15 +--
 drivers/ata/libata.h      |    2 
 drivers/ata/pata_bf54x.c  |    6 -
 drivers/ata/pata_scc.c    |    2 
 include/linux/libata.h    |   37 +++++---
 8 files changed, 234 insertions(+), 97 deletions(-)

Thanks.

--
tejun

[1] http://thread.gmane.org/gmane.linux.ide/31572

author
#7 lore raw

PATCH 3/5 libata: improve EH retry delay handling

EH retries were delayed by 5 seconds to ensure that resets don't occur back-to-back. However, this 5 second delay is superflous or excessive in many cases. For example, after IDENTIFY times out, there's no reason to wait five more seconds before retrying.

This patch adds ehc->last_reset timestamp and record the timestamp for the last reset trial or success and uses it to space resets by ATA_EH_RESET_COOL_DOWN which is 5 secs and removes unconditional 5 sec sleeps.

As this change makes inter-try waits often shorter and they're redundant in nature, this patch also removes the "retrying..." messages.

While at it, convert explicit rounding up division to DIV_ROUND_UP().

This change speeds up EH in many cases w/o sacrificing robustness.

Signed-off-by Tejun Heo <[email protected]>
---
 drivers/ata/libata-eh.c  |   38 ++++++++++++++++++++------------------
 drivers/ata/libata-pmp.c |   10 ----------
 include/linux/libata.h   |    2 ++
 3 files changed, 22 insertions(+), 28 deletions(-)
drivers/ata/libata-eh.c +20 -18
index 08dd07f..5b5ae63 100644
@@ -67,6 +67,9 @@ enum {
 	ATA_ECAT_DUBIOUS_UNK_DEV	= 7,
 	ATA_ECAT_NR			= 8,
 
+	/* always put at least this amount of time between resets */
+	ATA_EH_RESET_COOL_DOWN		=  5000,
+
 	/* Waiting in ->prereset can never be reliable.  It's
 	 * sometimes nice to wait there but it can't be depended upon;
 	 * otherwise, we wouldn't be resetting.  Just give it enough

@@ -485,6 +488,9 @@ void ata_scsi_error(struct Scsi_Host *host)
 				if (ata_ncq_enabled(dev))
 					ehc->saved_ncq_enabled |= 1 << devno;
 			}
+
+			/* set last reset timestamp to some time in the past */
+			ehc->last_reset = jiffies - 60 * HZ;
 		}
 
 		ap->pflags |= ATA_PFLAG_EH_IN_PROGRESS;

@@ -2088,11 +2094,17 @@ int ata_eh_reset(struct ata_link *link, int classify,
 	/*
 	 * Prepare to reset
 	 */
+	now = jiffies;
+	deadline = ata_deadline(ehc->last_reset, ATA_EH_RESET_COOL_DOWN);
+	if (time_before(now, deadline))
+		schedule_timeout_uninterruptible(deadline - now);
+
 	spin_lock_irqsave(ap->lock, flags);
 	ap->pflags |= ATA_PFLAG_RESETTING;
 	spin_unlock_irqrestore(ap->lock, flags);
 
 	ata_eh_about_to_do(link, NULL, ATA_EH_RESET);
+	ehc->last_reset = jiffies;
 
 	ata_link_for_each_dev(dev, link) {
 		/* If we issue an SRST then an ATA drive (not ATAPI)

@@ -2158,6 +2170,7 @@ int ata_eh_reset(struct ata_link *link, int classify,
 	/*
 	 * Perform reset
 	 */
+	ehc->last_reset = jiffies;
 	if (ata_is_host_link(link))
 		ata_eh_freeze_port(ap);
 

@@ -2278,6 +2291,7 @@ int ata_eh_reset(struct ata_link *link, int classify,
 
 	/* reset successful, schedule revalidation */
 	ata_eh_done(link, NULL, ATA_EH_RESET);
+	ehc->last_reset = jiffies;
 	ehc->i.action |= ATA_EH_REVALIDATE;
 
 	rc = 0;

@@ -2304,9 +2318,9 @@ int ata_eh_reset(struct ata_link *link, int classify,
 	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_printk(link, KERN_WARNING,
+			"reset failed (errno=%d), retrying in %u secs\n",
+			rc, DIV_ROUND_UP(jiffies_to_msecs(delta), 1000));
 
 		while (delta)
 			delta = schedule_timeout_uninterruptible(delta);

@@ -2623,7 +2637,7 @@ int ata_eh_recover(struct ata_port *ap, ata_prereset_fn_t prereset,
 {
 	struct ata_link *link;
 	struct ata_device *dev;
-	int nr_failed_devs, nr_disabled_devs;
+	int nr_failed_devs;
 	int rc;
 	unsigned long flags;
 

@@ -2666,7 +2680,6 @@ int ata_eh_recover(struct ata_port *ap, ata_prereset_fn_t prereset,
  retry:
 	rc = 0;
 	nr_failed_devs = 0;
-	nr_disabled_devs = 0;
 
 	/* if UNLOADING, finish immediately */
 	if (ap->pflags & ATA_PFLAG_UNLOADING)

@@ -2733,8 +2746,7 @@ int ata_eh_recover(struct ata_port *ap, ata_prereset_fn_t prereset,
 
 dev_fail:
 		nr_failed_devs++;
-		if (ata_eh_handle_dev_fail(dev, rc))
-			nr_disabled_devs++;
+		ata_eh_handle_dev_fail(dev, rc);
 
 		if (ap->pflags & ATA_PFLAG_FROZEN) {
 			/* PMP reset requires working host port.

@@ -2746,18 +2758,8 @@ dev_fail:
 		}
 	}
 
-	if (nr_failed_devs) {
-		if (nr_failed_devs != nr_disabled_devs) {
-			ata_port_printk(ap, KERN_WARNING, "failed to recover "
-					"some devices, retrying in 5 secs\n");
-			ssleep(5);
-		} else {
-			/* no device left to recover, repeat fast */
-			msleep(500);
-		}
-
+	if (nr_failed_devs)
 		goto retry;
-	}
 
  out:
 	if (rc && r_failed_link)
drivers/ata/libata-pmp.c +0 -10
index 3a2bce0..3374ec5 100644
@@ -724,19 +724,12 @@ static int sata_pmp_eh_recover_pmp(struct ata_port *ap,
 		}
 
 		if (tries) {
-			int sleep = ehc->i.flags & ATA_EHI_DID_RESET;
-
 			/* consecutive revalidation failures? speed down */
 			if (reval_failed)
 				sata_down_spd_limit(link);
 			else
 				reval_failed = 1;
 
-			ata_dev_printk(dev, KERN_WARNING,
-				       "retrying reset%s\n",
-				       sleep ? " in 5 secs" : "");
-			if (sleep)
-				ssleep(5);
 			ehc->i.action |= ATA_EH_RESET;
 			goto retry;
 		} else {

@@ -988,10 +981,7 @@ static int sata_pmp_eh_recover(struct ata_port *ap)
 		goto retry;
 
 	if (--pmp_tries) {
-		ata_port_printk(ap, KERN_WARNING,
-				"failed to recover PMP, retrying in 5 secs\n");
 		pmp_ehc->i.action |= ATA_EH_RESET;
-		ssleep(5);
 		goto retry;
 	}
include/linux/libata.h +2 -0
index 4bbe955..4fc9ab0 100644
@@ -602,6 +602,8 @@ struct ata_eh_context {
 	unsigned int		did_probe_mask;
 	unsigned int		saved_ncq_enabled;
 	u8			saved_xfer_mode[ATA_MAX_DEVICES];
+	/* timestamp for the last reset attempt or success */
+	unsigned long		last_reset;
 };
 
 struct ata_acpi_drive
Signature · 1 line
1.5.2.4
author
#3 lore raw

PATCH 2/5 libata: consistently use msecs for time durations

In reply to Tejun Heo

libata has been using mix of jiffies and msecs for time druations. This is getting confusing. As writing sub HZ values in jiffies is PITA and msecs_to_jiffies() can't be used as initializer, unify unit for all time durations to msecs. So, durations are in msecs and deadlines are in jiffies. ata_deadline() is added to compute deadline from a start time and duration in msecs.

While at it, drop now superflous _msec suffix from arguments and rename @timeout to @deadline if it represents a fixed point in time rather than duration.

Signed-off-by Tejun Heo <[email protected]>
---
 drivers/ata/libata-core.c |   44 +++++++++++++++++++++-----------------------
 drivers/ata/libata-eh.c   |   33 +++++++++++++++++----------------
 drivers/ata/libata-pmp.c  |    3 ++-
 drivers/ata/libata-sff.c  |   15 ++++++++-------
 drivers/ata/pata_bf54x.c  |    6 +++---
 drivers/ata/pata_scc.c    |    2 +-
 include/linux/libata.h    |   26 ++++++++++++++++----------
 7 files changed, 68 insertions(+), 61 deletions(-)
drivers/ata/libata-core.c +21 -23
index 3c89f20..eb9dfbc 100644
@@ -54,7 +54,6 @@
 #include <linux/completion.h>
 #include <linux/suspend.h>
 #include <linux/workqueue.h>
-#include <linux/jiffies.h>
 #include <linux/scatterlist.h>
 #include <linux/io.h>
 #include <scsi/scsi.h>

@@ -145,7 +144,7 @@ static int libata_dma_mask = ATA_DMA_MASK_ATA|ATA_DMA_MASK_ATAPI|ATA_DMA_MASK_CF
 module_param_named(dma, libata_dma_mask, int, 0444);
 MODULE_PARM_DESC(dma, "DMA enable/disable (0x1==ATA, 0x2==ATAPI, 0x4==CF)");
 
-static int ata_probe_timeout = ATA_TMOUT_INTERNAL / HZ;
+static int ata_probe_timeout = ATA_TMOUT_INTERNAL / 1000;
 module_param(ata_probe_timeout, int, 0444);
 MODULE_PARM_DESC(ata_probe_timeout, "Set ATA probing timeout (seconds)");
 

@@ -1533,7 +1532,7 @@ unsigned long ata_id_xfermask(const u16 *id)
  *	@ap: The ata_port to queue port_task for
  *	@fn: workqueue function to be scheduled
  *	@data: data for @fn to use
- *	@delay: delay time for workqueue function
+ *	@delay: delay time in msecs for workqueue function
  *
  *	Schedule @fn(@data) for execution after @delay jiffies using
  *	port_task.  There is one port_task per port and it's the

@@ -1552,7 +1551,7 @@ void ata_pio_queue_task(struct ata_port *ap, void *data, unsigned long delay)
 	ap->port_task_data = data;
 
 	/* may fail if ata_port_flush_task() in progress */
-	queue_delayed_work(ata_wq, &ap->port_task, delay);
+	queue_delayed_work(ata_wq, &ap->port_task, msecs_to_jiffies(delay));
 }
 
 /**

@@ -1685,7 +1684,7 @@ unsigned ata_exec_internal_sg(struct ata_device *dev,
 	spin_unlock_irqrestore(ap->lock, flags);
 
 	if (!timeout)
-		timeout = ata_probe_timeout * 1000 / HZ;
+		timeout = ata_probe_timeout * 1000;
 
 	rc = wait_for_completion_timeout(&wait, msecs_to_jiffies(timeout));
 

@@ -3319,7 +3318,7 @@ int ata_wait_ready(struct ata_link *link, unsigned long deadline,
 		   int (*check_ready)(struct ata_link *link))
 {
 	unsigned long start = jiffies;
-	unsigned long nodev_deadline = start + ATA_TMOUT_FF_WAIT;
+	unsigned long nodev_deadline = ata_deadline(start, ATA_TMOUT_FF_WAIT);
 	int warned = 0;
 
 	if (time_after(nodev_deadline, deadline))

@@ -3387,7 +3386,7 @@ int ata_wait_ready(struct ata_link *link, unsigned long deadline,
 int ata_wait_after_reset(struct ata_link *link, unsigned long deadline,
 				int (*check_ready)(struct ata_link *link))
 {
-	msleep(ATA_WAIT_AFTER_RESET_MSECS);
+	msleep(ATA_WAIT_AFTER_RESET);
 
 	return ata_wait_ready(link, deadline, check_ready);
 }

@@ -3417,13 +3416,13 @@ int ata_wait_after_reset(struct ata_link *link, unsigned long deadline,
 int sata_link_debounce(struct ata_link *link, const unsigned long *params,
 		       unsigned long deadline)
 {
-	unsigned long interval_msec = params[0];
-	unsigned long duration = msecs_to_jiffies(params[1]);
+	unsigned long interval = params[0];
+	unsigned long duration = params[1];
 	unsigned long last_jiffies, t;
 	u32 last, cur;
 	int rc;
 
-	t = jiffies + msecs_to_jiffies(params[2]);
+	t = ata_deadline(jiffies, params[2]);
 	if (time_before(t, deadline))
 		deadline = t;
 

@@ -3435,7 +3434,7 @@ int sata_link_debounce(struct ata_link *link, const unsigned long *params,
 	last_jiffies = jiffies;
 
 	while (1) {
-		msleep(interval_msec);
+		msleep(interval);
 		if ((rc = sata_scr_read(link, SCR_STATUS, &cur)))
 			return rc;
 		cur &= 0xf;

@@ -3444,7 +3443,8 @@ int sata_link_debounce(struct ata_link *link, const unsigned long *params,
 		if (cur == last) {
 			if (cur == 1 && time_before(jiffies, deadline))
 				continue;
-			if (time_after(jiffies, last_jiffies + duration))
+			if (time_after(jiffies,
+				       ata_deadline(last_jiffies, duration)))
 				return 0;
 			continue;
 		}

@@ -3636,7 +3636,8 @@ int sata_link_hardreset(struct ata_link *link, const unsigned long *timing,
 		if (check_ready) {
 			unsigned long pmp_deadline;
 
-			pmp_deadline = jiffies + ATA_TMOUT_PMP_SRST_WAIT;
+			pmp_deadline = ata_deadline(jiffies,
+						    ATA_TMOUT_PMP_SRST_WAIT);
 			if (time_after(pmp_deadline, deadline))
 				pmp_deadline = deadline;
 			ata_wait_ready(link, pmp_deadline, check_ready);

@@ -6073,8 +6074,6 @@ static void __init ata_parse_force_param(void)
 
 static int __init ata_init(void)
 {
-	ata_probe_timeout *= HZ;
-
 	ata_parse_force_param();
 
 	ata_wq = create_workqueue("ata");

@@ -6127,8 +6126,8 @@ int ata_ratelimit(void)
  *	@reg: IO-mapped register
  *	@mask: Mask to apply to read register value
  *	@val: Wait condition
- *	@interval_msec: polling interval in milliseconds
- *	@timeout_msec: timeout in milliseconds
+ *	@interval: polling interval in milliseconds
+ *	@timeout: timeout in milliseconds
  *
  *	Waiting for some bits of register to change is a common
  *	operation for ATA controllers.  This function reads 32bit LE

@@ -6146,10 +6145,9 @@ int ata_ratelimit(void)
  *	The final register value.
  */
 u32 ata_wait_register(void __iomem *reg, u32 mask, u32 val,
-		      unsigned long interval_msec,
-		      unsigned long timeout_msec)
+		      unsigned long interval, unsigned long timeout)
 {
-	unsigned long timeout;
+	unsigned long deadline;
 	u32 tmp;
 
 	tmp = ioread32(reg);

@@ -6158,10 +6156,10 @@ u32 ata_wait_register(void __iomem *reg, u32 mask, u32 val,
 	 * preceding writes reach the controller before starting to
 	 * eat away the timeout.
 	 */
-	timeout = jiffies + (timeout_msec * HZ) / 1000;
+	deadline = ata_deadline(jiffies, timeout);
 
-	while ((tmp & mask) == val && time_before(jiffies, timeout)) {
-		msleep(interval_msec);
+	while ((tmp & mask) == val && time_before(jiffies, deadline)) {
+		msleep(interval);
 		tmp = ioread32(reg);
 	}
drivers/ata/libata-eh.c +17 -16
index 7894d83..08dd07f 100644
@@ -66,15 +66,14 @@ enum {
 	ATA_ECAT_DUBIOUS_TOUT_HSM	= 6,
 	ATA_ECAT_DUBIOUS_UNK_DEV	= 7,
 	ATA_ECAT_NR			= 8,
-};
 
-/* Waiting in ->prereset can never be reliable.  It's sometimes nice
- * to wait there but it can't be depended upon; otherwise, we wouldn't
- * be resetting.  Just give it enough time for most drives to spin up.
- */
-enum {
-	ATA_EH_PRERESET_TIMEOUT		= 10 * HZ,
-	ATA_EH_FASTDRAIN_INTERVAL	= 3 * HZ,
+	/* Waiting in ->prereset can never be reliable.  It's
+	 * sometimes nice to wait there but it can't be depended upon;
+	 * otherwise, we wouldn't be resetting.  Just give it enough
+	 * time for most drives to spin up.
+	 */
+	ATA_EH_PRERESET_TIMEOUT		= 10000,
+	ATA_EH_FASTDRAIN_INTERVAL	=  3000,
 };
 
 /* The following table determines how we sequence resets.  Each entry

@@ -84,10 +83,10 @@ enum {
  * are mostly for error handling, hotplug and retarded devices.
  */
 static const unsigned long ata_eh_reset_timeouts[] = {
-	10 * HZ,	/* most drives spin up by 10sec */
-	10 * HZ,	/* > 99% working drives spin up before 20sec */
-	35 * HZ,	/* give > 30 secs of idleness for retarded devices */
-	5 * HZ,		/* and sweet one last chance */
+	10000,	/* most drives spin up by 10sec */
+	10000,	/* > 99% working drives spin up before 20sec */
+	35000,	/* give > 30 secs of idleness for retarded devices */
+	 5000,	/* and sweet one last chance */
 	/* > 1 min has elapsed, give up */
 };
 

@@ -641,7 +640,7 @@ void ata_eh_fastdrain_timerfn(unsigned long arg)
 		/* some qcs have finished, give it another chance */
 		ap->fastdrain_cnt = cnt;
 		ap->fastdrain_timer.expires =
-			jiffies + ATA_EH_FASTDRAIN_INTERVAL;
+			ata_deadline(jiffies, ATA_EH_FASTDRAIN_INTERVAL);
 		add_timer(&ap->fastdrain_timer);
 	}
 

@@ -681,7 +680,8 @@ static void ata_eh_set_pending(struct ata_port *ap, int fastdrain)
 
 	/* activate fast drain */
 	ap->fastdrain_cnt = cnt;
-	ap->fastdrain_timer.expires = jiffies + ATA_EH_FASTDRAIN_INTERVAL;
+	ap->fastdrain_timer.expires =
+		ata_deadline(jiffies, ATA_EH_FASTDRAIN_INTERVAL);
 	add_timer(&ap->fastdrain_timer);
 }
 

@@ -2125,7 +2125,8 @@ int ata_eh_reset(struct ata_link *link, int classify,
 	}
 
 	if (prereset) {
-		rc = prereset(link, jiffies + ATA_EH_PRERESET_TIMEOUT);
+		rc = prereset(link,
+			      ata_deadline(jiffies, ATA_EH_PRERESET_TIMEOUT));
 		if (rc) {
 			if (rc == -ENOENT) {
 				ata_link_printk(link, KERN_DEBUG,

@@ -2160,7 +2161,7 @@ int ata_eh_reset(struct ata_link *link, int classify,
 	if (ata_is_host_link(link))
 		ata_eh_freeze_port(ap);
 
-	deadline = jiffies + ata_eh_reset_timeouts[try++];
+	deadline = ata_deadline(jiffies, ata_eh_reset_timeouts[try++]);
 
 	if (reset) {
 		if (verbose)
drivers/ata/libata-pmp.c +2 -1
index 0f9386d..3a2bce0 100644
@@ -782,7 +782,8 @@ static int sata_pmp_eh_handle_disabled_links(struct ata_port *ap)
 		 * SError.N working.
 		 */
 		sata_link_hardreset(link, sata_deb_timing_normal,
-				jiffies + ATA_TMOUT_INTERNAL_QUICK, NULL, NULL);
+				ata_deadline(jiffies, ATA_TMOUT_INTERNAL_QUICK),
+				NULL, NULL);
 
 		/* unconditionally clear SError.N */
 		rc = sata_scr_write(link, SCR_ERROR, SERR_PHYRDY_CHG);
drivers/ata/libata-sff.c +8 -7
index 3c2d228..d25c129 100644
@@ -258,8 +258,8 @@ u8 ata_sff_altstatus(struct ata_port *ap)
 /**
  *	ata_sff_busy_sleep - sleep until BSY clears, or timeout
  *	@ap: port containing status register to be polled
- *	@tmout_pat: impatience timeout
- *	@tmout: overall timeout
+ *	@tmout_pat: impatience timeout in msecs
+ *	@tmout: overall timeout in msecs
  *
  *	Sleep until ATA Status register bit BSY clears,
  *	or a timeout occurs.

@@ -278,7 +278,7 @@ int ata_sff_busy_sleep(struct ata_port *ap,
 
 	status = ata_sff_busy_wait(ap, ATA_BUSY, 300);
 	timer_start = jiffies;
-	timeout = timer_start + tmout_pat;
+	timeout = ata_deadline(timer_start, tmout_pat);
 	while (status != 0xff && (status & ATA_BUSY) &&
 	       time_before(jiffies, timeout)) {
 		msleep(50);

@@ -290,7 +290,7 @@ int ata_sff_busy_sleep(struct ata_port *ap,
 				"port is slow to respond, please be patient "
 				"(Status 0x%x)\n", status);
 
-	timeout = timer_start + tmout;
+	timeout = ata_deadline(timer_start, tmout);
 	while (status != 0xff && (status & ATA_BUSY) &&
 	       time_before(jiffies, timeout)) {
 		msleep(50);

@@ -303,7 +303,7 @@ int ata_sff_busy_sleep(struct ata_port *ap,
 	if (status & ATA_BUSY) {
 		ata_port_printk(ap, KERN_ERR, "port failed to respond "
 				"(%lu secs, Status 0x%x)\n",
-				tmout / HZ, status);
+				DIV_ROUND_UP(tmout, 1000), status);
 		return -EBUSY;
 	}
 

@@ -1792,7 +1792,7 @@ int ata_sff_wait_after_reset(struct ata_link *link, unsigned int devmask,
 	unsigned int dev1 = devmask & (1 << 1);
 	int rc, ret = 0;
 
-	msleep(ATA_WAIT_AFTER_RESET_MSECS);
+	msleep(ATA_WAIT_AFTER_RESET);
 
 	/* always check readiness of the master device */
 	rc = ata_sff_wait_ready(link, deadline);

@@ -2275,7 +2275,8 @@ void ata_bus_reset(struct ata_port *ap)
 
 	/* issue bus reset */
 	if (ap->flags & ATA_FLAG_SRST) {
-		rc = ata_bus_softreset(ap, devmask, jiffies + 40 * HZ);
+		rc = ata_bus_softreset(ap, devmask,
+				       ata_deadline(jiffies, 40000));
 		if (rc && rc != -ENODEV)
 			goto err_out;
 	}
drivers/ata/pata_bf54x.c +3 -3
index 9ab8973..7b9be0e 100644
@@ -1008,7 +1008,7 @@ static void bfin_bus_post_reset(struct ata_port *ap, unsigned int devmask)
 	void __iomem *base = (void __iomem *)ap->ioaddr.ctl_addr;
 	unsigned int dev0 = devmask & (1 << 0);
 	unsigned int dev1 = devmask & (1 << 1);
-	unsigned long timeout;
+	unsigned long deadline;
 
 	/* if device 0 was found in ata_devchk, wait for its
 	 * BSY bit to clear

@@ -1019,7 +1019,7 @@ static void bfin_bus_post_reset(struct ata_port *ap, unsigned int devmask)
 	/* if device 1 was found in ata_devchk, wait for
 	 * register access, then wait for BSY to clear
 	 */
-	timeout = jiffies + ATA_TMOUT_BOOT;
+	deadline = ata_deadline(jiffies, ATA_TMOUT_BOOT);
 	while (dev1) {
 		u8 nsect, lbal;
 

@@ -1028,7 +1028,7 @@ static void bfin_bus_post_reset(struct ata_port *ap, unsigned int devmask)
 		lbal = read_atapi_register(base, ATA_REG_LBAL);
 		if ((nsect == 1) && (lbal == 1))
 			break;
-		if (time_after(jiffies, timeout)) {
+		if (time_after(jiffies, deadline)) {
 			dev1 = 0;
 			break;
 		}
drivers/ata/pata_scc.c +1 -1
index e965b25..a227dd3 100644
@@ -696,7 +696,7 @@ static void scc_bmdma_stop (struct ata_queued_cmd *qc)
 
 		if (reg & INTSTS_BMSINT) {
 			unsigned int classes;
-			unsigned long deadline = jiffies + ATA_TMOUT_BOOT;
+			unsigned long deadline = ata_deadline(jiffies, ATA_TMOUT_BOOT);
 			printk(KERN_WARNING "%s: Internal Bus Error\n", DRV_NAME);
 			out_be32(bmid_base + SCC_DMA_INTST, INTSTS_BMSINT);
 			/* TBD: SW reset */
include/linux/libata.h +16 -10
index 989088e..4bbe955 100644
@@ -27,6 +27,7 @@
 #define __LINUX_LIBATA_H__
 
 #include <linux/delay.h>
+#include <linux/jiffies.h>
 #include <linux/interrupt.h>
 #include <linux/dma-mapping.h>
 #include <linux/scatterlist.h>

@@ -115,7 +116,7 @@ enum {
 	/* tag ATA_MAX_QUEUE - 1 is reserved for internal commands */
 	ATA_MAX_QUEUE		= 32,
 	ATA_TAG_INTERNAL	= ATA_MAX_QUEUE - 1,
-	ATA_SHORT_PAUSE		= (HZ >> 6) + 1,
+	ATA_SHORT_PAUSE		= 16,
 
 	ATAPI_MAX_DRAIN		= 16 << 10,
 

@@ -234,17 +235,17 @@ enum {
 	/* bits 24:31 of host->flags are reserved for LLD specific flags */
 
 	/* various lengths of time */
-	ATA_TMOUT_BOOT		= 30 * HZ,	/* heuristic */
-	ATA_TMOUT_BOOT_QUICK	= 7 * HZ,	/* heuristic */
-	ATA_TMOUT_INTERNAL	= 30 * HZ,
-	ATA_TMOUT_INTERNAL_QUICK = 5 * HZ,
+	ATA_TMOUT_BOOT		= 30000,	/* heuristic */
+	ATA_TMOUT_BOOT_QUICK	=  7000,	/* heuristic */
+	ATA_TMOUT_INTERNAL	= 30000,
+	ATA_TMOUT_INTERNAL_QUICK = 5000,
 
 	/* FIXME: GoVault needs 2s but we can't afford that without
 	 * parallel probing.  800ms is enough for iVDR disk
 	 * HHD424020F7SV00.  Increase to 2secs when parallel probing
 	 * is in place.
 	 */
-	ATA_TMOUT_FF_WAIT	= 4 * HZ / 5,
+	ATA_TMOUT_FF_WAIT	=  800,
 
 	/* Spec mandates to wait for ">= 2ms" before checking status
 	 * after reset.  We wait 150ms, because that was the magic

@@ -256,14 +257,14 @@ enum {
 	 *
 	 * Old drivers/ide uses the 2mS rule and then waits for ready.
 	 */
-	ATA_WAIT_AFTER_RESET_MSECS = 150,
+	ATA_WAIT_AFTER_RESET	=  150,
 
 	/* If PMP is supported, we have to do follow-up SRST.  As some
 	 * PMPs don't send D2H Reg FIS after hardreset, LLDs are
 	 * advised to wait only for the following duration before
 	 * doing SRST.
 	 */
-	ATA_TMOUT_PMP_SRST_WAIT	= 1 * HZ,
+	ATA_TMOUT_PMP_SRST_WAIT	= 1000,
 
 	/* ATA bus states */
 	BUS_UNKNOWN		= 0,

@@ -897,8 +898,7 @@ extern void ata_host_resume(struct ata_host *host);
 #endif
 extern int ata_ratelimit(void);
 extern u32 ata_wait_register(void __iomem *reg, u32 mask, u32 val,
-			     unsigned long interval_msec,
-			     unsigned long timeout_msec);
+			     unsigned long interval, unsigned long timeout);
 extern int atapi_cmd_type(u8 opcode);
 extern void ata_tf_to_fis(const struct ata_taskfile *tf,
 			  u8 pmp, int is_cmd, u8 *fis);

@@ -1391,6 +1391,12 @@ static inline int ata_check_ready(u8 status)
 	return 0;
 }
 
+static inline unsigned long ata_deadline(unsigned long from_jiffies,
+					 unsigned long timeout_msecs)
+{
+	return from_jiffies + msecs_to_jiffies(timeout_msecs);
+}
+
 
 /**************************************************************************
  * PMP - drivers/ata/libata-pmp.c
Signature · 1 line
1.5.2.4
author
#10 lore raw

PATCH 4/5 libata: use ULONG_MAX to terminate reset timeout table

In reply to Tejun Heo

This doesn't introduce any functional changes. This is to make reset timeout table consistent with to-be-added command timeout tables.

Signed-off-by Tejun Heo <[email protected]>
---
 drivers/ata/libata-eh.c |    8 +++++---
 1 files changed, 5 insertions(+), 3 deletions(-)
drivers/ata/libata-eh.c +5 -3
index 5b5ae63..83d1451 100644
@@ -90,7 +90,7 @@ static const unsigned long ata_eh_reset_timeouts[] = {
 	10000,	/* > 99% working drives spin up before 20sec */
 	35000,	/* give > 30 secs of idleness for retarded devices */
 	 5000,	/* and sweet one last chance */
-	/* > 1 min has elapsed, give up */
+	ULONG_MAX, /* > 1 min has elapsed, give up */
 };
 
 static void __ata_port_freeze(struct ata_port *ap);

@@ -2077,13 +2077,12 @@ 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;
+	int max_tries = 0, try = 0;
 	struct ata_device *dev;
 	unsigned long deadline, now;
 	ata_reset_fn_t reset;

@@ -2094,6 +2093,9 @@ int ata_eh_reset(struct ata_link *link, int classify,
 	/*
 	 * Prepare to reset
 	 */
+	while (ata_eh_reset_timeouts[max_tries] != ULONG_MAX)
+		max_tries++;
+
 	now = jiffies;
 	deadline = ata_deadline(ehc->last_reset, ATA_EH_RESET_COOL_DOWN);
 	if (time_before(now, deadline))
Signature · 1 line
1.5.2.4
author
#11 lore raw

PATCH 5/5 libata: improve EH internal command timeout handling

In reply to Tejun Heo

ATA_TMOUT_INTERNAL which was 30secs were used for all internal commands which is way too long when something goes wrong. This patch implements command type based stepped timeouts. Different command types can use different timeouts and each command type can use different timeout values after timeouts.

ie. the initial timeout is set to a value which should cover most of the cases but not too long so that run away cases don't delay things too much. After the first try times out, the second try can use longer timeout and if that one times out too, it can go for full 30sec timeout.

IDENTIFYs use 5s - 10s - 30s timeout and all other commands use 5s - 10s timeouts.

This patch significantly cuts down the needed time to handle failure cases while still allowing libata to work with nut job devices through retries.

Signed-off-by Tejun Heo <[email protected]>
---
 drivers/ata/libata-core.c |   16 +++++-
 drivers/ata/libata-eh.c   |  121 ++++++++++++++++++++++++++++++++++++++++++++-
 drivers/ata/libata.h      |    2 +
 include/linux/libata.h    |    8 +++-
 4 files changed, 142 insertions(+), 5 deletions(-)
drivers/ata/libata-core.c +13 -3
index eb9dfbc..1a40d41 100644
@@ -144,7 +144,7 @@ static int libata_dma_mask = ATA_DMA_MASK_ATA|ATA_DMA_MASK_ATAPI|ATA_DMA_MASK_CF
 module_param_named(dma, libata_dma_mask, int, 0444);
 MODULE_PARM_DESC(dma, "DMA enable/disable (0x1==ATA, 0x2==ATAPI, 0x4==CF)");
 
-static int ata_probe_timeout = ATA_TMOUT_INTERNAL / 1000;
+static int ata_probe_timeout;
 module_param(ata_probe_timeout, int, 0444);
 MODULE_PARM_DESC(ata_probe_timeout, "Set ATA probing timeout (seconds)");
 

@@ -1611,6 +1611,7 @@ unsigned ata_exec_internal_sg(struct ata_device *dev,
 	struct ata_link *link = dev->link;
 	struct ata_port *ap = link->ap;
 	u8 command = tf->command;
+	int auto_timeout = 0;
 	struct ata_queued_cmd *qc;
 	unsigned int tag, preempted_tag;
 	u32 preempted_sactive, preempted_qc_active;

@@ -1683,8 +1684,14 @@ unsigned ata_exec_internal_sg(struct ata_device *dev,
 
 	spin_unlock_irqrestore(ap->lock, flags);
 
-	if (!timeout)
-		timeout = ata_probe_timeout * 1000;
+	if (!timeout) {
+		if (ata_probe_timeout)
+			timeout = ata_probe_timeout * 1000;
+		else {
+			timeout = ata_internal_cmd_timeout(dev, command);
+			auto_timeout = 1;
+		}
+	}
 
 	rc = wait_for_completion_timeout(&wait, msecs_to_jiffies(timeout));
 

@@ -1760,6 +1767,9 @@ unsigned ata_exec_internal_sg(struct ata_device *dev,
 
 	spin_unlock_irqrestore(ap->lock, flags);
 
+	if ((err_mask & AC_ERR_TIMEOUT) && auto_timeout)
+		ata_internal_cmd_timed_out(dev, command);
+
 	return err_mask;
 }
drivers/ata/libata-eh.c +120 -1

146 lines not on this page · raw message

drivers/ata/libata.h +2 -0
index 4514283..daf2b44 100644
@@ -151,6 +151,8 @@ extern void ata_scsi_dev_rescan(struct work_struct *work);
 extern int ata_bus_probe(struct ata_port *ap);
 
 /* libata-eh.c */
+extern unsigned long ata_internal_cmd_timeout(struct ata_device *dev, u8 cmd);
+extern void ata_internal_cmd_timed_out(struct ata_device *dev, u8 cmd);
 extern enum scsi_eh_timer_return ata_scsi_timed_out(struct scsi_cmnd *cmd);
 extern void ata_scsi_error(struct Scsi_Host *host);
 extern void ata_port_wait_eh(struct ata_port *ap);
include/linux/libata.h +7 -1
index 4fc9ab0..70cf9fd 100644
@@ -237,7 +237,6 @@ enum {
 	/* various lengths of time */
 	ATA_TMOUT_BOOT		= 30000,	/* heuristic */
 	ATA_TMOUT_BOOT_QUICK	=  7000,	/* heuristic */
-	ATA_TMOUT_INTERNAL	= 30000,
 	ATA_TMOUT_INTERNAL_QUICK = 5000,
 
 	/* FIXME: GoVault needs 2s but we can't afford that without

@@ -341,6 +340,11 @@ enum {
 
 	SATA_PMP_RW_TIMEOUT	= 3000,		/* PMP read/write timeout */
 
+	/* This should match the actual table size of
+	 * ata_eh_cmd_timeout_table in libata-eh.c.
+	 */
+	ATA_EH_CMD_TIMEOUT_TABLE_SIZE = 5,
+
 	/* Horkage types. May be set by libata or controller on drives
 	   (some horkage may be drive/controller pair dependant */
 

@@ -598,6 +602,8 @@ struct ata_eh_info {
 struct ata_eh_context {
 	struct ata_eh_info	i;
 	int			tries[ATA_MAX_DEVICES];
+	int			cmd_timeout_idx[ATA_MAX_DEVICES]
+					       [ATA_EH_CMD_TIMEOUT_TABLE_SIZE];
 	unsigned int		classes[ATA_MAX_DEVICES];
 	unsigned int		did_probe_mask;
 	unsigned int		saved_ncq_enabled;
Signature · 1 line
1.5.2.4
#8 lore raw

Re: [PATCH 3/5] libata: improve EH retry delay handling

In reply to Tejun Heo
Quoted 4 lines · On Tue, 20 May 2008 02:17:52 +0900 Tejun Heo <[email protected]> wrote:

EH retries were delayed by 5 seconds to ensure that resets don't occur back-to-back. However, this 5 second delay is superflous or excessive in many cases. For example, after IDENTIFY times out, there's no reason to wait five more seconds before retrying.

You'll crash some ATAPI CD-ROM drives without that delay - at least for PATA. As in power cycle required to get it back.

Perhaps we should check cable type here ?

Acked-by on v1 5/5
#12 lore raw

Re: [PATCH 5/5] libata: improve EH internal command timeout handling

In reply to Tejun Heo
Quoted 2 lines

IDENTIFYs use 5s - 10s - 30s timeout and all other commands use 5s - 10s timeouts.

Cache flushes need more but they now come via the libata-scsi code right ?

Quoted 6 lines

This patch significantly cuts down the needed time to handle failure cases while still allowing libata to work with nut job devices through retries.

Signed-off-by Tejun Heo <[email protected]>
Acked-by Alan Cox <[email protected]>
#13 lore raw

Re: [PATCH 5/5] libata: improve EH internal command timeout handling

Quoted 4 lines · Alan Cox
Quoted 2 lines

IDENTIFYs use 5s - 10s - 30s timeout and all other commands use 5s - 10s timeouts.

Cache flushes need more but they now come via the libata-scsi code right ?

via scsi layer via libata-scsi, yes. Kernel issues flush, SCSI translates to SYNC CACHE, libata-scsi translates to FLUSH CACHE.

We don't issue flushes outside of that code path (except SG_IO I suppose)

Jeff

#4 lore raw

Re: [PATCH 2/5] libata: consistently use msecs for time durations

In reply to Tejun Heo
Quoted 12 lines · Tejun Heo

libata has been using mix of jiffies and msecs for time druations. This is getting confusing. As writing sub HZ values in jiffies is PITA and msecs_to_jiffies() can't be used as initializer, unify unit for all time durations to msecs. So, durations are in msecs and deadlines are in jiffies. ata_deadline() is added to compute deadline from a start time and duration in msecs.

While at it, drop now superflous _msec suffix from arguments and rename @timeout to @deadline if it represents a fixed point in time rather than duration.

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

[...]

include/linux/libata.h static inline int ata_check_ready(u8 status) · line 1395

Quoted 2 lines
include/linux/libata.h +2 -0
+static inline unsigned long ata_deadline(unsigned long from_jiffies,
+					 unsigned long timeout_msecs)

							      ^^^^^^

Wouldn't that be against the newly adopted convention? Just a thought.

Regards,

Elias

author
#9 lore raw

Re: [PATCH 3/5] libata: improve EH retry delay handling

In reply to Alan Cox
Quoted 8 lines · Alan Cox
Quoted 4 lines · On Tue, 20 May 2008 02:17:52 +0900 Tejun Heo <[email protected]> wrote:

EH retries were delayed by 5 seconds to ensure that resets don't occur back-to-back. However, this 5 second delay is superflous or excessive in many cases. For example, after IDENTIFY times out, there's no reason to wait five more seconds before retrying.

Quoted 4 lines

You'll crash some ATAPI CD-ROM drives without that delay - at least for PATA. As in power cycle required to get it back.

Perhaps we should check cable type here ?

No, no, the five secs is still there.

e.g. Previously:

reset (fail) - 5s retry wait - reset
reset - IDENTIFY (30s timeout) - 5s retry wait - reset

Now:

reset (fail) - 5s retry wait - reset
reset - IDENTIFY (30s timeout) - reset

So, the inter-reset delay is never shorter than 5s.

Signature · 1 line
tejun
author
#5 lore raw

Re: [PATCH 2/5] libata: consistently use msecs for time durations

In reply to Elias Oltmanns
Quoted 12 lines · Elias Oltmanns
Quoted 11 lines · Tejun Heo

libata has been using mix of jiffies and msecs for time druations. This is getting confusing. As writing sub HZ values in jiffies is PITA and msecs_to_jiffies() can't be used as initializer, unify unit for all time durations to msecs. So, durations are in msecs and deadlines are in jiffies. ata_deadline() is added to compute deadline from a start time and duration in msecs.

While at it, drop now superflous _msec suffix from arguments and rename @timeout to @deadline if it represents a fixed point in time rather than duration.

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

[...]

include/linux/libata.h static inline int ata_check_ready(u8 status) · line 1395

Quoted 2 lines
include/linux/libata.h +2 -0
+static inline unsigned long ata_deadline(unsigned long from_jiffies,
+					 unsigned long timeout_msecs)

							      ^^^^^^

Wouldn't that be against the newly adopted convention? Just a thought.

Yeah, but as the function takes both jiffies and msecs, I thought it would be better to make it explicit. Hmmm... I can't really decide which way would be better.

Signature · 1 line
tejun
author
#14 lore raw

Re: [PATCH 5/5] libata: improve EH internal command timeout handling

In reply to Alan Cox
Quoted 5 lines · Alan Cox
Quoted 2 lines

IDENTIFYs use 5s - 10s - 30s timeout and all other commands use 5s - 10s timeouts.

Cache flushes need more but they now come via the libata-scsi code right ?

Quoted 7 lines
Quoted 5 lines

This patch significantly cuts down the needed time to handle failure cases while still allowing libata to work with nut job devices through retries.

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

Yeah, these are only for EH commands and for currently used commands 5s should be more than enough. If we ever need something which needs longer timeout, we can easily add it too.

Thanks.

Signature · 1 line
tejun
author
#15 lore raw
In reply to Tejun Heo
Quoted 7 lines · Tejun Heo

Hello,

[S]ATA has lots of legacy and not in a too healthy way. The desparate efforts to keep everything compatible made mundane things quite challenging. Probing definitely is one of them. We now span from ancient CF devices to port multipliers and quirky ones are easy to find all along the spectrum.

Ping.

Signature · 1 line
tejun
#16 lore raw
In reply to Tejun Heo
Quoted 43 lines · Tejun Heo

Hello,

[S]ATA has lots of legacy and not in a too healthy way. The desparate efforts to keep everything compatible made mundane things quite challenging. Probing definitely is one of them. We now span from ancient CF devices to port multipliers and quirky ones are easy to find all along the spectrum.

For example, according to the spec reset protocol and the actual implementations, there's no defined way to wait for the initial D2H Reg FIS after hardresetting a fan-out port. The problem is that the D2H Reg FIS can arrive during later steps of probing is in progress and a rogue FIS at the right time can easily make the controller fail or time out pending operations.

Another problem of the long spectrum is that we can't choose the one correct value as timeout for operations. A timeout value which can cover all the devices is too long and makes EH painfully slow when something goes wrong. Choosing shorter timeout means the ancient and weirdos won't work.

So, we have to resort to smart self adjusting timeouts. Early quickies to detect transient failures quickly and later long ones to cover odd devices. Deadline driven ata_eh_reset() was a step in that direction and this patchset, in addition to other timeout related improvements, applies it to the rest of EH commands.

This patchset contains the following patches.

01-libata-kill-unused-constants.patch 02-libata-consistently-use-msecs-for-time-durations.patch 03-libata-improve-EH-retry-delay-handling.patch 04-libata-use-ULONG_MAX-to-terminate-reset-timeout-tab.patch 05-libata-improve-EH-internal-command-timeout-handling.patch

#01, #02 and #04 are preparations. #02 makes liata use msecs consistently for time durations. #03 makes inter-try or inter-reset delays smarter and allows libata EH to delay less between tries without sacrificing robustness. #05 implements per command class stepped timeouts for EH commands, so that the first IDENTIFY try fails quickly but later on the device can take full 30secs to think about it while restricting maximum timeout for SET_FEATURES to 10secs.

Quoted 2 lines

This patchset makes whole EH behave much more swiftly and timeout cases bearable. :-)

ACK

for .27? Seems useful to .26, but also we are late in .26-rc and this is a lot of changes for -rc$LATE :)

author
#17 lore raw
Quoted 44 lines · Jeff Garzik
Quoted 43 lines · Tejun Heo

Hello,

[S]ATA has lots of legacy and not in a too healthy way. The desparate efforts to keep everything compatible made mundane things quite challenging. Probing definitely is one of them. We now span from ancient CF devices to port multipliers and quirky ones are easy to find all along the spectrum.

For example, according to the spec reset protocol and the actual implementations, there's no defined way to wait for the initial D2H Reg FIS after hardresetting a fan-out port. The problem is that the D2H Reg FIS can arrive during later steps of probing is in progress and a rogue FIS at the right time can easily make the controller fail or time out pending operations.

Another problem of the long spectrum is that we can't choose the one correct value as timeout for operations. A timeout value which can cover all the devices is too long and makes EH painfully slow when something goes wrong. Choosing shorter timeout means the ancient and weirdos won't work.

So, we have to resort to smart self adjusting timeouts. Early quickies to detect transient failures quickly and later long ones to cover odd devices. Deadline driven ata_eh_reset() was a step in that direction and this patchset, in addition to other timeout related improvements, applies it to the rest of EH commands.

This patchset contains the following patches.

01-libata-kill-unused-constants.patch 02-libata-consistently-use-msecs-for-time-durations.patch 03-libata-improve-EH-retry-delay-handling.patch 04-libata-use-ULONG_MAX-to-terminate-reset-timeout-tab.patch 05-libata-improve-EH-internal-command-timeout-handling.patch

#01, #02 and #04 are preparations. #02 makes liata use msecs consistently for time durations. #03 makes inter-try or inter-reset delays smarter and allows libata EH to delay less between tries without sacrificing robustness. #05 implements per command class stepped timeouts for EH commands, so that the first IDENTIFY try fails quickly but later on the device can take full 30secs to think about it while restricting maximum timeout for SET_FEATURES to 10secs.

Quoted 7 lines
Quoted 2 lines

This patchset makes whole EH behave much more swiftly and timeout cases bearable. :-)

ACK

for .27? Seems useful to .26, but also we are late in .26-rc and this is a lot of changes for -rc$LATE :)

Yes, for .27. It's too big a change for 26.

Thanks.

Signature · 1 line
tejun
Applied
#2 lore raw

Re: [PATCH 1/5] libata: kill unused constants

In reply to Tejun Heo
Quoted 13 lines · Tejun Heo

Kill a few unused constants.

Signed-off-by Tejun Heo <[email protected]>
---
 include/linux/libata.h |    3 ---
 1 files changed, 0 insertions(+), 3 deletions(-)
include/linux/libata.h +0 -0
index 95a2000..989088e 100644
@@ -111,13 +111,10 @@ enum {
 	/* various global constants */

include/linux/libata.h enum { · line 118

Quoted 10 lines
include/linux/libata.h +0 -3
 	LIBATA_MAX_PRD		= ATA_MAX_PRD / 2,
 	LIBATA_DUMB_MAX_PRD	= ATA_MAX_PRD / 4,	/* Worst case */
-	ATA_MAX_PORTS		= 8,
 	ATA_DEF_QUEUE		= 1,
 	/* tag ATA_MAX_QUEUE - 1 is reserved for internal commands */
 	ATA_MAX_QUEUE		= 32,
 	ATA_TAG_INTERNAL	= ATA_MAX_QUEUE - 1,
-	ATA_MAX_BUS		= 2,
-	ATA_DEF_BUSY_WAIT	= 10000,
 	ATA_SHORT_PAUSE		= (HZ >> 6) + 1,

applied

#6 lore raw

Re: [PATCH 2/5] libata: consistently use msecs for time durations

In reply to Tejun Heo
Quoted 12 lines · Tejun Heo

libata has been using mix of jiffies and msecs for time druations. This is getting confusing. As writing sub HZ values in jiffies is PITA and msecs_to_jiffies() can't be used as initializer, unify unit for all time durations to msecs. So, durations are in msecs and deadlines are in jiffies. ata_deadline() is added to compute deadline from a start time and duration in msecs.

While at it, drop now superflous _msec suffix from arguments and rename @timeout to @deadline if it represents a fixed point in time rather than duration.

Signed-off-by Tejun Heo <[email protected]>
Quoted 9 lines
---
 drivers/ata/libata-core.c |   44 +++++++++++++++++++++-----------------------
 drivers/ata/libata-eh.c   |   33 +++++++++++++++++----------------
 drivers/ata/libata-pmp.c  |    3 ++-
 drivers/ata/libata-sff.c  |   15 ++++++++-------
 drivers/ata/pata_bf54x.c  |    6 +++---
 drivers/ata/pata_scc.c    |    2 +-
 include/linux/libata.h    |   26 ++++++++++++++++----------
 7 files changed, 68 insertions(+), 61 deletions(-)

applied 2-5

#18 lore raw
In reply to Tejun Heo

Hi,
This patch is guilty for big delay when I boot my system with
marvell soc sata with 4140 pmp (without disks). I added print for the deadline and now variables in the ata_eh_reset.
here is the output:
ata2: SATA max UDMA/133 irq 29
sleep deadline -35455 now -29955
sleep done
ata1: SATA link down (SStatus 0 SControl 300)
sleep deadline -35419 now -29919
sleep done
ata2: SATA link up 3.0 Gbps (SStatus 123 SControl 300) ata2.15: Port Multiplier 1.1, 0x11ab:0x4140 r193, 5 ports, feat 0x1/0x1f
sleep deadline 500 now -29867

it seems that in may case the ehc->last_reset was not initialized.

saeed

Quoted 64 lines · Tejun Heo
Quoted 44 lines · Jeff Garzik
Quoted 43 lines · Tejun Heo

Hello,

[S]ATA has lots of legacy and not in a too healthy way. The desparate efforts to keep everything compatible made mundane things quite challenging. Probing definitely is one of them. We now span from ancient CF devices to port multipliers and quirky ones are easy to find all along the spectrum.

For example, according to the spec reset protocol and the actual implementations, there's no defined way to wait for the initial D2H Reg FIS after hardresetting a fan-out port. The problem is that the D2H Reg FIS can arrive during later steps of probing is in progress and a rogue FIS at the right time can easily make the controller fail or time out pending operations.

Another problem of the long spectrum is that we can't choose the one correct value as timeout for operations. A timeout value which can cover all the devices is too long and makes EH painfully slow when something goes wrong. Choosing shorter timeout means the ancient and weirdos won't work.

So, we have to resort to smart self adjusting timeouts. Early quickies to detect transient failures quickly and later long ones to cover odd devices. Deadline driven ata_eh_reset() was a step in that direction and this patchset, in addition to other timeout related improvements, applies it to the rest of EH commands.

This patchset contains the following patches.

01-libata-kill-unused-constants.patch 02-libata-consistently-use-msecs-for-time-durations.patch 03-libata-improve-EH-retry-delay-handling.patch 04-libata-use-ULONG_MAX-to-terminate-reset-timeout-tab.patch 05-libata-improve-EH-internal-command-timeout-handling.patch

#01, #02 and #04 are preparations. #02 makes liata use msecs consistently for time durations. #03 makes inter-try or inter-reset delays smarter and allows libata EH to delay less between tries without sacrificing robustness. #05 implements per command class stepped timeouts for EH commands, so that the first IDENTIFY try fails quickly but later on the device can take full 30secs to think about it while restricting maximum timeout for SET_FEATURES to 10secs.

Quoted 7 lines
Quoted 2 lines

This patchset makes whole EH behave much more swiftly and timeout cases bearable. :-)

ACK

for .27? Seems useful to .26, but also we are late in .26-rc and this is a lot of changes for -rc$LATE :)

Yes, for .27. It's too big a change for 26.

Thanks.

--
tejun
--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

author
#19 lore raw
Quoted 15 lines · saeed bishara

Hi,
This patch is guilty for big delay when I boot my system with
marvell soc sata with 4140 pmp (without disks). I added print for the deadline and now variables in the ata_eh_reset.
here is the output:
ata2: SATA max UDMA/133 irq 29
sleep deadline -35455 now -29955
sleep done
ata1: SATA link down (SStatus 0 SControl 300)
sleep deadline -35419 now -29919
sleep done
ata2: SATA link up 3.0 Gbps (SStatus 123 SControl 300) ata2.15: Port Multiplier 1.1, 0x11ab:0x4140 r193, 5 ports, feat 0x1/0x1f
sleep deadline 500 now -29867

Quoted 1 line

it seems that in may case the ehc->last_reset was not initialized.

Yeap, it didn't initialize last_reset for new pmp links. Patch to fix already has been posted and pending for upstream and -stable.

Thanks.

Signature · 1 line
tejun