[Buildroot] [PATCH-NEXT 0/4] package/linux-firmware: install into images for early loading support

[Buildroot] [PATCH-NEXT 0/4] package/linux-firmware: install into images for early loading support

[Peter Korsgaard]

Some drivers request their firmware very early when built into the
kernel, even before the initramfs is mounted - So the only way to
provide firmware for those drivers is to include them directly in the
kernel with the CONFIG_EXTRA_FIRMWARE option.

To support this, let linux-firmware install its files into
BINARIES_DIR in addition to TARGET_DIR, similar to how we do it for
intel-microcode.

Peter Korsgaard (4):
  package/linux-firmware: make target install macros accept a
    destination parameter
  package/linux-firmware.mk: get rid of temporary tarball for file
    installation
  package/linux-firmware: also install into images for early loading
    support
  linux: build after linux-firmware if enabled for early loading support

 linux/linux.mk                           |  3 ++-
 package/linux-firmware/linux-firmware.mk | 26 ++++++++++++++++--------
 2 files changed, 19 insertions(+), 10 deletions(-)

-- 
2.20.1

[Buildroot] [PATCH-NEXT 1/4] package/linux-firmware: make target install macros accept a destination parameter

[Peter Korsgaard]

So we can reuse them for also installing into the images directory

Signed-off-by: Peter Korsgaard <peter@korsgaard.com>
---
 package/linux-firmware/linux-firmware.mk | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/package/linux-firmware/linux-firmware.mk b/package/linux-firmware/linux-firmware.mk
index f23da171c6..3420266335 100644
--- a/package/linux-firmware/linux-firmware.mk
+++ b/package/linux-firmware/linux-firmware.mk
@@ -642,7 +642,7 @@ ifneq ($(LINUX_FIRMWARE_FILES),)
 define LINUX_FIRMWARE_INSTALL_FILES
 	cd $(@D) && \
 		$(TAR) cf install.tar $(sort $(LINUX_FIRMWARE_FILES)) && \
-		$(TAR) xf install.tar -C $(TARGET_DIR)/lib/firmware
+		$(TAR) xf install.tar -C $(1)
 endef
 endif
 
@@ -651,9 +651,9 @@ ifneq ($(LINUX_FIRMWARE_DIRS),)
 # into it in itself, should we re-install the package.
 define LINUX_FIRMWARE_INSTALL_DIRS
 	$(foreach d,$(LINUX_FIRMWARE_DIRS), \
-		rm -rf $(TARGET_DIR)/lib/firmware/$(d); \
-		mkdir -p $(dir $(TARGET_DIR)/lib/firmware/$(d)); \
-		cp -a $(@D)/$(d) $(TARGET_DIR)/lib/firmware/$(d)$(sep))
+		rm -rf $(1)/$(d); \
+		mkdir -p $(dir $(1)/$(d)); \
+		cp -a $(@D)/$(d) $(1)/$(d)$(sep))
 endef
 endif
 
@@ -687,7 +687,7 @@ endif
 # a/foo -> ../b/foo  where a/ (the directory where to put the symlink) does
 # not yet exist.
 define LINUX_FIRMWARE_CREATE_SYMLINKS
-	cd $(TARGET_DIR)/lib/firmware/ ; \
+	cd $(1) ; \
 	sed -r -e '/^Link: (.+) -> (.+)$$/!d; s//\1 \2/' $(@D)/WHENCE | \
 	while read f d; do \
 		if test -f $$(readlink -m $$(dirname $$f)/$$d); then \
@@ -699,9 +699,9 @@ endef
 
 define LINUX_FIRMWARE_INSTALL_TARGET_CMDS
 	mkdir -p $(TARGET_DIR)/lib/firmware
-	$(LINUX_FIRMWARE_INSTALL_FILES)
-	$(LINUX_FIRMWARE_INSTALL_DIRS)
-	$(LINUX_FIRMWARE_CREATE_SYMLINKS)
+	$(call LINUX_FIRMWARE_INSTALL_FILES,$(TARGET_DIR)/lib/firmware)
+	$(call LINUX_FIRMWARE_INSTALL_DIRS,$(TARGET_DIR)/lib/firmware)
+	$(call LINUX_FIRMWARE_CREATE_SYMLINKS,$(TARGET_DIR)/lib/firmware)
 endef
 
 $(eval $(generic-package))
-- 
2.20.1

[Buildroot] [PATCH-NEXT 2/4] package/linux-firmware.mk: get rid of temporary tarball for file installation

[Peter Korsgaard]

With the upcoming addition of the images-install step,
LINUX_FIRMWARE_INSTALl_FILES is called twice, for installing to the target
and to the images directory - Which may race with each other and cause
problems with the temporary install.tar tarball.

There is no specific reason to use a temporary file, we can just as well
pipe the two tar invocations together, so do that instead.

Signed-off-by: Peter Korsgaard <peter@korsgaard.com>
---
 package/linux-firmware/linux-firmware.mk | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/package/linux-firmware/linux-firmware.mk b/package/linux-firmware/linux-firmware.mk
index 3420266335..b6549ad371 100644
--- a/package/linux-firmware/linux-firmware.mk
+++ b/package/linux-firmware/linux-firmware.mk
@@ -641,8 +641,8 @@ endif
 ifneq ($(LINUX_FIRMWARE_FILES),)
 define LINUX_FIRMWARE_INSTALL_FILES
 	cd $(@D) && \
-		$(TAR) cf install.tar $(sort $(LINUX_FIRMWARE_FILES)) && \
-		$(TAR) xf install.tar -C $(1)
+		$(TAR) c $(sort $(LINUX_FIRMWARE_FILES)) | \
+		$(TAR) x -C $(1)
 endef
 endif
 
-- 
2.20.1

[Buildroot] [PATCH-NEXT 3/4] package/linux-firmware: also install into images for early loading support

[Peter Korsgaard]

