Attention is currently required from: Nico Huber, Edward O'Callaghan, Anastasia Klimchuk, Sergii Dmytruk.
16 comments:
Patchset:
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:
The final libdflashrom API / functionality would be something like:
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:
Patch Set #22, Line 131: const
We should be careful with `const` here. Flash access goes very deep down […]
Done
File writeprotect.c:
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.
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.
Patch Set #22, Line 59: intrepret
int*er*pret
Done
rather `unused` or `ignored`?
Done
Patch Set #22, 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.
Missing space.
Done
Patch Set #22, Line 70: size_t
Declarations inside a `for` made trouble in some environment. Please avoid it for now.
Done
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 […]
Done
Missing space.
Done
No dangling asterisk, please. Or maybe just use the long comment style […]
Done
Patch Set #22, 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.
Why initialize?
Done
Patch Set #22, Line 144: configuraitons
configura*ti*ons
Done
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.
To view, visit change 58479. To unsubscribe, or for help writing mail filters, visit settings.