[flashrom] enum test_state value for "depends"

[Stefan Tauner]

On Sun, 25 May 2014 23:31:40 +0200
Carl-Daniel Hailfinger <c-d.hailfinger.devel.2006 at gmx.net> wrote:

> Am 25.05.2014 11:09 schrieb Stefan Tauner:
> > On Sun, 25 May 2014 02:35:44 +0200
> > Carl-Daniel Hailfinger <c-d.hailfinger.devel.2006 at gmx.net> wrote:
> >
> >> Am 09.05.2014 18:33 schrieb Stefan Tauner:
> >>> Subject: [PATCH] Add two new states to enum test_state and use it for
> >>>  flashchips.
> >>>
> >>> The new enum test_state looks like this:
> >>> enum test_state {
> >>> 	OK = 0,
> >>> 	NT = 1,	/* Not tested */
> >>> 	BAD,	/* Known to not work */
> >>> 	DEP,	/* Support depends on configuration (e.g. Intel flash descriptor) */
> >>> 	NA,	/* Not applicable (e.g. write support on ROM chips) */
> >>> };
> >>>
> >>> The second new state 'NA' is introduced, among other things, to indicate
> >>> the erase and write states of real ROMs correctly. This is also implemented
> >>> by this patch and required to exchange the previous bit mask in struct
> >>> flashchip with a new struct containing an enum test_state for each operation.
> >>> The -L output is changed accordingly to print '-' in the case of an N/A state
> >>> and the wiki output uses a new template producing a greyed out cell.
> >>> Previous users of enum test_state are not affected by this change (yet).
> >>>
> >>> Signed-off-by: Stefan Tauner <stefan.tauner at alumni.tuwien.ac.at>
> >> Excellent work, thank you!
> >> I particularly like the #define TEST_* trickery to keep flashchips.c
> >> readable and relatively unchanged.
> >> Except for some corner cases of NA handling as explained below, this is
> >>
> >> Acked-by: Carl-Daniel Hailfinger <c-d.hailfinger.devel.2006 at gmx.net>

Thanks, committed in r1798.

> >>> diff --git a/flashrom.c b/flashrom.c
> >>> index c20461a..4956ded 100644
> >>> --- a/flashrom.c
> >>> +++ b/flashrom.c
> >>> @@ -1794,32 +1794,38 @@ void check_chip_supported(const struct flashchip *chip)
> >>>  			 "clone the contents of this chip (see man page for "
> >>>  			 "details).\n");
> >>>  	}
> >> AFAICS the test for OTP memory should be extended with a check for ROM
> >> chips (i.e. erase/write == NA) where writing isn't possible. Given that
> >> this function has informational purposes and does not return any value,
> >> it does serve a different purpose compared to chip_safety_check(). It
> >> will get called even if none of read/erase/write is specified. My
> >> opinion on this is not cast in stone, though.
> > What about this:
> > 	if ((chip->tested.erase == NA) && (chip->tested.write == NA)) {
> > 		msg_cdbg("This chip's main memory can not be altered by design (i.e. it is a ROM).\n");
> > 	}
> >
> > In case we ever support (non-E but UV) EPROMs we might also like to
> > have:
> > 	if ((chip->tested.erase == NA) && (chip->tested.write != NA)) {
> > 		msg_cdbg("This chip's main memory can not be erased electrically (i.e. it is an EPROM).\n");
> > 	}
> 
> Sounds good. I'd rephrase a bit to get rid of the "i.e.":
> "This ROM chip can not be written/erased by design"

I'd like to avoid the word "ROM" in general texts so... and I'd like to
keep the more precise main memory bit:
"This chip's main memory can not be erased/written by design."

> Can we ignore EPROMs for now? We have no way to differentiate between
> PROMs and EPROMs with the new test_state design. Both have erase==NA and
> write!=NA.

True. I have left that if clause out.

> >>> @@ -1876,7 +1882,7 @@ int chip_safety_check(const struct flashctx *flash, int force, int read_it, int
> >>>  	}
> >>>  	if (erase_it || write_it) {
> >>>  		/* Write needs erase. */
> >>> -		if (chip->tested & TEST_BAD_ERASE) {
> >>> +		if (chip->tested.erase == BAD) {
> >>>  			msg_cerr("Erase is not working on this chip. ");
> >>>  			if (!force)
> >>>  				return 1;
> > BTW this check (and the whole write logic) makes assumptions that dont
> > hold for EPROMs. Not an issue (yet), just noticing. Another related
> > case ist he Intel NIC EEPROM driver. It does not need an explicit erase
> > either because it is implicit on writes... but that uses the opaque
> > interface where we set the chip flags unconditionally anyway.
> 
> We'll cross that bridge when we merge the EEPROM driver.

Yes, and it is tagged as a fixme in it but I dont think it is a show
stopper... off-topic here anyway.

> >>> @@ -1236,7 +1236,10 @@ static int ich_hwseq_probe(struct flashctx *flash)
> >>>  		msg_cdbg("In that range are %d erase blocks with %d B each.\n",
> >>>  			 size_high / erase_size_high, erase_size_high);
> >>>  	}
> >>> -	flash->chip->tested = TEST_OK_PREW;
> >>> +	flash->chip->tested.probe = OK;
> >>> +	flash->chip->tested.read = OK;
> >>> +	flash->chip->tested.erase = OK;
> >>> +	flash->chip->tested.write = OK;
> >> AFAICS a compound literal should have worked here.
> >> flash->chip->tested = (struct tested)TEST_OK_PREW;
> > Oh, I was looking for something like that :)
> > Hm... seems to work when used directly in the definition too... should
> > I add it there instead or are there caveats?
> 
> Your choice.

Let's hope it does not break any compiler... (I hate if I have to say
that... never a good sign :P)

> > The template handling is btw something I'd like to move into flashrom's
> > code. Currently we rely on some mediawiki templates to be set
> > correctly. I'd rather define these things completely within the
> > source... unless we implement proper theming support of wiki output -
> > which we never will, hopefully. (The actual rationale is: we decide
> > all kinds of formatting stuff in the C code and there is no reason to
> > outsource some of them to the template engine of mediawiki).
> 
> To be honest... he who wrangles print_wiki.c gets to decide such stuff.

I'll put it on my todo list...

-- 
Kind regards/Mit freundlichen Grüßen, Stefan Tauner