* [PATCH] fdtdec: use full path in fdtdec_get_alias_seq comparison
@ 2020-03-27 17:28 Vladimir Oltean
2020-03-28 20:05 ` Simon Glass
0 siblings, 1 reply; 6+ messages in thread
From: Vladimir Oltean @ 2020-03-27 17:28 UTC (permalink / raw)
To: u-boot
From: Vladimir Oltean <vladimir.oltean@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@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));
+ path_len = strlen(path);
+ debug("Looking for '%s' at %d, path %s\n", base, offset, path);
aliases = fdt_path_offset(blob, "/aliases");
for (prop_offset = fdt_first_property_offset(blob, aliases);
@@ -469,17 +470,15 @@ int fdtdec_get_alias_seq(const void *blob, const char *base, int offset,
prop_offset = fdt_next_property_offset(blob, prop_offset)) {
const char *prop;
const char *name;
- const char *slash;
int len, val;
prop = fdt_getprop_by_offset(blob, prop_offset, &name, &len);
debug(" - %s, %s\n", name, prop);
- if (len < find_namelen || *prop != '/' || prop[len - 1] ||
+ if (len < path_len || *prop != '/' || prop[len - 1] ||
strncmp(name, base, base_len))
continue;
- slash = strrchr(prop, '/');
- if (strcmp(slash + 1, find_name))
+ if (strcmp(prop, path))
continue;
val = trailing_strtol(name);
if (val != -1) {
--
2.17.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH] fdtdec: use full path in fdtdec_get_alias_seq comparison
2020-03-27 17:28 [PATCH] fdtdec: use full path in fdtdec_get_alias_seq comparison Vladimir Oltean
@ 2020-03-28 20:05 ` Simon Glass
2020-03-28 20:25 ` Vladimir Oltean
0 siblings, 1 reply; 6+ messages in thread
From: Simon Glass @ 2020-03-28 20:05 UTC (permalink / raw)
To: u-boot
Hi Vladimir,
On Fri, 27 Mar 2020 at 11:29, Vladimir Oltean <olteanv@gmail.com> wrote:
>
> From: Vladimir Oltean <vladimir.oltean@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@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?
> + path_len = strlen(path);
> + debug("Looking for '%s' at %d, path %s\n", base, offset, path);
>
> aliases = fdt_path_offset(blob, "/aliases");
> for (prop_offset = fdt_first_property_offset(blob, aliases);
> @@ -469,17 +470,15 @@ int fdtdec_get_alias_seq(const void *blob, const char *base, int offset,
> prop_offset = fdt_next_property_offset(blob, prop_offset)) {
> const char *prop;
> const char *name;
> - const char *slash;
> int len, val;
>
> prop = fdt_getprop_by_offset(blob, prop_offset, &name, &len);
> debug(" - %s, %s\n", name, prop);
> - if (len < find_namelen || *prop != '/' || prop[len - 1] ||
> + if (len < path_len || *prop != '/' || prop[len - 1] ||
> strncmp(name, base, base_len))
> continue;
>
> - slash = strrchr(prop, '/');
> - if (strcmp(slash + 1, find_name))
> + if (strcmp(prop, path))
> continue;
> val = trailing_strtol(name);
> if (val != -1) {
> --
> 2.17.1
>
Regards,
Simon
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH] fdtdec: use full path in fdtdec_get_alias_seq comparison
2020-03-28 20:05 ` Simon Glass
@ 2020-03-28 20:25 ` Vladimir Oltean
2020-03-28 21:00 ` Simon Glass
0 siblings, 1 reply; 6+ messages in thread
From: Vladimir Oltean @ 2020-03-28 20:25 UTC (permalink / raw)
To: u-boot
Hi Simon,
On Sat, 28 Mar 2020 at 22:06, Simon Glass <sjg@chromium.org> wrote:
>
> Hi Vladimir,
>
> On Fri, 27 Mar 2020 at 11:29, Vladimir Oltean <olteanv@gmail.com> wrote:
> >
> > From: Vladimir Oltean <vladimir.oltean@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@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?
Why is this an odd case? I thought aliases are supposed to match on
full path, no?
> > + path_len = strlen(path);
> > + debug("Looking for '%s' at %d, path %s\n", base, offset, path);
> >
> > aliases = fdt_path_offset(blob, "/aliases");
> > for (prop_offset = fdt_first_property_offset(blob, aliases);
> > @@ -469,17 +470,15 @@ int fdtdec_get_alias_seq(const void *blob, const char *base, int offset,
> > prop_offset = fdt_next_property_offset(blob, prop_offset)) {
> > const char *prop;
> > const char *name;
> > - const char *slash;
> > int len, val;
> >
> > prop = fdt_getprop_by_offset(blob, prop_offset, &name, &len);
> > debug(" - %s, %s\n", name, prop);
> > - if (len < find_namelen || *prop != '/' || prop[len - 1] ||
> > + if (len < path_len || *prop != '/' || prop[len - 1] ||
> > strncmp(name, base, base_len))
> > continue;
> >
> > - slash = strrchr(prop, '/');
> > - if (strcmp(slash + 1, find_name))
> > + if (strcmp(prop, path))
> > continue;
> > val = trailing_strtol(name);
> > if (val != -1) {
> > --
> > 2.17.1
> >
>
> Regards,
> Simon
Regards,
-Vladimir
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH] fdtdec: use full path in fdtdec_get_alias_seq comparison
2020-03-28 20:25 ` Vladimir Oltean
@ 2020-03-28 21:00 ` Simon Glass
2020-03-28 21:12 ` Vladimir Oltean
0 siblings, 1 reply; 6+ messages in thread
From: Simon Glass @ 2020-03-28 21:00 UTC (permalink / raw)
To: u-boot
Hi Vladimir,
On Sat, 28 Mar 2020 at 14:25, Vladimir Oltean <olteanv@gmail.com> wrote:
>
> Hi Simon,
>
> On Sat, 28 Mar 2020 at 22:06, Simon Glass <sjg@chromium.org> wrote:
> >
> > Hi Vladimir,
> >
> > On Fri, 27 Mar 2020 at 11:29, Vladimir Oltean <olteanv@gmail.com> wrote:
> > >
> > > From: Vladimir Oltean <vladimir.oltean@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@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
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH] fdtdec: use full path in fdtdec_get_alias_seq comparison
2020-03-28 21:00 ` Simon Glass
@ 2020-03-28 21:12 ` Vladimir Oltean
2020-03-28 22:00 ` Simon Glass
0 siblings, 1 reply; 6+ messages in thread
From: Vladimir Oltean @ 2020-03-28 21:12 UTC (permalink / raw)
To: u-boot
On Sat, 28 Mar 2020 at 23:00, Simon Glass <sjg@chromium.org> wrote:
>
> Hi Vladimir,
>
> On Sat, 28 Mar 2020 at 14:25, Vladimir Oltean <olteanv@gmail.com> wrote:
> >
> > Hi Simon,
> >
> > On Sat, 28 Mar 2020 at 22:06, Simon Glass <sjg@chromium.org> wrote:
> > >
> > > Hi Vladimir,
> > >
> > > On Fri, 27 Mar 2020 at 11:29, Vladimir Oltean <olteanv@gmail.com> wrote:
> > > >
> > > > From: Vladimir Oltean <vladimir.oltean@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@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
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH] fdtdec: use full path in fdtdec_get_alias_seq comparison
2020-03-28 21:12 ` Vladimir Oltean
@ 2020-03-28 22:00 ` Simon Glass
0 siblings, 0 replies; 6+ messages in thread
From: Simon Glass @ 2020-03-28 22:00 UTC (permalink / raw)
To: u-boot
Hi Vladimir,
On Sat, 28 Mar 2020 at 15:12, Vladimir Oltean <olteanv@gmail.com> wrote:
>
> On Sat, 28 Mar 2020 at 23:00, Simon Glass <sjg@chromium.org> wrote:
> >
> > Hi Vladimir,
> >
> > On Sat, 28 Mar 2020 at 14:25, Vladimir Oltean <olteanv@gmail.com> wrote:
> > >
> > > Hi Simon,
> > >
> > > On Sat, 28 Mar 2020 at 22:06, Simon Glass <sjg@chromium.org> wrote:
> > > >
> > > > Hi Vladimir,
> > > >
> > > > On Fri, 27 Mar 2020 at 11:29, Vladimir Oltean <olteanv@gmail.com> wrote:
> > > > >
> > > > > From: Vladimir Oltean <vladimir.oltean@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@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
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2020-03-28 22:00 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2020-03-27 17:28 [PATCH] fdtdec: use full path in fdtdec_get_alias_seq comparison Vladimir Oltean
2020-03-28 20:05 ` Simon Glass
2020-03-28 20:25 ` Vladimir Oltean
2020-03-28 21:00 ` Simon Glass
2020-03-28 21:12 ` Vladimir Oltean
2020-03-28 22:00 ` Simon Glass
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox