[flashrom] enum test_state value for "depends"

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


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

-- 
http://www.hailfinger.org/