* [PATCH v2] of/fdt: Remove redundant kbasename function call
@ 2020-05-28 13:25 Qi Zheng
2020-05-29 2:57 ` Rob Herring
2021-01-26 0:33 ` Chris Packham
0 siblings, 2 replies; 6+ messages in thread
From: Qi Zheng @ 2020-05-28 13:25 UTC (permalink / raw)
To: robh+dt, robh, frowand.list; +Cc: devicetree, linux-kernel, Qi Zheng
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 <arch0.zheng@gmail.com>
---
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
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH v2] of/fdt: Remove redundant kbasename function call
2020-05-28 13:25 [PATCH v2] of/fdt: Remove redundant kbasename function call Qi Zheng
@ 2020-05-29 2:57 ` Rob Herring
2021-01-26 0:33 ` Chris Packham
1 sibling, 0 replies; 6+ messages in thread
From: Rob Herring @ 2020-05-29 2:57 UTC (permalink / raw)
To: Qi Zheng; +Cc: devicetree, frowand.list, robh+dt, linux-kernel
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 <arch0.zheng@gmail.com>
> ---
>
> Change in v2:
> remove another kbasename() also.
>
> drivers/of/fdt.c | 4 ----
> 1 file changed, 4 deletions(-)
>
Applied, thanks!
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] of/fdt: Remove redundant kbasename function call
2020-05-28 13:25 [PATCH v2] of/fdt: Remove redundant kbasename function call Qi Zheng
2020-05-29 2:57 ` Rob Herring
@ 2021-01-26 0:33 ` Chris Packham
2021-01-26 2:06 ` Rob Herring
1 sibling, 1 reply; 6+ messages in thread
From: Chris Packham @ 2021-01-26 0:33 UTC (permalink / raw)
To: Qi Zheng, robh+dt@kernel.org, robh@kernel.org,
frowand.list@gmail.com
Cc: devicetree@vger.kernel.org, linux-kernel@vger.kernel.org
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 <arch0.zheng@gmail.com>
> ---
>
> 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.
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH v2] of/fdt: Remove redundant kbasename function call
2021-01-26 0:33 ` Chris Packham
@ 2021-01-26 2:06 ` Rob Herring
2021-01-26 2:34 ` Chris Packham
2021-01-26 2:34 ` Rob Herring
0 siblings, 2 replies; 6+ messages in thread
From: Rob Herring @ 2021-01-26 2:06 UTC (permalink / raw)
To: Chris Packham
Cc: Qi Zheng, frowand.list@gmail.com, devicetree@vger.kernel.org,
linux-kernel@vger.kernel.org, linux-arm-kernel
+LAKML given it's an Arm issue
On Mon, Jan 25, 2021 at 6:47 PM Chris Packham
<Chris.Packham@alliedtelesis.co.nz> 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 <arch0.zheng@gmail.com>
> > ---
> >
> > 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
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH v2] of/fdt: Remove redundant kbasename function call
2021-01-26 2:06 ` Rob Herring
@ 2021-01-26 2:34 ` Chris Packham
2021-01-26 2:34 ` Rob Herring
1 sibling, 0 replies; 6+ messages in thread
From: Chris Packham @ 2021-01-26 2:34 UTC (permalink / raw)
To: Rob Herring
Cc: Qi Zheng, frowand.list@gmail.com, devicetree@vger.kernel.org,
linux-kernel@vger.kernel.org, linux-arm-kernel
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
> <Chris.Packham@alliedtelesis.co.nz> 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 <arch0.zheng@gmail.com>
>>> ---
>>>
>>> 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.
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH v2] of/fdt: Remove redundant kbasename function call
2021-01-26 2:06 ` Rob Herring
2021-01-26 2:34 ` Chris Packham
@ 2021-01-26 2:34 ` Rob Herring
1 sibling, 0 replies; 6+ messages in thread
From: Rob Herring @ 2021-01-26 2:34 UTC (permalink / raw)
To: Chris Packham
Cc: Qi Zheng, frowand.list@gmail.com, devicetree@vger.kernel.org,
linux-kernel@vger.kernel.org, linux-arm-kernel
On Mon, Jan 25, 2021 at 8:06 PM Rob Herring <robh+dt@kernel.org> wrote:
>
> +LAKML given it's an Arm issue
>
> On Mon, Jan 25, 2021 at 6:47 PM Chris Packham
> <Chris.Packham@alliedtelesis.co.nz> 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 <arch0.zheng@gmail.com>
> > > ---
> > >
> > > 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
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2021-01-26 21:13 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2020-05-28 13:25 [PATCH v2] of/fdt: Remove redundant kbasename function call Qi Zheng
2020-05-29 2:57 ` Rob Herring
2021-01-26 0:33 ` Chris Packham
2021-01-26 2:06 ` Rob Herring
2021-01-26 2:34 ` Chris Packham
2021-01-26 2:34 ` Rob Herring
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).