For version 1 to 3 of the device tree, this is the node full
path as a zero terminated string, starting with "/". The
following equation will not hold, since the node name has
been processed in the fdt_get_name().
*pathp == '/'
For version 16 and later, this is the node unit name only
(or an empty string for the root node). So the above
equation will still not hold.
So the kbasename() is redundant, just remove it.
Signed-off-by: Qi Zheng <[email protected]>
---
Change in v2:
remove another kbasename() also.
drivers/of/fdt.c | 4 ----
1 file changed, 4 deletions(-)
diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c
index 38619e9ef6b2..4602e467ca8b 100644
--- a/drivers/of/fdt.c
+++ b/drivers/of/fdt.c
@@ -643,8 +643,6 @@ int __init of_scan_flat_dt(int (*it)(unsigned long node,
offset = fdt_next_node(blob, offset, &depth)) {
pathp = fdt_get_name(blob, offset, NULL);
- if (*pathp == '/')
- pathp = kbasename(pathp);
rc = it(offset, pathp, depth, data);
}
return rc;
@@ -671,8 +669,6 @@ int __init of_scan_flat_dt_subnodes(unsigned long parent,
int rc;
pathp = fdt_get_name(blob, node, NULL);
- if (*pathp == '/')
- pathp = kbasename(pathp);
rc = it(node, pathp, data);
if (rc)
return rc;
--
2.25.1
On Thu, 28 May 2020 21:25:41 +0800, Qi Zheng wrote:
> For version 1 to 3 of the device tree, this is the node full
> path as a zero terminated string, starting with "/". The
> following equation will not hold, since the node name has
> been processed in the fdt_get_name().
>
> *pathp == '/'
>
> For version 16 and later, this is the node unit name only
> (or an empty string for the root node). So the above
> equation will still not hold.
>
> So the kbasename() is redundant, just remove it.
>
> Signed-off-by: Qi Zheng <[email protected]>
> ---
>
> Change in v2:
> remove another kbasename() also.
>
> drivers/of/fdt.c | 4 ----
> 1 file changed, 4 deletions(-)
>
Applied, thanks!
Hi All,
On 29/05/20 1:25 am, Qi Zheng wrote:
> For version 1 to 3 of the device tree, this is the node full
> path as a zero terminated string, starting with "/". The
> following equation will not hold, since the node name has
> been processed in the fdt_get_name().
>
> *pathp == '/'
>
> For version 16 and later, this is the node unit name only
> (or an empty string for the root node). So the above
> equation will still not hold.
>
> So the kbasename() is redundant, just remove it.
>
> Signed-off-by: Qi Zheng <[email protected]>
> ---
>
> Change in v2:
> remove another kbasename() also.
>
> drivers/of/fdt.c | 4 ----
> 1 file changed, 4 deletions(-)
>
> diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c
> index 38619e9ef6b2..4602e467ca8b 100644
> --- a/drivers/of/fdt.c
> +++ b/drivers/of/fdt.c
> @@ -643,8 +643,6 @@ int __init of_scan_flat_dt(int (*it)(unsigned long node,
> offset = fdt_next_node(blob, offset, &depth)) {
>
> pathp = fdt_get_name(blob, offset, NULL);
> - if (*pathp == '/')
> - pathp = kbasename(pathp);
> rc = it(offset, pathp, depth, data);
> }
> return rc;
> @@ -671,8 +669,6 @@ int __init of_scan_flat_dt_subnodes(unsigned long parent,
> int rc;
>
> pathp = fdt_get_name(blob, node, NULL);
> - if (*pathp == '/')
> - pathp = kbasename(pathp);
> rc = it(node, pathp, data);
> if (rc)
> return rc;
I'm trying to keep our boards up to date with newer kernels.
I've just hit a problem on an older board that uses
CONFIG_ARM_APPENDED_DTB and has a number of command line args passed up
from the bootloader that are required for a successful boot.
I'm stepping through kernel versions in the hope that keeping things
running is easier in smaller increments I'm up to v5.8. I'm not
currently able to check a newer kernel on this board but looking at the
code the problem still seems to exist in the latest tree.
early_init_dt_scan_chosen() searches for "chosen" prior to this change
the "/chosen" node that gets inserted by atags_to_fdt.c but with this
change it can't find it and fails to boot.
+LAKML given it's an Arm issue
On Mon, Jan 25, 2021 at 6:47 PM Chris Packham
<[email protected]> wrote:
>
> Hi All,
>
> On 29/05/20 1:25 am, Qi Zheng wrote:
> > For version 1 to 3 of the device tree, this is the node full
> > path as a zero terminated string, starting with "/". The
> > following equation will not hold, since the node name has
> > been processed in the fdt_get_name().
> >
> > *pathp == '/'
> >
> > For version 16 and later, this is the node unit name only
> > (or an empty string for the root node). So the above
> > equation will still not hold.
> >
> > So the kbasename() is redundant, just remove it.
> >
> > Signed-off-by: Qi Zheng <[email protected]>
> > ---
> >
> > Change in v2:
> > remove another kbasename() also.
> >
> > drivers/of/fdt.c | 4 ----
> > 1 file changed, 4 deletions(-)
> >
> > diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c
> > index 38619e9ef6b2..4602e467ca8b 100644
> > --- a/drivers/of/fdt.c
> > +++ b/drivers/of/fdt.c
> > @@ -643,8 +643,6 @@ int __init of_scan_flat_dt(int (*it)(unsigned long node,
> > offset = fdt_next_node(blob, offset, &depth)) {
> >
> > pathp = fdt_get_name(blob, offset, NULL);
> > - if (*pathp == '/')
> > - pathp = kbasename(pathp);
> > rc = it(offset, pathp, depth, data);
> > }
> > return rc;
> > @@ -671,8 +669,6 @@ int __init of_scan_flat_dt_subnodes(unsigned long parent,
> > int rc;
> >
> > pathp = fdt_get_name(blob, node, NULL);
> > - if (*pathp == '/')
> > - pathp = kbasename(pathp);
> > rc = it(node, pathp, data);
> > if (rc)
> > return rc;
>
> I'm trying to keep our boards up to date with newer kernels.
>
> I've just hit a problem on an older board that uses
> CONFIG_ARM_APPENDED_DTB and has a number of command line args passed up
> from the bootloader that are required for a successful boot.
>
> I'm stepping through kernel versions in the hope that keeping things
> running is easier in smaller increments I'm up to v5.8. I'm not
> currently able to check a newer kernel on this board but looking at the
> code the problem still seems to exist in the latest tree.
>
> early_init_dt_scan_chosen() searches for "chosen" prior to this change
> the "/chosen" node that gets inserted by atags_to_fdt.c but with this
> change it can't find it and fails to boot.
Given this code works for normal cases, I'm guessing the problem is in
atags_to_fdt.c or libfdt. Is it possible to add an empty chosen node
to the DT and see if that makes any difference.
Rob
On 26/01/21 3:06 pm, Rob Herring wrote:
> +LAKML given it's an Arm issue
>
> On Mon, Jan 25, 2021 at 6:47 PM Chris Packham
> <[email protected]> wrote:
>> Hi All,
>>
>> On 29/05/20 1:25 am, Qi Zheng wrote:
>>> For version 1 to 3 of the device tree, this is the node full
>>> path as a zero terminated string, starting with "/". The
>>> following equation will not hold, since the node name has
>>> been processed in the fdt_get_name().
>>>
>>> *pathp == '/'
>>>
>>> For version 16 and later, this is the node unit name only
>>> (or an empty string for the root node). So the above
>>> equation will still not hold.
>>>
>>> So the kbasename() is redundant, just remove it.
>>>
>>> Signed-off-by: Qi Zheng <[email protected]>
>>> ---
>>>
>>> Change in v2:
>>> remove another kbasename() also.
>>>
>>> drivers/of/fdt.c | 4 ----
>>> 1 file changed, 4 deletions(-)
>>>
>>> diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c
>>> index 38619e9ef6b2..4602e467ca8b 100644
>>> --- a/drivers/of/fdt.c
>>> +++ b/drivers/of/fdt.c
>>> @@ -643,8 +643,6 @@ int __init of_scan_flat_dt(int (*it)(unsigned long node,
>>> offset = fdt_next_node(blob, offset, &depth)) {
>>>
>>> pathp = fdt_get_name(blob, offset, NULL);
>>> - if (*pathp == '/')
>>> - pathp = kbasename(pathp);
>>> rc = it(offset, pathp, depth, data);
>>> }
>>> return rc;
>>> @@ -671,8 +669,6 @@ int __init of_scan_flat_dt_subnodes(unsigned long parent,
>>> int rc;
>>>
>>> pathp = fdt_get_name(blob, node, NULL);
>>> - if (*pathp == '/')
>>> - pathp = kbasename(pathp);
>>> rc = it(node, pathp, data);
>>> if (rc)
>>> return rc;
>> I'm trying to keep our boards up to date with newer kernels.
>>
>> I've just hit a problem on an older board that uses
>> CONFIG_ARM_APPENDED_DTB and has a number of command line args passed up
>> from the bootloader that are required for a successful boot.
>>
>> I'm stepping through kernel versions in the hope that keeping things
>> running is easier in smaller increments I'm up to v5.8. I'm not
>> currently able to check a newer kernel on this board but looking at the
>> code the problem still seems to exist in the latest tree.
>>
>> early_init_dt_scan_chosen() searches for "chosen" prior to this change
>> the "/chosen" node that gets inserted by atags_to_fdt.c but with this
>> change it can't find it and fails to boot.
> Given this code works for normal cases, I'm guessing the problem is in
> atags_to_fdt.c or libfdt.
It might be related tot this snippet of libfdt
const char *fdt_get_name(const void *fdt, int nodeoffset, int *len)
{
...
if (!can_assume(LATEST) && fdt_version(fdt) < 0x10) {
/*
* For old FDT versions, match the naming conventions
of V16:
* give only the leaf name (after all /). The actual tree
* contents are loosely checked.
*/
const char *leaf;
leaf = strrchr(nameptr, '/');
if (leaf == NULL) {
err = -FDT_ERR_BADSTRUCTURE;
goto fail;
}
nameptr = leaf+1;
}
...
}
On my system that if evaluates to 0
> Is it possible to add an empty chosen node
> to the DT and see if that makes any difference.
I'll give that a try.
On Mon, Jan 25, 2021 at 8:06 PM Rob Herring <[email protected]> wrote:
>
> +LAKML given it's an Arm issue
>
> On Mon, Jan 25, 2021 at 6:47 PM Chris Packham
> <[email protected]> wrote:
> >
> > Hi All,
> >
> > On 29/05/20 1:25 am, Qi Zheng wrote:
> > > For version 1 to 3 of the device tree, this is the node full
> > > path as a zero terminated string, starting with "/". The
> > > following equation will not hold, since the node name has
> > > been processed in the fdt_get_name().
> > >
> > > *pathp == '/'
> > >
> > > For version 16 and later, this is the node unit name only
> > > (or an empty string for the root node). So the above
> > > equation will still not hold.
> > >
> > > So the kbasename() is redundant, just remove it.
> > >
> > > Signed-off-by: Qi Zheng <[email protected]>
> > > ---
> > >
> > > Change in v2:
> > > remove another kbasename() also.
> > >
> > > drivers/of/fdt.c | 4 ----
> > > 1 file changed, 4 deletions(-)
> > >
> > > diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c
> > > index 38619e9ef6b2..4602e467ca8b 100644
> > > --- a/drivers/of/fdt.c
> > > +++ b/drivers/of/fdt.c
> > > @@ -643,8 +643,6 @@ int __init of_scan_flat_dt(int (*it)(unsigned long node,
> > > offset = fdt_next_node(blob, offset, &depth)) {
> > >
> > > pathp = fdt_get_name(blob, offset, NULL);
> > > - if (*pathp == '/')
> > > - pathp = kbasename(pathp);
> > > rc = it(offset, pathp, depth, data);
> > > }
> > > return rc;
> > > @@ -671,8 +669,6 @@ int __init of_scan_flat_dt_subnodes(unsigned long parent,
> > > int rc;
> > >
> > > pathp = fdt_get_name(blob, node, NULL);
> > > - if (*pathp == '/')
> > > - pathp = kbasename(pathp);
> > > rc = it(node, pathp, data);
> > > if (rc)
> > > return rc;
> >
> > I'm trying to keep our boards up to date with newer kernels.
> >
> > I've just hit a problem on an older board that uses
> > CONFIG_ARM_APPENDED_DTB and has a number of command line args passed up
> > from the bootloader that are required for a successful boot.
> >
> > I'm stepping through kernel versions in the hope that keeping things
> > running is easier in smaller increments I'm up to v5.8. I'm not
> > currently able to check a newer kernel on this board but looking at the
> > code the problem still seems to exist in the latest tree.
> >
> > early_init_dt_scan_chosen() searches for "chosen" prior to this change
> > the "/chosen" node that gets inserted by atags_to_fdt.c but with this
> > change it can't find it and fails to boot.
>
> Given this code works for normal cases, I'm guessing the problem is in
> atags_to_fdt.c or libfdt. Is it possible to add an empty chosen node
> to the DT and see if that makes any difference.
This will work. It's a atags_to_fdt.c bug. In node_offset(), we search
by path (/chosen) with fdt_path_offset. If not found, then we add a
subnode called '/chosen' (instead of 'chosen') with fdt_add_subnode().
Though arguably, libfdt could catch and handle this. I'll send a patch
in a minute.
Rob