[PATCH] fdtdec: use full path in fdtdec_get_alias_seq comparison

[Vladimir Oltean]

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?

Regards,
-Vladimir