[PATCH] mtd: rawnand: Fix unexpected timeouts in waitrdy

[PATCH] mtd: rawnand: Fix unexpected timeouts in waitrdy

[Martin Devera]

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 <devik@eaxlabs.cz>
---
 drivers/mtd/nand/raw/nand_base.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/mtd/nand/raw/nand_base.c b/drivers/mtd/nand/raw/nand_base.c
index d527e448ce19..beab3a775cc7 100644
--- a/drivers/mtd/nand/raw/nand_base.c
+++ b/drivers/mtd/nand/raw/nand_base.c
@@ -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)
-- 
2.11.0

Re: [PATCH] mtd: rawnand: Fix unexpected timeouts in waitrdy

[Miquel Raynal]

Hi Martin,

Martin Devera <devik@eaxlabs.cz> wrote on Tue, 10 Dec 2019 16:03:18
+0100:

> 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 <devik@eaxlabs.cz>
> ---
>  drivers/mtd/nand/raw/nand_base.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/mtd/nand/raw/nand_base.c b/drivers/mtd/nand/raw/nand_base.c
> index d527e448ce19..beab3a775cc7 100644
> --- a/drivers/mtd/nand/raw/nand_base.c
> +++ b/drivers/mtd/nand/raw/nand_base.c
> @@ -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)

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

Re: [PATCH] mtd: rawnand: Fix unexpected timeouts in waitrdy

[Martin DEVERA]

On 1/9/20 4:37 PM, Miquel Raynal wrote:
> Hi Martin,
>
> Martin Devera <devik@eaxlabs.cz> wrote on Tue, 10 Dec 2019 16:03:18
> +0100:
>
>> 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 <devik@eaxlabs.cz>
>> ---
>>   drivers/mtd/nand/raw/nand_base.c | 6 +++++-
>>   1 file changed, 5 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/mtd/nand/raw/nand_base.c b/drivers/mtd/nand/raw/nand_base.c
>> index d527e448ce19..beab3a775cc7 100644
>> --- a/drivers/mtd/nand/raw/nand_base.c
>> +++ b/drivers/mtd/nand/raw/nand_base.c
>> @@ -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)
> 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?
>
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

Re: [PATCH] mtd: rawnand: Fix unexpected timeouts in waitrdy

[Miquel Raynal]

Hi Martin,

Martin DEVERA <devik@eaxlabs.cz> wrote on Thu, 9 Jan 2020 17:17:30
+0100:

> On 1/9/20 4:37 PM, Miquel Raynal wrote:
> > Hi Martin,
> >
> > Martin Devera <devik@eaxlabs.cz> wrote on Tue, 10 Dec 2019 16:03:18
> > +0100:
> >  
> >> 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 <devik@eaxlabs.cz>
> >> ---
> >>   drivers/mtd/nand/raw/nand_base.c | 6 +++++-
> >>   1 file changed, 5 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/mtd/nand/raw/nand_base.c b/drivers/mtd/nand/raw/nand_base.c
> >> index d527e448ce19..beab3a775cc7 100644
> >> --- a/drivers/mtd/nand/raw/nand_base.c
> >> +++ b/drivers/mtd/nand/raw/nand_base.c
> >> @@ -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)  
> > 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?
> >  
> 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 ?

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