Attention is currently required from: Felix Singer, Edward O'Callaghan, Anastasia Klimchuk.
Patch set 15:Code-Review +1
3 comments:
Patchset:
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:
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:
Patch Set #15, 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?
To view, visit change 67240. To unsubscribe, or for help writing mail filters, visit settings.