Skip to contentSkip to conversation

mtd: rawnand: Fix unexpected timeouts in waitrdy

PATCH In review Martin Devera 2 participants 8 messages Dec 10, 2019 – Mar 10, 2020 1 message not in the archive

Subsystems (2)
Memory technology devices (MTD) NAND flash
lore
Share & tools
lore

Martin Devera

author
#0 lore raw

PATCH 1/1 mtd: rawnand: Fix unexpected timeouts in waitrdy

The used way to compute jiffies timeout brokes when jiffie difference is 1. Simply add 1 - it has no other side effects.
Fixes STM32MP1 FMC2 NAND controller which sometimes failed exactly in this way.

Signed-off-by Martin Devera <[email protected]>
---
 drivers/mtd/nand/raw/nand_base.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)
drivers/mtd/nand/raw/nand_base.c +5 -1
index d527e448ce19..beab3a775cc7 100644
@@ -721,7 +721,11 @@ int nand_soft_waitrdy(struct nand_chip *chip, unsigned long timeout_ms)
 	if (ret)
 		return ret;
 
-	timeout_ms = jiffies + msecs_to_jiffies(timeout_ms);
+	/* +1 below is necessary because if we are now in the last fraction
+	 * of jiffy and msecs_to_jiffies is 1 then we will wait only that
+	 * small jiffy fraction - possibly leading to false timeout
+	 */
+	timeout_ms = jiffies + msecs_to_jiffies(timeout_ms) + 1;
 	do {
 		ret = nand_read_data_op(chip, &status, sizeof(status), true);
 		if (ret)
Signature · 6 lines
2.11.0


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/
maintainer
#1 lore raw

Hi Martin,

Martin Devera <[email protected]> wrote on Tue, 10 Dec 2019 16:03:18 +0100:

Quoted 18 lines

The used way to compute jiffies timeout brokes when jiffie difference is 1. Simply add 1 - it has no other side effects.
Fixes STM32MP1 FMC2 NAND controller which sometimes failed exactly in this way.

Signed-off-by Martin Devera <[email protected]>
---
 drivers/mtd/nand/raw/nand_base.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)
drivers/mtd/nand/raw/nand_base.c +0 -0
index d527e448ce19..beab3a775cc7 100644
@@ -721,7 +721,11 @@ int nand_soft_waitrdy(struct nand_chip *chip, unsigned long timeout_ms)
 	if (ret)
 		return ret;

drivers/mtd/nand/raw/nand_base.c int nand_soft_waitrdy(struct nand_chip *chip, unsigned long timeout_ms) · line 731

Quoted 10 lines
drivers/mtd/nand/raw/nand_base.c +5 -1
 
