Re: [PATCH master v1 3/6] firmware: optionally turn missing firmware errors into warnings

[Marco Felsch <m.felsch@pengutronix.de>]

Hi Ahmad,

On 23-06-26, Ahmad Fatoum wrote:
> Previous commit turned compile-time errors into link-time errors.
> This commit goes a step further and allows the link to succeed
> unconditionally for build coverage and then dependent on the newly
> introduced CONFIG_MISSING_FIRMWARE_ERROR option abort the build with an
> error code and a listing of missing firmware printed to stderr.
> 
> In any case, barebox images which contain firmware in their PBL
> that's not available will be marked specially to reduce the risk
> of accidentally putting them to use:
> 
>   * They're truncated to zero size
> 
>   * The final "images built:" section marks them as having firmware
>     missing
> 
>   * They are omitted from the listing in the barebox-flash-images file
> 
>   * Each barebox-broken.img is accompanied with a
>     barebox-broken.img.missing-firmware containing a newline delimited
>     list of missing firmware images
> 
> Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
> ---
>  firmware/Kconfig     | 14 ++++++++++++++
>  firmware/Makefile    |  5 +++++
>  images/Makefile      | 21 ++++++++++++++++-----
>  scripts/Makefile.lib |  3 +++
>  4 files changed, 38 insertions(+), 5 deletions(-)
> 
> diff --git a/firmware/Kconfig b/firmware/Kconfig
> index 56ced00bc430..56d6d0d6c030 100644
> --- a/firmware/Kconfig
> +++ b/firmware/Kconfig
> @@ -6,6 +6,20 @@ config EXTRA_FIRMWARE_DIR
>  	string "Firmware blobs root directory"
>  	default "firmware"
>  
> +config MISSING_FIRMWARE_ERROR
> +	bool "Fail the build when required firmware is missing"
> +	default y

Do we need to make this default y? IMHO multi_v8_defconfig will set this
to 'n' anyway and so the 'n' will become the de-facto default.

