Re: [PATCH v2 6/6] RFC: binman: Improve allow missing for mkimage entry

[Simon Glass <sjg@chromium.org>]

Hi Jonas,

On Wed, 15 Feb 2023 at 11:25, Jonas Karlman <jonas@kwiboo.se> wrote:
>
> Hi Simon,
>
> On 2023-02-14 20:48, Simon Glass wrote:
> > Hi Jonas,
> >
> > On Tue, 14 Feb 2023 at 03:34, Jonas Karlman <jonas@kwiboo.se> wrote:
> >>
> >> Implement CheckMissing and CheckOptional methods that is adapted to
> >> Entry_mkimage in order to improve support for allow missing flag.
> >>
> >> Use collect_contents_to_file in multiple-data-files handling to improve
> >> support for allow missing and fake blobs handling.
> >>
> >> Signed-off-by: Jonas Karlman <jonas@kwiboo.se>
> >> ---
> >> Building for RK3568 without ROCKCHIP_TPL will result in the following
> >> error message, regardless if BINMAN_ALLOW_MISSING is used or not.
> >>
> >>   binman: Filename 'rockchip-tpl' not found in input path (...)
> >>
> >> With this patch and using BINMAN_ALLOW_MISSING=1 a new external blob
> >> with no content is created and then passed to mkimage, resulting in an
> >> error from the mkimage command.
> >>
> >>   binman: Error 255 running 'mkimage -d ./mkimage-0.simple-bin.mkimage:./mkimage-1.simple-bin.mkimage -n rk3568 -T rksd ./idbloader.img': mkimage: Can't read ./mkimage-0.simple-bin.mkimage: Invalid argument
> >>
> >> If the --fake-ext-blobs argument is also used I get the message I was
> >> expecting to see from the beginning.
> >>
> >>   Image 'main-section' is missing external blobs and is non-functional: rockchip-tpl
> >>
> >>   /binman/simple-bin/mkimage/rockchip-tpl:
> >>      An external TPL is required to initialize DRAM. Get the external TPL
> >>      binary and build with ROCKCHIP_TPL=/path/to/ddr.bin. One possible source
> >>      for the external TPL binary is https://github.com/rockchip-linux/rkbin>>>   Image 'main-section' has faked external blobs and is non-functional: rockchip-tpl
> >>
> >>   Some images are invalid
> >>
> >> Not sure how this should work, but I was expecting to see the message
> >> about the missing rockchip-tpl from the beginning instead of the generic
> >> "not found in input path" message. At leas with BINMAN_ALLOW_MISSING=1.
> >>
> >>  tools/binman/etype/mkimage.py | 37 +++++++++++++++++++++++++++++++----
> >>  1 file changed, 33 insertions(+), 4 deletions(-)
> >>
> >> diff --git a/tools/binman/etype/mkimage.py b/tools/binman/etype/mkimage.py
> >> index cb264c3cad0b..44510a8c40ba 100644
> >> --- a/tools/binman/etype/mkimage.py
> >> +++ b/tools/binman/etype/mkimage.py
> >> @@ -153,10 +153,12 @@ class Entry_mkimage(Entry):
> >>          if self._multiple_data_files:
> >>              fnames = []
> >>              uniq = self.GetUniqueName()
> >> -            for entry in self._mkimage_entries.values():
> >> -                if not entry.ObtainContents(fake_size=fake_size):
> >> +            for idx, entry in enumerate(self._mkimage_entries.values()):
> >> +                entry_data, entry_fname, _ = self.collect_contents_to_file(
> >> +                    [entry], 'mkimage-%s' % idx, fake_size)
> >> +                if entry_data is None:
> >
> > This is OK, I suppose, but I'm not quite sure what actually changes
> > here, other than writing each entry to a file?
>
> The collect_contents_to_file function seemed to handle the
> external/missing/optional/faked entry flags. So I changed to use that
> function in order to see if that would change anything, see below.
>
> Use of this function does make it possible to use entry type other
> then external blobs, not sure if that would ever be needed/useful.
>
> >
> > Also, if you do this, please add / extend a test that checks that the
> > output files are written, since there is otherwise no coverage here.
> >
> > What test uses these optional mkimage pieces?
>
> Sorry, I was not clear enough about the reason for these changes in my
> message above.
>
> Building with --rockchip-tpl-path=/path/to/existing/tpl works as
> expected and generates a working image.
>
> I was expecting that the missing-blob-help message added in patch 1
> would be shown by binman when rockchip-tpl-path was empty/not-existing,
> or at least together with the allow-missing flag.
>
> However, whatever I tested, empty/none-existing rockchip-tpl-path,
> allow-missing, fake-ext-blobs, I was not able to see this message.
> Instead, all I could get from binman was this "Filename 'rockchip-tpl'
> not found in input path" message.
>
> Maybe my assumptions about when these missing messages should be shown
> is wrong?
>
> Trying binman with the following two dts and --allow-missing gives
> different results. First one shows the missing message, second one show
> filename not found.
>
> binman {
>         rockchip-tpl {
>         };
> };
>
> binman {
>         mkimage {
>                 args = "-n rk3568 -T rksd";
>                 multiple-data-files;
>
>                 rockchip-tpl {
>                 };
>         };
> };
>
> With the changes in this patch I instead get the missing message when I
> also add the --fake-ext-blobs flag, so it clearly needs more work or
> a completely different approach if we want to be able to see the missing
> message added in patch 1.
>
> Adding a message that never will be displayed annoys me :-)
> Maybe I should just drop this rfc/patch for a v3 of this series?
>

Does the mkimage etype need a CheckMissing() function? That is how
binman knows whether something is missing.

Regards,
Simon
[..]