All of lore.kernel.org
 help / color / mirror / Atom feed
* [Buildroot] [PATCH] package/linux-firmware: fix symlink support
@ 2020-03-03 13:33 Antoine Tenart
  2020-03-03 13:41 ` Baruch Siach
                   ` (2 more replies)
  0 siblings, 3 replies; 10+ messages in thread
From: Antoine Tenart @ 2020-03-03 13:33 UTC (permalink / raw)
  To: buildroot

Since Linux-firmware's commit 9cfefbd7fbda ("Remove duplicate symlinks")
symlinks aren't distributed anymore. They are rather created at
installation time by a script provided in the project, copy-firmware.sh.
The description of the symlinks is done in the WHENCE file. Since the
bump to version 20200122, in commit 48cc1a89ae04, installation for many
firmwares was broken as Buildroot tried to install missing symlinks from
Linux-firmware.

The fix is not only to remove now missing symlinks, but to add logic to
create those symlinks as kernel modules will depend on them. The
solution taken by this patch is to create dynamically symlinks based on
their description in the WHENCE file *and* only if the file they'll
point to was installed in the target directory.

Fixes: 48cc1a89ae04 ("package/linux-firmware: bump to version 20200122")
Cc: james.hilliard1 at gmail.com
Signed-off-by: Antoine Tenart <antoine.tenart@bootlin.com>
---
 package/linux-firmware/linux-firmware.mk | 33 ++++++++++++++----------
 1 file changed, 19 insertions(+), 14 deletions(-)

diff --git a/package/linux-firmware/linux-firmware.mk b/package/linux-firmware/linux-firmware.mk
index f8e95d0648c9..125df74ef68d 100644
--- a/package/linux-firmware/linux-firmware.mk
+++ b/package/linux-firmware/linux-firmware.mk
@@ -71,9 +71,7 @@ endif
 
 # rt2xx
 ifeq ($(BR2_PACKAGE_LINUX_FIRMWARE_RALINK_RT2XX),y)
-# rt3090.bin is a symlink to rt2860.bin
-# rt3070.bin is a symlink to rt2870.bin
-LINUX_FIRMWARE_FILES += rt2860.bin rt2870.bin rt3070.bin rt3071.bin rt3090.bin
+LINUX_FIRMWARE_FILES += rt2860.bin rt2870.bin rt3071.bin
 LINUX_FIRMWARE_ALL_LICENSE_FILES += LICENCE.ralink-firmware.txt
 endif
 
@@ -214,8 +212,6 @@ endif
 
 # sd8688
 ifeq ($(BR2_PACKAGE_LINUX_FIRMWARE_LIBERTAS_SD8688),y)
-LINUX_FIRMWARE_FILES += libertas/sd8688.bin libertas/sd8688_helper.bin
-# The two files above are but symlinks to those two ones:
 LINUX_FIRMWARE_FILES += mrvl/sd8688.bin mrvl/sd8688_helper.bin
 LINUX_FIRMWARE_ALL_LICENSE_FILES += LICENCE.Marvell
 endif
@@ -320,12 +316,10 @@ endif
 
 # wl127x
 ifeq ($(BR2_PACKAGE_LINUX_FIRMWARE_TI_WL127X),y)
-# wl1271-nvs.bin is a symlink to wl127x-nvs.bin
 LINUX_FIRMWARE_FILES += \
 	ti-connectivity/wl1271-fw-2.bin \
 	ti-connectivity/wl1271-fw-ap.bin \
 	ti-connectivity/wl1271-fw.bin \
-	ti-connectivity/wl1271-nvs.bin \
 	ti-connectivity/wl127x-fw-3.bin \
 	ti-connectivity/wl127x-fw-plt-3.bin \
 	ti-connectivity/wl127x-nvs.bin \
@@ -341,15 +335,12 @@ endif
 
 # wl128x
 ifeq ($(BR2_PACKAGE_LINUX_FIRMWARE_TI_WL128X),y)
-# wl1271-nvs.bin and wl12xx-nvs.bin are symlinks to wl127x-nvs.bin
 LINUX_FIRMWARE_FILES += \
 	ti-connectivity/wl128x-fw-3.bin \
 	ti-connectivity/wl128x-fw-ap.bin \
 	ti-connectivity/wl128x-fw-plt-3.bin \
 	ti-connectivity/wl128x-fw.bin \
-	ti-connectivity/wl1271-nvs.bin \
 	ti-connectivity/wl128x-nvs.bin \
-	ti-connectivity/wl12xx-nvs.bin \
 	ti-connectivity/wl127x-nvs.bin \
 	ti-connectivity/wl128x-fw-4-mr.bin \
 	ti-connectivity/wl128x-fw-4-plt.bin \
@@ -363,13 +354,11 @@ endif
 
 # wl18xx
 ifeq ($(BR2_PACKAGE_LINUX_FIRMWARE_TI_WL18XX),y)
-# wl1271-nvs.bin is a symlink to wl127x-nvs.bin
 LINUX_FIRMWARE_FILES += \
 	ti-connectivity/wl18xx-fw.bin \
 	ti-connectivity/wl18xx-fw-2.bin \
 	ti-connectivity/wl18xx-fw-3.bin \
 	ti-connectivity/wl18xx-fw-4.bin \
-	ti-connectivity/wl1271-nvs.bin \
 	ti-connectivity/wl127x-nvs.bin \
 	ti-connectivity/TIInit_7.2.31.bts
 LINUX_FIRMWARE_ALL_LICENSE_FILES += LICENCE.ti-connectivity
@@ -568,8 +557,7 @@ LINUX_FIRMWARE_FILES += \
 endif
 
 ifeq ($(BR2_PACKAGE_LINUX_FIRMWARE_QAT_DH895XCC),y)
-# qat_mmp.bin is a symlink to qat_895xcc_mmp.bin
-LINUX_FIRMWARE_FILES += qat_895xcc.bin qat_895xcc_mmp.bin qat_mmp.bin
+LINUX_FIRMWARE_FILES += qat_895xcc.bin qat_895xcc_mmp.bin
 LINUX_FIRMWARE_ALL_LICENSE_FILES += LICENCE.qat_firmware
 endif
 
@@ -626,4 +614,21 @@ define LINUX_FIRMWARE_INSTALL_TARGET_CMDS
 	$(LINUX_FIRMWARE_INSTALL_DIRS)
 endef
 
+# Some firmware are distributed as a symlink, for drivers to load them using a
+# defined name other than the real one. Since 9cfefbd7fbda ("Remove duplicate
+# symlinks") those symlink aren't distributed in linux-firmware but are created
+# automatically by its copy-firmware.sh script during the installation, which
+# parses the WHENCE file where symlinks are described. We follow the same logic
+# here, adding symlink only for firmwares installed in the target directory.
+# The grep/sed parsing is taken from the script mentioned before.
+define LINUX_FIRMWARE_CREATE_SYMLINKS
+	grep -E '^Link:' $(@D)/WHENCE | sed -e's/^Link: *//g' -e's/-> //g' | while read f d; do \
+		if test -f $(TARGET_DIR)/lib/firmware/$$d; then \
+			ln -sf $$d $(TARGET_DIR)/lib/firmware/$$f ; \
+		fi ; \
+	done
+endef
+
+LINUX_FIRMWARE_POST_INSTALL_TARGET_HOOKS += LINUX_FIRMWARE_CREATE_SYMLINKS
+
 $(eval $(generic-package))