Some drivers request their firmware very early when built into the kernel,
even before the initramfs is mounted - So the only way to provide firmware
for those drivers is to include them directly in the kernel with the
CONFIG_EXTRA_FIRMWARE option.

An example of this is the uC firmware for modern Intel GPUs.

Conceptually you can point CONFIG_EXTRA_FIRMWARE to
${TARGET_DIR}/lib/firmware, but then you cannot remove the firmware from the
initramfs and pay the size twice (inside the kernel + in initramfs), so
instead also install linux-firmware to the image dir, similar to how we do
it for intel-microcode.

Signed-off-by: Peter Korsgaard <peter@korsgaard.com>
---
 package/linux-firmware/linux-firmware.mk | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/package/linux-firmware/linux-firmware.mk b/package/linux-firmware/linux-firmware.mk
index b6549ad371..a08d9ccfda 100644
--- a/package/linux-firmware/linux-firmware.mk
+++ b/package/linux-firmware/linux-firmware.mk
@@ -7,6 +7,7 @@
 LINUX_FIRMWARE_VERSION = 20201022
 LINUX_FIRMWARE_SITE = http://git.kernel.org/pub/scm/linux/kernel/git/firmware/linux-firmware.git
 LINUX_FIRMWARE_SITE_METHOD = git
+LINUX_FIRMWARE_INSTALL_IMAGES = YES
 
 LINUX_FIRMWARE_CPE_ID_VENDOR = kernel
 
@@ -704,4 +705,11 @@ define LINUX_FIRMWARE_INSTALL_TARGET_CMDS
 	$(call LINUX_FIRMWARE_CREATE_SYMLINKS,$(TARGET_DIR)/lib/firmware)
 endef
 
+define LINUX_FIRMWARE_INSTALL_IMAGES_CMDS
+	mkdir -p $(BINARIES_DIR)
+	$(call LINUX_FIRMWARE_INSTALL_FILES,$(BINARIES_DIR))
+	$(call LINUX_FIRMWARE_INSTALL_DIRS,$(BINARIES_DIR))
+	$(call LINUX_FIRMWARE_CREATE_SYMLINKS,$(BINARIES_DIR))
+endef
+
 $(eval $(generic-package))
-- 
2.20.1

[Buildroot] [PATCH-NEXT 4/4] linux: build after linux-firmware if enabled for early loading support

[Peter Korsgaard]

To support building in (a subset of) the linux-firmware files into the
kernel using the CONFIG_EXTRA_FIRMWARE option, we need to ensure that the
firmware files are installed before the Linux kernel is built, similar to
how it is done for intel-microcode.

Signed-off-by: Peter Korsgaard <peter@korsgaard.com>
---
 linux/linux.mk | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/linux/linux.mk b/linux/linux.mk
index a212f42c28..5e4b319cf1 100644
--- a/linux/linux.mk
+++ b/linux/linux.mk
@@ -78,7 +78,8 @@ LINUX_MAKE_ENV = \
 
 LINUX_INSTALL_IMAGES = YES
 LINUX_DEPENDENCIES = host-kmod \
-	$(if $(BR2_PACKAGE_INTEL_MICROCODE),intel-microcode)
+	$(if $(BR2_PACKAGE_INTEL_MICROCODE),intel-microcode) \
+	$(if $(BR2_PACKAGE_LINUX_FIRMWARE),linux-firmware)
 
 # Starting with 4.16, the generated kconfig paser code is no longer
 # shipped with the kernel sources, so we need flex and bison, but
-- 
2.20.1

[Buildroot] [PATCH-NEXT 2/4] package/linux-firmware.mk: get rid of temporary tarball for file installation

[Yann E. MORIN]

Peter, All,

On 2021-02-12 19:40 +0100, Peter Korsgaard spake thusly:
> With the upcoming addition of the images-install step,
> LINUX_FIRMWARE_INSTALl_FILES is called twice, for installing to the target
> and to the images directory - Which may race with each other and cause
> problems with the temporary install.tar tarball.
> 
> There is no specific reason to use a temporary file, we can just as well
> pipe the two tar invocations together, so do that instead.

Actually, there *is* a reason why an intermediate tarball is used, see
commit 21a283ffb0d (linux-firmware: fail build for missing file).

The idea is that, when we bump the version a firmware file may get
removed, and we may not notice right when updating (because of the 
many sub-options), and that file is still isted.

If that is the case, then the first tar will bail out on that missing
file, and stop right away, thus not including the followign files. But
because it is on the LHS of a pipe, its return code is ignored.

Then the second tar is still happy with what it gets, because it is a
proper tar archive (if the last file in a tarball was extracted
successfully, there is no way to know if the tarball was truncated or
not).

As a consequence, the copy is only partial, and the build does not fail.

Regards,
Yann E. MORIN.

> Signed-off-by: Peter Korsgaard <peter@korsgaard.com>
> ---
>  package/linux-firmware/linux-firmware.mk | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/package/linux-firmware/linux-firmware.mk b/package/linux-firmware/linux-firmware.mk
> index 3420266335..b6549ad371 100644
> --- a/package/linux-firmware/linux-firmware.mk
> +++ b/package/linux-firmware/linux-firmware.mk
> @@ -641,8 +641,8 @@ endif
>  ifneq ($(LINUX_FIRMWARE_FILES),)
>  define LINUX_FIRMWARE_INSTALL_FILES
>  	cd $(@D) && \
> -		$(TAR) cf install.tar $(sort $(LINUX_FIRMWARE_FILES)) && \
> -		$(TAR) xf install.tar -C $(1)
> +		$(TAR) c $(sort $(LINUX_FIRMWARE_FILES)) | \
> +		$(TAR) x -C $(1)
>  endef
>  endif
>  
> -- 
> 2.20.1
> 

