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

From: Frank Rowand <frowand.list@gmail.com>
To: Viresh Kumar <viresh.kumar@linaro.org>,
	Pantelis Antoniou <pantelis.antoniou@konsulko.com>,
	Rob Herring <robh+dt@kernel.org>
Cc: devicetree@vger.kernel.org, linux-kernel@vger.kernel.org,
	linux-kbuild@vger.kernel.org,
	Vincent Guittot <vincent.guittot@linaro.org>,
	Bill Mills <bill.mills@linaro.org>,
	anmar.oueja@linaro.org, Masahiro Yamada <masahiroy@kernel.org>
Subject: Re: [PATCH V4 0/3] scripts: dtc: Build fdtoverlay
Date: Tue, 19 Jan 2021 11:12:11 -0600	[thread overview]
Message-ID: <74f8aa8f-ffab-3b0f-186f-31fb7395ebbb@gmail.com> (raw)
In-Reply-To: <cover.1610431620.git.viresh.kumar@linaro.org>

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