-- 
2.24.1

^ permalink raw reply related	[flat|nested] 10+ messages in thread
* [Buildroot] [PATCH] package/linux-firmware: fix symlink support
  2020-03-03 13:33 [Buildroot] [PATCH] package/linux-firmware: fix symlink support Antoine Tenart
@ 2020-03-03 13:41 ` Baruch Siach
  2020-03-03 15:10   ` Yann E. MORIN
  2020-03-03 16:43 ` Yann E. MORIN
  2020-03-03 21:28 ` Yann E. MORIN
  2 siblings, 1 reply; 10+ messages in thread
From: Baruch Siach @ 2020-03-03 13:41 UTC (permalink / raw)
  To: buildroot

Hi Antoine,

On Tue, Mar 03, 2020 at 02:33:56PM +0100, Antoine Tenart wrote:
> Since Linux-firmware's commit 9cfefbd7fbda ("Remove duplicate symlinks")
> symlinks aren't distributed anymore. They are rather created at
> installation time by a script provided in the project, copy-firmware.sh.
> The description of the symlinks is done in the WHENCE file. Since the
> bump to version 20200122, in commit 48cc1a89ae04, installation for many
> firmwares was broken as Buildroot tried to install missing symlinks from
> Linux-firmware.
> 
> The fix is not only to remove now missing symlinks, but to add logic to
> create those symlinks as kernel modules will depend on them. The
> solution taken by this patch is to create dynamically symlinks based on
> their description in the WHENCE file *and* only if the file they'll
> point to was installed in the target directory.
> 
> Fixes: 48cc1a89ae04 ("package/linux-firmware: bump to version 20200122")
> Cc: james.hilliard1 at gmail.com
> Signed-off-by: Antoine Tenart <antoine.tenart@bootlin.com>

See commit 23d12793d5 ("package/linux-firmware: add missing symlinks") in the 
master branch.

baruch

> ---
>  package/linux-firmware/linux-firmware.mk | 33 ++++++++++++++----------
>  1 file changed, 19 insertions(+), 14 deletions(-)
> 
> diff --git a/package/linux-firmware/linux-firmware.mk b/package/linux-firmware/linux-firmware.mk
> index f8e95d0648c9..125df74ef68d 100644
> --- a/package/linux-firmware/linux-firmware.mk
> +++ b/package/linux-firmware/linux-firmware.mk
> @@ -71,9 +71,7 @@ endif
>  
>  # rt2xx
>  ifeq ($(BR2_PACKAGE_LINUX_FIRMWARE_RALINK_RT2XX),y)
> -# rt3090.bin is a symlink to rt2860.bin
> -# rt3070.bin is a symlink to rt2870.bin
> -LINUX_FIRMWARE_FILES += rt2860.bin rt2870.bin rt3070.bin rt3071.bin rt3090.bin
> +LINUX_FIRMWARE_FILES += rt2860.bin rt2870.bin rt3071.bin
>  LINUX_FIRMWARE_ALL_LICENSE_FILES += LICENCE.ralink-firmware.txt
>  endif
>  
> @@ -214,8 +212,6 @@ endif
>  
>  # sd8688
>  ifeq ($(BR2_PACKAGE_LINUX_FIRMWARE_LIBERTAS_SD8688),y)
> -LINUX_FIRMWARE_FILES += libertas/sd8688.bin libertas/sd8688_helper.bin
> -# The two files above are but symlinks to those two ones:
>  LINUX_FIRMWARE_FILES += mrvl/sd8688.bin mrvl/sd8688_helper.bin
>  LINUX_FIRMWARE_ALL_LICENSE_FILES += LICENCE.Marvell
>  endif
> @@ -320,12 +316,10 @@ endif
>  
>  # wl127x
>  ifeq ($(BR2_PACKAGE_LINUX_FIRMWARE_TI_WL127X),y)
> -# wl1271-nvs.bin is a symlink to wl127x-nvs.bin
>  LINUX_FIRMWARE_FILES += \
>  	ti-connectivity/wl1271-fw-2.bin \
>  	ti-connectivity/wl1271-fw-ap.bin \
>  	ti-connectivity/wl1271-fw.bin \
> -	ti-connectivity/wl1271-nvs.bin \
>  	ti-connectivity/wl127x-fw-3.bin \
>  	ti-connectivity/wl127x-fw-plt-3.bin \
>  	ti-connectivity/wl127x-nvs.bin \
> @@ -341,15 +335,12 @@ endif
>  
>  # wl128x
>  ifeq ($(BR2_PACKAGE_LINUX_FIRMWARE_TI_WL128X),y)
> -# wl1271-nvs.bin and wl12xx-nvs.bin are symlinks to wl127x-nvs.bin
>  LINUX_FIRMWARE_FILES += \
>  	ti-connectivity/wl128x-fw-3.bin \
>  	ti-connectivity/wl128x-fw-ap.bin \
>  	ti-connectivity/wl128x-fw-plt-3.bin \
>  	ti-connectivity/wl128x-fw.bin \
> -	ti-connectivity/wl1271-nvs.bin \
>  	ti-connectivity/wl128x-nvs.bin \
> -	ti-connectivity/wl12xx-nvs.bin \
>  	ti-connectivity/wl127x-nvs.bin \
>  	ti-connectivity/wl128x-fw-4-mr.bin \
>  	ti-connectivity/wl128x-fw-4-plt.bin \
> @@ -363,13 +354,11 @@ endif
>  
>  # wl18xx
>  ifeq ($(BR2_PACKAGE_LINUX_FIRMWARE_TI_WL18XX),y)
> -# wl1271-nvs.bin is a symlink to wl127x-nvs.bin
>  LINUX_FIRMWARE_FILES += \
>  	ti-connectivity/wl18xx-fw.bin \
>  	ti-connectivity/wl18xx-fw-2.bin \
>  	ti-connectivity/wl18xx-fw-3.bin \
>  	ti-connectivity/wl18xx-fw-4.bin \
> -	ti-connectivity/wl1271-nvs.bin \
>  	ti-connectivity/wl127x-nvs.bin \
>  	ti-connectivity/TIInit_7.2.31.bts
>  LINUX_FIRMWARE_ALL_LICENSE_FILES += LICENCE.ti-connectivity
> @@ -568,8 +557,7 @@ LINUX_FIRMWARE_FILES += \
>  endif
>  
>  ifeq ($(BR2_PACKAGE_LINUX_FIRMWARE_QAT_DH895XCC),y)
> -# qat_mmp.bin is a symlink to qat_895xcc_mmp.bin
> -LINUX_FIRMWARE_FILES += qat_895xcc.bin qat_895xcc_mmp.bin qat_mmp.bin
> +LINUX_FIRMWARE_FILES += qat_895xcc.bin qat_895xcc_mmp.bin
>  LINUX_FIRMWARE_ALL_LICENSE_FILES += LICENCE.qat_firmware
>  endif
>  
> @@ -626,4 +614,21 @@ define LINUX_FIRMWARE_INSTALL_TARGET_CMDS
>  	$(LINUX_FIRMWARE_INSTALL_DIRS)
>  endef
>  
> +# Some firmware are distributed as a symlink, for drivers to load them using a
> +# defined name other than the real one. Since 9cfefbd7fbda ("Remove duplicate
> +# symlinks") those symlink aren't distributed in linux-firmware but are created
> +# automatically by its copy-firmware.sh script during the installation, which
> +# parses the WHENCE file where symlinks are described. We follow the same logic
> +# here, adding symlink only for firmwares installed in the target directory.
> +# The grep/sed parsing is taken from the script mentioned before.
> +define LINUX_FIRMWARE_CREATE_SYMLINKS
> +	grep -E '^Link:' $(@D)/WHENCE | sed -e's/^Link: *//g' -e's/-> //g' | while read f d; do \
> +		if test -f $(TARGET_DIR)/lib/firmware/$$d; then \
> +			ln -sf $$d $(TARGET_DIR)/lib/firmware/$$f ; \
> +		fi ; \
> +	done
> +endef
> +
> +LINUX_FIRMWARE_POST_INSTALL_TARGET_HOOKS += LINUX_FIRMWARE_CREATE_SYMLINKS
> +
>  $(eval $(generic-package))

