mtd: cfi_cmdset_0002: cap the write-buffer chunk at 256 bytes on an 8-bit bus

The word count of the Write to Buffer command is a single bus word, so on an 8-bit bus a chunk can hold at most 256 bytes regardless of the buffer size the chip advertises.

A Micron M29EW (an x16 part wired in x8 mode) advertises a 512-byte write buffer. cfi_amdstd_write_buffers() used the full 512, CMD(511) truncated the count to 0xff on the way out, and the chip aborted the program on the 257th data byte. Every full-size chunk failed while the partial ones at the start of a write went through:

MTD do_write_buffer_wait(): software timeout, address:0x03278bff.
jffs2: Write of 4164 bytes at 0x02c789b4 failed. returned -5, retlen 68
jffs2: No space for garbage collection. Aborting GC thread

Clamp MaxBufWriteSize in the write-buffer fixup when the bank is one byte wide, and keep mtd->writebufsize (computed before the fixups run) consistent with it.

The bug predates the git history, so there is no Fixes: tag.

---

@@ -283,6 +283,17 @@ static void fixup_use_write_buffers(struct mtd_info *mtd)
 		pr_debug("Using buffer write method\n");
 		mtd->_write = cfi_amdstd_write_buffers;
 	}
+
+	/*
+	 * The word count of the Write to Buffer command is a single bus
+	 * word, so on an 8-bit bus a chunk holds at most 256 bytes no
+	 * matter how large a buffer the chip advertises.
+	 */
+	if (map_bankwidth_is_1(map) && cfi->cfiq->MaxBufWriteSize > 8) {
+		cfi->cfiq->MaxBufWriteSize = 8;
+		mtd->writebufsize = cfi_interleave(cfi) <<
+				    cfi->cfiq->MaxBufWriteSize;
+	}
 }
 #endif /* !FORCE_WORD_WRITE */
 

2.47.0

The word count of the Write to Buffer command is a single bus word per device, so an x8 device can be told to program at most 256 bytes regardless of the buffer size it advertises.

A Micron M29EW (an x16 part wired in x8 mode) advertises a 512-byte write buffer. cfi_amdstd_write_buffers() used the full 512, CMD(511) truncated the count to 0xff on the way out, and the chip aborted the program on the 257th data byte. Every full-size chunk failed while the partial ones at the start of a write went through:

MTD do_write_buffer_wait(): software timeout, address:0x03278bff.
jffs2: Write of 4164 bytes at 0x02c789b4 failed. returned -5, retlen 68
jffs2: No space for garbage collection. Aborting GC thread

Clamp MaxBufWriteSize in the write-buffer fixup for x8 devices, and keep mtd->writebufsize (computed before the fixups run) consistent with it.

The bug predates the git history, so there is no Fixes: tag.

---

v2: test the device width (cfi->device_type) instead of the aggregate bus
width. map_bankwidth_is_1() is false for x8 devices interleaved on a wider bus, which have the same 256-byte ceiling: do_write_buffer() sends CMD(words - 1), and CMD() replicates that count into each device's lane, where it is truncated to 8 bits - so two x8 chips with a 512-byte buffer are told 255 words and are still sent 512 bytes each. device_type is also immune to map_bankwidth_is_1() compiling to a constant 0 when CONFIG_MTD_MAP_BANK_WIDTH_1 is off. Caught by the Sashiko review bot.

@@ -283,6 +283,21 @@ static void fixup_use_write_buffers(struct mtd_info *mtd)
 		pr_debug("Using buffer write method\n");
 		mtd->_write = cfi_amdstd_write_buffers;
 	}
+
+	/*
+	 * The word count of the Write to Buffer command is a single bus
+	 * word per device, so an x8 device can be told to program at most
+	 * 256 bytes however large a buffer it advertises - including when
+	 * several of them are interleaved on a wider bus, where CMD()
+	 * replicates the count into each device's lane and it is truncated
+	 * there.
+	 */
+	if (cfi->device_type == CFI_DEVICETYPE_X8 &&
+	    cfi->cfiq->MaxBufWriteSize > 8) {
+		cfi->cfiq->MaxBufWriteSize = 8;
+		mtd->writebufsize = cfi_interleave(cfi) <<
+				    cfi->cfiq->MaxBufWriteSize;
+	}
 }
 #endif /* !FORCE_WORD_WRITE */
 

