Re: [PATCH] libfdt: overlay: Add export-symbols support

[Ayush Singh <ayush@beagleboard.org>]

On 16/01/25 19:10, Herve Codina wrote:
> Hi Ayush,
> 
> Cc Thomas and Lucas (interested in this topic).
> 
> 
> On Sun, 12 Jan 2025 00:58:52 +0530
> Ayush Singh <ayush@beagleboard.org> 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 searches the __symbols__ node.
>>
>> 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].
>>
>> The phandle resolution in overlay is now broken into 2 steps:
>> - fragment target phandles
>> - All other phandles
>>
>> The reason this is required is because to find the export-symbols node
>> in the target node, we first need to resolve the target phandles. Here
>> is an example of overlay that will generate the target __fixup__ after
>> the property fixup, and thus fail without this 2 step resolution.
>>
>> ```
>> /dts-v1/;
>> /plugin/;
>>
>> &connectora {
>>    prop = <&gpio_connector>;
>> };
>>
>> &connectorb {
>>    prop = <&gpio_connector>;
>> };
>> ```
>>
>> Using fdtdump, we get the following:
>>
>> ```
>> **** fdtdump is a low-level debugging tool, not meant for general use.
>> **** If you want to decompile a dtb, you probably want
>> ****     dtc -I dtb -O dts <filename>
>>
>> /dts-v1/;
>> // magic:               0xd00dfeed
>> // totalsize:           0x1b1 (433)
>> // off_dt_struct:       0x38
>> // off_dt_strings:      0x180
>> // off_mem_rsvmap:      0x28
>> // version:             17
>> // last_comp_version:   16
>> // boot_cpuid_phys:     0x0
>> // size_dt_strings:     0x31
>> // size_dt_struct:      0x148
>>
>> / {
>>      fragment@0 {
>>          target = <0xffffffff>;
>>          __overlay__ {
>>              prop = <0xffffffff>;
>>          };
>>      };
>>      fragment@1 {
>>          target = <0xffffffff>;
>>          __overlay__ {
>>              prop = <0xffffffff>;
>>          };
>>      };
>>      __fixups__ {
>>          connectora = "/fragment@0:target:0";
>>          gpio_connector = "/fragment@0/__overlay__:prop:0", "/fragment@1/__overlay__:prop:0";
>>          connectorb = "/fragment@1:target: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] and [1], the
>> following patch series adds support for export-symbols in fdtoverlay.
>>
>> This patch series is mostly a prototype for the functionality. Depending
>> on the direction, next revision of the patch will add tests.
>>
>> [0]: https://lore.kernel.org/all/20241209151830.95723-1-herve.codina@bootlin.com/
>> [1]: https://lore.kernel.org/r/20250110-export-symbols-v1-1-b6213fcd6c82@beagleboard.org
>> ---
>>   libfdt/fdt_overlay.c | 245 ++++++++++++++++++++++++++++++++++++++++++++++-----
>>   1 file changed, 222 insertions(+), 23 deletions(-)
> 
> Thanks a lot for this patch!
> 
> I did a little modification related to unitialized variables leading to
> compilation errors.
> 
> Here is my modification.
> --- 8< ---
> diff --git a/libfdt/fdt_overlay.c b/libfdt/fdt_overlay.c
> index 7cb2230..12745b5 100644
> --- a/libfdt/fdt_overlay.c
> +++ b/libfdt/fdt_overlay.c
> @@ -411,7 +411,7 @@ static int overlay_fixup_target_phandle(void *fdt, void *fdto, int symbols_off,
>          const char *symbol_path;
>          int prop_len;
>          int symbol_off;
> -       uint32_t phandle;
> +       uint32_t phandle = 0;
>   
>          value = fdt_getprop_by_offset(fdto, property,
>                                        &label, &len);
> @@ -544,7 +544,7 @@ static int overlay_fixup_non_target_phandle(void *fdt, void *fdto,
>          const char *symbol_path;
>          int prop_len;
>          int symbol_off;
> -       uint32_t symbol_phandle;
> +       uint32_t symbol_phandle = 0;
>   
>          value = fdt_getprop_by_offset(fdto, property, &label, &len);
>          if (!value) {
> --- 8< ---
> 
> 
> I tested your code and fdtoverlay failed when the __symbols__ node is not in
> the base devicetree.
> 
> With the export-symbols node, the resolution can be successful without the
> need of the global __symbols__ node.
> 
> The error was just related to a check in overlay_fixup_one_phandle().
> 
> All verification are already done in previous operations. And so, I removed
> the check and the symbols_off parameter (not needed anymore). Here is the
> exact modification I did:
> --- 8< ---
> diff --git a/libfdt/fdt_overlay.c b/libfdt/fdt_overlay.c
> index 12745b5..a9f0465 100644
> --- a/libfdt/fdt_overlay.c
> +++ b/libfdt/fdt_overlay.c
> @@ -308,7 +308,6 @@ static int overlay_update_local_references(void *fdto, uint32_t delta)
>   /**
>    * overlay_fixup_one_phandle - Set an overlay phandle to the base one
>    * @fdto: Device tree overlay blob
> - * @symbols_off: Node offset of the symbols node in the base device tree
>    * @path: Path to a node holding a phandle in the overlay
>    * @path_len: number of path characters to consider
>    * @name: Name of the property holding the phandle reference in the overlay
> @@ -327,7 +326,7 @@ static int overlay_update_local_references(void *fdto, uint32_t delta)
>    *      0 on success
>    *      Negative error code on failure
>    */
> -static int overlay_fixup_one_phandle(void *fdto, int symbols_off,
> +static int overlay_fixup_one_phandle(void *fdto,
>   				     const char *path, uint32_t path_len,
>   				     const char *name, uint32_t name_len,
>   				     int poffset, uint32_t phandle)
> @@ -335,9 +334,6 @@ static int overlay_fixup_one_phandle(void *fdto, int symbols_off,
>   	fdt32_t phandle_prop;
>   	int fixup_off;
>   
> -	if (symbols_off < 0)
> -		return symbols_off;
> -
>   	fixup_off = fdt_path_offset_namelen(fdto, path, path_len);
>   	if (fixup_off == -FDT_ERR_NOTFOUND)
>   		return -FDT_ERR_BADOVERLAY;
> @@ -468,7 +464,7 @@ static int overlay_fixup_target_phandle(void *fdt, void *fdto, int symbols_off,
>   		if (path_len == (fixup_len - 1))
>   			return -FDT_ERR_BADOVERLAY;
>   
> -		ret = overlay_fixup_one_phandle(fdto, symbols_off, path,
> +		ret = overlay_fixup_one_phandle(fdto, path,
>   						path_len, "target",
>   						sizeof("target") - 1, 0,
>   						phandle);
> @@ -628,7 +624,7 @@ static int overlay_fixup_non_target_phandle(void *fdt, void *fdto,
>   			target_phandle = symbol_phandle;
>   		}
>   
> -		ret = overlay_fixup_one_phandle(fdto, symbols_off, path,
> +		ret = overlay_fixup_one_phandle(fdto, path,
>   						path_len, name, name_len,
>   						poffset, target_phandle);
>   		if (ret)
> --- 8< ---
> 
> With this modification applied, fdtoverlay worked without any issue using
> the following commands:
>    dtc -I dts -O dtb base.dts > base.dtb
>    dtc -I dts -O dtb overlay.dtso > overlay.dtbo
>    fdtoverlay -i base.dtb -o full.dtb overlay.dtbo
> 
> The resulting full.dtb was correct.
> 
> The file base.dts used for the test was:
> --- 8< ---
> /dts-v1/;
> 
> /{
> 	somewhere {
> 		base_node1: base-node1 {
> 			prop = "base,node1";
> 		};
> 
> 		base-node2 {
> 			prop = "base,node2";
> 			ref-base-node1 = <&base_node1>;
> 		};
> 
> 		base_node3: base-node3 {
> 			prop = "base,node3";
> 		};
> 
> 		export-symbols {
> 			base_node = <&base_node3>;
> 		};
> 	};
> };
> --- 8< ---
> 
> And the overlay.dso:
> --- 8< ---
> /dts-v1/;
> /plugin/;
> 
> / {
> 	fragment@0 {
> 		target-path = "/somewhere";
> 
> 		__overlay__ {
> 			ovl_node1: ovl_node1 {
> 				prop = "ovl,node1";
> 			};
> 
> 			ovl_node2 {
> 				prop = "ovl,node2";
> 				ref-ovl-node1 = <&ovl_node1>;
> 			};
> 
> 			ovl_node3 {
> 				prop = "ovl,node3";
> 				ref-base-node = <&base_node>;
> 			};
> 		};
> 	};
> };
> --- 8< ---
> 
> Hope this will help moving forward on this topic.
> 
> Best regards,
> Hervé

Thanks for the feedback.

I will add the changes in v2 of the patch.

I will also be sending a patch series to devicetree-spec mailing list 
[0] to see if any changes would be needed to make it part of the spec 
rather than linux specific node.

[0]: https://lore.kernel.org/devicetree-spec/

Ayush Singh