[PATCH] CFI: prevent the false software timeouts on writes

[PATCH] CFI: prevent the false software timeouts on writes

[Sergei Shtylylov]

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

Hello.

       We've noticed that sometimes "MTD do_write_buffer(): software timeout"
message was printed out when writing to a Fujitsu NOR flash.
       It turned out that this was because of a race in the timeout handling
do_write_buffer(). A small timeout of (HZ / 1000) + 1 is used there, and
sometimes if the timer interrupt handling takes more than one or even two
jiffies (which is 1-2 ms with HZ == 1000) and that interrupt happens just
after chip_ready() call, the driver bails out from a ready polling loop
despite the chip has actually become ready while all those interrupts were
handled. To deal with this issue, extra check for chip ready is neccessary on
timeout expiration (and the checks should better be reordered).
       As do_write_oneword() uses the same approach, it needs to also be changed.

Signed-off-by: Konstantin Baidarov <kbaidarov@ru.mvista.com>
Signed-off-by: Sergei Shtylyov <sshtylyov@ru.mvista.com>





[-- Attachment #2: cfi_cmdset_0002-write-timeout.patch --]
[-- Type: text/plain, Size: 1324 bytes --]

Index: drivers/mtd/chips/cfi_cmdset_0002.c
===================================================================
RCS file: /home/cvs/mtd/drivers/mtd/chips/cfi_cmdset_0002.c,v
retrieving revision 1.120
diff -a -u -p -r1.120 cfi_cmdset_0002.c
--- drivers/mtd/chips/cfi_cmdset_0002.c	20 Jul 2005 21:01:13 -0000	1.120
+++ drivers/mtd/chips/cfi_cmdset_0002.c	25 Oct 2005 15:38:44 -0000
@@ -1014,16 +1014,16 @@ static int __xipram do_write_oneword(str
 			continue;
 		}
 
-		if (chip_ready(map, adr))
-			break;
-
-		if (time_after(jiffies, timeo)) {
+		if (time_after(jiffies, timeo) && !chip_ready(map, adr)){
 			xip_enable(map, chip, adr);
 			printk(KERN_WARNING "MTD %s(): software timeout\n", __func__);
 			xip_disable(map, chip, adr);
-                        break;
+			break;
 		}
 
+		if (chip_ready(map, adr))
+			break;
+
 		/* Latency issues. Drop the lock, wait a while and retry */
 		UDELAY(map, chip, adr, 1);
 	}
@@ -1275,13 +1275,13 @@ static int __xipram do_write_buffer(stru
 			continue;
 		}
 
+		if (time_after(jiffies, timeo) && !chip_ready(map, adr))
+			break;
+
 		if (chip_ready(map, adr)) {
 			xip_enable(map, chip, adr);
 			goto op_done;
 		}
-		    
-		if( time_after(jiffies, timeo))
-			break;
 
 		/* Latency issues. Drop the lock, wait a while and retry */
 		UDELAY(map, chip, adr, 1);

Re: [PATCH] CFI: prevent the false software timeouts on writes

[Vitaly Wool]

Hi Sergey,

I'm going to verify if this works with XIP and get back to you.

Best regards,
   Vitaly

Sergei Shtylylov wrote:

> Hello.
>
>       We've noticed that sometimes "MTD do_write_buffer(): software 
> timeout"
> message was printed out when writing to a Fujitsu NOR flash.
>       It turned out that this was because of a race in the timeout 
> handling
> do_write_buffer(). A small timeout of (HZ / 1000) + 1 is used there, and
> sometimes if the timer interrupt handling takes more than one or even two
> jiffies (which is 1-2 ms with HZ == 1000) and that interrupt happens just
> after chip_ready() call, the driver bails out from a ready polling loop
> despite the chip has actually become ready while all those interrupts 
> were
> handled. To deal with this issue, extra check for chip ready is 
> neccessary on
> timeout expiration (and the checks should better be reordered).
>       As do_write_oneword() uses the same approach, it needs to also 
> be changed.
>
> Signed-off-by: Konstantin Baidarov <kbaidarov@ru.mvista.com>
> Signed-off-by: Sergei Shtylyov <sshtylyov@ru.mvista.com>
>
>
>
>
>------------------------------------------------------------------------
>
>Index: drivers/mtd/chips/cfi_cmdset_0002.c
>===================================================================
>RCS file: /home/cvs/mtd/drivers/mtd/chips/cfi_cmdset_0002.c,v
>retrieving revision 1.120
>diff -a -u -p -r1.120 cfi_cmdset_0002.c
>--- drivers/mtd/chips/cfi_cmdset_0002.c	20 Jul 2005 21:01:13 -0000	1.120
>+++ drivers/mtd/chips/cfi_cmdset_0002.c	25 Oct 2005 15:38:44 -0000
>@@ -1014,16 +1014,16 @@ static int __xipram do_write_oneword(str
> 			continue;
> 		}
> 
>-		if (chip_ready(map, adr))
>-			break;
>-
>-		if (time_after(jiffies, timeo)) {
>+		if (time_after(jiffies, timeo) && !chip_ready(map, adr)){
> 			xip_enable(map, chip, adr);
> 			printk(KERN_WARNING "MTD %s(): software timeout\n", __func__);
> 			xip_disable(map, chip, adr);
>-                        break;
>+			break;
> 		}
> 
>+		if (chip_ready(map, adr))
>+			break;
>+
> 		/* Latency issues. Drop the lock, wait a while and retry */
> 		UDELAY(map, chip, adr, 1);
> 	}
>@@ -1275,13 +1275,13 @@ static int __xipram do_write_buffer(stru
> 			continue;
> 		}
> 
>+		if (time_after(jiffies, timeo) && !chip_ready(map, adr))
>+			break;
>+
> 		if (chip_ready(map, adr)) {
> 			xip_enable(map, chip, adr);
> 			goto op_done;
> 		}
>-		    
>-		if( time_after(jiffies, timeo))
>-			break;
> 
> 		/* Latency issues. Drop the lock, wait a while and retry */
> 		UDELAY(map, chip, adr, 1);
>
>
>
>
>  
>
>------------------------------------------------------------------------
>
>______________________________________________________
>Linux MTD discussion mailing list
>http://lists.infradead.org/mailman/listinfo/linux-mtd/
>  
>

Re: [PATCH] CFI: prevent the false software timeouts on writes

[Vitaly Wool]

Greetings,

yeah it works with XIP! :)
I guess you can ask for commit now if no one objects.