2.47.0

This sentence does not mean anything, I'm sorry.

failed? do you have hardware that actually triggered that issue?

That's incorrect. It indicates until when we need to backport it. So if it's the first Git commit, it's the first Git commit.

Hi Miquel,

Thanks for the review. v3 sent as a reply to v2.

Rewritten: the count goes out as a single bus word, CMD() replicates it into every device lane, and an x8 part reads it off eight data lines, so anything above 255 is truncated there.

Yes - a Cavium Octeon CN6335 board whose NOR is an M29EW strapped to x8. The log in the commit message is from it, and the clamp fixes it. v3 names the board.

Added. I checked the code is really there: 1da177e4c3f4 already sends CMD(words - 1) and already takes the chunk size from MaxBufWriteSize.

Vignesh is on To: for v3.

Thanks,
Orgad

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Ok, this matters a lot to me today, because we get dozens and dozens of hardening changes which do not always have an actual reality. If you fix a real problem you encountered, that's relevant.

You should use checkpatch.pl or b4's option --auto-to-cc to create the Cc list.

Thanks for the v3.

Miquèl

The Write to Buffer command takes the number of words to program minus one, and do_write_buffer() sends that count as a single bus word:

	map_write(map, CMD(words - 1), cmd_adr);

CMD() replicates the value into every device lane, so each device reads the count off its own data lines - eight of them on an x8 part. A count that does not fit in eight bits is truncated there, which limits one Write to Buffer to 256 words on an x8 device, however large a buffer the chip advertises in its CFI query.

The M29EW on a Cavium Octeon CN6335 board (an x16 part strapped to x8) advertises MaxBufWriteSize = 512 bytes, so cfi_amdstd_write_buffers() split the data into 512-byte chunks and do_write_buffer() sent a count of
511. The chip saw 511 & 0xff = 255, expected 256 bytes, and aborted the program when the 257th arrived. Chunks below 256 bytes - the leading partial chunk of a write - completed normally, so only the full-size ones failed:

MTD do_write_buffer_wait(): software timeout, address:0x03278bff.
jffs2: Write of 4164 bytes at 0x02c789b4 failed. returned -5, retlen 68
jffs2: No space for garbage collection. Aborting GC thread

Clamp MaxBufWriteSize for x8 devices in the write-buffer fixup, and recompute mtd->writebufsize, which cfi_cmdset_0002_setup() derives from MaxBufWriteSize before the fixups run.

The clamp deliberately keys off the device width rather than the part, even though the driver already recognises this one in is_m29ew(): the ceiling comes from the width of a device's data bus, not from the chip, so any x8 device advertising a larger buffer is told a count it cannot receive. The M29EW is only where it was found.

---

v3: rewrite the description - the v2 wording of the failure was not
comprehensible (Miquel). Name the board it was found on: the failure is not theoretical, the log above is from that board and the clamp fixes it. Add the Fixes: tag - do_write_buffer() already sent CMD(words - 1) in 1da177e4c3f4, so the first git commit is where the backport has to reach.

v2: test the device width (cfi->device_type) instead of the aggregate bus
width. map_bankwidth_is_1() is false for x8 devices interleaved on a wider bus, which have the same 256-byte ceiling: do_write_buffer() sends CMD(words - 1), and CMD() replicates that count into each device's lane, where it is truncated to 8 bits - so two x8 chips with a 512-byte buffer are told 255 words and are still sent 512 bytes each. device_type is also immune to map_bankwidth_is_1() compiling to a constant 0 when CONFIG_MTD_MAP_BANK_WIDTH_1 is off. Caught by the Sashiko review bot.

@@ -283,6 +283,21 @@ static void fixup_use_write_buffers(struct mtd_info *mtd)
 		pr_debug("Using buffer write method\n");
 		mtd->_write = cfi_amdstd_write_buffers;
 	}
