[PATCHSET #upstream] libata: improve timeout handling for EH commands

[PATCHSET #upstream] libata: improve timeout handling for EH commands

[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.

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

[PATCH 1/5] libata: kill unused constants

[Tejun Heo]

Kill a few unused constants.

Signed-off-by: Tejun Heo <htejun@gmail.com>
---
 include/linux/libata.h |    3 ---
 1 files changed, 0 insertions(+), 3 deletions(-)

diff --git a/include/linux/libata.h b/include/linux/libata.h
index 95a2000..989088e 100644
--- a/include/linux/libata.h
+++ b/include/linux/libata.h
@@ -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,
-- 
1.5.2.4

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

[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 <htejun@gmail.com>
---
 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(-)

diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
index 3c89f20..eb9dfbc 100644
--- a/drivers/ata/libata-core.c
+++ b/drivers/ata/libata-core.c
@@ -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);
 	}
 
diff --git a/drivers/ata/libata-eh.c b/drivers/ata/libata-eh.c
index 7894d83..08dd07f 100644
--- a/drivers/ata/libata-eh.c
+++ b/drivers/ata/libata-eh.c
@@ -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)
diff --git a/drivers/ata/libata-pmp.c b/drivers/ata/libata-pmp.c
index 0f9386d..3a2bce0 100644
--- a/drivers/ata/libata-pmp.c
+++ b/drivers/ata/libata-pmp.c
@@ -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);
diff --git a/drivers/ata/libata-sff.c b/drivers/ata/libata-sff.c
index 3c2d228..d25c129 100644
--- a/drivers/ata/libata-sff.c
+++ b/drivers/ata/libata-sff.c
@@ -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;
 	}
diff --git a/drivers/ata/pata_bf54x.c b/drivers/ata/pata_bf54x.c
index 9ab8973..7b9be0e 100644
--- a/drivers/ata/pata_bf54x.c
+++ b/drivers/ata/pata_bf54x.c
@@ -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;
 		}
diff --git a/drivers/ata/pata_scc.c b/drivers/ata/pata_scc.c
index e965b25..a227dd3 100644
--- a/drivers/ata/pata_scc.c
+++ b/drivers/ata/pata_scc.c
@@ -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 */
diff --git a/include/linux/libata.h b/include/linux/libata.h
index 989088e..4bbe955 100644
--- a/include/linux/libata.h
+++ b/include/linux/libata.h
@@ -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
-- 
1.5.2.4

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

[Tejun Heo]

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 <htejun@gmail.com>
---
 drivers/ata/libata-eh.c  |   38 ++++++++++++++++++++------------------
 drivers/ata/libata-pmp.c |   10 ----------
 include/linux/libata.h   |    2 ++
 3 files changed, 22 insertions(+), 28 deletions(-)

diff --git a/drivers/ata/libata-eh.c b/drivers/ata/libata-eh.c
index 08dd07f..5b5ae63 100644
--- a/drivers/ata/libata-eh.c
+++ b/drivers/ata/libata-eh.c
@@ -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)
diff --git a/drivers/ata/libata-pmp.c b/drivers/ata/libata-pmp.c
index 3a2bce0..3374ec5 100644
--- a/drivers/ata/libata-pmp.c
+++ b/drivers/ata/libata-pmp.c
@@ -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;
 	}
 
diff --git a/include/linux/libata.h b/include/linux/libata.h
index 4bbe955..4fc9ab0 100644
--- a/include/linux/libata.h
+++ b/include/linux/libata.h
@@ -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
-- 
1.5.2.4

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

[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 <htejun@gmail.com>
---
 drivers/ata/libata-eh.c |    8 +++++---
 1 files changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/ata/libata-eh.c b/drivers/ata/libata-eh.c
index 5b5ae63..83d1451 100644
--- a/drivers/ata/libata-eh.c
+++ b/drivers/ata/libata-eh.c
@@ -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))
-- 
1.5.2.4

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

[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 <htejun@gmail.com>
---
 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(-)

diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
index eb9dfbc..1a40d41 100644
--- a/drivers/ata/libata-core.c
+++ b/drivers/ata/libata-core.c
@@ -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;
 }
 
