[PATCH V4 0/3] scripts: dtc: Build fdtoverlay

[PATCH V4 0/3] scripts: dtc: Build fdtoverlay

[Viresh Kumar]

We will start building overlays for platforms soon in the kernel and
would need fdtoverlay tool going forward. Lets start fetching and
building it.

While at it, also remove fdtdump.c file, which isn't used by the kernel.

V4:
- Don't fetch and build fdtdump.c
- Remove fdtdump.c

Viresh Kumar (3):
  scripts: dtc: Add fdtoverlay.c to DTC_SOURCE
  scripts: dtc: Build fdtoverlay tool
  scripts: dtc: Remove the unused fdtdump.c file

 scripts/dtc/Makefile             |   6 +-
 scripts/dtc/fdtdump.c            | 163 -------------------------------
 scripts/dtc/update-dtc-source.sh |   6 +-
 3 files changed, 8 insertions(+), 167 deletions(-)
 delete mode 100644 scripts/dtc/fdtdump.c

-- 
2.25.0.rc1.19.g042ed3e048af

[PATCH V4 1/3] scripts: dtc: Add fdtoverlay.c to DTC_SOURCE

[Viresh Kumar]

We will start building overlays for platforms soon in the kernel and
would need fdtoverlay tool going forward. Lets start fetching it.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 scripts/dtc/update-dtc-source.sh | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/scripts/dtc/update-dtc-source.sh b/scripts/dtc/update-dtc-source.sh
index bc704e2a6a4a..f1c802011e1e 100755
--- a/scripts/dtc/update-dtc-source.sh
+++ b/scripts/dtc/update-dtc-source.sh
@@ -31,9 +31,9 @@ set -ev
 DTC_UPSTREAM_PATH=`pwd`/../dtc
 DTC_LINUX_PATH=`pwd`/scripts/dtc
 
-DTC_SOURCE="checks.c data.c dtc.c dtc.h flattree.c fstree.c livetree.c srcpos.c \
-		srcpos.h treesource.c util.c util.h version_gen.h yamltree.c \
-		dtc-lexer.l dtc-parser.y"
+DTC_SOURCE="checks.c data.c dtc.c dtc.h fdtoverlay.c flattree.c fstree.c \
+		livetree.c srcpos.c srcpos.h treesource.c util.c \
+		util.h version_gen.h yamltree.c dtc-lexer.l dtc-parser.y"
 LIBFDT_SOURCE="fdt.c fdt.h fdt_addresses.c fdt_empty_tree.c \
 		fdt_overlay.c fdt_ro.c fdt_rw.c fdt_strerror.c fdt_sw.c \
 		fdt_wip.c libfdt.h libfdt_env.h libfdt_internal.h"
-- 
2.25.0.rc1.19.g042ed3e048af

Re: [PATCH V4 1/3] scripts: dtc: Add fdtoverlay.c to DTC_SOURCE

[Frank Rowand]

On 1/12/21 2:29 AM, Viresh Kumar wrote:
> We will start building overlays for platforms soon in the kernel and
> would need fdtoverlay tool going forward. Lets start fetching it.
> 
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> ---
>  scripts/dtc/update-dtc-source.sh | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/scripts/dtc/update-dtc-source.sh b/scripts/dtc/update-dtc-source.sh
> index bc704e2a6a4a..f1c802011e1e 100755
> --- a/scripts/dtc/update-dtc-source.sh
> +++ b/scripts/dtc/update-dtc-source.sh
> @@ -31,9 +31,9 @@ set -ev
>  DTC_UPSTREAM_PATH=`pwd`/../dtc
>  DTC_LINUX_PATH=`pwd`/scripts/dtc
>  
> -DTC_SOURCE="checks.c data.c dtc.c dtc.h flattree.c fstree.c livetree.c srcpos.c \
> -		srcpos.h treesource.c util.c util.h version_gen.h yamltree.c \
> -		dtc-lexer.l dtc-parser.y"
> +DTC_SOURCE="checks.c data.c dtc.c dtc.h fdtoverlay.c flattree.c fstree.c \
> +		livetree.c srcpos.c srcpos.h treesource.c util.c \
> +		util.h version_gen.h yamltree.c dtc-lexer.l dtc-parser.y"
>  LIBFDT_SOURCE="fdt.c fdt.h fdt_addresses.c fdt_empty_tree.c \
>  		fdt_overlay.c fdt_ro.c fdt_rw.c fdt_strerror.c fdt_sw.c \
>  		fdt_wip.c libfdt.h libfdt_env.h libfdt_internal.h"
> 

I made this comment in the v2 email thread.  Copying it here since v4 is
the current version of the patch series.

DTC_SOURCE is for the dtc program.  Please add a FDTOVERLAY_SOURCE and
related use for the fdtoverlay program.

-Frank

[PATCH V4 2/3] scripts: dtc: Build fdtoverlay tool

[Viresh Kumar]

We will start building overlays for platforms soon in the kernel and
would need fdtoverlay going forward. Lets start building it.

The fdtoverlay program applies (or merges) one ore more overlay dtb
blobs to a base dtb blob. The kernel build system would later use
fdtoverlay to generate the overlaid blobs based on platform specific
configurations.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 scripts/dtc/Makefile | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/scripts/dtc/Makefile b/scripts/dtc/Makefile
index 4852bf44e913..5f19386a49eb 100644
--- a/scripts/dtc/Makefile
+++ b/scripts/dtc/Makefile
@@ -1,13 +1,17 @@
 # SPDX-License-Identifier: GPL-2.0
 # scripts/dtc makefile
 
-hostprogs-always-$(CONFIG_DTC)		+= dtc
+hostprogs-always-$(CONFIG_DTC)		+= dtc fdtoverlay
 hostprogs-always-$(CHECK_DT_BINDING)	+= dtc
 
 dtc-objs	:= dtc.o flattree.o fstree.o data.o livetree.o treesource.o \
 		   srcpos.o checks.o util.o
 dtc-objs	+= dtc-lexer.lex.o dtc-parser.tab.o
 
+libfdt-objs	:= fdt.o fdt_ro.o fdt_wip.o fdt_sw.o fdt_rw.o fdt_strerror.o fdt_empty_tree.o fdt_addresses.o fdt_overlay.o
+libfdt		= $(addprefix libfdt/,$(libfdt-objs))
+fdtoverlay-objs	:= $(libfdt) fdtoverlay.o util.o
+
 # Source files need to get at the userspace version of libfdt_env.h to compile
 HOST_EXTRACFLAGS += -I $(srctree)/$(src)/libfdt
 
-- 
2.25.0.rc1.19.g042ed3e048af

Re: [PATCH V4 2/3] scripts: dtc: Build fdtoverlay tool

[Frank Rowand]

Hi Viresh,

I made these comments in the v2 patch series.  I am copying them here since
this is the current version.

On 1/12/21 2:29 AM, Viresh Kumar wrote:
> We will start building overlays for platforms soon in the kernel and
> would need fdtoverlay going forward. Lets start building it.
> 
> The fdtoverlay program applies (or merges) one ore more overlay dtb
> blobs to a base dtb blob. The kernel build system would later use
> fdtoverlay to generate the overlaid blobs based on platform specific
> configurations.
> 
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> ---
>  scripts/dtc/Makefile | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/scripts/dtc/Makefile b/scripts/dtc/Makefile
> index 4852bf44e913..5f19386a49eb 100644
> --- a/scripts/dtc/Makefile
> +++ b/scripts/dtc/Makefile
> @@ -1,13 +1,17 @@
>  # SPDX-License-Identifier: GPL-2.0
>  # scripts/dtc makefile
>  
> -hostprogs-always-$(CONFIG_DTC)		+= dtc
> +hostprogs-always-$(CONFIG_DTC)		+= dtc fdtoverlay
>  hostprogs-always-$(CHECK_DT_BINDING)	+= dtc
>  
>  dtc-objs	:= dtc.o flattree.o fstree.o data.o livetree.o treesource.o \
>  		   srcpos.o checks.o util.o
>  dtc-objs	+= dtc-lexer.lex.o dtc-parser.tab.o
>  

