All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] binman: Skip node generation for images read from files
@ 2022-01-28 19:37 Jan Kiszka
  2022-01-30 23:19 ` Simon Glass
  0 siblings, 1 reply; 4+ messages in thread
From: Jan Kiszka @ 2022-01-28 19:37 UTC (permalink / raw)
  To: Simon Glass, U-Boot Mailing List

From: Jan Kiszka <jan.kiszka@siemens.com>

We can and should run the node generator only when creating a new image.
When we read it back, there is no need to generate nodes - they already
exits, and binman does not dive that deep into the image - and there is
no way to provide the required fdt-list. So store the mode in the image
object so that Entry_fit can simply skip generator nodes when reading
them from an fdtmap.

This unbreaks all read-backs of images that contain generator nodes in
their fdtmap. To confirm this, add a corresponding test case.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
  tools/binman/etype/fit.py             |  2 +-
  tools/binman/ftest.py                 | 18 ++++++++++++++++++
  tools/binman/image.py                 |  9 +++++++--
  tools/binman/test/219_fit_gennode.dts | 24 ++++++++++++++++++++++++
  4 files changed, 50 insertions(+), 3 deletions(-)
  create mode 100644 tools/binman/test/219_fit_gennode.dts

diff --git a/tools/binman/etype/fit.py b/tools/binman/etype/fit.py
index 6e5f020c502..6ad4a686df5 100644
--- a/tools/binman/etype/fit.py
+++ b/tools/binman/etype/fit.py
@@ -194,7 +194,7 @@ class Entry_fit(Entry):
                      # the FIT (e.g. "/images/kernel/u-boot"), so don't call
                      # fsw.add_node() or _AddNode() for it.
                      pass
-                elif subnode.name.startswith('@'):
+                elif self.GetImage().generate and subnode.name.startswith('@'):
                      if self._fdts:
                          # Generate notes for each FDT
                          for seq, fdt_fname in enumerate(self._fdts):
diff --git a/tools/binman/ftest.py b/tools/binman/ftest.py
index ca200ae9f8f..5400f76c676 100644
--- a/tools/binman/ftest.py
+++ b/tools/binman/ftest.py
@@ -5100,6 +5100,24 @@ fdt         fdtmap                Extract the devicetree blob from the fdtmap
          self.assertIn('Documentation is missing for modules: mkimage',
                        str(e.exception))
  
+    def testListWithGenNode(self):
+        """Check handling of an FDT map when the section cannot be found"""
+        entry_args = {
+            'of-list': 'test-fdt1 test-fdt2',
+        }
+        data = self._DoReadFileDtb(
+            '219_fit_gennode.dts',
+            entry_args=entry_args,
+            use_real_dtb=True,
+            extra_indirs=[os.path.join(self._indir, TEST_FDT_SUBDIR)])
+
+        try:
+            tmpdir, updated_fname = self._SetupImageInTmpdir()
+            with test_util.capture_sys_output() as (stdout, stderr):
+                self._RunBinman('ls', '-i', updated_fname)
+        finally:
+            shutil.rmtree(tmpdir)
+
  
  if __name__ == "__main__":
      unittest.main()
diff --git a/tools/binman/image.py b/tools/binman/image.py
index 0f0c1d29e80..cb5279c7ead 100644
--- a/tools/binman/image.py
+++ b/tools/binman/image.py
@@ -67,9 +67,13 @@ class Image(section.Entry_section):
              does not exist in binman. This is useful if an image was created by
              binman a newer version of binman but we want to list it in an older
              version which does not support all the entry types.
+        generate: If true, generator nodes are processed. If false they are
+            ignored which is useful when an existing image is read back from a
+            file.
      """
      def __init__(self, name, node, copy_to_orig=True, test=False,
-                 ignore_missing=False, use_expanded=False, missing_etype=False):
+                 ignore_missing=False, use_expanded=False, missing_etype=False,
+                 generate=True):
          super().__init__(None, 'section', node, test=test)
          self.copy_to_orig = copy_to_orig
          self.name = 'main-section'
@@ -83,6 +87,7 @@ class Image(section.Entry_section):
          self.use_expanded = use_expanded
          self.test_section_timeout = False
          self.bintools = {}
+        self.generate = generate
          if not test:
              self.ReadNode()
  
@@ -131,7 +136,7 @@ class Image(section.Entry_section):
          # Return an Image with the associated nodes
          root = dtb.GetRoot()
          image = Image('image', root, copy_to_orig=False, ignore_missing=True,
-                      missing_etype=True)
+                      missing_etype=True, generate=False)
  
          image.image_node = fdt_util.GetString(root, 'image-node', 'image')
          image.fdtmap_dtb = dtb
diff --git a/tools/binman/test/219_fit_gennode.dts b/tools/binman/test/219_fit_gennode.dts
new file mode 100644
index 00000000000..7820c472808
--- /dev/null
+++ b/tools/binman/test/219_fit_gennode.dts
@@ -0,0 +1,24 @@
+/dts-v1/;
+
+/ {
+	#address-cells = <1>;
+	#size-cells = <1>;
+
+	binman {
+		fit {
+			description = "test-desc";
+			#address-cells = <1>;
+			fit,fdt-list = "of-list";
+
+			images {
+				@fdt-SEQ {
+					description = "fdt-NAME.dtb";
+					type = "flat_dt";
+					compression = "none";
+				};
+			};
+		};
+		fdtmap {
+		};
+	};
+};
-- 
2.31.1

^ permalink raw reply related	[flat|nested] 4+ messages in thread

* Re: [PATCH] binman: Skip node generation for images read from files
  2022-01-28 19:37 [PATCH] binman: Skip node generation for images read from files Jan Kiszka
@ 2022-01-30 23:19 ` Simon Glass
  0 siblings, 0 replies; 4+ messages in thread
