Re: [flashrom] [PATCH 4/5] Use layout for verify operations too

[Carl-Daniel Hailfinger]

Am 24.12.2011 01:35 schrieb Stefan Tauner:
> this still reads the whole image in...
>
> Signed-off-by: Stefan Tauner

This kills the flashrom reliability guarantee. If someone specifies an
image for inclusion and flashing goes wrong outside the image region
(accidental erase for a too large region, may happen for Macronix
eLiteFlash which has the same ID as normal Macronix flash or for any
chip/programmer driver bug), flashrom won't notice and the user is left
with garbage in the chip despite a "SUCCESS" message.

The only case where skipping verify of any region is allowable is an
unreadable region. Such an unreadable region should be skipped with a
message along the lines of "Skipping verify for inaccessible region
0xfoo-0xbar".

And in the end it boils down to the old problem: How do we specify
regions and their characteristics?

Suggestion, more of an RFC than a hard proposal:

struct region{
char *name;
unsigned int rwflags;
unsigned int start;
unsigned int end;
_Bool included;
};

rwflags would be a bit for each of may_write and may_read, or a bit for
each of dont_write and dont_read. Not sure which one is preferable.

This also means we need a new layout file version.

Regards,
Carl-Daniel

[PATCH 4/5] Use layout for verify operations too

[Stefan Tauner]

On Mon, 26 Dec 2011 20:39:21 +0100
Carl-Daniel Hailfinger  wrote:
> Am 24.12.2011 01:35 schrieb Stefan Tauner:
> > this still reads the whole image in...
> >
> > Signed-off-by: Stefan Tauner
>
> This kills the flashrom reliability guarantee. If someone specifies an
> image for inclusion and flashing goes wrong outside the image region
> (accidental erase for a too large region, may happen for Macronix
> eLiteFlash which has the same ID as normal Macronix flash or for any
> chip/programmer driver bug), flashrom won't notice and the user is left
> with garbage in the chip despite a "SUCCESS" message.

right, this is nonsense. the patch afaics changes the verification not
only for -v but also for -w runs. how do you feel about changing the -v
operation only? my gut instinct screams "inconsistency!" but it might
be useful and the observed inconsistency real isn't one: the
verification in write mode needs to guarantee that old data is not
changed, but this is not necessary in -v.
if anyone would use this is another question of course :)
> The only case where skipping verify of any region is allowable is an
> unreadable region. Such an unreadable region should be skipped with a
> message along the lines of "Skipping verify for inaccessible region
> 0xfoo-0xbar".

jup, but this has nothing to do with what the user actually requested us
to do. this patch set focused on the CLI.
> And in the end it boils down to the old problem: How do we specify
> regions and their characteristics?
>
> Suggestion, more of an RFC than a hard proposal:
>
> struct region{
> char *name;
> unsigned int rwflags;

might also use _Bool, or bit fields.
both have the disadvantage that you cant access both bits
"concurrently". the question is if we will have to access them more
often together or individually.
we should probably implement macros to allow both no matter what the
data structure is(?)
> unsigned int start;
> unsigned int end;

or we could start introducing a flash address type and #define it to be
unsigned int? not really related, but id like to see something like
that sooner rather then later and this would be a good starting point :)
> _Bool included;

char *comment; // not to be used in a lot of places but at least in the
layout file itself. dunno if it makes sense to parse it (and hence
include in the data structure). 
we could specify information regarding the origin of a region and/or an
error message that explains the origin though.
by origin i mean that we will have various sources of access rights:
layout files, CLI switches, chips, programmers (and boards)... it might
be useful to know where it comes from in cases of errors or uncommon
behavior like your example above...
"Skipping verify for inaccessible region 0xfoo-0xbar specified by
programmer 'ichspi'".
> };
>
> rwflags would be a bit for each of may_write and may_read, or a bit for
> each of dont_write and dont_read. Not sure which one is preferable.

hm. i wonder if we need more than that... namely a "don't care" state
for both (read and write). this would be useful to define requested
operations and unify them with access rights.
for example the CLI + layout information could be put into a collection
of regions with read and write bits set appropriately and any
restriction from the flash chip might be queried and returned by a
similar collection. before the process is carried out we unify the
collections and look for conflicts. we would need/want a "verify" and
"erase" flag too then...

i have only a vague idea where we will use the region data structure. we
should probably make a list of use cases and needed fields.
> This also means we need a new layout file version.

i dont like the current one anyway. hex numbers without a pre-/suffixes
are just wrong. ;)