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).