Attention is currently required from: Felix Singer, Edward O'Callaghan, Anastasia Klimchuk.
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/67240 )
Change subject: tree/: Convert flashchip erase_block func ptr to enumerate ......................................................................
Patch Set 15: Code-Review+1
(3 comments)
Patchset:
PS15: Besides the at45db changes, looks OK.
[Food for thought]
We could use reproducible builds and some temporary macros to easily verify that the bulk of changes (e.g. flashchips.c) doesn't change any behavior. One way to do this is to make a reproducible commit (i.e. one that doesn't change the resulting flashrom binary) preceding this one that replaces the function references with macros that have the same names as the enum values. For example:
#define SPI_ERASE_AT45DB_SECTOR &spi_erase_at45db_sector
The point is that (un)wrapping stuff in macros doesn't change the resulting binaries (if done correctly). In this case, the resulting source files after macro expanding (i.e. preprocessing) should already be equivalent.
Yes, these macros are *cursed*, but they're *temporary*. Think of them as support structures in 3D printing or some other kind of "scaffolding": adding and removing them is work, but it makes the job substantially simpler and less error-prone.
The big question is... Does flashrom's build system (or the cacophony that having two build systems is) support reproducible builds?
File at45db.c:
https://review.coreboot.org/c/flashrom/+/67240/comment/3b10412f_ca55de40 PS15, Line 82: for (i = 0; i < NUM_ERASEFUNCTIONS; i++) { : const struct block_eraser *const eraser = &flash->chip->block_erasers[i]; : erasefunc_t *erase_func = lookup_erase_func_ptr_(eraser); : if (erase_func == &spi_erase_at45db_sector) { : for (j = 0; j < NUM_ERASEREGIONS; j++) { : cnt += eraser->eraseblocks[j].count; : } : } : } Is it necessary to refactor this *now*, i.e. in this commit? It doesn't seem to be the case, and it could cause regressions.
Could we go with the simple approach here, and consider refactor in a follow-up change?
``` for (i = 0; i < NUM_ERASEFUNCTIONS; i++) { if (flash->chip->block_erasers[i].block_erase == SPI_ERASE_AT45DB_SECTOR) { for (j = 0; j < NUM_ERASEREGIONS; j++) { cnt += flash->chip->block_erasers[i].eraseblocks[j].count; } } } ```
That is, just replace `&spi_erase_at45db_sector` with `SPI_ERASE_AT45DB_SECTOR`.
File include/flash.h:
https://review.coreboot.org/c/flashrom/+/67240/comment/6f99a0a2_b36cf2b9 PS15, Line 307: TEST_ERASE_INJECTOR, /* special case must come last. */ What does this mean? Looks like it's for testing the code, maybe add a "Used in tests" note to the comment?