Attention is currently required from: Edward O'Callaghan, Angel Pons, Nikolai Artemiev, Sergii Dmytruk. Nico Huber 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 22:
(24 comments)
File libflashrom.h:
https://review.coreboot.org/c/flashrom/+/58479/comment/b432f1a5_ced8847a PS22, Line 125: software If it's software protection, why not name it like that?
https://review.coreboot.org/c/flashrom/+/58479/comment/ab178333_98a78d51 PS22, Line 129: }; Please move to the commit that makes use of it.
https://review.coreboot.org/c/flashrom/+/58479/comment/be8aff87_0a6396e6 PS22, Line 131: const We should be careful with `const` here. Flash access goes very deep down into the code. How could we say for instance, that we won't have to change some state in the future, e.g. to keep track of the last used address, or an error indication.
So, we shouldn't use `const` on the flash context, unless the implementation is so simple that we can be sure.
File writeprotect.c:
PS22: A general note about coding style: You don't make much use of the `const` keyword. It can make the code much easier to read because the reader can always be sure that the variable holds its initial value. So I prefer to use it for all variables and parameters when possible, except for very small functions where one sees immediately that no assignments happen.
Modern languages like Rust made immutable variables the default \o/
https://review.coreboot.org/c/flashrom/+/58479/comment/17396b9e_51773c1b PS22, Line 33: struct reg_bit_info bit This would copy the whole struct on every call. Just seems uncommon.
https://review.coreboot.org/c/flashrom/+/58479/comment/1462f9ac_c87c3a56 PS22, Line 35: 0 Will this ever be consumed?
https://review.coreboot.org/c/flashrom/+/58479/comment/15f53ae5_f9edcffa PS22, Line 48: ** Library functions should be in a respective documentation group, e.g. @defgroup or @addtogroup.
https://review.coreboot.org/c/flashrom/+/58479/comment/7ce73e0d_aae350c5 PS22, Line 59: * No dangling asterisk, please.
https://review.coreboot.org/c/flashrom/+/58479/comment/ef9287b7_8c24454e PS22, Line 59: intrepret int*er*pret
https://review.coreboot.org/c/flashrom/+/58479/comment/82921c8e_3b2c7e48 PS22, Line 61: tmp rather `unused` or `ignored`?
https://review.coreboot.org/c/flashrom/+/58479/comment/3e392492_6f9f4378 PS22, Line 63: *cfg = (struct wp_chip_config) {0}; Won't this be overwritten where it matters? iow. will any 0 ever be consumed?
https://review.coreboot.org/c/flashrom/+/58479/comment/141764ef_406209cb PS22, Line 66: |= Does it make sense to continue if reading failed?
https://review.coreboot.org/c/flashrom/+/58479/comment/dd43046a_30ce303d PS22, Line 70: r( Missing space.
https://review.coreboot.org/c/flashrom/+/58479/comment/906a5714_a66631ee 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 avoid overflows without further costs. e.g.
for (i = 0; i < ARRAY_SIZE(cfg->bp); ++i) { if (bits->bp[i].reg == INVALID_REG) break; ...
or just put both in the for condition
for (i = 0; i < ARRAY_SIZE(cfg->bp) && bits->bp[i].reg != INVALID_REG; ++i) {
https://review.coreboot.org/c/flashrom/+/58479/comment/2266d827_a1249332 PS22, Line 70: size_t Declarations inside a `for` made trouble in some environment. Please avoid it for now.
https://review.coreboot.org/c/flashrom/+/58479/comment/5be0d602_8cef9385 PS22, Line 75: r( Missing space.
https://review.coreboot.org/c/flashrom/+/58479/comment/6489c05b_13f62a8e PS22, Line 78: } You could also put the loop into a function, to avoid repeating it, e.g. read_reg_bits().
https://review.coreboot.org/c/flashrom/+/58479/comment/6bdb81c5_b7ff2273 PS22, Line 84: * No dangling asterisk, please. Or maybe just use the long comment style with line breaks at the beginning/end. I think that's more common at the top level.
https://review.coreboot.org/c/flashrom/+/58479/comment/bce72fa7_4d726135 PS22, Line 86: uint8_t *reg_values, uint8_t *write_masks You can also write these as `uint8_t reg_values[MAX_REGISTERS]` to make the intention clear.
https://review.coreboot.org/c/flashrom/+/58479/comment/786dddc8_434c7b8b PS22, Line 122: INVALID_REG This is very confusing. I see it's skipped because the mask would be 0, but having to check that makes the code harder to read.
https://review.coreboot.org/c/flashrom/+/58479/comment/89fdf11a_41470051 PS22, Line 126: = 0 Why initialize?
https://review.coreboot.org/c/flashrom/+/58479/comment/e846f72a_57f51a0f PS22, Line 131: & write_masks[reg] This is not necessary as reg_values[] was initialized to 0.
https://review.coreboot.org/c/flashrom/+/58479/comment/24b7e0e9_0947d96c PS22, Line 144: configuraitons configura*ti*ons
https://review.coreboot.org/c/flashrom/+/58479/comment/a085366a_7c6f07a7 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 unset is preferred?