[flashrom] [PATCH 2/4] layout: Add -i <image>[:<file>] support.

Mon Sep 16 00:46:48 CEST 2013

Am 15.09.2013 20:14 schrieb Stefan Tauner:
> On Sun, 15 Sep 2013 04:15:44 +0200
> Carl-Daniel Hailfinger <c-d.hailfinger.devel.2006 at gmx.net> wrote:
>
>> Am 12.09.2013 22:40 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.
>>> Document the whole layout handling including the new features a bit better.
>>>
>>> based on chromiumos'
>>> d0ea9ed71e7f86bb8e8db2ca7c32a96de25343d8
>>> Signed-off-by: David Hendricks <dhendrix at chromium.org>
>>> Signed-off-by: Stefan Tauner <stefan.tauner at student.tuwien.ac.at>
>>>
>>> Signed-off-by: Stefan Tauner <stefan.tauner at student.tuwien.ac.at>
>> First review round, man page only. I want to get our terms right before
>> the existing confusion gets worse.
>>
>> Code review follows in a separate mail.
>>
>>> ---
>>>  flashrom.8.tmpl | 47 +++++++++++++--------------
>>>  flashrom.c      |  8 +++--
>>>  layout.c        | 98 ++++++++++++++++++++++++++++++++++++++++++++++++++-------
>>>  3 files changed, 116 insertions(+), 37 deletions(-)
>>>
>>> diff --git a/flashrom.8.tmpl b/flashrom.8.tmpl
>>> index 5ede423..b1371e4 100644
>>> --- a/flashrom.8.tmpl
>>> +++ b/flashrom.8.tmpl
>>> @@ -6,7 +6,7 @@ flashrom \- detect, read, write, verify and erase flash chips
>>>  \fB\-p\fR <programmername>[:<parameters>]
>>>                 [\fB\-E\fR|\fB\-r\fR <file>|\fB\-w\fR <file>|\fB\-v\fR <file>] \
>>>  [\fB\-c\fR <chipname>]
>>> -               [\fB\-l\fR <file> [\fB\-i\fR <image>]] [\fB\-n\fR] [\fB\-f\fR]]
>>> +               [\fB\-l\fR <file> [\fB\-i\fR <image>[:<file>]]...] [\fB\-n\fR] [\fB\-f\fR]]
>> The naming conflict between "image" (region) and "image file" (the file
>> we write to or read from the flash chip) is very unfortunate.
>> Unfortunately, we also lack good free single letters for a better name.
>> While --region (instead of --image) would be a really good choice for
>> the long option name, -r (read) and -R (revision/version) are already
>> used and -i is an admittedly difficult association for --region.
> hm. I have always read the -i as include, but in cli_classic it is
> really --image. Let's change that to -i/--include <region>:<file>?

Agreed. The time since we supported --include (with a totally different
meaning) is long enough to reuse that name.

>> The new syntax also has one catch: You can't specify a separate file for
>> a region without having that region actively written/read. While this
>> won't be a problem for ordinary users, it might pose a problem for
>> vendors like Google who want to use flashrom inside their firmware
>> update process. Such update processes (at least when I look at the
>> processes of other vendors) often do specify all files to source an
>> image from, but they desire to select the to-be-written parts of the
>> flash separately. Not sure if there is any practical relevance to this
>> observation, though.
>> David?
> My 2 cents: since we define the regions in a separate file I dont see a
> problem with this at all. What would be gained by having another
> "enable" flag or whatever?
>
>>>           [\fB\-V\fR[\fBV\fR[\fBV\fR]]] [\fB-o\fR <logfile>]
>>>  .SH DESCRIPTION
>>>  .B flashrom
>>> @@ -103,44 +103,45 @@ size for the flash bus.
>>>  * Force write even if write is known bad.
>>>  .TP
>>>  .B "\-l, \-\-layout <file>"
>>> -Read ROM layout from
>>> +Read layout entries from
>> Do we want to call them "ROM layout", "flash layout" or simply "layout"?
>> My preferences are 1. "ROM layout" 2. "flash layout".
> I dont like the word ROM in there at all. It is actually untrue in
> context of flashrom and the concept behind the whole layout thing (i.e.
> address ranges with attributes) are not specific to them at all. IMHO
> ROM layout implies some kind of chip-specific partitioning scheme.
> Please give some rationale of your preferences.

Think of "ROM" as generic name for an image file of the contents of some
flash/ROM chip. Just google for "ROMs" and you'll see what I mean.