-- 
     http://baruch.siach.name/blog/                  ~. .~   Tk Open Systems
=}------------------------------------------------ooO--U--Ooo------------{=
   - baruch at tkos.co.il - tel: +972.2.679.5364, http://www.tkos.co.il -

^ permalink raw reply	[flat|nested] 10+ messages in thread
* [Buildroot] [PATCH] package/linux-firmware: fix symlink support
  2020-03-03 13:41 ` Baruch Siach
@ 2020-03-03 15:10   ` Yann E. MORIN
  0 siblings, 0 replies; 10+ messages in thread
From: Yann E. MORIN @ 2020-03-03 15:10 UTC (permalink / raw)
  To: buildroot

Baruch, All,

On 2020-03-03 15:41 +0200, Baruch Siach spake thusly:
> Hi Antoine,
> 
> On Tue, Mar 03, 2020 at 02:33:56PM +0100, Antoine Tenart wrote:
> > Since Linux-firmware's commit 9cfefbd7fbda ("Remove duplicate symlinks")
> > symlinks aren't distributed anymore. They are rather created at
> > installation time by a script provided in the project, copy-firmware.sh.
> > The description of the symlinks is done in the WHENCE file. Since the
> > bump to version 20200122, in commit 48cc1a89ae04, installation for many
> > firmwares was broken as Buildroot tried to install missing symlinks from
> > Linux-firmware.
> > 
> > The fix is not only to remove now missing symlinks, but to add logic to
> > create those symlinks as kernel modules will depend on them. The
> > solution taken by this patch is to create dynamically symlinks based on
> > their description in the WHENCE file *and* only if the file they'll
> > point to was installed in the target directory.
> > 
> > Fixes: 48cc1a89ae04 ("package/linux-firmware: bump to version 20200122")
> > Cc: james.hilliard1 at gmail.com
> > Signed-off-by: Antoine Tenart <antoine.tenart@bootlin.com>
> 
> See commit 23d12793d5 ("package/linux-firmware: add missing symlinks") in the 
> master branch.

Indeed, but that's a screw up on my part: 23d12793d5 was supposed to be
applied on next, not master, and Antoine's patch is for next.

Besides, Antoine's patch implements exactly what I suggested earlier as
part of my reply to James:
    http://lists.busybox.net/pipermail/buildroot/2020-March/275504.html

So, as discussed on IRC with Antoine and Thomas, I'll cleanup my mess on
master (and handle the symlinks issue on next).

Regards,
Yann E. MORIN.

> baruch
> 
> > ---
> >  package/linux-firmware/linux-firmware.mk | 33 ++++++++++++++----------
> >  1 file changed, 19 insertions(+), 14 deletions(-)
> > 
> > diff --git a/package/linux-firmware/linux-firmware.mk b/package/linux-firmware/linux-firmware.mk
> > index f8e95d0648c9..125df74ef68d 100644
> > --- a/package/linux-firmware/linux-firmware.mk
> > +++ b/package/linux-firmware/linux-firmware.mk
> > @@ -71,9 +71,7 @@ endif
> >  
> >  # rt2xx
> >  ifeq ($(BR2_PACKAGE_LINUX_FIRMWARE_RALINK_RT2XX),y)
> > -# rt3090.bin is a symlink to rt2860.bin
> > -# rt3070.bin is a symlink to rt2870.bin
> > -LINUX_FIRMWARE_FILES += rt2860.bin rt2870.bin rt3070.bin rt3071.bin rt3090.bin
> > +LINUX_FIRMWARE_FILES += rt2860.bin rt2870.bin rt3071.bin
> >  LINUX_FIRMWARE_ALL_LICENSE_FILES += LICENCE.ralink-firmware.txt
> >  endif
> >  
> > @@ -214,8 +212,6 @@ endif
> >  
> >  # sd8688
> >  ifeq ($(BR2_PACKAGE_LINUX_FIRMWARE_LIBERTAS_SD8688),y)
> > -LINUX_FIRMWARE_FILES += libertas/sd8688.bin libertas/sd8688_helper.bin
> > -# The two files above are but symlinks to those two ones:
> >  LINUX_FIRMWARE_FILES += mrvl/sd8688.bin mrvl/sd8688_helper.bin
> >  LINUX_FIRMWARE_ALL_LICENSE_FILES += LICENCE.Marvell
> >  endif
> > @@ -320,12 +316,10 @@ endif
> >  
> >  # wl127x
> >  ifeq ($(BR2_PACKAGE_LINUX_FIRMWARE_TI_WL127X),y)
> > -# wl1271-nvs.bin is a symlink to wl127x-nvs.bin
> >  LINUX_FIRMWARE_FILES += \
> >  	ti-connectivity/wl1271-fw-2.bin \
> >  	ti-connectivity/wl1271-fw-ap.bin \
> >  	ti-connectivity/wl1271-fw.bin \
> > -	ti-connectivity/wl1271-nvs.bin \
> >  	ti-connectivity/wl127x-fw-3.bin \
> >  	ti-connectivity/wl127x-fw-plt-3.bin \
> >  	ti-connectivity/wl127x-nvs.bin \
> > @@ -341,15 +335,12 @@ endif
> >  
> >  # wl128x
> >  ifeq ($(BR2_PACKAGE_LINUX_FIRMWARE_TI_WL128X),y)
> > -# wl1271-nvs.bin and wl12xx-nvs.bin are symlinks to wl127x-nvs.bin
> >  LINUX_FIRMWARE_FILES += \
> >  	ti-connectivity/wl128x-fw-3.bin \
> >  	ti-connectivity/wl128x-fw-ap.bin \
> >  	ti-connectivity/wl128x-fw-plt-3.bin \
> >  	ti-connectivity/wl128x-fw.bin \
> > -	ti-connectivity/wl1271-nvs.bin \
> >  	ti-connectivity/wl128x-nvs.bin \
> > -	ti-connectivity/wl12xx-nvs.bin \
> >  	ti-connectivity/wl127x-nvs.bin \
> >  	ti-connectivity/wl128x-fw-4-mr.bin \
> >  	ti-connectivity/wl128x-fw-4-plt.bin \
> > @@ -363,13 +354,11 @@ endif
> >  
> >  # wl18xx
> >  ifeq ($(BR2_PACKAGE_LINUX_FIRMWARE_TI_WL18XX),y)
> > -# wl1271-nvs.bin is a symlink to wl127x-nvs.bin
> >  LINUX_FIRMWARE_FILES += \
> >  	ti-connectivity/wl18xx-fw.bin \
> >  	ti-connectivity/wl18xx-fw-2.bin \
> >  	ti-connectivity/wl18xx-fw-3.bin \
> >  	ti-connectivity/wl18xx-fw-4.bin \
> > -	ti-connectivity/wl1271-nvs.bin \
> >  	ti-connectivity/wl127x-nvs.bin \
> >  	ti-connectivity/TIInit_7.2.31.bts
> >  LINUX_FIRMWARE_ALL_LICENSE_FILES += LICENCE.ti-connectivity
> > @@ -568,8 +557,7 @@ LINUX_FIRMWARE_FILES += \
> >  endif
> >  
> >  ifeq ($(BR2_PACKAGE_LINUX_FIRMWARE_QAT_DH895XCC),y)
> > -# qat_mmp.bin is a symlink to qat_895xcc_mmp.bin
> > -LINUX_FIRMWARE_FILES += qat_895xcc.bin qat_895xcc_mmp.bin qat_mmp.bin
> > +LINUX_FIRMWARE_FILES += qat_895xcc.bin qat_895xcc_mmp.bin
> >  LINUX_FIRMWARE_ALL_LICENSE_FILES += LICENCE.qat_firmware
> >  endif
> >  
> > @@ -626,4 +614,21 @@ define LINUX_FIRMWARE_INSTALL_TARGET_CMDS
> >  	$(LINUX_FIRMWARE_INSTALL_DIRS)
> >  endef
> >  
> > +# Some firmware are distributed as a symlink, for drivers to load them using a
> > +# defined name other than the real one. Since 9cfefbd7fbda ("Remove duplicate
> > +# symlinks") those symlink aren't distributed in linux-firmware but are created
> > +# automatically by its copy-firmware.sh script during the installation, which
> > +# parses the WHENCE file where symlinks are described. We follow the same logic
> > +# here, adding symlink only for firmwares installed in the target directory.
> > +# The grep/sed parsing is taken from the script mentioned before.
> > +define LINUX_FIRMWARE_CREATE_SYMLINKS
> > +	grep -E '^Link:' $(@D)/WHENCE | sed -e's/^Link: *//g' -e's/-> //g' | while read f d; do \
> > +		if test -f $(TARGET_DIR)/lib/firmware/$$d; then \
> > +			ln -sf $$d $(TARGET_DIR)/lib/firmware/$$f ; \
> > +		fi ; \
> > +	done
> > +endef
> > +
> > +LINUX_FIRMWARE_POST_INSTALL_TARGET_HOOKS += LINUX_FIRMWARE_CREATE_SYMLINKS
> > +
> >  $(eval $(generic-package))
> 
> -- 
>      http://baruch.siach.name/blog/                  ~. .~   Tk Open Systems
> =}------------------------------------------------ooO--U--Ooo------------{=
>    - baruch at tkos.co.il - tel: +972.2.679.5364, http://www.tkos.co.il -
> _______________________________________________
> 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.  |
'------------------------------^-------^------------------^--------------------'

^ permalink raw reply	[flat|nested] 10+ messages in thread
* [Buildroot] [PATCH] package/linux-firmware: fix symlink support
  2020-03-03 13:33 [Buildroot] [PATCH] package/linux-firmware: fix symlink support Antoine Tenart
  2020-03-03 13:41 ` Baruch Siach
@ 2020-03-03 16:43 ` Yann E. MORIN
  2020-03-03 18:47   ` Antoine Tenart
  2020-03-03 21:28 ` Yann E. MORIN
  2 siblings, 1 reply; 10+ messages in thread
From: Yann E. MORIN @ 2020-03-03 16:43 UTC (permalink / raw)
  To: buildroot

Antoine, All,

On 2020-03-03 14:33 +0100, Antoine Tenart spake thusly:
> Since Linux-firmware's commit 9cfefbd7fbda ("Remove duplicate symlinks")
> symlinks aren't distributed anymore. They are rather created at
> installation time by a script provided in the project, copy-firmware.sh.
> The description of the symlinks is done in the WHENCE file. Since the
> bump to version 20200122, in commit 48cc1a89ae04, installation for many
> firmwares was broken as Buildroot tried to install missing symlinks from
> Linux-firmware.
> 
> The fix is not only to remove now missing symlinks, but to add logic to
> create those symlinks as kernel modules will depend on them. The
> solution taken by this patch is to create dynamically symlinks based on
> their description in the WHENCE file *and* only if the file they'll
> point to was installed in the target directory.

Thanks for this patch, which basically does what I called for in a
previous review:
    http://lists.busybox.net/pipermail/buildroot/2020-March/275504.html

However, to avoid replicating my previous mistakes, I did a bit more
in-depth analysis of this symlink issue, and it is a bit more involved
than just this patch.

The problem is that we have bunch of LINUX_FIRMWARE_FILES that are
globs, for example:

    37 ifeq ($(BR2_PACKAGE_LINUX_FIRMWARE_QCOM_ADRENO),y)
    38 LINUX_FIRMWARE_FILES += qcom/a*
    39 LINUX_FIRMWARE_ALL_LICENSE_FILES += LICENSE.qcom qcom/NOTICE.txt
    40 endif

So, because we have a glob here, the following symlinks will not be
created:

    Link: a300_pfp.fw -> qcom/a300_pfp.fw
    Link: a300_pm4.fw -> qcom/a300_pm4.fw

So, we need to get rid of the globs and only ever use full names.

See another comment below...

> Fixes: 48cc1a89ae04 ("package/linux-firmware: bump to version 20200122")
> Cc: james.hilliard1 at gmail.com
> Signed-off-by: Antoine Tenart <antoine.tenart@bootlin.com>
> ---
> diff --git a/package/linux-firmware/linux-firmware.mk b/package/linux-firmware/linux-firmware.mk
> index f8e95d0648c9..125df74ef68d 100644
> --- a/package/linux-firmware/linux-firmware.mk
> +++ b/package/linux-firmware/linux-firmware.mk
[--SNIP--]
> @@ -626,4 +614,21 @@ define LINUX_FIRMWARE_INSTALL_TARGET_CMDS
>  	$(LINUX_FIRMWARE_INSTALL_DIRS)
>  endef
>  
> +# Some firmware are distributed as a symlink, for drivers to load them using a
> +# defined name other than the real one. Since 9cfefbd7fbda ("Remove duplicate
> +# symlinks") those symlink aren't distributed in linux-firmware but are created
> +# automatically by its copy-firmware.sh script during the installation, which
> +# parses the WHENCE file where symlinks are described. We follow the same logic
> +# here, adding symlink only for firmwares installed in the target directory.
> +# The grep/sed parsing is taken from the script mentioned before.
> +define LINUX_FIRMWARE_CREATE_SYMLINKS
> +	grep -E '^Link:' $(@D)/WHENCE | sed -e's/^Link: *//g' -e's/-> //g' | while read f d; do \

You can combine the grep and sed into a single sed call:

    sed -r -e '/^Link: (.+) -> (.+)$$/!d; s//\1 \2/' $(@D)/WHENCE

> +		if test -f $(TARGET_DIR)/lib/firmware/$$d; then \
> +			ln -sf $$d $(TARGET_DIR)/lib/firmware/$$f ; \
> +		fi ; \
> +	done
> +endef
> +
> +LINUX_FIRMWARE_POST_INSTALL_TARGET_HOOKS += LINUX_FIRMWARE_CREATE_SYMLINKS

Use of a hook is not really useful here: we are using the generic infra,
so we can write whatever we want in the _INSTALL_TARGET_CMDS anyway, so
just define the macro before _INSTALL_TARGET_CMDS and call it from
there, like the other parts (notice the rename of the macro to match):

    621  define LINUX_FIRMWARE_INSTALL_TARGET_CMDS
    622          mkdir -p $(TARGET_DIR)/lib/firmware
    623          $(LINUX_FIRMWARE_INSTALL_FILES)
    624          $(LINUX_FIRMWARE_INSTALL_DIRS)
    625          $(LINUX_FIRMWARE_INSTALL_SYMLINKS)
    626  endef

Regards,
Yann E. MORIN.

>  $(eval $(generic-package))
> -- 
> 2.24.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.  |
'------------------------------^-------^------------------^--------------------'

^ permalink raw reply	[flat|nested] 10+ messages in thread
* [Buildroot] [PATCH] package/linux-firmware: fix symlink support
  2020-03-03 16:43 ` Yann E. MORIN
@ 2020-03-03 18:47   ` Antoine Tenart
  2020-03-03 21:15     ` Yann E. MORIN
  0 siblings, 1 reply; 10+ messages in thread
From: Antoine Tenart @ 2020-03-03 18:47 UTC (permalink / raw)
  To: buildroot

Hi Yann,

On Tue, Mar 03, 2020 at 05:43:02PM +0100, Yann E. MORIN wrote:
> On 2020-03-03 14:33 +0100, Antoine Tenart spake thusly:
> 
> The problem is that we have bunch of LINUX_FIRMWARE_FILES that are
> globs, for example:
> 
>     37 ifeq ($(BR2_PACKAGE_LINUX_FIRMWARE_QCOM_ADRENO),y)
>     38 LINUX_FIRMWARE_FILES += qcom/a*
>     39 LINUX_FIRMWARE_ALL_LICENSE_FILES += LICENSE.qcom qcom/NOTICE.txt
>     40 endif
> 
> So, because we have a glob here, the following symlinks will not be
> created:
> 
>     Link: a300_pfp.fw -> qcom/a300_pfp.fw
>     Link: a300_pm4.fw -> qcom/a300_pm4.fw

I don't get why they wouldn't be created. The symlink creation is done
after the files have been installed to the target directory, and is done
based on what has been installed. So the definition used to get those
firmware installed (being full names or not) shouldn't matter.

With this patch, and BR2_PACKAGE_LINUX_FIRMWARE_QCOM_ADRENO enabled, I
get the following files in $(TARGET_DIR)/lib/firmware:

./output/target/lib/firmware/
??? a300_pfp.fw -> qcom/a300_pfp.fw
??? a300_pm4.fw -> qcom/a300_pm4.fw
??? qcom
    ??? a300_pfp.fw
    ??? a300_pm4.fw
    ??? a530_pfp.fw
    ??? a530_pm4.fw
    ??? a530v3_gpmu.fw2
    ??? a530_zap.b00
    ??? a530_zap.b01
    ??? a530_zap.b02
    ??? a530_zap.mdt
    ??? a630_gmu.bin
    ??? a630_sqe.fw

> > +# Some firmware are distributed as a symlink, for drivers to load them using a
> > +# defined name other than the real one. Since 9cfefbd7fbda ("Remove duplicate
> > +# symlinks") those symlink aren't distributed in linux-firmware but are created
> > +# automatically by its copy-firmware.sh script during the installation, which
> > +# parses the WHENCE file where symlinks are described. We follow the same logic
> > +# here, adding symlink only for firmwares installed in the target directory.
> > +# The grep/sed parsing is taken from the script mentioned before.
> > +define LINUX_FIRMWARE_CREATE_SYMLINKS
> > +	grep -E '^Link:' $(@D)/WHENCE | sed -e's/^Link: *//g' -e's/-> //g' | while read f d; do \
> 
> You can combine the grep and sed into a single sed call:
> 
>     sed -r -e '/^Link: (.+) -> (.+)$$/!d; s//\1 \2/' $(@D)/WHENCE

The reasoning here was to reuse with as little change as possible the
code used in Linux-firmware's copy-firmware.sh script, to be easily
maintainable should they decide to update the link definition format.

I'm not pushing for either solution, I can change this for v2 if you
believe the two command should still be combined.

> > +		if test -f $(TARGET_DIR)/lib/firmware/$$d; then \
> > +			ln -sf $$d $(TARGET_DIR)/lib/firmware/$$f ; \
> > +		fi ; \
> > +	done
> > +endef
> > +
> > +LINUX_FIRMWARE_POST_INSTALL_TARGET_HOOKS += LINUX_FIRMWARE_CREATE_SYMLINKS
> 
> Use of a hook is not really useful here: we are using the generic infra,
> so we can write whatever we want in the _INSTALL_TARGET_CMDS anyway, so
> just define the macro before _INSTALL_TARGET_CMDS and call it from
> there, like the other parts (notice the rename of the macro to match):
> 
>     621  define LINUX_FIRMWARE_INSTALL_TARGET_CMDS
>     622          mkdir -p $(TARGET_DIR)/lib/firmware
>     623          $(LINUX_FIRMWARE_INSTALL_FILES)
>     624          $(LINUX_FIRMWARE_INSTALL_DIRS)
>     625          $(LINUX_FIRMWARE_INSTALL_SYMLINKS)
>     626  endef

OK, will do!

Thanks!
Antoine

-- 
Antoine T?nart, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

^ permalink raw reply	[flat|nested] 10+ messages in thread
* [Buildroot] [PATCH] package/linux-firmware: fix symlink support
  2020-03-03 18:47   ` Antoine Tenart
@ 2020-03-03 21:15     ` Yann E. MORIN
  2020-03-03 21:30       ` Antoine Tenart
  0 siblings, 1 reply; 10+ messages in thread
From: Yann E. MORIN @ 2020-03-03 21:15 UTC (permalink / raw)
  To: buildroot

Antoine, All,

On 2020-03-03 19:47 +0100, Antoine Tenart spake thusly:
> On Tue, Mar 03, 2020 at 05:43:02PM +0100, Yann E. MORIN wrote:
> > On 2020-03-03 14:33 +0100, Antoine Tenart spake thusly:
> > 
> > The problem is that we have bunch of LINUX_FIRMWARE_FILES that are
> > globs, for example:
> > 
> >     37 ifeq ($(BR2_PACKAGE_LINUX_FIRMWARE_QCOM_ADRENO),y)
> >     38 LINUX_FIRMWARE_FILES += qcom/a*
> >     39 LINUX_FIRMWARE_ALL_LICENSE_FILES += LICENSE.qcom qcom/NOTICE.txt
> >     40 endif
> > 
> > So, because we have a glob here, the following symlinks will not be
> > created:
> > 
> >     Link: a300_pfp.fw -> qcom/a300_pfp.fw
> >     Link: a300_pm4.fw -> qcom/a300_pm4.fw
> 
> I don't get why they wouldn't be created. The symlink creation is done
> after the files have been installed to the target directory, and is done
> based on what has been installed. So the definition used to get those
> firmware installed (being full names or not) shouldn't matter.
> 
> With this patch, and BR2_PACKAGE_LINUX_FIRMWARE_QCOM_ADRENO enabled, I
> get the following files in $(TARGET_DIR)/lib/firmware:
> 
> ./output/target/lib/firmware/
> ??? a300_pfp.fw -> qcom/a300_pfp.fw
> ??? a300_pm4.fw -> qcom/a300_pm4.fw
> ??? qcom
>     ??? a300_pfp.fw
>     ??? a300_pm4.fw
>     ??? a530_pfp.fw
>     ??? a530_pm4.fw
>     ??? a530v3_gpmu.fw2
>     ??? a530_zap.b00
>     ??? a530_zap.b01
>     ??? a530_zap.b02
>     ??? a530_zap.mdt
>     ??? a630_gmu.bin
>     ??? a630_sqe.fw

Indeed, and so do I. I just got confused between the 'Link:' tags and
the usual order of arguments to ln: they are reversed, and I looke at
file and links in the wrong location...

> > > +# Some firmware are distributed as a symlink, for drivers to load them using a
> > > +# defined name other than the real one. Since 9cfefbd7fbda ("Remove duplicate
> > > +# symlinks") those symlink aren't distributed in linux-firmware but are created
> > > +# automatically by its copy-firmware.sh script during the installation, which
> > > +# parses the WHENCE file where symlinks are described. We follow the same logic
> > > +# here, adding symlink only for firmwares installed in the target directory.
> > > +# The grep/sed parsing is taken from the script mentioned before.
> > > +define LINUX_FIRMWARE_CREATE_SYMLINKS
> > > +	grep -E '^Link:' $(@D)/WHENCE | sed -e's/^Link: *//g' -e's/-> //g' | while read f d; do \
> > 
> > You can combine the grep and sed into a single sed call:
> > 
> >     sed -r -e '/^Link: (.+) -> (.+)$$/!d; s//\1 \2/' $(@D)/WHENCE
> 
> The reasoning here was to reuse with as little change as possible the
> code used in Linux-firmware's copy-firmware.sh script, to be easily
> maintainable should they decide to update the link definition format.

I see, and that's a good reason. Yet, I've looked at the copy-firmware
script, and I am not impressed... I find the above to be more readable.

If the upstream script was callable with a list of firmware files to
install, then that would wonderfull, as we could re-use it instead of
reimplementing the logic ourselves (hint, hint ;-) ).

