enum test_state value for "depends"

[Stefan Tauner]

We currently have an
enum test_state {
    OK = 0,
    NT = 1,	/* Not tested */
    BAD
};

This is used in various arrays, namely those for
boards, board enables, chipsets, programmers, and rayer_spi (LPT)
devices. I'd like to add another state that indicates that support
is configuration-dependent. Most infamous use cases would be boards
with ME chipsets and ME chipsets. Another example is the atavia VT6421A
LPC programmer which works in general, just not all boards due to a not
fully understood offset configuration that is board-specific.

Names... CONFIG is quite long but obvious to understand, CD for
configuration-dependent would suite me too, but I am open to
suggestions. What about DEP?

[Carl-Daniel Hailfinger]

Am 01.05.2014 18:57 schrieb Stefan Tauner:
> We currently have an
> enum test_state {
>    OK = 0,
>    NT = 1,	/* Not tested */
>    BAD
> };
>
> This is used in various arrays, namely those for
> boards, board enables, chipsets, programmers, and rayer_spi (LPT)
> devices. I'd like to add another state that indicates that support
> is configuration-dependent. Most infamous use cases would be boards
> with ME chipsets and ME chipsets. Another example is the atavia VT6421A
> LPC programmer which works in general, just not all boards due to a not
> fully understood offset configuration that is board-specific.
>
> Names... CONFIG is quite long but obvious to understand, CD for
> configuration-dependent would suite me too, but I am open to
> suggestions. What about DEP?

DEP or RND (given that from flashrom's POV it's essentially random). But
yes, DEP sounds good. Thanks for introducing that valuable way to convey
information.

Ack.

Regards,
Carl-Daniel

[Stefan Tauner]

On Thu, 08 May 2014 23:26:51 +0200
Carl-Daniel Hailfinger  wrote:
> Am 01.05.2014 18:57 schrieb Stefan Tauner:
> > We currently have an
> > enum test_state {
> >    OK = 0,
> >    NT = 1,	/* Not tested */
> >    BAD
> > };
> >
> > This is used in various arrays, namely those for
> > boards, board enables, chipsets, programmers, and rayer_spi (LPT)
> > devices. I'd like to add another state that indicates that support
> > is configuration-dependent. Most infamous use cases would be boards
> > with ME chipsets and ME chipsets. Another example is the atavia VT6421A
> > LPC programmer which works in general, just not all boards due to a not
> > fully understood offset configuration that is board-specific.
> >
> > Names... CONFIG is quite long but obvious to understand, CD for
> > configuration-dependent would suite me too, but I am open to
> > suggestions. What about DEP?
>
> DEP or RND (given that from flashrom's POV it's essentially random). But
> yes, DEP sounds good. Thanks for introducing that valuable way to convey
> information.
>
> Ack.

Good, I'll take care of that. There is another, related thing... since
we support ROMs too now, we might think about an N/A state and/or
feature flag for e.g. write/erase operations of ROMs. Sure, we can tag
them with "no" (support), but that creates stinging red boxes in the
wiki and I don't like that. :)

[Stefan Tauner]

Design suggestion: http://www.flashrom.org/User_talk:Stefanct#New_states

[Stefan Tauner]

The following two patches include the new states in enum test_state and
change the previous .tested field in struct flashchip to a sub-struct
containing 4 enum test_state (one for each operation).

Both are semantically the same. The difference is that one renames
the .tested field of the struct to .state which helped a lot during
development of the patch, while the other one does simply replace the
old semantics of .tested without renaming it. The patch w/o the rename
is easier to read and much smaller, but it might be useful to change
the field and associated macros nevertheless to have a clear indication
of the change and to clearly distinguish patches following the old
style from newer ones.

