Change in flashrom[master]: libflashrom,writeprotect: add flashrom_wp_{read,write}_chip_config()
Attention is currently required from: Nico Huber, Edward O'Callaghan, Anastasia Klimchuk, Sergii Dmytruk. Nikolai Artemiev has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/58479 )
Change subject: libflashrom,writeprotect: add flashrom_wp_{read,write}_chip_config() ......................................................................
Patch Set 23:
(16 comments)
Patchset:
PS22:
Thinking further about this, there are more reasons to tie […]
This will be a fairly long comment and cover some things mentioned in other comments, but it's probably easiest to put everything here:
I've thought more about what sort of API should be exposed through libflashrom and I don't think exposing the wp_chip_config structure is a good idea, even if the structure is opaque to libflashrom users.
In addition to the config being to tied to the specific chip as you mention, properly sequencing read/modify/write/verify operations is unnecessary work and it increases the risk that introducing a new chip will break existing assumptions in the future.
Another reason to simplify the API is that linux's MTD interface exposes a much simpler protection API. It's really inconvenient to have two APIs for SPI/MTD protection and unifying them makes sense if we don't lost much IMO. Specifically MTD gives us these operations: - MEMLOCK: Enable HW protection and set range - MEMUNLOCK: Disable protection / clear range - MEMISLOCKED: Get current protection range / HW protection status.
The final libdflashrom API / functionality would be something like: - flashrom_wp_protect_flash(flashctx *, flashsize_t *start, flashsize_t *len) // Read config, set HW protection, set range, write, verify - flashrom_wp_unprotect_flash(flashctx *) // Read config, unset HW protection, set empty range, write, verify - flashrom_wp_get_protection_status(flashctx *, flashsize_t *start, flashsize_t *len, bool *hw_prot_enabled) // Read config, decode range / mode, write values to output vars
We can of course expand with additional functions if they're useful, but for now I think these will cover most standard use cases and provide a more flexible API.
The only major thing the new API doesn't allow is setting a protection range without enabling HW protection, but that would just get undone by flashrom or whatever other tool someone next uses to write the flash.
As for migrating the current code, I plan to keep the current writeprotect functions, but keep them inside writeprotect.c and only expose the new functions. Some things that need to be shared will probably get moved to writeprotect.h, like the chip config structure.
What do you think?
File libflashrom.h:
https://review.coreboot.org/c/flashrom/+/58479/comment/2dc379c2_f68bd077 PS22, Line 131: const
We should be careful with `const` here. Flash access goes very deep down […]
Done
File writeprotect.c:
https://review.coreboot.org/c/flashrom/+/58479/comment/e7b9526e_6fae517b PS22, Line 35: 0
Will this ever be consumed?
Good point. I thought it would be helpful to ensure it is zeroed if the bit isn't present, but the structure is zero-initialized anyway so we don't need to do that here.
https://review.coreboot.org/c/flashrom/+/58479/comment/cbf7d480_3ccb70ae PS22, Line 59: *
No dangling asterisk, please.
I've made the comment a bit bigger, I think the asterisks are ok now since there are leading and trailing comment lines.
https://review.coreboot.org/c/flashrom/+/58479/comment/d9aa7786_76ca184c PS22, Line 59: intrepret
int*er*pret
Done
https://review.coreboot.org/c/flashrom/+/58479/comment/b7e234e5_66393683 PS22, Line 61: tmp
rather `unused` or `ignored`?
Done
https://review.coreboot.org/c/flashrom/+/58479/comment/eefce310_37c9f4b7 PS22, Line 63: *cfg = (struct wp_chip_config) {0};
Won't this be overwritten where it matters? iow. […]
I think I might have relied on this in an earlier patch set but I don't remember now.
I think it's kind of nice to zero it out before we do anything, else but I can drop it if you want.
https://review.coreboot.org/c/flashrom/+/58479/comment/02149fac_a29c6663 PS22, Line 70: r(
Missing space.
Done
https://review.coreboot.org/c/flashrom/+/58479/comment/926f9597_d5f22b5d PS22, Line 70: size_t
Declarations inside a `for` made trouble in some environment. Please avoid it for now.
Done
https://review.coreboot.org/c/flashrom/+/58479/comment/694d2b85_2eb0d8a4 PS22, Line 70: for(size_t i = 0; bits->bp[i].reg != INVALID_REG; i++) {
Generally, it's preferred to loop over the target array. This way you […]
Done
https://review.coreboot.org/c/flashrom/+/58479/comment/37c2fc13_d2beeb61 PS22, Line 75: r(
Missing space.
Done
https://review.coreboot.org/c/flashrom/+/58479/comment/bb2ddb25_6cc35253 PS22, Line 84: *
No dangling asterisk, please. Or maybe just use the long comment style […]
Done
https://review.coreboot.org/c/flashrom/+/58479/comment/da2b5e4b_56f87daa PS22, Line 122: INVALID_REG
This is very confusing. I see it's skipped because the mask would be 0, […]
I've changed it to (INVALID_REG + 1), perhaps STATUS1 would be better though. I'll change it if you want.
https://review.coreboot.org/c/flashrom/+/58479/comment/1b44b521_87138102 PS22, Line 126: = 0
Why initialize?
Done
https://review.coreboot.org/c/flashrom/+/58479/comment/00045ff9_57888c6e PS22, Line 144: configuraitons
configura*ti*ons
Done
https://review.coreboot.org/c/flashrom/+/58479/comment/37c351de_7c726a76 PS22, Line 145: which of two configurations should be used if they both select the same : * protection range
Does this really work? How can we say, for instance, that SRP1 being […]
This was kind of an adaptation of an earlier function that just compared ranges so they could be sorted and a preferred range encoding could be chosen.
I realized that it could also be used to compare most bits in the config structure, so I extended it to allow all bits to be compared, which is useful when we want to verify a config we have read back from a chip.
In this context "preferred" is arbitrary, it should only be used to pick between two configurations if both of them are acceptable.
-
Nikolai Artemiev (Code Review)