-- 
.-----------------.--------------------.------------------.--------------------.
|  Yann E. MORIN  | Real-Time Embedded | /"\ ASCII RIBBON | Erics' conspiracy: |
| +33 662 376 056 | Software  Designer | \ / CAMPAIGN     |  ___               |
| +33 561 099 427 `------------.-------:  X  AGAINST      |  \e/  There is no  |
| http://ymorin.is-a-geek.org/ | _/*\_ | / \ HTML MAIL    |   v   conspiracy.  |
'------------------------------^-------^------------------^--------------------'

[Buildroot] [PATCH-NEXT 2/4] package/linux-firmware.mk: get rid of temporary tarball for file installation

[Yann E. MORIN]

Peter, All,

On 2021-02-12 21:39 +0100, Yann E. MORIN spake thusly:
> On 2021-02-12 19:40 +0100, Peter Korsgaard spake thusly:
> > There is no specific reason to use a temporary file, we can just as well
> > pipe the two tar invocations together, so do that instead.
> Actually, there *is* a reason why an intermediate tarball is used, see
> commit 21a283ffb0d (linux-firmware: fail build for missing file).

Of course, I missed providing a suggestion, so here it is, see below...

[--SNIP--]
> > Signed-off-by: Peter Korsgaard <peter@korsgaard.com>
> > ---
> >  package/linux-firmware/linux-firmware.mk | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/package/linux-firmware/linux-firmware.mk b/package/linux-firmware/linux-firmware.mk
> > index 3420266335..b6549ad371 100644
> > --- a/package/linux-firmware/linux-firmware.mk
> > +++ b/package/linux-firmware/linux-firmware.mk
> > @@ -641,8 +641,8 @@ endif
> >  ifneq ($(LINUX_FIRMWARE_FILES),)
> >  define LINUX_FIRMWARE_INSTALL_FILES
> >  	cd $(@D) && \
> > -		$(TAR) cf install.tar $(sort $(LINUX_FIRMWARE_FILES)) && \
> > -		$(TAR) xf install.tar -C $(1)
> > +		$(TAR) c $(sort $(LINUX_FIRMWARE_FILES)) | \
> > +		$(TAR) x -C $(1)

First, I think we should merge INSTALL_FILES and INSTALL_DIRS together
in a single command; there is no reason why we could not put directories
in a tarball...

Seconf, we should =create the archive once in _BUILD_CMDS. Then we can
extract it in _INSTALL_TARGET_CMDS and _INSTALL_IMAGES_CMDS.

    define LINUX_FIRMWARE_BUILD_CMDS
        cd $(@D) && \
        $(TAR) cf br-firmware.tar $(sort $(LINUX_FIRMWARE_FILES) $(LINUX_FIRMWARE_DIRS))
    endef

    define LINUX_FIRMWARE_INSTALL
        mkdir -p $(1)
        $(TAR) xf $(@D)/br-firmware.tar -C $(1)
        sed -r -e '/^Link: (.+) -> (.+)$$/!d; s//\1 \2/' $(@D)/WHENCE |
            blabla actual code to create symlinks
    endef

    define LINUX_FIRMWARE_INSTALL_TARGET_CMDS
        $(call LINUX_FIRMWARE_INSTALL, $(TARGET_DIR)/lib/firmware)
    endef

    define LINUX_FIRMWARE_INSTALL_IMAGES_CMDS
        $(call LINUX_FIRMWARE_INSTALL, $(IMAGES_DIR)/linux-firmware)
    endef

Totally untested, written directly in the mail...

Regards,
Yann E. MORIN.

> >  endef
> >  endif
> >  
> > -- 
> > 2.20.1
> > 
> 
> -- 
> .-----------------.--------------------.------------------.--------------------.
> |  Yann E. MORIN  | Real-Time Embedded | /"\ ASCII RIBBON | Erics' conspiracy: |
> | +33 662 376 056 | Software  Designer | \ / CAMPAIGN     |  ___               |
> | +33 561 099 427 `------------.-------:  X  AGAINST      |  \e/  There is no  |
> | http://ymorin.is-a-geek.org/ | _/*\_ | / \ HTML MAIL    |   v   conspiracy.  |
> '------------------------------^-------^------------------^--------------------'
> _______________________________________________
> buildroot mailing list
> buildroot at busybox.net
> http://lists.busybox.net/mailman/listinfo/buildroot