> +	help
> +	  In-tree Defconfigs that enable multiple boards with different firmware
> +	  binary requirements would say y here, so you don't need unrelated firmware
> +	  for the build to succeed.
> +
> +	  Defconfigs custom-tailored to products would say n here as all boards
> +	  being built should be functional and have their firmware available.
> +
> +	  If in doubt, say Y and refer to the documentation on where to acquire the
> +	  needed firmware.
> +
>  config HAVE_FIRMWARE_IMX_LPDDR4_PMU_TRAIN
>  	bool
>  	default y
> diff --git a/firmware/Makefile b/firmware/Makefile
> index c66d19c677e8..f9490a8e7a15 100644
> --- a/firmware/Makefile
> +++ b/firmware/Makefile
> @@ -58,6 +58,11 @@ filechk_fwbin = { \
>  	echo "\#endif"						;\
>  	echo ".global _fw_$(FWSTR)_end"				;\
>  	echo "_fw_$(FWSTR)_end:"				;\
> +	echo "\#ifdef __PBL__"					;\
> +	echo "    .section .missing_fw,\"a\""			;\
> +	echo "_fwname_$(FWSTR):"				;\
> +	echo ".ascii \"firmware/$(FWNAME)\\n\""			;\
> +	echo "\#endif" 						;\

Shouldn't this be part of the #ifdef no-firmware? This way we do add the
.missing_fw section always which shouldn't be the case, right?

>  }
>  
>  __fwbin_sha = { \
> diff --git a/images/Makefile b/images/Makefile
> index c93f9e268978..16d86a5a5da4 100644
> --- a/images/Makefile
> +++ b/images/Makefile
> @@ -72,6 +72,8 @@ $(obj)/%.pbl: $(pbl-lds) $(BAREBOX_PBL_OBJS) $(obj)/piggy.o $(obj)/sha_sum.o FOR
>  
>  $(obj)/%.pblb: $(obj)/%.pbl FORCE
>  	$(call if_changed,objcopy_bin,$(*F))
> +	$(Q)$(OBJCOPY) -O binary --only-section=.missing_fw $< $@.missing-firmware
> +	$(Q)[ -s $@.missing-firmware ] || rm -f $@.missing-firmware
>  	$(call cmd,check_file_size,$@,$(CONFIG_BAREBOX_MAX_IMAGE_SIZE))
>  
>  #
> @@ -127,10 +129,14 @@ $(obj)/barebox.z: $(obj)/../barebox.bin FORCE
>  
>  # %.img - create a copy from another file
>  # ----------------------------------------------------------------
> +
> +missing_fw = $(strip $(wildcard $(obj)/$(FILE_$(@F)).missing-firmware $(basename $(obj)/$(FILE_$(@F))).missing-firmware))
> +
>  .SECONDEXPANSION:
>  $(obj)/%.img: $(obj)/$$(FILE_$$(@F))
>  	$(Q)if [ -z $(FILE_$(@F)) ]; then echo "FILE_$(@F) empty!"; false; fi
> -	$(call if_changed,shipped)
> +	$(Q)$(if $(missing_fw),cat $(missing_fw) >$@.missing-firmware,rm -f $@.missing-firmware)

Okay, since we always add the missing-firmware section we need to clean
it up here, right? Isn't it easier to add the missing-firmware section
only if detected? If this is possible.

> +	$(call if_changed,$(if $(missing_fw),0size,shipped))
>  
>  board = $(srctree)/arch/$(SRCARCH)/boards
>  objboard = $(objtree)/arch/$(SRCARCH)/boards
> @@ -194,10 +200,15 @@ multi-image-build:
>  
>  images: $(image-y-path) $(flash-link) $(flash-list) FORCE
>  	@echo "images built:"
> -	@for i in $(image-y); do echo $$i; done
> +	@for i in $(image-y); do \
> +	  if [ -s $(obj)/$$i ]; then echo $$i; \
> +	  else >&2 echo "** firmware missing for $$i **"; \

Not sure if we should print an error if the user set
CONFIG_MISSING_FIRMWARE_ERROR to n.

Regards,
  Marco

> +	  $(if $(CONFIG_MISSING_FIRMWARE_ERROR), >&2 sed 's/^/\t/' <$(obj)/$${i}.missing-firmware; missing=1;) \
> +	  fi; done; test -n "$$missing" && \
> +	echo >&2 "Firmware missing in CONFIG_MISSING_FIRMWARE_ERROR=y build" && false
>  
>  __images_install: images
> -	@for i in $(image-y-path); do install -t "$(INSTALL_PATH)" $$i; done
> +	@for i in $(image-y-path); do if [ -s $$i ]; then install -t "$(INSTALL_PATH)" $$i; fi; done
>  
>  PHONY += __images_install
>  
> @@ -205,10 +216,10 @@ $(flash-link): $(link-dest) FORCE
>  	$(call if_changed,ln)
>  
>  $(flash-list): $(image-y-path)
> -	@for i in $^; do echo $$i; done > $@
> +	@for i in $^; do if [ -s $$i ]; then echo $$i; fi; done > $@
>  
>  clean-files := *.pbl *.pblb *.map start_*.imximg *.img barebox.z start_*.kwbimg \
>  	start_*.kwbuartimg *.socfpgaimg *.mlo *.t20img *.t20img.cfg *.t30img \
>  	*.t30img.cfg *.t124img *.t124img.cfg *.mlospi *.mlo *.mxsbs *.mxssd \
> -	start_*.simximg start_*.usimximg *.zynqimg *.image *.swapped
> +	start_*.simximg start_*.usimximg *.zynqimg *.image *.swapped *.missing-firmware
>  clean-files += pbl.lds
> diff --git a/scripts/Makefile.lib b/scripts/Makefile.lib
> index 42ee27499561..b8fb2684421e 100644
> --- a/scripts/Makefile.lib
> +++ b/scripts/Makefile.lib
> @@ -236,6 +236,9 @@ endef
>  # Shipped files
>  # ===========================================================================
>  
> +quiet_cmd_0size = 0SIZE $@
> +cmd_0size = : > $@
> +
>  quiet_cmd_shipped = SHIPPED $@
>  cmd_shipped = cat $< > $@
>  
> -- 
> 2.39.2
> 
> 
>