| Message ID | 20200327172855.11746-1-olteanv@gmail.com |
|---|---|
| State | Changes Requested |
| Delegated to: | Simon Glass |
| Headers | show |
| Series | fdtdec: use full path in fdtdec_get_alias_seq comparison | expand |
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@2120000/ethernet-switch@0/ports/port@0"; > eth1 = "/pcie@1f0000000/pci@0,5/ports/port@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@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
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@2120000/ethernet-switch@0/ports/port@0"; > > eth1 = "/pcie@1f0000000/pci@0,5/ports/port@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@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
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@2120000/ethernet-switch@0/ports/port@0"; > > > eth1 = "/pcie@1f0000000/pci@0,5/ports/port@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@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
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@2120000/ethernet-switch@0/ports/port@0"; > > > > eth1 = "/pcie@1f0000000/pci@0,5/ports/port@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@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
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@2120000/ethernet-switch@0/ports/port@0"; > > > > > eth1 = "/pcie@1f0000000/pci@0,5/ports/port@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@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
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) {