[PATCH v2] of/fdt: Remove redundant kbasename function call

[PATCH v2] of/fdt: Remove redundant kbasename function call

[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

Re: [PATCH v2] of/fdt: Remove redundant kbasename function call

[Rob Herring]

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!

Re: [PATCH v2] of/fdt: Remove redundant kbasename function call

[Chris Packham]

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.

Re: [PATCH v2] of/fdt: Remove redundant kbasename function call

[Rob Herring]

+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

Re: [PATCH v2] of/fdt: Remove redundant kbasename function call

[Chris Packham]

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.

Re: [PATCH v2] of/fdt: Remove redundant kbasename function call

[Rob Herring]

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