CFI: prevent the false software timeouts on writes

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 <[email protected]>

Signed-off-by Sergei Shtylyov <[email protected]>

------------------------------------------------------------------------

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

mtd/chips/cfi_cmdset_0002.c +8 -9

@@ -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/

Hi Sergey,

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

Best regards,
Vitaly

Quoted 80 lines · Sergei Shtylylov

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 <[email protected]>

Signed-off-by Sergei Shtylyov <[email protected]>

------------------------------------------------------------------------

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

mtd/chips/cfi_cmdset_0002.c +5 -5

@@ -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);

    }

mtd/chips/cfi_cmdset_0002.c +3 -3

@@ -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);
 
 
 
 
 
 
------------------------------------------------------------------------

Quoted 5 lines

______________________________________________________
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/

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

Quoted 81 lines · Vitaly Wool

Hi Sergey,

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

Best regards,
Vitaly

Quoted 72 lines · Sergei Shtylylov

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 <[email protected]>

Signed-off-by Sergei Shtylyov <[email protected]>

------------------------------------------------------------------------

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

mtd/chips/cfi_cmdset_0002.c +5 -5

@@ -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);

    }

mtd/chips/cfi_cmdset_0002.c +3 -2

@@ -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);