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

[Jonas Karlman]

Hi Simon,
On 2023-02-17 03:55, Simon Glass wrote:
> Hi Jonas,
> 
> On Wed, 15 Feb 2023 at 11:25, Jonas Karlman <jonas at 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 at 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 at 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.

I think I have found something that can work, will send a v3 of this series
without this rfc/path and instead send a separate series together with a
follow-up patch mentioned at [1].

[1] https://patchwork.ozlabs.org/project/uboot/patch/20230121190136.456341-6-jonas@kwiboo.se/#3044726

Regards,
Jonas

> 
> Regards,
> Simon
> [..]