Re: [RFC PATCH v2 2/5] kbuild: fit: Support selecting DTBs and config properties from a file

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [Medium] kbuild: fit: leading ./ in FIT_CONFIG_DTBS breaks
  Makefile.dtbs matching
- [Medium] kbuild: fit: basename lookup in _process_dtbs() breaks O=
  subdir DTBs
--

commit 7793aa0a3f31c2e79bf996dd2bb1dcd0d8affc8c
Author: Douglas Anderson <dianders@chromium.org>

kbuild: fit: Support selecting DTBs and config properties from a file

This commit adds support for a FIT_CONFIG make variable pointing to a
.fitconfig file that selects which DTBs to build and include in image.fit
and sets properties or subnodes on /configurations/conf-N nodes.

> diff --git a/scripts/Makefile.dtbs b/scripts/Makefile.dtbs
> index c4e4663902844..cc14fc905ea3b 100644
> --- a/scripts/Makefile.dtbs
> +++ b/scripts/Makefile.dtbs
> @@ -5,6 +5,15 @@ all-dtb := $(dtb-y) $(dtb-)
>  # If CONFIG_OF_ALL_DTBS is enabled, all DT blobs are built
>  dtb-$(CONFIG_OF_ALL_DTBS) += $(dtb-)
>  
> +# If FIT_CONFIG is specified, restrict $(dtb-y) to only the DTBs listed in
> +# $(FIT_CONFIG_FILE).
> +ifdef FIT_CONFIG_FILE
> +ifneq ($(filter arch/$(SRCARCH)/boot/dts%, $(obj)),)
> +dtb-y := $(filter $(all-dtb), \
> +	$(patsubst $(obj)/%, %, $(filter $(obj)/%, $(FIT_CONFIG_DTBS))))
> +endif
> +endif
> +
>  # Composite DTB (i.e. DTB constructed by overlay)
>  multi-dtb-y := $(call multi-search, $(dtb-y), .dtb, -dtbs)
>  # Primitive DTB compiled from *.dts
> diff --git a/scripts/Makefile.lib b/scripts/Makefile.lib
> index 19e96b30a122f..e08e35d4ed747 100644
> --- a/scripts/Makefile.lib
> +++ b/scripts/Makefile.lib
> @@ -427,8 +427,33 @@ quiet_cmd_fit = FIT     $@
>  		--name '$(UIMAGE_NAME)' $(FIT_EXTRA_ARGS) \
>  		$(if $(findstring 1,$(KBUILD_VERBOSE)),-v) \
>  		$(if $(FIT_DECOMPOSE_DTBS),--decompose-dtbs) \
> +		$(if $(FIT_CONFIG_FILE),--config $(FIT_CONFIG_FILE)) \
>  		--compress $(FIT_COMPRESSION) -k $< @$(word 2,$^)
>  
> +# If FIT_CONFIG is specified on the command line (either relative to $(srctree)
> +# or relative to arch/$(SRCARCH)/boot/dts/), locate that file and extract the
> +# list of .dtb files to build.
> +ifdef FIT_CONFIG
> +FIT_CONFIG_REL := $(firstword $(patsubst $(abs_srctree)/%,%,\
> +	$(patsubst $(srctree)/%,%,$(patsubst ./%,%,$(wildcard \
> +	$(FIT_CONFIG) \
> +	$(srctree)/$(FIT_CONFIG) \
> +	$(srctree)/arch/$(SRCARCH)/boot/dts/$(FIT_CONFIG))))))
> +ifeq ($(FIT_CONFIG_REL),)
> +$(error FIT_CONFIG '$(FIT_CONFIG)' not found)
> +endif
> +FIT_CONFIG_FILE := $(srctree)/$(FIT_CONFIG_REL)
> +ifndef FIT_CONFIG_DTBS
> +export FIT_CONFIG_DTBS := $(addprefix $(dir $(FIT_CONFIG_REL)), \
> +	$(shell $(MAKE_FIT) -C $(FIT_CONFIG_FILE) --list-dtbs))
> +$(if $(FIT_CONFIG_DTBS),,$(error Failed to parse DTB list from $(FIT_CONFIG_FILE)))
> +endif

[Severity: Medium]
Does $(dir $(FIT_CONFIG_REL)) prepend a leading ./ to every entry in
FIT_CONFIG_DTBS when FIT_CONFIG points to a file in the root of $(srctree)?

When FIT_CONFIG_REL has no slash (or when FIT_CONFIG is passed with a
leading ./ in an out-of-tree build so $(patsubst ./%,%,...) runs before
$(srctree)/ is stripped), $(dir $(FIT_CONFIG_REL)) evaluates to ./.  In
scripts/Makefile.dtbs, $(filter $(obj)/%, $(FIT_CONFIG_DTBS)) compares
against $(obj) without a leading ./, so nothing matches, dtb-y is cleared
across all DTS directories, and no DTBs are built.  Stripping ./ from
$(dir $(FIT_CONFIG_REL)) and running $(patsubst ./%,%,...) after stripping
$(srctree)/ and $(abs_srctree)/ would avoid this.

[ ... ]

> diff --git a/scripts/make_fit.py b/scripts/make_fit.py
> index 9bd35aa721191..0c0af80426411 100755
> --- a/scripts/make_fit.py
> +++ b/scripts/make_fit.py

[ ... ]

> @@ -322,9 +397,20 @@ def _process_dtbs(args, fsw, entries, fdts):
>  
>      # First figure out the unique DTB files that need compression
>      todo = []
> -    file_info = []  # List of (fname, model, compat, files) tuples
> +    file_info = []  # List of (fname, model, compat, files, props) tuples
> +
> +    if args.config:
> +        config = parse_config(args.config)
> +        by_name = {os.path.basename(fn): fn for fn in args.dtbs}
> +        config_dir = os.path.dirname(args.config)
> +        dtb_configs = [
> +            (by_name.get(dtb, os.path.join(config_dir, dtb)), props)
> +            for dtb, props in config.items()
> +        ]
> +    else:
> +        dtb_configs = [(fname, {}) for fname in args.dtbs]

[Severity: Medium]
Will by_name.get(dtb, ...) always miss in out-of-tree (O=) builds when a
.fitconfig section header includes a subdirectory path such as
[google/lga-blazer.dtb]?

by_name is keyed by os.path.basename(fn), so any dtb key containing a
slash never matches and falls back to os.path.join(config_dir, dtb).
Because scripts/Makefile.lib sets FIT_CONFIG_FILE in $(srctree),
config_dir points into the source tree rather than $(objtree) where the
compiled .dtb and .dtb.cmd files live, causing process_dtb() to fail with
FileNotFoundError.  Looking up os.path.basename(dtb) (or matching the
suffix against args.dtbs) would find the built DTB in $(objtree).

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261113210918.356261-1-dianders@chromium.org?part=2