Re: [PATCH v5 07/11] of: overlay: add FIT image overlay support

[Sascha Hauer <s.hauer@pengutronix.de>]

On Mon, Aug 18, 2025 at 07:26:15PM +0200, Marco Felsch wrote:
> This adds the support to load devicetree overlays from a FIT image.
> There are a few options to handle FIT overlays since the FIT overlay
> spec is not very strict.
> 
> This implements the most configurable case where each overlay does have
> its own config node (including the optional signature).
> 
> - The "pattern" filter matches the config-node names (the node names
>   below the configurations node), not the overlay image names (the node
>   names below the images node).
> 
> - The "compatible" filter check doesn't differ from the file based overlay
>   handling.
> 
> Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
> ---
>  Documentation/user/devicetree.rst |  19 +++---
>  drivers/of/overlay.c              | 126 +++++++++++++++++++++++++++++++++++---
>  2 files changed, 128 insertions(+), 17 deletions(-)
> 
> diff --git a/Documentation/user/devicetree.rst b/Documentation/user/devicetree.rst
> index 30a15a8865c154f8afe23d641da10d0afa84423b..644fb551adcf913ca9ed2ab3f1d5fbe7c367bdf6 100644
> --- a/Documentation/user/devicetree.rst
> +++ b/Documentation/user/devicetree.rst
> @@ -109,20 +109,23 @@ Overlays can be applied to the kernel device tree before it is handed over to
>  the kernel. The behaviour is controlled by different variables:
>  
>  ``global.of.overlay.path``
> -  Overlays are read from this directory. barebox will try to apply all overlays
> -  found here if not limited by one of the other variables below. When the path
> -  given here is an absolute path it is used as is. A relative path is relative
> -  to ``/`` or relative to the rootfs when using bootloader spec.
> +  Overlays are read from this path. The path can either be a directory which
> +  contains the overlays or an absolute path to a FIT-image. barebox will try to

is "absolute path" correct here?

Before it was either an absolute path or a path relative to the
rootfs...

> +  apply all overlays found if not limited by one of the other variables below.
> +  When the path given here is an absolute path it is used as is. A relative
> +  path is relative to ``/`` or relative to the rootfs when using bootloader
> +  spec.

...and this still seems to be the case.

Isn't this true for overlays in FIT images as weel? If yes I suggest to
just remove the "absolute".

