On Wed, 6 Jul 2016 01:57:59 +0530 Hatim Kanchwala hatim@hatimak.me wrote:
Following patches add new infrastructure for status register(s) and locking/unlocking of access protection. Detailed points for explaining each patch are included within them.
Some general thoughts -
- The code has a lot TODOs and FIXMEs (most of which I have addressed
to myself). They are short-term targets and represent the work-in-progress nature of the patches. Future revisions will resolve them.
- There are a lot of comments. Eventually much of that will be
relocated to documentation and wiki, so comments in future revisions will shrink.
I don't see a need to do so. Most of what I have seen so far belongs to the code and not the wiki. Maybe the howto on adding a flash chip should be expanded in the wiki though... when this patch set is completed.
- Some CLI code is borrowed from the ChromiumOS fork
(https://chromium.googlesource.com/chromiumos/third_party/flashrom/). IMO, the exact words to use for the options is subjective.
Let's postpone the CLI discussion altogether. My stance on this in general is that we might not need/want to support all features in the "classic" CLI.
- The names for struct status_register and struct wp defines (in
statusreg_layouts.c and writeprotect_layouts.c respectively) are not very creative and tend to be long. I couldn't come up with a generic scheme that captures most of the information regarding it. You are welcome to offer creative suggestions ;)
Further work -
- Read and write "configuration" registers (a lot of Spansion chips
have those)
- Add tested/untested status for new infrastructure similar to what
we have for chips (like TEST_PREW, etc.)
- write protection modes
- protection ranges
Updating the status bits is already a massive time sink I do not intend to dig even deeper. However, making it more obvious to developers which chips need further work (similar to util/list_yet_unsupported_chips.sh) might be useful. This can even be in the form of -L or -z output, however it should only indicate if an implementation is available and gather this information implicitly from pointers etc. without any dedicated TESTED_* flags. That way maintenance costs are minimal.
- Access protection for non-SPI chips (pointed to by Stefan over IRC)
We definitely want at least a proof of concept that includes support for that before we finish the general API of SRs and WPs.
- Reuse layouts for locking/unlocking (pointed to by Stafan and David
over IRC)
I deem this a CLI aspect.
- Handling WPS bit (a couple of GigaDevice chips have this)
- Exotic chips (mostly Atmel, now Adesto, chips fall under this
category)
Adding support for a AT45DB chip might be useful to see how generic the new API is (or need to be). Their wicked configurations helped me in the past to iron out some completely unrelated bugs. I have not checked if they support any WP scheme though.
Now to the patches themselves... this is obviously no in-depth review. Too many superficial things came up and need to be sorted out/discussed first.
- Don't use char for bytes or integral/countable types like in char pos_bit(...)
- Don't use C99 loop variable declarations because this leads to compile errors with old compilers. The alternative is to use -std=c99 but we are not ready to do that yet.
- static struct range ranges[LEN_RANGES] in writeprotect.c
Doesn't this explode if multiple chip definitions are matched in probes? In any case you should definitely not return pointers to local variables even if they are static. This is very bad API design because it is not obvious to the caller that the data pointed to might be changed outside of its control (cf. strtok and others that used to use an internal static buffer as well but have been superseded by re-entrant variants)
- Initially I was skeptical about the use of pointers to pre-defined structures for the SR and WP definitions. I proposed that myself but only because the alternative shown in the first PoC seemed to be even worse. Looking at the code I felt that it it might disperse the chip definitions too much. I have then played a bit with your code and compared the binary sizes of 0.9.9, your patches and my "fix" on top. I replaced the pointer to the struct status_register in struct flashchip with direct struct instances. These are the results:
0.9.9 text data bss dec hex filename 517381 2268 11712 531361 81ba1 flashrom
hatim: text data bss dec hex filename 554373 11228 12256 577857 8d141 flashrom
stefanct: text data bss dec hex filename 637037 6812 12256 656105 a02e9 flashrom
Your implementation adds about 40kB while the direct instantiation in my try adds almost 120kB. However, this is a maximum because the space for all structs is already pre-allocated and "initialized" in the image whereas for your implementation further structs would need to be added and only space for the pointers are reserved. Additionally, your data section would add to run-time memory usage.
With that in mind I think technically it does not make much sense to separate the definitions. However, this is only one aspect. One other is to provide means to share information. We naturally do this by sharing function pointers and there it has been a huge success because it allows us to easily spot and exploit similarities between chip families. So the separation itself is probably a good thing and I think you went into the right direction with it.
But maybe we can improve on the space requirements of the structs themselves because they are really huge in comparison to the remaining data in struct flashchip. For example: enum status_register_bit layout[MAX_STATUS_REGISTERS + 1][8] would cost us 8*4*sizeof(int) bytes per chip/status register definitoin with most compilers, i.e. 50 kB for 400 SR definitions and 32-bit integers. I tend to ignore this mostly because that's not that much for ordinary systems but e.g. WLAN routers are still stuck with a few MB of flash and there these 50 kB could be spent better. However, since we share these definitions in your implementation a lot it should be way less than these 50 kB... but if you can think of an approach that uses less memory without making the code much more complicated I am all for it. Maybe we could use some feature_bits to indicate the number of SRs of the respective chip and can get rid of the +1 at least?
- The function prototypes you added to chipdrivers.h should be moved to the respective header files.
- Some of the _generic functions in writeprotect are not really that generic. They almost only deal with SPI-specific behavior and should neither be named so generically and possibly not even reside in that module but spi25_statusreg.c instead. Please try to better abstract the generic WP API (as used by the function pointers) from the SPI-specific implementations. BTW we might get away without any print and print_wp_mode function pointers because they should simply print the generic representation, no? So spi_prettyprint_status_register_generic for example might actually be an actual generic function to be moved to a more general file (it currently resides in spi25_statusreg.c).
- Please comment: static char const *bit_string[3][32] uint8_t convention = 1, multiple = 0
- Please exploit and refine our dummy programmer to test the status register and OTP implementations of at least some variations. Especially simulating the latter makes sense in regression testing for obvious reasons :)
- Many of above generic statements apply to OTP as well, e.g. the print_status pointer might be superfluous.
- What about OTP in non-SPI chips? Have you researched that yet? I can't remember seeing that anywhere but in new chip generations I would not be surprised if there is support for it.
Don't despair due to the amount of criticism you might perceive. In general I think you are doing a very good job! I am fully aware that the goal of implementing all of this in a generic way is not easy but it is necessary to make it maintainable in the future. Thanks for your work!
Hope that helps a bit and please let me know if anything is unclear. I also might have not grasp everything and might be wrong on some points. Don't hesitate to correct me in that case :)