[PATCH] binman: Allow FIT binaries to have missing external blobs

[Simon Glass]

Hi Alper,

On Tue, 1 Sep 2020 at 06:23, Alper Nebi Yasak <alpernebiyasak at gmail.com> wrote:
>
> On 01/09/2020 13:36, Simon Glass wrote:
> > Hi Alper,
> >
> > I found that evb-rk3288 fails to build with the final patch from your
> > series. See u-boot-dm/testing for the tree.
>
> I had seen your patch and assumed that nothing was already using the
> allow-missing functionality in its FITs (since you're adding it now).
> Looks like the version in your tree (aa3f219e) succeeds in the pipeline,
> so I think there's nothing for me to do now. Thanks for fixing it.

That's right. It's a little bit subtle. In fact we rely on support for
missing entries already, but switching over to use sections for the
FIT contents means that those sections are not marked to 'allow
missing'. So in fact we fail when we cannot find the file.

I did try to do this as a separate patch, but switching to sections is
what creates the issue which is why I think I have to squash the
patch.

>
> > At present if a FIT references a missing external blob, binman reports an
> > error, even if the image is supposed to allow this.
> >
> > Propagate this setting down to the child sections of the FIT, so that the
> > behaviour is consistent.
> >
> > This is a fix-up patch to:
> >
> >    binman: Build FIT image subentries with the section etype
> >
> > and will be squashed into that.
> >
> > Signed-off-by: Simon Glass <sjg at chromium.org>
> > ---
> >
> >  tools/binman/etype/fit.py                  | 15 ++++++++
> >  tools/binman/ftest.py                      |  8 +++++
> >  tools/binman/test/168_fit_missing_blob.dts | 41 ++++++++++++++++++++++
> >  3 files changed, 64 insertions(+)
> >  create mode 100644 tools/binman/test/168_fit_missing_blob.dts
> >
> > diff --git a/tools/binman/etype/fit.py b/tools/binman/etype/fit.py
> > index acba53aa92c..615b2fd8778 100644
> > --- a/tools/binman/etype/fit.py
> > +++ b/tools/binman/etype/fit.py
> > @@ -169,3 +169,18 @@ class Entry_fit(Entry):
> >          fdt.Sync(auto_resize=True)
> >          data = fdt.GetContents()
> >          return data
> > +
> > +    def CheckMissing(self, missing_list):
> > +        """Check if any entries in this FIT have missing external blobs
> > +
> > +        If there are missing blobs, the entries are added to the list
> > +
> > +        Args:
> > +            missing_list: List of Entry objects to be added to
> > +        """
> > +        for path, section in self._fit_sections.items():
> > +            section.CheckMissing(missing_list)
> > +
> > +    def SetAllowMissing(self, allow_missing):
> > +        for section in self._fit_sections.values():
> > +            section.SetAllowMissing(allow_missing)
>
> I saw GetAllowMissing() in section.py, doesn't this also need it?

That is in Entry so we can just rely on that. The FIT itself cannot be missing.

>
> > diff --git a/tools/binman/ftest.py b/tools/binman/ftest.py
> > index 762535b5a74..e672967dbaa 100644
> > --- a/tools/binman/ftest.py
> > +++ b/tools/binman/ftest.py
> > @@ -3515,5 +3515,13 @@ class TestFunctional(unittest.TestCase):
> >                      U_BOOT_DTB_DATA)
> >          self.assertEqual(expected, data)
> >
> > +    def testFitExtblobMissingOk(self):
> > +        """Test a FIT with a missing external blob that is allowed"""
> > +        with test_util.capture_sys_output() as (stdout, stderr):
> > +            self._DoTestFile('168_fit_missing_blob.dts',
> > +                             allow_missing=True)
> > +        err = stderr.getvalue()
> > +        self.assertRegex(err, "Image 'main-section'.*missing.*: blob-ext")
> > +
> >  if __name__ == "__main__":
> >      unittest.main()
> > diff --git a/tools/binman/test/168_fit_missing_blob.dts b/tools/binman/test/168_fit_missing_blob.dts
> > new file mode 100644
> > index 00000000000..e007aa41d8a
> > --- /dev/null
> > +++ b/tools/binman/test/168_fit_missing_blob.dts
> > @@ -0,0 +1,41 @@
> > +// SPDX-License-Identifier: GPL-2.0+
> > +
> > +/dts-v1/;
> > +
> > +/ {
> > +     #address-cells = <1>;
> > +     #size-cells = <1>;
> > +
> > +     binman {
> > +             u-boot {
> > +             };
> > +             fit {
> > +                     description = "test-desc";
> > +                     #address-cells = <1>;
> > +                     fit,fdt-list = "of-list";
> > +
> > +                     images {
> > +                             kernel {
> > +                                     description = "ATF BL31";
> > +                                     type = "kernel";
> > +                                     arch = "ppc";
> > +                                     os = "linux";
> > +                                     compression = "gzip";
> > +                                     load = <00000000>;
> > +                                     entry = <00000000>;
> > +                                     hash-1 {
> > +                                             algo = "crc32";
> > +                                     };
> > +                                     hash-2 {
> > +                                             algo = "sha1";
> > +                                     };
> > +                                     blob-ext {
> > +                                             filename = "missing";
> > +                                     };
> > +                             };
> > +                     };
> > +             };
> > +             u-boot-nodtb {
> > +             };
> > +     };
> > +};
>
> Anyway, feel free to add if you need to:
>
> Reviewed-by: Alper Nebi Yasak <alpernebiyasak at gmail.com>
>
> (or Acked-by, or neither)

Thanks!

Regards,
Simon