> I'm not pushing for either solution, I can change this for v2 if you
> believe the two command should still be combined.

no need for a v2: the major road-block (the globs above) is not. I can
fix the sed and hook stuff locally (I already did it).

Thanks! :-)

Regards,
Yann E. MORIN.

> > > +		if test -f $(TARGET_DIR)/lib/firmware/$$d; then \
> > > +			ln -sf $$d $(TARGET_DIR)/lib/firmware/$$f ; \
> > > +		fi ; \
> > > +	done
> > > +endef
> > > +
> > > +LINUX_FIRMWARE_POST_INSTALL_TARGET_HOOKS += LINUX_FIRMWARE_CREATE_SYMLINKS
> > 
> > Use of a hook is not really useful here: we are using the generic infra,
> > so we can write whatever we want in the _INSTALL_TARGET_CMDS anyway, so
> > just define the macro before _INSTALL_TARGET_CMDS and call it from
> > there, like the other parts (notice the rename of the macro to match):
> > 
> >     621  define LINUX_FIRMWARE_INSTALL_TARGET_CMDS
> >     622          mkdir -p $(TARGET_DIR)/lib/firmware
> >     623          $(LINUX_FIRMWARE_INSTALL_FILES)
> >     624          $(LINUX_FIRMWARE_INSTALL_DIRS)
> >     625          $(LINUX_FIRMWARE_INSTALL_SYMLINKS)
> >     626  endef
> 
> OK, will do!
> 
> Thanks!
> Antoine
> 
> -- 
> Antoine T?nart, Bootlin
> Embedded Linux and Kernel engineering
> https://bootlin.com

