May 2018 - flashrom-gerrit - mail.coreboot.org

Change in flashrom[master]: linux_mtd: Initial import
by David Hendricks (Code Review) 15 May '18

15 May '18

David Hendricks has posted comments on this change. ( https://review.coreboot.org/25706 ) Change subject: linux_mtd: Initial import ...................................................................... Patch Set 6: (3 comments) I'll hold off on a bit to see if Brian sees my e-mail and has any ideas https://review.coreboot.org/#/c/25706/4/linux_mtd.c File linux_mtd.c: https://review.coreboot.org/#/c/25706/4/linux_mtd.c@231 PS4, Line 231: perr( > Maybe there actually is an alignment requirement? In that case […] strace showed the fread() working, but I didn't see it try to do the fwrite() after the fseek(). I'm not sure what that means, maybe fwrite() just doesn't work on certain types of files? I also tried adjusting chunksize, and reversing the size and nmemb args, but didn't have any luck there. So I sent an e-mail to one of the MTD maintainers (briannorris) in case he has any ideas. (BTW - the error string I mentioned earlier might not be relevant since fwrite() doesn't set errno) Since the MTD write-protect functions use a few ioctl()s, I think it would make more sense to convert from file descriptor --> stream in the cases where fread() can be used. Does that sound good? https://review.coreboot.org/#/c/25706/5/linux_mtd.c File linux_mtd.c: https://review.coreboot.org/#/c/25706/5/linux_mtd.c@208 PS5, Line 208: if (fread(buf + i, step, 1, dev_fp) != 1) { > `step, 1`, i.e. reading one big chunk, might be better optimized in libc. Done https://review.coreboot.org/#/c/25706/5/linux_mtd.c@314 PS5, Line 314: > Nit, we know both `buf` and "nor" are zero-terminated. There is no […] Done -- To view, visit https://review.coreboot.org/25706 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I3d6bb282863a5cf69909e28a1fc752b35f1b9599 Gerrit-Change-Number: 25706 Gerrit-PatchSet: 6 Gerrit-Owner: David Hendricks <david.hendricks(a)gmail.com> Gerrit-Reviewer: Brian Norris <briannorris(a)chromium.org> Gerrit-Reviewer: David Hendricks <david.hendricks(a)gmail.com> Gerrit-Reviewer: Jonathan Neuschäfer <j.neuschaefer(a)gmx.net> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Patrick Rudolph <patrick.rudolph(a)9elements.com> Gerrit-Reviewer: Paul Menzel <paulepanter(a)users.sourceforge.net> Gerrit-Reviewer: Philipp Deppenwiese <zaolin.daisuki(a)gmail.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Comment-Date: Tue, 15 May 2018 07:25:17 +0000 Gerrit-HasComments: Yes Gerrit-HasLabels: No

1 0

Change in flashrom[master]: linux_mtd: Initial import
by build bot (Jenkins) (Code Review) 15 May '18

15 May '18

build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/25706 ) Change subject: linux_mtd: Initial import ...................................................................... Patch Set 6: Verified+1 Build Successful https://qa.coreboot.org/job/flashrom_gerrit/1074/ : SUCCESS https://qa.coreboot.org/job/flashrom-customrules/1295/ : SUCCESS -- To view, visit https://review.coreboot.org/25706 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I3d6bb282863a5cf69909e28a1fc752b35f1b9599 Gerrit-Change-Number: 25706 Gerrit-PatchSet: 6 Gerrit-Owner: David Hendricks <david.hendricks(a)gmail.com> Gerrit-Reviewer: Brian Norris <briannorris(a)chromium.org> Gerrit-Reviewer: David Hendricks <david.hendricks(a)gmail.com> Gerrit-Reviewer: Jonathan Neuschäfer <j.neuschaefer(a)gmx.net> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Patrick Rudolph <patrick.rudolph(a)9elements.com> Gerrit-Reviewer: Paul Menzel <paulepanter(a)users.sourceforge.net> Gerrit-Reviewer: Philipp Deppenwiese <zaolin.daisuki(a)gmail.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Comment-Date: Tue, 15 May 2018 07:23:43 +0000 Gerrit-HasComments: No Gerrit-HasLabels: Yes