From: Simon Glass @ 2022-01-30 23:19 UTC (permalink / raw)
  To: Jan Kiszka; +Cc: U-Boot Mailing List

Hi Jan,

On Fri, 28 Jan 2022 at 12:37, Jan Kiszka <jan.kiszka@siemens.com> wrote:
>
> From: Jan Kiszka <jan.kiszka@siemens.com>
>
> We can and should run the node generator only when creating a new image.
> When we read it back, there is no need to generate nodes - they already
> exits, and binman does not dive that deep into the image - and there is
> no way to provide the required fdt-list. So store the mode in the image
> object so that Entry_fit can simply skip generator nodes when reading
> them from an fdtmap.
>
> This unbreaks all read-backs of images that contain generator nodes in
> their fdtmap. To confirm this, add a corresponding test case.
>
> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> ---
>   tools/binman/etype/fit.py             |  2 +-
>   tools/binman/ftest.py                 | 18 ++++++++++++++++++
>   tools/binman/image.py                 |  9 +++++++--
>   tools/binman/test/219_fit_gennode.dts | 24 ++++++++++++++++++++++++
>   4 files changed, 50 insertions(+), 3 deletions(-)
>   create mode 100644 tools/binman/test/219_fit_gennode.dts

(version 3 patch)

Reviewed-by: Simon Glass <sjg@chromium.org>
Tested-by: Simon Glass <sjg@chromium.org>

This has a lot of merge conflicts but they are my fault, so I am going
to tidy them up. PLMK if something looks wrong.

Regards,
Simon

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH] binman: Skip node generation for images read from files
  2022-01-16 15:51 Jan Kiszka