-- 
.-----------------.--------------------.------------------.--------------------.
|  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.  |
'------------------------------^-------^------------------^--------------------'

^ permalink raw reply	[flat|nested] 10+ messages in thread
* [Buildroot] [PATCH] package/linux-firmware: fix symlink support
  2020-03-03 13:33 [Buildroot] [PATCH] package/linux-firmware: fix symlink support Antoine Tenart
  2020-03-03 13:41 ` Baruch Siach
  2020-03-03 16:43 ` Yann E. MORIN
@ 2020-03-03 21:28 ` Yann E. MORIN
  2020-03-03 21:32   ` Antoine Tenart
  2 siblings, 1 reply; 10+ messages in thread
From: Yann E. MORIN @ 2020-03-03 21:28 UTC (permalink / raw)
  To: buildroot

Antoine, All,

On 2020-03-03 14:33 +0100, Antoine Tenart spake thusly:
> Since Linux-firmware's commit 9cfefbd7fbda ("Remove duplicate symlinks")
> symlinks aren't distributed anymore. They are rather created at
> installation time by a script provided in the project, copy-firmware.sh.
> The description of the symlinks is done in the WHENCE file. Since the
> bump to version 20200122, in commit 48cc1a89ae04, installation for many
> firmwares was broken as Buildroot tried to install missing symlinks from
> Linux-firmware.
> 
> The fix is not only to remove now missing symlinks, but to add logic to
> create those symlinks as kernel modules will depend on them. The
> solution taken by this patch is to create dynamically symlinks based on
> their description in the WHENCE file *and* only if the file they'll
> point to was installed in the target directory.
> 
> Fixes: 48cc1a89ae04 ("package/linux-firmware: bump to version 20200122")
> Cc: james.hilliard1 at gmail.com
> Signed-off-by: Antoine Tenart <antoine.tenart@bootlin.com>

