Attention is currently required from: Edward O'Callaghan, Nikolai Artemiev, Anastasia Klimchuk, Sergii Dmytruk. Nico Huber 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 27:
(6 comments)
Patchset:
PS22:
Passing an abstract configuration structure to WP functions is probably a better option than the int […]
Sorry for the silence. I think you made good progress on your own! The abstract struct leaves the function signatures nice and simple. However, exposed structs form an ABI along with potential trouble (see inline comment). It might be better to have an opaque struct with getters and setters, e.g.
``` enum fashrom_wp_mode { WP_MODE_DISABLE, WP_MODE_HARDWARE, }; struct flashrom_wp_cfg;
int flashrom_wp_cfg_new(struct flashrom_wp_cfg **); void flashrom_wp_cfg_release(struct flashrom_wp_cfg *); void flashrom_wp_set_mode(struct flashrom_wp_cfg *, enum flashrom_wp_mode); enum flashrom_wp_mode flashrom_wp_get_mode(const struct flashrom_wp_cfg *); void flashrom_wp_set_range(struct flashrom_wp_cfg *, size_t start, size_t len); void flashrom_wp_get_range(size_t *start, size_t *len, const struct flashrom_wp_cfg *);
int flashrom_wp_get_cfg(struct flashrom_wp_cfg **, struct flashrom_flashctx *); int flashrom_wp_set_cfg(struct flashrom_flashctx *, const struct flashrom_wp_cfg *); ```
This would allow us to extend the API in the future if necessary without touching any of the lines above. WDYT?
File libflashrom.h:
https://review.coreboot.org/c/flashrom/+/58479/comment/32b00ac6_d4caf998 PS27, Line 133: }; Having a struct in the API means we can't ever change the struct without versioning the ABI. Beside that I don't know the mechanisms behind the latter I think it's not necessary. For instance, we could have an opaque object like the others, and getters and setters for mode and range, WDYT?
https://review.coreboot.org/c/flashrom/+/58479/comment/af7b568a_0fab4e44 PS27, Line 138: #define FLASHROM_WP_ERR_VERIFY_FAILED 4 This could be an enum, maybe even without specific numbers?
File writeprotect.c:
https://review.coreboot.org/c/flashrom/+/58479/comment/7243012f_9f680caa PS22, Line 63: *cfg = (struct wp_chip_config) {0};
I think I might have relied on this in an earlier patch set but I don't remember now. […]
Done
File writeprotect.c:
https://review.coreboot.org/c/flashrom/+/58479/comment/d270cf6d_1b5994c8 PS27, Line 118: } Maybe drop braces here for concistency.
https://review.coreboot.org/c/flashrom/+/58479/comment/8415658b_aa245944 PS27, Line 161: to write : * to the chip drop