Best regards,
   Vitaly

Vitaly Wool wrote:

> Hi Sergey,
>
> I'm going to verify if this works with XIP and get back to you.
>
> Best regards,
>   Vitaly
>
> Sergei Shtylylov wrote:
>
>> Hello.
>>
>>       We've noticed that sometimes "MTD do_write_buffer(): software 
>> timeout"
>> message was printed out when writing to a Fujitsu NOR flash.
>>       It turned out that this was because of a race in the timeout 
>> handling
>> do_write_buffer(). A small timeout of (HZ / 1000) + 1 is used there, and
>> sometimes if the timer interrupt handling takes more than one or even 
>> two
>> jiffies (which is 1-2 ms with HZ == 1000) and that interrupt happens 
>> just
>> after chip_ready() call, the driver bails out from a ready polling loop
>> despite the chip has actually become ready while all those interrupts 
>> were
>> handled. To deal with this issue, extra check for chip ready is 
>> neccessary on
>> timeout expiration (and the checks should better be reordered).
>>       As do_write_oneword() uses the same approach, it needs to also 
>> be changed.
>>
>> Signed-off-by: Konstantin Baidarov <kbaidarov@ru.mvista.com>
>> Signed-off-by: Sergei Shtylyov <sshtylyov@ru.mvista.com>
>>
>>
>>
>>
>> ------------------------------------------------------------------------
>>
>> Index: drivers/mtd/chips/cfi_cmdset_0002.c
>> ===================================================================
>> RCS file: /home/cvs/mtd/drivers/mtd/chips/cfi_cmdset_0002.c,v
>> retrieving revision 1.120
>> diff -a -u -p -r1.120 cfi_cmdset_0002.c
>> --- drivers/mtd/chips/cfi_cmdset_0002.c    20 Jul 2005 21:01:13 
>> -0000    1.120
>> +++ drivers/mtd/chips/cfi_cmdset_0002.c    25 Oct 2005 15:38:44 -0000
>> @@ -1014,16 +1014,16 @@ static int __xipram do_write_oneword(str
>>             continue;
>>         }
>>
>> -        if (chip_ready(map, adr))
>> -            break;
>> -
>> -        if (time_after(jiffies, timeo)) {
>> +        if (time_after(jiffies, timeo) && !chip_ready(map, adr)){
>>             xip_enable(map, chip, adr);
>>             printk(KERN_WARNING "MTD %s(): software timeout\n", 
>> __func__);
>>             xip_disable(map, chip, adr);
>> -                        break;
>> +            break;
>>         }
>>
>> +        if (chip_ready(map, adr))
>> +            break;
>> +
>>         /* Latency issues. Drop the lock, wait a while and retry */
>>         UDELAY(map, chip, adr, 1);
>>     }
>> @@ -1275,13 +1275,13 @@ static int __xipram do_write_buffer(stru
>>             continue;
>>         }
>>
>> +        if (time_after(jiffies, timeo) && !chip_ready(map, adr))
>> +            break;
>> +
>>         if (chip_ready(map, adr)) {
>>             xip_enable(map, chip, adr);
>>             goto op_done;
>>         }
>> -            -        if( time_after(jiffies, timeo))
>> -            break;
>>
>>         /* Latency issues. Drop the lock, wait a while and retry */
>>         UDELAY(map, chip, adr, 1);
>>
>>
>>
>>
>>  
>>
>> ------------------------------------------------------------------------
>>
>> ______________________________________________________
>> Linux MTD discussion mailing list
>> http://lists.infradead.org/mailman/listinfo/linux-mtd/
>>  
>>
>
>
> ______________________________________________________
> Linux MTD discussion mailing list
> http://lists.infradead.org/mailman/listinfo/linux-mtd/
>
>

