[PATCH 0/5] Add support for resolving path references in overlays

[PATCH 0/5] Add support for resolving path references in overlays

[Ayush Singh]

dts allows references both in integer context:
	foo = <&bar>;
in which case it resolves to a phandle, but also in string/bytestring
context:
	foo = &bar;
In which case it resolves to a path.

Runtime overlays, only support the former, but not the latter. The
following patch attempts to solve this asymmetry.

Additionally, `__symbols__` does not support phandles, which
makes overlays modifying `__symbols__` rather limiting. More context
regarding this patch can be found here[0].

Implementation
**************

Overlay
=======

Properties to path references in overlays are left empty in the compiled
binary blob. Only `__fixups__` entry is generated for such properties.
This makes it simple to distinguish between phandles and paths when
applying the overlay (and also makes overlays small).

Application
===========

I have divided the overlay application into 2 stages.

1. Overlay prepare (`fdt_overlay_prepare`)

This step prepares the overlay for merging with the base device tree. In
At this stage, the base device tree is passed as read-only and only the
overlay is modified and resized.

Additionally, since any resizing will invalidate the offsets and
property references, I am creating a read-only copy of the table while
resolving the path references.

2. Overlay application (`fdt_overlay_apply`)

Performs the actual merging to base tree. The overlay is read-only at this
stage.

Limitations
***********

1. Local Path references

Currently, this patch series only implements path references to base
devicetree. This means local path references are still broken. I am
working on adding support for that using `__local_fixups__` but it is
not ready yet.

2. Breaking change for utilities

This is a breaking change for any utilities using `fdt_overlay_apply`
since I have moved some of it's functionality to `fdt_overlay_prepare`.
Not really sure how important this is.

If it is not desirable, I do have 2 ways to avoid it.

  a. Just expand both overlay and base device tree when `FDT_ERR_NOSPACE`
     is returned. A bit wasteful, but probably not a big deal.

  b. Add new error variant `FDT_ERR_OVERLAY_NOSPACE`.

Alternatives
************

Some alternative approaches that were considered:

1. Using aliases.

Currently, it is not possible to update aliases in device tree overlays.
I sent a patch a few weeks ago to add this support [1]. However, as was
outlined by Rob, this can break existing drivers that used the unused
indexes for devices not present in the aliases list.

2. Add support for phandles in `__symbols__`

This has been discussed in the following patch series [2]. However,
since there is no way to distinguish between strings and phandles in
devicetree (everything is bytestring), the type guessing is awkward.

[0]: https://lore.kernel.org/devicetree-compiler/44bfc9b3-8282-4cc7-8d9a-7292cac663ef@ti.com/T/#mf0f6ae4db0848f725ec6e2fb625291fa0d4eec71
[1]: https://lore.kernel.org/all/20241110-of-alias-v2-0-16da9844a93e@beagleboard.org/T/#t
[2]: https://lore.kernel.org/devicetree-compiler/44bfc9b3-8282-4cc7-8d9a-7292cac663ef@ti.com/T/#mbbc181b0ef394b85b76b2024d7e209ebe70f7003

Signed-off-by: Ayush Singh <ayush@beagleboard.org>

---
Ayush Singh (5):
      dtc: Allow path fixups in overlays
      libfdt: Add namelen variants for setprop
      fdtoverlay: Implement resolving path references
      tests: Fix overlay tests
      tests: Add path tests for overlay

 checks.c                                |   6 +-
 fdtoverlay.c                            |  34 +++++--
 libfdt/fdt_overlay.c                    | 174 +++++++++++++++++++-------------
 libfdt/fdt_rw.c                         |  19 ++++
 libfdt/libfdt.h                         |  93 ++++++++++++++++-
 libfdt/version.lds                      |   1 +
 livetree.c                              |  19 +++-
 tests/overlay.c                         |   1 +
 tests/overlay_bad_fixup.c               |   2 +-
 tests/overlay_overlay.dts               |  11 ++
 tests/overlay_overlay_manual_fixups.dts |  26 ++++-
 tests/overlay_overlay_nosugar.dts       |  19 ++++
 util.h                                  |   1 +
 13 files changed, 321 insertions(+), 85 deletions(-)
---
base-commit: 2d10aa2afe35527728db30b35ec491ecb6959e5c
change-id: 20241114-overlay-path-d9980477f76a

Best regards,
-- 
Ayush Singh <ayush@beagleboard.org>

[PATCH 1/5] dtc: Allow path fixups in overlays

[Ayush Singh]

Enable generating fixups entries for path references. These are the same
as the entries for phandles.

Path properties have empty value in generated overlay. This allows easy
differentiation between path references (size > 0) and path references
(size == 0).

Signed-off-by: Ayush Singh <ayush@beagleboard.org>
---
 checks.c   |  6 ++++--
 livetree.c | 19 +++++++++++++++++--
 2 files changed, 21 insertions(+), 4 deletions(-)

diff --git a/checks.c b/checks.c
index 6e06aeab5503f78c8a969f8d1d0e96be7b91749e..1ec59c6a1be375e1695b68203a0782cf23ee33fa 100644
--- a/checks.c
+++ b/checks.c
@@ -651,8 +651,10 @@ static void fixup_path_references(struct check *c, struct dt_info *dti,
 
 			refnode = get_node_by_ref(dt, m->ref);
 			if (!refnode) {
-				FAIL(c, dti, node, "Reference to non-existent node or label \"%s\"\n",
-				     m->ref);
+				if (!(dti->dtsflags & DTSF_PLUGIN))
+					FAIL(c, dti, node,
+					     "Reference to non-existent node or label \"%s\"\n",
+					     m->ref);
 				continue;
 			}
 
diff --git a/livetree.c b/livetree.c
index 49f723002f855764452b30b5a979b6096730a33b..4e26d378e09407d6a395c2f1bbd73c1874de4ae0 100644
--- a/livetree.c
+++ b/livetree.c
@@ -908,6 +908,13 @@ static bool any_fixup_tree(struct dt_info *dti, struct node *node)
 			if (!get_node_by_ref(dti->dt, m->ref))
 				return true;
 		}
+		m = prop->val.markers;
+		for_each_marker_of_type(m, REF_PATH)
+		{
+			if (m->ref) {
+				return true;
+			}
+		}
 	}
 
 	for_each_child(node, c) {
@@ -924,8 +931,8 @@ static void add_fixup_entry(struct dt_info *dti, struct node *fn,
 {
 	char *entry;
 
-	/* m->ref can only be a REF_PHANDLE, but check anyway */
-	assert(m->type == REF_PHANDLE);
+	/* m->ref can only be a REF_PHANDLE or REF_PATH, but check anyway */
+	assert(m->type == REF_PHANDLE || m->type == REF_PATH);
 
 	/* The format only permits fixups for references to label, not
 	 * references to path */
@@ -961,6 +968,14 @@ static void generate_fixups_tree_internal(struct dt_info *dti,
 			if (!refnode)
 				add_fixup_entry(dti, fn, node, prop, m);
 		}
+
+		m = prop->val.markers;
+		for_each_marker_of_type(m, REF_PATH)
+		{
+			refnode = get_node_by_ref(dt, m->ref);
+			if (!refnode)
+				add_fixup_entry(dti, fn, node, prop, m);
+		}
 	}
 
 	for_each_child(node, c)

-- 
2.47.0

Re: [PATCH 1/5] dtc: Allow path fixups in overlays

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 3127 bytes --]

On Sat, Nov 16, 2024 at 08:30:19PM +0530, Ayush Singh wrote:
> Enable generating fixups entries for path references. These are the same
> as the entries for phandles.
> 
> Path properties have empty value in generated overlay. This allows easy
> differentiation between path references (size > 0) and path references
> (size == 0).
> 
> Signed-off-by: Ayush Singh <ayush@beagleboard.org>
> ---
>  checks.c   |  6 ++++--
>  livetree.c | 19 +++++++++++++++++--
>  2 files changed, 21 insertions(+), 4 deletions(-)
> 
> diff --git a/checks.c b/checks.c
> index 6e06aeab5503f78c8a969f8d1d0e96be7b91749e..1ec59c6a1be375e1695b68203a0782cf23ee33fa 100644
> --- a/checks.c
> +++ b/checks.c
> @@ -651,8 +651,10 @@ static void fixup_path_references(struct check *c, struct dt_info *dti,
>  
>  			refnode = get_node_by_ref(dt, m->ref);
>  			if (!refnode) {
> -				FAIL(c, dti, node, "Reference to non-existent node or label \"%s\"\n",
> -				     m->ref);
> +				if (!(dti->dtsflags & DTSF_PLUGIN))
> +					FAIL(c, dti, node,
> +					     "Reference to non-existent node or label \"%s\"\n",
> +					     m->ref);
>  				continue;
>  			}
>  
> diff --git a/livetree.c b/livetree.c
> index 49f723002f855764452b30b5a979b6096730a33b..4e26d378e09407d6a395c2f1bbd73c1874de4ae0 100644
> --- a/livetree.c
> +++ b/livetree.c
> @@ -908,6 +908,13 @@ static bool any_fixup_tree(struct dt_info *dti, struct node *node)
>  			if (!get_node_by_ref(dti->dt, m->ref))
>  				return true;
>  		}
> +		m = prop->val.markers;
> +		for_each_marker_of_type(m, REF_PATH)
> +		{

Opening brace goes on the same line as the for_each()

> +			if (m->ref) {


As for phandle references, you don't need to emit a fixup if the
reference can be resolved within the current tree, so you want a
get_node_by_ref() before returning true.

> +				return true;
> +			}

No braces around single statements in dtc style.

> +		}
>  	}
>  
>  	for_each_child(node, c) {
> @@ -924,8 +931,8 @@ static void add_fixup_entry(struct dt_info *dti, struct node *fn,
>  {
>  	char *entry;
>  
> -	/* m->ref can only be a REF_PHANDLE, but check anyway */
> -	assert(m->type == REF_PHANDLE);
> +	/* m->ref can only be a REF_PHANDLE or REF_PATH, but check anyway */
> +	assert(m->type == REF_PHANDLE || m->type == REF_PATH);
>  
>  	/* The format only permits fixups for references to label, not
>  	 * references to path */
> @@ -961,6 +968,14 @@ static void generate_fixups_tree_internal(struct dt_info *dti,
>  			if (!refnode)
>  				add_fixup_entry(dti, fn, node, prop, m);
>  		}
> +
> +		m = prop->val.markers;
> +		for_each_marker_of_type(m, REF_PATH)
> +		{
> +			refnode = get_node_by_ref(dt, m->ref);
> +			if (!refnode)
> +				add_fixup_entry(dti, fn, node, prop, m);
> +		}

I don't see how phandle fixups are distinguished from path fixups.

>  	}
>  
>  	for_each_child(node, c)
> 

-- 
David Gibson (he or they)	| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you, not the other way
				| around.
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 1/5] dtc: Allow path fixups in overlays

[Ayush Singh]

On 03/12/24 09:47, David Gibson wrote:
> On Sat, Nov 16, 2024 at 08:30:19PM +0530, Ayush Singh wrote:
>> Enable generating fixups entries for path references. These are the same
>> as the entries for phandles.
>>
>> Path properties have empty value in generated overlay. This allows easy
>> differentiation between path references (size > 0) and path references
>> (size == 0).
>>
>> Signed-off-by: Ayush Singh <ayush@beagleboard.org>
>> ---
>>   checks.c   |  6 ++++--
>>   livetree.c | 19 +++++++++++++++++--
>>   2 files changed, 21 insertions(+), 4 deletions(-)
>>
>> diff --git a/checks.c b/checks.c
>> index 6e06aeab5503f78c8a969f8d1d0e96be7b91749e..1ec59c6a1be375e1695b68203a0782cf23ee33fa 100644
>> --- a/checks.c
>> +++ b/checks.c
>> @@ -651,8 +651,10 @@ static void fixup_path_references(struct check *c, struct dt_info *dti,
>>   
>>   			refnode = get_node_by_ref(dt, m->ref);
>>   			if (!refnode) {
>> -				FAIL(c, dti, node, "Reference to non-existent node or label \"%s\"\n",
>> -				     m->ref);
>> +				if (!(dti->dtsflags & DTSF_PLUGIN))
>> +					FAIL(c, dti, node,
>> +					     "Reference to non-existent node or label \"%s\"\n",
>> +					     m->ref);
>>   				continue;
>>   			}
>>   
>> diff --git a/livetree.c b/livetree.c
>> index 49f723002f855764452b30b5a979b6096730a33b..4e26d378e09407d6a395c2f1bbd73c1874de4ae0 100644
>> --- a/livetree.c
>> +++ b/livetree.c
>> @@ -908,6 +908,13 @@ static bool any_fixup_tree(struct dt_info *dti, struct node *node)
>>   			if (!get_node_by_ref(dti->dt, m->ref))
>>   				return true;
>>   		}
>> +		m = prop->val.markers;
>> +		for_each_marker_of_type(m, REF_PATH)
>> +		{
> 
> Opening brace goes on the same line as the for_each()
> 
>> +			if (m->ref) {
> 
> 
> As for phandle references, you don't need to emit a fixup if the
> reference can be resolved within the current tree, so you want a
> get_node_by_ref() before returning true.
> 
>> +				return true;
>> +			}
> 
> No braces around single statements in dtc style.

Is there any clang-format config or something for dtc? I am currently 
using the format config from Linux kernel.

> 
>> +		}
>>   	}
>>   
>>   	for_each_child(node, c) {
>> @@ -924,8 +931,8 @@ static void add_fixup_entry(struct dt_info *dti, struct node *fn,
>>   {
>>   	char *entry;
>>   
>> -	/* m->ref can only be a REF_PHANDLE, but check anyway */
>> -	assert(m->type == REF_PHANDLE);
>> +	/* m->ref can only be a REF_PHANDLE or REF_PATH, but check anyway */
>> +	assert(m->type == REF_PHANDLE || m->type == REF_PATH);
>>   
>>   	/* The format only permits fixups for references to label, not
>>   	 * references to path */
>> @@ -961,6 +968,14 @@ static void generate_fixups_tree_internal(struct dt_info *dti,
>>   			if (!refnode)
>>   				add_fixup_entry(dti, fn, node, prop, m);
>>   		}
>> +
>> +		m = prop->val.markers;
>> +		for_each_marker_of_type(m, REF_PATH)
>> +		{
>> +			refnode = get_node_by_ref(dt, m->ref);
>> +			if (!refnode)
>> +				add_fixup_entry(dti, fn, node, prop, m);
>> +		}
> 
> I don't see how phandle fixups are distinguished from path fixups.
> 
>>   	}
>>   
>>   	for_each_child(node, c)
>>
> 

Ayush Singh

Re: [PATCH 1/5] dtc: Allow path fixups in overlays

[Geert Uytterhoeven]

Hi Ayush,

On Tue, Dec 3, 2024 at 8:29 AM Ayush Singh <ayush@beagleboard.org> wrote:
> On 03/12/24 09:47, David Gibson wrote:
> > On Sat, Nov 16, 2024 at 08:30:19PM +0530, Ayush Singh wrote:
> >> --- a/livetree.c
> >> +++ b/livetree.c
> >> @@ -908,6 +908,13 @@ static bool any_fixup_tree(struct dt_info *dti, struct node *node)
> >>                      if (!get_node_by_ref(dti->dt, m->ref))
> >>                              return true;
> >>              }
> >> +            m = prop->val.markers;
> >> +            for_each_marker_of_type(m, REF_PATH)
> >> +            {
> >
> > Opening brace goes on the same line as the for_each()
> >
> >> +                    if (m->ref) {
> >
> >
> > As for phandle references, you don't need to emit a fixup if the
> > reference can be resolved within the current tree, so you want a
> > get_node_by_ref() before returning true.
> >
> >> +                            return true;
> >> +                    }
> >
> > No braces around single statements in dtc style.
>
> Is there any clang-format config or something for dtc? I am currently
> using the format config from Linux kernel.

The above are the same rules as the Linux kernel style rules?

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH 1/5] dtc: Allow path fixups in overlays

[Ayush Singh]

On 03/12/24 13:44, Geert Uytterhoeven wrote:
> Hi Ayush,
> 
> On Tue, Dec 3, 2024 at 8:29 AM Ayush Singh <ayush@beagleboard.org> wrote:
>> On 03/12/24 09:47, David Gibson wrote:
>>> On Sat, Nov 16, 2024 at 08:30:19PM +0530, Ayush Singh wrote:
>>>> --- a/livetree.c
>>>> +++ b/livetree.c
>>>> @@ -908,6 +908,13 @@ static bool any_fixup_tree(struct dt_info *dti, struct node *node)
>>>>                       if (!get_node_by_ref(dti->dt, m->ref))
>>>>                               return true;
>>>>               }
>>>> +            m = prop->val.markers;
>>>> +            for_each_marker_of_type(m, REF_PATH)
>>>> +            {
>>>
>>> Opening brace goes on the same line as the for_each()
>>>
>>>> +                    if (m->ref) {
>>>
>>>
>>> As for phandle references, you don't need to emit a fixup if the
>>> reference can be resolved within the current tree, so you want a
>>> get_node_by_ref() before returning true.
>>>
>>>> +                            return true;
>>>> +                    }
>>>
>>> No braces around single statements in dtc style.
>>
>> Is there any clang-format config or something for dtc? I am currently
>> using the format config from Linux kernel.
> 
> The above are the same rules as the Linux kernel style rules?
> 
> Gr{oetje,eeting}s,
> 
>                          Geert
> 

My bad, I should have put the response on some other line. I was just 
asking if it would be fine to use configs from linux kernel from my next 
patch series since dtc does not seem to have one in it's tree.

I was not using any format and checkpatch config right now.

Ayush Singh

Re: [PATCH 1/5] dtc: Allow path fixups in overlays

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 2181 bytes --]

