Attention is currently required from: Edward O'Callaghan, Angel Pons, Nikolai Artemiev, Sergii Dmytruk.
24 comments:
File libflashrom.h:
Patch Set #22, Line 125: software
If it's software protection, why not name it like that?
Please move to the commit that makes use of it.
Patch Set #22, 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:
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/
Patch Set #22, Line 33: struct reg_bit_info bit
This would copy the whole struct on every call. Just seems uncommon.
Will this ever be consumed?
Library functions should be in a respective documentation group, e.g. @defgroup or @addtogroup.
No dangling asterisk, please.
Patch Set #22, Line 59: intrepret
int*er*pret
rather `unused` or `ignored`?
Patch Set #22, Line 63: *cfg = (struct wp_chip_config) {0};
Won't this be overwritten where it matters? iow. will any 0 ever be consumed?
Does it make sense to continue if reading failed?
Missing space.
Patch Set #22, 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) {Patch Set #22, Line 70: size_t
Declarations inside a `for` made trouble in some environment. Please avoid it for now.
Missing space.
You could also put the loop into a function, to avoid repeating it, e.g.
read_reg_bits().
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.
Patch Set #22, 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.
Patch Set #22, 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.
Why initialize?
Patch Set #22, Line 131: & write_masks[reg]
This is not necessary as reg_values[] was initialized to 0.
Patch Set #22, Line 144: configuraitons
configura*ti*ons
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?
To view, visit change 58479. To unsubscribe, or for help writing mail filters, visit settings.