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

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/[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.

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 <[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.

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/[email protected]/

Ayush Singh