[PATCH] nand_wait_ready timeout fix

[PATCH] nand_wait_ready timeout fix

[Matthieu CASTET]

nand_wait_ready timeout should not assume HZ=1000.
Make it independent of HZ value like it is done in nand_wait.

Signed-off-by: Matthieu CASTET <matthieu.castet@parrot.com>
---
 drivers/mtd/nand/nand_base.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
index d5ece6e..ee49fe2 100644
--- a/drivers/mtd/nand/nand_base.c
+++ b/drivers/mtd/nand/nand_base.c
@@ -492,7 +492,7 @@ static void panic_nand_wait_ready(struct mtd_info *mtd, unsigned long timeo)
 void nand_wait_ready(struct mtd_info *mtd)
 {
 	struct nand_chip *chip = mtd->priv;
-	unsigned long timeo = jiffies + 2;
+	unsigned long timeo = jiffies + (2 * HZ) / 1000;
 
 	/* 400ms timeout */
 	if (in_interrupt() || oops_in_progress)
-- 
1.7.10.4

Re: [PATCH] nand_wait_ready timeout fix

[Artem Bityutskiy]

[-- Attachment #1: Type: text/plain, Size: 1066 bytes --]

On Mon, 2012-11-05 at 14:51 +0100, Matthieu CASTET wrote:
> nand_wait_ready timeout should not assume HZ=1000.
> Make it independent of HZ value like it is done in nand_wait.
> 
> Signed-off-by: Matthieu CASTET <matthieu.castet@parrot.com>
> ---
>  drivers/mtd/nand/nand_base.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
> index d5ece6e..ee49fe2 100644
> --- a/drivers/mtd/nand/nand_base.c
> +++ b/drivers/mtd/nand/nand_base.c
> @@ -492,7 +492,7 @@ static void panic_nand_wait_ready(struct mtd_info *mtd, unsigned long timeo)
>  void nand_wait_ready(struct mtd_info *mtd)
>  {
>  	struct nand_chip *chip = mtd->priv;
> -	unsigned long timeo = jiffies + 2;
> +	unsigned long timeo = jiffies + (2 * HZ) / 1000;

I HZ=100, (2 * HZ) / 1000 will be 0, which is probably wrong.

Instead, this cruft should be re-worked and should use milliseconds for
time-outs, and use things like 'msecs_to_jiffies()' instead.


-- 
Best Regards,
Artem Bityutskiy

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH] nand_wait_ready timeout fix

[Matthieu CASTET]

Artem Bityutskiy a écrit :
> On Mon, 2012-11-05 at 14:51 +0100, Matthieu CASTET wrote:
>> nand_wait_ready timeout should not assume HZ=1000.
>> Make it independent of HZ value like it is done in nand_wait.
>>
>> Signed-off-by: Matthieu CASTET <matthieu.castet@parrot.com>
>> ---
>>  drivers/mtd/nand/nand_base.c |    2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
>> index d5ece6e..ee49fe2 100644
>> --- a/drivers/mtd/nand/nand_base.c
>> +++ b/drivers/mtd/nand/nand_base.c
>> @@ -492,7 +492,7 @@ static void panic_nand_wait_ready(struct mtd_info *mtd, unsigned long timeo)
>>  void nand_wait_ready(struct mtd_info *mtd)
>>  {
>>  	struct nand_chip *chip = mtd->priv;
>> -	unsigned long timeo = jiffies + 2;
>> +	unsigned long timeo = jiffies + (2 * HZ) / 1000;
> 
> I HZ=100, (2 * HZ) / 1000 will be 0, which is probably wrong.
> 
> Instead, this cruft should be re-worked and should use milliseconds for
> time-outs, and use things like 'msecs_to_jiffies()' instead.
> 
> 

Ok,

Should I also update nand_wait [1] ?


Matthieu

[1]
static int nand_wait(struct mtd_info *mtd, struct nand_chip *chip)
{

    unsigned long timeo = jiffies;
    int status, state = chip->state;

    if (state == FL_ERASING)
        timeo += (HZ * 400) / 1000;
    else
        timeo += (HZ * 20) / 1000;

Re: [PATCH] nand_wait_ready timeout fix

[Artem Bityutskiy]

[-- Attachment #1: Type: text/plain, Size: 165 bytes --]

On Thu, 2012-11-22 at 18:06 +0100, Matthieu CASTET wrote:
> Should I also update nand_wait [1] ?

Would be nice I think.

-- 
Best Regards,
Artem Bityutskiy

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[PATCH] nand_wait_ready timeout fix

[Matthieu CASTET]

nand_wait_ready timeout should not assume HZ=100.
Make it independent of HZ value by using msecs_to_jiffies.

Signed-off-by: Matthieu CASTET <matthieu.castet@parrot.com>
---
 drivers/mtd/nand/nand_base.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
index 1a03b7f..16e95e7 100644
--- a/drivers/mtd/nand/nand_base.c
+++ b/drivers/mtd/nand/nand_base.c
@@ -492,7 +492,7 @@ static void panic_nand_wait_ready(struct mtd_info *mtd, unsigned long timeo)
 void nand_wait_ready(struct mtd_info *mtd)
 {
 	struct nand_chip *chip = mtd->priv;
-	unsigned long timeo = jiffies + 2;
+	unsigned long timeo = jiffies + msecs_to_jiffies(20);
 
 	/* 400ms timeout */
 	if (in_interrupt() || oops_in_progress)
-- 
1.7.10.4

RE: [PATCH] nand_wait_ready timeout fix

[Johan Gunnarsson]

> -----Original Message-----
> From: linux-mtd-bounces@lists.infradead.org [mailto:linux-mtd-
> bounces@lists.infradead.org] On Behalf Of Matthieu CASTET
> Sent: den 22 november 2012 18:31
> To: linux-mtd@lists.infradead.org
> Cc: Matthieu CASTET; dedekind1@gmail.com
> Subject: [PATCH] nand_wait_ready timeout fix
> 
> nand_wait_ready timeout should not assume HZ=100.
> Make it independent of HZ value by using msecs_to_jiffies.
> 
> Signed-off-by: Matthieu CASTET <matthieu.castet@parrot.com>
> ---
>  drivers/mtd/nand/nand_base.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/mtd/nand/nand_base.c
> b/drivers/mtd/nand/nand_base.c
> index 1a03b7f..16e95e7 100644
> --- a/drivers/mtd/nand/nand_base.c
> +++ b/drivers/mtd/nand/nand_base.c
> @@ -492,7 +492,7 @@ static void panic_nand_wait_ready(struct mtd_info
> *mtd, unsigned long timeo)
>  void nand_wait_ready(struct mtd_info *mtd)
>  {
>  	struct nand_chip *chip = mtd->priv;
> -	unsigned long timeo = jiffies + 2;
> +	unsigned long timeo = jiffies + msecs_to_jiffies(20);

This won't work well for HZ <= 50 (probably not a very realistic case,
but still...)

I was involved in a discussion about this some months ago. The output of
that discussion was that it is probably safe to set the timeout to much
longer, like 1000ms, for both program and erase commands.

> 
>  	/* 400ms timeout */
>  	if (in_interrupt() || oops_in_progress)
> --
> 1.7.10.4
> 
> 
> ______________________________________________________
> Linux MTD discussion mailing list
> http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH] nand_wait_ready timeout fix

[Artem Bityutskiy]

[-- Attachment #1: Type: text/plain, Size: 314 bytes --]

On Thu, 2012-11-22 at 18:31 +0100, Matthieu CASTET wrote:
> nand_wait_ready timeout should not assume HZ=100.
> Make it independent of HZ value by using msecs_to_jiffies.
> 
> Signed-off-by: Matthieu CASTET <matthieu.castet@parrot.com>

Pushed to l2-mtd.git, thanks!

-- 
Best Regards,
Artem Bityutskiy

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]