Attention is currently required from: Nico Huber, Angel Pons, Anastasia Klimchuk. Sergii Dmytruk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/59075 )
Change subject: [RFC][WPTST] tests: test write protection ......................................................................
Patch Set 11:
(10 comments)
File tests/chip_wp.c:
https://review.coreboot.org/c/flashrom/+/59075/comment/a0965549_7a649db4 PS10, Line 141: /* Use first 4 KiB for a range. */ : range.len = 0x1000; : assert_int_equal(0, flashrom_wp_set_range(&flash, &cfg, &range)); : : /* Change range to last 4 KiB. */ : range.start = 0x00fff000; : assert_int_equal(0, flashrom_wp_set_range(&flash, &cfg, &range));
This looks like 2 in 1 tests (1 for first 4KiB range, 2 for last 4KiB). […]
One range should be enough.
https://review.coreboot.org/c/flashrom/+/59075/comment/c446348b_f0429daa PS10, Line 159: set_wp_mode_dummyflasher_test_success
I think the test is now good, given the comment, but maybe better name is "switch_wp_mode_.... […]
Done
https://review.coreboot.org/c/flashrom/+/59075/comment/b501dcb1_7861b677 PS10, Line 188: /* : * Check that "permanent" mode can't be unset and it's not considered to be : * an error at the level of library calls (verification should be done : * higher). : */
That's a very good comment, thank you! Only need to fix indentation.
Done
https://review.coreboot.org/c/flashrom/+/59075/comment/59f70075_abfa3e45 PS10, Line 220: cfg.srp_bit_count = 1; : assert_int_equal(0, flashrom_wp_set_mode(&cfg, WP_MODE_DISABLED));
I see two parts here: 1) with srp_bit_count = 2, 2) with srp_bit_count = 1 […]
Split these four tests.
https://review.coreboot.org/c/flashrom/+/59075/comment/2497edad_e2fa43fb PS10, Line 290: assert_int_equal(0x004000, range.start); : assert_int_equal(0xffc000, range.len);
just to check, do range start/len come from spi_status? the comment above param_dup only mentions re […]
Yes, range is encoded by those bits. Datasheets contain tables of bit values and ranges that they produce (there is some logic to individual bits, but it's not very straightforward). Expanded that commend and fixed, since it diverged from the code.
https://review.coreboot.org/c/flashrom/+/59075/comment/0c7f128f_a8e174e9 PS10, Line 310: assert_int_equal(0, flashrom_layout_include_region(layout, "head")); : assert_int_equal(0, flashrom_layout_include_region(layout, "tail"));
It is not clear in the context of test method where "head" and "tail" come from. […]
I've added comments. These calls are here, because the test below this one includes only the "head".
https://review.coreboot.org/c/flashrom/+/59075/comment/951e060c_8e05110c PS10, Line 313: Write protection takes effect only after changing SRP values.
if you can add a bit more for example […]
Done
https://review.coreboot.org/c/flashrom/+/59075/comment/04750d50_8a4f977e PS10, Line 329: Try erasing the chip again
also you can add continue the comment say […]
Done
https://review.coreboot.org/c/flashrom/+/59075/comment/de0ef341_9f3b1a9c PS10, Line 364: printf("Erase chip operation started.\n"); : assert_int_equal(0, do_erase(&flash)); : printf("Erase chip operation done.\n");
Oh now I compare this test scenario and previous, and looks like first 4 KiB are special? Because in […]
Done
File tests/write_protect.c:
https://review.coreboot.org/c/flashrom/+/59075/comment/97976bd0_6e14b1c5 PS5, Line 143: wp=no
Here "wp" means WP pin of the flash chip. […]
Done
-
Sergii Dmytruk (Code Review)