[flashrom] [PATCH 0/6] Initial status register and access protection infrastructure

[Stefan Tauner]

On Wed,  6 Jul 2016 01:57:59 +0530
Hatim Kanchwala <hatim at 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 :)
-- 
Kind regards/Mit freundlichen Grüßen, Stefan Tauner