Re: [edk2-devel] [PATCH 0/2] Support FDT library.

["Pedro Falcato" <pedro.falcato@gmail.com>]

On Thu, Mar 30, 2023 at 11:59 PM Kinney, Michael D
<michael.d.kinney@intel.com> wrote:
>
>
>
> > -----Original Message-----
> > From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Pedro Falcato
> > Sent: Thursday, March 30, 2023 2:50 PM
> > To: devel@edk2.groups.io; Lin, Benny <benny.lin@intel.com>
> > Cc: Kinney, Michael D <michael.d.kinney@intel.com>; Gao, Liming <gaoliming@byosoft.com.cn>; Liu, Zhiguang
> > <zhiguang.liu@intel.com>; Sean Brogan <sean.brogan@microsoft.com>; Michael Kubacki <mikuback@linux.microsoft.com>
> > Subject: Re: [edk2-devel] [PATCH 0/2] Support FDT library.
> >
> > On Thu, Mar 30, 2023 at 6:13 PM Benny Lin <benny.lin@intel.com> wrote:
> > >
> > > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=4392
> > > Add FDT support in EDK2 by submodule 3rd party libfdt
> > > (https://github.com/devicetree-org/pylibfdt/tree/main/libfdt)
> > >
> > > Cc: Michael D Kinney <michael.d.kinney@intel.com>
> > > Cc: Liming Gao <gaoliming@byosoft.com.cn>
> > > Cc: Zhiguang Liu <zhiguang.liu@intel.com>
> > > Cc: Sean Brogan <sean.brogan@microsoft.com>
> > > Cc: Michael Kubacki <mikuback@linux.microsoft.com>
> > > Signed-off-by: Benny Lin <benny.lin@intel.com>
> > >
> > > Benny Lin (2):
> > >   MdePkg: Support FDT library.
> > >   .pytool: Support FDT library.
> > >
> > >  .gitmodules                               |   3 +
> > >  .pytool/CISettings.py                     |   2 +
> > >  MdePkg/Include/Library/FdtLib.h           | 300 ++++++++++++++++++++++
> > >  MdePkg/Library/BaseFdtLib/BaseFdtLib.inf  |  62 +++++
> > >  MdePkg/Library/BaseFdtLib/BaseFdtLib.uni  |  14 +
> > >  MdePkg/Library/BaseFdtLib/FdtLib.c        | 284 ++++++++++++++++++++
> > >  MdePkg/Library/BaseFdtLib/LibFdtSupport.h | 102 ++++++++
> > >  MdePkg/Library/BaseFdtLib/LibFdtWrapper.c | 138 ++++++++++
> > >  MdePkg/Library/BaseFdtLib/libfdt          |   1 +
> > >  MdePkg/Library/BaseFdtLib/limits.h        |  10 +
> > >  MdePkg/Library/BaseFdtLib/stdbool.h       |  10 +
> > >  MdePkg/Library/BaseFdtLib/stddef.h        |  10 +
> > >  MdePkg/Library/BaseFdtLib/stdint.h        |  10 +
> > >  MdePkg/Library/BaseFdtLib/stdlib.h        |  10 +
> > >  MdePkg/Library/BaseFdtLib/string.h        |  10 +
> > >  MdePkg/MdePkg.ci.yaml                     |  17 +-
> > >  MdePkg/MdePkg.dec                         |   4 +
> > >  MdePkg/MdePkg.dsc                         |   2 +
> > >  ReadMe.rst                                |   1 +
> > >  19 files changed, 988 insertions(+), 2 deletions(-)
> > >  create mode 100644 MdePkg/Include/Library/FdtLib.h
> > >  create mode 100644 MdePkg/Library/BaseFdtLib/BaseFdtLib.inf
> > >  create mode 100644 MdePkg/Library/BaseFdtLib/BaseFdtLib.uni
> > >  create mode 100644 MdePkg/Library/BaseFdtLib/FdtLib.c
> > >  create mode 100644 MdePkg/Library/BaseFdtLib/LibFdtSupport.h
> > >  create mode 100644 MdePkg/Library/BaseFdtLib/LibFdtWrapper.c
> > >  create mode 160000 MdePkg/Library/BaseFdtLib/libfdt
> > >  create mode 100644 MdePkg/Library/BaseFdtLib/limits.h
> > >  create mode 100644 MdePkg/Library/BaseFdtLib/stdbool.h
> > >  create mode 100644 MdePkg/Library/BaseFdtLib/stddef.h
> > >  create mode 100644 MdePkg/Library/BaseFdtLib/stdint.h
> > >  create mode 100644 MdePkg/Library/BaseFdtLib/stdlib.h
> > >  create mode 100644 MdePkg/Library/BaseFdtLib/string.h
> > >
> > > --
> > > 2.39.1.windows.1
> >
> > There's already a copy of libfdt plus "FdtLib" at
> > https://github.com/tianocore/edk2/tree/master/EmbeddedPkg/Library/FdtLib.
> > Please figure out what to do with it.
> > It's an old copy and has been accidentally uncrustify'd so it's
> > probably a good idea to at least ditch that specific copy for a git
> > submodule.
>
> I have discussed this overlap with Leif. After this patch series is
> added, the EmbeddedPkg maintainers can convert that package to use
> this lib and delete the duplicate sources.
Ok, SGTM.
>
> >
> > Also, NAK to Yet Another libc Implementation (and not a particularly
> > good one at that).
>
> Please provide constructive feedback on what is not good about this specific libc
> Implementation so that appropriate code updates could be made for this patch series.

Done.
> This is following the same pattern as other libs that are consuming a submodule
> that requires some amount of libc support.
>
> Getting down to one libc wrapper would be great.  Do you want to provide a proposed
> implementation?  That could be handled as a separate task.

I would like it if we could stop contributing to that problem. We very
much *know* there is a problem with both libc fragments and compiler
intrinsic fragments all over edk2.
A proper, correct C standard library is not trivial to implement
(hence the multiple problems I did find from a quick read of the
patch).
We also have a whole libc implementation in edk2-libc that seems to be
permanently gathering dust until Intel touches it for Python purposes
from time to time. So between crypto, libfdt, etc, could we try to
unify things here a bit?

Thanks,
Pedro