Re: [PATCH v2 2/2] firmware_loader: Enable compressed files with EXTRA_FIRMWARE

[Kevin Martin <kevinmbecause@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 5400 bytes --]

On Sun, 7 Jan 2024, Masahiro Yamada wrote:

> On Fri, Jan 5, 2024 at 3:11 PM Kevin Martin <kevinmbecause@gmail.com> wrote:
> >
> > The linux-firmware packages on Gentoo, Fedora, Arch, and others
> > compress the firmware files. This works well with
> > CONFIG_FW_LOADER_COMPRESS but does not work with CONFIG_EXTRA_FIRMWARE.
> > This patch allows the build system to decompress firmware files
> > specified by CONFIG_EXTRA_FIRMWARE. Uncompressed files are used first,
> > then the compressed files are used.
> >
> > The patch works by copying or decompressing the specified firmware files
> > into the build directory, then compiling them in from there. I would
> > prefer to not copy any uncompressed files, but I have not found a clean
> > way to do that.
> >
> > Signed-off-by: Kevin Martin <kevinmbecause@gmail.com>
> > ---
> > Changes in v2:
> > - Changed .gitignore to ignore all firmware files copied into the
> > directory.
> > - Fixed a tab.
> >
> >  drivers/base/firmware_loader/Kconfig            |  5 ++++-
> >  drivers/base/firmware_loader/builtin/.gitignore |  5 ++++-
> >  drivers/base/firmware_loader/builtin/Makefile   | 16 ++++++++++++----
> >  3 files changed, 20 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/base/firmware_loader/Kconfig b/drivers/base/firmware_loader/Kconfig
> > index 5ca00e02f..b7a908bff 100644
> > --- a/drivers/base/firmware_loader/Kconfig
> > +++ b/drivers/base/firmware_loader/Kconfig
> > @@ -76,7 +76,10 @@ config EXTRA_FIRMWARE
> >           image since it combines both GPL and non-GPL work. You should
> >           consult a lawyer of your own before distributing such an image.
> >
> > -         NOTE: Compressed files are not supported in EXTRA_FIRMWARE.
> > +         NOTE: Compressed files are supported by EXTRA_FIRMWARE. The build
> > +         system will look for uncompressed files first then fall back to
> > +         searching for compressed files in a similar way to
> > +         CONFIG_FW_LOADER_COMPRESS.
> >
> >  config EXTRA_FIRMWARE_DIR
> >         string "Firmware blobs root directory"
> > diff --git a/drivers/base/firmware_loader/builtin/.gitignore b/drivers/base/firmware_loader/builtin/.gitignore
> > index 166f76b43..b3124ee78 100644
> > --- a/drivers/base/firmware_loader/builtin/.gitignore
> > +++ b/drivers/base/firmware_loader/builtin/.gitignore
> > @@ -1,2 +1,5 @@
> >  # SPDX-License-Identifier: GPL-2.0
> > -*.gen.S
> > +*
> > +!.gitignore
> > +!Makefile
> > +!main.c
> > diff --git a/drivers/base/firmware_loader/builtin/Makefile b/drivers/base/firmware_loader/builtin/Makefile
> > index 6c067dedc..cc60eb441 100644
> > --- a/drivers/base/firmware_loader/builtin/Makefile
> > +++ b/drivers/base/firmware_loader/builtin/Makefile
> > @@ -20,7 +20,7 @@ filechk_fwbin = \
> >         echo "    .section .rodata"                             ;\
> >         echo "    .p2align 4"                                   ;\
> >         echo "_fw_$(FWSTR)_bin:"                                ;\
> > -       echo "    .incbin \"$(fwdir)/$(FWNAME)\""               ;\
> > +       echo "    .incbin \"$(obj)/$(FWNAME)\""                 ;\
> >         echo "_fw_end:"                                         ;\
> >         echo "    .section .rodata.str,\"aMS\",$(PROGBITS),1"   ;\
> >         echo "    .p2align $(ASM_ALIGN)"                        ;\
> > @@ -36,7 +36,15 @@ $(obj)/%.gen.S: FORCE
> >         $(call filechk,fwbin)
> >
> >  # The .o files depend on the binaries directly; the .S files don't.
> > -$(addprefix $(obj)/, $(firmware)): $(obj)/%.gen.o: $(fwdir)/%
> > +$(addprefix $(obj)/, $(firmware)): $(obj)/%.gen.o: $(obj)/%
> >
> > -targets := $(patsubst $(obj)/%,%, \
> > -                                $(shell find $(obj) -name \*.gen.S 2>/dev/null))
> > +$(obj)/% : $(fwdir)/% FORCE
> > +       $(call if_changed,copy)
> > +
> > +$(obj)/% : $(fwdir)/%.xz FORCE
> > +       $(call if_changed,xzdec)
> > +
> > +$(obj)/% : $(fwdir)/%.zst FORCE
> > +       $(call if_changed,zstddec)
> > +
> > +targets := $(patsubst %.gen.o, %.gen.S, $(firmware)) $(CONFIG_EXTRA_FIRMWARE)
> 
> 
> I noticed that "make clean" leaves copied firmware files
> in drivers/base/firmware_loader/builtin/.
> 
> 
> You need to clean up all files in
> drivers/base/firmware_loader/builtin/
> except Makefile, main.c.
> 
> The following worked for me.
> 
> 
> diff --git a/drivers/base/firmware_loader/builtin/Makefile
> b/drivers/base/firmware_loader/builtin/Makefile
> index bcac1723dc32..4d62ee9f06f6 100644
> --- a/drivers/base/firmware_loader/builtin/Makefile
> +++ b/drivers/base/firmware_loader/builtin/Makefile
> @@ -48,3 +48,5 @@ $(obj)/% : $(fwdir)/%.zst FORCE
>         $(call if_changed,zstddec)
> 
>  targets := $(patsubst %.gen.o, %.gen.S, $(firmware)) $(CONFIG_EXTRA_FIRMWARE)
> +
> +clean-files := $(filter-out Makefile main.c, $(patsubst $(obj)/%,%,
> $(wildcard $(obj)/*)))
> 

This explains why the "shell find" command was used. I was attempting to 
generate the list from $(CONFIG_EXTRA_FIRMWARE), but that is not defined 
during "make clean" as I am just now learning. While I would prefer to use 
a whitelist instead of a blacklist, I do not know a way to accomplish 
that. If everyone is ok with wiping out all files other than Makefile and 
main.c, then I will add the wildcard to "targets" and submit a new 
patch revision.

> 
> 
> 
> 
> 
> 
> 
> 
> 
> 
> 
> 
> -- 
> Best Regards
> Masahiro Yamada
>