# The upstream project builds libfdt as a separate library.  We are choosing to
# instead directly link the libfdt object files into fdtoverly

> +libfdt-objs	:= fdt.o fdt_ro.o fdt_wip.o fdt_sw.o fdt_rw.o fdt_strerror.o fdt_empty_tree.o fdt_addresses.o fdt_overlay.o
> +libfdt		= $(addprefix libfdt/,$(libfdt-objs))
> +fdtoverlay-objs	:= $(libfdt) fdtoverlay.o util.o
> +
>  # Source files need to get at the userspace version of libfdt_env.h to compile
>  HOST_EXTRACFLAGS += -I $(srctree)/$(src)/libfdt
>  
> 

In general, I am a proponent of using shared libraries (which the upstream project
builds by default) because if a security bug in the library is fixed, it is fixed
for all users of the library.

In this specific case, I actually prefer the implementation that the patch provides
(directly linking the library object files into fdtoverlay, which uses the library)
because it is the only user of the library _and_ fdtoverlay will not inadvertently
use the system wide libfdt if it happens to be installed (as it is on my system).

Any thoughts on this Rob?

-Frank

Re: [PATCH V4 2/3] scripts: dtc: Build fdtoverlay tool

[Rob Herring]

On Tue, Jan 19, 2021 at 12:28 PM Frank Rowand <frowand.list@gmail.com> wrote:
>
> Hi Viresh,
>
> I made these comments in the v2 patch series.  I am copying them here since
> this is the current version.
>
> On 1/12/21 2:29 AM, Viresh Kumar wrote:
> > We will start building overlays for platforms soon in the kernel and
> > would need fdtoverlay going forward. Lets start building it.
> >
> > The fdtoverlay program applies (or merges) one ore more overlay dtb
> > blobs to a base dtb blob. The kernel build system would later use
> > fdtoverlay to generate the overlaid blobs based on platform specific
> > configurations.
> >
> > Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> > ---
> >  scripts/dtc/Makefile | 6 +++++-
> >  1 file changed, 5 insertions(+), 1 deletion(-)
> >
> > diff --git a/scripts/dtc/Makefile b/scripts/dtc/Makefile
> > index 4852bf44e913..5f19386a49eb 100644
> > --- a/scripts/dtc/Makefile
> > +++ b/scripts/dtc/Makefile
> > @@ -1,13 +1,17 @@
> >  # SPDX-License-Identifier: GPL-2.0
> >  # scripts/dtc makefile
> >
> > -hostprogs-always-$(CONFIG_DTC)               += dtc
> > +hostprogs-always-$(CONFIG_DTC)               += dtc fdtoverlay
> >  hostprogs-always-$(CHECK_DT_BINDING) += dtc
> >
> >  dtc-objs     := dtc.o flattree.o fstree.o data.o livetree.o treesource.o \
> >                  srcpos.o checks.o util.o
> >  dtc-objs     += dtc-lexer.lex.o dtc-parser.tab.o
> >
>
> # The upstream project builds libfdt as a separate library.  We are choosing to
> # instead directly link the libfdt object files into fdtoverly
>
> > +libfdt-objs  := fdt.o fdt_ro.o fdt_wip.o fdt_sw.o fdt_rw.o fdt_strerror.o fdt_empty_tree.o fdt_addresses.o fdt_overlay.o
> > +libfdt               = $(addprefix libfdt/,$(libfdt-objs))
> > +fdtoverlay-objs      := $(libfdt) fdtoverlay.o util.o
> > +
> >  # Source files need to get at the userspace version of libfdt_env.h to compile
> >  HOST_EXTRACFLAGS += -I $(srctree)/$(src)/libfdt
> >
> >
>
> In general, I am a proponent of using shared libraries (which the upstream project
> builds by default) because if a security bug in the library is fixed, it is fixed
> for all users of the library.
>
> In this specific case, I actually prefer the implementation that the patch provides
> (directly linking the library object files into fdtoverlay, which uses the library)
> because it is the only user of the library _and_ fdtoverlay will not inadvertently
> use the system wide libfdt if it happens to be installed (as it is on my system).
>
> Any thoughts on this Rob?

I agree. No point in complicating the build to do a library.

Rob

[PATCH V4 3/3] scripts: dtc: Remove the unused fdtdump.c file

[Viresh Kumar]

This was copied from external DTC repository long back and isn't used
anymore. Over that the dtc tool can be used to generate the dts source
back from the dtb. Remove the unused fdtdump.c file.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 scripts/dtc/fdtdump.c | 163 ------------------------------------------
 1 file changed, 163 deletions(-)
 delete mode 100644 scripts/dtc/fdtdump.c

