David Hendricks has posted comments on this change. ( https://review.coreboot.org/17945 )
Change subject: Add functions to read/erase/write/verify by layout ......................................................................
Patch Set 4:
(5 comments)
a few quick comments... I need to re-review this patch later on.
https://review.coreboot.org/#/c/17945/2/flashrom.c File flashrom.c:
PS2, Line 1647: cinfo("\nWarning: Chip content is ident
I'm tempted but I'd rather see less global variables than more. At
SGTM. I'd also rename it to "default" or "default_layout" while we're at it, but that's your call.
PS2, Line 1721:
could just break (both loops) in this case
Actually, I think this will need to be expanded to patch the content buffers if eraseblocks[i].size doesn't align with the targeted region (more on that below). Not sure how it will look in the end, will think about it some more...
PS2, Line 1836: * @param buffer Buffer of full chip size to read into. Are you sure info->erase_end and info->erase_start are valid here? The caller (walk_by_layout()) doesn't initialize them.
PS2, Line 1847: c int read_erase
Ok, trying to sort this out. In case of `-A` (noverify-all) we
I tried to patch the layout in advance in the chromiumos branch, but the result was kinda ugly and inflexible (https://chromium-review.googlesource.com/#/c/240176/). Unfortunately I never had time to go back and implement a solution that could work with different block sizes, so I ended up just assuming that the smallest size must work :-/ Like you say, it doesn't play well with the block-erase fallback.
IIRC the issue was the way contents and offsets were passed around awkwardly (and inconsistently?) to the helper functions. Passing a walk_info struct all the way down the stack should help a lot. The innermost loop in walk_eraseblocks() can use that info to patch both curcontent and newcontent with the missing parts (patch newcontent for the update, curcontent for verification later on).
Line 1929: ret = 0; The other members should probably be initialized to 0xdeadbeef (or something out of range of usual flash chips) to help debug when somebody re-factors all of this.
-
David Hendricks (Code Review)