[PATCH] binman: Skip processing "hash" subnodes of FIT subsections

[Simon Glass]

Hi Alper,

On Tue, 8 Feb 2022 at 16:07, Alper Nebi Yasak <alpernebiyasak at gmail.com> wrote:
>
> Binman's FIT entry type can have image subentries with "hash" subnodes
> intended to be processed by mkimage, but not binman. However, the Entry
> class and any subclass that reuses its implementation tries to process
> these unconditionally. This can lead to an error when boards specify
> hash algorithms that binman doesn't support, but mkimage supports.
>
> Let entries skip processing these "hash" subnodes based on an instance
> variable, and set this instance variable for FIT subsections. Also
> re-enable processing of calculated and missing properties of FIT entries
> which was disabled to mitigate this issue.
>
> Signed-off-by: Alper Nebi Yasak <alpernebiyasak at gmail.com>
> ---
> This applies on top of u-boot-dm/master, and does not resend my
> "binman: Update image positions of FIT subentries" patch [1] which
> should be applied on top of this.
>
> [1] https://patchwork.ozlabs.org/project/uboot/patch/20220207220809.4497-6-alpernebiyasak@gmail.com/

Perfect, thanks

>
>  tools/binman/entry.py     | 15 +++++++++++----
>  tools/binman/etype/fit.py | 12 ++----------
>  2 files changed, 13 insertions(+), 14 deletions(-)
>
> diff --git a/tools/binman/entry.py b/tools/binman/entry.py
> index dc26f8f167b0..6f98353c8569 100644
> --- a/tools/binman/entry.py
> +++ b/tools/binman/entry.py
> @@ -78,6 +78,8 @@ class Entry(object):
>          external: True if this entry contains an external binary blob
>          bintools: Bintools used by this entry (only populated for Image)
>          missing_bintools: List of missing bintools for this entry
> +        update_hash: True if this entry's "hash" subnode should be
> +            updated with a hash of the entry contents
>      """
>      def __init__(self, section, etype, node, name_prefix=''):
>          # Put this here to allow entry-docs and help to work without libfdt
> @@ -111,6 +113,7 @@ def __init__(self, section, etype, node, name_prefix=''):
>          self.allow_fake = False
>          self.bintools = {}
>          self.missing_bintools = []
> +        self.update_hash = True
>
>      @staticmethod
>      def FindEntryClass(etype, expanded):
> @@ -315,9 +318,11 @@ def AddMissingProperties(self, have_image_pos):
>
>          if self.compress != 'none':
>              state.AddZeroProp(self._node, 'uncomp-size')
> -        err = state.CheckAddHashProp(self._node)
> -        if err:
> -            self.Raise(err)
> +
> +        if self.update_hash:
> +            err = state.CheckAddHashProp(self._node)
> +            if err:
> +                self.Raise(err)
>
>      def SetCalculatedProperties(self):
>          """Set the value of device-tree properties calculated by binman"""
> @@ -333,7 +338,9 @@ def SetCalculatedProperties(self):
>                  state.SetInt(self._node, 'orig-size', self.orig_size, True)
>          if self.uncomp_size is not None:
>              state.SetInt(self._node, 'uncomp-size', self.uncomp_size)
> -        state.CheckSetHashValue(self._node, self.GetData)
> +
> +        if self.update_hash:
> +            state.CheckSetHashValue(self._node, self.GetData)
>
>      def ProcessFdt(self, fdt):
>          """Allow entries to adjust the device tree
> diff --git a/tools/binman/etype/fit.py b/tools/binman/etype/fit.py
> index a56b0564f9a1..a979d0f1a613 100644
> --- a/tools/binman/etype/fit.py
> +++ b/tools/binman/etype/fit.py
> @@ -186,6 +186,8 @@ def _AddNode(base_node, depth, node):
>                  # 'data' property later.
>                  entry = Entry.Create(self.section, node, etype='section')
>                  entry.ReadNode()
> +                # The hash subnodes here are for mkimage, not binman.
> +                entry.update_hash = False

I know it is less Pythonic but I would like to have this be a function
in Entry, like SetUpdateHash(bool), so that it feels more like that
class has control over things.

>                  self._entries[rel_path] = entry
>

Also can you please add a test that uses a FIT containing hash{} nodes?

>              for subnode in node.subnodes:
> @@ -294,13 +296,3 @@ def _BuildInput(self, fdt):
>      def AddBintools(self, tools):
>          super().AddBintools(tools)
>          self.mkimage = self.AddBintool(tools, 'mkimage')
> -
> -    def AddMissingProperties(self, have_image_pos):
> -        # We don't want to interfere with any hash properties in the FIT, so
> -        # disable this for now.
> -        pass
> -
> -    def SetCalculatedProperties(self):
> -        # We don't want to interfere with any hash properties in the FIT, so
> -        # disable this for now.
> -        pass
> --
> 2.34.1
>

Regards,
Simon