diff --git a/scripts/dtc/fdtdump.c b/scripts/dtc/fdtdump.c
deleted file mode 100644
index 7d460a50b513..000000000000
--- a/scripts/dtc/fdtdump.c
+++ /dev/null
@@ -1,163 +0,0 @@
-// SPDX-License-Identifier: GPL-2.0
-/*
- * fdtdump.c - Contributed by Pantelis Antoniou <pantelis.antoniou AT gmail.com>
- */
-
-#include <stdint.h>
-#include <stdio.h>
-#include <stdlib.h>
-#include <string.h>
-#include <ctype.h>
-
-#include <fdt.h>
-#include <libfdt_env.h>
-
-#include "util.h"
-
-#define ALIGN(x, a)	(((x) + ((a) - 1)) & ~((a) - 1))
-#define PALIGN(p, a)	((void *)(ALIGN((unsigned long)(p), (a))))
-#define GET_CELL(p)	(p += 4, *((const uint32_t *)(p-4)))
-
-static void print_data(const char *data, int len)
-{
-	int i;
-	const char *p = data;
-
-	/* no data, don't print */
-	if (len == 0)
-		return;
-
-	if (util_is_printable_string(data, len)) {
-		printf(" = \"%s\"", (const char *)data);
-	} else if ((len % 4) == 0) {
-		printf(" = <");
-		for (i = 0; i < len; i += 4)
-			printf("0x%08x%s", fdt32_to_cpu(GET_CELL(p)),
-			       i < (len - 4) ? " " : "");
-		printf(">");
-	} else {
-		printf(" = [");
-		for (i = 0; i < len; i++)
-			printf("%02x%s", *p++, i < len - 1 ? " " : "");
-		printf("]");
-	}
-}
-
-static void dump_blob(void *blob)
-{
-	struct fdt_header *bph = blob;
-	uint32_t off_mem_rsvmap = fdt32_to_cpu(bph->off_mem_rsvmap);
-	uint32_t off_dt = fdt32_to_cpu(bph->off_dt_struct);
-	uint32_t off_str = fdt32_to_cpu(bph->off_dt_strings);
-	struct fdt_reserve_entry *p_rsvmap =
-		(struct fdt_reserve_entry *)((char *)blob + off_mem_rsvmap);
-	const char *p_struct = (const char *)blob + off_dt;
-	const char *p_strings = (const char *)blob + off_str;
-	uint32_t version = fdt32_to_cpu(bph->version);
-	uint32_t totalsize = fdt32_to_cpu(bph->totalsize);
-	uint32_t tag;
-	const char *p, *s, *t;
-	int depth, sz, shift;
-	int i;
-	uint64_t addr, size;
-
-	depth = 0;
-	shift = 4;
-
-	printf("/dts-v1/;\n");
-	printf("// magic:\t\t0x%x\n", fdt32_to_cpu(bph->magic));
-	printf("// totalsize:\t\t0x%x (%d)\n", totalsize, totalsize);
-	printf("// off_dt_struct:\t0x%x\n", off_dt);
-	printf("// off_dt_strings:\t0x%x\n", off_str);
-	printf("// off_mem_rsvmap:\t0x%x\n", off_mem_rsvmap);
-	printf("// version:\t\t%d\n", version);
-	printf("// last_comp_version:\t%d\n",
-	       fdt32_to_cpu(bph->last_comp_version));
-	if (version >= 2)
-		printf("// boot_cpuid_phys:\t0x%x\n",
-		       fdt32_to_cpu(bph->boot_cpuid_phys));
-
-	if (version >= 3)
-		printf("// size_dt_strings:\t0x%x\n",
-		       fdt32_to_cpu(bph->size_dt_strings));
-	if (version >= 17)
-		printf("// size_dt_struct:\t0x%x\n",
-		       fdt32_to_cpu(bph->size_dt_struct));
-	printf("\n");
-
-	for (i = 0; ; i++) {
-		addr = fdt64_to_cpu(p_rsvmap[i].address);
-		size = fdt64_to_cpu(p_rsvmap[i].size);
-		if (addr == 0 && size == 0)
-			break;
-
-		printf("/memreserve/ %llx %llx;\n",
-		       (unsigned long long)addr, (unsigned long long)size);
-	}
-
-	p = p_struct;
-	while ((tag = fdt32_to_cpu(GET_CELL(p))) != FDT_END) {
-
-		/* printf("tag: 0x%08x (%d)\n", tag, p - p_struct); */
-
-		if (tag == FDT_BEGIN_NODE) {
-			s = p;
-			p = PALIGN(p + strlen(s) + 1, 4);
-
-			if (*s == '\0')
-				s = "/";
-
-			printf("%*s%s {\n", depth * shift, "", s);
-
-			depth++;
-			continue;
-		}
-
-		if (tag == FDT_END_NODE) {
-			depth--;
-
-			printf("%*s};\n", depth * shift, "");
-			continue;
-		}
-
-		if (tag == FDT_NOP) {
-			printf("%*s// [NOP]\n", depth * shift, "");
-			continue;
-		}
-
-		if (tag != FDT_PROP) {
-			fprintf(stderr, "%*s ** Unknown tag 0x%08x\n", depth * shift, "", tag);
-			break;
-		}
-		sz = fdt32_to_cpu(GET_CELL(p));
-		s = p_strings + fdt32_to_cpu(GET_CELL(p));
-		if (version < 16 && sz >= 8)
-			p = PALIGN(p, 8);
-		t = p;
-
-		p = PALIGN(p + sz, 4);
-
-		printf("%*s%s", depth * shift, "", s);
-		print_data(t, sz);
-		printf(";\n");
-	}
-}
-
-
-int main(int argc, char *argv[])
-{
-	char *buf;
-
-	if (argc < 2) {
-		fprintf(stderr, "supply input filename\n");
-		return 5;
-	}
-
-	buf = utilfdt_read(argv[1]);
-	if (buf)
-		dump_blob(buf);
-	else
-		return 10;
-
-	return 0;
-}
-- 
2.25.0.rc1.19.g042ed3e048af

Re: [PATCH V4 0/3] scripts: dtc: Build fdtoverlay

[Frank Rowand]

On 1/12/21 2:28 AM, Viresh Kumar wrote:
> We will start building overlays for platforms soon in the kernel and
> would need fdtoverlay tool going forward. Lets start fetching and
> building it.
> 
> While at it, also remove fdtdump.c file, which isn't used by the kernel.
> 
> V4:
> - Don't fetch and build fdtdump.c
> - Remove fdtdump.c
> 
> Viresh Kumar (3):
>   scripts: dtc: Add fdtoverlay.c to DTC_SOURCE
>   scripts: dtc: Build fdtoverlay tool
>   scripts: dtc: Remove the unused fdtdump.c file
> 
>  scripts/dtc/Makefile             |   6 +-
>  scripts/dtc/fdtdump.c            | 163 -------------------------------
>  scripts/dtc/update-dtc-source.sh |   6 +-
>  3 files changed, 8 insertions(+), 167 deletions(-)
>  delete mode 100644 scripts/dtc/fdtdump.c
> 

My first inclination was to accept fdtoverlay, as is, from the upstream
project.

But my experiences debugging use of fdtoverlay against the existing
unittest overlay files has me very wary of accepting fdtoverlay in
it's current form.

As an exmple, adding an overlay that fails to reply results in the
following build messages:

   linux--5.11-rc> make zImage
   make[1]: Entering directory '/local/frowand_nobackup/src/git_linus/build/dragon_linus_5.11-rc'
     GEN     Makefile
     CALL    /local/frowand_nobackup/src/git_linus/linux--5.11-rc/scripts/checksyscalls.sh
     CALL    /local/frowand_nobackup/src/git_linus/linux--5.11-rc/scripts/atomic/check-atomics.sh
     CHK     include/generated/compile.h
     FDTOVERLAY drivers/of/unittest-data/static_test.dtb

   Failed to apply 'drivers/of/unittest-data/overlay.dtb': FDT_ERR_NOTFOUND
   make[4]: *** [/local/frowand_nobackup/src/git_linus/linux--5.11-rc/drivers/of/unittest-data/Makefile:96: drivers/of/unittest-data/static_test.dtb] Error 1
   make[3]: *** [/local/frowand_nobackup/src/git_linus/linux--5.11-rc/scripts/Makefile.build:496: drivers/of/unittest-data] Error 2
   make[2]: *** [/local/frowand_nobackup/src/git_linus/linux--5.11-rc/scripts/Makefile.build:496: drivers/of] Error 2
   make[1]: *** [/local/frowand_nobackup/src/git_linus/linux--5.11-rc/Makefile:1805: drivers] Error 2
   make[1]: Leaving directory '/local/frowand_nobackup/src/git_linus/build/dragon_linus_5.11-rc'
   make: *** [Makefile:185: __sub-make] Error 2


The specific error message (copied from above) is:

   Failed to apply 'drivers/of/unittest-data/overlay.dtb': FDT_ERR_NOTFOUND

which is cryptic and does not even point to the location in the overlay that
is problematic.  If you look at the source of fdtoverlay / libfdt, you will
find that FDT_ERR_NOTFOUND may be generated in one of many places.

I do _not_ want to do a full review of fdtoverlay, but I think that it is
reasonable to request enhancing fdtoverlay in the parent project to generate
usable error messages before enabling fdtoverlay in the Linux kernel tree.

fdtoverlay in it's current form adds a potential maintenance burden to me
(as the overlay maintainer).  I now have the experience of how difficult it
was to debug the use of fdtoverlay in the context of the proposed patch to
use it with the devicetree unittest overlay .dtb files.

-Frank

Re: [PATCH V4 0/3] scripts: dtc: Build fdtoverlay

[Viresh Kumar]

+David.