On Tue, Dec 03, 2024 at 02:14:46PM +0530, Ayush Singh wrote:
> On 03/12/24 13:44, Geert Uytterhoeven wrote:
> > Hi Ayush,
> > 
> > On Tue, Dec 3, 2024 at 8:29 AM Ayush Singh <ayush@beagleboard.org> wrote:
> > > On 03/12/24 09:47, David Gibson wrote:
> > > > On Sat, Nov 16, 2024 at 08:30:19PM +0530, Ayush Singh wrote:
> > > > > --- a/livetree.c
> > > > > +++ b/livetree.c
> > > > > @@ -908,6 +908,13 @@ static bool any_fixup_tree(struct dt_info *dti, struct node *node)
> > > > >                       if (!get_node_by_ref(dti->dt, m->ref))
> > > > >                               return true;
> > > > >               }
> > > > > +            m = prop->val.markers;
> > > > > +            for_each_marker_of_type(m, REF_PATH)
> > > > > +            {
> > > > 
> > > > Opening brace goes on the same line as the for_each()
> > > > 
> > > > > +                    if (m->ref) {
> > > > 
> > > > 
> > > > As for phandle references, you don't need to emit a fixup if the
> > > > reference can be resolved within the current tree, so you want a
> > > > get_node_by_ref() before returning true.
> > > > 
> > > > > +                            return true;
> > > > > +                    }
> > > > 
> > > > No braces around single statements in dtc style.
> > > 
> > > Is there any clang-format config or something for dtc? I am currently
> > > using the format config from Linux kernel.
> > 
> > The above are the same rules as the Linux kernel style rules?
> > 
> > Gr{oetje,eeting}s,
> > 
> >                          Geert
> > 
> 
> My bad, I should have put the response on some other line. I was just asking
> if it would be fine to use configs from linux kernel from my next patch
> series since dtc does not seem to have one in it's tree.
> 
> I was not using any format and checkpatch config right now.

I'd be happy to look at a patch adding clang format config to the dtc
tree (you can use the kernel one as a starting point).

-- 
David Gibson (he or they)	| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you, not the other way
				| around.
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 1/5] dtc: Allow path fixups in overlays

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 3791 bytes --]