-- 
.-----------------.--------------------.------------------.--------------------.
|  Yann E. MORIN  | Real-Time Embedded | /"\ ASCII RIBBON | Erics' conspiracy: |
| +33 662 376 056 | Software  Designer | \ / CAMPAIGN     |  ___               |
| +33 561 099 427 `------------.-------:  X  AGAINST      |  \e/  There is no  |
| http://ymorin.is-a-geek.org/ | _/*\_ | / \ HTML MAIL    |   v   conspiracy.  |
'------------------------------^-------^------------------^--------------------'

[Buildroot] [PATCH-NEXT 2/4] package/linux-firmware.mk: get rid of temporary tarball for file installation

[Peter Korsgaard]

>>>>> "Yann" == Yann E MORIN <yann.morin.1998@free.fr> writes:

 > Peter, All,
 > On 2021-02-12 19:40 +0100, Peter Korsgaard spake thusly:
 >> With the upcoming addition of the images-install step,
 >> LINUX_FIRMWARE_INSTALl_FILES is called twice, for installing to the target
 >> and to the images directory - Which may race with each other and cause
 >> problems with the temporary install.tar tarball.
 >> 
 >> There is no specific reason to use a temporary file, we can just as well
 >> pipe the two tar invocations together, so do that instead.

 > Actually, there *is* a reason why an intermediate tarball is used, see
 > commit 21a283ffb0d (linux-firmware: fail build for missing file).

 > The idea is that, when we bump the version a firmware file may get
 > removed, and we may not notice right when updating (because of the 
 > many sub-options), and that file is still isted.

 > If that is the case, then the first tar will bail out on that missing
 > file, and stop right away, thus not including the followign files. But
 > because it is on the LHS of a pipe, its return code is ignored.

 > Then the second tar is still happy with what it gets, because it is a
 > proper tar archive (if the last file in a tarball was extracted
 > successfully, there is no way to know if the tarball was truncated or
 > not).

 > As a consequence, the copy is only partial, and the build does not fail.

Ok, so we need to stick a set -o pipefail before?

-- 
Bye, Peter Korsgaard

[Buildroot] [PATCH-NEXT 2/4] package/linux-firmware.mk: get rid of temporary tarball for file installation

[Yann E. MORIN]

Peter, All,

On 2021-02-12 22:07 +0100, Yann E. MORIN spake thusly:
> On 2021-02-12 21:39 +0100, Yann E. MORIN spake thusly:
> > On 2021-02-12 19:40 +0100, Peter Korsgaard spake thusly:
> > > There is no specific reason to use a temporary file, we can just as well
> > > pipe the two tar invocations together, so do that instead.
> > Actually, there *is* a reason why an intermediate tarball is used, see
> > commit 21a283ffb0d (linux-firmware: fail build for missing file).
> 
> Of course, I missed providing a suggestion, so here it is, see below...
> 
> [--SNIP--]
> > > Signed-off-by: Peter Korsgaard <peter@korsgaard.com>
> > > ---
> > >  package/linux-firmware/linux-firmware.mk | 4 ++--
> > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/package/linux-firmware/linux-firmware.mk b/package/linux-firmware/linux-firmware.mk
> > > index 3420266335..b6549ad371 100644
> > > --- a/package/linux-firmware/linux-firmware.mk
> > > +++ b/package/linux-firmware/linux-firmware.mk
> > > @@ -641,8 +641,8 @@ endif
> > >  ifneq ($(LINUX_FIRMWARE_FILES),)
> > >  define LINUX_FIRMWARE_INSTALL_FILES
> > >  	cd $(@D) && \
> > > -		$(TAR) cf install.tar $(sort $(LINUX_FIRMWARE_FILES)) && \
> > > -		$(TAR) xf install.tar -C $(1)
> > > +		$(TAR) c $(sort $(LINUX_FIRMWARE_FILES)) | \
> > > +		$(TAR) x -C $(1)
> 
> First, I think we should merge INSTALL_FILES and INSTALL_DIRS together
> in a single command; there is no reason why we could not put directories
> in a tarball...
> 
> Seconf, we should =create the archive once in _BUILD_CMDS. Then we can
> extract it in _INSTALL_TARGET_CMDS and _INSTALL_IMAGES_CMDS.

I whipped up something quick during my morning coffee:

https://git.buildroot.org/~ymorin/git/buildroot/commit/?h=yem/linux-fw-rework&id=8dd299681fdf80c284680833175387ac6a428a11

Lightly tested by enabling a random set of firmwares, and comparing the
lists of installed files before/after are identical. Then tested by
enabling all firmwares. No delta in either case: it installed the same
set of files, and there was no missing/extra ones.

Regards,
Yann E. MORIN.

-- 
.-----------------.--------------------.------------------.--------------------.
|  Yann E. MORIN  | Real-Time Embedded | /"\ ASCII RIBBON | Erics' conspiracy: |
| +33 662 376 056 | Software  Designer | \ / CAMPAIGN     |  ___               |
| +33 561 099 427 `------------.-------:  X  AGAINST      |  \e/  There is no  |
| http://ymorin.is-a-geek.org/ | _/*\_ | / \ HTML MAIL    |   v   conspiracy.  |
'------------------------------^-------^------------------^--------------------'

[Buildroot] [PATCH-NEXT 2/4] package/linux-firmware.mk: get rid of temporary tarball for file installation

[Yann E. MORIN]

Peter, All,

On 2021-02-12 22:37 +0100, Peter Korsgaard spake thusly:
> >>>>> "Yann" == Yann E MORIN <yann.morin.1998@free.fr> writes:
>  > Peter, All,
>  > On 2021-02-12 19:40 +0100, Peter Korsgaard spake thusly:
>  >> There is no specific reason to use a temporary file, we can just as well
>  >> pipe the two tar invocations together, so do that instead.
>  > Actually, there *is* a reason why an intermediate tarball is used, see
>  > commit 21a283ffb0d (linux-firmware: fail build for missing file).
> Ok, so we need to stick a set -o pipefail before?

That wqe'd have to use set -o pipefail is ugly ;-)

I prefer if we could cleanup the install sequence, as I suggested and
proposed in another reply.

Regards,
Yann E. MORIN.

