Attention is currently required from: Nico Huber, Angel Pons, Sergii Dmytruk. Anastasia Klimchuk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/59075 )
Change subject: [RFC][WPTST] tests: test write protection ......................................................................
Patch Set 10:
(10 comments)
File tests/chip_wp.c:
https://review.coreboot.org/c/flashrom/+/59075/comment/84805f27_24f92c0a PS7, Line 177: /* Check initial mode. */ : assert_int_equal(0, flashrom_wp_get_mode(&cfg, &mode)); : assert_int_equal(WP_MODE_DISABLED, mode); : : /* Switch modes in dummyflasher. */ : assert_int_equal(0, flashrom_wp_set_mode(&cfg, WP_MODE_PERMANENT)); : assert_int_equal(0, flashrom_wp_write_chip_config(&flash, &cfg)); : assert_int_equal(0, flashrom_wp_set_mode(&cfg, WP_MODE_POWER_CYCLE)); : assert_int_equal(0, flashrom_wp_write_chip_config(&flash, &cfg)); : : /* Final mode should be "permanent". */ : assert_int_equal(0, flashrom_wp_read_chip_config(&flash, &cfg)); : assert_int_equal(0, flashrom_wp_get_mode(&cfg, &mode)); : assert_int_equal(WP_MODE_PERMANENT, mode);
I think it's both: […]
Yes it's better, I mark this one as resolved. There are two minor comments on this test method, but big question is solved.
File tests/chip_wp.c:
https://review.coreboot.org/c/flashrom/+/59075/comment/f86a27b3_7648e211 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). Is there a reason to have set range twice in a sequence, or this can be two tests?
https://review.coreboot.org/c/flashrom/+/59075/comment/3a021a8e_a5c8f1b3 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_...." instead of set_wp_mode....
https://review.coreboot.org/c/flashrom/+/59075/comment/d24c30c3_5877f811 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.
https://review.coreboot.org/c/flashrom/+/59075/comment/124c5dc2_790dcf91 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 Similar to other tests, a question is whether this is a sequence needed for a test scenario (then just add a comment), or this are two independent scenarios (then split). And the same for 3 more tests below.
https://review.coreboot.org/c/flashrom/+/59075/comment/58694d2f_84ec5d58 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 register bits.
https://review.coreboot.org/c/flashrom/+/59075/comment/7d7621e6_5f6e143e 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 think these two assertions need to be moved to setup_chip.
https://review.coreboot.org/c/flashrom/+/59075/comment/d66d3fce_c214348e PS10, Line 313: Write protection takes effect only after changing SRP values. if you can add a bit more for example , so at this stage WP is not enabled and erase completes successfully.
or something similar
https://review.coreboot.org/c/flashrom/+/59075/comment/dc114c0e_baedfce4 PS10, Line 329: Try erasing the chip again also you can add continue the comment say , however because now wp is enabled, erase operation fails.
or something
https://review.coreboot.org/c/flashrom/+/59075/comment/d7d233eb_7225cb46 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 this scenario, erase completes successfully even with wp enabled?
Let's add more comments to explain what these two tests are testing, and what is expected behaviour. The code itself looks good, just need to be more documented.