[flashrom] enum test_state value for "depends"

[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>
> 
> 
> > 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");
	}

> > @@ -1862,7 +1868,7 @@ int chip_safety_check(const struct flashctx *flash, int force, int read_it, int
> >  
> >  	if (read_it || erase_it || write_it || verify_it) {
> >  		/* Everything needs read. */
> > -		if (chip->tested & TEST_BAD_READ) {
> > +		if (chip->tested.read == BAD) {
> >  			msg_cerr("Read is not working on this chip. ");
> >  			if (!force)
> >  				return 1;
> 
> We need an additional check for NA with message "This chip does not
> support read", "This chip has no read functionality" or something
> similar. We should try to have the messages for NA and BAD differ in a
> way which makes it easy for users (or at least for us) to understand the
> difference.
> Same for the cases below.

Yes. I have added
		if (chip->tested.erase == NA) {
			msg_cerr("Erase is not possible on this chip.\n");
			return 1;
		}

and
		if (chip->tested.write == NA) {
			msg_cerr("Write is not possible on this chip.\n");
			return 1;
		}
at the top of the respective checks. Wording: "not possible" vs. "not
working" and there is no way to force it.

> > @@ -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.

> > @@ -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?

> 
> 
> >  	return 1;
> >  }
> >  
> > diff --git a/print_wiki.c b/print_wiki.c
> > index 5dcc4b6..2dc8156 100644
> > --- a/print_wiki.c
> > +++ b/print_wiki.c
> > @@ -287,7 +286,35 @@ static void print_supported_chips_wiki(int cols)
> >  			c = !c;
> >  
> >  		old = f;
> > -		t = f->tested;
> > +		const char *probe, *read, *write, *erase;
> > +		switch (f->tested.probe) {
> > +			case OK: probe = "OK"; break;
> > +			case BAD: probe = "No"; break;
> > +			case NA: probe = "NA"; break;
> 
> Could you please print "N/A" here and in the cases below for
> consistency? We already print "N/A" if the voltage is unknown.

No, because the NA is a symbol for a mediawiki template that changes
the background of the table cell too while the N/A for the voltage is
literal. And I think we want it like that (i.e. we dont want to change
the background color for N/A voltages because that information is not
very important. hm... that N/A for voltages is actually wrong, because
of course they have allowed voltage ranges... we just don't have it set
yet. I'll change that to a '?' just like the untested state is handled
for the tested state (just without the background color change).

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).

> > +			case DEP: probe = "Dep"; break;
> > +			default: probe = "?3"; break;

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