@ 2022-01-17  3:12 ` Simon Glass
  0 siblings, 0 replies; 4+ messages in thread
From: Simon Glass @ 2022-01-17  3:12 UTC (permalink / raw)
  To: Jan Kiszka; +Cc: U-Boot Mailing List, Su, Bao Cheng (RC-CN DF FA R&D)

Hi Jan,

On Sun, 16 Jan 2022 at 08:51, Jan Kiszka <jan.kiszka@siemens.com> wrote:
>
> From: Jan Kiszka <jan.kiszka@siemens.com>
>
> This unbreaks all read-backs of images that contain generator nodes in
> their fdtmap.

This issue is subtle enough that I think it could use a few lines of
explanation.

>
> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> ---
>
> I tried to write some test case as well, but the testsuite is too
> fragile and too non-intuitive to me to extend it. E.g., just adding a
> fdtmap to 170_fit_fdt.dts made existing tests fail, for unclear reasons.
> I have to leave that to someone who understands the mechanics better.

That's because a fake FDT is used in most tests, to save time and
reduce complexity. In your case you need a real one so that you don't
just get fake junk in the fdtmap. The -9 FDT_ERR_BADMAGIC is produced
because the fdt is not really an fdt, but is U_BOOT_DTB_DATA (i.e.
'udtb').

You can call _DoReadFileRealDtb() to make things work - see testFdpmap().

But note that you should not reuse an existing dts for new tests.
Create a new one with the things you want in it and use that in your
new test.

>
>   tools/binman/entry.py         | 5 ++++-
>   tools/binman/etype/fit.py     | 2 +-
>   tools/binman/etype/section.py | 4 ++--
>   tools/binman/image.py         | 7 ++++---
>   4 files changed, 11 insertions(+), 7 deletions(-)
>
> diff --git a/tools/binman/entry.py b/tools/binman/entry.py
> index bac90bbbcd..fdb9746fda 100644
> --- a/tools/binman/entry.py
> +++ b/tools/binman/entry.py
> @@ -75,7 +75,7 @@ class Entry(object):
>               available. This is mainly used for testing.
>           external: True if this entry contains an external binary blob
>       """
> -    def __init__(self, section, etype, node, name_prefix=''):
> +    def __init__(self, section, etype, node, name_prefix='', generate=None):
>           # Put this here to allow entry-docs and help to work without libfdt
>           global state
>           from binman import state
> @@ -105,6 +105,9 @@ class Entry(object):
>           self.external = False
>           self.allow_missing = False
>           self.allow_fake = False
> +        if generate == None:

is None

> +            generate = section.generate if section else True
> +        self.generate = generate

But I think it would be simpler to have a flag in the Image (as you
have) and not copy it elsewhere.

>
>       @staticmethod
>       def FindEntryClass(etype, expanded):
> diff --git a/tools/binman/etype/fit.py b/tools/binman/etype/fit.py
> index b41187df80..4e4d2f9c22 100644
> --- a/tools/binman/etype/fit.py
> +++ b/tools/binman/etype/fit.py
> @@ -193,7 +193,7 @@ class Entry_fit(Entry):
>                       # the FIT (e.g. "/images/kernel/u-boot"), so don't call
>                       # fsw.add_node() or _AddNode() for it.
>                       pass
> -                elif subnode.name.startswith('@'):
> +                elif self.generate and subnode.name.startswith('@'):

You can call self.GetImage().generate here so you don't need to copy
the 'generate' flag.

>                       if self._fdts:
>                           # Generate notes for each FDT
>                           for seq, fdt_fname in enumerate(self._fdts):
> diff --git a/tools/binman/etype/section.py b/tools/binman/etype/section.py
> index 7a55d03231..319156a09a 100644
> --- a/tools/binman/etype/section.py
> +++ b/tools/binman/etype/section.py
> @@ -154,9 +154,9 @@ class Entry_section(Entry):
>       available. This is set by the `SetAllowMissing()` method, if
>       `--allow-missing` is passed to binman.
>       """
> -    def __init__(self, section, etype, node, test=False):
> +    def __init__(self, section, etype, node, test=False, generate=None):
>           if not test:
> -            super().__init__(section, etype, node)
> +            super().__init__(section, etype, node, generate=generate)
>           self._entries = OrderedDict()
>           self._pad_byte = 0
>           self._sort = False
> diff --git a/tools/binman/image.py b/tools/binman/image.py
> index f0a7d65299..1ff97e687c 100644
> --- a/tools/binman/image.py
> +++ b/tools/binman/image.py
> @@ -69,8 +69,9 @@ class Image(section.Entry_section):
>               version which does not support all the entry types.
>       """
>       def __init__(self, name, node, copy_to_orig=True, test=False,
> -                 ignore_missing=False, use_expanded=False, missing_etype=False):
> -        super().__init__(None, 'section', node, test=test)
> +                 ignore_missing=False, use_expanded=False, missing_etype=False,
> +                 generate=True):

Please remember to document 'generate' in the comments for class Image.

> +        super().__init__(None, 'section', node, test=test, generate=generate)
>           self.copy_to_orig = copy_to_orig
>           self.name = 'main-section'
>           self.image_name = name
> @@ -130,7 +131,7 @@ class Image(section.Entry_section):
>           # Return an Image with the associated nodes
>           root = dtb.GetRoot()
>           image = Image('image', root, copy_to_orig=False, ignore_missing=True,
> -                      missing_etype=True)
> +                      missing_etype=True, generate=False)
>
>           image.image_node = fdt_util.GetString(root, 'image-node', 'image')
>           image.fdtmap_dtb = dtb
> --
> 2.31.1

Regards,
Simon

^ permalink raw reply	[flat|nested] 4+ messages in thread

* [PATCH] binman: Skip node generation for images read from files
@ 2022-01-16 15:51 Jan Kiszka
  2022-01-17  3:12 ` Simon Glass
  0 siblings, 1 reply; 4+ messages in thread
From: Jan Kiszka @ 2022-01-16 15:51 UTC (permalink / raw)
  To: Simon Glass, U-Boot Mailing List; +Cc: Su, Bao Cheng (RC-CN DF FA R&D)