>  ``global.of.overlay.compatible``
>    This is a space separated list of compatibles. Only overlays matching one of
>    these compatibles will be applied. When this list is empty then all overlays
>    will be applied. Overlays that don't have a compatible are considered being
>    always compatible.
>  ``global.of.overlay.pattern``
> -  This is a space separated list of file patterns. An overlay is only applied
> -  when its filename matches one of the patterns. The patterns can contain
> -  ``*`` and ``?`` as wildcards. The default is ``*`` which means all files are
> -  applied.
> +  This is a space separated list of file patterns or FIT-image config-node name
> +  patterns. An overlay is only applied when its filename or FIT-image
> +  config-node name matches one of the patterns. The patterns can contain ``*``
> +  and ``?`` as wildcards. The default is ``*`` which means all files or FIT-Image
> +  config-nodes are applied.
>  ``global.of.overlay.filter``
>    This is a space separated list of filters to apply. There are two generic filters:
>    ``pattern`` matches ``global.of.overlay.pattern`` above, ``compatible`` matches
> diff --git a/drivers/of/overlay.c b/drivers/of/overlay.c
> index f02a12d44f0d53db3fb7bb065461c0ef193d0ab3..6defd6fa6908ff2b2fd00d161464af37ac17e040 100644
> --- a/drivers/of/overlay.c
> +++ b/drivers/of/overlay.c
> @@ -8,10 +8,12 @@
>   */
>  #define pr_fmt(fmt) "of_overlay: " fmt
>  
> +#include <bootm.h>
>  #include <common.h>
>  #include <of.h>
>  #include <errno.h>
>  #include <globalvar.h>
> +#include <image-fit.h>
>  #include <magicvar.h>
>  #include <string.h>
>  #include <libfile.h>
> @@ -463,8 +465,100 @@ static int of_overlay_apply_dir(struct device_node *root, const char *dirname,
>  	return ret;
>  }
>  
> +static int of_overlay_apply_fit(struct device_node *root, struct fit_handle *fit,
> +				struct device_node *config)
> +{
> +	const char *name = config->name;
> +	struct device_node *overlay;
> +	unsigned long ovl_sz;
> +	const void *ovl;
> +	int ret;
> +
> +	if (!of_overlay_matches_filter(name, NULL))
> +		return 0;

Can we have a
	pr_debug("FIT config \"%s\" doesn't match filter \"%s\"\n",
		name, of_overlay_filter);

Here?

> +
> +	ret = fit_open_image(fit, config, "fdt", &ovl, &ovl_sz);
> +	if (ret)
> +		return ret;
> +
> +	overlay = of_unflatten_dtb(ovl, ovl_sz);

	if (IS_ERR(overlay))
		...

> +
> +	if (!of_overlay_matches_filter(NULL, overlay)) {
> +		ret = 0;
> +		goto out;
> +	}

I think a pr_debug printing the reason would be appropriate here as
well.

> +
> +	ret = of_overlay_apply_tree(root, overlay);
> +	if (ret == -ENODEV)
> +		pr_debug("Not applied %s (not compatible)\n", name);
> +	else if (ret)
> +		pr_err("Cannot apply %s: %s\n", name, strerror(-ret));
> +	else
> +		pr_info("Applied %s\n", name);
> +
> +out:
> +	of_delete_node(overlay);
> +
> +	return ret;
> +}
> +
> +static bool of_overlay_valid_config(struct fit_handle *fit,
> +				    struct device_node *config)
> +{
> +	/*
> +	 * Either kernel or firmware is marked as mandatory by U-Boot
> +	 * (doc/usage/fit/source_file_format.rst) except for overlays
> +	 * (doc/usage/fit/overlay-fdt-boot.rst). Therefore we need to ensure
> +	 * that only "fdt" config nodes are recognized as overlay config node.
> +	 */
> +	if (!fit_has_image(fit, config, "fdt") ||
> +	    fit_has_image(fit, config, "kernel") ||
> +	    fit_has_image(fit, config, "firmware"))
> +		return false;
> +
> +	return true;
> +}
> +
> +static int of_overlay_global_fixup_fit(struct device_node *root,
> +				       const char *fit_path, loff_t fit_size)
> +{
> +	enum bootm_verify verify = bootm_get_verify_mode();
> +	struct device_node *conf_node;
> +	struct fit_handle *fit;
> +	int ret;
> +
> +	if (!IS_ENABLED(CONFIG_FITIMAGE)) {
> +		pr_err("FIT based overlay handling requested while CONFIG_FITIMAGE=n\n");
> +		return -EINVAL;
> +	}
> +
> +	fit = fit_open(fit_path, 0, verify, fit_size);
> +	if (IS_ERR(fit)) {
> +		pr_err("Loading FIT image %s failed with: %pe\n", fit_path, fit);
> +		return PTR_ERR(fit);
> +	}
> +
> +	for_each_child_of_node(fit->configurations, conf_node) {
> +		if (!of_overlay_valid_config(fit, conf_node))
> +			continue;
> +
> +		ret = fit_config_verify_signature(fit, conf_node);
> +		if (ret)
> +			goto out;
> +
> +		ret = of_overlay_apply_fit(root, fit, conf_node);
> +		if (ret)
> +			goto out;
> +	}
> +
> +out:
> +	fit_close(fit);
> +	return ret;
> +}
> +
>  static int of_overlay_global_fixup(struct device_node *root, void *data)
>  {
> +	struct stat s;
>  	char *dir;
>  	int ret;
>  
> @@ -476,10 +570,24 @@ static int of_overlay_global_fixup(struct device_node *root, void *data)
>  	else
>  		dir = concat_path_file(of_overlay_basedir, of_overlay_path);
>  
> -	ret = of_overlay_apply_dir(root, dir, true);
>  
> -	free(dir);
> +	if (stat(dir, &s)) {
> +		pr_err("Cannot stat global.of.overlay.path (%s): %s\n",
> +		       dir, strerror(errno));

Use %m which prints strerror(errno) directly.

Sascha

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |