[PATCH] checks: Add support for export-symbols

[PATCH] checks: Add support for export-symbols

[Ayush Singh]

The export symbols node adds some additional symbols that can be used
in the symbols resolution. The resolver tries to match unresolved
symbols first using the export symbols node and, if a match is not
found, it tries the normal route and walks the tree.

Contrary to symbols available in the global __symbols__ node, symbols
listed in the export symbols node can be considered as local symbols.
Indeed, they can be changed depending on the node the overlay is going
to be applied to and are only visible from the current node properties.

export-symbols are phandle based as opposed to global __symbols__. This
allows for much simpler use with overlays as opposed to __symbols__
where paths require resizing of overlays as show in [0].

[0]: https://lore.kernel.org/devicetree-compiler/6b2dba90-3c52-4933-88f3-b47f96dc7710@beagleboard.org/T/#m8259c8754f680b9da7b91f7b7dd89f10da91d8ed

Signed-off-by: Ayush Singh <ayush@beagleboard.org>
---
As discussed in the export-symbols kernel patch series [0], the
following patch series adds support for export-symbols in the base
devicetree compiler.

This patch series is mostly a prototype for the functionality. Depending
on the direction, next revision of the patch will add tests.

Support for export-symbols in overlays will be part of a seperate patch
series.

[0]: https://lore.kernel.org/all/20241209151830.95723-1-herve.codina@bootlin.com/
---
 checks.c | 42 ++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 40 insertions(+), 2 deletions(-)

diff --git a/checks.c b/checks.c
index 123f2eb425f4a2f8ac22bfe10a842bf08e296ba1..e577f30a3ed9b762aee073fa1ff9d866f5de60b6 100644
--- a/checks.c
+++ b/checks.c
@@ -600,11 +600,37 @@ ERROR(name_properties, check_name_properties, NULL, &name_is_string);
  * Reference fixup functions
  */
 