On 19-01-21, 11:12, Frank Rowand wrote:
> On 1/12/21 2:28 AM, Viresh Kumar wrote:
> > We will start building overlays for platforms soon in the kernel and
> > would need fdtoverlay tool going forward. Lets start fetching and
> > building it.
> > 
> > While at it, also remove fdtdump.c file, which isn't used by the kernel.
> > 
> > V4:
> > - Don't fetch and build fdtdump.c
> > - Remove fdtdump.c
> > 
> > Viresh Kumar (3):
> >   scripts: dtc: Add fdtoverlay.c to DTC_SOURCE
> >   scripts: dtc: Build fdtoverlay tool
> >   scripts: dtc: Remove the unused fdtdump.c file
> > 
> >  scripts/dtc/Makefile             |   6 +-
> >  scripts/dtc/fdtdump.c            | 163 -------------------------------
> >  scripts/dtc/update-dtc-source.sh |   6 +-
> >  3 files changed, 8 insertions(+), 167 deletions(-)
> >  delete mode 100644 scripts/dtc/fdtdump.c
> > 
> 
> My first inclination was to accept fdtoverlay, as is, from the upstream
> project.
> 
> But my experiences debugging use of fdtoverlay against the existing
> unittest overlay files has me very wary of accepting fdtoverlay in
> it's current form.
> 
> As an exmple, adding an overlay that fails to reply results in the
> following build messages:
> 
>    linux--5.11-rc> make zImage
>    make[1]: Entering directory '/local/frowand_nobackup/src/git_linus/build/dragon_linus_5.11-rc'
>      GEN     Makefile
>      CALL    /local/frowand_nobackup/src/git_linus/linux--5.11-rc/scripts/checksyscalls.sh
>      CALL    /local/frowand_nobackup/src/git_linus/linux--5.11-rc/scripts/atomic/check-atomics.sh
>      CHK     include/generated/compile.h
>      FDTOVERLAY drivers/of/unittest-data/static_test.dtb
> 
>    Failed to apply 'drivers/of/unittest-data/overlay.dtb': FDT_ERR_NOTFOUND
>    make[4]: *** [/local/frowand_nobackup/src/git_linus/linux--5.11-rc/drivers/of/unittest-data/Makefile:96: drivers/of/unittest-data/static_test.dtb] Error 1
>    make[3]: *** [/local/frowand_nobackup/src/git_linus/linux--5.11-rc/scripts/Makefile.build:496: drivers/of/unittest-data] Error 2
>    make[2]: *** [/local/frowand_nobackup/src/git_linus/linux--5.11-rc/scripts/Makefile.build:496: drivers/of] Error 2
>    make[1]: *** [/local/frowand_nobackup/src/git_linus/linux--5.11-rc/Makefile:1805: drivers] Error 2
>    make[1]: Leaving directory '/local/frowand_nobackup/src/git_linus/build/dragon_linus_5.11-rc'
>    make: *** [Makefile:185: __sub-make] Error 2
> 
> 
> The specific error message (copied from above) is:
> 
>    Failed to apply 'drivers/of/unittest-data/overlay.dtb': FDT_ERR_NOTFOUND
> 
> which is cryptic and does not even point to the location in the overlay that
> is problematic.  If you look at the source of fdtoverlay / libfdt, you will
> find that FDT_ERR_NOTFOUND may be generated in one of many places.
> 
> I do _not_ want to do a full review of fdtoverlay, but I think that it is
> reasonable to request enhancing fdtoverlay in the parent project to generate
> usable error messages before enabling fdtoverlay in the Linux kernel tree.
> 
> fdtoverlay in it's current form adds a potential maintenance burden to me
> (as the overlay maintainer).  I now have the experience of how difficult it
> was to debug the use of fdtoverlay in the context of the proposed patch to
> use it with the devicetree unittest overlay .dtb files.
> 
> -Frank

-- 
viresh

Re: [PATCH V4 0/3] scripts: dtc: Build fdtoverlay

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 4493 bytes --]

On Wed, Jan 20, 2021 at 10:47:40AM +0530, Viresh Kumar wrote:
> +David.
> 
> On 19-01-21, 11:12, Frank Rowand wrote:
> > On 1/12/21 2:28 AM, Viresh Kumar wrote:
> > > We will start building overlays for platforms soon in the kernel and
> > > would need fdtoverlay tool going forward. Lets start fetching and
> > > building it.
> > > 
> > > While at it, also remove fdtdump.c file, which isn't used by the kernel.
> > > 
> > > V4:
> > > - Don't fetch and build fdtdump.c
> > > - Remove fdtdump.c
> > > 
> > > Viresh Kumar (3):
> > >   scripts: dtc: Add fdtoverlay.c to DTC_SOURCE
> > >   scripts: dtc: Build fdtoverlay tool
> > >   scripts: dtc: Remove the unused fdtdump.c file
> > > 
> > >  scripts/dtc/Makefile             |   6 +-
> > >  scripts/dtc/fdtdump.c            | 163 -------------------------------
> > >  scripts/dtc/update-dtc-source.sh |   6 +-
> > >  3 files changed, 8 insertions(+), 167 deletions(-)
> > >  delete mode 100644 scripts/dtc/fdtdump.c
> > > 
> > 
> > My first inclination was to accept fdtoverlay, as is, from the upstream
> > project.
> > 
> > But my experiences debugging use of fdtoverlay against the existing
> > unittest overlay files has me very wary of accepting fdtoverlay in
> > it's current form.
> > 
> > As an exmple, adding an overlay that fails to reply results in the
> > following build messages:
> > 
> >    linux--5.11-rc> make zImage
> >    make[1]: Entering directory '/local/frowand_nobackup/src/git_linus/build/dragon_linus_5.11-rc'
> >      GEN     Makefile
> >      CALL    /local/frowand_nobackup/src/git_linus/linux--5.11-rc/scripts/checksyscalls.sh
> >      CALL    /local/frowand_nobackup/src/git_linus/linux--5.11-rc/scripts/atomic/check-atomics.sh
> >      CHK     include/generated/compile.h
> >      FDTOVERLAY drivers/of/unittest-data/static_test.dtb
> > 
> >    Failed to apply 'drivers/of/unittest-data/overlay.dtb': FDT_ERR_NOTFOUND
> >    make[4]: *** [/local/frowand_nobackup/src/git_linus/linux--5.11-rc/drivers/of/unittest-data/Makefile:96: drivers/of/unittest-data/static_test.dtb] Error 1
> >    make[3]: *** [/local/frowand_nobackup/src/git_linus/linux--5.11-rc/scripts/Makefile.build:496: drivers/of/unittest-data] Error 2
> >    make[2]: *** [/local/frowand_nobackup/src/git_linus/linux--5.11-rc/scripts/Makefile.build:496: drivers/of] Error 2
> >    make[1]: *** [/local/frowand_nobackup/src/git_linus/linux--5.11-rc/Makefile:1805: drivers] Error 2
> >    make[1]: Leaving directory '/local/frowand_nobackup/src/git_linus/build/dragon_linus_5.11-rc'
> >    make: *** [Makefile:185: __sub-make] Error 2
> > 
> > 
> > The specific error message (copied from above) is:
> > 
> >    Failed to apply 'drivers/of/unittest-data/overlay.dtb': FDT_ERR_NOTFOUND
> > 
> > which is cryptic and does not even point to the location in the overlay that
> > is problematic.  If you look at the source of fdtoverlay / libfdt, you will
> > find that FDT_ERR_NOTFOUND may be generated in one of many places.
> > 
> > I do _not_ want to do a full review of fdtoverlay, but I think that it is
> > reasonable to request enhancing fdtoverlay in the parent project to generate
> > usable error messages before enabling fdtoverlay in the Linux kernel tree.

That's... actually much harder than it sounds.  fdtoverlay is
basically a trivial wrapper around the fdt_overlay_apply() function in
libfdt.  Matching the conventions of the rest of the library, really
it's only way to report errors is a single error code.

Returning richer errors is not an easy problem in a C library,
especially one designed to be usable in embedded systems, without an
allocator or much else available.

Of course it would be possible to write a friendly command line tool
specifically for applying overlays, which could give better errors.
fdtoverlay as it stands isn't really that - it was pretty much written
just to invoke fdt_overlay_apply() in testcases.

> > fdtoverlay in it's current form adds a potential maintenance burden to me
> > (as the overlay maintainer).  I now have the experience of how difficult it
> > was to debug the use of fdtoverlay in the context of the proposed patch to
> > use it with the devicetree unittest overlay .dtb files.
> > 
> > -Frank
> 

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH V4 0/3] scripts: dtc: Build fdtoverlay