1 0

Change in flashrom[master]: linux_mtd: Initial import
by David Hendricks (Code Review) 15 May '18

15 May '18

Hello Patrick Rudolph, Paul Menzel, Jonathan Neuschäfer, Philipp Deppenwiese, build bot (Jenkins), Nico Huber, Brian Norris, I'd like you to reexamine a change. Please visit https://review.coreboot.org/25706 to look at the new patch set (#6). Change subject: linux_mtd: Initial import ...................................................................... linux_mtd: Initial import This imports a series of patches from chromiumos for MTD support. The patches are squashed to ease review and original Change-Ids have been removed to avoid confusing Gerrit. There are a few changes to integrate the code: - Conflict resolution - Makefile changes - Remove file library usage from linux_mtd. We may revisit this and use it for other Linux interfaces later on. This consolidated patch is Signed-off-by: David Hendricks <dhendricks(a)fb.com> The first commit's message is: Initial MTD support This adds MTD support to flashrom so that we can read, erase, and write content on a NOR flash chip via MTD. BUG=chrome-os-partner:40208 BRANCH=none TEST=read, write, and erase works on Oak Signed-off-by: David Hendricks <dhendrix(a)chromium.org> Reviewed-on: https://chromium-review.googlesource.com/272983 Reviewed-by: Shawn N <shawnn(a)chromium.org> This is the 2nd commit message: linux_mtd: Fix compilation errors This fixes compilation errors from the initial import patch. Signed-off-by: David Hendricks <dhendricks(a)fb.com> This is the 3rd commit message: linux_mtd: Suppress message if NOR device not found This just suppresses a message that might cause confusion for unsuspecting users. BUG=none BRANCH=none TEST=ran on veyron_mickey, "NOR type device not found" message no longer appears under normal circumstances. Signed-off-by: David Hendricks <dhendrix(a)chromium.org> Reviewed-on: https://chromium-review.googlesource.com/302145 Commit-Ready: David Hendricks <dhendrix(a)chromium.org> Tested-by: David Hendricks <dhendrix(a)chromium.org> Reviewed-by: Shawn N <shawnn(a)chromium.org> This is the 4th commit message: linux_mtd: Support for NO_ERASE type devices Some mtd devices have the MTD_NO_ERASE flag set. This means these devices don't require an erase to write and might not have implemented an erase function. We should be conservative and skip erasing altogether, falling back to performing writes over the whole flash. BUG=b:35104688 TESTED=Zaius flash is now written correctly for the 0xff regions. Signed-off-by: William A. Kennington III <wak(a)google.com> Reviewed-on: https://chromium-review.googlesource.com/472128 Commit-Ready: William Kennington <wak(a)google.com> Tested-by: William Kennington <wak(a)google.com> Reviewed-by: Brian Norris <briannorris(a)chromium.org> This is the 5th commit message: 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 Signed-off-by: Brian Norris <briannorris(a)chromium.org> Reviewed-on: https://chromium-review.googlesource.com/344006 Reviewed-by: David Hendricks <dhendrix(a)chromium.org> This is the 6th commit message: 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: I3d6bb282863a5cf69909e28a1fc752b35f1b9599 Signed-off-by: Brian Norris <briannorris(a)chromium.org> Reviewed-on: https://chromium-review.googlesource.com/505409 Reviewed-by: David Hendricks <dhendrix(a)chromium.org> Reviewed-by: Martin Roth <martinroth(a)chromium.org> Reviewed-by: William Kennington <wak(a)google.com> --- M Makefile M flash.h M flashrom.c M internal.c A linux_mtd.c M programmer.h 6 files changed, 465 insertions(+), 2 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/06/25706/6 -- To view, visit https://review.coreboot.org/25706 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I3d6bb282863a5cf69909e28a1fc752b35f1b9599 Gerrit-Change-Number: 25706 Gerrit-PatchSet: 6 Gerrit-Owner: David Hendricks <david.hendricks(a)gmail.com> Gerrit-Reviewer: Brian Norris <briannorris(a)chromium.org> Gerrit-Reviewer: David Hendricks <david.hendricks(a)gmail.com> Gerrit-Reviewer: Jonathan Neuschäfer <j.neuschaefer(a)gmx.net> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Patrick Rudolph <patrick.rudolph(a)9elements.com> Gerrit-Reviewer: Paul Menzel <paulepanter(a)users.sourceforge.net> Gerrit-Reviewer: Philipp Deppenwiese <zaolin.daisuki(a)gmail.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>