>>>  .BR <file> .
>>>  .sp
>>> -flashrom supports ROM layouts. This allows you to flash certain parts of
>>> -the flash chip only. A ROM layout file contains multiple lines with the
>>> -following syntax:
>>> +A layout entry describes an address region of the flash chip and gives it a name. This allows to access certain
>>> +parts of the flash chip only. A layout file can contain multiple entries one per line with the following syntax:
>> I am not a native speaker, but the sentences above look odd. Suggested
>> rewording :
>>
>> flashrom supports reading/writing/erasing flash chips in whole (default)
>> or in part (when a ROM layout is given).
> That is actually not true. Giving the layout alone does not make
> flashrom access only parts. Arguably "supporting" does not necessarily
> imply that it *does* so, but I would rather not write it like that in
> the manual.

What about
"[...] (when a ROM layout file is given and certain other conditions are
met)."

>> Layouts are stored in layout
>> files. A layout entry describes an address region/range in the virtual
>> file representing the flash chip contents and assigns a name to that
>> region. A layout file consists of one or more lines with the following
>> syntax:
>>
>>
>>>  .sp
>>> -.B "  startaddr:endaddr imagename"
>>> +.B "  startaddr:endaddr regionname"
>>>  .sp
>>>  .BR "startaddr " "and " "endaddr "
>>> -are hexadecimal addresses within the ROM file and do not refer to any
>>> -physical address. Please note that using a 0x prefix for those hexadecimal
>>> +are hexadecimal addresses within the ROM file representing the flash ROM contents.
>> are hexadecimal addresses within the virtual file representing the flash
>> chip contents
> So... "virtual file representing the flash chip contents" seems to be
> the new wording for... that. We should try to come up with a shorter
> way to refer to that. (VFRTFCC hm... nope. :)

"ROM image"? "ROM file"? I do prefer "ROM image".
Suggested explanation to be added somewhere in the man page:
"For historical reasons, the contents read from or written to a flash
chip are called ROM image. Addresses within a ROM image start at 0
regardless of the address where the flash chip is mapped in hardware."

>>>  .sp
>>> -If you only want to update the image named
>>> -.BR "normal " "in a ROM based on the layout above, run"
>>> -.sp
>>> -.B "  flashrom \-p prog \-\-layout rom.layout \-\-image normal \-w some.rom"
>>> +.TP
>>> +.B "\-i, \-\-image <name>[:<file>]"
>>> +Work on the flash region
>>> +.B name
>>> +instead of the full address space if a layout file is given and parsed correctly.
>>> +Multiple such image parameters can be used to include the union of different regions.
>> s/include/work on/
> ok
>>> +The optional
>>> +.B file
>>> +parameter specifies the name of an image file that is used to map the contents of that very region only.
>> "image file" naming again.
>>
>>
>>> +This is useful for example if you want to write only a part of the flash chip and leave everything else alone
>>> +without preparing an image of the complete chip manually:
>> Uhh... sounds denglish. Suggested rewording:
>> This allows replacing part of a ROM file with contents from another file
>> before writing the resulting contents to the flash chip, eliminating the
>> separate step of manually assembling a combined ROM image.
> You seem to imply that the -i parameters do only overwrite parts of the
> file given by -w. While this is true for the current implementation,
> David and I discussed about making the file parameter after -w
> optional.

Ouch! I think that is a really really bad idea. I can see reasons to
avoid full image files, but it feels wrong to have undefined regions in
the image. Especially if writing fails and recovery is needed.

> Also, your sentence makes it sound as if the *file* is
> modified instead of the flash chip.

Yes, my bad. Let's finish the terminology discussion first.

>>> +.BR "normal " "and " "fallback " "in a ROM based on the layout mentioned above, run"
>>>  .sp
>>>  .B "  flashrom \-p prog \-l rom.layout \-i normal -i fallback \-w some.rom"
>>>  .sp
>>> -Overlapping sections are not supported.
>>> -.TP
>>> -.B "\-i, \-\-image <imagename>"
>>> -Only flash region/image
>>> -.B <imagename>
>>> -from flash layout.
>>> +.RB "Overlapping sections are resolved in an implementation-dependent manner - do " "not " "rely on it."
>> I reserve the right to have flashrom error out on overlapping regions
>> unless you can convince me such an error would be a bad idea. Suggested
>> rewording:
>> .RB "Overlapping region definitions are resolved in an
>> implementation-dependent manner or may yield an error - do " "not "
>> "rely on any observed flashrom behaviour."
> Redundant. Bailing out is a valid implementation-dependent resolution
> strategy ;) But I can add that for clarification...

"resolved" implied success for me.

> Please let us decide on the exact names for images, layouts, layout
> entries, their names etc. and their respective API parameters before
> discussing any complete sentences in the manual.

Agreed.

Regards,
Carl-Daniel

-- 
http://www.hailfinger.org/