libata: fix last_reset timestamp handling

PATCH New Tejun Heo 2 participants 3 messages Nov 4, 2008 – Nov 11, 2008 libata

Share & tools

Tejun Heo

author

2008-11-04 07:32 UTC #0 lore raw

PATCH 1/1 libata: fix last_reset timestamp handling

ehc->last_reset is used to ensure that resets are not issued too close to each other. It's initialized to jiffies minus one minute on EH entry. However, when new links are initialized after PMP is probed, new links have zero for this timestamp resulting in long wait depending on the current jiffies.

This patch makes last_set considered iff ATA_EHI_DID_RESET is set, in which case last_reset is always initialized. As an added precaution, WARN_ON() is added so that warning is printed if last_reset is in future.

This problem is spotted and debugged by Shane Huang.

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

Cc Shane Huang <[email protected]>

---
This one also should go into 2.6.27-x. Thanks.

 drivers/ata/libata-eh.c |   20 +++++++++++---------
 1 file changed, 11 insertions(+), 9 deletions(-)

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

index 6b05809..1b46f60 100644
@@ -632,9 +632,6 @@ 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;

@@ -2308,10 +2305,15 @@ int ata_eh_reset(struct ata_link *link, int classify,
 	if (link->flags & ATA_LFLAG_NO_SRST)
 		softreset = NULL;
 
-	now = jiffies;
-	deadline = ata_deadline(ehc->last_reset, ATA_EH_RESET_COOL_DOWN);
-	if (time_before(now, deadline))
-		schedule_timeout_uninterruptible(deadline - now);
+	/* make sure each reset attemp is at least COOL_DOWN apart */
+	if (ehc->i.flags & ATA_EHI_DID_RESET) {
+		now = jiffies;
+		WARN_ON(time_after(ehc->last_reset, now));
+		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;

@@ -2406,7 +2408,6 @@ 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);

@@ -2418,6 +2419,7 @@ int ata_eh_reset(struct ata_link *link, int classify,
 					reset == softreset ? "soft" : "hard");
 
 		/* mark that this EH session started with reset */
+		ehc->last_reset = jiffies;
 		if (reset == hardreset)
 			ehc->i.flags |= ATA_EHI_DID_HARDRESET;
 		else

@@ -2562,7 +2564,7 @@ int ata_eh_reset(struct ata_link *link, int classify,
 	ata_eh_done(link, NULL, ATA_EH_RESET);
 	if (slave)
 		ata_eh_done(slave, NULL, ATA_EH_RESET);
-	ehc->last_reset = jiffies;
+	ehc->last_reset = jiffies;	/* update to completion time */
 	ehc->i.action |= ATA_EH_REVALIDATE;
 
 	rc = 0;

Tejun Heo

author

2008-11-04 08:08 UTC #1 lore raw

PATCH 1/1 libata: fix last_reset timestamp handling

This problem is spotted and debugged by Shane Huang.

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

Cc Shane Huang <[email protected]>

---
This one also should go into 2.6.27-x. Thanks.

Minor update. Kill ehc->last_reset update right after ata_eh_about_to_do(). This does no harm but is unnecessary with the patch applied.

 drivers/ata/libata-eh.c |   21 +++++++++++----------
 1 file changed, 11 insertions(+), 10 deletions(-)

drivers/ata/libata-eh.c +11 ~~-10~~

index 6b05809..8147a83 100644
@@ -632,9 +632,6 @@ 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;

@@ -2308,17 +2305,21 @@ int ata_eh_reset(struct ata_link *link, int classify,
 	if (link->flags & ATA_LFLAG_NO_SRST)
 		softreset = NULL;
 
-	now = jiffies;
-	deadline = ata_deadline(ehc->last_reset, ATA_EH_RESET_COOL_DOWN);
-	if (time_before(now, deadline))
-		schedule_timeout_uninterruptible(deadline - now);
+	/* make sure each reset attemp is at least COOL_DOWN apart */
+	if (ehc->i.flags & ATA_EHI_DID_RESET) {
+		now = jiffies;
+		WARN_ON(time_after(ehc->last_reset, now));
+		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_for_each_dev(dev, link, ALL) {
 		/* If we issue an SRST then an ATA drive (not ATAPI)

@@ -2406,7 +2407,6 @@ 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);

@@ -2418,6 +2418,7 @@ int ata_eh_reset(struct ata_link *link, int classify,
 					reset == softreset ? "soft" : "hard");
 
 		/* mark that this EH session started with reset */
+		ehc->last_reset = jiffies;
 		if (reset == hardreset)
 			ehc->i.flags |= ATA_EHI_DID_HARDRESET;
 		else

@@ -2562,7 +2563,7 @@ int ata_eh_reset(struct ata_link *link, int classify,
 	ata_eh_done(link, NULL, ATA_EH_RESET);
 	if (slave)
 		ata_eh_done(slave, NULL, ATA_EH_RESET);
-	ehc->last_reset = jiffies;
+	ehc->last_reset = jiffies;	/* update to completion time */
 	ehc->i.action |= ATA_EH_REVALIDATE;
 
 	rc = 0;

Jeff Garzik

Applied

2008-11-11 08:02 UTC #2 lore raw

Quoted 18 lines · Tejun Heo

ehc->last_reset is used to ensure that resets are not issued too close to each other. It's initialized to jiffies minus one minute on EH entry. However, when new links are initialized after PMP is probed, new links have zero for this timestamp resulting in long wait depending on the current jiffies.
This patch makes last_set considered iff ATA_EHI_DID_RESET is set, in which case last_reset is always initialized. As an added precaution, WARN_ON() is added so that warning is printed if last_reset is in future.
This problem is spotted and debugged by Shane Huang.
Signed-off-by Tejun Heo <[email protected]>
Cc Shane Huang <[email protected]>
---
This one also should go into 2.6.27-x. Thanks.

Quoted 6 lines

Minor update. Kill ehc->last_reset update right after ata_eh_about_to_do(). This does no harm but is unnecessary with the patch applied.
 drivers/ata/libata-eh.c |   21 +++++++++++----------
 1 file changed, 11 insertions(+), 10 deletions(-)

applied