Concurrency erase/write on the same AMD chip with not aligned words

Concurrency erase/write on the same AMD chip with not aligned words

[Stefano Babic]

Hi all,

I have seen a problem when I am writing not-aligned words on AMD chips
while on the same chip another process starts an erase procedure for
other sectors or a different MTD device. 

I have tested with the cfi_cmdset_0002.c v.1.75 and I have found the
problem in the cfi_amstd_write() function.

In fact, when we want to write less that CFIDEV_BUSWIDTH bytes, the
function performs a read/modify/write on the flash with the
instructions:

        if (ofs & (CFIDEV_BUSWIDTH-1)) {
		.............
                map->copy_from(map, tmp_buf, ....

The flash is directly read using the copy_from() function. However, if
another process is currently erasing the same chip (even if it erases on
a different MTD device), we are not allowed to read from flash because
we are reading only the status of the internal state machine (DQ7, DQ6,
DQ5). The resulting read value is wrong and then the write function has
no success (a wrong value is written).

I set the check with the cfi status before the copy_from() as usual with
(mutex omissis):

        if (cfi->chips[chipnum].state != FL_READY) {
		.....

to be sure that the chip is not in FL_ERASING and everything is working
fine. 

Has anybody had the same problem ? 


stefano

-- 

Re: Concurrency erase/write on the same AMD chip with not aligned words

[David Woodhouse]

On Tue, 2003-08-05 at 11:12, Stefano Babic wrote:
> Hi all,
> 
> I have seen a problem when I am writing not-aligned words on AMD chips
> while on the same chip another process starts an erase procedure for
> other sectors or a different MTD device. 

 <...>

> I set the check with the cfi status before the copy_from() as usual with
> (mutex omissis):
> 
>         if (cfi->chips[chipnum].state != FL_READY) {
> 		.....
> 
> to be sure that the chip is not in FL_ERASING and everything is working
> fine. 
> 
> Has anybody had the same problem ? 

I haven't seen it personally, but it definitely looks like you're right.
Please could you send a patch?

-- 
dwmw2

Re: Concurrency erase/write on the same AMD chip with not aligned words

[Stefano Babic]

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

On Thu, 2003-08-07 at 12:36, David Woodhouse wrote:
> 
> I haven't seen it personally, but it definitely looks like you're right.
> Please could you send a patch?

Hi David,

I send you the patch file for the cfi_cmdset_0002.c .

stefano



[-- Attachment #2: patch_cficmdset_0002 --]
[-- Type: text/x-patch, Size: 13411 bytes --]

diff -Naur mtd-snapshot-2003071/cfi_cmdset_0002.c new/cfi_cmdset_0002.c
--- mtd-snapshot-2003071/cfi_cmdset_0002.c	Fri Aug  8 08:31:20 2003
+++ new/cfi_cmdset_0002.c	Fri Aug  8 08:32:08 2003
@@ -238,7 +238,7 @@
 		break;
 
 	default:
-	        printk(KERN_WARNING "Unsupported buswidth\n");
+	        printk(KERN_WARNING "Unsupported buswidth %d\n",CFIDEV_BUSWIDTH);
 		goto setup_err;
 		break;
 	}
@@ -468,10 +468,11 @@
 static int do_write_oneword(struct map_info *map, struct flchip *chip, unsigned long adr, cfi_word datum, int fast)
 {
 	unsigned long timeo = jiffies + HZ;
-	unsigned int oldstatus, status, prev_oldstatus, prev_status;
-	unsigned int dq6;
 	struct cfi_private *cfi = map->fldrv_priv;
 
+	cfi_word oldstatus, status, prev_oldstatus, prev_status;
+	cfi_word dq6;
+
     /* We use a 1ms + 1 jiffies generic timeout for writes (most devices have
        a max write time of a few hundreds usec). However, we should use the
        maximum timeout value given by the chip at probe time instead. 
@@ -484,7 +485,7 @@
 	DECLARE_WAITQUEUE(wait, current);
 	int ret = 0;
 	int ta = 0;
-
+        
  retry:
 	cfi_spin_lock(chip->mutex);
 
@@ -512,7 +513,12 @@
 	chip->state = FL_WRITING;
 
 	adr += chip->start;
-	DEBUG( MTD_DEBUG_LEVEL3, "MTD %s(): WRITE 0x%.8lx(0x%.8x)\n",
+        status = cfi_read(map, adr);
+        
+	DEBUG( MTD_DEBUG_LEVEL3, "MTD %s(): READ 0x%.8lx(0x%.8x)\n",
+	       __func__, adr, status );
+
+      	DEBUG( MTD_DEBUG_LEVEL3, "MTD %s(): WRITE 0x%.8lx(0x%.8x)\n",
 	       __func__, adr, datum );
 
 	ENABLE_VPP(map);
@@ -557,6 +563,7 @@
 	 * - Thayne Harbaugh
 	 */
 	dq6 = CMD(1<<6);
+
 	/* See comment above for timeout value. */
 	timeo = jiffies + uWriteTimeout; 
 		
@@ -591,6 +598,7 @@
 		} else 
 			udelay(1);
 
+
 		oldstatus = cfi_read( map, adr );
 		status = cfi_read( map, adr );
 		DEBUG( MTD_DEBUG_LEVEL3, "MTD %s(): Check 0x%.8x 0x%.8x\n",
@@ -623,13 +631,13 @@
 	}
 
 	if ( ta ) {
-		int dq5mask = ( ( status ^ oldstatus ) & dq6 ) >> 1;
+		cfi_word dq5mask = ( ( status ^ oldstatus ) & dq6 ) >> 1;
 		if ( status & dq5mask ) {
 			/* dq5 asserted - decode interleave chips */
 			printk( KERN_WARNING
-				"MTD %s(): FLASH internal timeout: 0x%.8x\n",
+				"MTD %s(): FLASH internal timeout: 0x%.8x 0x%.8x 0x%.8x\n",
 				__func__,
-				status & dq5mask );
+				status & dq5mask, status, datum );
 		} else {
 			printk( KERN_WARNING
 				"MTD %s(): Software timed out during write.\n",
@@ -675,6 +683,7 @@
 	int ret = 0;
 	int chipnum;
 	unsigned long ofs, chipstart;
+	DECLARE_WAITQUEUE(wait, current);
 
 	*retlen = 0;
 	if (!len)
@@ -683,7 +692,7 @@
 	chipnum = to >> cfi->chipshift;
 	ofs = to  - (chipnum << cfi->chipshift);
 	chipstart = cfi->chips[chipnum].start;
-
+        
 	/* If it's not bus-aligned, do the first byte write */
 	if (ofs & (CFIDEV_BUSWIDTH-1)) {
 		unsigned long bus_ofs = ofs & ~(CFIDEV_BUSWIDTH-1);
@@ -692,10 +701,36 @@
 		u_char tmp_buf[sizeof(cfi_word)];
 		cfi_word datum;
 
-		map->copy_from(map, tmp_buf, bus_ofs + cfi->chips[chipnum].start, CFIDEV_BUSWIDTH);
-		while (len && i < CFIDEV_BUSWIDTH)
-			tmp_buf[i++] = buf[n++], len--;
+ retry:
+	cfi_spin_lock(cfi->chips[chipnum].mutex);
 
+	if (cfi->chips[chipnum].state != FL_READY) {
+#if 0
+	        printk(KERN_DEBUG "Waiting for chip to write, status = %d\n", cfi->chips[chipnum].state);
+#endif
+		set_current_state(TASK_UNINTERRUPTIBLE);
+		add_wait_queue(&cfi->chips[chipnum].wq, &wait);
+                
+		cfi_spin_unlock(cfi->chips[chipnum].mutex);
+
+		schedule();
+		remove_wait_queue(&cfi->chips[chipnum].wq, &wait);
+#if 0
+		printk(KERN_DEBUG "Wake up to write:\n");
+		if(signal_pending(current))
+			return -EINTR;
+#endif
+		goto retry;
+	}
+        	
+		map->copy_from(map, tmp_buf, bus_ofs + cfi->chips[chipnum].start, CFIDEV_BUSWIDTH);
+                
+		cfi_spin_unlock(cfi->chips[chipnum].mutex);
+                
+		while (len && i < CFIDEV_BUSWIDTH) {
+			tmp_buf[i++] = buf[n++];
+                        len--;
+                }
 		if (cfi_buswidth_is_2()) {
 			datum = *(__u16*)tmp_buf;
 		} else if (cfi_buswidth_is_4()) {
@@ -794,7 +829,32 @@
 		u_char tmp_buf[sizeof(cfi_word)];
 		cfi_word datum;
 
+ retry1:
+	cfi_spin_lock(cfi->chips[chipnum].mutex);
+
+	if (cfi->chips[chipnum].state != FL_READY) {
+#if 0
+	        printk(KERN_DEBUG "Waiting for chip to write, status = %d\n", cfi->chips[chipnum].state);
+#endif
+		set_current_state(TASK_UNINTERRUPTIBLE);
+		add_wait_queue(&cfi->chips[chipnum].wq, &wait);
+                
+		cfi_spin_unlock(cfi->chips[chipnum].mutex);
+
+		schedule();
+		remove_wait_queue(&cfi->chips[chipnum].wq, &wait);
+#if 0
+		printk(KERN_DEBUG "Wake up to write:\n");
+		if(signal_pending(current))
+			return -EINTR;
+#endif
+		goto retry1;
+	}
+        	
 		map->copy_from(map, tmp_buf, ofs + cfi->chips[chipnum].start, CFIDEV_BUSWIDTH);
+                
+		cfi_spin_unlock(cfi->chips[chipnum].mutex);
+
 		while (len--)
 			tmp_buf[i++] = buf[n++];
 
@@ -822,16 +882,17 @@
 
 static inline int do_erase_chip(struct map_info *map, struct flchip *chip)
 {
-	unsigned int oldstatus, status, prev_oldstatus, prev_status;
-	unsigned int dq6;
 	unsigned long timeo = jiffies + HZ;
 	unsigned long int adr;
 	struct cfi_private *cfi = map->fldrv_priv;
 	DECLARE_WAITQUEUE(wait, current);
 	int ret = 0;
 	int ta = 0;
-	cfi_word ones = 0;
 
+	cfi_word status, prev_status;
+	cfi_word dq7, dq5;
+	cfi_word ones = 0;
+        
  retry:
 	cfi_spin_lock(chip->mutex);
 
@@ -879,15 +940,14 @@
 	 * command to the chip.
 	 */
 	/* see comments in do_write_oneword */
-	dq6 = CMD(1<<6);
+	dq7 = CMD(1<<7);
+	dq5 = CMD(1<<5);
 
-	oldstatus = cfi_read(map, adr);
 	status = cfi_read(map, adr);
-	DEBUG( MTD_DEBUG_LEVEL3, "MTD %s(): Check 0x%.8x 0x%.8x\n",
-	       __func__, oldstatus, status );
-
-	while( ( ( status ^ oldstatus ) & dq6 )
-	       && ! ( ta = time_after(jiffies, timeo) ) ) {
+        
+	while( ((status & dq7) != dq7) && 
+	       ((status & dq5) != dq5) &&
+	       ! ( ta = time_after(jiffies, timeo) ) ) {
 		int wait_reps;
 
 		/* an initial short sleep */
@@ -916,8 +976,9 @@
 
 		/* Busy wait for 1/10 of a milisecond */
 		for(wait_reps = 0;
-		    (wait_reps < 100)
-			    && ( ( status ^ oldstatus ) & dq6 );
+		    (wait_reps < 100) &&
+		    ((status & dq7) != dq7) && 
+		    ((status & dq5) != dq5);
 		    wait_reps++) {
 			
 			/* Latency issues. Drop the lock, wait a while and retry */
@@ -926,23 +987,17 @@
 			cfi_udelay(1);
 
 			cfi_spin_lock(chip->mutex);
-			oldstatus = cfi_read(map, adr);
 			status = cfi_read(map, adr);
-			DEBUG( MTD_DEBUG_LEVEL3, "MTD %s(): Check 0x%.8x 0x%.8x\n",
-			       __func__, oldstatus, status );
 		}
-		oldstatus = cfi_read(map, adr);
 		status = cfi_read(map, adr);
 		DEBUG( MTD_DEBUG_LEVEL3, "MTD %s(): Check 0x%.8x 0x%.8x\n",
-		       __func__, oldstatus, status );
+		       __func__, status );
 	}
 
-	prev_oldstatus = oldstatus;
 	prev_status = status;
-	oldstatus = cfi_read(map, adr);
 	status = cfi_read(map, adr);
 	DEBUG( MTD_DEBUG_LEVEL3, "MTD %s(): Check 0x%.8x 0x%.8x\n",
-	       __func__, oldstatus, status );
+	       __func__, status );
 
 	if ( cfi_buswidth_is_1() ) {
 		ones =  (__u8)~0;
@@ -950,18 +1005,20 @@
 		ones = (__u16)~0;
 	} else if ( cfi_buswidth_is_4() ) {
 		ones = (__u32)~0;
+        } else if ( cfi_buswidth_is_8() ) {
+	        ones = (__u64)~0;
 	} else {
-		printk(KERN_WARNING "Unsupported buswidth\n");
+		printk(KERN_WARNING "Unsupported buswidth %d\n",CFIDEV_BUSWIDTH);
 		goto erase_failed;
 	}
 	
-	if ( oldstatus == ones && status == ones ) {
+	if ( prev_status == ones && status == ones ) {
 		/* success - do nothing */
 		goto erase_done;
 	}
 
 	if ( ta ) {
-		int dq5mask = ( ( status ^ oldstatus ) & dq6 ) >> 1;
+		cfi_word dq5mask = ( ( status ^ prev_status ) & dq7 ) >> 2;
 		if ( status & dq5mask ) {
 			/* dq5 asserted - decode interleave chips */
 			printk( KERN_WARNING
@@ -981,10 +1038,9 @@
 	       __func__ );
 
 	printk(KERN_WARNING
-	       "MTD %s(): 0x%.8lx(0x%.8x): 0x%.8x 0x%.8x 0x%.8x 0x%.8x\n",
+	       "MTD %s(): 0x%.8lx(0x%.8x): 0x%.8x 0x%.8x\n",
 	       __func__, adr, ones,
-	       prev_oldstatus, prev_status,
-	       oldstatus, status);
+	       prev_status, status);
 
  erase_failed:
 	ret = -EIO;
@@ -1003,15 +1059,17 @@
 
 static inline int do_erase_oneblock(struct map_info *map, struct flchip *chip, unsigned long adr)
 {
-	unsigned int oldstatus, status, prev_oldstatus, prev_status;
-	unsigned int dq6;
 	unsigned long timeo = jiffies + HZ;
 	struct cfi_private *cfi = map->fldrv_priv;
 	DECLARE_WAITQUEUE(wait, current);
 	int ret = 0;
 	int ta = 0;
+        int i;
+        
+	cfi_word status, prev_status;
+	cfi_word dq7, dq5;
 	cfi_word ones = 0;
-
+        
  retry:
 	cfi_spin_lock(chip->mutex);
 
@@ -1035,7 +1093,7 @@
 	chip->state = FL_ERASING;
 
 	adr += chip->start;
-	DEBUG( MTD_DEBUG_LEVEL3, "MTD %s(): ERASE 0x%.8lx\n",
+	DEBUG( MTD_DEBUG_LEVEL3, "MTD %s(): ERASE 0x%.8x 0x%.8x\n",
 	       __func__, adr );
 
 	ENABLE_VPP(map);
@@ -1044,7 +1102,6 @@
 	cfi_send_gen_cmd(0x80, cfi->addr_unlock1, chip->start, map, cfi, CFI_DEVICETYPE_X8, NULL);
 	cfi_send_gen_cmd(0xAA, cfi->addr_unlock1, chip->start, map, cfi, CFI_DEVICETYPE_X8, NULL);
 	cfi_send_gen_cmd(0x55, cfi->addr_unlock2, chip->start, map, cfi, CFI_DEVICETYPE_X8, NULL);
-
 	cfi_write(map, CMD(0x30), adr);
 	
 	timeo = jiffies + (HZ*20);
@@ -1055,19 +1112,21 @@
 	 * The toggling stops as soon as the procedure is completed.
 	 *
 	 * If the process has gone on for too long on the chip bit 5 gets.
-	 * After bit5 is set you can kill the operation by sending a reset
+	 * After bit5 is set you can kill the operation by Wackysending a reset
 	 * command to the chip.
 	 */
 	/* see comments in do_write_oneword */
-	dq6 = CMD(1<<6);
+	dq7 = CMD(1<<7);
+	dq5 = CMD(1<<5);
 
-	oldstatus = cfi_read(map, adr);
-	status = cfi_read(map, adr);
-	DEBUG( MTD_DEBUG_LEVEL3, "MTD %s(): Check 0x%.8x 0x%.8x\n",
-	       __func__, oldstatus, status );
+	status    = cfi_read(map, adr);
 
-	while( ( ( status ^ oldstatus ) & dq6 )
-	       && ! ( ta = time_after(jiffies, timeo) ) ) {
+	DEBUG( MTD_DEBUG_LEVEL3, "MTD %s(): Check 0x%.8x\n",
+	       __func__, status );
+
+	while( ((status & dq7) != dq7) && 
+	       ((status & dq5) != dq5) &&
+		!( ta = time_after(jiffies, timeo) )) {
 		int wait_reps;
 
 		/* an initial short sleep */
@@ -1096,9 +1155,10 @@
 
 		/* Busy wait for 1/10 of a milisecond */
 		for(wait_reps = 0;
-		    (wait_reps < 100)
-			    && ( ( status ^ oldstatus ) & dq6 );
-		    wait_reps++) {
+		    (wait_reps < 100) &&
+			((status & dq7) != dq7) && 
+			((status & dq5) != dq5);
+			wait_reps++) {
 			
 			/* Latency issues. Drop the lock, wait a while and retry */
 			cfi_spin_unlock(chip->mutex);
@@ -1106,23 +1166,19 @@
 			cfi_udelay(1);
 		
 			cfi_spin_lock(chip->mutex);
-			oldstatus = cfi_read(map, adr);
 			status = cfi_read(map, adr);
 			DEBUG( MTD_DEBUG_LEVEL3, "MTD %s(): Check 0x%.8x 0x%.8x\n",
-			       __func__, oldstatus, status );
+			       __func__, status );
 		}
-		oldstatus = cfi_read(map, adr);
 		status = cfi_read(map, adr);
-		DEBUG( MTD_DEBUG_LEVEL3, "MTD %s(): Check 0x%.8x 0x%.8x\n",
-		       __func__, oldstatus, status );
+		DEBUG( MTD_DEBUG_LEVEL3, "MTD %s(): Check 0x%.8x\n",
+		       __func__, status );
 	}
+	prev_status     = status;
+	status          = cfi_read(map, adr);
 
-	prev_oldstatus = oldstatus;
-	prev_status = status;
-	oldstatus = cfi_read(map, adr);
-	status = cfi_read(map, adr);
-	DEBUG( MTD_DEBUG_LEVEL3, "MTD %s(): Check 0x%.8x 0x%.8x\n",
-	       __func__, oldstatus, status );
+	DEBUG( MTD_DEBUG_LEVEL3, "MTD %s(): Check 0x%.8x\n",
+	       __func__, status );
 
 	if ( cfi_buswidth_is_1() ) {
 		ones =  (__u8)~0;
@@ -1130,18 +1186,20 @@
 		ones = (__u16)~0;
 	} else if ( cfi_buswidth_is_4() ) {
 		ones = (__u32)~0;
+	} else if ( cfi_buswidth_is_8() ) {
+		ones = (__u64)~0;
 	} else {
-		printk(KERN_WARNING "Unsupported buswidth\n");
+		printk(KERN_WARNING "Unsupported buswidth %d\n",CFIDEV_BUSWIDTH);
 		goto erase_failed;
 	}
 
-	if ( oldstatus == ones && status == ones ) {
+	if ( prev_status == ones && status == ones ) {
 		/* success - do nothing */
 		goto erase_done;
 	}
 
 	if ( ta ) {
-		int dq5mask = ( ( status ^ oldstatus ) & dq6 ) >> 1;
+		cfi_word dq5mask = ( ( status ^ prev_status ) & dq7 ) >> 2;
 		if ( status & dq5mask ) {
 			/* dq5 asserted - decode interleave chips */
 			printk( KERN_WARNING
@@ -1161,10 +1219,9 @@
 	       __func__ );
 
 	printk(KERN_WARNING
-	       "MTD %s(): 0x%.8lx(0x%.8x): 0x%.8x 0x%.8x 0x%.8x 0x%.8x\n",
+	       "MTD %s(): 0x%.8lx(0x%.8x): 0x%.8x 0x%.8x\n",
 	       __func__, adr, ones,
-	       prev_oldstatus, prev_status,
-	       oldstatus, status);
+	       prev_status, status);
 
  erase_failed:
 	ret = -EIO;
@@ -1174,6 +1231,7 @@
 
  erase_done:
 	DISABLE_VPP(map);
+        
 	chip->state = FL_READY;
 	wake_up(&chip->wq);
 	cfi_spin_unlock(chip->mutex);

Re: Concurrency erase/write on the same AMD chip with not aligned words

[Thayne Harbaugh]

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

On Fri, 2003-08-08 at 00:42, Stefano Babic wrote:
> On Thu, 2003-08-07 at 12:36, David Woodhouse wrote:
> > 
> > I haven't seen it personally, but it definitely looks like you're right.
> > Please could you send a patch?
> 
> Hi David,
> 
> I send you the patch file for the cfi_cmdset_0002.c .
> 
> stefano

I looked through the patch and it might cause problems with AMD
compatible chips that do not support suspend/resume concurrent writes.

There needs to be a mechanism for dealing with supported/unsupported
features in cfi_cmdset_0002.  One of these days I'll finish my current
ECC project and I'll be able to put time back into this.  (let's hope
it's real soon).


-- 
Thayne Harbaugh
Linux Networx

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

Re: Concurrency erase/write on the same AMD chip with not aligned words

[Thayne Harbaugh]

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

On Tue, 2003-08-05 at 04:12, Stefano Babic wrote:
> Hi all,
> 
> I have seen a problem when I am writing not-aligned words on AMD chips
> while on the same chip another process starts an erase procedure for
> other sectors or a different MTD device. 

Stefano,

The fix for this is in cfi_cmdset_0002.c v1.87.  I had to pick through
your patch a bit, but I think all the relevant pieces are there.  Will
you test it please?

Thanks for your help and the patch.

-- 
Thayne Harbaugh
Linux Networx

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