On Mon, 26 Dec 2011 20:39:21 +0100 Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@gmx.net wrote:
Am 24.12.2011 01:35 schrieb Stefan Tauner:
this still reads the whole image in...
Signed-off-by: Stefan Tauner stefan.tauner@student.tuwien.ac.at
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. ;)