From: Jan Kiszka <jan.kiszka@siemens.com>

This unbreaks all read-backs of images that contain generator nodes in
their fdtmap.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---

I tried to write some test case as well, but the testsuite is too
fragile and too non-intuitive to me to extend it. E.g., just adding a
fdtmap to 170_fit_fdt.dts made existing tests fail, for unclear reasons.
I have to leave that to someone who understands the mechanics better.

  tools/binman/entry.py         | 5 ++++-
  tools/binman/etype/fit.py     | 2 +-
  tools/binman/etype/section.py | 4 ++--
  tools/binman/image.py         | 7 ++++---
  4 files changed, 11 insertions(+), 7 deletions(-)

diff --git a/tools/binman/entry.py b/tools/binman/entry.py
index bac90bbbcd..fdb9746fda 100644
--- a/tools/binman/entry.py
+++ b/tools/binman/entry.py
@@ -75,7 +75,7 @@ class Entry(object):
              available. This is mainly used for testing.
          external: True if this entry contains an external binary blob
      """
-    def __init__(self, section, etype, node, name_prefix=''):
+    def __init__(self, section, etype, node, name_prefix='', generate=None):
          # Put this here to allow entry-docs and help to work without libfdt
          global state
          from binman import state
@@ -105,6 +105,9 @@ class Entry(object):
          self.external = False
          self.allow_missing = False
          self.allow_fake = False
+        if generate == None:
+            generate = section.generate if section else True
+        self.generate = generate
  
      @staticmethod
      def FindEntryClass(etype, expanded):
diff --git a/tools/binman/etype/fit.py b/tools/binman/etype/fit.py
index b41187df80..4e4d2f9c22 100644
--- a/tools/binman/etype/fit.py
+++ b/tools/binman/etype/fit.py
@@ -193,7 +193,7 @@ class Entry_fit(Entry):
                      # the FIT (e.g. "/images/kernel/u-boot"), so don't call
                      # fsw.add_node() or _AddNode() for it.
                      pass
-                elif subnode.name.startswith('@'):
+                elif self.generate and subnode.name.startswith('@'):
                      if self._fdts:
                          # Generate notes for each FDT
                          for seq, fdt_fname in enumerate(self._fdts):
diff --git a/tools/binman/etype/section.py b/tools/binman/etype/section.py
index 7a55d03231..319156a09a 100644
--- a/tools/binman/etype/section.py
+++ b/tools/binman/etype/section.py
@@ -154,9 +154,9 @@ class Entry_section(Entry):
      available. This is set by the `SetAllowMissing()` method, if
      `--allow-missing` is passed to binman.
      """
-    def __init__(self, section, etype, node, test=False):
+    def __init__(self, section, etype, node, test=False, generate=None):
          if not test:
-            super().__init__(section, etype, node)
+            super().__init__(section, etype, node, generate=generate)
          self._entries = OrderedDict()
          self._pad_byte = 0
          self._sort = False
diff --git a/tools/binman/image.py b/tools/binman/image.py
index f0a7d65299..1ff97e687c 100644
--- a/tools/binman/image.py
+++ b/tools/binman/image.py
@@ -69,8 +69,9 @@ class Image(section.Entry_section):
              version which does not support all the entry types.
      """
      def __init__(self, name, node, copy_to_orig=True, test=False,
-                 ignore_missing=False, use_expanded=False, missing_etype=False):
-        super().__init__(None, 'section', node, test=test)
+                 ignore_missing=False, use_expanded=False, missing_etype=False,
+                 generate=True):
+        super().__init__(None, 'section', node, test=test, generate=generate)
          self.copy_to_orig = copy_to_orig
          self.name = 'main-section'
          self.image_name = name
@@ -130,7 +131,7 @@ class Image(section.Entry_section):
          # Return an Image with the associated nodes
          root = dtb.GetRoot()
          image = Image('image', root, copy_to_orig=False, ignore_missing=True,
-                      missing_etype=True)
+                      missing_etype=True, generate=False)
  
          image.image_node = fdt_util.GetString(root, 'image-node', 'image')
          image.fdtmap_dtb = dtb
-- 
2.31.1

^ permalink raw reply related	[flat|nested] 4+ messages in thread

end of thread, other threads:[~2022-01-30 23:19 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-01-28 19:37 [PATCH] binman: Skip node generation for images read from files Jan Kiszka
2022-01-30 23:19 ` Simon Glass
  -- strict thread matches above, loose matches on Subject: below --
2022-01-16 15:51 Jan Kiszka
2022-01-17  3:12 ` Simon Glass

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.