do_erase_oneblock failing to detect lock-bit failure

(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 ....

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

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

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

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

diff -uNr clean/drivers/mtd/chips/cfi_cmdset_0001.c mtd/drivers/mtd/chips/cfi_cmdset_0001.c
@@ -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
@@ -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)