Re: [flashrom] enum test_state value for "depends"

[Carl-Daniel Hailfinger]

Am 25.05.2014 11:09 schrieb Stefan Tauner:
> On Sun, 25 May 2014 02:35:44 +0200
> Carl-Daniel Hailfinger  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
> >
> > 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
> > > 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"
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.
> > > @@ -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.

Thanks!
> > > @@ -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.
> > > @@ -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.
> > > 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).

Thanks!
> 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.
> > > 	case DEP: probe = "Dep"; break;
> > > 	default: probe = "?3"; break;

Regards,
Carl-Daniel