diff --git a/drivers/ata/libata-eh.c b/drivers/ata/libata-eh.c
index 83d1451..d5f03a6 100644
--- a/drivers/ata/libata-eh.c
+++ b/drivers/ata/libata-eh.c
@@ -67,6 +67,8 @@ enum {
 	ATA_ECAT_DUBIOUS_UNK_DEV	= 7,
 	ATA_ECAT_NR			= 8,
 
+	ATA_EH_CMD_DFL_TIMEOUT		=  5000,
+
 	/* always put at least this amount of time between resets */
 	ATA_EH_RESET_COOL_DOWN		=  5000,
 
@@ -93,6 +95,53 @@ static const unsigned long ata_eh_reset_timeouts[] = {
 	ULONG_MAX, /* > 1 min has elapsed, give up */
 };
 
+static const unsigned long ata_eh_identify_timeouts[] = {
+	 5000,	/* covers > 99% of successes and not too boring on failures */
+	10000,  /* combined time till here is enough even for media access */
+	30000,	/* for true idiots */
+	ULONG_MAX,
+};
+
+static const unsigned long ata_eh_other_timeouts[] = {
+	 5000,	/* same rationale as identify timeout */
+	10000,	/* ditto */
+	/* but no merciful 30sec for other commands, it just isn't worth it */
+	ULONG_MAX,
+};
+
+struct ata_eh_cmd_timeout_ent {
+	const u8		*commands;
+	const unsigned long	*timeouts;
+};
+
+/* The following table determines timeouts to use for EH internal
+ * commands.  Each table entry is a command class and matches the
+ * commands the entry applies to and the timeout table to use.
+ *
+ * On the retry after a command timed out, the next timeout value from
+ * the table is used.  If the table doesn't contain further entries,
+ * the last value is used.
+ *
+ * ehc->cmd_timeout_idx keeps track of which timeout to use per
+ * command class, so if SET_FEATURES times out on the first try, the
+ * next try will use the second timeout value only for that class.
+ */
+#define CMDS(cmds...)	(const u8 []){ cmds, 0 }
+static const struct ata_eh_cmd_timeout_ent
+ata_eh_cmd_timeout_table[ATA_EH_CMD_TIMEOUT_TABLE_SIZE] = {
+	{ .commands = CMDS(ATA_CMD_ID_ATA, ATA_CMD_ID_ATAPI),
+	  .timeouts = ata_eh_identify_timeouts, },
+	{ .commands = CMDS(ATA_CMD_READ_NATIVE_MAX, ATA_CMD_READ_NATIVE_MAX_EXT),
+	  .timeouts = ata_eh_other_timeouts, },
+	{ .commands = CMDS(ATA_CMD_SET_MAX, ATA_CMD_SET_MAX_EXT),
+	  .timeouts = ata_eh_other_timeouts, },
+	{ .commands = CMDS(ATA_CMD_SET_FEATURES),
+	  .timeouts = ata_eh_other_timeouts, },
+	{ .commands = CMDS(ATA_CMD_INIT_DEV_PARAMS),
+	  .timeouts = ata_eh_other_timeouts, },
+};
+#undef CMDS
+
 static void __ata_port_freeze(struct ata_port *ap);
 #ifdef CONFIG_PM
 static void ata_eh_handle_port_suspend(struct ata_port *ap);
@@ -238,6 +287,73 @@ void ata_port_pbar_desc(struct ata_port *ap, int bar, ssize_t offset,
 
 #endif /* CONFIG_PCI */
 
+static int ata_lookup_timeout_table(u8 cmd)
+{
+	int i;
+
+	for (i = 0; i < ATA_EH_CMD_TIMEOUT_TABLE_SIZE; i++) {
+		const u8 *cur;
+
+		for (cur = ata_eh_cmd_timeout_table[i].commands; *cur; cur++)
+			if (*cur == cmd)
+				return i;
+	}
+
+	return -1;
+}
+
+/**
+ *	ata_internal_cmd_timeout - determine timeout for an internal command
+ *	@dev: target device
+ *	@cmd: internal command to be issued
+ *
+ *	Determine timeout for internal command @cmd for @dev.
+ *
+ *	LOCKING:
+ *	EH context.
+ *
+ *	RETURNS:
+ *	Determined timeout.
+ */
+unsigned long ata_internal_cmd_timeout(struct ata_device *dev, u8 cmd)
+{
+	struct ata_eh_context *ehc = &dev->link->eh_context;
+	int ent = ata_lookup_timeout_table(cmd);
+	int idx;
+
+	if (ent < 0)
+		return ATA_EH_CMD_DFL_TIMEOUT;
+
+	idx = ehc->cmd_timeout_idx[dev->devno][ent];
+	return ata_eh_cmd_timeout_table[ent].timeouts[idx];
+}
+
+/**
+ *	ata_internal_cmd_timed_out - notification for internal command timeout
+ *	@dev: target device
+ *	@cmd: internal command which timed out
+ *
+ *	Notify EH that internal command @cmd for @dev timed out.  This
+ *	function should be called only for commands whose timeouts are
+ *	determined using ata_internal_cmd_timeout().
+ *
+ *	LOCKING:
+ *	EH context.
+ */
+void ata_internal_cmd_timed_out(struct ata_device *dev, u8 cmd)
+{
+	struct ata_eh_context *ehc = &dev->link->eh_context;
+	int ent = ata_lookup_timeout_table(cmd);
+	int idx;
+
+	if (ent < 0)
+		return;
+
+	idx = ehc->cmd_timeout_idx[dev->devno][ent];
+	if (ata_eh_cmd_timeout_table[ent].timeouts[idx + 1] != ULONG_MAX)
+		ehc->cmd_timeout_idx[dev->devno][ent]++;
+}
+
 static void ata_ering_record(struct ata_ering *ering, unsigned int eflags,
 			     unsigned int err_mask)
 {
@@ -2600,8 +2716,11 @@ static int ata_eh_handle_dev_fail(struct ata_device *dev, int err)
 			ata_eh_detach_dev(dev);
 
 		/* schedule probe if necessary */
-		if (ata_eh_schedule_probe(dev))
+		if (ata_eh_schedule_probe(dev)) {
 			ehc->tries[dev->devno] = ATA_EH_DEV_TRIES;
+			memset(ehc->cmd_timeout_idx[dev->devno], 0,
+			       sizeof(ehc->cmd_timeout_idx[dev->devno]));
+		}
 
 		return 1;
 	} else {
diff --git a/drivers/ata/libata.h b/drivers/ata/libata.h
index 4514283..daf2b44 100644
--- a/drivers/ata/libata.h
+++ b/drivers/ata/libata.h
@@ -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);
diff --git a/include/linux/libata.h b/include/linux/libata.h
index 4fc9ab0..70cf9fd 100644
--- a/include/linux/libata.h
+++ b/include/linux/libata.h
@@ -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;
-- 
1.5.2.4

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

[Alan Cox]

On Tue, 20 May 2008 02:17:52 +0900
Tejun Heo <htejun@gmail.com> 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 ?

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

[Alan Cox]

> 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 ?

> 
> 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 <htejun@gmail.com>

Acked-by: Alan Cox <alan@redhat.com>

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

[Jeff Garzik]

Alan Cox wrote:
>> 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

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

[Elias Oltmanns]

Tejun Heo <htejun@gmail.com> wrote:
> 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 <htejun@gmail.com>
[...]
> +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

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

[Tejun Heo]

Alan Cox wrote:
> On Tue, 20 May 2008 02:17:52 +0900
> Tejun Heo <htejun@gmail.com> 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 ?

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.

-- 
tejun

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

[Tejun Heo]

Elias Oltmanns wrote:
> Tejun Heo <htejun@gmail.com> wrote:
>> 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 <htejun@gmail.com>
> [...]
>> +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.

-- 
tejun

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

[Tejun Heo]

Alan Cox wrote:
>> 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 ?
> 
>> 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 <htejun@gmail.com>
> 
> Acked-by: Alan Cox <alan@redhat.com>

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.

-- 
tejun

Re: [PATCHSET #upstream] libata: improve timeout handling for EH commands

[Tejun Heo]

Tejun Heo wrote:
> 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.

-- 
tejun

Re: [PATCHSET #upstream] libata: improve timeout handling for EH commands

[Jeff Garzik]

Tejun Heo wrote:
> 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.  :-)

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 :)

Re: [PATCHSET #upstream] libata: improve timeout handling for EH commands

[Tejun Heo]

Jeff Garzik wrote:
> Tejun Heo wrote:
>> 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.  :-)
> 
> 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

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

[Jeff Garzik]

Tejun Heo wrote:
> Kill a few unused constants.
> 
> Signed-off-by: Tejun Heo <htejun@gmail.com>
> ---
>  include/linux/libata.h |    3 ---
>  1 files changed, 0 insertions(+), 3 deletions(-)
> 
> diff --git a/include/linux/libata.h b/include/linux/libata.h
> index 95a2000..989088e 100644
> --- a/include/linux/libata.h
> +++ b/include/linux/libata.h
> @@ -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,

applied

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

[Jeff Garzik]

Tejun Heo wrote:
> 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 <htejun@gmail.com>
> ---
>  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

Re: [PATCHSET #upstream] libata: improve timeout handling for EH commands

[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

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

saeed


On Wed, Jun 4, 2008 at 7:02 AM, Tejun Heo <htejun@gmail.com> wrote:
> Jeff Garzik wrote:
>> Tejun Heo wrote:
>>> 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.  :-)
>>
>> 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 majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

Re: [PATCHSET #upstream] libata: improve timeout handling for EH commands

[Tejun Heo]

saeed bishara wrote:
> 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.

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.

-- 
tejun