I did find a problem with some symlinks that is not created.

    Link: cxgb4/t4fw.bin -> t4fw-1.24.11.0.bin

The target is itself in the cxgb4/ directory, so your code does not
catch it as an isntalled blob, and thus does not create the symlink.

I anyway applied, because it at least fixes the build by dropping the
entries tht no longer exist as they are symlinks.

So this patch is just a partial fix.

Care to send an update to also account for the kind of symlinks above?

Thanks!

Regards,
Yann E. MORIN.

> ---
>  package/linux-firmware/linux-firmware.mk | 33 ++++++++++++++----------
>  1 file changed, 19 insertions(+), 14 deletions(-)
> 
> diff --git a/package/linux-firmware/linux-firmware.mk b/package/linux-firmware/linux-firmware.mk
> index f8e95d0648c9..125df74ef68d 100644
> --- a/package/linux-firmware/linux-firmware.mk
> +++ b/package/linux-firmware/linux-firmware.mk
> @@ -71,9 +71,7 @@ endif
>  
>  # rt2xx
>  ifeq ($(BR2_PACKAGE_LINUX_FIRMWARE_RALINK_RT2XX),y)
> -# rt3090.bin is a symlink to rt2860.bin
> -# rt3070.bin is a symlink to rt2870.bin
> -LINUX_FIRMWARE_FILES += rt2860.bin rt2870.bin rt3070.bin rt3071.bin rt3090.bin
> +LINUX_FIRMWARE_FILES += rt2860.bin rt2870.bin rt3071.bin
>  LINUX_FIRMWARE_ALL_LICENSE_FILES += LICENCE.ralink-firmware.txt
>  endif
>  
> @@ -214,8 +212,6 @@ endif
>  
>  # sd8688
>  ifeq ($(BR2_PACKAGE_LINUX_FIRMWARE_LIBERTAS_SD8688),y)
> -LINUX_FIRMWARE_FILES += libertas/sd8688.bin libertas/sd8688_helper.bin
> -# The two files above are but symlinks to those two ones:
>  LINUX_FIRMWARE_FILES += mrvl/sd8688.bin mrvl/sd8688_helper.bin
>  LINUX_FIRMWARE_ALL_LICENSE_FILES += LICENCE.Marvell
>  endif
> @@ -320,12 +316,10 @@ endif
>  
>  # wl127x
>  ifeq ($(BR2_PACKAGE_LINUX_FIRMWARE_TI_WL127X),y)
> -# wl1271-nvs.bin is a symlink to wl127x-nvs.bin
>  LINUX_FIRMWARE_FILES += \
>  	ti-connectivity/wl1271-fw-2.bin \
>  	ti-connectivity/wl1271-fw-ap.bin \
>  	ti-connectivity/wl1271-fw.bin \
> -	ti-connectivity/wl1271-nvs.bin \
>  	ti-connectivity/wl127x-fw-3.bin \
>  	ti-connectivity/wl127x-fw-plt-3.bin \
>  	ti-connectivity/wl127x-nvs.bin \
> @@ -341,15 +335,12 @@ endif
>  
>  # wl128x
>  ifeq ($(BR2_PACKAGE_LINUX_FIRMWARE_TI_WL128X),y)
> -# wl1271-nvs.bin and wl12xx-nvs.bin are symlinks to wl127x-nvs.bin
>  LINUX_FIRMWARE_FILES += \
>  	ti-connectivity/wl128x-fw-3.bin \
>  	ti-connectivity/wl128x-fw-ap.bin \
>  	ti-connectivity/wl128x-fw-plt-3.bin \
>  	ti-connectivity/wl128x-fw.bin \
> -	ti-connectivity/wl1271-nvs.bin \
>  	ti-connectivity/wl128x-nvs.bin \
> -	ti-connectivity/wl12xx-nvs.bin \
>  	ti-connectivity/wl127x-nvs.bin \
>  	ti-connectivity/wl128x-fw-4-mr.bin \
>  	ti-connectivity/wl128x-fw-4-plt.bin \
> @@ -363,13 +354,11 @@ endif
>  
>  # wl18xx
>  ifeq ($(BR2_PACKAGE_LINUX_FIRMWARE_TI_WL18XX),y)
> -# wl1271-nvs.bin is a symlink to wl127x-nvs.bin
>  LINUX_FIRMWARE_FILES += \
>  	ti-connectivity/wl18xx-fw.bin \
>  	ti-connectivity/wl18xx-fw-2.bin \
>  	ti-connectivity/wl18xx-fw-3.bin \
>  	ti-connectivity/wl18xx-fw-4.bin \
> -	ti-connectivity/wl1271-nvs.bin \
>  	ti-connectivity/wl127x-nvs.bin \
>  	ti-connectivity/TIInit_7.2.31.bts
>  LINUX_FIRMWARE_ALL_LICENSE_FILES += LICENCE.ti-connectivity
> @@ -568,8 +557,7 @@ LINUX_FIRMWARE_FILES += \
>  endif
>  
>  ifeq ($(BR2_PACKAGE_LINUX_FIRMWARE_QAT_DH895XCC),y)
> -# qat_mmp.bin is a symlink to qat_895xcc_mmp.bin
> -LINUX_FIRMWARE_FILES += qat_895xcc.bin qat_895xcc_mmp.bin qat_mmp.bin
> +LINUX_FIRMWARE_FILES += qat_895xcc.bin qat_895xcc_mmp.bin
>  LINUX_FIRMWARE_ALL_LICENSE_FILES += LICENCE.qat_firmware
>  endif
>  
> @@ -626,4 +614,21 @@ define LINUX_FIRMWARE_INSTALL_TARGET_CMDS
>  	$(LINUX_FIRMWARE_INSTALL_DIRS)
>  endef
>  
> +# Some firmware are distributed as a symlink, for drivers to load them using a
> +# defined name other than the real one. Since 9cfefbd7fbda ("Remove duplicate
> +# symlinks") those symlink aren't distributed in linux-firmware but are created
> +# automatically by its copy-firmware.sh script during the installation, which
> +# parses the WHENCE file where symlinks are described. We follow the same logic
> +# here, adding symlink only for firmwares installed in the target directory.
> +# The grep/sed parsing is taken from the script mentioned before.
> +define LINUX_FIRMWARE_CREATE_SYMLINKS
> +	grep -E '^Link:' $(@D)/WHENCE | sed -e's/^Link: *//g' -e's/-> //g' | while read f d; do \
> +		if test -f $(TARGET_DIR)/lib/firmware/$$d; then \
> +			ln -sf $$d $(TARGET_DIR)/lib/firmware/$$f ; \
> +		fi ; \
> +	done
> +endef
> +
> +LINUX_FIRMWARE_POST_INSTALL_TARGET_HOOKS += LINUX_FIRMWARE_CREATE_SYMLINKS
> +
>  $(eval $(generic-package))
> -- 
> 2.24.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.  |
'------------------------------^-------^------------------^--------------------'

