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

[Frank Rowand <frowand.list@gmail.com>]

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
>>
>