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

David Gibson <[email protected]> Thu, 6 Mar 2025 16:36:21 +1100
Newsgroups	org.kernel.vger.devicetree-compiler
Message-ID	<Z8k0VeNXbuJ3kFj3@zatzit>
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/[email protected]/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 <[email protected]>
> ---
> 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/[email protected]/
> ---
>  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
signature.asc (application/pgp-signature, 833 B)