do_erase_oneblock failing to detect lock-bit failure

do_erase_oneblock failing to detect lock-bit failure

[Dan Eisenhut]

(Corporate email didn't do references, switching to something nicer)
Now that I've verified my lock-bits are setting and clearing okay, I'm
trying to test that erase fails properly when a block is locked.

We have a strange setup on our custom board.  Data lines 0-7 are swapped
with lines 8-15 going into the CFI flash chip.  This is what the
hardware guys are calling "byte-lane swapping" or reordering.  To
compensate for this, I had to enable CONFIG_MTD_CFI_LE_BYTE_SWAP.  This
appears to be working for reading and writing data.  (Is there a better
way to handle this?)

But because of this, do_erase_oneblock fails to handle a failure when a
block is locked.  Starting at line 1372 of cfi_cmdset_0001.c, cfi_read
returns a value of 0xa200 into status (a short) indicating ready, error
in block erasure, and block lock-bit detected.  chipstatus (a unsigned
char) is assigned the value 0x00 since the 0xa2 portion is chopped off
in the implicit type conversion.  My interleave is 1 so chipstatus is
not modified (only one chip for this bank).

Then when it gets down to checking for the protection bit it compares
against "chipstatus & 0x20" instead of "status & CMD(0x20)" so it
automatically fails.  Eventually a zero is returned indicating erase
success.

Is byte-lane swapping common?  Wouldn't this code fail for someone
without byte-lane swapping but requiring little endian enabled?  By
changing the if statements with (chipstatus & 0xNN) with (status &
CMD(0xNN)) appears to correct my problem, but I sure this is not the
best solution.

Dan

Line#1372 - cfi_cmdset_0001.c
-----------------------------
status = cfi_read(map, adr);

/* check for lock bit */
if (status & CMD(0x3a)) {
        unsigned char chipstatus = status;
        if (status != CMD(status & 0xff)) {
                int i;
                for (i = 1; i<CFIDEV_INTERLEAVE; i++) {
                              chipstatus |= status >> (cfi->device_type
* 8);
                }
                printk(KERN_WARNING "Status is not identical for all
chips: 0x%llx. Merging to give 0x%02x\n", (__u64)status, chipstatus);
        }
        /* Reset the error bits */
        cfi_write(map, CMD(0x50), adr);
        cfi_write(map, CMD(0x70), adr);

        if ((chipstatus & 0x30) == 0x30) {
                printk(KERN_NOTICE "Chip reports improper command
sequence: status 0x%llx\n", (__u64)status);
                ret = -EIO;
        } else if (chipstatus & 0x02) {
                /* Protection bit set */
                ret = -EROFS;
        } else if ....

Re: do_erase_oneblock failing to detect lock-bit failure

[David Woodhouse]

On Thu, 2004-03-25 at 07:30 -0600, Dan Eisenhut wrote:
> Is byte-lane swapping common?  

Not really. It's generally a symptom of hardware engineers who really
don't understand the problem, and think it might help.

> Wouldn't this code fail for someone
> without byte-lane swapping but requiring little endian enabled?  By
> changing the if statements with (chipstatus & 0xNN) with (status &
> CMD(0xNN)) appears to correct my problem, but I sure this is not the
> best solution.

You made me think about it again and now my head hurts. I suspect that
we should be using cfi_read_query() to read the status bits, not
cfi_read(). 


-- 
dwmw2

Re: do_erase_oneblock failing to detect lock-bit failure

[Dan Eisenhut]

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

On Thu, 2004-03-25 at 07:40, David Woodhouse wrote:
> On Thu, 2004-03-25 at 07:30 -0600, Dan Eisenhut wrote:
> > Is byte-lane swapping common?  
> 
> Not really. It's generally a symptom of hardware engineers who really
> don't understand the problem, and think it might help.

I looked into why they did this and apparently it was a recommendation
from Motorola.  We are using a MPC8270 processor.

> > Wouldn't this code fail for someone
> > without byte-lane swapping but requiring little endian enabled?  By
> > changing the if statements with (chipstatus & 0xNN) with (status &
> > CMD(0xNN)) appears to correct my problem, but I sure this is not the
> > best solution.
> 
> You made me think about it again and now my head hurts. I suspect that
> we should be using cfi_read_query() to read the status bits, not
> cfi_read(). 

I tried changing cfi_read to cfi_read_query, which sets status to
0x00a2.  But the immediate next check of "if (status & CMD(0x3a))" fails
since CMD(0x3a) resolves to 0x3a00.

Looking at it a little more, I made a couple changes which corrects the
problem for me.  (See attached diff)  How would this effect other boards
that are currently working on the existing code?  I'm not sure how
boards that do any kind of swapping were working in the first place. 
Maybe just never tested the return value on an erase to a locked block?

Dan

[-- Attachment #2: mtd_cfi_erase.diff --]
[-- Type: text/x-patch, Size: 1272 bytes --]

diff -uNr clean/drivers/mtd/chips/cfi_cmdset_0001.c mtd/drivers/mtd/chips/cfi_cmdset_0001.c
--- clean/drivers/mtd/chips/cfi_cmdset_0001.c	2004-03-26 10:33:48.000000000 -0600
+++ mtd/drivers/mtd/chips/cfi_cmdset_0001.c	2004-03-26 10:34:26.000000000 -0600
@@ -1373,8 +1373,8 @@
 
 	/* check for lock bit */
 	if (status & CMD(0x3a)) {
-		unsigned char chipstatus = status;
-		if (status != CMD(status & 0xff)) {
+		unsigned char chipstatus = cfi_to_cpu(status);
+		if (status != CMD(cfi_to_cpu(status) & 0xff)) {
 			int i;
 			for (i = 1; i<CFIDEV_INTERLEAVE; i++) {
 				      chipstatus |= status >> (cfi->device_type * 8);
diff -uNr clean/include/linux/mtd/cfi.h mtd/include/linux/mtd/cfi.h
--- clean/include/linux/mtd/cfi.h	2004-03-26 10:33:50.000000000 -0600
+++ mtd/include/linux/mtd/cfi.h	2004-03-26 10:35:42.000000000 -0600
@@ -473,6 +473,21 @@
 	}
 }
 
+static inline __u8 cfi_to_cpu(cfi_word val)
+{
+	if (cfi_buswidth_is_1()) {
+		return val;
+	} else if (cfi_buswidth_is_2()) {
+		return cfi16_to_cpu(val);
+	} else if (cfi_buswidth_is_4()) {
+		return cfi32_to_cpu(val);
+	} else if (cfi_buswidth_is_8()) {
+		return cfi64_to_cpu(val);
+	} else {
+		return 0;
+	}
+}
+
 static inline void cfi_udelay(int us)
 {
 #if LINUX_VERSION_CODE >= KERNEL_VERSION(2,2,0)