Nico Huber has posted comments on this change. ( https://review.coreboot.org/17945 )
Change subject: Add functions to read/erase/write/verify by layout ......................................................................
Patch Set 2:
(3 comments)
I'm not sure which path to go from here. I could leave it as is, in case tests are positive (I've fixed the curcontents patching in the latest patch set). Or try to perfect it (e.g. by making curcontents/newcontents completely transparent to the walk_* functions). But I guess the ugly parts would vanish anyway, if we tackle the erase function decision later.
https://review.coreboot.org/#/c/17945/2/flashrom.c File flashrom.c:
PS2, Line 1773: read_by_layout(flashctx, info->curcontents)
If we'd move this before the first try and read the current region
Should move anyway, or else we'd read again after the last function failed.
PS2, Line 1836: const unsigned int erase_len = info->erase_end + 1 - info->erase_start;
Are you sure info->erase_end and info->erase_start are valid here? The call
We're called by walk_eraseblocks() which sets them... generally:
something_by_layout() -- optionally sets curcontents/newcontents walk_by_layout() -- sets region_start/region_end walk_eraseblocks()-- sets erase_start/erase_end per_blockfn() -- does the job
PS2, Line 1847: region_unaligned
I tried to patch the layout in advance in the chromiumos branch, but the re
After reading your comment above, I too thought walk_eraseblocks() would be a better place for the patching, but... the "Note" in the comment here gives a hint that it would become more complex. We can't patch into info->newcontents without walking the layout again to decide which parts to patch. So I decided to introduce the `newc` buffer which only contains the changes that will be written for the current region.
This is not the optimal solution but, IMO, the cleaner one: 1. the code is less complex. 2. we do things step by step. after walk_by_ layout() is done with one interation the flash chip is in the ob- vious state: we have updated the data for all regions up to the last iteration (without patching data for later regions in beforehand). 3. the caller of walk_by_layout() (who decides about the used per_ blockfn()) and the per_blockfn() share a contract how curcontents and newcontents are handled. this contract is transparent to walk_ by_layout() and walk_eraseblocks() (with the exception of the re- reading in the fallback case).
If we'd keep the `newc` buffer but move the patching up into walk_ eraseblocks(), we'd have to mess with the per_blockfn() signature (adding a parameter for the newcontents).
-
Nico Huber (Code Review)