Re: [PATCH] CFI: prevent the false software timeouts on writes

[Sergei Shtylylov]

Hail you!

> yeah it works with XIP! :)
> I guess you can ask for commit now if no one objects.

    So, I beseech you sirs to have a mercy on my patch, and commit it with 
least possible delay.

God speed.

  > Vitaly Wool wrote:
> 
>> Hi Sergey,
>>
>> I'm going to verify if this works with XIP and get back to you.
>>
>> Best regards,
>>   Vitaly
>>
>> Sergei Shtylylov wrote:
>>
>>> Hello.
>>>
>>>       We've noticed that sometimes "MTD do_write_buffer(): software 
>>> timeout"
>>> message was printed out when writing to a Fujitsu NOR flash.
>>>       It turned out that this was because of a race in the timeout 
>>> handling
>>> do_write_buffer(). A small timeout of (HZ / 1000) + 1 is used there, and
>>> sometimes if the timer interrupt handling takes more than one or even 
>>> two
>>> jiffies (which is 1-2 ms with HZ == 1000) and that interrupt happens 
>>> just
>>> after chip_ready() call, the driver bails out from a ready polling loop
>>> despite the chip has actually become ready while all those interrupts 
>>> were
>>> handled. To deal with this issue, extra check for chip ready is 
>>> neccessary on
>>> timeout expiration (and the checks should better be reordered).
>>>       As do_write_oneword() uses the same approach, it needs to also 
>>> be changed.
>>>
>>> Signed-off-by: Konstantin Baidarov <kbaidarov@ru.mvista.com>
>>> Signed-off-by: Sergei Shtylyov <sshtylyov@ru.mvista.com>
>>>
>>>
>>>
>>>
>>> ------------------------------------------------------------------------
>>>
>>> Index: drivers/mtd/chips/cfi_cmdset_0002.c
>>> ===================================================================
>>> RCS file: /home/cvs/mtd/drivers/mtd/chips/cfi_cmdset_0002.c,v
>>> retrieving revision 1.120
>>> diff -a -u -p -r1.120 cfi_cmdset_0002.c
>>> --- drivers/mtd/chips/cfi_cmdset_0002.c    20 Jul 2005 21:01:13 
>>> -0000    1.120
>>> +++ drivers/mtd/chips/cfi_cmdset_0002.c    25 Oct 2005 15:38:44 -0000
>>> @@ -1014,16 +1014,16 @@ static int __xipram do_write_oneword(str
>>>             continue;
>>>         }
>>>
>>> -        if (chip_ready(map, adr))
>>> -            break;
>>> -
>>> -        if (time_after(jiffies, timeo)) {
>>> +        if (time_after(jiffies, timeo) && !chip_ready(map, adr)){
>>>             xip_enable(map, chip, adr);
>>>             printk(KERN_WARNING "MTD %s(): software timeout\n", 
>>> __func__);
>>>             xip_disable(map, chip, adr);
>>> -                        break;
>>> +            break;
>>>         }
>>>
>>> +        if (chip_ready(map, adr))
>>> +            break;
>>> +
>>>         /* Latency issues. Drop the lock, wait a while and retry */
>>>         UDELAY(map, chip, adr, 1);
>>>     }
>>> @@ -1275,13 +1275,13 @@ static int __xipram do_write_buffer(stru
>>>             continue;
>>>         }
>>>
>>> +        if (time_after(jiffies, timeo) && !chip_ready(map, adr))
>>> +            break;
>>> +
>>>         if (chip_ready(map, adr)) {
>>>             xip_enable(map, chip, adr);
>>>             goto op_done;
>>>         }
>>> -            -        if( time_after(jiffies, timeo))
>>> -            break;
>>>
>>>         /* Latency issues. Drop the lock, wait a while and retry */
>>>         UDELAY(map, chip, adr, 1);