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

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

[Marco Felsch]

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,%,$@)))
 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 $@)
 
 clean-files += *.sha.bin *.sum
 
diff --git a/images/Makefile b/images/Makefile
index 7b4e01953185..c0105609eec2 100644
--- a/images/Makefile
+++ b/images/Makefile
@@ -132,7 +132,7 @@ $(obj)/barebox.sha.bin: $(obj)/barebox.sum FORCE
 	$(call if_changed,sha256bin)
 
 $(obj)/barebox.sum: $(obj)/barebox.z FORCE
-	$(call if_changed,sha256sum)
+	$(call if_changed,sha256sum,$<)
 
 
 # barebox.z - compressed barebox binary
diff --git a/scripts/Makefile.lib b/scripts/Makefile.lib
index 0b236babb275..f205e08afc13 100644
--- a/scripts/Makefile.lib
+++ b/scripts/Makefile.lib
@@ -301,7 +301,7 @@ quiet_cmd_sha256bin ?= SHA-BIN $@
 			while read -r byte; do printf '\%o' $$byte; done)" > $@
 
 quiet_cmd_sha256sum ?= SHA     $@
-      cmd_sha256sum ?= sha256sum $< > $@
+      cmd_sha256sum ?= sha256sum $2 > $@
 
 # Decompressor for barebox proper binary when using PBL
 # ---------------------------------------------------------------------------
-- 
2.39.2

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

[Sascha Hauer]

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.

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 $@)
                                                                             ^^^

$(obj) should be $(fwdir) as this is the directory where the firmware
files are.

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 |

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

[Sascha Hauer]

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 |

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

[Marco Felsch]

Hi Sascha,

sry. for the delayed answer.

On 24-01-17, Sascha Hauer wrote:
> 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...

yes.

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

and yes.

> 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 $@)

I wasn't considering this but it looks good to me. Now I see the issue
with $(obj)/% as well :/ I used the $(obj)/firmware path for my firmware
binaries and therefore had no issues with out-of-tree building.

Regards,
  Marco

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