2022-04-27 11:43:17

by Clément Léger

[permalink] [raw]
Subject: [PATCH 1/3] of: always populate a root node

When enabling CONFIG_OF on a platform where of_root is not populated by
firmware, we end up without a root node. In order to apply overlays and
create subnodes of the root node, we need one. This commit creates an
empty root node if not present.

Co-developed-by: Rob Herring <[email protected]>
Signed-off-by: Rob Herring <[email protected]>
Signed-off-by: Clément Léger <[email protected]>
---
drivers/of/base.c | 16 ++++++++++++++--
1 file changed, 14 insertions(+), 2 deletions(-)

diff --git a/drivers/of/base.c b/drivers/of/base.c
index e7d92b67cb8a..6b8584c39f73 100644
--- a/drivers/of/base.c
+++ b/drivers/of/base.c
@@ -177,6 +177,19 @@ void __init of_core_init(void)
pr_err("failed to register existing nodes\n");
return;
}
+
+ if (!of_root) {
+ of_root = kzalloc(sizeof(*of_root), GFP_KERNEL);
+ if (!of_root) {
+ mutex_unlock(&of_mutex);
+ pr_err("failed to create root node\n");
+ return;
+ }
+
+ of_root->full_name = "/";
+ of_node_init(of_root);
+ }
+
for_each_of_allnodes(np) {
__of_attach_node_sysfs(np);
if (np->phandle && !phandle_cache[of_phandle_cache_hash(np->phandle)])
@@ -185,8 +198,7 @@ void __init of_core_init(void)
mutex_unlock(&of_mutex);

/* Symlink in /proc as required by userspace ABI */
- if (of_root)
- proc_symlink("device-tree", NULL, "/sys/firmware/devicetree/base");
+ proc_symlink("device-tree", NULL, "/sys/firmware/devicetree/base");
}

