[PATCH] [RESEND] mtd: docg4: fix status polling loop

[PATCH] [RESEND] mtd: docg4: fix status polling loop

[Mike Dunn]

The loop that polls the status register waiting for an operation to complete
foolishly bases the timeout simply on the number of loop iterations that have
ocurred.  When I increased the processor clock speed, timeouts started to appear
for long block erasure operations.  This patch measures the timeout using
jiffies.

Signed-off-by: Mike Dunn <mikedunn@newsguy.co>
---

This is a rework of an earlier patch, based on Artem's recommendation to use
jiffies, not udelay() (thanks Artem).  I didn't mark it 'v2' because the rework
makes the original name of the patch inaccurate, so now it has a different name.

This was originally sent a couple months ago... was probably a bad time.

 drivers/mtd/nand/docg4.c | 15 ++++++---------
 1 file changed, 6 insertions(+), 9 deletions(-)

diff --git a/drivers/mtd/nand/docg4.c b/drivers/mtd/nand/docg4.c
index fa25e7a..6c3be21 100644
--- a/drivers/mtd/nand/docg4.c
+++ b/drivers/mtd/nand/docg4.c
@@ -44,6 +44,7 @@
 #include <linux/mtd/nand.h>
 #include <linux/bch.h>
 #include <linux/bitrev.h>
+#include <linux/jiffies.h>
 
 /*
  * In "reliable mode" consecutive 2k pages are used in parallel (in some
@@ -269,7 +270,7 @@ static int poll_status(struct docg4_priv *doc)
 	 */
 
 	uint16_t flash_status;
-	unsigned int timeo;
+	unsigned long timeo;
 	void __iomem *docptr = doc->virtadr;
 
 	dev_dbg(doc->dev, "%s...\n", __func__);
@@ -277,22 +278,18 @@ static int poll_status(struct docg4_priv *doc)
 	/* hardware quirk requires reading twice initially */
 	flash_status = readw(docptr + DOC_FLASHCONTROL);
 
-	timeo = 1000;
+	timeo = jiffies + msecs_to_jiffies(200); /* generous timeout */
 	do {
 		cpu_relax();
 		flash_status = readb(docptr + DOC_FLASHCONTROL);
-	} while (!(flash_status & DOC_CTRL_FLASHREADY) && --timeo);
+	} while (!(flash_status & DOC_CTRL_FLASHREADY) &&
+		 time_before(jiffies, timeo));
 
-
-	if (!timeo) {
+	if (unlikely(!(flash_status & DOC_CTRL_FLASHREADY))) {
 		dev_err(doc->dev, "%s: timed out!\n", __func__);
 		return NAND_STATUS_FAIL;
 	}
 
-	if (unlikely(timeo < 50))
-		dev_warn(doc->dev, "%s: nearly timed out; %d remaining\n",
-			 __func__, timeo);
-
 	return 0;
 }
 
-- 
1.8.1.5

[PATCH] [RESEND] mtd: docg4: fix status polling loop

[Mike Dunn]

The loop that polls the status register waiting for an operation to complete
foolishly bases the timeout simply on the number of loop iterations that have
ocurred.  When I increased the processor clock speed, timeouts started to appear
for long block erasure operations.  This patch measures the timeout using
jiffies.

Signed-off-by: Mike Dunn <mikedunn@newsguy.com>
---

This is a rework of an earlier patch, based on Artem's recommendation to use
jiffies, not udelay() (thanks Artem).  I didn't mark it 'v2' because the rework
makes the original name of the patch inaccurate, so now it has a different name.

This was originally sent a couple months ago... was probably a bad time.

 drivers/mtd/nand/docg4.c | 15 ++++++---------
 1 file changed, 6 insertions(+), 9 deletions(-)

diff --git a/drivers/mtd/nand/docg4.c b/drivers/mtd/nand/docg4.c
index fa25e7a..6c3be21 100644
--- a/drivers/mtd/nand/docg4.c
+++ b/drivers/mtd/nand/docg4.c
@@ -44,6 +44,7 @@
 #include <linux/mtd/nand.h>
 #include <linux/bch.h>
 #include <linux/bitrev.h>
+#include <linux/jiffies.h>
 
 /*
  * In "reliable mode" consecutive 2k pages are used in parallel (in some
@@ -269,7 +270,7 @@ static int poll_status(struct docg4_priv *doc)
 	 */
 
 	uint16_t flash_status;
-	unsigned int timeo;
+	unsigned long timeo;
 	void __iomem *docptr = doc->virtadr;
 
 	dev_dbg(doc->dev, "%s...\n", __func__);
@@ -277,22 +278,18 @@ static int poll_status(struct docg4_priv *doc)
 	/* hardware quirk requires reading twice initially */
 	flash_status = readw(docptr + DOC_FLASHCONTROL);
 
