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] tests: test write protection ......................................................................
Patch Set 5:
(13 comments)
Patchset:
PS5:
Thank you for working on tests!! I did more detailed code review. […]
Previous patches in the chain make tests possible. I've seen your name in `git blame` for dummyflasher, so I'd appreciate if you could take a look at those changes.
File tests/meson.build:
https://review.coreboot.org/c/flashrom/+/59075/comment/7dfb619c_32c0d6d1 PS5, Line 26: write_protect
I know I asked you to rename the file, but now I did more detailed review and I think the better nam […]
Done
File tests/tests.h:
https://review.coreboot.org/c/flashrom/+/59075/comment/ede9ed34_950fdc89 PS5, Line 67: write_protection.c
chip_wp. […]
Done
File tests/tests.c:
https://review.coreboot.org/c/flashrom/+/59075/comment/4f5eee49_5f821cc8 PS5, Line 357: write_protection
chip_wp
Done
https://review.coreboot.org/c/flashrom/+/59075/comment/24266dfa_3509c179 PS5, Line 364: write_protection.c
chip_wp. […]
Done
File tests/write_protect.c:
https://review.coreboot.org/c/flashrom/+/59075/comment/a6c445d8_380d6f57 PS5, Line 27: No mocking. Using WP emulation in dummy programmer.
Maybe a complete phrase is better, like "Tests in this file do not use any mocking, because using wr […]
Done
https://review.coreboot.org/c/flashrom/+/59075/comment/bb68168c_c391d9c6 PS5, Line 101: invalid_setup
We can be more specific in test name, since it covers invalid range (not just any invalid setup). […]
Done
https://review.coreboot.org/c/flashrom/+/59075/comment/0208bba5_1a451d3f PS5, Line 117: struct flashrom_wp_chip_config cfg;
Lets move this declaration in the beginning , together with flash, layout, mock_chip. […]
Done
https://review.coreboot.org/c/flashrom/+/59075/comment/0379418b_bef99b4f PS5, Line 120: /* Invalid range. */ : range.start = 0x1000; : range.len = 0x1000;
why chip_len is initialised at creation time, but start/len initialised separately, few lines later? […]
There used to me more checks, but changes to WP patches cased their removal. Done.
https://review.coreboot.org/c/flashrom/+/59075/comment/00f10e2d_61ba7747 PS5, Line 143: wp=no
I haven't looked at the rest of your chain (tell me if I should), but this looks confusing: param sa […]
Here "wp" means WP pin of the flash chip. Could rename it to "wppin" or "hwp" (hardware write-protection). The latter might be better because actual pin is negated and I wanted param to set state of hardware protection rather than state of a pin.
https://review.coreboot.org/c/flashrom/+/59075/comment/3548f97f_d7ceb39a PS5, Line 152: /* Use first 4 KiB for a range. */ : range.len = 0x1000; : assert_int_equal(0, flashrom_wp_set_range(&cfg, &range)); : : /* Change range to last 4 KiB. */ : range.start = 0x00fff000; : assert_int_equal(0, flashrom_wp_set_range(&cfg, &range));
This code looks like as if test is testing two scenarios? If there are indeed two different scenario […]
Broke it into set_wp_range_dummyflasher_test_success and set_wp_mode_dummyflasher_test_success.
https://review.coreboot.org/c/flashrom/+/59075/comment/949de99f_3a491bcb PS5, Line 164: /* Switch modes in configuration only. */ : assert_int_equal(0, flashrom_wp_set_mode(&cfg, WP_MODE_HARDWARE)); : assert_int_equal(0, flashrom_wp_set_mode(&cfg, WP_MODE_DISABLED)); : assert_int_equal(0, flashrom_wp_set_mode(&cfg, WP_MODE_POWER_CYCLE)); : assert_int_equal(0, flashrom_wp_set_mode(&cfg, WP_MODE_PERMANENT));
Do all these need flash context, chip, layout? If not, extract into separate small tests that only d […]
Done
https://review.coreboot.org/c/flashrom/+/59075/comment/3b0d090f_ab8e3ccd PS5, Line 170: /* Switch modes in dummyflasher. */ : 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)); : : /* Check final mode. */ : 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);
Which ones of these need layout, if any? […]
Reduced checks performed by the two tests. Layout isn't needed, but dummyflasher is needed to test how WP code behaves on saving state.