| Message ID | cover.1610431620.git.viresh.kumar@linaro.org (mailing list archive) |
|---|---|
| Headers | show |
| Series | scripts: dtc: Build fdtoverlay | expand |
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
+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
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 >
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 >> >
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.
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.
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
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.