On Sun, 25 May 2014 02:35:44 +0200 Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@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@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@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;