[Frank Rowand]

Hi David,

On 1/22/21 12:34 AM, David Gibson wrote:
> On Wed, Jan 20, 2021 at 10:47:40AM +0530, Viresh Kumar wrote:
>> +David.
>>
>> On 19-01-21, 11:12, Frank Rowand wrote:
>>> On 1/12/21 2:28 AM, Viresh Kumar wrote:
>>>> We will start building overlays for platforms soon in the kernel and
>>>> would need fdtoverlay tool going forward. Lets start fetching and
>>>> building it.
>>>>
>>>> While at it, also remove fdtdump.c file, which isn't used by the kernel.
>>>>
>>>> V4:
>>>> - Don't fetch and build fdtdump.c
>>>> - Remove fdtdump.c
>>>>
>>>> Viresh Kumar (3):
>>>>   scripts: dtc: Add fdtoverlay.c to DTC_SOURCE
>>>>   scripts: dtc: Build fdtoverlay tool
>>>>   scripts: dtc: Remove the unused fdtdump.c file
>>>>
>>>>  scripts/dtc/Makefile             |   6 +-
>>>>  scripts/dtc/fdtdump.c            | 163 -------------------------------
>>>>  scripts/dtc/update-dtc-source.sh |   6 +-
>>>>  3 files changed, 8 insertions(+), 167 deletions(-)
>>>>  delete mode 100644 scripts/dtc/fdtdump.c
>>>>
>>>
>>> My first inclination was to accept fdtoverlay, as is, from the upstream
>>> project.
>>>
>>> But my experiences debugging use of fdtoverlay against the existing
>>> unittest overlay files has me very wary of accepting fdtoverlay in
>>> it's current form.
>>>
>>> As an exmple, adding an overlay that fails to reply results in the
>>> following build messages:
>>>
>>>    linux--5.11-rc> make zImage
>>>    make[1]: Entering directory '/local/frowand_nobackup/src/git_linus/build/dragon_linus_5.11-rc'
>>>      GEN     Makefile
>>>      CALL    /local/frowand_nobackup/src/git_linus/linux--5.11-rc/scripts/checksyscalls.sh
>>>      CALL    /local/frowand_nobackup/src/git_linus/linux--5.11-rc/scripts/atomic/check-atomics.sh
>>>      CHK     include/generated/compile.h
>>>      FDTOVERLAY drivers/of/unittest-data/static_test.dtb
>>>
>>>    Failed to apply 'drivers/of/unittest-data/overlay.dtb': FDT_ERR_NOTFOUND
>>>    make[4]: *** [/local/frowand_nobackup/src/git_linus/linux--5.11-rc/drivers/of/unittest-data/Makefile:96: drivers/of/unittest-data/static_test.dtb] Error 1
>>>    make[3]: *** [/local/frowand_nobackup/src/git_linus/linux--5.11-rc/scripts/Makefile.build:496: drivers/of/unittest-data] Error 2
>>>    make[2]: *** [/local/frowand_nobackup/src/git_linus/linux--5.11-rc/scripts/Makefile.build:496: drivers/of] Error 2
>>>    make[1]: *** [/local/frowand_nobackup/src/git_linus/linux--5.11-rc/Makefile:1805: drivers] Error 2
>>>    make[1]: Leaving directory '/local/frowand_nobackup/src/git_linus/build/dragon_linus_5.11-rc'
>>>    make: *** [Makefile:185: __sub-make] Error 2
>>>
>>>
>>> The specific error message (copied from above) is:
>>>
>>>    Failed to apply 'drivers/of/unittest-data/overlay.dtb': FDT_ERR_NOTFOUND
>>>
>>> which is cryptic and does not even point to the location in the overlay that
>>> is problematic.  If you look at the source of fdtoverlay / libfdt, you will
>>> find that FDT_ERR_NOTFOUND may be generated in one of many places.
>>>
>>> I do _not_ want to do a full review of fdtoverlay, but I think that it is
>>> reasonable to request enhancing fdtoverlay in the parent project to generate
>>> usable error messages before enabling fdtoverlay in the Linux kernel tree.
> 

> That's... actually much harder than it sounds.  fdtoverlay is
> basically a trivial wrapper around the fdt_overlay_apply() function in
> libfdt.  Matching the conventions of the rest of the library, really
> it's only way to report errors is a single error code.
> 
> Returning richer errors is not an easy problem in a C library,
> especially one designed to be usable in embedded systems, without an
> allocator or much else available.
> 
> Of course it would be possible to write a friendly command line tool
> specifically for applying overlays, which could give better errors.
> fdtoverlay as it stands isn't really that - it was pretty much written
> just to invoke fdt_overlay_apply() in testcases.

Thank you for providing that context.

I do not know if there is a way to enable the code that is currently in libfdt
to both be useful as an embedded library (for example, U-boot seems to often
have a need to keep memory usage very small) and also be part of a tool with
effective warning and error messages.

Before having looked at libfdt only at a cursory level while debugging the proposed
use of fdtoverlay in Linux, my first thought was that maybe it would be possible
to add warning and error messages within "#ifdef" blocks, or other ways that
cause the error code to _not_ be compiled as part of library version of libfdt,
but only be compiled as part of fdtoverlay _when built in the Linux kernel_
(noting that the proposed Linux patch builds the libfdt files as part of
the fdtoverlay compile instead of as a discrete library).  After looking at
the libfdt source a tiny bit more carefully, I would probably shoot down this
suggestion, as it makes the source code uglier and harder to understand and
maintain for the primary purpose of being an embedded library.

Do you have any thoughts on how warning and error messages could be added,
or if it is even possible?  Or maybe your suggestion of writing a "friendly
command line tool specifically for applying overlays" is the path that
Viresh should pursue?

-Frank

> 
>>> fdtoverlay in it's current form adds a potential maintenance burden to me
>>> (as the overlay maintainer).  I now have the experience of how difficult it
>>> was to debug the use of fdtoverlay in the context of the proposed patch to
>>> use it with the devicetree unittest overlay .dtb files.
>>>
>>> -Frank
>>
> 

Re: [PATCH V4 0/3] scripts: dtc: Build fdtoverlay

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 6028 bytes --]

