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

[Ahmad Fatoum <a.fatoum@pengutronix.de>]

Hello Marco,

On 03.07.23 08:25, Marco Felsch wrote:
> 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.

Existing users will get the new default. Users that build in a BSP
will want y too as they may not see warnings. It doesn't help people
copying the multi defconfig and not changing it, but that's not a reason
for having existing configs to not fail when they lack firmware.

> 
>> +	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?

It doesn't matter much. Linker will garbage collect the unused section.

> 
>>  }
>>  
>>  __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.

It's possible and I don't feel strong one way or the other.

> 
>> +	$(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.

We need to alert the user somehow that the image is missing/non-functional.
It's just a warning message.

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

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