| Message ID | 20200528132541.463300-1-arch0.zheng@gmail.com |
|---|---|
| State | Accepted, archived |
| Headers | show |
| Series | [v2] of/fdt: Remove redundant kbasename function call | expand |
| Context | Check | Description |
|---|---|---|
| robh/checkpatch | success |
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!
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.
+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
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.
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
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;
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(-)