^ permalink raw reply	[flat|nested] 10+ messages in thread
* [Buildroot] [PATCH] package/linux-firmware: fix symlink support
  2020-03-03 21:15     ` Yann E. MORIN
@ 2020-03-03 21:30       ` Antoine Tenart
  0 siblings, 0 replies; 10+ messages in thread
From: Antoine Tenart @ 2020-03-03 21:30 UTC (permalink / raw)
  To: buildroot

Yann,

On Tue, Mar 03, 2020 at 10:15:52PM +0100, Yann E. MORIN wrote:
> On 2020-03-03 19:47 +0100, Antoine Tenart spake thusly:
> > On Tue, Mar 03, 2020 at 05:43:02PM +0100, Yann E. MORIN wrote:
> > > On 2020-03-03 14:33 +0100, Antoine Tenart spake thusly:
> > > > +# Some firmware are distributed as a symlink, for drivers to load them using a
> > > > +# defined name other than the real one. Since 9cfefbd7fbda ("Remove duplicate
> > > > +# symlinks") those symlink aren't distributed in linux-firmware but are created
> > > > +# automatically by its copy-firmware.sh script during the installation, which
> > > > +# parses the WHENCE file where symlinks are described. We follow the same logic
> > > > +# here, adding symlink only for firmwares installed in the target directory.
> > > > +# The grep/sed parsing is taken from the script mentioned before.
> > > > +define LINUX_FIRMWARE_CREATE_SYMLINKS
> > > > +	grep -E '^Link:' $(@D)/WHENCE | sed -e's/^Link: *//g' -e's/-> //g' | while read f d; do \
> > > 
> > > You can combine the grep and sed into a single sed call:
> > > 
> > >     sed -r -e '/^Link: (.+) -> (.+)$$/!d; s//\1 \2/' $(@D)/WHENCE
> > 
> > The reasoning here was to reuse with as little change as possible the
> > code used in Linux-firmware's copy-firmware.sh script, to be easily
> > maintainable should they decide to update the link definition format.
> 
> I see, and that's a good reason. Yet, I've looked at the copy-firmware
> script, and I am not impressed... I find the above to be more readable.

