Attention is currently required from: Nico Huber, Edward O'Callaghan, Angel Pons. Anastasia Klimchuk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/56501 )
Change subject: tests: Add tests to erase a chip ......................................................................
Patch Set 2:
(9 comments)
Patchset:
PS2: The newest test that I have added uses real spi_send_command, however some previous tests are mocking the same function. Means, we have to have two tests executables where one of executables uses real spi_send_command and second one wraps spi_send_command. I do intent to split this into two patches, but I just wanted to upload this for review to check that we all agree on the overall idea. Thanks for reviews! I have two cool tests instead of one, because of the great comments from you!
File tests/chip.c:
https://review.coreboot.org/c/flashrom/+/56501/comment/ec48827b_ae250329 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);
+1. […]
Andel and Edward, thanks to your great comments, I spent some time with this, and now I have one more test, which is actually using dummyflasher for all chip operations!!! This is 10x more cool, thank you so much!
I still want to keep first test with fake read/write/erase, because they log and also as a foundation for further (later) experiments with various combinations of chip size and layout. Second test has to have it all like real W25Q128.V.
https://review.coreboot.org/c/flashrom/+/56501/comment/60d43c62_6771937e PS1, Line 42: printf("Unlock chip called\n");
Even better would be to prevent erases/writes if the chip hasn't been unlocked.
Currently unlock is optional, which is if (flash->chip->unlock) flash->chip->unlock(flash); Or do you mean "prevent erases/writes if unlock function present, but hasn't been invoked"?
https://review.coreboot.org/c/flashrom/+/56501/comment/38ef8d90_09bf9281 PS1, Line 57: char *param_dup = strdup("");
The second parameter of `programmer_init()` is of `const char *` type. […]
This only works for the case when param is empty string. If it is not empty, it needs to be strdup'ed. I think this is because they way extract_programmer_params works.
https://review.coreboot.org/c/flashrom/+/56501/comment/6f16b1dd_0627fd11 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 p […]
Now I have two tests, one with small size chip, another with large chip. Yes right, doesn't seem to be any issues.
https://review.coreboot.org/c/flashrom/+/56501/comment/722fb5ee_e9e91cfa 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). […]
I corrected chip size and layout size, I think I just got confused that total_size is in KiB. The intention was to have fake chip with small size, covered by layout and covered by erase blocks.
https://review.coreboot.org/c/flashrom/+/56501/comment/4ff203c8_ac8a5c24 PS1, Line 86: 0x00000000
0
Done
https://review.coreboot.org/c/flashrom/+/56501/comment/68dfaf9e_6a4f931f PS1, Line 86: 0x00002000
chip->total_size * KiB
Done (with -1 because it starts from 0).
https://review.coreboot.org/c/flashrom/+/56501/comment/0bdc5b3e_100912e8 PS1, Line 114: free(param_dup);
If using a string literal on line 57, this must be removed.
Removed for empty param (still need strdup for non-empty param).
-
Anastasia Klimchuk (Code Review)