static struct property *__of_find_property(const struct device_node *np,
--
2.34.1


2022-05-03 14:36:43

by Rob Herring (Arm)

[permalink] [raw]
Subject: Re: [PATCH 1/3] of: always populate a root node

On Wed, Apr 27, 2022 at 11:45:00AM +0200, Cl?ment L?ger wrote:
> When enabling CONFIG_OF on a platform where of_root is not populated by
> firmware, we end up without a root node. In order to apply overlays and
> create subnodes of the root node, we need one. This commit creates an
> empty root node if not present.

The existing unittest essentially does the same thing for running the
tests on non-DT systems. It should be modified to use this support
instead. Maybe that's just removing the unittest code that set of_root.

I expect Frank will have some comments.

> Co-developed-by: Rob Herring <[email protected]>
> Signed-off-by: Rob Herring <[email protected]>
> Signed-off-by: Cl?ment L?ger <[email protected]>
> ---
> drivers/of/base.c | 16 ++++++++++++++--
> 1 file changed, 14 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/of/base.c b/drivers/of/base.c
> index e7d92b67cb8a..6b8584c39f73 100644
> --- a/drivers/of/base.c
> +++ b/drivers/of/base.c
> @@ -177,6 +177,19 @@ void __init of_core_init(void)
> pr_err("failed to register existing nodes\n");
> return;
> }
> +
> + if (!of_root) {
> + of_root = kzalloc(sizeof(*of_root), GFP_KERNEL);
> + if (!of_root) {
> + mutex_unlock(&of_mutex);
> + pr_err("failed to create root node\n");
> + return;
> + }
> +
> + of_root->full_name = "/";
> + of_node_init(of_root);
> + }
> +
> for_each_of_allnodes(np) {
> __of_attach_node_sysfs(np);
> if (np->phandle && !phandle_cache[of_phandle_cache_hash(np->phandle)])
> @@ -185,8 +198,7 @@ void __init of_core_init(void)
> mutex_unlock(&of_mutex);
>
> /* Symlink in /proc as required by userspace ABI */
> - if (of_root)
> - proc_symlink("device-tree", NULL, "/sys/firmware/devicetree/base");
> + proc_symlink("device-tree", NULL, "/sys/firmware/devicetree/base");
> }
>
> static struct property *__of_find_property(const struct device_node *np,
> --
> 2.34.1
>
>

2022-05-03 16:24:09

by Clément Léger

[permalink] [raw]
Subject: Re: [PATCH 1/3] of: always populate a root node

Le Tue, 3 May 2022 08:45:11 -0500,
Rob Herring <[email protected]> a écrit :

> On Wed, Apr 27, 2022 at 11:45:00AM +0200, Clément Léger wrote:
> > When enabling CONFIG_OF on a platform where of_root is not populated by
> > firmware, we end up without a root node. In order to apply overlays and
> > create subnodes of the root node, we need one. This commit creates an
> > empty root node if not present.
>
> The existing unittest essentially does the same thing for running the
> tests on non-DT systems. It should be modified to use this support
> instead. Maybe that's just removing the unittest code that set of_root.

Acked, I'll try the unit test on my system.

>
> I expect Frank will have some comments.
>
> > Co-developed-by: Rob Herring <[email protected]>
> > Signed-off-by: Rob Herring <[email protected]>
> > Signed-off-by: Clément Léger <[email protected]>
> > ---
> > drivers/of/base.c | 16 ++++++++++++++--
> > 1 file changed, 14 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/of/base.c b/drivers/of/base.c
> > index e7d92b67cb8a..6b8584c39f73 100644
> > --- a/drivers/of/base.c
> > +++ b/drivers/of/base.c
> > @@ -177,6 +177,19 @@ void __init of_core_init(void)
> > pr_err("failed to register existing nodes\n");
> > return;
> > }
> > +
> > + if (!of_root) {
> > + of_root = kzalloc(sizeof(*of_root), GFP_KERNEL);
> > + if (!of_root) {
> > + mutex_unlock(&of_mutex);
> > + pr_err("failed to create root node\n");
> > + return;
> > + }
> > +
> > + of_root->full_name = "/";
> > + of_node_init(of_root);
> > + }
> > +
> > for_each_of_allnodes(np) {
> > __of_attach_node_sysfs(np);
> > if (np->phandle && !phandle_cache[of_phandle_cache_hash(np->phandle)])
> > @@ -185,8 +198,7 @@ void __init of_core_init(void)
> > mutex_unlock(&of_mutex);
> >
> > /* Symlink in /proc as required by userspace ABI */
> > - if (of_root)
> > - proc_symlink("device-tree", NULL, "/sys/firmware/devicetree/base");
> > + proc_symlink("device-tree", NULL, "/sys/firmware/devicetree/base");
> > }
> >
> > static struct property *__of_find_property(const struct device_node *np,
> > --
> > 2.34.1
> >
> >



--
Clément Léger,
Embedded Linux and Kernel engineer at Bootlin
https://bootlin.com

2022-05-04 18:27:04

by Frank Rowand

[permalink] [raw]
Subject: Re: [PATCH 1/3] of: always populate a root node

On 5/3/22 08:45, Rob Herring wrote:
> On Wed, Apr 27, 2022 at 11:45:00AM +0200, Clément Léger wrote:
>> When enabling CONFIG_OF on a platform where of_root is not populated by
>> firmware, we end up without a root node. In order to apply overlays and
>> create subnodes of the root node, we need one. This commit creates an
>> empty root node if not present.
>
> The existing unittest essentially does the same thing for running the
> tests on non-DT systems. It should be modified to use this support
> instead. Maybe that's just removing the unittest code that set of_root.
>
> I expect Frank will have some comments.
>

< snip >

This patch series is next on my list, after what I am currently working
on (updating the .dts -> .dtso patch). I may get to this today, but
more likely it will be tomorrow.

-Frank

2022-05-17 06:44:29

by Frank Rowand

[permalink] [raw]
Subject: Re: [PATCH 1/3] of: always populate a root node

On 5/3/22 08:45, Rob Herring wrote:
> On Wed, Apr 27, 2022 at 11:45:00AM +0200, Clément Léger wrote:
>> When enabling CONFIG_OF on a platform where of_root is not populated by
>> firmware, we end up without a root node. In order to apply overlays and
>> create subnodes of the root node, we need one. This commit creates an
>> empty root node if not present.
>
> The existing unittest essentially does the same thing for running the
> tests on non-DT systems. It should be modified to use this support
> instead. Maybe that's just removing the unittest code that set of_root.
>
> I expect Frank will have some comments.

My preference would be for unflatten_and_copy_device_tree() to
use a compiled in FDT that only contains a root node, in the
case that no valid device tree is found (in other words,
"if (!initial_boot_params)".

unflatten_and_copy_device_tree() calls unittest_unflatten_overlay_base()
after unflattening the device tree passed into the booting kernel. This
step is needed for a specific portion of the unittests.

I'm still looking at the bigger picture of using overlays for the PCIe
card, so more comments will be coming about that bigger picture.

-Frank

>
>> Co-developed-by: Rob Herring <[email protected]>
>> Signed-off-by: Rob Herring <[email protected]>
>> Signed-off-by: Clément Léger <[email protected]>
>> ---
>> drivers/of/base.c | 16 ++++++++++++++--
>> 1 file changed, 14 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/of/base.c b/drivers/of/base.c
>> index e7d92b67cb8a..6b8584c39f73 100644
>> --- a/drivers/of/base.c
>> +++ b/drivers/of/base.c
>> @@ -177,6 +177,19 @@ void __init of_core_init(void)
>> pr_err("failed to register existing nodes\n");
>> return;
>> }
>> +
>> + if (!of_root) {
>> + of_root = kzalloc(sizeof(*of_root), GFP_KERNEL);
>> + if (!of_root) {
>> + mutex_unlock(&of_mutex);
>> + pr_err("failed to create root node\n");
>> + return;
>> + }
>> +
>> + of_root->full_name = "/";
>> + of_node_init(of_root);
>> + }
>> +
>> for_each_of_allnodes(np) {
>> __of_attach_node_sysfs(np);
>> if (np->phandle && !phandle_cache[of_phandle_cache_hash(np->phandle)])
>> @@ -185,8 +198,7 @@ void __init of_core_init(void)
>> mutex_unlock(&of_mutex);
>>
>> /* Symlink in /proc as required by userspace ABI */
>> - if (of_root)
>> - proc_symlink("device-tree", NULL, "/sys/firmware/devicetree/base");
>> + proc_symlink("device-tree", NULL, "/sys/firmware/devicetree/base");
>> }
>>
>> static struct property *__of_find_property(const struct device_node *np,
>> --
>> 2.34.1
>>
>>


2022-05-17 18:28:31

by Clément Léger

[permalink] [raw]
Subject: Re: [PATCH 1/3] of: always populate a root node

Le Mon, 16 May 2022 23:11:03 -0400,
Frank Rowand <[email protected]> a écrit :

> On 5/3/22 08:45, Rob Herring wrote:
> > On Wed, Apr 27, 2022 at 11:45:00AM +0200, Clément Léger wrote:
> >> When enabling CONFIG_OF on a platform where of_root is not populated by
> >> firmware, we end up without a root node. In order to apply overlays and
> >> create subnodes of the root node, we need one. This commit creates an
> >> empty root node if not present.
> >
> > The existing unittest essentially does the same thing for running the
> > tests on non-DT systems. It should be modified to use this support
> > instead. Maybe that's just removing the unittest code that set of_root.
> >
> > I expect Frank will have some comments.
>
> My preference would be for unflatten_and_copy_device_tree() to
> use a compiled in FDT that only contains a root node, in the
> case that no valid device tree is found (in other words,
> "if (!initial_boot_params)".

Ok, so basically, instead of creating the root node manually, you
expect a device-tree which contains the following to be builtin the
kernel and unflattened if needed:

/ {

};

Maybe "chosen" and "aliases" nodes should also be provided as empty
nodes since the unittest are creating them anyway and the core DT code
also uses them.

Thanks,

Clément

>
> unflatten_and_copy_device_tree() calls unittest_unflatten_overlay_base()
> after unflattening the device tree passed into the booting kernel. This
> step is needed for a specific portion of the unittests.
>
> I'm still looking at the bigger picture of using overlays for the PCIe
> card, so more comments will be coming about that bigger picture.
>
> -Frank
>


--
Clément Léger,
Embedded Linux and Kernel engineer at Bootlin
https://bootlin.com

2022-05-18 04:35:12

by Frank Rowand

[permalink] [raw]
Subject: Re: [PATCH 1/3] of: always populate a root node

On 5/17/22 02:37, Clément Léger wrote:
> Le Mon, 16 May 2022 23:11:03 -0400,
> Frank Rowand <[email protected]> a écrit :
>
>> On 5/3/22 08:45, Rob Herring wrote:
>>> On Wed, Apr 27, 2022 at 11:45:00AM +0200, Clément Léger wrote:
>>>> When enabling CONFIG_OF on a platform where of_root is not populated by
>>>> firmware, we end up without a root node. In order to apply overlays and
>>>> create subnodes of the root node, we need one. This commit creates an
>>>> empty root node if not present.
>>>
>>> The existing unittest essentially does the same thing for running the
>>> tests on non-DT systems. It should be modified to use this support
>>> instead. Maybe that's just removing the unittest code that set of_root.
>>>
>>> I expect Frank will have some comments.
>>
>> My preference would be for unflatten_and_copy_device_tree() to
>> use a compiled in FDT that only contains a root node, in the
>> case that no valid device tree is found (in other words,
>> "if (!initial_boot_params)".
>
> Ok, so basically, instead of creating the root node manually, you
> expect a device-tree which contains the following to be builtin the
> kernel and unflattened if needed:
>
> / {
>
> };

Yes. If you agree with this I can create a patch to implement it. I think
it is useful even stand alone from the rest of the series.

>
> Maybe "chosen" and "aliases" nodes should also be provided as empty
> nodes since the unittest are creating them anyway and the core DT code
> also uses them.

No. Unittest does not create both of them (I'm pretty sure, but I'm not
going to double check). If I recall correctly, unittest adds a property
in one of those two nodes, and thus implicitly creates the node if not
already present. Unittest does populate internal pointers to those two
nodes if the nodes are present (otherwise the pointers will have the
value of null). There is no need for the nodes to be present if empty.

-Frank

>
> Thanks,
>
> Clément
>
>>
>> unflatten_and_copy_device_tree() calls unittest_unflatten_overlay_base()
>> after unflattening the device tree passed into the booting kernel. This
>> step is needed for a specific portion of the unittests.
>>
>> I'm still looking at the bigger picture of using overlays for the PCIe
>> card, so more comments will be coming about that bigger picture.
>>
>> -Frank
>>
>
>


2022-05-18 10:24:45

by Clément Léger

[permalink] [raw]
Subject: Re: [PATCH 1/3] of: always populate a root node

Le Tue, 17 May 2022 11:03:41 -0400,
Frank Rowand <[email protected]> a écrit :

> On 5/17/22 02:37, Clément Léger wrote:
> > Le Mon, 16 May 2022 23:11:03 -0400,
> > Frank Rowand <[email protected]> a écrit :
> >
> >> On 5/3/22 08:45, Rob Herring wrote:
> >>> On Wed, Apr 27, 2022 at 11:45:00AM +0200, Clément Léger wrote:
> >>>> When enabling CONFIG_OF on a platform where of_root is not populated by
> >>>> firmware, we end up without a root node. In order to apply overlays and
> >>>> create subnodes of the root node, we need one. This commit creates an
> >>>> empty root node if not present.
> >>>
> >>> The existing unittest essentially does the same thing for running the
> >>> tests on non-DT systems. It should be modified to use this support
> >>> instead. Maybe that's just removing the unittest code that set of_root.
> >>>
> >>> I expect Frank will have some comments.
> >>
> >> My preference would be for unflatten_and_copy_device_tree() to
> >> use a compiled in FDT that only contains a root node, in the
> >> case that no valid device tree is found (in other words,
> >> "if (!initial_boot_params)".
> >
> > Ok, so basically, instead of creating the root node manually, you
> > expect a device-tree which contains the following to be builtin the
> > kernel and unflattened if needed:
> >
> > / {
> >
> > };
>
> Yes. If you agree with this I can create a patch to implement it. I think
> it is useful even stand alone from the rest of the series.

If you want to implement this, feel free to do so, I'll (at least) be
able to test it.

>
> >
> > Maybe "chosen" and "aliases" nodes should also be provided as empty
> > nodes since the unittest are creating them anyway and the core DT code
> > also uses them.
>
> No. Unittest does not create both of them (I'm pretty sure, but I'm not
> going to double check). If I recall correctly, unittest adds a property
> in one of those two nodes, and thus implicitly creates the node if not
> already present. Unittest does populate internal pointers to those two
> nodes if the nodes are present (otherwise the pointers will have the
> value of null). There is no need for the nodes to be present if empty.

Acked, makes sense.

Clément

>
> -Frank
>
> >
> > Thanks,
> >
> > Clément
> >
> >>
> >> unflatten_and_copy_device_tree() calls unittest_unflatten_overlay_base()
> >> after unflattening the device tree passed into the booting kernel. This
> >> step is needed for a specific portion of the unittests.
> >>
> >> I'm still looking at the bigger picture of using overlays for the PCIe
> >> card, so more comments will be coming about that bigger picture.
> >>
> >> -Frank
> >>
> >
> >
>



--
Clément Léger,
Embedded Linux and Kernel engineer at Bootlin
https://bootlin.com