On Tue, Dec 03, 2024 at 12:59:28PM +0530, Ayush Singh wrote:
> On 03/12/24 09:47, David Gibson wrote:
> > On Sat, Nov 16, 2024 at 08:30:19PM +0530, Ayush Singh wrote:
> > > Enable generating fixups entries for path references. These are the same
> > > as the entries for phandles.
> > > 
> > > Path properties have empty value in generated overlay. This allows easy
> > > differentiation between path references (size > 0) and path references
> > > (size == 0).
> > > 
> > > Signed-off-by: Ayush Singh <ayush@beagleboard.org>
> > > ---
> > >   checks.c   |  6 ++++--
> > >   livetree.c | 19 +++++++++++++++++--
> > >   2 files changed, 21 insertions(+), 4 deletions(-)
> > > 
> > > diff --git a/checks.c b/checks.c
> > > index 6e06aeab5503f78c8a969f8d1d0e96be7b91749e..1ec59c6a1be375e1695b68203a0782cf23ee33fa 100644
> > > --- a/checks.c
> > > +++ b/checks.c
> > > @@ -651,8 +651,10 @@ static void fixup_path_references(struct check *c, struct dt_info *dti,
> > >   			refnode = get_node_by_ref(dt, m->ref);
> > >   			if (!refnode) {
> > > -				FAIL(c, dti, node, "Reference to non-existent node or label \"%s\"\n",
> > > -				     m->ref);
> > > +				if (!(dti->dtsflags & DTSF_PLUGIN))
> > > +					FAIL(c, dti, node,
> > > +					     "Reference to non-existent node or label \"%s\"\n",
> > > +					     m->ref);
> > >   				continue;
> > >   			}
> > > diff --git a/livetree.c b/livetree.c
> > > index 49f723002f855764452b30b5a979b6096730a33b..4e26d378e09407d6a395c2f1bbd73c1874de4ae0 100644
> > > --- a/livetree.c
> > > +++ b/livetree.c
> > > @@ -908,6 +908,13 @@ static bool any_fixup_tree(struct dt_info *dti, struct node *node)
> > >   			if (!get_node_by_ref(dti->dt, m->ref))
> > >   				return true;
> > >   		}
> > > +		m = prop->val.markers;
> > > +		for_each_marker_of_type(m, REF_PATH)
> > > +		{
> > 
> > Opening brace goes on the same line as the for_each()
> > 
> > > +			if (m->ref) {
> > 
> > 
> > As for phandle references, you don't need to emit a fixup if the
> > reference can be resolved within the current tree, so you want a
> > get_node_by_ref() before returning true.
> > 
> > > +				return true;
> > > +			}
> > 
> > No braces around single statements in dtc style.
> 
> Is there any clang-format config or something for dtc? I am currently using
> the format config from Linux kernel.

No, sorry.  Using the one from the kernel should be very close.

> 
> > 
> > > +		}
> > >   	}
> > >   	for_each_child(node, c) {
> > > @@ -924,8 +931,8 @@ static void add_fixup_entry(struct dt_info *dti, struct node *fn,
> > >   {
> > >   	char *entry;
> > > -	/* m->ref can only be a REF_PHANDLE, but check anyway */
> > > -	assert(m->type == REF_PHANDLE);
> > > +	/* m->ref can only be a REF_PHANDLE or REF_PATH, but check anyway */
> > > +	assert(m->type == REF_PHANDLE || m->type == REF_PATH);
> > >   	/* The format only permits fixups for references to label, not
> > >   	 * references to path */
> > > @@ -961,6 +968,14 @@ static void generate_fixups_tree_internal(struct dt_info *dti,
> > >   			if (!refnode)
> > >   				add_fixup_entry(dti, fn, node, prop, m);
> > >   		}
> > > +
> > > +		m = prop->val.markers;
> > > +		for_each_marker_of_type(m, REF_PATH)
> > > +		{
> > > +			refnode = get_node_by_ref(dt, m->ref);
> > > +			if (!refnode)
> > > +				add_fixup_entry(dti, fn, node, prop, m);
> > > +		}
> > 
> > I don't see how phandle fixups are distinguished from path fixups.
> > 
> > >   	}
> > >   	for_each_child(node, c)
> > > 
> > 
> 
> Ayush Singh
> 
> 

-- 
David Gibson (he or they)	| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you, not the other way
				| around.
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH 2/5] libfdt: Add namelen variants for setprop

[Ayush Singh]

Helper functions to setproperty with length of property name similar to
getprop_namelen variants.

Signed-off-by: Ayush Singh <ayush@beagleboard.org>
---
 libfdt/fdt_rw.c | 19 +++++++++++++++++
 libfdt/libfdt.h | 63 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 util.h          |  1 +
 3 files changed, 83 insertions(+)

diff --git a/libfdt/fdt_rw.c b/libfdt/fdt_rw.c
index 3621d3651d3f4bd82b7af66c60d023e3139add03..9e66c2332bf4d3868c1388c294a195b152b6aefd 100644
--- a/libfdt/fdt_rw.c
+++ b/libfdt/fdt_rw.c
@@ -7,6 +7,8 @@
 
 #include <fdt.h>
 #include <libfdt.h>
+#include "../util.h"
+#include <assert.h>
 
 #include "libfdt_internal.h"
 
@@ -288,6 +290,23 @@ int fdt_setprop(void *fdt, int nodeoffset, const char *name,
 	return 0;
 }
 
+int fdt_setprop_namelen(void *fdt, int nodeoffset, const char *name,
+			size_t namelen, const void *val, int len)
+{
+	int ret;
+	char *name_temp = xmalloc(namelen + 1);
+
+	assert(namelen <= strlen(name));
+
+	memcpy(name_temp, name, namelen);
+	name_temp[namelen] = '\0';
+	ret = fdt_setprop(fdt, nodeoffset, name_temp, val, len);
+
+	free(name_temp);
+
+	return ret;
+}
+
 int fdt_appendprop(void *fdt, int nodeoffset, const char *name,
 		   const void *val, int len)
 {
diff --git a/libfdt/libfdt.h b/libfdt/libfdt.h
index 96782bc57b8412d73dff92d6e0ad494ec0a23909..999b3800dff5c001b9c1babf370d45024c81a9aa 100644
--- a/libfdt/libfdt.h
+++ b/libfdt/libfdt.h
@@ -1690,6 +1690,38 @@ int fdt_set_name(void *fdt, int nodeoffset, const char *name);
 int fdt_setprop(void *fdt, int nodeoffset, const char *name,
 		const void *val, int len);
 
+/**
+ * fdt_setprop_namelen - create or change a property
+ * @fdt: pointer to the device tree blob
+ * @nodeoffset: offset of the node whose property to change
+ * @name: name of the property to change
+ * @namelen: length of the name
+ * @val: pointer to data to set the property value to
+ * @len: length of the property value
+ *
+ * fdt_setprop_namelen() sets the value of the named property in the given
+ * node to the given value and length, creating the property if it
+ * does not already exist.
+ *
+ * This function may insert or delete data from the blob, and will
+ * therefore change the offsets of some existing nodes.
+ *
+ * returns:
+ *	0, on success
+ *	-FDT_ERR_NOSPACE, there is insufficient free space in the blob to
+ *		contain the new property value
+ *	-FDT_ERR_BADOFFSET, nodeoffset did not point to FDT_BEGIN_NODE tag
+ *	-FDT_ERR_BADLAYOUT,
+ *	-FDT_ERR_BADMAGIC,
+ *	-FDT_ERR_BADVERSION,
+ *	-FDT_ERR_BADSTATE,
+ *	-FDT_ERR_BADSTRUCTURE,
+ *	-FDT_ERR_BADLAYOUT,
+ *	-FDT_ERR_TRUNCATED, standard meanings
+ */
+int fdt_setprop_namelen(void *fdt, int nodeoffset, const char *name,
+			size_t namelen, const void *val, int len);
+
 /**
  * fdt_setprop_placeholder - allocate space for a property
  * @fdt: pointer to the device tree blob
@@ -1839,6 +1871,37 @@ static inline int fdt_setprop_cell(void *fdt, int nodeoffset, const char *name,
 #define fdt_setprop_string(fdt, nodeoffset, name, str) \
 	fdt_setprop((fdt), (nodeoffset), (name), (str), strlen(str)+1)
 
+/**
+ * fdt_setprop_namelen_string - set a property to a string value
+ * @fdt: pointer to the device tree blob
+ * @nodeoffset: offset of the node whose property to change
+ * @name: name of the property to change
+ * @str: string value for the property
+ *
+ * fdt_setprop_namelen_string() sets the value of the named property in the
+ * given node to the given string value (using the length of the
+ * string to determine the new length of the property), or creates a
+ * new property with that value if it does not already exist.
+ *
+ * This function may insert or delete data from the blob, and will
+ * therefore change the offsets of some existing nodes.
+ *
+ * returns:
+ *	0, on success
+ *	-FDT_ERR_NOSPACE, there is insufficient free space in the blob to
+ *		contain the new property value
+ *	-FDT_ERR_BADOFFSET, nodeoffset did not point to FDT_BEGIN_NODE tag
+ *	-FDT_ERR_BADLAYOUT,
+ *	-FDT_ERR_BADMAGIC,
+ *	-FDT_ERR_BADVERSION,
+ *	-FDT_ERR_BADSTATE,
+ *	-FDT_ERR_BADSTRUCTURE,
+ *	-FDT_ERR_BADLAYOUT,
+ *	-FDT_ERR_TRUNCATED, standard meanings
+ */
+#define fdt_setprop_namelen_string(fdt, nodeoffset, name, namelen, str)    \
+	fdt_setprop_namelen((fdt), (nodeoffset), (name), (namelen), (str), \
+			    strlen(str) + 1)
 
 /**
  * fdt_setprop_empty - set a property to an empty value
diff --git a/util.h b/util.h
index 800f2e2c55b150d3c30101d1e1f1daaa0e4e3264..3e2ebaabe1e8c7e9ad6821ac2ca17a72e7f4f57d 100644
--- a/util.h
+++ b/util.h
@@ -6,6 +6,7 @@
 #include <stdarg.h>
 #include <stdbool.h>
 #include <getopt.h>
+#include <stdio.h>
 
 /*
  * Copyright 2011 The Chromium Authors, All Rights Reserved.

-- 
2.47.0

Re: [PATCH 2/5] libfdt: Add namelen variants for setprop

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 6525 bytes --]

Sorry I've taken so long to reply.

On Sat, Nov 16, 2024 at 08:30:20PM +0530, Ayush Singh wrote:
> Helper functions to setproperty with length of property name similar to
> getprop_namelen variants.

The idea of this patch is good, independent of the rest of the series.
I'm kind of surprised we don't already have a setprop_namelen()
actually.

Unfortunately, this implementation isn't usable as is.

> 
> Signed-off-by: Ayush Singh <ayush@beagleboard.org>
> ---
>  libfdt/fdt_rw.c | 19 +++++++++++++++++
>  libfdt/libfdt.h | 63 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  util.h          |  1 +
>  3 files changed, 83 insertions(+)
> 
> diff --git a/libfdt/fdt_rw.c b/libfdt/fdt_rw.c
> index 3621d3651d3f4bd82b7af66c60d023e3139add03..9e66c2332bf4d3868c1388c294a195b152b6aefd 100644
> --- a/libfdt/fdt_rw.c
> +++ b/libfdt/fdt_rw.c
> @@ -7,6 +7,8 @@
>  
>  #include <fdt.h>
>  #include <libfdt.h>
> +#include "../util.h"
> +#include <assert.h>

libfdt is designed to compile and work in limited environments (early
stage bootloaders, for example).  Therefore it's very limited in what
libc functions it's permitted to use: basically it can only use the
things listed in libfdt_env.h.

assert() is not ok, malloc() is right out.

>  #include "libfdt_internal.h"
>  
> @@ -288,6 +290,23 @@ int fdt_setprop(void *fdt, int nodeoffset, const char *name,
>  	return 0;
>  }
>  
> +int fdt_setprop_namelen(void *fdt, int nodeoffset, const char *name,
> +			size_t namelen, const void *val, int len)
> +{
> +	int ret;
> +	char *name_temp = xmalloc(namelen + 1);

As noted, we don't use an allocator in libfdt.  In fact that's a large
part of the reason the _namlen() functions exist in the first place.
To add this, you'll need to start with an fdt_get_property_namelen_w()
in terms of fdt_get_property_namelen(), then make
fdt_resize_property_() and fdt_add_property_() take the name length,
then finally fdt_setprop_placeholder_namelen() and
fdt_setprop_namelen().  The current fdt_setprop_placeholder() and
fdt_setprop() can then become trivial wrappers around the namelen
versions (like fdt_getprop() wraps fdt_getprop_namelen()).

> +
> +	assert(namelen <= strlen(name));
> +
> +	memcpy(name_temp, name, namelen);
> +	name_temp[namelen] = '\0';
> +	ret = fdt_setprop(fdt, nodeoffset, name_temp, val, len);
> +
> +	free(name_temp);
> +
> +	return ret;
> +}
> +
>  int fdt_appendprop(void *fdt, int nodeoffset, const char *name,
>  		   const void *val, int len)
>  {
> diff --git a/libfdt/libfdt.h b/libfdt/libfdt.h
> index 96782bc57b8412d73dff92d6e0ad494ec0a23909..999b3800dff5c001b9c1babf370d45024c81a9aa 100644
> --- a/libfdt/libfdt.h
> +++ b/libfdt/libfdt.h
> @@ -1690,6 +1690,38 @@ int fdt_set_name(void *fdt, int nodeoffset, const char *name);
>  int fdt_setprop(void *fdt, int nodeoffset, const char *name,
>  		const void *val, int len);
>  
> +/**
> + * fdt_setprop_namelen - create or change a property
> + * @fdt: pointer to the device tree blob
> + * @nodeoffset: offset of the node whose property to change
> + * @name: name of the property to change
> + * @namelen: length of the name
> + * @val: pointer to data to set the property value to
> + * @len: length of the property value
> + *
> + * fdt_setprop_namelen() sets the value of the named property in the given
> + * node to the given value and length, creating the property if it
> + * does not already exist.
> + *
> + * This function may insert or delete data from the blob, and will
> + * therefore change the offsets of some existing nodes.
> + *
> + * returns:
> + *	0, on success
> + *	-FDT_ERR_NOSPACE, there is insufficient free space in the blob to
> + *		contain the new property value
> + *	-FDT_ERR_BADOFFSET, nodeoffset did not point to FDT_BEGIN_NODE tag
> + *	-FDT_ERR_BADLAYOUT,
> + *	-FDT_ERR_BADMAGIC,
> + *	-FDT_ERR_BADVERSION,
> + *	-FDT_ERR_BADSTATE,
> + *	-FDT_ERR_BADSTRUCTURE,
> + *	-FDT_ERR_BADLAYOUT,
> + *	-FDT_ERR_TRUNCATED, standard meanings
> + */
> +int fdt_setprop_namelen(void *fdt, int nodeoffset, const char *name,
> +			size_t namelen, const void *val, int len);
> +
>  /**
>   * fdt_setprop_placeholder - allocate space for a property
>   * @fdt: pointer to the device tree blob
> @@ -1839,6 +1871,37 @@ static inline int fdt_setprop_cell(void *fdt, int nodeoffset, const char *name,
>  #define fdt_setprop_string(fdt, nodeoffset, name, str) \
>  	fdt_setprop((fdt), (nodeoffset), (name), (str), strlen(str)+1)
>  
> +/**
> + * fdt_setprop_namelen_string - set a property to a string value
> + * @fdt: pointer to the device tree blob
> + * @nodeoffset: offset of the node whose property to change
> + * @name: name of the property to change
> + * @str: string value for the property
> + *
> + * fdt_setprop_namelen_string() sets the value of the named property in the
> + * given node to the given string value (using the length of the
> + * string to determine the new length of the property), or creates a
> + * new property with that value if it does not already exist.
> + *
> + * This function may insert or delete data from the blob, and will
> + * therefore change the offsets of some existing nodes.
> + *
> + * returns:
> + *	0, on success
> + *	-FDT_ERR_NOSPACE, there is insufficient free space in the blob to
> + *		contain the new property value
> + *	-FDT_ERR_BADOFFSET, nodeoffset did not point to FDT_BEGIN_NODE tag
> + *	-FDT_ERR_BADLAYOUT,
> + *	-FDT_ERR_BADMAGIC,
> + *	-FDT_ERR_BADVERSION,
> + *	-FDT_ERR_BADSTATE,
> + *	-FDT_ERR_BADSTRUCTURE,
> + *	-FDT_ERR_BADLAYOUT,
> + *	-FDT_ERR_TRUNCATED, standard meanings
> + */
> +#define fdt_setprop_namelen_string(fdt, nodeoffset, name, namelen, str)    \
> +	fdt_setprop_namelen((fdt), (nodeoffset), (name), (namelen), (str), \
> +			    strlen(str) + 1)
>  
>  /**
>   * fdt_setprop_empty - set a property to an empty value
> diff --git a/util.h b/util.h
> index 800f2e2c55b150d3c30101d1e1f1daaa0e4e3264..3e2ebaabe1e8c7e9ad6821ac2ca17a72e7f4f57d 100644
> --- a/util.h
> +++ b/util.h
> @@ -6,6 +6,7 @@
>  #include <stdarg.h>
>  #include <stdbool.h>
>  #include <getopt.h>
> +#include <stdio.h>
>  
>  /*
>   * Copyright 2011 The Chromium Authors, All Rights Reserved.
> 

-- 
David Gibson (he or they)	| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you, not the other way
				| around.
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 2/5] libfdt: Add namelen variants for setprop

[Ayush Singh]

On 03/12/24 09:42, David Gibson wrote:
> Sorry I've taken so long to reply.
> 
> On Sat, Nov 16, 2024 at 08:30:20PM +0530, Ayush Singh wrote:
>> Helper functions to setproperty with length of property name similar to
>> getprop_namelen variants.
> 
> The idea of this patch is good, independent of the rest of the series.
> I'm kind of surprised we don't already have a setprop_namelen()
> actually.
> 
> Unfortunately, this implementation isn't usable as is.

Ok, I will fix things and send this as a separate patch series. I will 
need to make a lot of changes in the rest of the patch series anyway, so 
it should work out well.

> 
>>
>> Signed-off-by: Ayush Singh <ayush@beagleboard.org>
>> ---
>>   libfdt/fdt_rw.c | 19 +++++++++++++++++
>>   libfdt/libfdt.h | 63 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>   util.h          |  1 +
>>   3 files changed, 83 insertions(+)
>>
>> diff --git a/libfdt/fdt_rw.c b/libfdt/fdt_rw.c
>> index 3621d3651d3f4bd82b7af66c60d023e3139add03..9e66c2332bf4d3868c1388c294a195b152b6aefd 100644
>> --- a/libfdt/fdt_rw.c
>> +++ b/libfdt/fdt_rw.c
>> @@ -7,6 +7,8 @@
>>   
>>   #include <fdt.h>
>>   #include <libfdt.h>
>> +#include "../util.h"
>> +#include <assert.h>
> 
> libfdt is designed to compile and work in limited environments (early
> stage bootloaders, for example).  Therefore it's very limited in what
> libc functions it's permitted to use: basically it can only use the
> things listed in libfdt_env.h.
> 
> assert() is not ok, malloc() is right out.
> 
>>   #include "libfdt_internal.h"
>>   
>> @@ -288,6 +290,23 @@ int fdt_setprop(void *fdt, int nodeoffset, const char *name,
>>   	return 0;
>>   }
>>   
>> +int fdt_setprop_namelen(void *fdt, int nodeoffset, const char *name,
>> +			size_t namelen, const void *val, int len)
>> +{
>> +	int ret;
>> +	char *name_temp = xmalloc(namelen + 1);
> 
> As noted, we don't use an allocator in libfdt.  In fact that's a large
> part of the reason the _namlen() functions exist in the first place.
> To add this, you'll need to start with an fdt_get_property_namelen_w()
> in terms of fdt_get_property_namelen(), then make
> fdt_resize_property_() and fdt_add_property_() take the name length,
> then finally fdt_setprop_placeholder_namelen() and
> fdt_setprop_namelen().  The current fdt_setprop_placeholder() and
> fdt_setprop() can then become trivial wrappers around the namelen
> versions (like fdt_getprop() wraps fdt_getprop_namelen()).
> 
>> +
>> +	assert(namelen <= strlen(name));
>> +
>> +	memcpy(name_temp, name, namelen);
>> +	name_temp[namelen] = '\0';
>> +	ret = fdt_setprop(fdt, nodeoffset, name_temp, val, len);
>> +
>> +	free(name_temp);
>> +
>> +	return ret;
>> +}
>> +
>>   int fdt_appendprop(void *fdt, int nodeoffset, const char *name,
>>   		   const void *val, int len)
>>   {
>> diff --git a/libfdt/libfdt.h b/libfdt/libfdt.h
>> index 96782bc57b8412d73dff92d6e0ad494ec0a23909..999b3800dff5c001b9c1babf370d45024c81a9aa 100644
>> --- a/libfdt/libfdt.h
>> +++ b/libfdt/libfdt.h
>> @@ -1690,6 +1690,38 @@ int fdt_set_name(void *fdt, int nodeoffset, const char *name);
>>   int fdt_setprop(void *fdt, int nodeoffset, const char *name,
>>   		const void *val, int len);
>>   
>> +/**
>> + * fdt_setprop_namelen - create or change a property
>> + * @fdt: pointer to the device tree blob
>> + * @nodeoffset: offset of the node whose property to change
>> + * @name: name of the property to change
>> + * @namelen: length of the name
>> + * @val: pointer to data to set the property value to
>> + * @len: length of the property value
>> + *
>> + * fdt_setprop_namelen() sets the value of the named property in the given
>> + * node to the given value and length, creating the property if it
>> + * does not already exist.
>> + *
>> + * This function may insert or delete data from the blob, and will
>> + * therefore change the offsets of some existing nodes.
>> + *
>> + * returns:
>> + *	0, on success
>> + *	-FDT_ERR_NOSPACE, there is insufficient free space in the blob to
>> + *		contain the new property value
>> + *	-FDT_ERR_BADOFFSET, nodeoffset did not point to FDT_BEGIN_NODE tag
>> + *	-FDT_ERR_BADLAYOUT,
>> + *	-FDT_ERR_BADMAGIC,
>> + *	-FDT_ERR_BADVERSION,
>> + *	-FDT_ERR_BADSTATE,
>> + *	-FDT_ERR_BADSTRUCTURE,
>> + *	-FDT_ERR_BADLAYOUT,
>> + *	-FDT_ERR_TRUNCATED, standard meanings
>> + */
>> +int fdt_setprop_namelen(void *fdt, int nodeoffset, const char *name,
>> +			size_t namelen, const void *val, int len);
>> +
>>   /**
>>    * fdt_setprop_placeholder - allocate space for a property
>>    * @fdt: pointer to the device tree blob
>> @@ -1839,6 +1871,37 @@ static inline int fdt_setprop_cell(void *fdt, int nodeoffset, const char *name,
>>   #define fdt_setprop_string(fdt, nodeoffset, name, str) \
>>   	fdt_setprop((fdt), (nodeoffset), (name), (str), strlen(str)+1)
>>   
>> +/**
>> + * fdt_setprop_namelen_string - set a property to a string value
>> + * @fdt: pointer to the device tree blob
>> + * @nodeoffset: offset of the node whose property to change
>> + * @name: name of the property to change
>> + * @str: string value for the property
>> + *
>> + * fdt_setprop_namelen_string() sets the value of the named property in the
>> + * given node to the given string value (using the length of the
>> + * string to determine the new length of the property), or creates a
>> + * new property with that value if it does not already exist.
>> + *
>> + * This function may insert or delete data from the blob, and will
>> + * therefore change the offsets of some existing nodes.
>> + *
>> + * returns:
>> + *	0, on success
>> + *	-FDT_ERR_NOSPACE, there is insufficient free space in the blob to
>> + *		contain the new property value
>> + *	-FDT_ERR_BADOFFSET, nodeoffset did not point to FDT_BEGIN_NODE tag
>> + *	-FDT_ERR_BADLAYOUT,
>> + *	-FDT_ERR_BADMAGIC,
>> + *	-FDT_ERR_BADVERSION,
>> + *	-FDT_ERR_BADSTATE,
>> + *	-FDT_ERR_BADSTRUCTURE,
>> + *	-FDT_ERR_BADLAYOUT,
>> + *	-FDT_ERR_TRUNCATED, standard meanings
>> + */
>> +#define fdt_setprop_namelen_string(fdt, nodeoffset, name, namelen, str)    \
>> +	fdt_setprop_namelen((fdt), (nodeoffset), (name), (namelen), (str), \
>> +			    strlen(str) + 1)
>>   
>>   /**
>>    * fdt_setprop_empty - set a property to an empty value
>> diff --git a/util.h b/util.h
>> index 800f2e2c55b150d3c30101d1e1f1daaa0e4e3264..3e2ebaabe1e8c7e9ad6821ac2ca17a72e7f4f57d 100644
>> --- a/util.h
>> +++ b/util.h
>> @@ -6,6 +6,7 @@
>>   #include <stdarg.h>
>>   #include <stdbool.h>
>>   #include <getopt.h>
>> +#include <stdio.h>
>>   
>>   /*
>>    * Copyright 2011 The Chromium Authors, All Rights Reserved.
>>
> 

Ayush Singh

[PATCH 3/5] fdtoverlay: Implement resolving path references

[Ayush Singh]

dts allows references both in integer context:
	foo = <&bar>;
in which case it resolves to a phandle, but also in string/bytestring
context:
	foo = &bar;
In which case it resolves to a path.

Runtime overlays, only support the former, but not the latter.

This patch implements resolving path references when applying overlays.

Signed-off-by: Ayush Singh <ayush@beagleboard.org>
---
 fdtoverlay.c         |  34 +++++++---
 libfdt/fdt_overlay.c | 174 ++++++++++++++++++++++++++++++---------------------
 libfdt/libfdt.h      |  30 ++++++++-
 libfdt/version.lds   |   1 +
 4 files changed, 160 insertions(+), 79 deletions(-)

diff --git a/fdtoverlay.c b/fdtoverlay.c
index ee1eb8f3ad28b14762aca9ab5c8a4e36aac967d8..3b6e666212f7c6742ab1e7897c5a0288ff4049ac 100644
--- a/fdtoverlay.c
+++ b/fdtoverlay.c
@@ -44,16 +44,36 @@ static void *apply_one(char *base, const char *overlay, size_t *buf_len,
 		       const char *name)
 {
 	char *tmp = NULL;
-	char *tmpo;
+	char *tmpo = NULL;
 	int ret;
 	bool has_symbols;
+	size_t tmpo_len = fdt_totalsize(overlay);
 
-	/*
-	 * We take copies first, because a failed apply can trash
-	 * both the base blob and the overlay
-	 */
-	tmpo = xmalloc(fdt_totalsize(overlay));
+	/* Prepare DT overlay */
+	do {
+		tmpo = xrealloc(tmpo, tmpo_len);
+
+		ret = fdt_open_into(overlay, tmpo, tmpo_len);
+		if (ret) {
+			fprintf(stderr,
+				"\nFailed to make temporary copy of overlay: %s\n",
+				fdt_strerror(ret));
+			goto fail;
+		}
+
+		ret = fdt_overlay_prepare(base, tmpo);
+		if (ret == -FDT_ERR_NOSPACE) {
+			tmpo_len += BUF_INCREMENT;
+		}
+	} while (ret == -FDT_ERR_NOSPACE);
+
+	if (ret) {
+		fprintf(stderr, "\nFailed to prepare overlay '%s': %s\n", name,
+			fdt_strerror(ret));
+		goto fail;
+	}
 
+	/* Apply DT overlay to base tree */
 	do {
 		tmp = xrealloc(tmp, *buf_len);
 		ret = fdt_open_into(base, tmp, *buf_len);
@@ -66,8 +86,6 @@ static void *apply_one(char *base, const char *overlay, size_t *buf_len,
 		ret = fdt_path_offset(tmp, "/__symbols__");
 		has_symbols = ret >= 0;
 
-		memcpy(tmpo, overlay, fdt_totalsize(overlay));
-
 		ret = fdt_overlay_apply(tmp, tmpo);
 		if (ret == -FDT_ERR_NOSPACE) {
 			*buf_len += BUF_INCREMENT;
diff --git a/libfdt/fdt_overlay.c b/libfdt/fdt_overlay.c
index 28b667ffc490e5a24a2b116714eb218d8d16a995..a912bfc580363b5a1376a2b89397db0e7e93a7df 100644
--- a/libfdt/fdt_overlay.c
+++ b/libfdt/fdt_overlay.c
@@ -10,6 +10,7 @@
 #include <libfdt.h>
 
 #include "libfdt_internal.h"
+#include "../util.h"
 
 /**
  * overlay_get_target_phandle - retrieves the target phandle of a fragment
@@ -306,7 +307,7 @@ static int overlay_update_local_references(void *fdto, uint32_t delta)
 }
 
 /**
- * overlay_fixup_one_phandle - Set an overlay phandle to the base one
+ * overlay_fixup_one_path_or_phandle - Set an overlay path/phandle to the base one
  * @fdt: Base Device Tree blob
  * @fdto: Device tree overlay blob
  * @symbols_off: Node offset of the symbols node in the base device tree
@@ -315,27 +316,30 @@ static int overlay_update_local_references(void *fdto, uint32_t delta)
  * @name: Name of the property holding the phandle reference in the overlay
  * @name_len: number of name characters to consider
  * @poffset: Offset within the overlay property where the phandle is stored
- * @phandle: Phandle referencing the node
+ * @symbol_path: path of the pointed node
  *
- * overlay_fixup_one_phandle() resolves an overlay phandle pointing to
- * a node in the base device tree.
+ * overlay_fixup_one_path_or_phandle() resolves an overlay path/phandle pointing 
+ * to a node in the base device tree.
  *
  * This is part of the device tree overlay application process, when
- * you want all the phandles in the overlay to point to the actual
+ * you want all the paths/phandles in the overlay to point to the actual
  * base dt nodes.
  *
  * returns:
  *      0 on success
  *      Negative error code on failure
  */
-static int overlay_fixup_one_phandle(void *fdt, void *fdto,
-				     int symbols_off,
-				     const char *path, uint32_t path_len,
-				     const char *name, uint32_t name_len,
-				     int poffset, uint32_t phandle)
+static int overlay_fixup_one_path_or_phandle(const void *fdt, void *fdto,
+					     int symbols_off, const char *path,
+					     uint32_t path_len,
+					     const char *name,
+					     uint32_t name_len, int poffset,
+					     const char *symbol_path)
 {
+	const void *prop;
+	int fixup_off, prop_len, symbol_off;
+	uint32_t phandle;
 	fdt32_t phandle_prop;
-	int fixup_off;
 
 	if (symbols_off < 0)
 		return symbols_off;
@@ -346,45 +350,60 @@ static int overlay_fixup_one_phandle(void *fdt, void *fdto,
 	if (fixup_off < 0)
 		return fixup_off;
 
-	phandle_prop = cpu_to_fdt32(phandle);
-	return fdt_setprop_inplace_namelen_partial(fdto, fixup_off,
-						   name, name_len, poffset,
-						   &phandle_prop,
-						   sizeof(phandle_prop));
+	prop = fdt_getprop_namelen(fdto, fixup_off, name, name_len, &prop_len);
+	if (!prop)
+		return -FDT_ERR_BADOVERLAY;
+
+	/* path references are empty dt properties in overlay */
+	if (prop_len == 0) {
+		return fdt_setprop_namelen_string(fdto, fixup_off, name,
+						  name_len, symbol_path);
+	} else {
+		symbol_off = fdt_path_offset(fdt, symbol_path);
+		if (symbol_off < 0)
+			return symbol_off;
+
+		phandle = fdt_get_phandle(fdt, symbol_off);
+		if (!phandle)
+			return -FDT_ERR_NOTFOUND;
+		phandle_prop = cpu_to_fdt32(phandle);
+		return fdt_setprop_inplace_namelen_partial(
+			fdto, fixup_off, name, name_len, poffset, &phandle_prop,
+			sizeof(phandle_prop));
+	}
 };
 
 /**
- * overlay_fixup_phandle - Set an overlay phandle to the base one
+ * overlay_fixup_path_or_phandle - Set an overlay path/phandle to the base one
  * @fdt: Base Device Tree blob
  * @fdto: Device tree overlay blob
+ * @fixups: Device tree overlay ro copy
  * @symbols_off: Node offset of the symbols node in the base device tree
  * @property: Property offset in the overlay holding the list of fixups
  *
- * overlay_fixup_phandle() resolves all the overlay phandles pointed
- * to in a __fixups__ property, and updates them to match the phandles
- * in use in the base device tree.
+ * overlay_fixup_path_or_phandle() resolves all the overlay paths/phandles
+ * pointed to in a __fixups__ property, and updates them to match the
+ * paths/phandles in use in the base device tree.
  *
  * This is part of the device tree overlay application process, when
- * you want all the phandles in the overlay to point to the actual
+ * you want all the paths/phandles in the overlay to point to the actual
  * base dt nodes.
  *
  * returns:
  *      0 on success
  *      Negative error code on failure
  */
-static int overlay_fixup_phandle(void *fdt, void *fdto, int symbols_off,
-				 int property)
+static int overlay_fixup_path_or_phandle(const void *fdt, void *fdto,
+					 const void *fixups, int symbols_off,
+					 int property)
 {
 	const char *value;
 	const char *label;
 	int len;
 	const char *symbol_path;
 	int prop_len;
-	int symbol_off;
-	uint32_t phandle;
 
-	value = fdt_getprop_by_offset(fdto, property,
-				      &label, &len);
+	value = fdt_getprop_by_offset(fixups, property, &label, &len);
 	if (!value) {
 		if (len == -FDT_ERR_NOTFOUND)
 			return -FDT_ERR_INTERNAL;
@@ -395,14 +414,6 @@ static int overlay_fixup_phandle(void *fdt, void *fdto, int symbols_off,
 	symbol_path = fdt_getprop(fdt, symbols_off, label, &prop_len);
 	if (!symbol_path)
 		return prop_len;
-	
-	symbol_off = fdt_path_offset(fdt, symbol_path);
-	if (symbol_off < 0)
-		return symbol_off;
-	
-	phandle = fdt_get_phandle(fdt, symbol_off);
-	if (!phandle)
-		return -FDT_ERR_NOTFOUND;
 
 	do {
 		const char *path, *name, *fixup_end;
@@ -415,6 +426,7 @@ static int overlay_fixup_phandle(void *fdt, void *fdto, int symbols_off,
 		fixup_end = memchr(value, '\0', len);
 		if (!fixup_end)
 			return -FDT_ERR_BADOVERLAY;
+
 		fixup_len = fixup_end - fixup_str;
 
 		len -= fixup_len + 1;
@@ -443,9 +455,10 @@ static int overlay_fixup_phandle(void *fdt, void *fdto, int symbols_off,
 		if ((*endptr != '\0') || (endptr <= (sep + 1)))
 			return -FDT_ERR_BADOVERLAY;
 
-		ret = overlay_fixup_one_phandle(fdt, fdto, symbols_off,
-						path, path_len, name, name_len,
-						poffset, phandle);
+		ret = overlay_fixup_one_path_or_phandle(fdt, fdto, symbols_off,
+							path, path_len, name,
+							name_len, poffset,
+							symbol_path);
 		if (ret)
 			return ret;
 	} while (len > 0);
@@ -454,26 +467,26 @@ static int overlay_fixup_phandle(void *fdt, void *fdto, int symbols_off,
 }
 
 /**
- * overlay_fixup_phandles - Resolve the overlay phandles to the base
- *                          device tree
+ * overlay_fixup_paths_or_phandles - Resolve the overlay paths/phandles to the
+ *                                   base device tree
  * @fdt: Base Device Tree blob
  * @fdto: Device tree overlay blob
  *
- * overlay_fixup_phandles() resolves all the overlay phandles pointing
- * to nodes in the base device tree.
+ * overlay_fixup_paths_or_phandles() resolves all the overlay paths/phandles
+ * pointing to nodes in the base device tree.
  *
  * This is one of the steps of the device tree overlay application
- * process, when you want all the phandles in the overlay to point to
+ * process, when you want all the paths/phandles in the overlay to point to
  * the actual base dt nodes.
  *
  * returns:
  *      0 on success
  *      Negative error code on failure
  */
-static int overlay_fixup_phandles(void *fdt, void *fdto)
+static int overlay_fixup_paths_or_phandles(const void *fdt, void *fdto)
 {
-	int fixups_off, symbols_off;
-	int property;
+	void *tmp = NULL;
+	int fixups_off, symbols_off, property, depth = 0, ret;
 
 	/* We can have overlays without any fixups */
 	fixups_off = fdt_path_offset(fdto, "/__fixups__");
@@ -487,15 +500,30 @@ static int overlay_fixup_phandles(void *fdt, void *fdto)
 	if ((symbols_off < 0 && (symbols_off != -FDT_ERR_NOTFOUND)))
 		return symbols_off;
 
-	fdt_for_each_property_offset(property, fdto, fixups_off) {
-		int ret;
+	ret = fdt_next_node(fdto, fixups_off, &depth);
+	if (ret < 0)
+		return ret;
 
-		ret = overlay_fixup_phandle(fdt, fdto, symbols_off, property);
+	/* resolving path references will resize the overlay and make fixup references
+	 * invalid. So use a ro copy for reading fixups.
+	 */
+	tmp = xmalloc(fdt_totalsize(fdto));
+	memcpy(tmp, fdto, fdt_totalsize(fdto));
+
+	fdt_for_each_property_offset(property, tmp, fixups_off)
+	{
+		ret = overlay_fixup_path_or_phandle(fdt, fdto, tmp, symbols_off,
+						    property);
 		if (ret)
-			return ret;
+			goto cleanup;
 	}
 
-	return 0;
+	ret = 0;
+
+cleanup:
+	free(tmp);
+
+	return ret;
 }
 
 /**
@@ -653,7 +681,7 @@ static int overlay_update_local_conflicting_references(void *fdto,
  *      0 on success
  *      Negative error code on failure
  */
-static int overlay_prevent_phandle_overwrite_node(void *fdt, int fdtnode,
+static int overlay_prevent_phandle_overwrite_node(const void *fdt, int fdtnode,
 						  void *fdto, int fdtonode)
 {
 	uint32_t fdt_phandle, fdto_phandle;
@@ -712,7 +740,7 @@ static int overlay_prevent_phandle_overwrite_node(void *fdt, int fdtnode,
  *      0 on success
  *      Negative error code on failure
  */
-static int overlay_prevent_phandle_overwrite(void *fdt, void *fdto)
+static int overlay_prevent_phandle_overwrite(const void *fdt, void *fdto)
 {
 	int fragment;
 
@@ -766,8 +794,7 @@ static int overlay_prevent_phandle_overwrite(void *fdt, void *fdto)
  *      0 on success
  *      Negative error code on failure
  */
-static int overlay_apply_node(void *fdt, int target,
-			      void *fdto, int node)
+static int overlay_apply_node(void *fdt, int target, const void *fdto, int node)
 {
 	int property;
 	int subnode;
@@ -828,7 +855,7 @@ static int overlay_apply_node(void *fdt, int target,
  *      0 on success
  *      Negative error code on failure
  */
-static int overlay_merge(void *fdt, void *fdto)
+static int overlay_merge(void *fdt, const void *fdto)
 {
 	int fragment;
 
@@ -904,7 +931,7 @@ static int get_path_len(const void *fdt, int nodeoffset)
  *      0 on success
  *      Negative error code on failure
  */
-static int overlay_symbol_update(void *fdt, void *fdto)
+static int overlay_symbol_update(void *fdt, const void *fdto)
 {
 	int root_sym, ov_sym, prop, path_len, fragment, target;
 	int len, frag_name_len, ret, rel_path_len;
@@ -1039,7 +1066,7 @@ static int overlay_symbol_update(void *fdt, void *fdto)
 	return 0;
 }
 
-int fdt_overlay_apply(void *fdt, void *fdto)
+int fdt_overlay_prepare(const void *fdt, void *fdto)
 {
 	uint32_t delta;
 	int ret;
@@ -1061,8 +1088,8 @@ int fdt_overlay_apply(void *fdt, void *fdto)
 	if (ret)
 		goto err;
 
-	/* Update fdto's phandles using symbols from fdt */
-	ret = overlay_fixup_phandles(fdt, fdto);
+	/* Update fdto's paths and phandles using symbols from fdt */
+	ret = overlay_fixup_paths_or_phandles(fdt, fdto);
 	if (ret)
 		goto err;
 
@@ -1071,6 +1098,23 @@ int fdt_overlay_apply(void *fdt, void *fdto)
 	if (ret)
 		goto err;
 
+	return 0;
+err:
+	/*
+	 * The overlay might have been damaged, erase its magic.
+	 */
+	fdt_set_magic(fdto, ~0);
+
+	return ret;
+}
+
+int fdt_overlay_apply(void *fdt, const void *fdto)
+{
+	int ret = 0;
+
+	FDT_RO_PROBE(fdt);
+	FDT_RO_PROBE(fdto);
+
 	ret = overlay_merge(fdt, fdto);
 	if (ret)
 		goto err;
@@ -1079,19 +1123,9 @@ int fdt_overlay_apply(void *fdt, void *fdto)
 	if (ret)
 		goto err;
 
-	/*
-	 * The overlay has been damaged, erase its magic.
-	 */
-	fdt_set_magic(fdto, ~0);
-
 	return 0;
 
 err:
-	/*
-	 * The overlay might have been damaged, erase its magic.
-	 */
-	fdt_set_magic(fdto, ~0);
-
 	/*
 	 * The base device tree might have been damaged, erase its
 	 * magic.
diff --git a/libfdt/libfdt.h b/libfdt/libfdt.h
index 999b3800dff5c001b9c1babf370d45024c81a9aa..c14b778108427d0d578b86bc79f18dc88be05524 100644
--- a/libfdt/libfdt.h
+++ b/libfdt/libfdt.h
@@ -2215,6 +2215,34 @@ int fdt_add_subnode(void *fdt, int parentoffset, const char *name);
  */
 int fdt_del_node(void *fdt, int nodeoffset);
 
+/**
+ * fdt_overlay_prepare - Prepares DT overlay for merging with base DT
+ * @fdt: const pointer to the base device tree blob
+ * @fdto: pointer to the device tree overlay blob
+ *
+ * fdt_overlay_prepare() will take care of any resizing/prepartion of the
+ * given device tree overlay.
+ *
+ * returns:
+ *	0, on success
+ *	-FDT_ERR_NOSPACE, there's not enough space in the device tree overlay
+ *	-FDT_ERR_NOTFOUND, the overlay points to some nonexistent nodes or
+ *		properties in the base DT
+ *	-FDT_ERR_BADPHANDLE,
+ *	-FDT_ERR_BADOVERLAY,
+ *	-FDT_ERR_NOPHANDLES,
+ *	-FDT_ERR_INTERNAL,
+ *	-FDT_ERR_BADLAYOUT,
+ *	-FDT_ERR_BADMAGIC,
+ *	-FDT_ERR_BADOFFSET,
+ *	-FDT_ERR_BADPATH,
+ *	-FDT_ERR_BADVERSION,
+ *	-FDT_ERR_BADSTRUCTURE,
+ *	-FDT_ERR_BADSTATE,
+ *	-FDT_ERR_TRUNCATED, standard meanings
+ */
+int fdt_overlay_prepare(const void *fdt, void *fdto);
+
 /**
  * fdt_overlay_apply - Applies a DT overlay on a base DT
  * @fdt: pointer to the base device tree blob
@@ -2244,7 +2272,7 @@ int fdt_del_node(void *fdt, int nodeoffset);
  *	-FDT_ERR_BADSTATE,
  *	-FDT_ERR_TRUNCATED, standard meanings
  */
-int fdt_overlay_apply(void *fdt, void *fdto);
+int fdt_overlay_apply(void *fdt, const void *fdto);
 
 /**
  * fdt_overlay_target_offset - retrieves the offset of a fragment's target
diff --git a/libfdt/version.lds b/libfdt/version.lds
index 989cd89f1051ce59255a3b3e60493be4e5e985cc..88ef44bb19ec69aabac61a1254264966e0a9d668 100644
--- a/libfdt/version.lds
+++ b/libfdt/version.lds
@@ -66,6 +66,7 @@ LIBFDT_1.2 {
 		fdt_stringlist_search;
 		fdt_stringlist_get;
 		fdt_resize;
+		fdt_overlay_prepare;
 		fdt_overlay_apply;
 		fdt_get_string;
 		fdt_find_max_phandle;

-- 
2.47.0

Re: [PATCH 3/5] fdtoverlay: Implement resolving path references

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 18157 bytes --]

On Sat, Nov 16, 2024 at 08:30:21PM +0530, Ayush Singh wrote:
> dts allows references both in integer context:
> 	foo = <&bar>;
> in which case it resolves to a phandle, but also in string/bytestring
> context:
> 	foo = &bar;
> In which case it resolves to a path.
> 
> Runtime overlays, only support the former, but not the latter.
> 
> This patch implements resolving path references when applying overlays.
> 
> Signed-off-by: Ayush Singh <ayush@beagleboard.org>
> ---
>  fdtoverlay.c         |  34 +++++++---
>  libfdt/fdt_overlay.c | 174 ++++++++++++++++++++++++++++++---------------------
>  libfdt/libfdt.h      |  30 ++++++++-
>  libfdt/version.lds   |   1 +
>  4 files changed, 160 insertions(+), 79 deletions(-)
> 
> diff --git a/fdtoverlay.c b/fdtoverlay.c
> index ee1eb8f3ad28b14762aca9ab5c8a4e36aac967d8..3b6e666212f7c6742ab1e7897c5a0288ff4049ac 100644
> --- a/fdtoverlay.c
> +++ b/fdtoverlay.c
> @@ -44,16 +44,36 @@ static void *apply_one(char *base, const char *overlay, size_t *buf_len,
>  		       const char *name)
>  {
>  	char *tmp = NULL;
> -	char *tmpo;
> +	char *tmpo = NULL;
>  	int ret;
>  	bool has_symbols;
> +	size_t tmpo_len = fdt_totalsize(overlay);
>  
> -	/*
> -	 * We take copies first, because a failed apply can trash
> -	 * both the base blob and the overlay
> -	 */
> -	tmpo = xmalloc(fdt_totalsize(overlay));

Changing the comment and allocation logic seems like an unrelated
change to what you're actually trying to accomplish here.

> +	/* Prepare DT overlay */
> +	do {
> +		tmpo = xrealloc(tmpo, tmpo_len);
> +
> +		ret = fdt_open_into(overlay, tmpo, tmpo_len);
> +		if (ret) {
> +			fprintf(stderr,
> +				"\nFailed to make temporary copy of overlay: %s\n",
> +				fdt_strerror(ret));
> +			goto fail;
> +		}
> +
> +		ret = fdt_overlay_prepare(base, tmpo);
> +		if (ret == -FDT_ERR_NOSPACE) {
> +			tmpo_len += BUF_INCREMENT;
> +		}
> +	} while (ret == -FDT_ERR_NOSPACE);
> +
> +	if (ret) {
> +		fprintf(stderr, "\nFailed to prepare overlay '%s': %s\n", name,
> +			fdt_strerror(ret));
> +		goto fail;
> +	}
>  
> +	/* Apply DT overlay to base tree */
>  	do {
>  		tmp = xrealloc(tmp, *buf_len);
>  		ret = fdt_open_into(base, tmp, *buf_len);
> @@ -66,8 +86,6 @@ static void *apply_one(char *base, const char *overlay, size_t *buf_len,
>  		ret = fdt_path_offset(tmp, "/__symbols__");
>  		has_symbols = ret >= 0;
>  
> -		memcpy(tmpo, overlay, fdt_totalsize(overlay));
> -
>  		ret = fdt_overlay_apply(tmp, tmpo);
>  		if (ret == -FDT_ERR_NOSPACE) {
>  			*buf_len += BUF_INCREMENT;
> diff --git a/libfdt/fdt_overlay.c b/libfdt/fdt_overlay.c
> index 28b667ffc490e5a24a2b116714eb218d8d16a995..a912bfc580363b5a1376a2b89397db0e7e93a7df 100644
> --- a/libfdt/fdt_overlay.c
> +++ b/libfdt/fdt_overlay.c
> @@ -10,6 +10,7 @@
>  #include <libfdt.h>
>  
>  #include "libfdt_internal.h"
> +#include "../util.h"

As noted elsewhere including ../util.h (and specifically all the
things it includes indirectly) in libfdt is not permitted.

>  /**
>   * overlay_get_target_phandle - retrieves the target phandle of a fragment
> @@ -306,7 +307,7 @@ static int overlay_update_local_references(void *fdto, uint32_t delta)
>  }
>  
>  /**
> - * overlay_fixup_one_phandle - Set an overlay phandle to the base one
> + * overlay_fixup_one_path_or_phandle - Set an overlay path/phandle to the base one
>   * @fdt: Base Device Tree blob
>   * @fdto: Device tree overlay blob
>   * @symbols_off: Node offset of the symbols node in the base device tree
> @@ -315,27 +316,30 @@ static int overlay_update_local_references(void *fdto, uint32_t delta)
>   * @name: Name of the property holding the phandle reference in the overlay
>   * @name_len: number of name characters to consider
>   * @poffset: Offset within the overlay property where the phandle is stored
> - * @phandle: Phandle referencing the node
> + * @symbol_path: path of the pointed node
>   *
> - * overlay_fixup_one_phandle() resolves an overlay phandle pointing to
> - * a node in the base device tree.
> + * overlay_fixup_one_path_or_phandle() resolves an overlay path/phandle pointing 
> + * to a node in the base device tree.
>   *
>   * This is part of the device tree overlay application process, when
> - * you want all the phandles in the overlay to point to the actual
> + * you want all the paths/phandles in the overlay to point to the actual
>   * base dt nodes.
>   *
>   * returns:
>   *      0 on success
>   *      Negative error code on failure
>   */
> -static int overlay_fixup_one_phandle(void *fdt, void *fdto,
> -				     int symbols_off,
> -				     const char *path, uint32_t path_len,
> -				     const char *name, uint32_t name_len,
> -				     int poffset, uint32_t phandle)
> +static int overlay_fixup_one_path_or_phandle(const void *fdt, void *fdto,
> +					     int symbols_off, const char *path,
> +					     uint32_t path_len,
> +					     const char *name,
> +					     uint32_t name_len, int poffset,
> +					     const char *symbol_path)
>  {
> +	const void *prop;
> +	int fixup_off, prop_len, symbol_off;
> +	uint32_t phandle;
>  	fdt32_t phandle_prop;
> -	int fixup_off;
>  
>  	if (symbols_off < 0)
>  		return symbols_off;
> @@ -346,45 +350,60 @@ static int overlay_fixup_one_phandle(void *fdt, void *fdto,
>  	if (fixup_off < 0)
>  		return fixup_off;
>  
> -	phandle_prop = cpu_to_fdt32(phandle);
> -	return fdt_setprop_inplace_namelen_partial(fdto, fixup_off,
> -						   name, name_len, poffset,
> -						   &phandle_prop,
> -						   sizeof(phandle_prop));
> +	prop = fdt_getprop_namelen(fdto, fixup_off, name, name_len, &prop_len);
> +	if (!prop)
> +		return -FDT_ERR_BADOVERLAY;
> +
> +	/* path references are empty dt properties in overlay */

Oh.. nope, this is wrong.  Path references need not be a whole
property, they can be combined with other things just like phandle
references.  You need to distinguish the two fixup types in the
overlay blob so you know which one to apply.

> +	if (prop_len == 0) {
> +		return fdt_setprop_namelen_string(fdto, fixup_off, name,
> +						  name_len, symbol_path);

.. and you need to splice the path into the existing property if there
is one.

> +	} else {
> +		symbol_off = fdt_path_offset(fdt, symbol_path);
> +		if (symbol_off < 0)
> +			return symbol_off;
> +
> +		phandle = fdt_get_phandle(fdt, symbol_off);
> +		if (!phandle)
> +			return -FDT_ERR_NOTFOUND;
> +		phandle_prop = cpu_to_fdt32(phandle);
> +		return fdt_setprop_inplace_namelen_partial(
> +			fdto, fixup_off, name, name_len, poffset, &phandle_prop,
> +			sizeof(phandle_prop));
> +	}
>  };
>  
>  /**
> - * overlay_fixup_phandle - Set an overlay phandle to the base one
> + * overlay_fixup_path_or_phandle - Set an overlay path/phandle to the base one
>   * @fdt: Base Device Tree blob
>   * @fdto: Device tree overlay blob
> + * @fixups: Device tree overlay ro copy
>   * @symbols_off: Node offset of the symbols node in the base device tree
>   * @property: Property offset in the overlay holding the list of fixups
>   *
> - * overlay_fixup_phandle() resolves all the overlay phandles pointed
> - * to in a __fixups__ property, and updates them to match the phandles
> - * in use in the base device tree.
> + * overlay_fixup_path_or_phandle() resolves all the overlay paths/phandles
> + * pointed to in a __fixups__ property, and updates them to match the
> + * paths/phandles in use in the base device tree.
>   *
>   * This is part of the device tree overlay application process, when
> - * you want all the phandles in the overlay to point to the actual
> + * you want all the paths/phandles in the overlay to point to the actual
>   * base dt nodes.
>   *
>   * returns:
>   *      0 on success
>   *      Negative error code on failure
>   */
> -static int overlay_fixup_phandle(void *fdt, void *fdto, int symbols_off,
> -				 int property)
> +static int overlay_fixup_path_or_phandle(const void *fdt, void *fdto,
> +					 const void *fixups, int symbols_off,
> +					 int property)
>  {
>  	const char *value;
>  	const char *label;
>  	int len;
>  	const char *symbol_path;
>  	int prop_len;
> -	int symbol_off;
> -	uint32_t phandle;
>  
> -	value = fdt_getprop_by_offset(fdto, property,
> -				      &label, &len);
> +	value = fdt_getprop_by_offset(fixups, property, &label, &len);
>  	if (!value) {
>  		if (len == -FDT_ERR_NOTFOUND)
>  			return -FDT_ERR_INTERNAL;
> @@ -395,14 +414,6 @@ static int overlay_fixup_phandle(void *fdt, void *fdto, int symbols_off,
>  	symbol_path = fdt_getprop(fdt, symbols_off, label, &prop_len);
>  	if (!symbol_path)
>  		return prop_len;
> -	
> -	symbol_off = fdt_path_offset(fdt, symbol_path);
> -	if (symbol_off < 0)
> -		return symbol_off;
> -	
> -	phandle = fdt_get_phandle(fdt, symbol_off);
> -	if (!phandle)
> -		return -FDT_ERR_NOTFOUND;
>  
>  	do {
>  		const char *path, *name, *fixup_end;
> @@ -415,6 +426,7 @@ static int overlay_fixup_phandle(void *fdt, void *fdto, int symbols_off,
>  		fixup_end = memchr(value, '\0', len);
>  		if (!fixup_end)
>  			return -FDT_ERR_BADOVERLAY;
> +
>  		fixup_len = fixup_end - fixup_str;
>  
>  		len -= fixup_len + 1;
> @@ -443,9 +455,10 @@ static int overlay_fixup_phandle(void *fdt, void *fdto, int symbols_off,
>  		if ((*endptr != '\0') || (endptr <= (sep + 1)))
>  			return -FDT_ERR_BADOVERLAY;
>  
> -		ret = overlay_fixup_one_phandle(fdt, fdto, symbols_off,
> -						path, path_len, name, name_len,
> -						poffset, phandle);
> +		ret = overlay_fixup_one_path_or_phandle(fdt, fdto, symbols_off,
> +							path, path_len, name,
> +							name_len, poffset,
> +							symbol_path);
>  		if (ret)
>  			return ret;
>  	} while (len > 0);
> @@ -454,26 +467,26 @@ static int overlay_fixup_phandle(void *fdt, void *fdto, int symbols_off,
>  }
>  
>  /**
> - * overlay_fixup_phandles - Resolve the overlay phandles to the base
> - *                          device tree
> + * overlay_fixup_paths_or_phandles - Resolve the overlay paths/phandles to the
> + *                                   base device tree
>   * @fdt: Base Device Tree blob
>   * @fdto: Device tree overlay blob
>   *
> - * overlay_fixup_phandles() resolves all the overlay phandles pointing
> - * to nodes in the base device tree.
> + * overlay_fixup_paths_or_phandles() resolves all the overlay paths/phandles
> + * pointing to nodes in the base device tree.
>   *
>   * This is one of the steps of the device tree overlay application
> - * process, when you want all the phandles in the overlay to point to
> + * process, when you want all the paths/phandles in the overlay to point to
>   * the actual base dt nodes.
>   *
>   * returns:
>   *      0 on success
>   *      Negative error code on failure
>   */
> -static int overlay_fixup_phandles(void *fdt, void *fdto)
> +static int overlay_fixup_paths_or_phandles(const void *fdt, void *fdto)
>  {
> -	int fixups_off, symbols_off;
> -	int property;
> +	void *tmp = NULL;
> +	int fixups_off, symbols_off, property, depth = 0, ret;
>  
>  	/* We can have overlays without any fixups */
>  	fixups_off = fdt_path_offset(fdto, "/__fixups__");
> @@ -487,15 +500,30 @@ static int overlay_fixup_phandles(void *fdt, void *fdto)
>  	if ((symbols_off < 0 && (symbols_off != -FDT_ERR_NOTFOUND)))
>  		return symbols_off;
>  
> -	fdt_for_each_property_offset(property, fdto, fixups_off) {
> -		int ret;
> +	ret = fdt_next_node(fdto, fixups_off, &depth);
> +	if (ret < 0)
> +		return ret;
>  
> -		ret = overlay_fixup_phandle(fdt, fdto, symbols_off, property);
> +	/* resolving path references will resize the overlay and make fixup references
> +	 * invalid. So use a ro copy for reading fixups.
> +	 */
> +	tmp = xmalloc(fdt_totalsize(fdto));
> +	memcpy(tmp, fdto, fdt_totalsize(fdto));

No allocator, sorry.  You're going to have to do this the hard way.

> +
> +	fdt_for_each_property_offset(property, tmp, fixups_off)
> +	{
> +		ret = overlay_fixup_path_or_phandle(fdt, fdto, tmp, symbols_off,
> +						    property);
>  		if (ret)
> -			return ret;
> +			goto cleanup;
>  	}
>  
> -	return 0;
> +	ret = 0;
> +
> +cleanup:
> +	free(tmp);
> +
> +	return ret;
>  }
>  
>  /**
> @@ -653,7 +681,7 @@ static int overlay_update_local_conflicting_references(void *fdto,
>   *      0 on success
>   *      Negative error code on failure
>   */
> -static int overlay_prevent_phandle_overwrite_node(void *fdt, int fdtnode,
> +static int overlay_prevent_phandle_overwrite_node(const void *fdt, int fdtnode,
>  						  void *fdto, int fdtonode)
>  {
>  	uint32_t fdt_phandle, fdto_phandle;
> @@ -712,7 +740,7 @@ static int overlay_prevent_phandle_overwrite_node(void *fdt, int fdtnode,
>   *      0 on success
>   *      Negative error code on failure
>   */
> -static int overlay_prevent_phandle_overwrite(void *fdt, void *fdto)
> +static int overlay_prevent_phandle_overwrite(const void *fdt, void *fdto)
>  {
>  	int fragment;
>  
> @@ -766,8 +794,7 @@ static int overlay_prevent_phandle_overwrite(void *fdt, void *fdto)
>   *      0 on success
>   *      Negative error code on failure
>   */
> -static int overlay_apply_node(void *fdt, int target,
> -			      void *fdto, int node)
> +static int overlay_apply_node(void *fdt, int target, const void *fdto, int node)
>  {
>  	int property;
>  	int subnode;
> @@ -828,7 +855,7 @@ static int overlay_apply_node(void *fdt, int target,
>   *      0 on success
>   *      Negative error code on failure
>   */
> -static int overlay_merge(void *fdt, void *fdto)
> +static int overlay_merge(void *fdt, const void *fdto)
>  {
>  	int fragment;
>  
> @@ -904,7 +931,7 @@ static int get_path_len(const void *fdt, int nodeoffset)
>   *      0 on success
>   *      Negative error code on failure
>   */
> -static int overlay_symbol_update(void *fdt, void *fdto)
> +static int overlay_symbol_update(void *fdt, const void *fdto)
>  {
>  	int root_sym, ov_sym, prop, path_len, fragment, target;
>  	int len, frag_name_len, ret, rel_path_len;
> @@ -1039,7 +1066,7 @@ static int overlay_symbol_update(void *fdt, void *fdto)
>  	return 0;
>  }
>  
> -int fdt_overlay_apply(void *fdt, void *fdto)
> +int fdt_overlay_prepare(const void *fdt, void *fdto)
>  {
>  	uint32_t delta;
>  	int ret;
> @@ -1061,8 +1088,8 @@ int fdt_overlay_apply(void *fdt, void *fdto)
>  	if (ret)
>  		goto err;
>  
> -	/* Update fdto's phandles using symbols from fdt */
> -	ret = overlay_fixup_phandles(fdt, fdto);
> +	/* Update fdto's paths and phandles using symbols from fdt */
> +	ret = overlay_fixup_paths_or_phandles(fdt, fdto);
>  	if (ret)
>  		goto err;
>  
> @@ -1071,6 +1098,23 @@ int fdt_overlay_apply(void *fdt, void *fdto)
>  	if (ret)
>  		goto err;
>  
> +	return 0;
> +err:
> +	/*
> +	 * The overlay might have been damaged, erase its magic.
> +	 */
> +	fdt_set_magic(fdto, ~0);
> +
> +	return ret;
> +}
> +
> +int fdt_overlay_apply(void *fdt, const void *fdto)
> +{
> +	int ret = 0;
> +
> +	FDT_RO_PROBE(fdt);
> +	FDT_RO_PROBE(fdto);
> +
>  	ret = overlay_merge(fdt, fdto);
>  	if (ret)
>  		goto err;
> @@ -1079,19 +1123,9 @@ int fdt_overlay_apply(void *fdt, void *fdto)
>  	if (ret)
>  		goto err;
>  
> -	/*
> -	 * The overlay has been damaged, erase its magic.
> -	 */
> -	fdt_set_magic(fdto, ~0);
> -
>  	return 0;
>  
>  err:
> -	/*
> -	 * The overlay might have been damaged, erase its magic.
> -	 */
> -	fdt_set_magic(fdto, ~0);
> -
>  	/*
>  	 * The base device tree might have been damaged, erase its
>  	 * magic.
> diff --git a/libfdt/libfdt.h b/libfdt/libfdt.h
> index 999b3800dff5c001b9c1babf370d45024c81a9aa..c14b778108427d0d578b86bc79f18dc88be05524 100644
> --- a/libfdt/libfdt.h
> +++ b/libfdt/libfdt.h
> @@ -2215,6 +2215,34 @@ int fdt_add_subnode(void *fdt, int parentoffset, const char *name);
>   */
>  int fdt_del_node(void *fdt, int nodeoffset);
>  
> +/**
> + * fdt_overlay_prepare - Prepares DT overlay for merging with base DT
> + * @fdt: const pointer to the base device tree blob
> + * @fdto: pointer to the device tree overlay blob
> + *
> + * fdt_overlay_prepare() will take care of any resizing/prepartion of the
> + * given device tree overlay.
> + *
> + * returns:
> + *	0, on success
> + *	-FDT_ERR_NOSPACE, there's not enough space in the device tree overlay
> + *	-FDT_ERR_NOTFOUND, the overlay points to some nonexistent nodes or
> + *		properties in the base DT
> + *	-FDT_ERR_BADPHANDLE,
> + *	-FDT_ERR_BADOVERLAY,
> + *	-FDT_ERR_NOPHANDLES,
> + *	-FDT_ERR_INTERNAL,
> + *	-FDT_ERR_BADLAYOUT,
> + *	-FDT_ERR_BADMAGIC,
> + *	-FDT_ERR_BADOFFSET,
> + *	-FDT_ERR_BADPATH,
> + *	-FDT_ERR_BADVERSION,
> + *	-FDT_ERR_BADSTRUCTURE,
> + *	-FDT_ERR_BADSTATE,
> + *	-FDT_ERR_TRUNCATED, standard meanings
> + */
> +int fdt_overlay_prepare(const void *fdt, void *fdto);
> +
>  /**
>   * fdt_overlay_apply - Applies a DT overlay on a base DT
>   * @fdt: pointer to the base device tree blob
> @@ -2244,7 +2272,7 @@ int fdt_del_node(void *fdt, int nodeoffset);
>   *	-FDT_ERR_BADSTATE,
>   *	-FDT_ERR_TRUNCATED, standard meanings
>   */
> -int fdt_overlay_apply(void *fdt, void *fdto);
> +int fdt_overlay_apply(void *fdt, const void *fdto);
>  
>  /**
>   * fdt_overlay_target_offset - retrieves the offset of a fragment's target
> diff --git a/libfdt/version.lds b/libfdt/version.lds
> index 989cd89f1051ce59255a3b3e60493be4e5e985cc..88ef44bb19ec69aabac61a1254264966e0a9d668 100644
> --- a/libfdt/version.lds
> +++ b/libfdt/version.lds
> @@ -66,6 +66,7 @@ LIBFDT_1.2 {
>  		fdt_stringlist_search;
>  		fdt_stringlist_get;
>  		fdt_resize;
> +		fdt_overlay_prepare;
>  		fdt_overlay_apply;
>  		fdt_get_string;
>  		fdt_find_max_phandle;
> 

-- 
David Gibson (he or they)	| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you, not the other way
				| around.
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH 4/5] tests: Fix overlay tests

[Ayush Singh]

Fix tests broken by introduction of fdt_overlay_prepare()

Signed-off-by: Ayush Singh <ayush@beagleboard.org>
---
 tests/overlay.c           | 1 +
 tests/overlay_bad_fixup.c | 2 +-
 2 files changed, 2 insertions(+), 1 deletion(-)

diff --git a/tests/overlay.c b/tests/overlay.c
index 2d27918a336939743c4ef6bd0c9b93d1e9dfc8fb..9d387aba8f85cf1b5a3d397613708b5c82655b68 100644
--- a/tests/overlay.c
+++ b/tests/overlay.c
@@ -203,6 +203,7 @@ int main(int argc, char *argv[])
 	fdt_overlay = open_dt(argv[2]);
 
 	/* Apply the overlay */
+	CHECK(fdt_overlay_prepare(fdt_base, fdt_overlay));
 	CHECK(fdt_overlay_apply(fdt_base, fdt_overlay));
 
 	fdt_overlay_change_int_property(fdt_base);
diff --git a/tests/overlay_bad_fixup.c b/tests/overlay_bad_fixup.c
index 029bc7982d328b0dd538dd30f066ae414b989232..2f7471a60d926a959ac5dfb66ef02712259e1a4d 100644
--- a/tests/overlay_bad_fixup.c
+++ b/tests/overlay_bad_fixup.c
@@ -51,7 +51,7 @@ int main(int argc, char *argv[])
 	fdt_overlay = open_dt(argv[2]);
 
 	/* Apply the overlay */
-	CHECK(fdt_overlay_apply(fdt_base, fdt_overlay), -FDT_ERR_BADOVERLAY);
+	CHECK(fdt_overlay_prepare(fdt_base, fdt_overlay), -FDT_ERR_BADOVERLAY);
 
 	PASS();
 }

-- 
2.47.0

Re: [PATCH 4/5] tests: Fix overlay tests

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 1763 bytes --]

On Sat, Nov 16, 2024 at 08:30:22PM +0530, Ayush Singh wrote:
> Fix tests broken by introduction of fdt_overlay_prepare()

You should fix the tests in the same patch that broke them.

But.. more to the point, I don't see any reason to rename
fdt_overlay_apply() to fdt_overlay_prepare().  And indeed doing so
will break the ABI.

> 
> Signed-off-by: Ayush Singh <ayush@beagleboard.org>
> ---
>  tests/overlay.c           | 1 +
>  tests/overlay_bad_fixup.c | 2 +-
>  2 files changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/tests/overlay.c b/tests/overlay.c
> index 2d27918a336939743c4ef6bd0c9b93d1e9dfc8fb..9d387aba8f85cf1b5a3d397613708b5c82655b68 100644
> --- a/tests/overlay.c
> +++ b/tests/overlay.c
> @@ -203,6 +203,7 @@ int main(int argc, char *argv[])
>  	fdt_overlay = open_dt(argv[2]);
>  
>  	/* Apply the overlay */
> +	CHECK(fdt_overlay_prepare(fdt_base, fdt_overlay));
>  	CHECK(fdt_overlay_apply(fdt_base, fdt_overlay));
>  
>  	fdt_overlay_change_int_property(fdt_base);
> diff --git a/tests/overlay_bad_fixup.c b/tests/overlay_bad_fixup.c
> index 029bc7982d328b0dd538dd30f066ae414b989232..2f7471a60d926a959ac5dfb66ef02712259e1a4d 100644
> --- a/tests/overlay_bad_fixup.c
> +++ b/tests/overlay_bad_fixup.c
> @@ -51,7 +51,7 @@ int main(int argc, char *argv[])
>  	fdt_overlay = open_dt(argv[2]);
>  
>  	/* Apply the overlay */
> -	CHECK(fdt_overlay_apply(fdt_base, fdt_overlay), -FDT_ERR_BADOVERLAY);
> +	CHECK(fdt_overlay_prepare(fdt_base, fdt_overlay), -FDT_ERR_BADOVERLAY);
>  
>  	PASS();
>  }
> 

-- 
David Gibson (he or they)	| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you, not the other way
				| around.
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH 5/5] tests: Add path tests for overlay

[Ayush Singh]

Add tests to verify path reference support in overlays

Signed-off-by: Ayush Singh <ayush@beagleboard.org>
---
 tests/overlay_overlay.dts               | 11 +++++++++++
 tests/overlay_overlay_manual_fixups.dts | 26 +++++++++++++++++++++++++-
 tests/overlay_overlay_nosugar.dts       | 19 +++++++++++++++++++
 3 files changed, 55 insertions(+), 1 deletion(-)

diff --git a/tests/overlay_overlay.dts b/tests/overlay_overlay.dts
index c4ef1d47f1f159c5284c8f7282a0232d944ecfd1..18382762eb3d7c26b0f2e507acc3803f38194fb1 100644
--- a/tests/overlay_overlay.dts
+++ b/tests/overlay_overlay.dts
@@ -50,3 +50,14 @@
 		new-sub-test-property;
 	};
 };
+
+&test {
+	test-patha = &test;
+	test-pathb = &test;
+};
+
+&test {
+	sub-path-test-node {
+		test-path = &test;
+	};
+};
diff --git a/tests/overlay_overlay_manual_fixups.dts b/tests/overlay_overlay_manual_fixups.dts
index a5715b6048acaeebcdab56060040a339d08686a3..c40297aaefaaa6d168e42f864639a05bbbe69f57 100644
--- a/tests/overlay_overlay_manual_fixups.dts
+++ b/tests/overlay_overlay_manual_fixups.dts
@@ -86,6 +86,25 @@
 		};
 	};
 
+  fragment@8 {
+		target = <0xffffffff /*&test*/>;
+
+		__overlay__ {
+			test-patha;
+			test-pathb;
+		};
+	};
+
+	fragment@9 {
+		target = <0xffffffff /*&test*/>;
+
+		__overlay__ {
+			sub-path-test-node {
+				test-path;
+			};
+		};
+	};
+
 	__fixups__ {
 		test = "/fragment@0:target:0",
 		       "/fragment@1:target:0",
@@ -95,7 +114,12 @@
 		       "/fragment@5:target:0",
 		       "/fragment@5/__overlay__:test-phandle:0",
 		       "/fragment@6:target:0",
-		       "/fragment@7:target:0";
+		       "/fragment@7:target:0",
+		       "/fragment@8:target:0",
+		       "/fragment@8/__overlay__:test-patha:0",
+		       "/fragment@8/__overlay__:test-pathb:0",
+		       "/fragment@9:target:0",
+		       "/fragment@9/__overlay__/sub-path-test-node:test-path:0";
 	};
 	__local_fixups__ {
 		fragment@5 {
diff --git a/tests/overlay_overlay_nosugar.dts b/tests/overlay_overlay_nosugar.dts
index b5947e96fb00dcf2c321c9f43e708863053b14b3..7e54ae5f5e7641bdf08626200e9471067b0f7677 100644
--- a/tests/overlay_overlay_nosugar.dts
+++ b/tests/overlay_overlay_nosugar.dts
@@ -83,4 +83,23 @@
 			};
 		};
 	};
+
+	fragment@8 {
+		target = <&test>;
+
+		__overlay__ {
+			test-patha = &test;
+			test-pathb = &test;
+		};
+	};
+
+	fragment@9 {
+		target = <&test>;
+
+		__overlay__ {
+			sub-path-test-node {
+				test-path = &test;
+			};
+		};
+	};
 };

-- 
2.47.0

Re: [PATCH 5/5] tests: Add path tests for overlay

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 3272 bytes --]

On Sat, Nov 16, 2024 at 08:30:23PM +0530, Ayush Singh wrote:
> Add tests to verify path reference support in overlays
> 
> Signed-off-by: Ayush Singh <ayush@beagleboard.org>
> ---
>  tests/overlay_overlay.dts               | 11 +++++++++++
>  tests/overlay_overlay_manual_fixups.dts | 26 +++++++++++++++++++++++++-
>  tests/overlay_overlay_nosugar.dts       | 19 +++++++++++++++++++
>  3 files changed, 55 insertions(+), 1 deletion(-)
> 
> diff --git a/tests/overlay_overlay.dts b/tests/overlay_overlay.dts
> index c4ef1d47f1f159c5284c8f7282a0232d944ecfd1..18382762eb3d7c26b0f2e507acc3803f38194fb1 100644
> --- a/tests/overlay_overlay.dts
> +++ b/tests/overlay_overlay.dts
> @@ -50,3 +50,14 @@
>  		new-sub-test-property;
>  	};
>  };
> +
> +&test {
> +	test-patha = &test;
> +	test-pathb = &test;
> +};
> +
> +&test {
> +	sub-path-test-node {
> +		test-path = &test;
> +	};
> +};

You should test path references combined with other pieces too:

	test-pathc = "a string", &test, "another string";

In fact it would even be a good idea to test path references combined
with phandle references.
	test-pathd = "a string", <0x1 0x2 &test>, &test;

> diff --git a/tests/overlay_overlay_manual_fixups.dts b/tests/overlay_overlay_manual_fixups.dts
> index a5715b6048acaeebcdab56060040a339d08686a3..c40297aaefaaa6d168e42f864639a05bbbe69f57 100644
> --- a/tests/overlay_overlay_manual_fixups.dts
> +++ b/tests/overlay_overlay_manual_fixups.dts
> @@ -86,6 +86,25 @@
>  		};
>  	};
>  
> +  fragment@8 {
> +		target = <0xffffffff /*&test*/>;
> +
> +		__overlay__ {
> +			test-patha;
> +			test-pathb;
> +		};
> +	};
> +
> +	fragment@9 {
> +		target = <0xffffffff /*&test*/>;
> +
> +		__overlay__ {
> +			sub-path-test-node {
> +				test-path;
> +			};
> +		};
> +	};
> +
>  	__fixups__ {
>  		test = "/fragment@0:target:0",
>  		       "/fragment@1:target:0",
> @@ -95,7 +114,12 @@
>  		       "/fragment@5:target:0",
>  		       "/fragment@5/__overlay__:test-phandle:0",
>  		       "/fragment@6:target:0",
> -		       "/fragment@7:target:0";
> +		       "/fragment@7:target:0",
> +		       "/fragment@8:target:0",
> +		       "/fragment@8/__overlay__:test-patha:0",
> +		       "/fragment@8/__overlay__:test-pathb:0",
> +		       "/fragment@9:target:0",
> +		       "/fragment@9/__overlay__/sub-path-test-node:test-path:0";
>  	};
>  	__local_fixups__ {
>  		fragment@5 {
> diff --git a/tests/overlay_overlay_nosugar.dts b/tests/overlay_overlay_nosugar.dts
> index b5947e96fb00dcf2c321c9f43e708863053b14b3..7e54ae5f5e7641bdf08626200e9471067b0f7677 100644
> --- a/tests/overlay_overlay_nosugar.dts
> +++ b/tests/overlay_overlay_nosugar.dts
> @@ -83,4 +83,23 @@
>  			};
>  		};
>  	};
> +
> +	fragment@8 {
> +		target = <&test>;
> +
> +		__overlay__ {
> +			test-patha = &test;
> +			test-pathb = &test;
> +		};
> +	};
> +
> +	fragment@9 {
> +		target = <&test>;
> +
> +		__overlay__ {
> +			sub-path-test-node {
> +				test-path = &test;
> +			};
> +		};
> +	};
>  };
> 

-- 
David Gibson (he or they)	| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you, not the other way
				| around.
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 5/5] tests: Add path tests for overlay

[Ayush Singh]

On 03/12/24 10:16, David Gibson wrote:
> On Sat, Nov 16, 2024 at 08:30:23PM +0530, Ayush Singh wrote:
>> Add tests to verify path reference support in overlays
>>
>> Signed-off-by: Ayush Singh <ayush@beagleboard.org>
>> ---
>>   tests/overlay_overlay.dts               | 11 +++++++++++
>>   tests/overlay_overlay_manual_fixups.dts | 26 +++++++++++++++++++++++++-
>>   tests/overlay_overlay_nosugar.dts       | 19 +++++++++++++++++++
>>   3 files changed, 55 insertions(+), 1 deletion(-)
>>
>> diff --git a/tests/overlay_overlay.dts b/tests/overlay_overlay.dts
>> index c4ef1d47f1f159c5284c8f7282a0232d944ecfd1..18382762eb3d7c26b0f2e507acc3803f38194fb1 100644
>> --- a/tests/overlay_overlay.dts
>> +++ b/tests/overlay_overlay.dts
>> @@ -50,3 +50,14 @@
>>   		new-sub-test-property;
>>   	};
>>   };
>> +
>> +&test {
>> +	test-patha = &test;
>> +	test-pathb = &test;
>> +};
>> +
>> +&test {
>> +	sub-path-test-node {
>> +		test-path = &test;
>> +	};
>> +};
> 
> You should test path references combined with other pieces too:
> 
> 	test-pathc = "a string", &test, "another string";
> 
> In fact it would even be a good idea to test path references combined
> with phandle references.
> 	test-pathd = "a string", <0x1 0x2 &test>, &test;

I have hit a bit of roadblock with the following case:

	test-pathe = "a string", &test1, &test2;

Resolving &test1 will make the poffset for &test2 invalid. So do I need 
to worry about multiple path references in the same property? I do have 
a few ways to deal with this, but maybe I am missing something:

1. Introduce something like `__fixups_path__` where we can use a 
different way to store information regarding path references. I am not 
too keen on going this way though. If we really need to introduce a new 
fixup node, might as well go all the way and have the node work for 
phandles as well.

2. Change the resolution algorithm to maybe create an intermediate tree 
first from `__fixups__` and resolve each property in reverse order in 
the tree. It seems great, until the fact that we cannot use heap comes 
into play. Playing with offsets all over the place has not been great in 
v2 (lots of bugs), and this will just make it harder.

Again, maybe there is a data structure more suited for our case, or 
maybe there is something I missed since I am new to this codebase, so 
thought it would be better to discuss it here.

> 
>> diff --git a/tests/overlay_overlay_manual_fixups.dts b/tests/overlay_overlay_manual_fixups.dts
>> index a5715b6048acaeebcdab56060040a339d08686a3..c40297aaefaaa6d168e42f864639a05bbbe69f57 100644
>> --- a/tests/overlay_overlay_manual_fixups.dts
>> +++ b/tests/overlay_overlay_manual_fixups.dts
>> @@ -86,6 +86,25 @@
>>   		};
>>   	};
>>   
>> +  fragment@8 {
>> +		target = <0xffffffff /*&test*/>;
>> +
>> +		__overlay__ {
>> +			test-patha;
>> +			test-pathb;
>> +		};
>> +	};
>> +
>> +	fragment@9 {
>> +		target = <0xffffffff /*&test*/>;
>> +
>> +		__overlay__ {
>> +			sub-path-test-node {
>> +				test-path;
>> +			};
>> +		};
>> +	};
>> +
>>   	__fixups__ {
>>   		test = "/fragment@0:target:0",
>>   		       "/fragment@1:target:0",
>> @@ -95,7 +114,12 @@
>>   		       "/fragment@5:target:0",
>>   		       "/fragment@5/__overlay__:test-phandle:0",
>>   		       "/fragment@6:target:0",
>> -		       "/fragment@7:target:0";
>> +		       "/fragment@7:target:0",
>> +		       "/fragment@8:target:0",
>> +		       "/fragment@8/__overlay__:test-patha:0",
>> +		       "/fragment@8/__overlay__:test-pathb:0",
>> +		       "/fragment@9:target:0",
>> +		       "/fragment@9/__overlay__/sub-path-test-node:test-path:0";
>>   	};
>>   	__local_fixups__ {
>>   		fragment@5 {
>> diff --git a/tests/overlay_overlay_nosugar.dts b/tests/overlay_overlay_nosugar.dts
>> index b5947e96fb00dcf2c321c9f43e708863053b14b3..7e54ae5f5e7641bdf08626200e9471067b0f7677 100644
>> --- a/tests/overlay_overlay_nosugar.dts
>> +++ b/tests/overlay_overlay_nosugar.dts
>> @@ -83,4 +83,23 @@
>>   			};
>>   		};
>>   	};
>> +
>> +	fragment@8 {
>> +		target = <&test>;
>> +
>> +		__overlay__ {
>> +			test-patha = &test;
>> +			test-pathb = &test;
>> +		};
>> +	};
>> +
>> +	fragment@9 {
>> +		target = <&test>;
>> +
>> +		__overlay__ {
>> +			sub-path-test-node {
>> +				test-path = &test;
>> +			};
>> +		};
>> +	};
>>   };
>>
> 

Ayush Singh

Re: [PATCH 5/5] tests: Add path tests for overlay

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 4642 bytes --]

On Sat, Dec 14, 2024 at 10:15:49AM +0530, Ayush Singh wrote:
> On 03/12/24 10:16, David Gibson wrote:
> > On Sat, Nov 16, 2024 at 08:30:23PM +0530, Ayush Singh wrote:
> > > Add tests to verify path reference support in overlays
> > > 
> > > Signed-off-by: Ayush Singh <ayush@beagleboard.org>
> > > ---
> > >   tests/overlay_overlay.dts               | 11 +++++++++++
> > >   tests/overlay_overlay_manual_fixups.dts | 26 +++++++++++++++++++++++++-
> > >   tests/overlay_overlay_nosugar.dts       | 19 +++++++++++++++++++
> > >   3 files changed, 55 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/tests/overlay_overlay.dts b/tests/overlay_overlay.dts
> > > index c4ef1d47f1f159c5284c8f7282a0232d944ecfd1..18382762eb3d7c26b0f2e507acc3803f38194fb1 100644
> > > --- a/tests/overlay_overlay.dts
> > > +++ b/tests/overlay_overlay.dts
> > > @@ -50,3 +50,14 @@
> > >   		new-sub-test-property;
> > >   	};
> > >   };
> > > +
> > > +&test {
> > > +	test-patha = &test;
> > > +	test-pathb = &test;
> > > +};
> > > +
> > > +&test {
> > > +	sub-path-test-node {
> > > +		test-path = &test;
> > > +	};
> > > +};
> > 
> > You should test path references combined with other pieces too:
> > 
> > 	test-pathc = "a string", &test, "another string";
> > 
> > In fact it would even be a good idea to test path references combined
> > with phandle references.
> > 	test-pathd = "a string", <0x1 0x2 &test>, &test;
> 
> I have hit a bit of roadblock with the following case:
> 
> 	test-pathe = "a string", &test1, &test2;

Oh... good catch.  I felt like I was missing some additional wrinkle
with path substitutions at runtime... and here it is.  Ouch that
complicates things a bunch.

> Resolving &test1 will make the poffset for &test2 invalid. So do I need to
> worry about multiple path references in the same property? I do have a few
> ways to deal with this, but maybe I am missing something:

I really dislike having the arbitrary seeming constraint that you
can't put multiple path substitutions in a single property, especially
since that constraint doesn't apply for compile time substitution.
So, yeah, I think we need to worry about this.

> 1. Introduce something like `__fixups_path__` where we can use a different
> way to store information regarding path references. I am not too keen on
> going this way though. If we really need to introduce a new fixup node,
> might as well go all the way and have the node work for phandles as well.

At least in principle, I like the idea of improving the representation
to handle this case.  However, minimally extending the current format
just for this case doesn't seem like a good idea.  If we have to go
that far, seems like we should go further and clean up a bunch of the
other ugly problems with the current representation.

Note that my being comfortable with adding this feature in (roughly)
the current representation at all was based on missing this serious
complication.

> 2. Change the resolution algorithm to maybe create an intermediate tree
> first from `__fixups__` and resolve each property in reverse order in the
> tree. It seems great, until the fact that we cannot use heap comes into
> play. Playing with offsets all over the place has not been great in v2 (lots
> of bugs), and this will just make it harder.

Yeah, this is really tricky.  The basic problem is that the fixups are
grouped by the target of the reference, not the property into which
we're substituting.  That's quite diffferent from the internal
representation in dtc, where the fixups are represented as "markers"
attached to the property which the substitution is happening.  The
dtbo representation makes it awkward to get the fixups in order; and
not having a heap takes it from awkward to very, very tricky.

Note that it would not be sufficient to order just the path
substitutions we'd need to handle all substitutions - both path and
phandle - in a property in reverse order to correctly handle:
	prop = &target0, <&target1>;

...and, it gets even worse. In the un-substitute property, both of
those references are at offset 0, so the offsets alone aren't enough
information to correctly order the substitutions.  Somehow or other we
need additional information in the dtbo.

Crap.

At this stage, I don't know this feature is feasible without a rather
wider ranging revision of how dtbos are encoded.

-- 
David Gibson (he or they)	| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you, not the other way
				| around.
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 0/5] Add support for resolving path references in overlays

[Ayush Singh]

On 11/16/24 20:30, Ayush Singh wrote:

> dts allows references both in integer context:
> 	foo = <&bar>;
> in which case it resolves to a phandle, but also in string/bytestring
> context:
> 	foo = &bar;
> In which case it resolves to a path.
>
> Runtime overlays, only support the former, but not the latter. The
> following patch attempts to solve this asymmetry.
>
> Additionally, `__symbols__` does not support phandles, which
> makes overlays modifying `__symbols__` rather limiting. More context
> regarding this patch can be found here[0].
>
> Implementation
> **************
>
> Overlay
> =======
>
> Properties to path references in overlays are left empty in the compiled
> binary blob. Only `__fixups__` entry is generated for such properties.
> This makes it simple to distinguish between phandles and paths when
> applying the overlay (and also makes overlays small).
>
> Application
> ===========
>
> I have divided the overlay application into 2 stages.
>
> 1. Overlay prepare (`fdt_overlay_prepare`)
>
> This step prepares the overlay for merging with the base device tree. In
> At this stage, the base device tree is passed as read-only and only the
> overlay is modified and resized.
>
> Additionally, since any resizing will invalidate the offsets and
> property references, I am creating a read-only copy of the table while
> resolving the path references.
>
> 2. Overlay application (`fdt_overlay_apply`)
>
> Performs the actual merging to base tree. The overlay is read-only at this
> stage.
>
> Limitations
> ***********
>
> 1. Local Path references
>
> Currently, this patch series only implements path references to base
> devicetree. This means local path references are still broken. I am
> working on adding support for that using `__local_fixups__` but it is
> not ready yet.
>
> 2. Breaking change for utilities
>
> This is a breaking change for any utilities using `fdt_overlay_apply`
> since I have moved some of it's functionality to `fdt_overlay_prepare`.
> Not really sure how important this is.
>
> If it is not desirable, I do have 2 ways to avoid it.
>
>    a. Just expand both overlay and base device tree when `FDT_ERR_NOSPACE`
>       is returned. A bit wasteful, but probably not a big deal.
>
>    b. Add new error variant `FDT_ERR_OVERLAY_NOSPACE`.
>
> Alternatives
> ************
>
> Some alternative approaches that were considered:
>
> 1. Using aliases.
>
> Currently, it is not possible to update aliases in device tree overlays.
> I sent a patch a few weeks ago to add this support [1]. However, as was
> outlined by Rob, this can break existing drivers that used the unused
> indexes for devices not present in the aliases list.
>
> 2. Add support for phandles in `__symbols__`
>
> This has been discussed in the following patch series [2]. However,
> since there is no way to distinguish between strings and phandles in
> devicetree (everything is bytestring), the type guessing is awkward.
>
> [0]: https://lore.kernel.org/devicetree-compiler/44bfc9b3-8282-4cc7-8d9a-7292cac663ef@ti.com/T/#mf0f6ae4db0848f725ec6e2fb625291fa0d4eec71
> [1]: https://lore.kernel.org/all/20241110-of-alias-v2-0-16da9844a93e@beagleboard.org/T/#t
> [2]: https://lore.kernel.org/devicetree-compiler/44bfc9b3-8282-4cc7-8d9a-7292cac663ef@ti.com/T/#mbbc181b0ef394b85b76b2024d7e209ebe70f7003
>
> Signed-off-by: Ayush Singh <ayush@beagleboard.org>
>
> ---
> Ayush Singh (5):
>        dtc: Allow path fixups in overlays
>        libfdt: Add namelen variants for setprop
>        fdtoverlay: Implement resolving path references
>        tests: Fix overlay tests
>        tests: Add path tests for overlay
>
>   checks.c                                |   6 +-
>   fdtoverlay.c                            |  34 +++++--
>   libfdt/fdt_overlay.c                    | 174 +++++++++++++++++++-------------
>   libfdt/fdt_rw.c                         |  19 ++++
>   libfdt/libfdt.h                         |  93 ++++++++++++++++-
>   libfdt/version.lds                      |   1 +
>   livetree.c                              |  19 +++-
>   tests/overlay.c                         |   1 +
>   tests/overlay_bad_fixup.c               |   2 +-
>   tests/overlay_overlay.dts               |  11 ++
>   tests/overlay_overlay_manual_fixups.dts |  26 ++++-
>   tests/overlay_overlay_nosugar.dts       |  19 ++++
>   util.h                                  |   1 +
>   13 files changed, 321 insertions(+), 85 deletions(-)
> ---
> base-commit: 2d10aa2afe35527728db30b35ec491ecb6959e5c
> change-id: 20241114-overlay-path-d9980477f76a
>
> Best regards,


cc David Gibson <david@gibson.dropbear.id.au>


Sorry for missing the maintainer of dtc and fdt in the initial patches.


Ayush Singh