On Mon, Jan 25, 2021 at 09:42:21PM -0600, Frank Rowand wrote:
> Hi David,
> 
> On 1/22/21 12:34 AM, David Gibson wrote:
> > On Wed, Jan 20, 2021 at 10:47:40AM +0530, Viresh Kumar wrote:
> >> +David.
> >>
> >> On 19-01-21, 11:12, Frank Rowand wrote:
> >>> On 1/12/21 2:28 AM, Viresh Kumar wrote:
> >>>> We will start building overlays for platforms soon in the kernel and
> >>>> would need fdtoverlay tool going forward. Lets start fetching and
> >>>> building it.
> >>>>
> >>>> While at it, also remove fdtdump.c file, which isn't used by the kernel.
> >>>>
> >>>> V4:
> >>>> - Don't fetch and build fdtdump.c
> >>>> - Remove fdtdump.c
> >>>>
> >>>> Viresh Kumar (3):
> >>>>   scripts: dtc: Add fdtoverlay.c to DTC_SOURCE
> >>>>   scripts: dtc: Build fdtoverlay tool
> >>>>   scripts: dtc: Remove the unused fdtdump.c file
> >>>>
> >>>>  scripts/dtc/Makefile             |   6 +-
> >>>>  scripts/dtc/fdtdump.c            | 163 -------------------------------
> >>>>  scripts/dtc/update-dtc-source.sh |   6 +-
> >>>>  3 files changed, 8 insertions(+), 167 deletions(-)
> >>>>  delete mode 100644 scripts/dtc/fdtdump.c
> >>>>
> >>>
> >>> My first inclination was to accept fdtoverlay, as is, from the upstream
> >>> project.
> >>>
> >>> But my experiences debugging use of fdtoverlay against the existing
> >>> unittest overlay files has me very wary of accepting fdtoverlay in
> >>> it's current form.
> >>>
> >>> As an exmple, adding an overlay that fails to reply results in the
> >>> following build messages:
> >>>
> >>>    linux--5.11-rc> make zImage
> >>>    make[1]: Entering directory '/local/frowand_nobackup/src/git_linus/build/dragon_linus_5.11-rc'
> >>>      GEN     Makefile
> >>>      CALL    /local/frowand_nobackup/src/git_linus/linux--5.11-rc/scripts/checksyscalls.sh
> >>>      CALL    /local/frowand_nobackup/src/git_linus/linux--5.11-rc/scripts/atomic/check-atomics.sh
> >>>      CHK     include/generated/compile.h
> >>>      FDTOVERLAY drivers/of/unittest-data/static_test.dtb
> >>>
> >>>    Failed to apply 'drivers/of/unittest-data/overlay.dtb': FDT_ERR_NOTFOUND
> >>>    make[4]: *** [/local/frowand_nobackup/src/git_linus/linux--5.11-rc/drivers/of/unittest-data/Makefile:96: drivers/of/unittest-data/static_test.dtb] Error 1
> >>>    make[3]: *** [/local/frowand_nobackup/src/git_linus/linux--5.11-rc/scripts/Makefile.build:496: drivers/of/unittest-data] Error 2
> >>>    make[2]: *** [/local/frowand_nobackup/src/git_linus/linux--5.11-rc/scripts/Makefile.build:496: drivers/of] Error 2
> >>>    make[1]: *** [/local/frowand_nobackup/src/git_linus/linux--5.11-rc/Makefile:1805: drivers] Error 2
> >>>    make[1]: Leaving directory '/local/frowand_nobackup/src/git_linus/build/dragon_linus_5.11-rc'
> >>>    make: *** [Makefile:185: __sub-make] Error 2
> >>>
> >>>
> >>> The specific error message (copied from above) is:
> >>>
> >>>    Failed to apply 'drivers/of/unittest-data/overlay.dtb': FDT_ERR_NOTFOUND
> >>>
> >>> which is cryptic and does not even point to the location in the overlay that
> >>> is problematic.  If you look at the source of fdtoverlay / libfdt, you will
> >>> find that FDT_ERR_NOTFOUND may be generated in one of many places.
> >>>
> >>> I do _not_ want to do a full review of fdtoverlay, but I think that it is
> >>> reasonable to request enhancing fdtoverlay in the parent project to generate
> >>> usable error messages before enabling fdtoverlay in the Linux kernel tree.
> > 
> 
> > That's... actually much harder than it sounds.  fdtoverlay is
> > basically a trivial wrapper around the fdt_overlay_apply() function in
> > libfdt.  Matching the conventions of the rest of the library, really
> > it's only way to report errors is a single error code.
> > 
> > Returning richer errors is not an easy problem in a C library,
> > especially one designed to be usable in embedded systems, without an
> > allocator or much else available.
> > 
> > Of course it would be possible to write a friendly command line tool
> > specifically for applying overlays, which could give better errors.
> > fdtoverlay as it stands isn't really that - it was pretty much written
> > just to invoke fdt_overlay_apply() in testcases.
> 
> Thank you for providing that context.
> 
> I do not know if there is a way to enable the code that is currently in libfdt
> to both be useful as an embedded library (for example, U-boot seems to often
> have a need to keep memory usage very small) and also be part of a tool with
> effective warning and error messages.

Yeah, I don't know either.

> Before having looked at libfdt only at a cursory level while debugging the proposed
> use of fdtoverlay in Linux, my first thought was that maybe it would be possible
> to add warning and error messages within "#ifdef" blocks, or other ways that
> cause the error code to _not_ be compiled as part of library version of libfdt,
> but only be compiled as part of fdtoverlay _when built in the Linux kernel_
> (noting that the proposed Linux patch builds the libfdt files as part of
> the fdtoverlay compile instead of as a discrete library).  After looking at
> the libfdt source a tiny bit more carefully, I would probably shoot down this
> suggestion, as it makes the source code uglier and harder to understand and
> maintain for the primary purpose of being an embedded library.

Oof.  That sounds really ugly, but maybe it could be pulled off.

> Do you have any thoughts on how warning and error messages could be added,
> or if it is even possible?  Or maybe your suggestion of writing a "friendly
> command line tool specifically for applying overlays" is the path that
> Viresh should pursue?

I think at this stage it's a matter of trying a few approaches and
seeing what works out.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH V4 0/3] scripts: dtc: Build fdtoverlay

[Viresh Kumar]

On 01-02-21, 15:07, David Gibson wrote:
> On Mon, Jan 25, 2021 at 09:42:21PM -0600, Frank Rowand wrote:
> > Before having looked at libfdt only at a cursory level while debugging the proposed
> > use of fdtoverlay in Linux, my first thought was that maybe it would be possible
> > to add warning and error messages within "#ifdef" blocks, or other ways that
> > cause the error code to _not_ be compiled as part of library version of libfdt,
> > but only be compiled as part of fdtoverlay _when built in the Linux kernel_
> > (noting that the proposed Linux patch builds the libfdt files as part of
> > the fdtoverlay compile instead of as a discrete library).  After looking at
> > the libfdt source a tiny bit more carefully, I would probably shoot down this
> > suggestion, as it makes the source code uglier and harder to understand and
> > maintain for the primary purpose of being an embedded library.
> 
> Oof.  That sounds really ugly, but maybe it could be pulled off.

I started looking at this and I was able to get to a not so ugly
solution.

Do this in dtc:
-------------------------8<-------------------------
---
 dtc.h        | 6 ++++++
 fdtoverlay.c | 2 ++
 2 files changed, 8 insertions(+)

diff --git a/dtc.h b/dtc.h
index d3e82fb8e3db..cc1e591b3f8c 100644
--- a/dtc.h
+++ b/dtc.h
@@ -29,6 +29,12 @@
 #define debug(...)
 #endif
 