-	timeout_ms = jiffies + msecs_to_jiffies(timeout_ms);
+	/* +1 below is necessary because if we are now in the last fraction
+	 * of jiffy and msecs_to_jiffies is 1 then we will wait only that
+	 * small jiffy fraction - possibly leading to false timeout
+	 */
+	timeout_ms = jiffies + msecs_to_jiffies(timeout_ms) + 1;
 	do {
 		ret = nand_read_data_op(chip, &status, sizeof(status), true);
 		if (ret)

I don't really what you are fixing here, I suspect the root cause to be a wrongly calculated timeout_ms in the calling driver.

It is the responsibility of the caller to use this function with a relevant timeout_ms parameter. Maybe Christophe can help you here?

Thanks,
Miquèl

author
#2 lore raw
Quoted 36 lines · Miquel Raynal

Hi Martin,

Martin Devera <[email protected]> wrote on Tue, 10 Dec 2019 16:03:18 +0100:

Quoted 18 lines

The used way to compute jiffies timeout brokes when jiffie difference is 1. Simply add 1 - it has no other side effects.
Fixes STM32MP1 FMC2 NAND controller which sometimes failed exactly in this way.

Signed-off-by Martin Devera <[email protected]>
---
  drivers/mtd/nand/raw/nand_base.c | 6 +++++-
  1 file changed, 5 insertions(+), 1 deletion(-)
drivers/mtd/nand/raw/nand_base.c +0 -0
index d527e448ce19..beab3a775cc7 100644
@@ -721,7 +721,11 @@ int nand_soft_waitrdy(struct nand_chip *chip, unsigned long timeout_ms)
  	if (ret)
  		return ret;

drivers/mtd/nand/raw/nand_base.c int nand_soft_waitrdy(struct nand_chip *chip, unsigned long timeout_ms) · line 731

Quoted 10 lines
drivers/mtd/nand/raw/nand_base.c +5 -1
 
-	timeout_ms = jiffies + msecs_to_jiffies(timeout_ms);
+	/* +1 below is necessary because if we are now in the last fraction
+	 * of jiffy and msecs_to_jiffies is 1 then we will wait only that
+	 * small jiffy fraction - possibly leading to false timeout
+	 */
+	timeout_ms = jiffies + msecs_to_jiffies(timeout_ms) + 1;
  	do {
  		ret = nand_read_data_op(chip, &status, sizeof(status), true);
  		if (ret)

I don't really what you are fixing here, I suspect the root cause to be a wrongly calculated timeout_ms in the calling driver.

Quoted 3 lines

It is the responsibility of the caller to use this function with a relevant timeout_ms parameter. Maybe Christophe can help you here?

Hi Miquel,

assume that nand_soft_waitrdy is called with timeout_ms==1. I suppose it is valid case. Jiffies are 1000 for example (assume something more like 1000.99 - just before incrementing to 1001).
We compute timeout_ms = 1000+msecs_to_jiffies(1) = 1001 (at least for my jiffies rate). nand_read_data_op is called for the first time and returns 0. During the call jiffies changes to 1001 thus "while loop" ends here (wrongly).
Notice that routine was called with expected timeout 1ms but actual timeout used was something between 0...1ms (which I also measured by tracing & scope on the bus).
Or is my analysis flawed somewhere ?

Thanks,

Martin

maintainer
#3 lore raw

Hi Martin,

Martin DEVERA <[email protected]> wrote on Thu, 9 Jan 2020 17:17:30 +0100:

Quoted 41 lines · Miquel Raynal
Quoted 35 lines

Hi Martin,

Martin Devera <[email protected]> wrote on Tue, 10 Dec 2019 16:03:18 +0100:

Quoted 17 lines

The used way to compute jiffies timeout brokes when jiffie difference is 1. Simply add 1 - it has no other side effects.
Fixes STM32MP1 FMC2 NAND controller which sometimes failed exactly in this way.

Signed-off-by Martin Devera <[email protected]>
---
  drivers/mtd/nand/raw/nand_base.c | 6 +++++-
  1 file changed, 5 insertions(+), 1 deletion(-)
drivers/mtd/nand/raw/nand_base.c +0 -0
index d527e448ce19..beab3a775cc7 100644
@@ -721,7 +721,11 @@ int nand_soft_waitrdy(struct nand_chip *chip, unsigned long timeout_ms)
  	if (ret)

drivers/mtd/nand/raw/nand_base.c int nand_soft_waitrdy(struct nand_chip *chip, unsigned long timeout_ms) · line 731

Quoted 10 lines
drivers/mtd/nand/raw/nand_base.c +5 -0
  		return ret;  
  >> -	timeout_ms = jiffies + msecs_to_jiffies(timeout_ms);  
+	/* +1 below is necessary because if we are now in the last fraction
+	 * of jiffy and msecs_to_jiffies is 1 then we will wait only that
+	 * small jiffy fraction - possibly leading to false timeout
+	 */
+	timeout_ms = jiffies + msecs_to_jiffies(timeout_ms) + 1;
  	do {
  		ret = nand_read_data_op(chip, &status, sizeof(status), true);
  		if (ret)

I don't really what you are fixing here, I suspect the root cause to be a wrongly calculated timeout_ms in the calling driver.

Quoted 3 lines

It is the responsibility of the caller to use this function with a relevant timeout_ms parameter. Maybe Christophe can help you here?

Hi Miquel,

Quoted 9 lines

assume that nand_soft_waitrdy is called with timeout_ms==1. I suppose it is valid case. Jiffies are 1000 for example (assume something more like 1000.99 - just before incrementing to 1001).
We compute timeout_ms = 1000+msecs_to_jiffies(1) = 1001 (at least for my jiffies rate). nand_read_data_op is called for the first time and returns 0. During the call jiffies changes to 1001 thus "while loop" ends here (wrongly).
Notice that routine was called with expected timeout 1ms but actual timeout used was something between 0...1ms (which I also measured by tracing & scope on the bus).
Or is my analysis flawed somewhere ?

I agree with your analysis. Even if nand_soft_waitrdy will no longer be used by the stm32 driver (Christophe sent a patch for that) I am fine applying this change.

Could you add a comment to explain the '+1' and resend?

Thanks,
Miquèl

A message that is not in the archive (sent off-list or not yet received): [email protected]

maintainer
#5 lore raw

Hi Martin,

Martin DEVERA <[email protected]> wrote on Thu, 9 Jan 2020 18:43:46 +0100:

Quoted 58 lines · Miquel Raynal
Quoted 57 lines

Hi Martin,

Martin DEVERA <[email protected]> wrote on Thu, 9 Jan 2020 17:17:30 +0100:

Quoted 39 lines · Miquel Raynal
Quoted 34 lines

Hi Martin,

Martin Devera <[email protected]> wrote on Tue, 10 Dec 2019 16:03:18 +0100:

Quoted 17 lines
Quoted 1 line
Quoted 1 line
Quoted 1 line

The used way to compute jiffies timeout brokes when

jiffie difference is 1. Simply add 1 - it has no other side effects.
Fixes STM32MP1 FMC2 NAND controller which sometimes failed exactly in this way.

Signed-off-by Martin Devera <[email protected]>
---
   drivers/mtd/nand/raw/nand_base.c | 6 +++++-
   1 file changed, 5 insertions(+), 1 deletion(-)
drivers/mtd/nand/raw/nand_base.c +0 -0
index d527e448ce19..beab3a775cc7 100644
@@ -721,7 +721,11 @@ int nand_soft_waitrdy(struct nand_chip *chip, unsigned long timeout_ms)
   	if (ret)

drivers/mtd/nand/raw/nand_base.c int nand_soft_waitrdy(struct nand_chip *chip, unsigned long timeout_ms) · line 731

Quoted 10 lines
drivers/mtd/nand/raw/nand_base.c +5 -0
   		return ret;  
   >> -	timeout_ms = jiffies + msecs_to_jiffies(timeout_ms);  
+	/* +1 below is necessary because if we are now in the last fraction
+	 * of jiffy and msecs_to_jiffies is 1 then we will wait only that
+	 * small jiffy fraction - possibly leading to false timeout
+	 */
+	timeout_ms = jiffies + msecs_to_jiffies(timeout_ms) + 1;
   	do {
   		ret = nand_read_data_op(chip, &status, sizeof(status), true);
   		if (ret)

I don't really what you are fixing here, I suspect the root cause to be a wrongly calculated timeout_ms in the calling driver.

Quoted 3 lines

It is the responsibility of the caller to use this function with a relevant timeout_ms parameter. Maybe Christophe can help you here?

Quoted 1 line
Quoted 1 line

Hi Miquel,

Quoted 9 lines

assume that nand_soft_waitrdy is called with timeout_ms==1. I suppose it is valid case. Jiffies are 1000 for example (assume something more like 1000.99 - just before incrementing to 1001).
We compute timeout_ms = 1000+msecs_to_jiffies(1) = 1001 (at least for my jiffies rate). nand_read_data_op is called for the first time and returns 0. During the call jiffies changes to 1001 thus "while loop" ends here (wrongly).
Notice that routine was called with expected timeout 1ms but actual timeout used was something between 0...1ms (which I also measured by tracing & scope on the bus).
Or is my analysis flawed somewhere ?

I agree with your analysis. Even if nand_soft_waitrdy will no longer be used by the stm32 driver (Christophe sent a patch for that) I am fine applying this change.

Quoted 5 lines
Quoted 2 lines

Could you add a comment to explain the '+1' and resend?

Can you give me some guidance please ? Where should I add more comment to the git commit or to the C code ? I tried to comment both commit and code, probably you find the comments not clear enough ?

Sorry for not explaining: Could you add the example to the commit message? The comment is fine, besides the fact that it should start like this:

	/*
	 * Bla bla bla

Also please change the commit title, maybe

mtd: rawnand: Ensure nand_soft_waitrdy wait period is enough

Thanks,
Miquèl

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

maintainer
#6 lore raw

Hi Martin,

I forgot to mention: please don't forget to keep everyone in copy. I re-added the mtd-list in my previous answer.

Miquel Raynal <[email protected]> wrote on Thu, 9 Jan 2020 19:02:40 +0100:

Quoted 76 lines

Hi Martin,

Martin DEVERA <[email protected]> wrote on Thu, 9 Jan 2020 18:43:46 +0100:

Quoted 58 lines · Miquel Raynal
Quoted 57 lines

Hi Martin,

Martin DEVERA <[email protected]> wrote on Thu, 9 Jan 2020 17:17:30 +0100:

Quoted 39 lines · Miquel Raynal
Quoted 34 lines

Hi Martin,

Martin Devera <[email protected]> wrote on Tue, 10 Dec 2019 16:03:18 +0100:

Quoted 17 lines
Quoted 1 line
Quoted 1 line
Quoted 1 line

The used way to compute jiffies timeout brokes when

jiffie difference is 1. Simply add 1 - it has no other side effects.
Fixes STM32MP1 FMC2 NAND controller which sometimes failed exactly in this way.

Signed-off-by Martin Devera <[email protected]>
---
   drivers/mtd/nand/raw/nand_base.c | 6 +++++-
   1 file changed, 5 insertions(+), 1 deletion(-)
drivers/mtd/nand/raw/nand_base.c +0 -0
index d527e448ce19..beab3a775cc7 100644
@@ -721,7 +721,11 @@ int nand_soft_waitrdy(struct nand_chip *chip, unsigned long timeout_ms)
   	if (ret)

drivers/mtd/nand/raw/nand_base.c int nand_soft_waitrdy(struct nand_chip *chip, unsigned long timeout_ms) · line 731

Quoted 10 lines
drivers/mtd/nand/raw/nand_base.c +5 -0
   		return ret;    
   >> -	timeout_ms = jiffies + msecs_to_jiffies(timeout_ms);    
+	/* +1 below is necessary because if we are now in the last fraction
+	 * of jiffy and msecs_to_jiffies is 1 then we will wait only that
+	 * small jiffy fraction - possibly leading to false timeout
+	 */
+	timeout_ms = jiffies + msecs_to_jiffies(timeout_ms) + 1;
   	do {
   		ret = nand_read_data_op(chip, &status, sizeof(status), true);
   		if (ret)

I don't really what you are fixing here, I suspect the root cause to be a wrongly calculated timeout_ms in the calling driver.

Quoted 3 lines

It is the responsibility of the caller to use this function with a relevant timeout_ms parameter. Maybe Christophe can help you here?

Quoted 1 line
Quoted 1 line

Hi Miquel,

Quoted 9 lines

assume that nand_soft_waitrdy is called with timeout_ms==1. I suppose it is valid case. Jiffies are 1000 for example (assume something more like 1000.99 - just before incrementing to 1001).
We compute timeout_ms = 1000+msecs_to_jiffies(1) = 1001 (at least for my jiffies rate). nand_read_data_op is called for the first time and returns 0. During the call jiffies changes to 1001 thus "while loop" ends here (wrongly).
Notice that routine was called with expected timeout 1ms but actual timeout used was something between 0...1ms (which I also measured by tracing & scope on the bus).
Or is my analysis flawed somewhere ?

I agree with your analysis. Even if nand_soft_waitrdy will no longer be used by the stm32 driver (Christophe sent a patch for that) I am fine applying this change.

Quoted 5 lines
Quoted 2 lines

Could you add a comment to explain the '+1' and resend?

Can you give me some guidance please ? Where should I add more comment to the git commit or to the C code ? I tried to comment both commit and code, probably you find the comments not clear enough ?

Sorry for not explaining: Could you add the example to the commit message? The comment is fine, besides the fact that it should start like this:

	/*
	 * Bla bla bla
Quoted 6 lines

Also please change the commit title, maybe

mtd: rawnand: Ensure nand_soft_waitrdy wait period is enough

Thanks,
Miquèl

Thanks,
Miquèl

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

author
#7 lore raw

PATCH 1/1 mtd: rawnand: Ensure nand_soft_waitrdy wait period is enough

The used way to compute jiffies timeout brokes when jiffie difference is 1.
Assume that nand_soft_waitrdy is called with timeout_ms==1. Jiffies are 1000 for example (assume something more like 1000.99
- just before incrementing to 1001).
We compute timeout_ms = 1000+msecs_to_jiffies(1) = 1001. nand_read_data_op is called for the first time and returns 0. During the call jiffies changes to 1001 thus "while loop" ends here (wrongly). Notice that routine was called with expected timeout 1ms but actual timeout used was something between 0...1ms.

Fixes STM32MP1 FMC2 NAND controller which sometimes failed exactly in this way.

Signed-off-by Martin Devera <[email protected]>
---
 drivers/mtd/nand/raw/nand_base.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)
drivers/mtd/nand/raw/nand_base.c +6 -1
index d527e448ce19..9a7e09c8d8be 100644
@@ -721,7 +721,12 @@ int nand_soft_waitrdy(struct nand_chip *chip, unsigned long timeout_ms)
 	if (ret)
 		return ret;
 
-	timeout_ms = jiffies + msecs_to_jiffies(timeout_ms);
+	/*
+	 * +1 below is necessary because if we are now in the last fraction
+	 * of jiffy and msecs_to_jiffies is 1 then we will wait only that
+	 * small jiffy fraction - possibly leading to false timeout
+	 */
+	timeout_ms = jiffies + msecs_to_jiffies(timeout_ms) + 1;
 	do {
 		ret = nand_read_data_op(chip, &status, sizeof(status), true);
 		if (ret)
Signature · 6 lines
2.11.0


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/
maintainer Applied
#8 lore raw

Re: [PATCH] mtd: rawnand: Ensure nand_soft_waitrdy wait period is enough

Quoted 11 lines · Martin Devera

The used way to compute jiffies timeout brokes when jiffie difference is 1.
Assume that nand_soft_waitrdy is called with timeout_ms==1. Jiffies are 1000 for example (assume something more like 1000.99
- just before incrementing to 1001).
We compute timeout_ms = 1000+msecs_to_jiffies(1) = 1001. nand_read_data_op is called for the first time and returns 0. During the call jiffies changes to 1001 thus "while loop" ends here (wrongly). Notice that routine was called with expected timeout 1ms but actual timeout used was something between 0...1ms.

Quoted 4 lines

Fixes STM32MP1 FMC2 NAND controller which sometimes failed exactly in this way.

Signed-off-by Martin Devera <[email protected]>

Applied to https://git.kernel.org/pub/scm/linux/kernel/git/mtd/linux.git nand/next, thanks.

Miquel

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/