-- 
.-----------------.--------------------.------------------.--------------------.
|  Yann E. MORIN  | Real-Time Embedded | /"\ ASCII RIBBON | Erics' conspiracy: |
| +33 662 376 056 | Software  Designer | \ / CAMPAIGN     |  ___               |
| +33 561 099 427 `------------.-------:  X  AGAINST      |  \e/  There is no  |
| http://ymorin.is-a-geek.org/ | _/*\_ | / \ HTML MAIL    |   v   conspiracy.  |
'------------------------------^-------^------------------^--------------------'

[Buildroot] [PATCH-NEXT 2/4] package/linux-firmware.mk: get rid of temporary tarball for file installation

[Peter Korsgaard]

>>>>> "Yann" == Yann E MORIN <yann.morin.1998@free.fr> writes:

Hi,

 > I whipped up something quick during my morning coffee:

 > https://git.buildroot.org/~ymorin/git/buildroot/commit/?h=yem/linux-fw-rework&id=8dd299681fdf80c284680833175387ac6a428a11

 > Lightly tested by enabling a random set of firmwares, and comparing the
 > lists of installed files before/after are identical. Then tested by
 > enabling all firmwares. No delta in either case: it installed the same
 > set of files, and there was no missing/extra ones.

Ok, that looks good. I see that you dropped the installation macros, so
how do you suggest to handle the installation to $BINARIES_DIR? Copy the
entire installation logic?

-- 
Bye, Peter Korsgaard

[Buildroot] [PATCH-NEXT 2/4] package/linux-firmware.mk: get rid of temporary tarball for file installation

[Peter Korsgaard]

>>>>> "Yann" == Yann E MORIN <yann.morin.1998@free.fr> writes:

 > Peter, All,
 > On 2021-02-12 22:37 +0100, Peter Korsgaard spake thusly:
 >> >>>>> "Yann" == Yann E MORIN <yann.morin.1998@free.fr> writes:
 >> > Peter, All,
 >> > On 2021-02-12 19:40 +0100, Peter Korsgaard spake thusly:
 >> >> There is no specific reason to use a temporary file, we can just as well
 >> >> pipe the two tar invocations together, so do that instead.
 >> > Actually, there *is* a reason why an intermediate tarball is used, see
 >> > commit 21a283ffb0d (linux-firmware: fail build for missing file).
 >> Ok, so we need to stick a set -o pipefail before?

 > That wqe'd have to use set -o pipefail is ugly ;-)

 > I prefer if we could cleanup the install sequence, as I suggested and
 > proposed in another reply.

Well, having our temporary tarball in the build directory is also kind
of ugly, but ok - Fine by me.

-- 
Bye, Peter Korsgaard

[Buildroot] [PATCH-NEXT 2/4] package/linux-firmware.mk: get rid of temporary tarball for file installation

[Yann E. MORIN]

Peter, All,

On 2021-02-13 10:47 +0100, Peter Korsgaard spake thusly:
> >>>>> "Yann" == Yann E MORIN <yann.morin.1998@free.fr> writes:
> 
>  > Peter, All,
>  > On 2021-02-12 22:37 +0100, Peter Korsgaard spake thusly:
>  >> >>>>> "Yann" == Yann E MORIN <yann.morin.1998@free.fr> writes:
>  >> > Peter, All,
>  >> > On 2021-02-12 19:40 +0100, Peter Korsgaard spake thusly:
>  >> >> There is no specific reason to use a temporary file, we can just as well
>  >> >> pipe the two tar invocations together, so do that instead.
>  >> > Actually, there *is* a reason why an intermediate tarball is used, see
>  >> > commit 21a283ffb0d (linux-firmware: fail build for missing file).
>  >> Ok, so we need to stick a set -o pipefail before?
>  > I prefer if we could cleanup the install sequence, as I suggested and
>  > proposed in another reply.
> Well, having our temporary tarball in the build directory is also kind
> of ugly, but ok - Fine by me.

That will not be worse than now: we already leave the temporary tarball
in the build directory...

Regards,
Yann E. MORIN.

-- 
.-----------------.--------------------.------------------.--------------------.
|  Yann E. MORIN  | Real-Time Embedded | /"\ ASCII RIBBON | Erics' conspiracy: |
| +33 662 376 056 | Software  Designer | \ / CAMPAIGN     |  ___               |
| +33 561 099 427 `------------.-------:  X  AGAINST      |  \e/  There is no  |
| http://ymorin.is-a-geek.org/ | _/*\_ | / \ HTML MAIL    |   v   conspiracy.  |
'------------------------------^-------^------------------^--------------------'

[Buildroot] [PATCH-NEXT 2/4] package/linux-firmware.mk: get rid of temporary tarball for file installation

[Yann E. MORIN]

Peter, All,

On 2021-02-13 10:46 +0100, Peter Korsgaard spake thusly:
> >>>>> "Yann" == Yann E MORIN <yann.morin.1998@free.fr> writes:
>  > I whipped up something quick during my morning coffee:
>  > https://git.buildroot.org/~ymorin/git/buildroot/commit/?h=yem/linux-fw-rework&id=8dd299681fdf80c284680833175387ac6a428a11
> Ok, that looks good. I see that you dropped the installation macros, so
> how do you suggest to handle the installation to $BINARIES_DIR? Copy the
> entire installation logic?

Ah, this patch is juste a cleanup. It replaces your patch 2.

The idea was that, after this cleanup, you'd adapt your patch 1 n top of
it, with something like:

    diff --git a/package/linux-firmware/linux-firmware.mk b/package/linux-firmware/linux-firmware.mk
    index 9bc59435ef..8defedd309 100644
    --- a/package/linux-firmware/linux-firmware.mk
    +++ b/package/linux-firmware/linux-firmware.mk
    @@ -674,10 +674,10 @@ endif
     # sure we canonicalize the pointed-to file, to cover the symlinks of the form
     # a/foo -> ../b/foo  where a/ (the directory where to put the symlink) does
     # not yet exist.
    -define LINUX_FIRMWARE_INSTALL_TARGET_CMDS
    -	mkdir -p $(TARGET_DIR)/lib/firmware
    -	$(TAR) xf $(@D)/br-firmware.tar -C $(TARGET_DIR)/lib/firmware/
    -	cd $(TARGET_DIR)/lib/firmware/ ; \
    +define LINUX_FIRMWARE_INSTALL_FW
    +	mkdir -p $(1)
    +	$(TAR) xf $(@D)/br-firmware.tar -C $(1)
    +	cd $(1) ; \
     	sed -r -e '/^Link: (.+) -> (.+)$$/!d; s//\1 \2/' $(@D)/WHENCE | \
     	while read f d; do \
     		if test -f $$(readlink -m $$(dirname $$f)/$$d); then \
    @@ -687,4 +687,8 @@ define LINUX_FIRMWARE_INSTALL_TARGET_CMDS
     	done
     endef
     
    +define LINUX_FIRMWARE_INSTALL_TARGET_CMDS
    +	$(call LINUX_FIRMWARE_INSTALL_FW, $(TARGET_DIR)/lib/firmware)
    +endef
    +
     $(eval $(generic-package))

And then your patch 3 look like:

    diff --git a/package/linux-firmware/linux-firmware.mk b/package/linux-firmware/linux-firmware.mk
    index 9bc59435ef..8defedd309 100644
    --- a/package/linux-firmware/linux-firmware.mk
    +++ b/package/linux-firmware/linux-firmware.mk
    @@ -691,4 +691,8 @@ define LINUX_FIRMWARE_INSTALL_TARGET_CMDS
     	$(call LINUX_FIRMWARE_INSTALL_FW, $(TARGET_DIR)/lib/firmware)
     endef
     
    +define LINUX_FIRMWARE_INSTALL_IMAGES_CMDS
    +	$(call LINUX_FIRMWARE_INSTALL_FW, $(BINARIES_DIR)/linux-firmware)
    +endef
    +
     $(eval $(generic-package))