+
+	/*
+	 * The word count of the Write to Buffer command is a single bus
+	 * word per device, so an x8 device can be told to program at most
+	 * 256 bytes however large a buffer it advertises - including when
+	 * several of them are interleaved on a wider bus, where CMD()
+	 * replicates the count into each device's lane and it is truncated
+	 * there.
+	 */
+	if (cfi->device_type == CFI_DEVICETYPE_X8 &&
+	    cfi->cfiq->MaxBufWriteSize > 8) {
+		cfi->cfiq->MaxBufWriteSize = 8;
+		mtd->writebufsize = cfi_interleave(cfi) <<
+				    cfi->cfiq->MaxBufWriteSize;
+	}
 }
 #endif /* !FORCE_WORD_WRITE */
 

2.47.0

Applied to mtd/next, thanks!

[1/1] mtd: cfi_cmdset_0002: cap the write-buffer chunk at 256 bytes on an x8 device

      commit: cfc5ebc9540e29f8f96db88bbfa261139e5afedb

Patche(s) should be available on mtd/linux.git and will be part of the next PR (provided that no robot complains by then).

Kind regards,
Miquèl

Commit cfc5ebc9540e ("mtd: cfi_cmdset_0002: cap the write-buffer chunk at 256 bytes on an x8 device") limits a Write to Buffer to 256 bytes when cfi->device_type is CFI_DEVICETYPE_X8, because do_write_buffer() sends the word count as a single bus word and an 8-bit data lane truncates it.

The M29EW that commit was written for is an x16 part strapped to x8. The probe reports it as

phys_mapped_flash: Found 1 x16 devices at 0x0 in 8-bit bank. Manufacturer ID 0x000089 Chip ID 0x00007e

so its device_type is CFI_DEVICETYPE_X16, the clamp does not apply, and every full 512-byte chunk still times out:

MTD do_write_buffer_wait(): software timeout, address:0x002203ff.

What limits the count is the width of each device's data lane, not the part's type. Test that instead: map_bankwidth(map) / cfi_interleave(cfi) is 1 for an x8 part, for x8 parts interleaved on a wider bus, and for an x16 part in an 8-bit bank.

On the board (Octeon CN6335, the same M29EW) writes of 1, 2, 32, 64 and 256 bytes to an erased partition read back correctly and 512, 1024, 2048 and 4096 failed with the timeout above; with this patch every size up to 64 KiB reads back correctly and the u-boot environment can be saved from Linux again.

---

@@ -286,13 +286,14 @@ static void fixup_use_write_buffers(struct mtd_info *mtd)
 
 	/*
 	 * The word count of the Write to Buffer command is a single bus
-	 * word per device, so an x8 device can be told to program at most
-	 * 256 bytes however large a buffer it advertises - including when
-	 * several of them are interleaved on a wider bus, where CMD()
-	 * replicates the count into each device's lane and it is truncated
-	 * there.
+	 * word per device, so a device on an 8-bit data lane can be told
+	 * to program at most 256 bytes however large a buffer it
+	 * advertises: an x8 part, x8 parts interleaved on a wider bus
+	 * (CMD() replicates the count into each device's lane and it is
+	 * truncated there), and an x16 part strapped to x8, which the
+	 * probe reports as an x16 device in an 8-bit bank.
 	 */
-	if (cfi->device_type == CFI_DEVICETYPE_X8 &&
+	if (map_bankwidth(map) / cfi_interleave(cfi) == 1 &&
 	    cfi->cfiq->MaxBufWriteSize > 8) {
 		cfi->cfiq->MaxBufWriteSize = 8;
 		mtd->writebufsize = cfi_interleave(cfi) <<

2.47.0

Hi Orgad,

It no longer applies, can you please rebase and resend?

Thanks,
Miquèl

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Hi Miquel,

Sorry about that. I resent it as v2, as a new thread this time instead of under the v3 mail of the first patch (which is already in your tree):

https://lore.kernel.org/all/[email protected]/

It applies cleanly here to mtd/next at 2f7fa8d90694; the file has not changed since cfc5ebc9540e. If it still fails for you, which tree and commit are you applying it to?

Thanks,
Orgad