[PATCHv2 0/2] overlays: auto allocate phandles for nodes in base fdt

[PATCHv2 0/2] overlays: auto allocate phandles for nodes in base fdt

[kevans-HZy0K5TPuP5AfugRpC6u6w]

Nodes in the base fdt that do not have phandles cannot currently be referenced
in overlays; we can target them for an overlay, but we cannot use them as a
reference in another property if needed. This is quite limiting for some
use-cases where you're needing cross-references that don't currently exist in
the base.

Kyle Evans (2):
  fdt_overlay: Allocate phandles as needed for nodes referenced in base
    fdt
  fdt_overlay: Basic regression tests for automatically allocated
    phandles

 libfdt/fdt_overlay.c                               | 83 +++++++++++++++++++---
 tests/overlay_base_manual_symbols_auto_phandle.dts | 24 +++++++
 tests/run_tests.sh                                 |  6 ++
 3 files changed, 104 insertions(+), 9 deletions(-)
 create mode 100644 tests/overlay_base_manual_symbols_auto_phandle.dts

-- 
2.15.1

[PATCH v2 1/2] overlays: auto allocate phandles for nodes in base fdt

[kevans-HZy0K5TPuP5AfugRpC6u6w]

Currently, references cannot be made to nodes in the base that do not already
have phandles, limiting us to nodes that have been referenced in the base fdt.
Lift that restriction by allocating them on an as-needed basis.

Signed-off-by: Kyle Evans <kevans-HZy0K5TPuP5AfugRpC6u6w@public.gmane.org>
---

Changes since v1:
 - Added a function to grab the next phandle; once we've assigned one phandle
 to a node automatically, we'll need to choose max phandle from the base
 blob instead of the overlay since they've not necessarily been merged yet.

 libfdt/fdt_overlay.c | 83 ++++++++++++++++++++++++++++++++++++++++++++++------
 1 file changed, 74 insertions(+), 9 deletions(-)

diff --git a/libfdt/fdt_overlay.c b/libfdt/fdt_overlay.c
index bd81241..10a57ae 100644
--- a/libfdt/fdt_overlay.c
+++ b/libfdt/fdt_overlay.c
@@ -335,6 +335,66 @@ static int overlay_update_local_references(void *fdto, uint32_t delta)
 						    delta);
 }
 
+/**
+ * overlay_next_phandle - Grab next phandle to be assigned
+ * @fdt: Base Device Tree blob
+ * @fdto: Device tree overlay blob
+ *
+ * overlay_next_phandle() determines the next phandle to be assigned to the
+ * fdt blob.
+ *
+ * This is part of the device tree overlay application process, when you need to
+ * assign a phandle to a node that doesn't currently have one.
+ *
+ * returns:
+ *      Next phandle to be assigned on success
+ *      Negative error code on failure
+ */
+static int overlay_next_phandle(void *fdt, void *fdto)
+{
+	int base_max, overlay_max;
+
+	base_max = fdt_get_max_phandle(fdt);
+	if (base_max < 0)
+		return base_max;
+	overlay_max = fdt_get_max_phandle(fdto);
+	if (overlay_max < 0)
+		return overlay_max;
+	return (base_max > overlay_max ? base_max : overlay_max) + 1;
+}
+
+/**
+ * overlay_assign_phandle - Assign a phandle to a symbol in the base fdt
+ * @fdt: Base Device Tree blob
+ * @fdto: Device tree overlay blob
+ * @symbol_off: Node offset of the symbol to be assigned a phandle
+ *
+ * overlay_assign_phandle() assigns the next phandle available to the requested
+ * node in the base device tree.
+ *
+ * This is part of the device tree overlay application process, when you want to
+ * reference a symbol in the base device tree that doesn't yet have a phandle.
+ *
+ * returns:
+ *      phandle assigned on success
+ *      0 on failure
+ */
+static int overlay_assign_phandle(void *fdt, void *fdto, int symbol_off)
+{
+	int phandle, ret;
+	fdt32_t phandle_val;
+
+	/* Overlay phandles have already been adjusted, grab highest phandle */
+	phandle = overlay_next_phandle(fdt, fdto);
+	if (phandle < 0)
+		return 0;
+	phandle_val = cpu_to_fdt32(phandle);
+	ret = fdt_setprop(fdt, symbol_off, "phandle", &phandle_val, sizeof(phandle_val));
+	if (!ret)
+		return phandle;
+	return 0;
+}
+
 /**
  * overlay_fixup_one_phandle - Set an overlay phandle to the base one
  * @fdt: Base Device Tree blob
@@ -359,7 +419,7 @@ static int overlay_update_local_references(void *fdto, uint32_t delta)
  *      Negative error code on failure
  */
 static int overlay_fixup_one_phandle(void *fdt, void *fdto,
-				     int symbols_off,
+				     int *symbols_off,
 				     const char *path, uint32_t path_len,
 				     const char *name, uint32_t name_len,
 				     int poffset, const char *label)