And then your final patch 4.

Regards,
Yann E. MORIN.

-- 
.-----------------.--------------------.------------------.--------------------.
|  Yann E. MORIN  | Real-Time Embedded | /"\ ASCII RIBBON | Erics' conspiracy: |
| +33 662 376 056 | Software  Designer | \ / CAMPAIGN     |  ___               |
| +33 561 099 427 `------------.-------:  X  AGAINST      |  \e/  There is no  |
| http://ymorin.is-a-geek.org/ | _/*\_ | / \ HTML MAIL    |   v   conspiracy.  |
'------------------------------^-------^------------------^--------------------'

[Buildroot] [PATCH-NEXT 4/4] linux: build after linux-firmware if enabled for early loading support

[Yann E. MORIN]

On 2021-02-12 19:40 +0100, Peter Korsgaard spake thusly:
> To support building in (a subset of) the linux-firmware files into the
> kernel using the CONFIG_EXTRA_FIRMWARE option, we need to ensure that the
> firmware files are installed before the Linux kernel is built, similar to
> how it is done for intel-microcode.
> 
> Signed-off-by: Peter Korsgaard <peter@korsgaard.com>
> ---
>  linux/linux.mk | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/linux/linux.mk b/linux/linux.mk
> index a212f42c28..5e4b319cf1 100644
> --- a/linux/linux.mk
> +++ b/linux/linux.mk
> @@ -78,7 +78,8 @@ LINUX_MAKE_ENV = \
>  
>  LINUX_INSTALL_IMAGES = YES
>  LINUX_DEPENDENCIES = host-kmod \
> -	$(if $(BR2_PACKAGE_INTEL_MICROCODE),intel-microcode)
> +	$(if $(BR2_PACKAGE_INTEL_MICROCODE),intel-microcode) \
> +	$(if $(BR2_PACKAGE_LINUX_FIRMWARE),linux-firmware)

You also need to tell the kernel where to find those firmware files,
with CONFIG_EXTRA_FIRMWARE_DIR, otherwise it will look in /lib/firmware.

So we'd need something like:

    $(if $(BR2_PACKAGE_LINUX_FIRMWARE),
        $(call KCONFIG_SET_OPT,CONFIG_EXTRA_FIRMWARE_DIR,"$${BR_BINARIES_DIR}/linux-firmware"))

(note the $$ escaping and the use of a shell-level variable, like is
done for CONFIG_INITRAMFS_SOURCE).

Regards,
Yann E. MORIN.

>  # Starting with 4.16, the generated kconfig paser code is no longer
>  # shipped with the kernel sources, so we need flex and bison, but
> -- 
> 2.20.1
> 
> _______________________________________________
> buildroot mailing list
> buildroot at busybox.net
> http://lists.busybox.net/mailman/listinfo/buildroot

-- 
.-----------------.--------------------.------------------.--------------------.
|  Yann E. MORIN  | Real-Time Embedded | /"\ ASCII RIBBON | Erics' conspiracy: |
| +33 662 376 056 | Software  Designer | \ / CAMPAIGN     |  ___               |
| +33 561 099 427 `------------.-------:  X  AGAINST      |  \e/  There is no  |
| http://ymorin.is-a-geek.org/ | _/*\_ | / \ HTML MAIL    |   v   conspiracy.  |
'------------------------------^-------^------------------^--------------------'

[Buildroot] [PATCH-NEXT 2/4] package/linux-firmware.mk: get rid of temporary tarball for file installation

[Peter Korsgaard]

>>>>> "Yann" == Yann E MORIN <yann.morin.1998@free.fr> writes:

Hi,

 >> >> Ok, so we need to stick a set -o pipefail before?
 >> > I prefer if we could cleanup the install sequence, as I suggested and
 >> > proposed in another reply.
 >> Well, having our temporary tarball in the build directory is also kind
 >> of ugly, but ok - Fine by me.

 > That will not be worse than now: we already leave the temporary tarball
 > in the build directory...

Yes, I know - Which was part of the reason for me wanting to change
it. But oh well, it is not a big deal.

-- 
Bye, Peter Korsgaard

[Buildroot] [PATCH-NEXT 2/4] package/linux-firmware.mk: get rid of temporary tarball for file installation

[Yann E. MORIN]

Peter, All,

On 2021-02-13 11:14 +0100, Peter Korsgaard spake thusly:
> >>>>> "Yann" == Yann E MORIN <yann.morin.1998@free.fr> writes:
>  >> >> Ok, so we need to stick a set -o pipefail before?
>  >> > I prefer if we could cleanup the install sequence, as I suggested and
>  >> > proposed in another reply.
>  >> Well, having our temporary tarball in the build directory is also kind
>  >> of ugly, but ok - Fine by me.
>  > That will not be worse than now: we already leave the temporary tarball
>  > in the build directory...
> Yes, I know - Which was part of the reason for me wanting to change
> it. But oh well, it is not a big deal.

Ah, sorry, I miss-read that part, then...

But as I said in the commit log of that proposal of mine, I don't see
how we can do it easily without a tarball... But if you think of a
better alternative, I'm all ears^Weyes. ;-)

Regards,
Yann E. MORIN.