+#ifdef VERBOSE
+#define pr_err(...)    fprintf(stderr, __VA_ARGS__)
+#else
+#define pr_err(...)
+#endif
+
 #define DEFAULT_FDT_VERSION    17
 
 /*
diff --git a/fdtoverlay.c b/fdtoverlay.c
index 5350af65679f..28ceac0d8079 100644
--- a/fdtoverlay.c
+++ b/fdtoverlay.c
@@ -16,6 +16,7 @@
 
 #include <libfdt.h>
 
+#include "dtc.h"
 #include "util.h"
 
 #define BUF_INCREMENT  65536
@@ -76,6 +77,7 @@ static void *apply_one(char *base, const char *overlay, size_t *buf_len,
        if (ret) {
                fprintf(stderr, "\nFailed to apply '%s': %s\n",
                        name, fdt_strerror(ret));
+               pr_err("New error\n");
                goto fail;
        }
 

-------------------------8<-------------------------
And do this in kernel:
-------------------------8<-------------------------

diff --git a/scripts/dtc/Makefile b/scripts/dtc/Makefile
index c8c21e0f2531..9dafb9773f06 100644
--- a/scripts/dtc/Makefile
+++ b/scripts/dtc/Makefile
@@ -13,6 +13,7 @@ dtc-objs      += dtc-lexer.lex.o dtc-parser.tab.o
 libfdt-objs    := fdt.o fdt_ro.o fdt_wip.o fdt_sw.o fdt_rw.o fdt_strerror.o fdt_empty_tree.o fdt_addresses.o fdt_overlay.o
 libfdt         = $(addprefix libfdt/,$(libfdt-objs))
 fdtoverlay-objs        := $(libfdt) fdtoverlay.o util.o
+HOSTCFLAGS_fdtoverlay.o := -DVERBOSE
 
 # Source files need to get at the userspace version of libfdt_env.h to compile
 HOST_EXTRACFLAGS += -I $(srctree)/$(src)/libfdt

-------------------------8<-------------------------

Will that be acceptable ? With this we can add as many error messages
to libfdt without affecting any other users of it other than Linux.

-- 
viresh

Re: [PATCH V4 0/3] scripts: dtc: Build fdtoverlay

[Rob Herring]

On Sun, Jan 31, 2021 at 10:39 PM David Gibson
<david@gibson.dropbear.id.au> wrote:
>
> On Mon, Jan 25, 2021 at 09:42:21PM -0600, Frank Rowand wrote:
> > Hi David,
> >
> > On 1/22/21 12:34 AM, David Gibson wrote:
> > > On Wed, Jan 20, 2021 at 10:47:40AM +0530, Viresh Kumar wrote:
> > >> +David.
> > >>
> > >> On 19-01-21, 11:12, Frank Rowand wrote:
> > >>> On 1/12/21 2:28 AM, Viresh Kumar wrote:
> > >>>> We will start building overlays for platforms soon in the kernel and
> > >>>> would need fdtoverlay tool going forward. Lets start fetching and
> > >>>> building it.
> > >>>>
> > >>>> While at it, also remove fdtdump.c file, which isn't used by the kernel.
> > >>>>
> > >>>> V4:
> > >>>> - Don't fetch and build fdtdump.c
> > >>>> - Remove fdtdump.c
> > >>>>
> > >>>> Viresh Kumar (3):
> > >>>>   scripts: dtc: Add fdtoverlay.c to DTC_SOURCE
> > >>>>   scripts: dtc: Build fdtoverlay tool
> > >>>>   scripts: dtc: Remove the unused fdtdump.c file
> > >>>>
> > >>>>  scripts/dtc/Makefile             |   6 +-
> > >>>>  scripts/dtc/fdtdump.c            | 163 -------------------------------
> > >>>>  scripts/dtc/update-dtc-source.sh |   6 +-
> > >>>>  3 files changed, 8 insertions(+), 167 deletions(-)
> > >>>>  delete mode 100644 scripts/dtc/fdtdump.c
> > >>>>
> > >>>
> > >>> My first inclination was to accept fdtoverlay, as is, from the upstream
> > >>> project.
> > >>>
> > >>> But my experiences debugging use of fdtoverlay against the existing
> > >>> unittest overlay files has me very wary of accepting fdtoverlay in
> > >>> it's current form.
> > >>>
> > >>> As an exmple, adding an overlay that fails to reply results in the
> > >>> following build messages:
> > >>>
> > >>>    linux--5.11-rc> make zImage
> > >>>    make[1]: Entering directory '/local/frowand_nobackup/src/git_linus/build/dragon_linus_5.11-rc'
> > >>>      GEN     Makefile
> > >>>      CALL    /local/frowand_nobackup/src/git_linus/linux--5.11-rc/scripts/checksyscalls.sh
> > >>>      CALL    /local/frowand_nobackup/src/git_linus/linux--5.11-rc/scripts/atomic/check-atomics.sh
> > >>>      CHK     include/generated/compile.h
> > >>>      FDTOVERLAY drivers/of/unittest-data/static_test.dtb
> > >>>
> > >>>    Failed to apply 'drivers/of/unittest-data/overlay.dtb': FDT_ERR_NOTFOUND
> > >>>    make[4]: *** [/local/frowand_nobackup/src/git_linus/linux--5.11-rc/drivers/of/unittest-data/Makefile:96: drivers/of/unittest-data/static_test.dtb] Error 1
> > >>>    make[3]: *** [/local/frowand_nobackup/src/git_linus/linux--5.11-rc/scripts/Makefile.build:496: drivers/of/unittest-data] Error 2
> > >>>    make[2]: *** [/local/frowand_nobackup/src/git_linus/linux--5.11-rc/scripts/Makefile.build:496: drivers/of] Error 2
> > >>>    make[1]: *** [/local/frowand_nobackup/src/git_linus/linux--5.11-rc/Makefile:1805: drivers] Error 2
> > >>>    make[1]: Leaving directory '/local/frowand_nobackup/src/git_linus/build/dragon_linus_5.11-rc'
> > >>>    make: *** [Makefile:185: __sub-make] Error 2
> > >>>
> > >>>
> > >>> The specific error message (copied from above) is:
> > >>>
> > >>>    Failed to apply 'drivers/of/unittest-data/overlay.dtb': FDT_ERR_NOTFOUND
> > >>>
> > >>> which is cryptic and does not even point to the location in the overlay that
> > >>> is problematic.  If you look at the source of fdtoverlay / libfdt, you will
> > >>> find that FDT_ERR_NOTFOUND may be generated in one of many places.
> > >>>
> > >>> I do _not_ want to do a full review of fdtoverlay, but I think that it is
> > >>> reasonable to request enhancing fdtoverlay in the parent project to generate
> > >>> usable error messages before enabling fdtoverlay in the Linux kernel tree.
> > >
> >
> > > That's... actually much harder than it sounds.  fdtoverlay is
> > > basically a trivial wrapper around the fdt_overlay_apply() function in
> > > libfdt.  Matching the conventions of the rest of the library, really
> > > it's only way to report errors is a single error code.
> > >
> > > Returning richer errors is not an easy problem in a C library,
> > > especially one designed to be usable in embedded systems, without an
> > > allocator or much else available.
> > >
> > > Of course it would be possible to write a friendly command line tool
> > > specifically for applying overlays, which could give better errors.
> > > fdtoverlay as it stands isn't really that - it was pretty much written
> > > just to invoke fdt_overlay_apply() in testcases.
> >
> > Thank you for providing that context.
> >
> > I do not know if there is a way to enable the code that is currently in libfdt
> > to both be useful as an embedded library (for example, U-boot seems to often
> > have a need to keep memory usage very small) and also be part of a tool with
> > effective warning and error messages.
>
> Yeah, I don't know either.
>
> > Before having looked at libfdt only at a cursory level while debugging the proposed
> > use of fdtoverlay in Linux, my first thought was that maybe it would be possible
> > to add warning and error messages within "#ifdef" blocks, or other ways that
> > cause the error code to _not_ be compiled as part of library version of libfdt,
> > but only be compiled as part of fdtoverlay _when built in the Linux kernel_
> > (noting that the proposed Linux patch builds the libfdt files as part of
> > the fdtoverlay compile instead of as a discrete library).  After looking at
> > the libfdt source a tiny bit more carefully, I would probably shoot down this
> > suggestion, as it makes the source code uglier and harder to understand and
> > maintain for the primary purpose of being an embedded library.
>
> Oof.  That sounds really ugly, but maybe it could be pulled off.
>
> > Do you have any thoughts on how warning and error messages could be added,
> > or if it is even possible?  Or maybe your suggestion of writing a "friendly
> > command line tool specifically for applying overlays" is the path that
> > Viresh should pursue?
>
> I think at this stage it's a matter of trying a few approaches and
> seeing what works out.

Another way would be applying overlays to dtc's live tree. This could
apply overlays from dts in addition to dtb. It could be a plug-in if
we ever get that finished up.

The downside of this is not testing libfdt's code and possible
differences between 2 implementations.

Rob

Re: [PATCH V4 0/3] scripts: dtc: Build fdtoverlay

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 7790 bytes --]

On Thu, Feb 04, 2021 at 08:25:23AM -0600, Rob Herring wrote:
> On Sun, Jan 31, 2021 at 10:39 PM David Gibson
> <david@gibson.dropbear.id.au> wrote:
> >
> > On Mon, Jan 25, 2021 at 09:42:21PM -0600, Frank Rowand wrote:
> > > Hi David,
> > >
> > > On 1/22/21 12:34 AM, David Gibson wrote:
> > > > On Wed, Jan 20, 2021 at 10:47:40AM +0530, Viresh Kumar wrote:
> > > >> +David.
> > > >>
> > > >> On 19-01-21, 11:12, Frank Rowand wrote:
> > > >>> On 1/12/21 2:28 AM, Viresh Kumar wrote:
> > > >>>> We will start building overlays for platforms soon in the kernel and
> > > >>>> would need fdtoverlay tool going forward. Lets start fetching and
> > > >>>> building it.
> > > >>>>
> > > >>>> While at it, also remove fdtdump.c file, which isn't used by the kernel.
> > > >>>>
> > > >>>> V4:
> > > >>>> - Don't fetch and build fdtdump.c
> > > >>>> - Remove fdtdump.c
> > > >>>>
> > > >>>> Viresh Kumar (3):
> > > >>>>   scripts: dtc: Add fdtoverlay.c to DTC_SOURCE
> > > >>>>   scripts: dtc: Build fdtoverlay tool
> > > >>>>   scripts: dtc: Remove the unused fdtdump.c file
> > > >>>>
> > > >>>>  scripts/dtc/Makefile             |   6 +-
> > > >>>>  scripts/dtc/fdtdump.c            | 163 -------------------------------
> > > >>>>  scripts/dtc/update-dtc-source.sh |   6 +-
> > > >>>>  3 files changed, 8 insertions(+), 167 deletions(-)
> > > >>>>  delete mode 100644 scripts/dtc/fdtdump.c
> > > >>>>
> > > >>>
> > > >>> My first inclination was to accept fdtoverlay, as is, from the upstream
> > > >>> project.
> > > >>>
> > > >>> But my experiences debugging use of fdtoverlay against the existing
> > > >>> unittest overlay files has me very wary of accepting fdtoverlay in
> > > >>> it's current form.
> > > >>>
> > > >>> As an exmple, adding an overlay that fails to reply results in the
> > > >>> following build messages:
> > > >>>
> > > >>>    linux--5.11-rc> make zImage
> > > >>>    make[1]: Entering directory '/local/frowand_nobackup/src/git_linus/build/dragon_linus_5.11-rc'
> > > >>>      GEN     Makefile
> > > >>>      CALL    /local/frowand_nobackup/src/git_linus/linux--5.11-rc/scripts/checksyscalls.sh
> > > >>>      CALL    /local/frowand_nobackup/src/git_linus/linux--5.11-rc/scripts/atomic/check-atomics.sh
> > > >>>      CHK     include/generated/compile.h
> > > >>>      FDTOVERLAY drivers/of/unittest-data/static_test.dtb
> > > >>>
> > > >>>    Failed to apply 'drivers/of/unittest-data/overlay.dtb': FDT_ERR_NOTFOUND
> > > >>>    make[4]: *** [/local/frowand_nobackup/src/git_linus/linux--5.11-rc/drivers/of/unittest-data/Makefile:96: drivers/of/unittest-data/static_test.dtb] Error 1
> > > >>>    make[3]: *** [/local/frowand_nobackup/src/git_linus/linux--5.11-rc/scripts/Makefile.build:496: drivers/of/unittest-data] Error 2
> > > >>>    make[2]: *** [/local/frowand_nobackup/src/git_linus/linux--5.11-rc/scripts/Makefile.build:496: drivers/of] Error 2
> > > >>>    make[1]: *** [/local/frowand_nobackup/src/git_linus/linux--5.11-rc/Makefile:1805: drivers] Error 2
> > > >>>    make[1]: Leaving directory '/local/frowand_nobackup/src/git_linus/build/dragon_linus_5.11-rc'
> > > >>>    make: *** [Makefile:185: __sub-make] Error 2
> > > >>>
> > > >>>
> > > >>> The specific error message (copied from above) is:
> > > >>>
> > > >>>    Failed to apply 'drivers/of/unittest-data/overlay.dtb': FDT_ERR_NOTFOUND
> > > >>>
> > > >>> which is cryptic and does not even point to the location in the overlay that
> > > >>> is problematic.  If you look at the source of fdtoverlay / libfdt, you will
> > > >>> find that FDT_ERR_NOTFOUND may be generated in one of many places.
> > > >>>
> > > >>> I do _not_ want to do a full review of fdtoverlay, but I think that it is
> > > >>> reasonable to request enhancing fdtoverlay in the parent project to generate
> > > >>> usable error messages before enabling fdtoverlay in the Linux kernel tree.
> > > >
> > >
> > > > That's... actually much harder than it sounds.  fdtoverlay is
> > > > basically a trivial wrapper around the fdt_overlay_apply() function in
> > > > libfdt.  Matching the conventions of the rest of the library, really
> > > > it's only way to report errors is a single error code.
> > > >
> > > > Returning richer errors is not an easy problem in a C library,
> > > > especially one designed to be usable in embedded systems, without an
> > > > allocator or much else available.
> > > >
> > > > Of course it would be possible to write a friendly command line tool
> > > > specifically for applying overlays, which could give better errors.
> > > > fdtoverlay as it stands isn't really that - it was pretty much written
> > > > just to invoke fdt_overlay_apply() in testcases.
> > >
> > > Thank you for providing that context.
> > >
> > > I do not know if there is a way to enable the code that is currently in libfdt
> > > to both be useful as an embedded library (for example, U-boot seems to often
> > > have a need to keep memory usage very small) and also be part of a tool with
> > > effective warning and error messages.
> >
> > Yeah, I don't know either.
> >
> > > Before having looked at libfdt only at a cursory level while debugging the proposed
> > > use of fdtoverlay in Linux, my first thought was that maybe it would be possible
> > > to add warning and error messages within "#ifdef" blocks, or other ways that
> > > cause the error code to _not_ be compiled as part of library version of libfdt,
> > > but only be compiled as part of fdtoverlay _when built in the Linux kernel_
> > > (noting that the proposed Linux patch builds the libfdt files as part of
> > > the fdtoverlay compile instead of as a discrete library).  After looking at
> > > the libfdt source a tiny bit more carefully, I would probably shoot down this
> > > suggestion, as it makes the source code uglier and harder to understand and
> > > maintain for the primary purpose of being an embedded library.
> >
> > Oof.  That sounds really ugly, but maybe it could be pulled off.
> >
> > > Do you have any thoughts on how warning and error messages could be added,
> > > or if it is even possible?  Or maybe your suggestion of writing a "friendly
> > > command line tool specifically for applying overlays" is the path that
> > > Viresh should pursue?
> >
> > I think at this stage it's a matter of trying a few approaches and
> > seeing what works out.
> 
> Another way would be applying overlays to dtc's live tree. This could
> apply overlays from dts in addition to dtb. It could be a plug-in if
> we ever get that finished up.

This is actually a really interesting idea, because in a sense dtc
already *does* apply overlays.  It's just that it effectively resolves
as it is parsing, rather than realizing separate overlay objects then
merging as a separate step.

I would actually like to change that, so that it *does* explicitly
produce a chain of overlays internally.  That has advantages for the
checking code, because some checks make sense to apply to individual
overlay fragments, but some only make sense on a fully resolved tree.

As a bonus, it could handle this use case.  Unlike libfdt, dtc is a
much more normal userspace program and adding extra verbose debugging
is no realy problem.

It probably is more work in the short term, though.

> The downside of this is not testing libfdt's code and possible
> differences between 2 implementations.

That can be mitigated by having a bunch of examples in the testsuite
where we cross compare fdtoverlay's output with dtc's.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]