Hi Alper,
On Tue, 8 Feb 2022 at 16:07, Alper Nebi Yasak <alpernebiya...@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 <alpernebiya...@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-alpernebiya...@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