+static const char *fixup_export_symbol(struct node *export_symbols,
+				       struct marker *m)
+{
+	struct property *prop;
+	struct marker *em;
+
+	for_each_property(export_symbols, prop) {
+		if (streq(m->ref, prop->name)) {
+			em = prop->val.markers;
+			for_each_marker_of_type(em, REF_PHANDLE) {
+				return em->ref;
+			}
+		}
+	}
+
+	return NULL;
+}
+
 static void fixup_phandle_references(struct check *c, struct dt_info *dti,
 				     struct node *node)
 {
 	struct node *dt = dti->dt;
 	struct property *prop;
+	struct node *child, *export_symbols = NULL;
+
+	for_each_child(node, child) {
+		if (streq(child->name, "export-symbols")) {
+			export_symbols = child;
+			break;
+		}
+	}
 
 	for_each_property(node, prop) {
 		struct marker *m = prop->val.markers;
@@ -612,13 +638,25 @@ static void fixup_phandle_references(struct check *c, struct dt_info *dti,
 		cell_t phandle;
 
 		for_each_marker_of_type(m, REF_PHANDLE) {
+			const char *ref = NULL;
+
 			assert(m->offset + sizeof(cell_t) <= prop->val.len);
 
-			refnode = get_node_by_ref(dt, m->ref);
+			/* Check export-symbols if present */
+			if (export_symbols)
+				ref = fixup_export_symbol(export_symbols, m);
+
+			/* If entry not found in export-symbols (or export-symbols not present), 
+			 * search the normal way.
+			 */
+			if (!ref)
+				ref = m->ref;
+
+			refnode = get_node_by_ref(dt, ref);
 			if (! refnode) {
 				if (!(dti->dtsflags & DTSF_PLUGIN))
 					FAIL(c, dti, node, "Reference to non-existent node or "
-							"label \"%s\"\n", m->ref);
+							"label \"%s\"\n", ref);
 				else /* mark the entry as unresolved */
 					*((fdt32_t *)(prop->val.val + m->offset)) =
 						cpu_to_fdt32(0xffffffff);

---
base-commit: 18f4f305fdd7e14c8941658a29c7b85c27d41de4
change-id: 20250110-export-symbols-e2ea3df01ea9

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

Re: [PATCH] checks: Add support for export-symbols

[David Gibson]

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

On Fri, Jan 10, 2025 at 11:18:55PM +0530, Ayush Singh wrote:
> The export symbols node adds some additional symbols that can be used
> in the symbols resolution. The resolver tries to match unresolved
> symbols first using the export symbols node and, if a match is not
> found, it tries the normal route and walks the tree.
> 
> Contrary to symbols available in the global __symbols__ node, symbols
> listed in the export symbols node can be considered as local symbols.
> Indeed, they can be changed depending on the node the overlay is going
> to be applied to and are only visible from the current node properties.
> 
> export-symbols are phandle based as opposed to global __symbols__. This
> allows for much simpler use with overlays as opposed to __symbols__
> where paths require resizing of overlays as show in [0].
> 
> [0]:
> https://lore.kernel.org/devicetree-compiler/6b2dba90-3c52-4933-88f3-b47f96dc7710@beagleboard.org/T/#m8259c8754f680b9da7b91f7b7dd89f10da91d8ed

I'm not sold on the concept of export-symbols in the first place.  But
at the very least this commit message needs to explain what actually
needs to be done within dtc to handle them.  It's not obvious to me
why it needs to do anything here, as opposed to in dtc consumers.

> 
> Signed-off-by: Ayush Singh <ayush@beagleboard.org>
> ---
> As discussed in the export-symbols kernel patch series [0], the
> following patch series adds support for export-symbols in the base
> devicetree compiler.
> 
> This patch series is mostly a prototype for the functionality. Depending
> on the direction, next revision of the patch will add tests.
> 
> Support for export-symbols in overlays will be part of a seperate patch
> series.
> 
> [0]: https://lore.kernel.org/all/20241209151830.95723-1-herve.codina@bootlin.com/
> ---
>  checks.c | 42 ++++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 40 insertions(+), 2 deletions(-)
> 
> diff --git a/checks.c b/checks.c
> index 123f2eb425f4a2f8ac22bfe10a842bf08e296ba1..e577f30a3ed9b762aee073fa1ff9d866f5de60b6 100644
> --- a/checks.c
> +++ b/checks.c
> @@ -600,11 +600,37 @@ ERROR(name_properties, check_name_properties, NULL, &name_is_string);
>   * Reference fixup functions
>   */
>  
> +static const char *fixup_export_symbol(struct node *export_symbols,
> +				       struct marker *m)
> +{
> +	struct property *prop;
> +	struct marker *em;
> +
> +	for_each_property(export_symbols, prop) {
> +		if (streq(m->ref, prop->name)) {
> +			em = prop->val.markers;
> +			for_each_marker_of_type(em, REF_PHANDLE) {
> +				return em->ref;

This requires the contents of the symbol to be defined as a reference
to work.  Yes, that's the obvious way to do it, but in existing things
if you really want to hand code a phandle value, you can do that.

This makes it so that trees that would otherwise have byte-for-byte
identical dtb output don't work the same way.

> +			}
> +		}
> +	}
> +
> +	return NULL;
> +}
> +
>  static void fixup_phandle_references(struct check *c, struct dt_info *dti,
>  				     struct node *node)
>  {
>  	struct node *dt = dti->dt;
>  	struct property *prop;
> +	struct node *child, *export_symbols = NULL;
> +
> +	for_each_child(node, child) {
> +		if (streq(child->name, "export-symbols")) {
> +			export_symbols = child;
> +			break;
> +		}
> +	}
>  
>  	for_each_property(node, prop) {
>  		struct marker *m = prop->val.markers;
> @@ -612,13 +638,25 @@ static void fixup_phandle_references(struct check *c, struct dt_info *dti,
>  		cell_t phandle;
>  
>  		for_each_marker_of_type(m, REF_PHANDLE) {
> +			const char *ref = NULL;
> +
>  			assert(m->offset + sizeof(cell_t) <= prop->val.len);
>  
> -			refnode = get_node_by_ref(dt, m->ref);
> +			/* Check export-symbols if present */
> +			if (export_symbols)
> +				ref = fixup_export_symbol(export_symbols, m);
> +
> +			/* If entry not found in export-symbols (or export-symbols not present), 
> +			 * search the normal way.
> +			 */
> +			if (!ref)
> +				ref = m->ref;

This muddles semantic layers together.  In dtc, the *contents* of
nodes and properties doesn't affect the output other than that node or
property (though it might affect warnings).  Only actual dts syntax
affects output non-locally.  This changes that, which I think is a
very bad idea for understandability.

> +
> +			refnode = get_node_by_ref(dt, ref);
>  			if (! refnode) {
>  				if (!(dti->dtsflags & DTSF_PLUGIN))
>  					FAIL(c, dti, node, "Reference to non-existent node or "
> -							"label \"%s\"\n", m->ref);
> +							"label \"%s\"\n", ref);
>  				else /* mark the entry as unresolved */
>  					*((fdt32_t *)(prop->val.val + m->offset)) =
>  						cpu_to_fdt32(0xffffffff);
> 
> ---
> base-commit: 18f4f305fdd7e14c8941658a29c7b85c27d41de4
> change-id: 20250110-export-symbols-e2ea3df01ea9
> 
> Best regards,

-- 
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] checks: Add support for export-symbols

[Ayush Singh]

On 3/6/25 11:06, David Gibson wrote:

> On Fri, Jan 10, 2025 at 11:18:55PM +0530, Ayush Singh wrote:
>> The export symbols node adds some additional symbols that can be used
>> in the symbols resolution. The resolver tries to match unresolved
>> symbols first using the export symbols node and, if a match is not
>> found, it tries the normal route and walks the tree.
>>
>> Contrary to symbols available in the global __symbols__ node, symbols
>> listed in the export symbols node can be considered as local symbols.
>> Indeed, they can be changed depending on the node the overlay is going
>> to be applied to and are only visible from the current node properties.
>>
>> export-symbols are phandle based as opposed to global __symbols__. This
>> allows for much simpler use with overlays as opposed to __symbols__
>> where paths require resizing of overlays as show in [0].
>>
>> [0]:
>> https://lore.kernel.org/devicetree-compiler/6b2dba90-3c52-4933-88f3-b47f96dc7710@beagleboard.org/T/#m8259c8754f680b9da7b91f7b7dd89f10da91d8ed
> I'm not sold on the concept of export-symbols in the first place.  But
> at the very least this commit message needs to explain what actually
> needs to be done within dtc to handle them.  It's not obvious to me
> why it needs to do anything here, as opposed to in dtc consumers.

It would be great if we can take the discussion regarding export-symbols 
here [0].

What do you mean my consumers? The kernel should do it at runtime or 
something?

>> Signed-off-by: Ayush Singh <ayush@beagleboard.org>
>> ---
>> As discussed in the export-symbols kernel patch series [0], the
>> following patch series adds support for export-symbols in the base
>> devicetree compiler.
>>
>> This patch series is mostly a prototype for the functionality. Depending
>> on the direction, next revision of the patch will add tests.
>>
>> Support for export-symbols in overlays will be part of a seperate patch
>> series.
>>
>> [0]: https://lore.kernel.org/all/20241209151830.95723-1-herve.codina@bootlin.com/
>> ---
>>   checks.c | 42 ++++++++++++++++++++++++++++++++++++++++--
>>   1 file changed, 40 insertions(+), 2 deletions(-)
>>
>> diff --git a/checks.c b/checks.c
>> index 123f2eb425f4a2f8ac22bfe10a842bf08e296ba1..e577f30a3ed9b762aee073fa1ff9d866f5de60b6 100644
>> --- a/checks.c
>> +++ b/checks.c
>> @@ -600,11 +600,37 @@ ERROR(name_properties, check_name_properties, NULL, &name_is_string);
>>    * Reference fixup functions
>>    */
>>   
>> +static const char *fixup_export_symbol(struct node *export_symbols,
>> +				       struct marker *m)
>> +{
>> +	struct property *prop;
>> +	struct marker *em;
>> +
>> +	for_each_property(export_symbols, prop) {
>> +		if (streq(m->ref, prop->name)) {
>> +			em = prop->val.markers;
>> +			for_each_marker_of_type(em, REF_PHANDLE) {
>> +				return em->ref;
> This requires the contents of the symbol to be defined as a reference
> to work.  Yes, that's the obvious way to do it, but in existing things
> if you really want to hand code a phandle value, you can do that.
>
> This makes it so that trees that would otherwise have byte-for-byte
> identical dtb output don't work the same way.

I see, I did not consider hardcoding phandle value.

>> +			}
>> +		}
>> +	}
>> +
>> +	return NULL;
>> +}
>> +
>>   static void fixup_phandle_references(struct check *c, struct dt_info *dti,
>>   				     struct node *node)
>>   {
>>   	struct node *dt = dti->dt;
>>   	struct property *prop;
>> +	struct node *child, *export_symbols = NULL;
>> +
>> +	for_each_child(node, child) {
>> +		if (streq(child->name, "export-symbols")) {
>> +			export_symbols = child;
>> +			break;
>> +		}
>> +	}
>>   
>>   	for_each_property(node, prop) {
>>   		struct marker *m = prop->val.markers;
>> @@ -612,13 +638,25 @@ static void fixup_phandle_references(struct check *c, struct dt_info *dti,
>>   		cell_t phandle;
>>   
>>   		for_each_marker_of_type(m, REF_PHANDLE) {
>> +			const char *ref = NULL;
>> +
>>   			assert(m->offset + sizeof(cell_t) <= prop->val.len);
>>   
>> -			refnode = get_node_by_ref(dt, m->ref);
>> +			/* Check export-symbols if present */
>> +			if (export_symbols)
>> +				ref = fixup_export_symbol(export_symbols, m);
>> +
>> +			/* If entry not found in export-symbols (or export-symbols not present),
>> +			 * search the normal way.
>> +			 */
>> +			if (!ref)
>> +				ref = m->ref;
> This muddles semantic layers together.  In dtc, the *contents* of
> nodes and properties doesn't affect the output other than that node or
> property (though it might affect warnings).  Only actual dts syntax
> affects output non-locally.  This changes that, which I think is a
> very bad idea for understandability.

Don't __symbols__ and aliases already do this in a way, just globally I 
guess?

>> +
>> +			refnode = get_node_by_ref(dt, ref);
>>   			if (! refnode) {
>>   				if (!(dti->dtsflags & DTSF_PLUGIN))
>>   					FAIL(c, dti, node, "Reference to non-existent node or "
>> -							"label \"%s\"\n", m->ref);
>> +							"label \"%s\"\n", ref);
>>   				else /* mark the entry as unresolved */
>>   					*((fdt32_t *)(prop->val.val + m->offset)) =
>>   						cpu_to_fdt32(0xffffffff);
>>
>> ---
>> base-commit: 18f4f305fdd7e14c8941658a29c7b85c27d41de4
>> change-id: 20250110-export-symbols-e2ea3df01ea9
>>
>> Best regards,


[0]: 
https://lore.kernel.org/all/20250225-export-symbols-v1-1-693049e3e187@beagleboard.org/

Ayush Singh