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:
(10 comments)
Mostly commented for myself for the next iteration. Feel free to look at it anyway.
https://review.coreboot.org/#/c/17945/2/flashrom.c File flashrom.c:
PS2, Line 1647: struct fl_layout_single *const fallback
Since the fallback is always the entire chip, would it make sense to omit t
I'm tempted but I'd rather see less global variables than more. At some point, somebody will ask why he can't use libflashrom with his weird programmer array in a multi-threaded application... and I can point a finger at others. :)
We could make the fallback structure part of the flashctx though. How about that?
PS2, Line 1672: void * should be `uint8_t *` for pointer arithmetic
PS2, Line 1695: unsigned int wondering why I didn't use chipoff_t for consistency
PS2, Line 1705: int size_t
PS2, Line 1708: int size_t
PS2, Line 1721: info->region_end < info->erase_start could just break (both loops) in this case
PS2, Line 1759: ret rather `error`
PS2, Line 1844: newcontents better `info->newcontents`
PS2, Line 1847: region_unaligned
Has to update info->curcontents too for `-A` case.
Ok, trying to sort this out. In case of `-A` (noverify-all) we don't read the full flash chip in advance but only included regions. So `info->curcontents` may be undefined for the parts outside the unaligned region. Reading per erase block might be slow, so we don't want to fill `curcontents` after selecting the block eraser but want to read as much as possible in advance.
However, `curcontents` is always valid for the part that we want to change and `newcontents` is already patched correctly. So in the worst case, we'd unnecessarily erase the block but write the correct data.
Solutions: * Write the above into a comment and ignore the unnecessary erase * Patch curcontents too: Copy what we just read into `newc` * Patch the layout used to read in advance to read complete erase blocks (wouldn't have to read() here then but could always rely on `curcontents`). Preferred solution but doesn't play with our block-eraser fallback.
In any case: Document the contract explicitly, that `curcontents` either guarantees to contain the layout regions only or every touched eraseblock.
PS2, Line 1862: newc - info->erase_start + info->region_end + 1
Hmmm, somehow I think this can be made clearer to the reader. We're offsett
ack