[PATCH 5/9] fdt: Correct condition for bloblist existing

[Simon Glass]

Hi Raymond,

On Fri, 21 Jun 2024 at 10:40, Raymond Mao <raymond.mao at linaro.org> wrote:
>
> Hi Simon,
>
> On Fri, 21 Jun 2024 at 10:58, Simon Glass <sjg at chromium.org> wrote:
>>
>> Hi Ilias,
>>
>> On Wed, 19 Jun 2024 at 06:41, Ilias Apalodimas
>> <ilias.apalodimas at linaro.org> wrote:
>> >
>> > Hi Simon,
>> >
>> > On Wed, Jun 12, 2024 at 02:24:31PM -0600, Simon Glass wrote:
>> > > Hi Ilias,
>> > >
>> > > On Wed, 12 Jun 2024 at 00:02, Ilias Apalodimas
>> > > <ilias.apalodimas at linaro.org> wrote:
>> > > >
>> > > > [...]
>> > > >
>> > > > > > > >> ---
>> > > > > > > >>
>> > > > > > > >>  lib/fdtdec.c | 12 ++++++++++--
>> > > > > > > >>  1 file changed, 10 insertions(+), 2 deletions(-)
>> > > > > > > >>
>> > > > > > > >> diff --git a/lib/fdtdec.c b/lib/fdtdec.c
>> > > > > > > >> index b2c59ab3818..b141244e3b9 100644
>> > > > > > > >> --- a/lib/fdtdec.c
>> > > > > > > >> +++ b/lib/fdtdec.c
>> > > > > > > >> @@ -1669,8 +1669,16 @@ int fdtdec_setup(void)
>> > > > > > > >>  {
>> > > > > > > >>         int ret = -ENOENT;
>> > > > > > > >>
>> > > > > > > >> -       /* If allowing a bloblist, check that first */
>> > > > > > > >> -       if (CONFIG_IS_ENABLED(BLOBLIST)) {
>> > > > > > > >> +       /*
>> > > > > > > >> +        * If allowing a bloblist, check that first. This would be better
>> > > > > > > >> +        * handled with an OF_BLOBLIST Kconfig, but that caused far too much
>> > > > > > > >> +        * argument, so add a hack here, used e.g. by chromebook_coral
>> > > > > > > >
>> > > > > > > > I am a bit confused by this comment - It means you will not use OF_BLOBLIST,
>> > > > > > > > but actually you are using it below. Is it a typo?
>> > > > > > >
>> > > > > > > Basically it would be cleaner to have a separate, phase-specific
>> > > > > > > Kconfig control as to whether the DT can come from the bloblist (I
>> > > > > > > can't remember the Kconfig name I suggested, nor the patch as it was
>> > > > > > > last year sometime). But for now I am adding this hack to get a few
>> > > > > > > boards working again.
>> > > > > >
>> > > > > > I am a bit confused.
>> > > > > > First of all the comment is innapropriate. We went through a lengthy
>> > > > > > discussion on BLOBLIST/OF_BLOSLIST etc and,  even Tom chimed in and we
>> > > > > > made up our minds. Why are you adding this comment now? Why do code
>> > > > > > comments have to illustrate your personal opinion -- which was
>> > > > > > rejected?
>> > > > >
>> > > > > I'm sorry for the tone of the comment. I am not trying to offend
>> > > > > anyone here and I'm happy to change the language.
>> > > >
>> > > > Yes please a comment explaining why that piece of code is there is far
>> > > > more intuitive.
>> > >
>> > > OK, once we have agreed the below I can do that.
>> > >
>> > > >
>> > > > >  As I probably
>> > > > > mentioned at the time, my accepted patch breaks my workflow and
>> > > > > several boards. I hope you can understand how frustrating that sort of
>> > > > > thing can be.
>> > > >
>> > > > Yes, I do and I am fine with a short-term patch that fixes issues, as
>> > > > long as its not too intrusive. There is a very thin line between quick
>> > > > and dirty fixes to spaghetti unreadable code. But we should have
>> > > > comments and/or commit messages indicating that this needs a proper
>> > > > fix
>> > >
>> > > I spent a lot of time explaining this last time.
>> > >
>> > > >
>> > > > > Also, now that I have my lab back up and running and I
>> > > > > would like these boards to work again on mainline!
>> > > > >
>> > > > > >
>> > > > > > Grepping for OF_BLOBLIST, I can't find any matches, so is the above if a typo?
>> > > > >
>> > > > > Remember, it was a patch you rejected :-)
>> > > >
>> > > > I don't maintain any of that. I only gave some feedback along the
>> > > > lines of "bloblist was designed to be auto-discoverable, I don't see
>> > > > how adding an explicit Kconfig helps". IIRC we eventually followed
>> > > > what Tom suggested.
>> > >
>> > > I'm not trying to point the finger here. So far the boards are broken
>> > > in mainline...I'm just trying to fix that,
>> > >
>> > > >
>> > > > In any case, the amount of bike-shedding in the topic is too much. Do
>> > > > you mind explaining the problem in your workflow again? Perhaps we can
>> > > > find a solution that is integrated in bloblist_maybe_init() instead of
>> > > > injecting ifs on when a bloblist should or should not be searched for
>> > > > all over the place.
>> > >
>> > > TPL (or SPL) sets up a bloblist with bits of info in it, but no DT
>> > > (which is in memory-mapped SPI flash)
>> > > U-Boot proper starts up, it wants to get the bloblist but not hang if
>> > > the bloblist doesn't have a DT
>> >
>> > Sure, that's reasonable and IIRC that's the design we agreed a while back.
>> > Looking at the code, if the DT isn't found in a bloblist and the feature is
>> > enabled, we don't crash. We just print a debug message and continue to find
>> > the DT as we used. Why do we have to skip the call to
>> > bloblist_maybe_init()?
>>
>> Because at this point, if there is no bloblist, then it needs to be
>> created. But creating it this early may fail, e.g. since there is no
>> malloc(). The intent of this code is to locate an FDT from an existing
>> bloblist. There is no point in creating a new bloblist here, since it
>> obviously won't have an FDT in it.
>>
> Can we add a bool arg to bloblist_init for this?
> Eg. int bloblist_init(bool allow_malloc);

Yes, we can do anything, but that seems like a hack to me...if we init
the bloblist for the first time in the current phase, then it
obviously won't have an FDT inside it. It is the unconditional
requirement of an FDT which is causing the problems here.

Regards,
Simon