Re: [PATCH] firmware: fix missing firmware handling for external firmware

[Sascha Hauer <sha@pengutronix.de>]

On Tue, Jan 16, 2024 at 03:44:16PM +0100, Sascha Hauer wrote:
> Hi Marco,
> 
> On Wed, Jan 10, 2024 at 03:59:08PM +0100, Marco Felsch wrote:
> > Currently we depend on the external firmware file since we need it for
> > the sha256sum. But this dependency break the 'optional missing firmware'
> > feature.
> > 
> > Fix this by dropping the firmware file prerequisite and instead evaluate
> > the FWNAME_EXISTS variable. If the firmware file does not exist a dummy
> > file is created and the build continues till the linking process (like
> > we do for the other firmware).
> > 
> > Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
> > ---
> >  firmware/Makefile    | 6 +++---
> >  images/Makefile      | 2 +-
> >  scripts/Makefile.lib | 2 +-
> >  3 files changed, 5 insertions(+), 5 deletions(-)
> > 
> > diff --git a/firmware/Makefile b/firmware/Makefile
> > index 51d98d54bf14..a07b31caec73 100644
> > --- a/firmware/Makefile
> > +++ b/firmware/Makefile
> > @@ -41,7 +41,7 @@ fwobjdir := $(objtree)/firmware
> >  pbl-y     := $(addsuffix .gen.o, $(pbl-firmware-y))
> >  obj-pbl-y := $(addsuffix .gen.o, $(firmware-y))
> >  
> > -FWNAME    = $(patsubst $(obj)/%.extgen.S,%,$(patsubst $(obj)/%.gen.S,%,$@))
> > +FWNAME    = $(patsubst $(obj)/%.sum,%,$(patsubst $(obj)/%.extgen.S,%,$(patsubst $(obj)/%.gen.S,%,$@)))
> 
> The purpose of this change is not clear to me. You replace the pattern
> %.sum with %, but I don't see how this ever matches.

Ok, now I see why this is needed. FWNAME works on $@, the filename of
the target of the rule...

> 
> Dropping this change has no visible effect for me.
> 
> >  FWSTR     = $(subst /,_,$(subst .,_,$(subst -,_,$(FWNAME))))
> >  FWNAME_EXISTS = $(if $(wildcard $(fwdir)/$(FWNAME)),1,0)
> >  
> > @@ -91,8 +91,8 @@ $(obj)/%.extgen.S: $(obj)/%.sha.bin FORCE
> >  $(obj)/%.sha.bin: $(obj)/%.sum FORCE
> >  	$(call if_changed,sha256bin)
> >  
> > -$(obj)/%.sum: $(obj)/% FORCE
> > -	$(call if_changed,sha256sum)
> > +$(obj)/%.sum: FORCE
> > +	$(if $(findstring 1,$(FWNAME_EXISTS)), $(call if_changed,sha256sum,$(obj)/$*), @touch $@)

...and the target here indeed has the pattern %.sum

I think we should rather use the target stem which you are using
already. Drop FWNAME_EXISTS so this becomes:

	$(if $(wildcard $(fwdir)/$*), $(call if_changed,sha256sum,$(fwdir)/$*),@touch $@)

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 |