Am 24.12.2011 01:35 schrieb Stefan Tauner:
Add an optional sub-parameter to the -i parameter to allow building the image to be written from multiple files. This will also allow regions to be read from flash and written to separate image files in a later patch.
based on chromiumos' d0ea9ed71e7f86bb8e8db2ca7c32a96de25343d8 Signed-off-by: David Hendricks dhendrix@chromium.org
Signed-off-by: Stefan Tauner stefan.tauner@student.tuwien.ac.at
Just a few short comments, not a complete review yet. General comment: Author attribution seems to have been partially lost and/or another patch ("make read before write configurable") was mixed in.
Implementation issues: The function names are totally misleading. write_buf_to_file() does not write a buffer to a file anymore. write_dump_to_file() doesn't do what the function name says either. write_buf_to_file() should stay exactly like it is now unless someone redefines what a buffer or a file is. If you need a function which writes all flash chip contents to one file and parts of it to other files, call it write_chipcontents_to_files_with_layout or something like that.
Design issues: Specifying a file name from which flashrom should fetch a region and to which flashrom should write a region has three main issues: - It kills UI consistency. What happens if you have a region called "all" which covers the whole flash chip? Specifying a filename for region "all" means any supplied filename as --read or --write parameter will be completely ignored but still be required. - If one image is specified with a file name, will the file name for --read/--write be ignored? If not, this means we still can't deal with read-locked regions. If yes, what's the point in requiring a file name for --read/--write? - IMHO specifying region file names should happen in a layout file, but that's arguably debatable.
Regards, Carl-Daniel
On Sun, 25 Dec 2011 01:59:35 +0100 Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@gmx.net wrote:
Author attribution seems to have been partially lost and/or another patch ("make read before write configurable") was mixed in.
right. i mangled and merged three of the patches together when rebasing them for "Dispensored"'s X8DTU three days ago. i will repost the series now as layout 2.1 in the hope that i have separated them correctly (git helped a lot :) but please be aware of possible artifacts.
some of the comments do not regard the --image patch but later ones. i will work on those issues regardless of that - no need to re-comment them.
Design issues: Specifying a file name from which flashrom should fetch a region and to which flashrom should write a region has three main issues:
- It kills UI consistency. What happens if you have a region called
"all" which covers the whole flash chip? Specifying a filename for region "all" means any supplied filename as --read or --write parameter will be completely ignored but still be required.
for --read it is not ignored iirc but only those regions marked to be fetched via -i are included and the rest is filled with 0x00s. this creates a chip-sized file which might be useful to some e.g. if they want to manipulate it by editing some stuff. i will call this (i.e. files which are parameters to --write/--read/--verify but are used/filled only partially) "complete" files below. in the case of --read'ing "all" this would produce two identical files (i guess). for --write you are right to the point. i notices this problem but did not even try to mitigate it because i wanted to discuss the whole approached first so thanks for bringing it up :)
without thinking too much about the possible implementation i would say that we want to *require* the bare minimum of files and images for a given operation to complete it successfully.
currently i always write a "complete" image for --read operations but this might not be wanted and could be optional - if the -i option is used at least once. atm it is not necessary to give an image name because it defaults to the range's name. but this also forcibly creates files, when the -i option is used - even if the user does not need it. maybe we should let the desire for an image be defined by including the (optional) ':'?
for --write we need enough data to cover the to be written range(s). this can come from the "complete" image or the -i ones. and in case we care for the erase block layout we also need to be sure that the data is read from the chip where it is needed because it would get deleted but was not supplied/included via files. the latter is not an issue (yet) because the complete chip is (still) read in the case of writes.
- If one image is specified with a file name, will the file name for
--read/--write be ignored? If not, this means we still can't deal with read-locked regions. If yes, what's the point in requiring a file name for --read/--write?
if one disables the complete reads (via the variable of the infrastructure patch) it will not be ignored completely, but will not get in the way either (i.e. provide "background" data for write operations which is then not used for -i options where an image is also included or for read operations as "complete" image).
- IMHO specifying region file names should happen in a layout file, but
that's arguably debatable.
no harm in supporting both :) i did not do so due to the existing implementation in chromiumos and the needed layout file changes. the command line is easier to use in case you want to create differently named files of the same region - for example when working with multiple chips. precedence of data to be written (to the chip) should imho be: -i's file (name) > layout's file (name) > "complete" file
another point which needs to be discussed is the precedence of overlapping regions. without any -i's files given this is not an issue. we just merge the regions automatically - the data for a single address is the same anyway. when different files are given this is no longer true. i think the best option would be to abort and/or require --force in that case in addition to a defined precedence (e.g. last -i wins).
On Sun, Dec 25, 2011 at 8:42 AM, Stefan Tauner < stefan.tauner@student.tuwien.ac.at> wrote:
Design issues: Specifying a file name from which flashrom should fetch a region and to which flashrom should write a region has three main issues:
- It kills UI consistency. What happens if you have a region called
"all" which covers the whole flash chip? Specifying a filename for region "all" means any supplied filename as --read or --write parameter will be completely ignored but still be required.
for --read it is not ignored iirc but only those regions marked to be fetched via -i are included and the rest is filled with 0x00s. this creates a chip-sized file which might be useful to some e.g. if they want to manipulate it by editing some stuff. i will call this (i.e. files which are parameters to --write/--read/--verify but are used/filled only partially) "complete" files below.
Yes -- the idea here is to be able to quickly read only the specified region(s), manipulate them, and then write them back.
It's kind of a funky usage model, but it's helped us save a lot of precious time during device manufacturing.
in the case of --read'ing "all" this would produce two identical files
(i guess).
for --write you are right to the point.
i notices this problem but did not even try to mitigate it because i wanted to discuss the whole approached first so thanks for bringing it up :)
For read, if the sum of regions included covers the entire chip, then you will end up with two identical files. Included regions will also show up in the -r argument, while the rest is filled in with 0xff's.
For write, the -w argument gets ignored if a region and input file is specified with -i region:filename. The -w argument is used if the :filename portion is omitted, or if no -i argument is used. It's kind of a kludge, and the "write_it" variable needs to be set somehow which is why -w is required in any case.
In both cases, the assumption is that users to specify -i explicitly wish to supersede default behavior of -r and -w. To make this more explicit, perhaps -r and -w should no longer require an argument?
currently i always write a "complete" image for --read operations but
this might not be wanted and could be optional - if the -i option is used at least once. atm it is not necessary to give an image name because it defaults to the range's name. but this also forcibly creates files, when the -i option is used - even if the user does not need it. maybe we should let the desire for an image be defined by including the (optional) ':'?
Using the :filename syntax is currently optional, and does not forcibly create files. (Maybe there is something in your patch that is not in chromium os branch that I am missing?)
for --write we need enough data to cover the to be written range(s).
this can come from the "complete" image or the -i ones. and in case we care for the erase block layout we also need to be sure that the data is read from the chip where it is needed because it would get deleted but was not supplied/included via files. the latter is not an issue (yet) because the complete chip is (still) read in the case of writes.
Yes... I think the old infrastructure was not quite sufficient to take into account the erase block sizes and intelligently read all the data necessary to do the partial write. So we ended up just reading the entire ROM anyway.
another point which needs to be discussed is the precedence of
overlapping regions. without any -i's files given this is not an issue. we just merge the regions automatically - the data for a single address is the same anyway. when different files are given this is no longer true. i think the best option would be to abort and/or require --force in that case in addition to a defined precedence (e.g. last -i wins).
I'd prefer to simply abort if -i files are given and the regions overlap. Better to simply avoid doing something damaging in that case, IMHO.
-
Carl-Daniel Hailfinger -
David Hendricks -
Stefan Tauner