[PATCH] fdtdec: use full path in fdtdec_get_alias_seq comparison

Simon Glass sjg at chromium.org
Sat Mar 28 23:00:53 CET 2020


Hi Vladimir,

On Sat, 28 Mar 2020 at 15:12, Vladimir Oltean <olteanv at gmail.com> wrote:
>
> On Sat, 28 Mar 2020 at 23:00, Simon Glass <sjg at chromium.org> wrote:
> >
> > Hi Vladimir,
> >
> > On Sat, 28 Mar 2020 at 14:25, Vladimir Oltean <olteanv at gmail.com> wrote:
> > >
> > > Hi Simon,
> > >
> > > On Sat, 28 Mar 2020 at 22:06, Simon Glass <sjg at chromium.org> wrote:
> > > >
> > > > Hi Vladimir,
> > > >
> > > > On Fri, 27 Mar 2020 at 11:29, Vladimir Oltean <olteanv at gmail.com> wrote:
> > > > >
> > > > > From: Vladimir Oltean <vladimir.oltean at nxp.com>
> > > > >
> > > > > Currently fdtdec_get_alias_seq() calls fdt_get_name() which returns only
> > > > > the name of the leaf node. So it needs to also trim the path of the
> > > > > alias to the leaf name only, leading to imperfect matches.
> > > > >
> > > > > This means that the following aliases:
> > > > >
> > > > >     /aliases {
> > > > >         eth0 = "/dspi at 2120000/ethernet-switch at 0/ports/port at 0";
> > > > >         eth1 = "/pcie at 1f0000000/pci at 0,5/ports/port at 0";
> > > > >     };
> > > > >
> > > > > will make fdtdec_get_alias_seq to return a seq of zero for both aliases,
> > > > > as the match will only take place on the "port at 0" portion.
> > > > >
> > > > > Fix this by calling fdt_get_path and comparing the full path of the
> > > > > alias.
> > > > >
> > > > > Fixes: 5c33c9fdbb3f ("fdt: Add a function to get the alias sequence of a node")
> > > > > Signed-off-by: Vladimir Oltean <vladimir.oltean at nxp.com>
> > > > > ---
> > > > >  lib/fdtdec.c | 15 +++++++--------
> > > > >  1 file changed, 7 insertions(+), 8 deletions(-)
> > > > >
> > > > > diff --git a/lib/fdtdec.c b/lib/fdtdec.c
> > > > > index eb11fc898e30..55c1b36e823b 100644
> > > > > --- a/lib/fdtdec.c
> > > > > +++ b/lib/fdtdec.c
> > > > > @@ -455,13 +455,14 @@ int fdtdec_get_alias_seq(const void *blob, const char *base, int offset,
> > > > >                          int *seqp)
> > > > >  {
> > > > >         int base_len = strlen(base);
> > > > > -       const char *find_name;
> > > > > -       int find_namelen;
> > > > >         int prop_offset;
> > > > > +       char path[64];
> > > > > +       int path_len;
> > > > >         int aliases;
> > > > >
> > > > > -       find_name = fdt_get_name(blob, offset, &find_namelen);
> > > > > -       debug("Looking for '%s' at %d, name %s\n", base, offset, find_name);
> > > > > +       fdt_get_path(blob, offset, path, sizeof(path));
> > > >
> > > > This is really slow. I would prefer not to change this for what seems
> > > > like an odd case.
> > > >
> > > > Can you enable CONFIG_OF_LIVE instead?
> > > >
> > >
> > > No, why would I want to do that?
> >
> > Because it uses a different algorithm - see of_alias_get_id(). It
> > might already work.
> >
> > > Why is this an odd case? I thought aliases are supposed to match on
> > > full path, no?
> >
> > Because we've been using the current algorithm for 5+ years without any comment.
> >
> > Note that fdt_get_path() likely takes a long time in SPL and before relocation.
> >
> > Regards,
> > Simon
>
> So we have no disagreement about the fact that the function doesn't do
> what's written on the box, and that the implementation for
> CONFIG_OF_LIVE=y and CONFIG_OF_LIVE=n yields different results,
> however your argument is that we should leave it alone because nobody
> noticed thus far? Don't I count?

You are very special and count a lot :-)

Actually my argument is mostly that fdt_get_path() is a very slow
function and I would like to avoid calling it in SPL if at all
possible.

Does your board need to access this alias before relocation?

Regards,
Simon


More information about the U-Boot mailing list