@@ -370,10 +430,10 @@ static int overlay_fixup_one_phandle(void *fdt, void *fdto,
 	int symbol_off, fixup_off;
 	int prop_len;
 
-	if (symbols_off < 0)
-		return symbols_off;
+	if (*symbols_off < 0)
+		return *symbols_off;
 
-	symbol_path = fdt_getprop(fdt, symbols_off, label,
+	symbol_path = fdt_getprop(fdt, *symbols_off, label,
 				  &prop_len);
 	if (!symbol_path)
 		return prop_len;
@@ -383,8 +443,14 @@ static int overlay_fixup_one_phandle(void *fdt, void *fdto,
 		return symbol_off;
 
 	phandle = fdt_get_phandle(fdt, symbol_off);
-	if (!phandle)
-		return -FDT_ERR_NOTFOUND;
+	if (!phandle) {
+		phandle = overlay_assign_phandle(fdt, fdto, symbol_off);
+		if (phandle == 0)
+			return -FDT_ERR_NOTFOUND;
+
+		/* Re-lookup symbols offset, it's been invalidated */
+		*symbols_off = fdt_path_offset(fdt, "/__symbols__");
+	}
 
 	fixup_off = fdt_path_offset_namelen(fdto, path, path_len);
 	if (fixup_off == -FDT_ERR_NOTFOUND)
@@ -418,7 +484,7 @@ static int overlay_fixup_one_phandle(void *fdt, void *fdto,
  *      0 on success
  *      Negative error code on failure
  */
-static int overlay_fixup_phandle(void *fdt, void *fdto, int symbols_off,
+static int overlay_fixup_phandle(void *fdt, void *fdto, int *symbols_off,
 				 int property)
 {
 	const char *value;
@@ -519,8 +585,7 @@ static int overlay_fixup_phandles(void *fdt, void *fdto)
 
 	fdt_for_each_property_offset(property, fdto, fixups_off) {
 		int ret;
-
-		ret = overlay_fixup_phandle(fdt, fdto, symbols_off, property);
+		ret = overlay_fixup_phandle(fdt, fdto, &symbols_off, property);
 		if (ret)
 			return ret;
 	}
-- 
2.15.1

[PATCH v2 2/2] overlays: auto allocate phandles for nodes in base fdt

[kevans-HZy0K5TPuP5AfugRpC6u6w]

Add some regression tests for automatically allocated phandles. The base dts is
simply a copy of overlay_base_manual_symbols.dts with the forced phandle
removed, thus copyright notices remain intact.

Signed-off-by: Kyle Evans <kevans-HZy0K5TPuP5AfugRpC6u6w@public.gmane.org>
---
 tests/overlay_base_manual_symbols_auto_phandle.dts | 24 ++++++++++++++++++++++
 tests/run_tests.sh                                 |  6 ++++++
 2 files changed, 30 insertions(+)
 create mode 100644 tests/overlay_base_manual_symbols_auto_phandle.dts

diff --git a/tests/overlay_base_manual_symbols_auto_phandle.dts b/tests/overlay_base_manual_symbols_auto_phandle.dts
new file mode 100644
index 0000000..6acdcf8
--- /dev/null
+++ b/tests/overlay_base_manual_symbols_auto_phandle.dts
@@ -0,0 +1,24 @@
+/*
+ * Copyright (c) 2016 NextThing Co
+ * Copyright (c) 2016 Free Electrons
+ *
+ * SPDX-License-Identifier:	GPL-2.0+
+ */
+
+/dts-v1/;
+
+/ {
+	test: test-node {
+		test-int-property = <42>;
+		test-str-property = "foo";
+
+		subtest: sub-test-node {
+			sub-test-property;
+		};
+	};
+	__symbols__ {
+		test = &test;
+	};
+};
+
+
diff --git a/tests/run_tests.sh b/tests/run_tests.sh
index d36dffb..6ed6d02 100755
--- a/tests/run_tests.sh
+++ b/tests/run_tests.sh
@@ -198,12 +198,18 @@ libfdt_overlay_tests () {
     run_test check_path overlay_base_manual_symbols.test.dtb not-exists "/__fixups__"
     run_test check_path overlay_base_manual_symbols.test.dtb not-exists "/__local_fixups__"
 
+    run_dtc_test -I dts -O dtb -o overlay_base_manual_symbols_auto_phandle.test.dtb overlay_base_manual_symbols_auto_phandle.dts
+    run_test check_path overlay_base_manual_symbols_auto_phandle.test.dtb exists "/__symbols__"
+    run_test check_path overlay_base_manual_symbols_auto_phandle.test.dtb not-exists "/__fixups__"
+    run_test check_path overlay_base_manual_symbols_auto_phandle.test.dtb not-exists "/__local_fixups__"
+
     run_dtc_test -I dts -O dtb -o overlay_overlay_manual_fixups.test.dtb overlay_overlay_manual_fixups.dts
     run_test check_path overlay_overlay_manual_fixups.test.dtb not-exists "/__symbols__"
     run_test check_path overlay_overlay_manual_fixups.test.dtb exists "/__fixups__"
     run_test check_path overlay_overlay_manual_fixups.test.dtb exists "/__local_fixups__"
 
     run_test overlay overlay_base_manual_symbols.test.dtb overlay_overlay_manual_fixups.test.dtb
+    run_test overlay overlay_base_manual_symbols_auto_phandle.test.dtb overlay_overlay_manual_fixups.test.dtb
 
     # test simplified plugin syntax
     run_dtc_test -@ -I dts -O dtb -o overlay_overlay_simple.dtb overlay_overlay_simple.dts
-- 
2.15.1

Re: [PATCH v2 1/2] overlays: auto allocate phandles for nodes in base fdt

[David Gibson]

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

Regardless of anything else, these two patches need different one-line
summaries.

On Mon, Jan 01, 2018 at 12:59:44AM -0600, kevans-HZy0K5TPuP5AfugRpC6u6w@public.gmane.org wrote:
> Currently, references cannot be made to nodes in the base that do not already
> have phandles, limiting us to nodes that have been referenced in the base fdt.
> Lift that restriction by allocating them on an as-needed basis.

Hmm.  I'm a bit dubious about this.

- My feeling is that one of the problems with the overlay format is
  that it's already too free, allowing the overlay to change
  essentially anything in the base tree.  So I'm not that keen on
  making it even more free.

- An overlay can already add a 'phandle' property to a node in the
  base tree.  Can you use that directly instead of adding a new
  mechanism?


> Signed-off-by: Kyle Evans <kevans-HZy0K5TPuP5AfugRpC6u6w@public.gmane.org>
> ---
> 
> Changes since v1:
>  - Added a function to grab the next phandle; once we've assigned one phandle
>  to a node automatically, we'll need to choose max phandle from the base
>  blob instead of the overlay since they've not necessarily been merged yet.
> 
>  libfdt/fdt_overlay.c | 83 ++++++++++++++++++++++++++++++++++++++++++++++------
>  1 file changed, 74 insertions(+), 9 deletions(-)
> 
> diff --git a/libfdt/fdt_overlay.c b/libfdt/fdt_overlay.c
> index bd81241..10a57ae 100644
> --- a/libfdt/fdt_overlay.c
> +++ b/libfdt/fdt_overlay.c
> @@ -335,6 +335,66 @@ static int overlay_update_local_references(void *fdto, uint32_t delta)
>  						    delta);
>  }
>  
> +/**
> + * overlay_next_phandle - Grab next phandle to be assigned
> + * @fdt: Base Device Tree blob
> + * @fdto: Device tree overlay blob
> + *
> + * overlay_next_phandle() determines the next phandle to be assigned to the
> + * fdt blob.
> + *
> + * This is part of the device tree overlay application process, when you need to
> + * assign a phandle to a node that doesn't currently have one.
> + *
> + * returns:
> + *      Next phandle to be assigned on success
> + *      Negative error code on failure
> + */
> +static int overlay_next_phandle(void *fdt, void *fdto)
> +{
> +	int base_max, overlay_max;
> +
> +	base_max = fdt_get_max_phandle(fdt);
> +	if (base_max < 0)
> +		return base_max;
> +	overlay_max = fdt_get_max_phandle(fdto);
> +	if (overlay_max < 0)
> +		return overlay_max;
> +	return (base_max > overlay_max ? base_max : overlay_max) + 1;
> +}

This is deceptively expensive, doing a full scan of both base and
overlay blobs whenever you want to get a new phandle.  I also don't
think it's quite necessary: I'm pretty sure the way we adjust the
overlay phandles they always have to be greater than those in the base
tree.  You could find the maximum phandle of the (adjusted) overlay
while evaluating overlay_adjust_local_phandles().

That would let you generate new phandles without extra full scans
through the blobs.

> +
> +/**
> + * overlay_assign_phandle - Assign a phandle to a symbol in the base fdt
> + * @fdt: Base Device Tree blob
> + * @fdto: Device tree overlay blob
> + * @symbol_off: Node offset of the symbol to be assigned a phandle

I don't like that name - this is is the offset of the node that the
symbol references, not the symbol itself (which is a property).

> + *
> + * overlay_assign_phandle() assigns the next phandle available to the requested
> + * node in the base device tree.
> + *
> + * This is part of the device tree overlay application process, when you want to
> + * reference a symbol in the base device tree that doesn't yet have a phandle.
> + *
> + * returns:
> + *      phandle assigned on success
> + *      0 on failure
> + */
> +static int overlay_assign_phandle(void *fdt, void *fdto, int symbol_off)
> +{
> +	int phandle, ret;
> +	fdt32_t phandle_val;
> +
> +	/* Overlay phandles have already been adjusted, grab highest phandle */
> +	phandle = overlay_next_phandle(fdt, fdto);
> +	if (phandle < 0)
> +		return 0;
> +	phandle_val = cpu_to_fdt32(phandle);
> +	ret = fdt_setprop(fdt, symbol_off, "phandle", &phandle_val, sizeof(phandle_val));
> +	if (!ret)
> +		return phandle;

You can use fdt_setprop_u32() to avoid the manual byteswap.

> +	return 0;
> +}
> +
>  /**
>   * overlay_fixup_one_phandle - Set an overlay phandle to the base one
>   * @fdt: Base Device Tree blob
> @@ -359,7 +419,7 @@ static int overlay_update_local_references(void *fdto, uint32_t delta)
>   *      Negative error code on failure
>   */
>  static int overlay_fixup_one_phandle(void *fdt, void *fdto,
> -				     int symbols_off,
> +				     int *symbols_off,
>  				     const char *path, uint32_t path_len,
>  				     const char *name, uint32_t name_len,
>  				     int poffset, const char *label)
> @@ -370,10 +430,10 @@ static int overlay_fixup_one_phandle(void *fdt, void *fdto,
>  	int symbol_off, fixup_off;
>  	int prop_len;
>  
> -	if (symbols_off < 0)
> -		return symbols_off;
> +	if (*symbols_off < 0)
> +		return *symbols_off;
>  
> -	symbol_path = fdt_getprop(fdt, symbols_off, label,
> +	symbol_path = fdt_getprop(fdt, *symbols_off, label,
>  				  &prop_len);
>  	if (!symbol_path)
>  		return prop_len;
> @@ -383,8 +443,14 @@ static int overlay_fixup_one_phandle(void *fdt, void *fdto,
>  		return symbol_off;
>  
>  	phandle = fdt_get_phandle(fdt, symbol_off);
> -	if (!phandle)
> -		return -FDT_ERR_NOTFOUND;
> +	if (!phandle) {
> +		phandle = overlay_assign_phandle(fdt, fdto, symbol_off);
> +		if (phandle == 0)
> +			return -FDT_ERR_NOTFOUND;
> +
> +		/* Re-lookup symbols offset, it's been invalidated */
> +		*symbols_off = fdt_path_offset(fdt, "/__symbols__");
> +	}
>  
>  	fixup_off = fdt_path_offset_namelen(fdto, path, path_len);
>  	if (fixup_off == -FDT_ERR_NOTFOUND)
> @@ -418,7 +484,7 @@ static int overlay_fixup_one_phandle(void *fdt, void *fdto,
>   *      0 on success
>   *      Negative error code on failure
>   */
> -static int overlay_fixup_phandle(void *fdt, void *fdto, int symbols_off,
> +static int overlay_fixup_phandle(void *fdt, void *fdto, int *symbols_off,
>  				 int property)
>  {
>  	const char *value;
> @@ -519,8 +585,7 @@ static int overlay_fixup_phandles(void *fdt, void *fdto)
>  
>  	fdt_for_each_property_offset(property, fdto, fixups_off) {
>  		int ret;
> -
> -		ret = overlay_fixup_phandle(fdt, fdto, symbols_off, property);
> +		ret = overlay_fixup_phandle(fdt, fdto, &symbols_off, property);
>  		if (ret)
>  			return ret;
>  	}

-- 
David Gibson			| 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 v2 1/2] overlays: auto allocate phandles for nodes in base fdt

[Kyle Evans]

[Resending with a proper mail client, because it didn't go to the lists]

On Tue, Jan 2, 2018 at 11:42 PM, David Gibson
<david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org> wrote:
> Regardless of anything else, these two patches need different one-line
> summaries.
>
> On Mon, Jan 01, 2018 at 12:59:44AM -0600, kevans-HZy0K5TPuP5AfugRpC6u6w@public.gmane.org wrote:
>> Currently, references cannot be made to nodes in the base that do not already
>> have phandles, limiting us to nodes that have been referenced in the base fdt.
>> Lift that restriction by allocating them on an as-needed basis.
>
> Hmm.  I'm a bit dubious about this.
>
> - My feeling is that one of the problems with the overlay format is
>   that it's already too free, allowing the overlay to change
>   essentially anything in the base tree.  So I'm not that keen on
>   making it even more free.
>
> - An overlay can already add a 'phandle' property to a node in the
>   base tree.  Can you use that directly instead of adding a new
>   mechanism?
>

That feels like it might be a bit difficult to work with, for a couple
of reasons that might be logically wrong, so forgive me if I'm
thinking wrong:

 - A phandle is just a number, so it won't get adjusted as overlays
get applied. All of the overlays that one of my boards needs to apply
would need to choose sufficiently high phandles that they hopefully
won't conflict with other overlays, especially as new nodes get
introduced with their own phandles.

 - If more than one overlay needs to reference a specific node, we
have other conflicts. These overlays aren't necessarily related, but
they would need to agree with each other on the phandle of the node OR
make sure they apply in a specific order so one can add the phandle
and subsequent overlays can reference them symbolically only as
intended.

The problem is that these overlays aren't necessarily curated by a
single authority, so conflicts with multiple overlays trying to
reference a node is scary. Ideally, we'd like these things to be as
usable as possible for average Joe Blow.

Re: [PATCH v2 1/2] overlays: auto allocate phandles for nodes in base fdt

[David Gibson]

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

On Wed, Jan 03, 2018 at 08:04:52AM -0600, Kyle Evans wrote:
> [Resending with a proper mail client, because it didn't go to the lists]
> 
> On Tue, Jan 2, 2018 at 11:42 PM, David Gibson
> <david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org> wrote:
> > Regardless of anything else, these two patches need different one-line
> > summaries.
> >
> > On Mon, Jan 01, 2018 at 12:59:44AM -0600, kevans-HZy0K5TPuP5AfugRpC6u6w@public.gmane.org wrote:
> >> Currently, references cannot be made to nodes in the base that do not already
> >> have phandles, limiting us to nodes that have been referenced in the base fdt.
> >> Lift that restriction by allocating them on an as-needed basis.
> >
> > Hmm.  I'm a bit dubious about this.
> >
> > - My feeling is that one of the problems with the overlay format is
> >   that it's already too free, allowing the overlay to change
> >   essentially anything in the base tree.  So I'm not that keen on
> >   making it even more free.
> >
> > - An overlay can already add a 'phandle' property to a node in the
> >   base tree.  Can you use that directly instead of adding a new
> >   mechanism?
> >
> 
> That feels like it might be a bit difficult to work with, for a couple
> of reasons that might be logically wrong, so forgive me if I'm
> thinking wrong:
> 
>  - A phandle is just a number, so it won't get adjusted as overlays
> get applied. All of the overlays that one of my boards needs to apply
> would need to choose sufficiently high phandles that they hopefully
> won't conflict with other overlays, especially as new nodes get
> introduced with their own phandles.

So, I think that could be handled by applying a local fixup to it in
the overlay which would offset it correctly.  I'd have to work through
the details, but I won't bother because..

>  - If more than one overlay needs to reference a specific node, we
> have other conflicts. These overlays aren't necessarily related, but
> they would need to agree with each other on the phandle of the node OR
> make sure they apply in a specific order so one can add the phandle
> and subsequent overlays can reference them symbolically only as
> intended.

..that's a good point.  Adding the phandle in the overlay effectively
*requires* that the phandle be missing in the base, which gets really
messy if you have a bunch of overlays to juggle.

You could maybe handle it by handling "base tree fixes" as a separate
overlay which must be applied first before the "real" overlays, but at
that point your solution is probably a better one.

> The problem is that these overlays aren't necessarily curated by a
> single authority, so conflicts with multiple overlays trying to
> reference a node is scary. Ideally, we'd like these things to be as
> usable as possible for average Joe Blow.

Yeah, fair enough.  I'll re-evaluate your patches with that in mind.

-- 
David Gibson			| 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 v2 1/2] overlays: auto allocate phandles for nodes in base fdt

[Kyle Evans]

On Wed, Jan 3, 2018 at 9:26 PM, David Gibson
<david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org> wrote:
> On Wed, Jan 03, 2018 at 08:04:52AM -0600, Kyle Evans wrote:
>> [... snip ...]
>> The problem is that these overlays aren't necessarily curated by a
>> single authority, so conflicts with multiple overlays trying to
>> reference a node is scary. Ideally, we'd like these things to be as
>> usable as possible for average Joe Blow.
>
> Yeah, fair enough.  I'll re-evaluate your patches with that in mind.

I appreciate your consideration, thank you. =) I'm not necessarily too
keen on requiring our users to likely have to disassemble every
overlay they receive or consider which order they apply in if they're
not functionally related.

It also helps me out as I work on hardware that doesn't have complete
DTS in mainline Linux yet, so I'm frequently adding nodes via overlay
that reference base nodes not yet with phandles. Adding phandles to
everything as I go gets kind of messy in my overlays and makes it that
much more difficult to assemble an upstreamable patch.

I've also had fleeting thoughts of writing a tool like dtdiff, but
will actually generate an overlay of the difference between the two
dts being compared. The main use case here being that we follow Linux
releases (not -rc) for our dts, so automatically generating a DTBO
from where we're at to the next stable release (perhaps in the later
-rc stage) would be really really helpful to evaluate if we still work
and what we need to fix.

Re: [PATCH v2 1/2] overlays: auto allocate phandles for nodes in base fdt

[Frank Rowand]

On 12/31/17 22:59, kevans-HZy0K5TPuP5AfugRpC6u6w@public.gmane.org wrote:
> Currently, references cannot be made to nodes in the base that do not already
> have phandles, limiting us to nodes that have been referenced in the base fdt.
> Lift that restriction by allocating them on an as-needed basis.

I am very confused.  If I understand correctly, the problem is:

  A base devicetree contains a __symbols__ node, where one of the properties
  in that node has a value which is a path to a node, where that node does
  not have a phandle property.

Is that correct?

> 
> Signed-off-by: Kyle Evans <kevans-HZy0K5TPuP5AfugRpC6u6w@public.gmane.org>
> ---
> 
> Changes since v1:
>  - Added a function to grab the next phandle; once we've assigned one phandle
>  to a node automatically, we'll need to choose max phandle from the base
>  blob instead of the overlay since they've not necessarily been merged yet.
> 
>  libfdt/fdt_overlay.c | 83 ++++++++++++++++++++++++++++++++++++++++++++++------
>  1 file changed, 74 insertions(+), 9 deletions(-)
> 
> diff --git a/libfdt/fdt_overlay.c b/libfdt/fdt_overlay.c
> index bd81241..10a57ae 100644
> --- a/libfdt/fdt_overlay.c
> +++ b/libfdt/fdt_overlay.c
> @@ -335,6 +335,66 @@ static int overlay_update_local_references(void *fdto, uint32_t delta)
>  						    delta);
>  }
>  
> +/**
> + * overlay_next_phandle - Grab next phandle to be assigned
> + * @fdt: Base Device Tree blob
> + * @fdto: Device tree overlay blob
> + *
> + * overlay_next_phandle() determines the next phandle to be assigned to the
> + * fdt blob.
> + *
> + * This is part of the device tree overlay application process, when you need to
> + * assign a phandle to a node that doesn't currently have one.
> + *
> + * returns:
> + *      Next phandle to be assigned on success
> + *      Negative error code on failure
> + */
> +static int overlay_next_phandle(void *fdt, void *fdto)
> +{
> +	int base_max, overlay_max;
> +
> +	base_max = fdt_get_max_phandle(fdt);
> +	if (base_max < 0)
> +		return base_max;
> +	overlay_max = fdt_get_max_phandle(fdto);
> +	if (overlay_max < 0)
> +		return overlay_max;
> +	return (base_max > overlay_max ? base_max : overlay_max) + 1;
> +}
> +
> +/**
> + * overlay_assign_phandle - Assign a phandle to a symbol in the base fdt
> + * @fdt: Base Device Tree blob
> + * @fdto: Device tree overlay blob
> + * @symbol_off: Node offset of the symbol to be assigned a phandle
> + *
> + * overlay_assign_phandle() assigns the next phandle available to the requested
> + * node in the base device tree.
> + *
> + * This is part of the device tree overlay application process, when you want to
> + * reference a symbol in the base device tree that doesn't yet have a phandle.
> + *
> + * returns:
> + *      phandle assigned on success
> + *      0 on failure
> + */
> +static int overlay_assign_phandle(void *fdt, void *fdto, int symbol_off)
> +{
> +	int phandle, ret;
> +	fdt32_t phandle_val;
> +
> +	/* Overlay phandles have already been adjusted, grab highest phandle */
> +	phandle = overlay_next_phandle(fdt, fdto);
> +	if (phandle < 0)
> +		return 0;
> +	phandle_val = cpu_to_fdt32(phandle);
> +	ret = fdt_setprop(fdt, symbol_off, "phandle", &phandle_val, sizeof(phandle_val));
> +	if (!ret)
> +		return phandle;
> +	return 0;
> +}
> +
>  /**
>   * overlay_fixup_one_phandle - Set an overlay phandle to the base one
>   * @fdt: Base Device Tree blob
> @@ -359,7 +419,7 @@ static int overlay_update_local_references(void *fdto, uint32_t delta)
>   *      Negative error code on failure
>   */
>  static int overlay_fixup_one_phandle(void *fdt, void *fdto,
> -				     int symbols_off,
> +				     int *symbols_off,
>  				     const char *path, uint32_t path_len,
>  				     const char *name, uint32_t name_len,
>  				     int poffset, const char *label)
> @@ -370,10 +430,10 @@ static int overlay_fixup_one_phandle(void *fdt, void *fdto,
>  	int symbol_off, fixup_off;
>  	int prop_len;
>  
> -	if (symbols_off < 0)
> -		return symbols_off;
> +	if (*symbols_off < 0)
> +		return *symbols_off;
>  
> -	symbol_path = fdt_getprop(fdt, symbols_off, label,
> +	symbol_path = fdt_getprop(fdt, *symbols_off, label,
>  				  &prop_len);
>  	if (!symbol_path)
>  		return prop_len;
> @@ -383,8 +443,14 @@ static int overlay_fixup_one_phandle(void *fdt, void *fdto,
>  		return symbol_off;
>  
>  	phandle = fdt_get_phandle(fdt, symbol_off);
> -	if (!phandle)
> -		return -FDT_ERR_NOTFOUND;
> +	if (!phandle) {
> +		phandle = overlay_assign_phandle(fdt, fdto, symbol_off);
> +		if (phandle == 0)
> +			return -FDT_ERR_NOTFOUND;
> +
> +		/* Re-lookup symbols offset, it's been invalidated */
> +		*symbols_off = fdt_path_offset(fdt, "/__symbols__");
> +	}
>  
>  	fixup_off = fdt_path_offset_namelen(fdto, path, path_len);
>  	if (fixup_off == -FDT_ERR_NOTFOUND)
> @@ -418,7 +484,7 @@ static int overlay_fixup_one_phandle(void *fdt, void *fdto,
>   *      0 on success
>   *      Negative error code on failure
>   */
> -static int overlay_fixup_phandle(void *fdt, void *fdto, int symbols_off,
> +static int overlay_fixup_phandle(void *fdt, void *fdto, int *symbols_off,
>  				 int property)
>  {
>  	const char *value;
> @@ -519,8 +585,7 @@ static int overlay_fixup_phandles(void *fdt, void *fdto)
>  
>  	fdt_for_each_property_offset(property, fdto, fixups_off) {
>  		int ret;
> -
> -		ret = overlay_fixup_phandle(fdt, fdto, symbols_off, property);
> +		ret = overlay_fixup_phandle(fdt, fdto, &symbols_off, property);
>  		if (ret)
>  			return ret;
>  	}
> 

Re: [PATCH v2 1/2] overlays: auto allocate phandles for nodes in base fdt

[David Gibson]

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

On Thu, Jan 04, 2018 at 08:21:34AM -0600, Kyle Evans wrote:
> On Wed, Jan 3, 2018 at 9:26 PM, David Gibson
> <david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org> wrote:
> > On Wed, Jan 03, 2018 at 08:04:52AM -0600, Kyle Evans wrote:
> >> [... snip ...]
> >> The problem is that these overlays aren't necessarily curated by a
> >> single authority, so conflicts with multiple overlays trying to
> >> reference a node is scary. Ideally, we'd like these things to be as
> >> usable as possible for average Joe Blow.
> >
> > Yeah, fair enough.  I'll re-evaluate your patches with that in mind.
> 
> I appreciate your consideration, thank you. =) I'm not necessarily too
> keen on requiring our users to likely have to disassemble every
> overlay they receive or consider which order they apply in if they're
> not functionally related.
> 
> It also helps me out as I work on hardware that doesn't have complete
> DTS in mainline Linux yet, so I'm frequently adding nodes via overlay
> that reference base nodes not yet with phandles. Adding phandles to
> everything as I go gets kind of messy in my overlays and makes it that
> much more difficult to assemble an upstreamable patch.
> 
> I've also had fleeting thoughts of writing a tool like dtdiff, but
> will actually generate an overlay of the difference between the two
> dts being compared. The main use case here being that we follow Linux
> releases (not -rc) for our dts, so automatically generating a DTBO
> from where we're at to the next stable release (perhaps in the later
> -rc stage) would be really really helpful to evaluate if we still work
> and what we need to fix.

That's a cool idea.

-- 
David Gibson			| 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 --]