Hello Brian Norris,
I'd like you to do a code review. Please visit
https://review.coreboot.org/22508
to review the following change.
Change subject: linux_mtd: do reads in eraseblock-sized chunks ......................................................................
linux_mtd: do reads in eraseblock-sized chunks
It's probably not the best idea to try to do an 8MB read in one syscall. Theoretically, this should work; but MTD just relies on the SPI driver to deliver the whole read in one transfer, and many SPI drivers haven't been tested well with large transfer sizes.
I'd consider this a workaround, but it's still good to have IMO.
BUG=chrome-os-partner:53215 TEST=boot kevin; `flashrom --read ...` TEST=check for performance regression on oak BRANCH=none
Change-Id: I3a8e160795cd24ec1851c00394892536a2a9d0c7 Signed-off-by: Brian Norris briannorris@chromium.org Reviewed-on: https://chromium-review.googlesource.com/344006 Reviewed-by: David Hendricks dhendrix@chromium.org --- M linux_mtd.c 1 file changed, 15 insertions(+), 6 deletions(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/08/22508/1
diff --git a/linux_mtd.c b/linux_mtd.c index f09c572..7d1e286 100644 --- a/linux_mtd.c +++ b/linux_mtd.c @@ -187,15 +187,26 @@ static int linux_mtd_read(struct flashctx *flash, uint8_t *buf, unsigned int start, unsigned int len) { + unsigned int eb_size = flash->chip->block_erasers[0].eraseblocks[0].size; + unsigned int i; + if (lseek(dev_fd, start, SEEK_SET) != start) { msg_perr("Cannot seek to 0x%06x: %s\n", start, strerror(errno)); return 1; }
- if (read(dev_fd, buf, len) != len) { - msg_perr("Cannot read 0x%06x bytes at 0x%06x: %s\n", - start, len, strerror(errno)); - return 1; + for (i = 0; i < len; ) { + /* Try to align reads to eraseblock size */ + unsigned int step = eb_size - ((start + i) % eb_size); + step = min(step, len - i); + + if (read(dev_fd, buf + i, step) != step) { + msg_perr("Cannot read 0x%06x bytes at 0x%06x: %s\n", + step, start + i, strerror(errno)); + return 1; + } + + i += step; }
return 0; @@ -265,8 +276,6 @@ }
static struct opaque_master opaque_master_linux_mtd = { - /* FIXME: Do we need to change these (especially write) as per - * page size requirements? */ .max_data_read = MAX_DATA_UNSPECIFIED, .max_data_write = MAX_DATA_UNSPECIFIED, .probe = linux_mtd_probe,