[flashrom] enum test_state value for "depends"
Carl-Daniel Hailfinger
c-d.hailfinger.devel.2006 at gmx.net
Sun May 25 23:31:40 CEST 2014
Am 25.05.2014 11:09 schrieb 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");
> }
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
--
http://www.hailfinger.org/
More information about the flashrom
mailing list