-	timeo = 1000;
+	timeo = jiffies + msecs_to_jiffies(200); /* generous timeout */
 	do {
 		cpu_relax();
 		flash_status = readb(docptr + DOC_FLASHCONTROL);
-	} while (!(flash_status & DOC_CTRL_FLASHREADY) && --timeo);
+	} while (!(flash_status & DOC_CTRL_FLASHREADY) &&
+		 time_before(jiffies, timeo));
 
-
-	if (!timeo) {
+	if (unlikely(!(flash_status & DOC_CTRL_FLASHREADY))) {
 		dev_err(doc->dev, "%s: timed out!\n", __func__);
 		return NAND_STATUS_FAIL;
 	}
 
-	if (unlikely(timeo < 50))
-		dev_warn(doc->dev, "%s: nearly timed out; %d remaining\n",
-			 __func__, timeo);
-
 	return 0;
 }
 
-- 
1.8.1.5

Re: [PATCH] [RESEND] mtd: docg4: fix status polling loop

[Brian Norris]

Hi Mike,

On Thu, Oct 10, 2013 at 10:25:27AM -0700, Mike Dunn wrote:
> The loop that polls the status register waiting for an operation to complete
> foolishly bases the timeout simply on the number of loop iterations that have
> ocurred.  When I increased the processor clock speed, timeouts started to appear
> for long block erasure operations.  This patch measures the timeout using
> jiffies.
> 
> Signed-off-by: Mike Dunn <mikedunn@newsguy.com>
> ---
> 
> This is a rework of an earlier patch, based on Artem's recommendation to use
> jiffies, not udelay() (thanks Artem).  I didn't mark it 'v2' because the rework
> makes the original name of the patch inaccurate, so now it has a different name.
> 
> This was originally sent a couple months ago... was probably a bad time.

Wow, this identical patch was sent as far back as May 31 and is being
submitted for the 3rd time now :( I hope there will be fewer "bad times"
like this in the future.

Applied to l2-mtd.git. Thanks for the tenacity.

Is this -stable material, despite the long delay (pun unintended)?

Brian

Re: [PATCH] [RESEND] mtd: docg4: fix status polling loop

[Mike Dunn]

On 10/11/2013 11:16 AM, Brian Norris wrote:
> Hi Mike,
> 
> On Thu, Oct 10, 2013 at 10:25:27AM -0700, Mike Dunn wrote:
>> The loop that polls the status register waiting for an operation to complete
>> foolishly bases the timeout simply on the number of loop iterations that have
>> ocurred.  When I increased the processor clock speed, timeouts started to appear
>> for long block erasure operations.  This patch measures the timeout using
>> jiffies.
>>
>> Signed-off-by: Mike Dunn <mikedunn@newsguy.com>
>> ---
>>
>> This is a rework of an earlier patch, based on Artem's recommendation to use
>> jiffies, not udelay() (thanks Artem).  I didn't mark it 'v2' because the rework
>> makes the original name of the patch inaccurate, so now it has a different name.
>>
>> This was originally sent a couple months ago... was probably a bad time.
> 
> Wow, this identical patch was sent as far back as May 31 and is being
> submitted for the 3rd time now :( I hope there will be fewer "bad times"
> like this in the future.


Thanks much Brian.  No big deal.  There's only one other user of this driver
that I'm aware of :(

Thanks also for volunteering to shoulder some of the mtd maintenance duties.
Good job!

Mike

Re: [PATCH] [RESEND] mtd: docg4: fix status polling loop

[Brian Norris]

On Fri, Oct 11, 2013 at 11:31:18AM -0700, Mike Dunn wrote:
> On 10/11/2013 11:16 AM, Brian Norris wrote:
> > On Thu, Oct 10, 2013 at 10:25:27AM -0700, Mike Dunn wrote:
> >> The loop that polls the status register waiting for an operation to complete
> >> foolishly bases the timeout simply on the number of loop iterations that have
> >> ocurred.  When I increased the processor clock speed, timeouts started to appear
> >> for long block erasure operations.  This patch measures the timeout using
> >> jiffies.
> >>
> >> Signed-off-by: Mike Dunn <mikedunn@newsguy.com>
> >> ---
> >>
> >> This is a rework of an earlier patch, based on Artem's recommendation to use
> >> jiffies, not udelay() (thanks Artem).  I didn't mark it 'v2' because the rework
> >> makes the original name of the patch inaccurate, so now it has a different name.
> >>
> >> This was originally sent a couple months ago... was probably a bad time.
> > 
> > Wow, this identical patch was sent as far back as May 31 and is being
> > submitted for the 3rd time now :( I hope there will be fewer "bad times"
> > like this in the future.
> 
> 
> Thanks much Brian.  No big deal.  There's only one other user of this driver
> that I'm aware of :(
> 
> Thanks also for volunteering to shoulder some of the mtd maintenance duties.
> Good job!

Thanks! It's an interesting job.

Brian