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 functions for reading/writing WP settings ......................................................................
Patch Set 28:
Sorry for the silence. I think you made good progress on your own! The abstract […]
That sounds good. I'm working on implementing it now.
https://review.coreboot.org/c/flashrom/+/58479/comment/29939841_b3c4fc52 PS27, Line 133: };
Having a struct in the API means we can't ever change the struct without […]
That sounds good to me, it's good to address ABI stability problems early on.
I've been going through and updating the patches, one thing I'm not sure about is how the `flashrom_wp_get_ranges()` function (CB:58481) should be handled. It currently allocates an output buffer of `struct wp_range` elements, but that has the same ABI problem.
We could have something like a ```flashrom_wp_get_ranges(size_t **starts, size_t **lengths, size_t *counts, flashrom_flashctx *)``` function, though it's a bit messy compared to just returning an array of structures. Alternatively we could have an opaque wp_range_collection or something like that and an accessor function: ``` get_wp_range_from_collection(struct wp_range_collection *, size_t index) ```
https://review.coreboot.org/c/flashrom/+/58479/comment/689828b6_cbf40989 PS27, Line 138: #define FLASHROM_WP_ERR_VERIFY_FAILED 4
This could be an enum, maybe even without specific numbers?
I'm open to using an enum, though we would probably want to explicitly assign values to ensure API stability right?
https://review.coreboot.org/c/flashrom/+/58479/comment/4df583fb_daaf2d28 PS22, Line 125: software
If it's software protection, why not name it like that?
I've removed the comment now, mentioning software protection is confusing/inaccurate since software protection may not be active and can easily be disabled if it is.
https://review.coreboot.org/c/flashrom/+/58479/comment/233d2333_f8e81846 PS22, Line 56: struct wp_chip_config *cfg
Only realized this now: this requires the caller to know the size of […]