-- 
.-----------------.--------------------.------------------.--------------------.
|  Yann E. MORIN  | Real-Time Embedded | /"\ ASCII RIBBON | Erics' conspiracy: |
| +33 662 376 056 | Software  Designer | \ / CAMPAIGN     |  ___               |
| +33 561 099 427 `------------.-------:  X  AGAINST      |  \e/  There is no  |
| http://ymorin.is-a-geek.org/ | _/*\_ | / \ HTML MAIL    |   v   conspiracy.  |
'------------------------------^-------^------------------^--------------------'

[Buildroot] [PATCH-NEXT 2/4] package/linux-firmware.mk: get rid of temporary tarball for file installation

[Peter Korsgaard]

>>>>> "Yann" == Yann E MORIN <yann.morin.1998@free.fr> writes:

 > Peter, All,
 > On 2021-02-13 11:14 +0100, Peter Korsgaard spake thusly:
 >> >>>>> "Yann" == Yann E MORIN <yann.morin.1998@free.fr> writes:
 >> >> >> Ok, so we need to stick a set -o pipefail before?
 >> >> > I prefer if we could cleanup the install sequence, as I suggested and
 >> >> > proposed in another reply.
 >> >> Well, having our temporary tarball in the build directory is also kind
 >> >> of ugly, but ok - Fine by me.
 >> > That will not be worse than now: we already leave the temporary tarball
 >> > in the build directory...
 >> Yes, I know - Which was part of the reason for me wanting to change
 >> it. But oh well, it is not a big deal.

 > Ah, sorry, I miss-read that part, then...

 > But as I said in the commit log of that proposal of mine, I don't see
 > how we can do it easily without a tarball... But if you think of a
 > better alternative, I'm all ears^Weyes. ;-)

Well, it is either a tarball or using set -o pipefail. Anyway, the
tarball is fine by me.

-- 
Bye, Peter Korsgaard

[Buildroot] [PATCH-NEXT 4/4] linux: build after linux-firmware if enabled for early loading support

[Peter Korsgaard]

>>>>> "Yann" == Yann E MORIN <yann.morin.1998@free.fr> writes:

 > On 2021-02-12 19:40 +0100, Peter Korsgaard spake thusly:
 >> To support building in (a subset of) the linux-firmware files into the
 >> kernel using the CONFIG_EXTRA_FIRMWARE option, we need to ensure that the
 >> firmware files are installed before the Linux kernel is built, similar to
 >> how it is done for intel-microcode.
 >> 
 >> Signed-off-by: Peter Korsgaard <peter@korsgaard.com>
 >> ---
 >> linux/linux.mk | 3 ++-
 >> 1 file changed, 2 insertions(+), 1 deletion(-)
 >> 
 >> diff --git a/linux/linux.mk b/linux/linux.mk
 >> index a212f42c28..5e4b319cf1 100644
 >> --- a/linux/linux.mk
 >> +++ b/linux/linux.mk
 >> @@ -78,7 +78,8 @@ LINUX_MAKE_ENV = \
 >> 
 >> LINUX_INSTALL_IMAGES = YES
 >> LINUX_DEPENDENCIES = host-kmod \
 >> -	$(if $(BR2_PACKAGE_INTEL_MICROCODE),intel-microcode)
 >> +	$(if $(BR2_PACKAGE_INTEL_MICROCODE),intel-microcode) \
 >> +	$(if $(BR2_PACKAGE_LINUX_FIRMWARE),linux-firmware)

 > You also need to tell the kernel where to find those firmware files,
 > with CONFIG_EXTRA_FIRMWARE_DIR, otherwise it will look in /lib/firmware.

Yes, but that you can take care of in linux config file/fragment. We
already expose BR2_BINARIES_DIR in the environment for this, E.G. you
can do:

CONFIG_EXTRA_FIRMWARE_DIR="${BR_BINARIES_DIR}"
CONFIG_EXTRA_FIRMWARE="intel-ucode/06-5e-03 i915/kbl_dmc_ver1_04.bin"

(or whatever files you want to include).

 > So we'd need something like:

 >     $(if $(BR2_PACKAGE_LINUX_FIRMWARE),
 >         $(call KCONFIG_SET_OPT,CONFIG_EXTRA_FIRMWARE_DIR,"$${BR_BINARIES_DIR}/linux-firmware"))

NIT: Only ${BR_BINARIES_DIR}, not a linux-firmware subdir, otherwise you
cannot include both microcode and linux-firmware files.


 > (note the $$ escaping and the use of a shell-level variable, like is
 > done for CONFIG_INITRAMFS_SOURCE).

Given that the user already has to use a custom kernel config
file/fragment to specify what files to include, I don't think it makes
sense to override the CONFIG_EXTRA_FIRMWARE_DIR setting. We don't do it
for intel-microcode either.

-- 
Bye, Peter Korsgaard

[Buildroot] [PATCH-NEXT 4/4] linux: build after linux-firmware if enabled for early loading support

[Yann E. MORIN]

On 2021-02-13 11:32 +0100, Peter Korsgaard spake thusly:
> >>>>> "Yann" == Yann E MORIN <yann.morin.1998@free.fr> writes:
> 
>  > On 2021-02-12 19:40 +0100, Peter Korsgaard spake thusly:
>  >> To support building in (a subset of) the linux-firmware files into the
>  >> kernel using the CONFIG_EXTRA_FIRMWARE option, we need to ensure that the
>  >> firmware files are installed before the Linux kernel is built, similar to
>  >> how it is done for intel-microcode.
>  >> 
>  >> Signed-off-by: Peter Korsgaard <peter@korsgaard.com>
>  >> ---
>  >> linux/linux.mk | 3 ++-
>  >> 1 file changed, 2 insertions(+), 1 deletion(-)
>  >> 
>  >> diff --git a/linux/linux.mk b/linux/linux.mk
>  >> index a212f42c28..5e4b319cf1 100644
>  >> --- a/linux/linux.mk
>  >> +++ b/linux/linux.mk
>  >> @@ -78,7 +78,8 @@ LINUX_MAKE_ENV = \
>  >> 
>  >> LINUX_INSTALL_IMAGES = YES
>  >> LINUX_DEPENDENCIES = host-kmod \
>  >> -	$(if $(BR2_PACKAGE_INTEL_MICROCODE),intel-microcode)
>  >> +	$(if $(BR2_PACKAGE_INTEL_MICROCODE),intel-microcode) \
>  >> +	$(if $(BR2_PACKAGE_LINUX_FIRMWARE),linux-firmware)
> 
>  > You also need to tell the kernel where to find those firmware files,
>  > with CONFIG_EXTRA_FIRMWARE_DIR, otherwise it will look in /lib/firmware.
> 
> Yes, but that you can take care of in linux config file/fragment. We
> already expose BR2_BINARIES_DIR in the environment for this, E.G. you
> can do:
> 
> CONFIG_EXTRA_FIRMWARE_DIR="${BR_BINARIES_DIR}"
> CONFIG_EXTRA_FIRMWARE="intel-ucode/06-5e-03 i915/kbl_dmc_ver1_04.bin"
> 
> (or whatever files you want to include).

But don't we want to make that seamless for the user? If they don't have
CONFIG_EXTRA_FIRMWARE=y, then setting CONFIG_EXTRA_FIRMWARE_DIR is a
no-op.

I think it is not obvious for a user to know that they can use
${BR_BINARIES_DIR} in their kernel defconfig/fragment: this is
documented nowhere. Also, with the BR_ (not BR2_) prefix, it is in
that namespace which we intedned to be reserved (but that we never
enforced) for internal, non-public variables...

>  > So we'd need something like:
>  >     $(if $(BR2_PACKAGE_LINUX_FIRMWARE),
>  >         $(call KCONFIG_SET_OPT,CONFIG_EXTRA_FIRMWARE_DIR,"$${BR_BINARIES_DIR}/linux-firmware"))
> 
> NIT: Only ${BR_BINARIES_DIR}, not a linux-firmware subdir, otherwise you
> cannot include both microcode and linux-firmware files.

Meh.. That's not nice, because then it means BINARIES_DIR is clutterred
by all those firmware files.

Also, intel-microcode is in a sub-directory, and will make it less easy
for users to find the file(s) they will actually have to flash on their
devices...

Can we move those so that we can use: CONFIG_EXTRA_FIRMWARE_DIR="$(BINARIES_DIR)/firmware/"
(for example)?

>  > (note the $$ escaping and the use of a shell-level variable, like is
>  > done for CONFIG_INITRAMFS_SOURCE).
> Given that the user already has to use a custom kernel config
> file/fragment to specify what files to include, I don't think it makes
> sense to override the CONFIG_EXTRA_FIRMWARE_DIR setting. We don't do it
> for intel-microcode either.

I think we should, too.

Regards,
Yann E. MORIN.

-- 
.-----------------.--------------------.------------------.--------------------.
|  Yann E. MORIN  | Real-Time Embedded | /"\ ASCII RIBBON | Erics' conspiracy: |
| +33 662 376 056 | Software  Designer | \ / CAMPAIGN     |  ___               |
| +33 561 099 427 `------------.-------:  X  AGAINST      |  \e/  There is no  |
| http://ymorin.is-a-geek.org/ | _/*\_ | / \ HTML MAIL    |   v   conspiracy.  |
'------------------------------^-------^------------------^--------------------'

