Re: [PATCH v4 1/7] binman: Allow writing section contents to a file

[quentin.schulz@theobroma-systems.com]

Hi Simon,

On 11/7/22 4:28 PM, Simon Glass <sjg@chromium.org> wrote:
> Hi Quentin,
> 
> On Mon, 7 Nov 2022 at 07:25, Quentin Schulz
> <quentin.schulz@theobroma-systems.com> wrote:
> >
> > Hi Simon,
> >
> > On 11/6/22 23:40, Simon Glass wrote:
> >> At present only the image (which is a section) has a filename. Move this
> >> implementation to the entry_Section class so that any section can have a
> >> filename. With this, the section data is written to a file.
> >>
> >> This allows parts of an image to be written, along with the entire image.
> >>
> >> Signed-off-by: Simon Glass <sjg@chromium.org>
> >> ---
> >>
> >> (no changes since v1)
> >>
> >>    tools/binman/binman.rst                 |  5 +++++
> >>    tools/binman/etype/section.py           | 12 +++++++++-
> >>    tools/binman/ftest.py                   | 14 ++++++++++++
> >>    tools/binman/image.py                   |  3 ---
> >>    tools/binman/test/261_section_fname.dts | 29 +++++++++++++++++++++++++
> >>    5 files changed, 59 insertions(+), 4 deletions(-)
> >>    create mode 100644 tools/binman/test/261_section_fname.dts
> >>
> >> diff --git a/tools/binman/binman.rst b/tools/binman/binman.rst
> >> index fda16f1992d..79578ff127b 100644
> >> --- a/tools/binman/binman.rst
> >> +++ b/tools/binman/binman.rst
> >> @@ -837,6 +837,11 @@ name-prefix:
> >>        renamed to 'ro-u-boot' and 'rw-u-boot'. This can be useful to
> >>        distinguish binaries with otherwise identical names.
> >>
> >> +filename:
> >> +    This allows the contents of the section to be written to a file in the
> >> +    output directory. This can sometimes be useful to use the data in one
> >> +    section in different image, since there is currently no way to share data
> >> +    beteen images other than through files.
> >>
> >
> > IIRC, this is currently incorrect until we have inter-image dependencies
> > since binman is building images in parallel by default. Suggesting this
> > is a possible use-case is at beast misleading. For me, this is only
> > useful for archiving embedded binaries, e.g. what we did for
> > idbloader.img for Rockchip lately (which is "needed" only to keep the
> > 3rd party tutorials/documentation not outdated).
> >
> > Maybe I missed some recent development that fixes this, lemme know if
> > that's the case or if my assumptions are wrong.
> 
> Images themselves are built one after the other, so far as I know:
> 
>              for image in images.values():
>                  invalid |= ProcessImage(image, args.update_fdt, args.map,
>                                         allow_missing=args.allow_missing,
>                                         allow_fake_blobs=args.fake_ext_blobs)
> 
> WIthin each image Binman tries to build everything in parallel if
> possible, but it is currently possible to use the outputs of one image
> in a subsequent one.
> 

That seems right indeed. ProcessImage uses OrderedDict internally. Just need to trust that the parsing of the DTB is reproducible too but I guess that's fine.

This also explains why you didn't change the u-boot-rockchip-spi.bin image generation I complained about in another review.

I'm a bit scared that implicitly relying images being sequentially created is not going to bite us back in the future once/if we do parallel building of images.

I guess this is fine then :)

Cheers,
Quentin