Attention is currently required from: Nico Huber, Edward O'Callaghan, Anastasia Klimchuk. Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/56501 )
Change subject: tests: Add test to erase a chip ......................................................................
Patch Set 1:
(9 comments)
File tests/chip.c:
https://review.coreboot.org/c/flashrom/+/56501/comment/eab68aaf_20dec834 PS1, Line 26: /* : * Populate buf with the value used for erase operation, this allows to pass : * verification checks and also emulates successful erase. : */ : buf = memset(buf, 0xff, len);
should this memset happen instead in the block erase fn and use a singleton state here to see if tha […]
+1. Doing the erases in `block_erase_chip()` accounting for the `blockaddr` and `blocklen` parameters is much more robust. This would also allow testing partial block updates, e.g. smallest erase block size is 2 KiB, but the boundary between two contiguous regions isn't aligned to at least 2 KiB, and only one of the regions is included.
Hmmm, but doesn't dummyflasher already emulate flash chips? Why not use them, then?
https://review.coreboot.org/c/flashrom/+/56501/comment/93c297da_fef98dab PS1, Line 42: printf("Unlock chip called\n");
The singleton state could also be queried with a counter to ensure a double unlock isn't done?
Even better would be to prevent erases/writes if the chip hasn't been unlocked.
https://review.coreboot.org/c/flashrom/+/56501/comment/7ea8c4ec_68a8a071 PS1, Line 57: char *param_dup = strdup(""); The second parameter of `programmer_init()` is of `const char *` type. You don't need `strdup()`, just use a string literal:
const char *param = "";
https://review.coreboot.org/c/flashrom/+/56501/comment/5d80eee8_0b01b265 PS1, Line 60: "aklm" 😜
https://review.coreboot.org/c/flashrom/+/56501/comment/a1646ce6_167b73e3 PS1, Line 61: /* : * Total size less than 16 * 1024 to skip some steps : * in flashrom.c#prepare_flash_access. : */ Is skipping these steps an issue, though? `spi_master_no_4ba_modes()` can only return true for SPI programmers with the `SPI_MASTER_NO_4BA_MODES` flag. Currently, only Dediprog sets this flag.
https://review.coreboot.org/c/flashrom/+/56501/comment/a544af9d_e9e8fe20 PS1, Line 73: {2 * 1024, 3} Huh? This is just three blocks of 2 KiB each. The flash chip size is 8 MiB (`total_size` is in KiB).
If you emulate the flash chip contents for the test (zero-initialise the buffer and erase the blocks in `block_erase_chip`), you should see that verification fails.
https://review.coreboot.org/c/flashrom/+/56501/comment/38370cac_1bca4647 PS1, Line 86: 0x00002000 chip->total_size * KiB
https://review.coreboot.org/c/flashrom/+/56501/comment/291c939a_bc526206 PS1, Line 86: 0x00000000 0
https://review.coreboot.org/c/flashrom/+/56501/comment/0d89f67c_90d92970 PS1, Line 114: free(param_dup); If using a string literal on line 57, this must be removed.
-
Angel Pons (Code Review)