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

[Jonas Karlman]

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?

Regards,
Jonas

> 
>>                      return False
>> -                fnames.append(tools.get_input_filename(entry.GetDefaultFilename()))
>> +                fnames.append(entry_fname)
>>              input_fname = ":".join(fnames)
>>          else:
>>              data, input_fname, uniq = self.collect_contents_to_file(
>> @@ -165,7 +167,7 @@ class Entry_mkimage(Entry):
>>                  return False
>>          if self._imagename:
>>              image_data, imagename_fname, _ = self.collect_contents_to_file(
>> -                [self._imagename], 'mkimage-n', 1024)
>> +                [self._imagename], 'mkimage-n', fake_size)
>>              if image_data is None:
>>                  return False
>>          outfile = self._filename if self._filename else 'mkimage-out.%s' % uniq
>> @@ -216,6 +218,20 @@ class Entry_mkimage(Entry):
>>          if self._imagename:
>>              self._imagename.SetAllowFakeBlob(allow_fake)
>>
>> +    def CheckMissing(self, missing_list):
>> +        """Check if any entries in this section have missing external blobs
>> +
>> +        If there are missing (non-optional) blobs, the entries are added to the
>> +        list
>> +
>> +        Args:
>> +            missing_list: List of Entry objects to be added to
>> +        """
>> +        for entry in self._mkimage_entries.values():
>> +            entry.CheckMissing(missing_list)
>> +        if self._imagename:
>> +            self._imagename.CheckMissing(missing_list)
>> +
>>      def CheckFakedBlobs(self, faked_blobs_list):
>>          """Check if any entries in this section have faked external blobs
>>
>> @@ -229,6 +245,19 @@ class Entry_mkimage(Entry):
>>          if self._imagename:
>>              self._imagename.CheckFakedBlobs(faked_blobs_list)
>>
>> +    def CheckOptional(self, optional_list):
>> +        """Check the section for missing but optional external blobs
>> +
>> +        If there are missing (optional) blobs, the entries are added to the list
>> +
>> +        Args:
>> +            optional_list (list): List of Entry objects to be added to
>> +        """
>> +        for entry in self._mkimage_entries.values():
>> +            entry.CheckOptional(optional_list)
>> +        if self._imagename:
>> +            self._imagename.CheckOptional(optional_list)
>> +
>>      def AddBintools(self, btools):
>>          super().AddBintools(btools)
>>          self.mkimage = self.AddBintool(btools, 'mkimage')
>> --
>> 2.39.1
>>
> 
> Regards,
> Simon