That is the least we can say, not impressed :-)

> > I'm not pushing for either solution, I can change this for v2 if you
> > believe the two command should still be combined.
> 
> no need for a v2: the major road-block (the globs above) is not. I can
> fix the sed and hook stuff locally (I already did it).

Thanks!

Antoine

-- 
Antoine T?nart, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

^ permalink raw reply	[flat|nested] 10+ messages in thread
* [Buildroot] [PATCH] package/linux-firmware: fix symlink support
  2020-03-03 21:28 ` Yann E. MORIN
@ 2020-03-03 21:32   ` Antoine Tenart
  2020-03-03 21:44     ` Antoine Tenart
  0 siblings, 1 reply; 10+ messages in thread
From: Antoine Tenart @ 2020-03-03 21:32 UTC (permalink / raw)
  To: buildroot

Yann,

On Tue, Mar 03, 2020 at 10:28:53PM +0100, Yann E. MORIN wrote:
> On 2020-03-03 14:33 +0100, Antoine Tenart spake thusly:
> > Since Linux-firmware's commit 9cfefbd7fbda ("Remove duplicate symlinks")
> > symlinks aren't distributed anymore. They are rather created at
> > installation time by a script provided in the project, copy-firmware.sh.
> > The description of the symlinks is done in the WHENCE file. Since the
> > bump to version 20200122, in commit 48cc1a89ae04, installation for many
> > firmwares was broken as Buildroot tried to install missing symlinks from
> > Linux-firmware.
> > 
> > The fix is not only to remove now missing symlinks, but to add logic to
> > create those symlinks as kernel modules will depend on them. The
> > solution taken by this patch is to create dynamically symlinks based on
> > their description in the WHENCE file *and* only if the file they'll
> > point to was installed in the target directory.
> > 
> > Fixes: 48cc1a89ae04 ("package/linux-firmware: bump to version 20200122")
> > Cc: james.hilliard1 at gmail.com
> > Signed-off-by: Antoine Tenart <antoine.tenart@bootlin.com>
> 
> I did find a problem with some symlinks that is not created.
> 
>     Link: cxgb4/t4fw.bin -> t4fw-1.24.11.0.bin
> 
> The target is itself in the cxgb4/ directory, so your code does not
> catch it as an isntalled blob, and thus does not create the symlink.
> 
> I anyway applied, because it at least fixes the build by dropping the
> entries tht no longer exist as they are symlinks.
> 
> So this patch is just a partial fix.
> 
> Care to send an update to also account for the kind of symlinks above?

Good catch! I'll have a look at this.

Thanks,
Antoine

-- 
Antoine T?nart, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

^ permalink raw reply	[flat|nested] 10+ messages in thread
* [Buildroot] [PATCH] package/linux-firmware: fix symlink support
  2020-03-03 21:32   ` Antoine Tenart
@ 2020-03-03 21:44     ` Antoine Tenart
  0 siblings, 0 replies; 10+ messages in thread
From: Antoine Tenart @ 2020-03-03 21:44 UTC (permalink / raw)
  To: buildroot

On Tue, Mar 03, 2020 at 10:32:03PM +0100, Antoine Tenart wrote:
> On Tue, Mar 03, 2020 at 10:28:53PM +0100, Yann E. MORIN wrote:
> > On 2020-03-03 14:33 +0100, Antoine Tenart spake thusly:
> > > Since Linux-firmware's commit 9cfefbd7fbda ("Remove duplicate symlinks")
> > > symlinks aren't distributed anymore. They are rather created at
> > > installation time by a script provided in the project, copy-firmware.sh.
> > > The description of the symlinks is done in the WHENCE file. Since the
> > > bump to version 20200122, in commit 48cc1a89ae04, installation for many
> > > firmwares was broken as Buildroot tried to install missing symlinks from
> > > Linux-firmware.
> > > 
> > > The fix is not only to remove now missing symlinks, but to add logic to
> > > create those symlinks as kernel modules will depend on them. The
> > > solution taken by this patch is to create dynamically symlinks based on
> > > their description in the WHENCE file *and* only if the file they'll
> > > point to was installed in the target directory.
> > > 
> > > Fixes: 48cc1a89ae04 ("package/linux-firmware: bump to version 20200122")
> > > Cc: james.hilliard1 at gmail.com
> > > Signed-off-by: Antoine Tenart <antoine.tenart@bootlin.com>
> > 
> > I did find a problem with some symlinks that is not created.
> > 
> >     Link: cxgb4/t4fw.bin -> t4fw-1.24.11.0.bin
> > 
> > The target is itself in the cxgb4/ directory, so your code does not
> > catch it as an isntalled blob, and thus does not create the symlink.
> > 
> > I anyway applied, because it at least fixes the build by dropping the
> > entries tht no longer exist as they are symlinks.
> > 
> > So this patch is just a partial fix.
> > 
> > Care to send an update to also account for the kind of symlinks above?
> 
> Good catch! I'll have a look at this.

It seems to be fixed by:

  - if test -f $(TARGET_DIR)/lib/firmware/$$d; then \
  + if test -f $(TARGET_DIR)/lib/firmware/$$(dirname $$f)/$$d; then \

I'll make some tests tomorrow.

Antoine

-- 
Antoine T?nart, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

^ permalink raw reply	[flat|nested] 10+ messages in thread
end of thread, other threads:[~2020-03-03 21:44 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-03-03 13:33 [Buildroot] [PATCH] package/linux-firmware: fix symlink support Antoine Tenart
2020-03-03 13:41 ` Baruch Siach
2020-03-03 15:10   ` Yann E. MORIN
2020-03-03 16:43 ` Yann E. MORIN
2020-03-03 18:47   ` Antoine Tenart
2020-03-03 21:15     ` Yann E. MORIN
2020-03-03 21:30       ` Antoine Tenart
2020-03-03 21:28 ` Yann E. MORIN
2020-03-03 21:32   ` Antoine Tenart
2020-03-03 21:44     ` Antoine Tenart

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.