Hi Caleb,
On Wed, 27 Nov 2024 at 10:12, Caleb Connolly <caleb.conno...@linaro.org> wrote:
>
> Hi Simon,
>
> On 27/11/2024 17:52, Simon Glass wrote:
> > Hi Caleb,
> >
> > On Tue, 12 Nov 2024 at 22:21, Caleb Connolly <caleb.conno...@linaro.org>
> > wrote:
> >>
> >> Under some conditions it's possible to hit the null condition here like
> >> when running with OF_LIVE and using the ofnode API before
> >> initr_of_live() is called. There is very little null
> >> checking for this in the FDT framework, so returning null here can
> >> result in weird null pointer conditions.
> >>
> >> Instead let's return the control FDT in the fallback case, this is
> >> usually what the user was expecting.
> >>
> >> Signed-off-by: Caleb Connolly <caleb.conno...@linaro.org>
> >> ---
> >> drivers/core/ofnode.c | 2 +-
> >> 1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/core/ofnode.c b/drivers/core/ofnode.c
> >> index 950895e72a99..7a7f25fc537c 100644
> >> --- a/drivers/core/ofnode.c
> >> +++ b/drivers/core/ofnode.c
> >> @@ -152,9 +152,9 @@ void *ofnode_lookup_fdt(ofnode node)
> >> uint i = OFTREE_TREE_ID(node.of_offset);
> >>
> >> if (i >= oftree_count) {
> >> log_debug("Invalid tree ID %x\n", i);
> >> - return NULL;
> >> + return (void *)gd->fdt_blob;
> >> }
> >>
> >> return oftree_list[i];
> >> } else {
> >> --
> >> 2.47.0
> >>
> >
> > Eek I really don't like that, since it will silently return an
> > unexpected value.
> What's the unexpected value? The only caller for this is ofnode_to_fdt()
> and the return value for that is never checked. It seems clear to me
> that in practise NULL is the unexpected value here.
Firstly, it looks like ofnode_lookup_fdt() should be static
If a node offset cannot be found, that suggests that oftree_list[] is
empty, which suggests that oftree_reset() has not been called yet.
oftree_reset() is called after relocation. Before relocation, it just
assumes that the control FDT is being used.
I wonder if you are hitting this problem after relocation, in one of
the functions in init_sequence_r[] before initr_dm()?
In any case this does seem like a bug that we should fix.
>
> > I think we should panic. Do you know what code path
> > gets you here?
>
> Hmm, I don't exactly recall. Should really have made a note when I wrote
> this patch :/
Yes, I know that feeling :-(
>
> Guess I'll drop this from my dev branch and come back when I figure this
> out again.
Yes OK. A patch which panics would be fine for now, if you like.
> >
> > More work is needed in livetree to sort of the rough edges, sadly...
Regards,
Simon
