[flashrom-gerrit] Change in flashrom[master]: linux_mtd: make read/write loop chunks consistent, and documented

[David Hendricks (Code Review)]

Hello Brian Norris,

I'd like you to do a code review. Please visit

https://review.coreboot.org/22512

to review the following change.

Change subject: linux_mtd: make read/write loop chunks consistent, and documented
......................................................................

linux_mtd: make read/write loop chunks consistent, and documented

Theoretically, there should be no maximum size for the read() and
write() syscalls on an MTD (well, except for the size of the entire
device). But practical concerns (i.e., bugs) have meant we don't quite
do this.

For reads:
Bug https://b/35573113 shows that some SPI-based MTD drivers don't yet
handle very large transactions. So we artificially limit this to
block-sized chunks.

For writes:
It's not clear there is a hard limit. Some drivers will already split
large writes into smaller chunks automatically. Others don't do any
splitting. At any rate, using *small* chunks can actually be a problem
for some devices (b:35104688), as they get worse performance (doing an
internal read/modify/write). This could be fixed in other ways by
advertizing their true "write chunk size" to user space somehow, but
this isn't so easy.

As a simpler fix, we can just increase the loop increment to match the
read loop. Per David, the original implementation (looping over page
chunks) was just being paranoid.

So this patch:
 * clarifies comments in linux_mtd_read(), to note that the chunking is
   somewhat of a hack that ideally can be fixed (with bug reference)
 * simplifies the linux_mtd_write() looping to match the structure in
   linux_mtd_read(), including dropping several unnecessary seeks, and
   correcting the error messages (they referred to "reads" and had the
   wrong parameters)
 * change linux_mtd_write() to align its chunks to eraseblocks, not page
   sizes

Note that the "->page_size" parameter is still somewhat ill-defined, and
only set by the upper layers for "opaque" flash. And it's not actually
used in this driver now. If we could figure out what we really want to
use it for, then we could try to set it appropriately.

BRANCH=none
BUG=b:35104688
TEST=various flashrom tests on Kevin
TEST=Reading and writing to flash works on our zaius machines over mtd

Change-Id: I551bb85269c854f6888c6bfad8031b14fcf10273
Signed-off-by: Brian Norris 
Reviewed-on: https://chromium-review.googlesource.com/505409
Reviewed-by: David Hendricks 
Reviewed-by: Martin Roth 
Reviewed-by: William Kennington 
---
M linux_mtd.c
1 file changed, 29 insertions(+), 24 deletions(-)

git pull ssh://review.coreboot.org:29418/flashrom refs/changes/12/22512/1

diff --git a/linux_mtd.c b/linux_mtd.c
index 9320bd1..5d67d27 100644
--- a/linux_mtd.c
+++ b/linux_mtd.c
@@ -199,7 +199,12 @@
    }

for (i = 0; i < len; ) {
-		/* Try to align reads to eraseblock size */
+		/*
+		 * Try to align reads to eraseblock size.
+		 * FIXME: Shouldn't actually be necessary, but not all MTD
+		 * drivers handle arbitrary large reads well. See, for example,
+		 * https://b/35573113
+		 */
    	unsigned int step = eb_size - ((start + i) % eb_size);
    	step = min(step, len - i);

@@ -215,39 +220,39 @@
    return 0;
 }

-/* this version assumes we must divide the write request into pages ourselves */
+/* this version assumes we must divide the write request into chunks ourselves */
 static int linux_mtd_write(struct flashctx *flash, const uint8_t *buf,
    			unsigned int start, unsigned int len)
 {
-	unsigned int page;
-	unsigned int chunksize, page_size;
-
-	chunksize = page_size = flash->chip->page_size;
+	unsigned int chunksize = flash->chip->block_erasers[0].eraseblocks[0].size;
+	unsigned int i;

if (!mtd_device_is_writeable)
    	return 1;

-	for (page = start / page_size;
-		page <= (start + len - 1) / page_size; page++) {
-		unsigned int i, starthere, lenhere;
+	if (lseek(dev_fd, start, SEEK_SET) != start) {
+		msg_perr("Cannot seek to 0x%06x: %s\n", start, strerror(errno));
+		return 1;
+	}

-		starthere = max(start, page * page_size);
-		lenhere = min(start + len, (page + 1) * page_size) - starthere;
-		for (i = 0; i < lenhere; i += chunksize) {
-			unsigned int towrite = min(chunksize, lenhere - i);
+	/*
+	 * Try to align writes to eraseblock size. We want these large enough
+	 * to give MTD room for optimizing performance.
+	 * FIXME: Shouldn't need to divide this up at all, but not all MTD
+	 * drivers handle arbitrary large writes well. See, for example,
+	 * https://b/35573113
+	 */
+	for (i = 0; i < len; ) {
+		unsigned int step = chunksize - ((start + i) % chunksize);
+		step = min(step, len - i);

-			if (lseek(dev_fd, starthere, SEEK_SET) != starthere) {
-				msg_perr("Cannot seek to 0x%06x: %s\n",
-						start, strerror(errno));
-				return 1;
-			}
-
-			if (write(dev_fd, &buf[starthere - start], towrite) != towrite) {
-				msg_perr("Cannot read 0x%06x bytes at 0x%06x: "
-					"%s\n", start, len, strerror(errno));
-				return 1;
-			}
+		if (write(dev_fd, buf + i, step) != step) {
+			msg_perr("Cannot write 0x%06x bytes at 0x%06x: %s\n",
+					step, start + i, strerror(errno));
+			return 1;
    	}
+
+		i += step;
    }

return 0;