[Carl-Daniel Hailfinger]

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.
> if (TEST_OK_MASK != (chip->tested & TEST_OK_MASK)) {
> if ((chip->tested.probe == BAD) || (chip->tested.probe == NT) ||
>    (chip->tested.read == BAD)  || (chip->tested.read == NT) ||
>    (chip->tested.erase == BAD) || (chip->tested.erase == NT) ||
>    (chip->tested.write == BAD) || (chip->tested.write == NT)){
> msg_cinfo("===\n");
> if (chip->tested & TEST_BAD_MASK) {
> if ((chip->tested.probe == BAD) ||
>     (chip->tested.read == BAD) ||
>     (chip->tested.erase == BAD) ||
>     (chip->tested.write == BAD)) {
> msg_cinfo("This flash part has status NOT WORKING for operations:");
> 	if (chip->tested & TEST_BAD_PROBE)
> 	if (chip->tested.probe == BAD)
> 	msg_cinfo(" PROBE");
> 	if (chip->tested & TEST_BAD_READ)
> 	if (chip->tested.read == BAD)
> 	msg_cinfo(" READ");
> 	if (chip->tested & TEST_BAD_ERASE)
> 	if (chip->tested.erase == BAD)
> 	msg_cinfo(" ERASE");
> 	if (chip->tested & TEST_BAD_WRITE)
> 	if (chip->tested.write == BAD)
> 	msg_cinfo(" WRITE");
> msg_cinfo("\n");
> }
> if ((!(chip->tested & TEST_BAD_PROBE) && !(chip->tested & TEST_OK_PROBE)) ||
>     (!(chip->tested & TEST_BAD_READ) && !(chip->tested & TEST_OK_READ)) ||
>     (!(chip->tested & TEST_BAD_ERASE) && !(chip->tested & TEST_OK_ERASE)) ||
>     (!(chip->tested & TEST_BAD_WRITE) && !(chip->tested & TEST_OK_WRITE))) {
> if ((chip->tested.probe == NT) ||
>     (chip->tested.read == NT) ||
>     (chip->tested.erase == NT) ||
>     (chip->tested.write == NT)) {
> msg_cinfo("This flash part has status UNTESTED for operations:");
> 	if (!(chip->tested & TEST_BAD_PROBE) && !(chip->tested & TEST_OK_PROBE))
> 	if (chip->tested.probe == NT)
> 	msg_cinfo(" PROBE");
> 	if (!(chip->tested & TEST_BAD_READ) && !(chip->tested & TEST_OK_READ))
> 	if (chip->tested.read == NT)
> 	msg_cinfo(" READ");
> 	if (!(chip->tested & TEST_BAD_ERASE) && !(chip->tested & TEST_OK_ERASE))
> 	if (chip->tested.erase == NT)
> 	msg_cinfo(" ERASE");
> 	if (!(chip->tested & TEST_BAD_WRITE) && !(chip->tested & TEST_OK_WRITE))
> 	if (chip->tested.write == NT)
> 	msg_cinfo(" WRITE");
> msg_cinfo("\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.
> @@ -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;
>
> @@ -1889,7 +1895,7 @@ int chip_safety_check(const struct flashctx *flash, int force, int read_it, int
>    	}
>    }
>    if (write_it) {
> if (chip->tested & TEST_BAD_WRITE) {
> if (chip->tested.write == BAD) {
> msg_cerr("Write is not working on this chip. ");
> if (!force)
> 	return 1;
>
> diff --git a/ichspi.c b/ichspi.c
> index 6c394db..c12ad0d 100644
> --- a/ichspi.c
> +++ b/ichspi.c
> @@ -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;
> 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.
> 	case DEP: probe = "Dep"; break;
> 	default: probe = "?3"; break;
> }
> switch (f->tested.read) {
> 	case OK: read = "OK"; break;
> 	case BAD: read = "No"; break;
> 	case NA: read = "NA"; break;
> 	case DEP: read = "Dep"; break;
> 	default: read = "?3"; break;
> }
> switch (f->tested.erase) {
> 	case OK: erase = "OK"; break;
> 	case BAD: erase = "No"; break;
> 	case NA: erase = "NA"; break;
> 	case DEP: erase = "Dep"; break;
> 	default: erase = "?3"; break;
> }
> switch (f->tested.write) {
> 	case OK: write = "OK"; break;
> 	case BAD: write = "No"; break;
> 	case NA: write = "NA"; break;
> 	case DEP: write = "Dep"; break;
> 	default: write = "?3"; break;
> }
> s = flashbuses_to_text(f->bustype);
> sprintf(vmin, "%0.03f", f->voltage.min / (double)1000);
> sprintf(vmax, "%0.03f", f->voltage.max / (double)1000);

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

[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

[Stefan Tauner]

On Sun, 25 May 2014 23:31:40 +0200
Carl-Daniel Hailfinger  wrote:
> 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

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

[Stefan Tauner]

On Mon, 26 May 2014 02:37:02 +0200
Stefan Tauner  wrote:
> > > 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...

That's actually not so easy to achieve. The cells are currently styled
by inline CSS, e.g. the template for an OK cell contains:

style="background:lime; color:black; text-align:center;" class="table-ok" | {{{1|OK}}}

Of course we could just output this for every cell, implementing our
own trivial template engine in C (which just inserts the text as above
where previously the template name was printed - really trivial).
The downside of that is that it creates huge output, about 400kB
currently. That's not too bad IMHO... apart from breaking the form
text field of MediaWiki in Opera - but Opera is dead anyway.

I have looked for workarounds... using CSS in the html header would be
the most sane one. It is of course not possible to insert something in
the header from a normal wiki page, without extensions... there are
extensions to allow definition of CSS, but I would rather refrain from
using that and keep templates instead.

Another alternative would be some kind of page-local/inline templates.
This is also possible with extensions (using so-called variables) and I
don't want to use that for the same reason as above.

So my conclusion is to keep using templates for the time being, unless
someone knows another way to achieve my goal.

[PATCH] print_wiki: Expand previous MediaWiki templates within C.

[Stefan Tauner]

Signed-off-by: Stefan Tauner 
---

On top of r1804 the discussed approach of expanding the templates within
flashrom would look like below and increase the -z output
from ~186 kB to 393 kB. This is not for merge IMO.

print_wiki.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/print_wiki.c b/print_wiki.c
index b01ab75..615925d 100644
--- a/print_wiki.c
+++ b/print_wiki.c
@@ -101,12 +101,12 @@ static const char programmer_th[] = "\
 static const char *test_state_to_template(enum test_state test_state)
 {
    switch (test_state) {
-	case OK: return "OK";
-	case BAD: return "No";
-	case NA: return "NA";
-	case DEP: return "Dep";
+	case OK: return "style="background:lime; color:black; text-align:center;" class="table-ok" | {{{1|OK}}}";
+	case BAD: return "style="background:red; color:black; text-align:center;" class="table-no" | {{{1|No}}}";
+	case NA: return "style="background:Gainsboro; color:Gray; text-align:center;" class="table-na" | {{{1|N/A}}}";
+	case DEP: return "style="background:orange; color:black; text-align:center;" class="table-deb" | {{{1|Dep}}}";
    case NT:
-	default: return "?3";
+	default: return "style="background:#bfff00; color:black; text-align:center;" class="table-unknown" | {{{1|?}}}";
    }
 }

@@ -207,7 +207,7 @@ static void print_supported_boards_wiki_helper(const char *devicetype, int cols,
    	}

printf("|- bgcolor="#%s"\n| %s || %s%s %s%s || %s%s%s%s "
-		       "|| {{%s}}", (color) ? "eeeeee" : "dddddd",
+		       "|| %s", (color) ? "eeeeee" : "dddddd",
    	       boards[i].vendor,
    	       boards[i].url ? "[" : "",
    	       boards[i].url ? boards[i].url : "",
@@ -304,7 +304,7 @@ static void print_supported_chips_wiki(int cols)
    	sprintf(vmin, "%0.03f", f->voltage.min / (double)1000);
    	sprintf(vmax, "%0.03f", f->voltage.max / (double)1000);
    	printf("|- bgcolor="#%s"\n| %s || %s || align="right" | %d "
-		       "|| %s || {{%s}} || {{%s}} || {{%s}} || {{%s}}"
+		       "|| %s || %s || %s || %s || %s"
    	       "|| %s || %s \n",
    	       (c == 1) ? "eeeeee" : "dddddd", f->vendor, f->name,
    	       f->total_size, s,
@@ -351,7 +351,7 @@ static void print_supported_devs_wiki_helper(const struct programmer_entry prog)
    	printf("|- bgcolor="#%s"\n", (c) ? "eeeeee" : "dddddd");
    	if (i == 0)
    		printf("| rowspan="%u" | %s |", count, prog.name);
-		printf("| %s || %s || %04x:%04x || {{%s}}\n", devs[i].vendor_name, devs[i].device_name,
+		printf("| %s || %s || %04x:%04x || %s\n", devs[i].vendor_name, devs[i].device_name,
    	       devs[i].vendor_id, devs[i].device_id, test_state_to_template(devs[i].status));
    }
 }