1 0

Change in flashrom[master]: linux_mtd: Initial import
by Nico Huber (Code Review) 14 May '18

14 May '18

Nico Huber has posted comments on this change. ( https://review.coreboot.org/25706 ) Change subject: linux_mtd: Initial import ...................................................................... Patch Set 5: Code-Review+2 (3 comments) https://review.coreboot.org/#/c/25706/4/linux_mtd.c File linux_mtd.c: https://review.coreboot.org/#/c/25706/4/linux_mtd.c@231 PS4, Line 231: perr( > I encountered a "Bad file descriptor" error when attempting to convert this to fwrite(), but will lo […] Maybe there actually is an alignment requirement? In that case we'd have to retry partial writes. Doable, but maybe that gets too complicated. This could also be a sign that fread() only works by chance. Maybe looking at an strace might give a clue. If you end up keeping the mixed fread() / write() usage, you can also use fileno() on the stream object to avoid maintaining the additional `fd`. https://review.coreboot.org/#/c/25706/5/linux_mtd.c File linux_mtd.c: https://review.coreboot.org/#/c/25706/5/linux_mtd.c@208 PS5, Line 208: if (fread(buf + i, 1, step, dev_fp) != step) { `step, 1`, i.e. reading one big chunk, might be better optimized in libc. https://review.coreboot.org/#/c/25706/5/linux_mtd.c@314 PS5, Line 314: , sizeof(buf) Nit, we know both `buf` and "nor" are zero-terminated. There is no need for str*n*cmp(). -- To view, visit https://review.coreboot.org/25706 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I3d6bb282863a5cf69909e28a1fc752b35f1b9599 Gerrit-Change-Number: 25706 Gerrit-PatchSet: 5 Gerrit-Owner: David Hendricks <david.hendricks(a)gmail.com> Gerrit-Reviewer: Brian Norris <briannorris(a)chromium.org> Gerrit-Reviewer: David Hendricks <david.hendricks(a)gmail.com> Gerrit-Reviewer: Jonathan Neuschäfer <j.neuschaefer(a)gmx.net> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Patrick Rudolph <patrick.rudolph(a)9elements.com> Gerrit-Reviewer: Paul Menzel <paulepanter(a)users.sourceforge.net> Gerrit-Reviewer: Philipp Deppenwiese <zaolin.daisuki(a)gmail.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Comment-Date: Mon, 14 May 2018 11:24:55 +0000 Gerrit-HasComments: Yes Gerrit-HasLabels: Yes

1 0

Change in flashrom[master]: linux_mtd: Initial import
by David Hendricks (Code Review) 14 May '18

14 May '18

David Hendricks has posted comments on this change. ( https://review.coreboot.org/25706 ) Change subject: linux_mtd: Initial import ...................................................................... Patch Set 5: (17 comments) > Patch Set 4: > > (17 comments) > > Looks good over all. The only real issues that have to be fixed is > a leaked `FILE *` and internal URLs, IIRC. Mostly everything else > is in the "we can do better" category ;) Thanks for the review. I did a sanity check with a Chromebook that uses MTD, but hit a snag when converting write() to fwrite(). I'll take another look tomorrow and update the man page as well. https://review.coreboot.org/#/c/25706/4/flashrom.c File flashrom.c: https://review.coreboot.org/#/c/25706/4/flashrom.c@352 PS4, Line 352: Remove the ".*" at the end since MTD files don't use minor numbers (AFAIK). https://review.coreboot.org/#/c/25706/4/internal.c File internal.c: https://review.coreboot.org/#/c/25706/4/internal.c@241 PS4, Line 241: msg_perr("Processor detection/init failed.\n" > A little nicer would be an inline stub in the header file in […] Done. I also moved the call to the inline above the call to pci_init_common() since MTD will obviate the need for it if successful. https://review.coreboot.org/#/c/25706/4/linux_mtd.c File linux_mtd.c: https://review.coreboot.org/#/c/25706/4/linux_mtd.c@59 PS4, Line 59: > Using low-level read() might be ok here together with sysfs. […] Done https://review.coreboot.org/#/c/25706/4/linux_mtd.c@94 PS4, Line 94: read_sysfs_string(sysfs_pat > Nit, same as `*endptr`? Done... I think? LMK if I misunderstood your comment. https://review.coreboot.org/#/c/25706/4/linux_mtd.c@131 PS4, Line 131: read_sysfs_int(sys > How portable is this? Should be fairly portable and is supported by all modern versions of GCC and clang AFAIK. However now that you mention it I don't see mingw support so I went ahead and replaced it with a generic popcnt implementation. https://review.coreboot.org/#/c/25706/4/linux_mtd.c@152 PS4, Line 152: > This only runs once on init, right? msg_pdbg() would be more […] Done https://review.coreboot.org/#/c/25706/4/linux_mtd.c@167 PS4, Line 167: } > Nit, line break seems unnecessary. Done https://review.coreboot.org/#/c/25706/4/linux_mtd.c@187 PS4, Line 187: : static int linux_mtd_re > I doubt, my browser would find much there. Removed https://review.coreboot.org/#/c/25706/4/linux_mtd.c@193 PS4, Line 193: > Again, fread() is generally preferred. read() doesn't retry if […] Done https://review.coreboot.org/#/c/25706/4/linux_mtd.c@224 PS4, Line 224: sers[0].eraseblocks[0].size; : unsigned int i; > ... Done https://review.coreboot.org/#/c/25706/4/linux_mtd.c@231 PS4, Line 231: perr( > Same as for read(). I encountered a "Bad file descriptor" error when attempting to convert this to fwrite(), but will look into it more tomorrow. Hopefully it was just a silly error. https://review.coreboot.org/#/c/25706/4/linux_mtd.c@256 PS4, Line 256: > Bail out like above? Done https://review.coreboot.org/#/c/25706/4/linux_mtd.c@294 PS4, Line 294: .read = linux_mtd_read, > Never closed? I would move it on top, initialize to `NULL` and […] Yeah, may as well use read_sysfs_string(). Done. https://review.coreboot.org/#/c/25706/4/linux_mtd.c@320 PS4, Line 320: char dev_path[32]; > `goto linux_mtd_setup_exit;` Done. https://review.coreboot.org/#/c/25706/4/linux_mtd.c@326 PS4, Line 326: etup_exit; > Nit, why cast? Hmmm, I thought there were compiler warnings earlier on, but I don't seem to be getting them now. I'll remove the cast for now. https://review.coreboot.org/#/c/25706/4/linux_mtd.c@361 PS4, Line 361: n 0; > `*endptr` would also cover trailing data. Done. https://review.coreboot.org/#/c/25706/4/linux_mtd.c@364 PS4, Line 364: int linux_mtd_init(void) > Nits, missing full-stop, output should be broken before 80 chars. Done -- To view, visit https://review.coreboot.org/25706 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I3d6bb282863a5cf69909e28a1fc752b35f1b9599 Gerrit-Change-Number: 25706 Gerrit-PatchSet: 5 Gerrit-Owner: David Hendricks <david.hendricks(a)gmail.com> Gerrit-Reviewer: Brian Norris <briannorris(a)chromium.org> Gerrit-Reviewer: David Hendricks <david.hendricks(a)gmail.com> Gerrit-Reviewer: Jonathan Neuschäfer <j.neuschaefer(a)gmx.net> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Patrick Rudolph <patrick.rudolph(a)9elements.com> Gerrit-Reviewer: Paul Menzel <paulepanter(a)users.sourceforge.net> Gerrit-Reviewer: Philipp Deppenwiese <zaolin.daisuki(a)gmail.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Comment-Date: Mon, 14 May 2018 08:07:56 +0000 Gerrit-HasComments: Yes Gerrit-HasLabels: No

1 0

Change in flashrom[master]: linux_mtd: Initial import
by build bot (Jenkins) (Code Review) 14 May '18

14 May '18

build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/25706 ) Change subject: linux_mtd: Initial import ...................................................................... Patch Set 5: Verified+1 Build Successful https://qa.coreboot.org/job/flashrom_gerrit/1073/ : SUCCESS https://qa.coreboot.org/job/flashrom-customrules/1294/ : SUCCESS -- To view, visit https://review.coreboot.org/25706 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I3d6bb282863a5cf69909e28a1fc752b35f1b9599 Gerrit-Change-Number: 25706 Gerrit-PatchSet: 5 Gerrit-Owner: David Hendricks <david.hendricks(a)gmail.com> Gerrit-Reviewer: Brian Norris <briannorris(a)chromium.org> Gerrit-Reviewer: David Hendricks <david.hendricks(a)gmail.com> Gerrit-Reviewer: Jonathan Neuschäfer <j.neuschaefer(a)gmx.net> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Patrick Rudolph <patrick.rudolph(a)9elements.com> Gerrit-Reviewer: Paul Menzel <paulepanter(a)users.sourceforge.net> Gerrit-Reviewer: Philipp Deppenwiese <zaolin.daisuki(a)gmail.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Comment-Date: Mon, 14 May 2018 08:06:29 +0000 Gerrit-HasComments: No Gerrit-HasLabels: Yes

1 0

Change in flashrom[master]: linux_mtd: Initial import
by David Hendricks (Code Review) 14 May '18

14 May '18

Hello Patrick Rudolph, Paul Menzel, Jonathan Neuschäfer, Philipp Deppenwiese, build bot (Jenkins), Nico Huber, Brian Norris, I'd like you to reexamine a change. Please visit https://review.coreboot.org/25706 to look at the new patch set (#5). Change subject: linux_mtd: Initial import ...................................................................... linux_mtd: Initial import This imports a series of patches from chromiumos for MTD support. The patches are squashed to ease review and original Change-Ids have been removed to avoid confusing Gerrit. There are a few changes to integrate the code: - Conflict resolution - Makefile changes - Remove file library usage from linux_mtd. We may revisit this and use it for other Linux interfaces later on. This consolidated patch is Signed-off-by: David Hendricks <dhendricks(a)fb.com> The first commit's message is: Initial MTD support This adds MTD support to flashrom so that we can read, erase, and write content on a NOR flash chip via MTD. BUG=chrome-os-partner:40208 BRANCH=none TEST=read, write, and erase works on Oak Signed-off-by: David Hendricks <dhendrix(a)chromium.org> Reviewed-on: https://chromium-review.googlesource.com/272983 Reviewed-by: Shawn N <shawnn(a)chromium.org> This is the 2nd commit message: linux_mtd: Fix compilation errors This fixes compilation errors from the initial import patch. Signed-off-by: David Hendricks <dhendricks(a)fb.com> This is the 3rd commit message: linux_mtd: Suppress message if NOR device not found This just suppresses a message that might cause confusion for unsuspecting users. BUG=none BRANCH=none TEST=ran on veyron_mickey, "NOR type device not found" message no longer appears under normal circumstances. Signed-off-by: David Hendricks <dhendrix(a)chromium.org> Reviewed-on: https://chromium-review.googlesource.com/302145 Commit-Ready: David Hendricks <dhendrix(a)chromium.org> Tested-by: David Hendricks <dhendrix(a)chromium.org> Reviewed-by: Shawn N <shawnn(a)chromium.org> This is the 4th commit message: linux_mtd: Support for NO_ERASE type devices Some mtd devices have the MTD_NO_ERASE flag set. This means these devices don't require an erase to write and might not have implemented an erase function. We should be conservative and skip erasing altogether, falling back to performing writes over the whole flash. BUG=b:35104688 TESTED=Zaius flash is now written correctly for the 0xff regions. Signed-off-by: William A. Kennington III <wak(a)google.com> Reviewed-on: https://chromium-review.googlesource.com/472128 Commit-Ready: William Kennington <wak(a)google.com> Tested-by: William Kennington <wak(a)google.com> Reviewed-by: Brian Norris <briannorris(a)chromium.org> This is the 5th commit message: 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 Signed-off-by: Brian Norris <briannorris(a)chromium.org> Reviewed-on: https://chromium-review.googlesource.com/344006 Reviewed-by: David Hendricks <dhendrix(a)chromium.org> This is the 6th commit message: 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: I3d6bb282863a5cf69909e28a1fc752b35f1b9599 Signed-off-by: Brian Norris <briannorris(a)chromium.org> Reviewed-on: https://chromium-review.googlesource.com/505409 Reviewed-by: David Hendricks <dhendrix(a)chromium.org> Reviewed-by: Martin Roth <martinroth(a)chromium.org> Reviewed-by: William Kennington <wak(a)google.com> --- M Makefile M flash.h M flashrom.c M internal.c A linux_mtd.c M programmer.h 6 files changed, 464 insertions(+), 2 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/06/25706/5 -- To view, visit https://review.coreboot.org/25706 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I3d6bb282863a5cf69909e28a1fc752b35f1b9599 Gerrit-Change-Number: 25706 Gerrit-PatchSet: 5 Gerrit-Owner: David Hendricks <david.hendricks(a)gmail.com> Gerrit-Reviewer: Brian Norris <briannorris(a)chromium.org> Gerrit-Reviewer: David Hendricks <david.hendricks(a)gmail.com> Gerrit-Reviewer: Jonathan Neuschäfer <j.neuschaefer(a)gmx.net> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Patrick Rudolph <patrick.rudolph(a)9elements.com> Gerrit-Reviewer: Paul Menzel <paulepanter(a)users.sourceforge.net> Gerrit-Reviewer: Philipp Deppenwiese <zaolin.daisuki(a)gmail.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>

1 0

Change in flashrom[staging]: Enable writes with active ME
by Nico Huber (Code Review) 13 May '18

13 May '18

Nico Huber has abandoned this change. ( https://review.coreboot.org/18740 ) Change subject: Enable writes with active ME ...................................................................... Abandoned Continued on master: https://review.coreboot.org/26261/ -- To view, visit https://review.coreboot.org/18740 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: staging Gerrit-MessageType: abandon Gerrit-Change-Id: I94a5e7074b845c227e43d76d04dd1a71082a1cef Gerrit-Change-Number: 18740 Gerrit-PatchSet: 15 Gerrit-Owner: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: David Hendricks <david.hendricks(a)gmail.com> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Paul Menzel <paulepanter(a)users.sourceforge.net> Gerrit-Reviewer: Philippe Mathieu-Daudé <f4bug(a)amsat.org> Gerrit-Reviewer: Stefan Reinauer <stefan.reinauer(a)coreboot.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Philipp Deppenwiese <zaolin.daisuki(a)gmail.com>

1 0

Change in flashrom[master]: Enable writes with active ME
by Nico Huber (Code Review) 13 May '18

13 May '18

Nico Huber has posted comments on this change. ( https://review.coreboot.org/26261 ) Change subject: Enable writes with active ME ...................................................................... Patch Set 2: Untested, atm. -- To view, visit https://review.coreboot.org/26261 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I94a5e7074b845c227e43d76d04dd1a71082a1cef Gerrit-Change-Number: 26261 Gerrit-PatchSet: 2 Gerrit-Owner: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Comment-Date: Sun, 13 May 2018 13:48:51 +0000 Gerrit-HasComments: No Gerrit-HasLabels: No

1 0

Change in flashrom[master]: Enable writes with active ME
by build bot (Jenkins) (Code Review) 13 May '18

13 May '18

build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/26261 ) Change subject: Enable writes with active ME ...................................................................... Patch Set 2: Verified+1 Build Successful https://qa.coreboot.org/job/flashrom_gerrit/1072/ : SUCCESS https://qa.coreboot.org/job/flashrom-customrules/1293/ : SUCCESS -- To view, visit https://review.coreboot.org/26261 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I94a5e7074b845c227e43d76d04dd1a71082a1cef Gerrit-Change-Number: 26261 Gerrit-PatchSet: 2 Gerrit-Owner: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Comment-Date: Sun, 13 May 2018 13:47:07 +0000 Gerrit-HasComments: No Gerrit-HasLabels: Yes

1 0

2024

2023

2022

2021

2020

2019

2018

2017

flashrom-gerrit May 2018