[Buildroot] [PATCH-NEXT 4/4] linux: build after linux-firmware if enabled for early loading support

[Peter Korsgaard]

>>>>> "Yann" == Yann E MORIN <yann.morin.1998@free.fr> writes:

Hi,

 >> Yes, but that you can take care of in linux config file/fragment. We
 >> already expose BR2_BINARIES_DIR in the environment for this, E.G. you
 >> can do:
 >> 
 >> CONFIG_EXTRA_FIRMWARE_DIR="${BR_BINARIES_DIR}"
 >> CONFIG_EXTRA_FIRMWARE="intel-ucode/06-5e-03 i915/kbl_dmc_ver1_04.bin"
 >> 
 >> (or whatever files you want to include).

 > But don't we want to make that seamless for the user? If they don't have
 > CONFIG_EXTRA_FIRMWARE=y, then setting CONFIG_EXTRA_FIRMWARE_DIR is a
 > no-op.

I would argue not. There is a cost to every kernel config override we
make (E.G. maintenance but also the confusion it leads to when you have
a config saying A but you end up with B because Buildroot tries to be
clever). It is not because you have linux-firmware enabled that there
couldn't be a use case for wanting to point CONFIG_EXTRA_FIRMWARE_DIR to
other firmware files (E.G. some custom firmwares in your board directory
or similar).


 > I think it is not obvious for a user to know that they can use
 > ${BR_BINARIES_DIR} in their kernel defconfig/fragment: this is
 > documented nowhere. Also, with the BR_ (not BR2_) prefix, it is in
 > that namespace which we intedned to be reserved (but that we never
 > enforced) for internal, non-public variables...

True. It dates back to 2015:

Author: Thomas De Schampheleire <thomas.de.schampheleire@gmail.com>
Date:   Tue Feb 3 15:21:48 2015 +0100

    linux: avoid unnecessary changes in defconfig for INITRAMFS_SOURCE

 >> NIT: Only ${BR_BINARIES_DIR}, not a linux-firmware subdir, otherwise you
 >> cannot include both microcode and linux-firmware files.

 > Meh.. That's not nice, because then it means BINARIES_DIR is clutterred
 > by all those firmware files.

True, but BINARIES_DIR is already like that today, only a few packages
use a subdir.

 > Also, intel-microcode is in a sub-directory, and will make it less easy
 > for users to find the file(s) they will actually have to flash on their
 > devices...

Yes and no. intel-microcode indeed installs its (flat tree of) files
under intel-ucode, but that directly matches what the kernel looks for
(intel-ucode/<file>). Likewise, the kernel looks for the i915 firmware
under i915/<file>, so to match the expectations of the kernel we will
need to install the linux-firmware files (E.G. the i915 directory)
directly in BINARIES_DIR.

Or alternatively move the intel-microcode AND linux-firmware files into
a sub directory like $BINARIES_DIR/firmware/intel-ucode/<file>), but I
am not sure if that is any nicer and it is perhaps not clear exactly
what belongs under firmware/ and what doesn't.

-- 
Bye, Peter Korsgaard