Attention is currently required from: Simon Buhrow, Angel Pons. Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/58467 )
Change subject: flashchips.c: big erase blocksize first ......................................................................
Patch Set 5:
(3 comments)
Patchset:
PS5:
The clou is the condition […]
I'm generally not a fan of special cases. However, if we can find a contained solution for the full-chip erase case, I wouldn't block it.
Technically, checks like `per_blockfn == &erase_block` are a layering violation, i.e. something is done in a lower layer that should have been handled at a higher level. I left an inline comment about how I would avoid it.
Eventually, I would prefer a generic solution for the small block-size problem, I think it's not hard to achieve. We should replace the current fallback-to-bigger-blocks strategy with one that checks if consecutive calls of a small erase function would completely fill a bigger one. There's one obstacle, however: The fallback strategy was necessary because some programmers can restrict what commands we can send. I would handle that by filtering the list of erase functions right after the chip was probed.
File flashrom.c:
https://review.coreboot.org/c/flashrom/+/58467/comment/242d4715_0b2ac6b4 PS5, Line 1261: if ((block_size == flashctx->chip->total_size * 1024) && per_blockfn == &erase_block){ I guess this would be much easier to handle in flashrom_flash_erase(). If the size matches the full chip and the last eraseblocks[] entry too, then call that eraser directly, and if it fails fall back to erase_by_layout(). No loops and definitely no nested loops necessary.
https://review.coreboot.org/c/flashrom/+/58467/comment/e3d2c7a5_3870fea2 PS5, Line 1304: } Why change this? Doesn't the code handle this case gracefully?