Re: [edk2-announce] Re: Soft Feature Freeze starts now for edk2-stable202008

["Abner Chang" <abner.chang@hpe.com>]

> -----Original Message-----
> From: Laszlo Ersek [mailto:lersek@redhat.com]
> Sent: Wednesday, August 19, 2020 9:37 PM
> To: Leif Lindholm <leif@nuviainc.com>; Chang, Abner (HPS SW/FW
> Technologist) <abner.chang@hpe.com>
> Cc: devel@edk2.groups.io; liming.gao <liming.gao@intel.com>;
> announce@edk2.groups.io; afish@apple.com; Kinney, Michael D
> <michael.d.kinney@intel.com>
> Subject: Re: [edk2-announce] Re: Soft Feature Freeze starts now for edk2-
> stable202008
> 
> On 08/19/20 15:19, Laszlo Ersek wrote:
> > On 08/19/20 13:48, Leif Lindholm wrote:
> >> (Slightly trimmed recipient list due to different patch being
> >> discussed.)
> >>
> >> So, I can't make this call, because I'm the one who messed up.
> >>
> >> This patch does exactly what I had requested Abner to do some time
> >> back (off-list, unfortunately), and I was *convinced* I gave it an
> >> R-b as soon as it hit my inbox - until Abner nudged me about it yesterday.
> >>
> >> The patch in question is
> >> https://edk2.groups.io/g/devel/topic/76021725
> >
> > My understanding is:
> >
> > (1) there is an external project that consumes the FDT library in
> > EmbeddedPkg, meaning the lib class header
> "EmbeddedPkg/Include/libfdt.h"
> > and the lib instance "EmbeddedPkg/Library/FdtLib/FdtLib.inf",
> >
> > (2) the lib class header pulls in "fdt.h" and "libfdt_env.h",
> >
> > (3) the external project is not edk2-platforms,
> >
> > (4) the external project wants -- for some strange reason -- edk2's
> > "libfdt_env.h" to provide an strncmp() function (or function-like
> > macro), with that particular stncmp() implementation not being needed
> > in either edk2-platforms or edk2 itself,
> >
> > (5) the patch for adding said strncmp() was posted on Aug 6th (at
> > least when viewed from my time zone), i.e., before the SFF,
> >
> > (6) it was reviewed 12 days later (within the SFF)
> >
> > If my understanding is correct, then I don't see how this patch could
> > be considered a bugfix -- even as a feature addition, it seems hardly
> > justified to me --, and there would have been ~8 days before the SFF
> > to review it.
> >
> > I think we should postpone the patch until after the stable tag.
> 
> Regarding OpenSBI, as of commit 9d56961b2314, the sbi_strncmp() function
> has not been removed, and it seems that sbi_strncmp() is still used / called in
> at least some build configurations (where the
> 
> lib/utils/libfdt/libfdt_env.h:#define strncmp           sbi_strncmp
> 
> definition is supposed to be in effect).
> 
> This means that the codebase can not rid itself of sbi_strncmp().
The code base doesn't have to get rid of using sbi_strncmp function (the code base defines bunch of sbi_strxxx functions) which may be used in other non-fdt related OpenSBI code and it is out of fdt library scope.

> 
> To me this indicates that OpenSBI commit 2cfd2fc90488 ("lib: utils: Use
> strncmp in fdt_parse_hart_id()", 2020-07-29) was wrong. It should have
> replaced sbi_strcmp() with sbi_strncmp(), not strncmp().
We don’t use libfdt_env.h from OpenSBI, we use libfdt_env.h from EmbeddedPkg which is impossible to define a macro to replace "sbi_strncmp" with  AsciiStrnCmp in Edk2 libfdt_env.h. We definitely can include sbi_string.h somewhere in EDK2 RISC-V header file (this is the temp solution I use now  before edk2 upstream has this fix), so it won't pop up build error. But this temp solution looks ugly and specifically. Furthermore, OpenSBI fdt helper lib is sort of a partial fdtlib code, use C API keeps the consistency with fdtlib makes sense to me.
> 
> After all, the size limit seems to be the motivation for the entire change --
> put a size limit on the string comparison in fdt_parse_hart_id(). Commit
> 2cfd2fc90488 did two things at the same
> time: replace the size-unbounded comparison with the size-bounded one,
> *and* switch to the standard C function name from the self-contained
> function. The former goal looks justifiable, the latter I don't understand
Actually the major motivation is not for the bounded comparison, that is to use C API and leverage edk2 libfdt_env.h.
> 
> Now: I realize that, going back to edk2 commit fa4a70633868
> ("EmbeddedPkg: Added support to libfdt", 2012-09-27), we already have a
> bunch of wrappers in "EmbeddedPkg/Include/libfdt_env.h". So I guess
> adding "one more" is not inconsistent with those -- even though the lib
> instance within edk2 doesn't need the new function.
> 
> But it's still a micro-feature whose review should have completed before the
> SFF.
> 
> Thanks
> Laszlo