Re: [Buildroot] [RFC PATCH] package/linux-firmware: Add more Intel WiFi 22000 series

[Arnout Vandecappelle <arnout@mind.be>]

On 20/08/2022 16:21, Yann E. MORIN wrote:
> Stefan, All,
> 
> On 2022-08-20 15:40 +0200, Stefan Agner spake thusly:
>> On 2022-08-20 14:13, Yann E. MORIN wrote:
>>> And then, we'd have to code some non-trivial magic that iterates over
>>> all iwlwifi-Qu{,Z}-*.ucode and check if their API part is in the range,
>>> and for a single "family" of firmwares, keep the highest one (how do we
>>> know that two firmware files are f the same family? Just because they
>>> only differ in API version?) That's a bit brittle...
>> To avoid the direct dependency I was thinking of using the
>> BR2_TOOLCHAIN_HEADERS_AT_LEAST Kconfigs, essentially maintain a list of
>> default max supported API per kernel version in Kconfig. Not sure if
>> that is a good idea.
> 
> Indeed, not really, because the version of the running kernel may be
> different than the version of the kernel headers used to buid the
> toolchain.
> 
> For example, you could build a toolchain with kernel headers 3.0, and
> run a 5.19 kernel. In such a case, the highest API derived from the
> kernel headers version may very well be lower than the lowest API
> supported by the running kernel.
> 
>>>> +LINUX_FIRMWARE_IWLWIFI_22000_UCODE_API_GLOB = $(call qstrip,$(BR2_PACKAGE_LINUX_FIRMWARE_IWLWIFI_22000_UCODE_API_GLOB))
>>>> +LINUX_FIRMWARE_FILES += \
> [--SNIP--]
>>> This list is not entirely alphabetically sorted.
>> This is on purpose, I've used the order in
>> drivers/net/wireless/intel/iwlwifi/cfg/22000.c.
> 
> Then, add a comment above, like:
>      # Keep this list in the same order as in kernel's driver
> 
>>> Also, why do you extend the prefixes, from iwlwifi-QuZ- and iwlwifi-Qu-,
>>> to include extra c0, b0, a0 and so on? Why can we just have:
>>>      iwlwifi-Qu-*-$(LINUX_FIRMWARE_IWLWIFI_22000_UCODE_API_GLOB).ucode
>>>      iwlwifi-QuZ-*-$(LINUX_FIRMWARE_IWLWIFI_22000_UCODE_API_GLOB).ucode

  Note that this could be simplified to just
iwlwifi-Q*-$(LINUX_FIRMWARE_IWLWIFI_22000_UCODE_API_GLOB).ucode

>> Same reason: Align with kernel sources.
> 
> But then, users would had a need for one of the firmwares now excluded
> from the list will have no way to enable them.

  To allow the user full freedom there, the u, uZ and a0 etc. could be made part 
of the glob, i.e.
iwlwifi-Q$(LINUX_FIRMWARE_IWLWIFI_22000_UCODE_API_GLOB).ucode


>>> Oh, and in at least linu 5.17, there are also references to
>>> iwlwifi-QuQnj-, iwlwifi-SoSnj- and a bunch of others. And
>>> specifically, there is also iwlwifi-cc-a0- which in Buildroot is
>>> installed with BR2_PACKAGE_LINUX_FIRMWARE_IWLWIFI_22260 and not
>>> BR2_PACKAGE_LINUX_FIRMWARE_IWLWIFI_22000
>> There are more firmwares in the kernel sources than present in the
>> linux-firmware git repository. This is essentially the common
>> denominator of Linux 5.15 and the current version of linux-firmware. It
> 
> But why limit ourselves to what is know to 5.15? If someone uses 5.19,
> they might need other firmware files? And even if 5.15 and 5.17 have
> exactly the same set, then the upcoming 6.0 might need more or a newer
> familly (the QUQnj or QUSnj for example).

  Also, when we update linux-firmware, we're not going to look at all the new 
blobs that are in there. So we want to be as future-safe as possible with the 
globs we use.


>> might deploy too many firmwares for an older kernel, but it is
>> guaranteed to work (list a non existing firmware causes the tar command
>> in the build step to complain).
> 
> Well, it is easy to solve:
> 
>      # Fiter-out firmware famillies that do not match the API version glob
>      # so that 'tar' does not whine later on.
>      LINUX_FIRMWARE_FILES += \
>          $(notdir $(wildcard $(patsubst %,$(@D)/%, \

  Maybe we should just add "shopt nullglob" to the command that creates the tarball?

>              iwlwifi-Qu-*-$(LINUX_FIRMWARE_IWLWIFI_22000_UCODE_API_GLOB).ucode \

  It would make sense for the globs to be allowed to be a list of globs instead 
of a single one - but that would be a bit complicated to implement together with 
the wildcard.

>              iwlwifi-QuZ-*-$(LINUX_FIRMWARE_IWLWIFI_22000_UCODE_API_GLOB).ucode \
>              iwlwifi-QuQnj-*-$(LINUX_FIRMWARE_IWLWIFI_22000_UCODE_API_GLOB).ucode \
>              iwlwifi-QuSnj-*-$(LINUX_FIRMWARE_IWLWIFI_22000_UCODE_API_GLOB).ucode \
>          )))
> 
> But adding new firmware "categories" (QuQnj or QuSnj et al.) should be a
> seaprate patch, of course.
> 
>>> So, maybe we could split the families further as an alternate solution?
>> I'd prefer to group them as they are grouped in the kernel sources.
> 
> I am slightly conflicted on this one. I would prefer they be grouped by
> whatever the user can use to identify the chip: by actually looking up
> the reference on the chip, by looking up lspci/lsusb/lshw/... But I can
> also see the appeal of matching the kernel driver... But if we were to
> change the categorisation, that would be in a separate patch.

  Well, you actually want to have just the firmware that matches both the chip 
and the driver... Our current options for linux-firmware are a bit in the middle.


> But maybe, going for boolean entries was not a good idea to begin with,
> and just a single glob option for all of the iwlwifi family as a whole
> would be better. Like:
> 
>      config BR2_PKG_LNX_FW_IWLWIFI
>          bool "iwlwifi familly/ies"
> 
>      config BR2_PKG_LNX_FW_IWLWIFI_GLOBS
>          string "iwlwifi familly globs"
>          default "*"
>          depends on BR2_PKG_LNX_FW_IWLWIFI
>          help
>            List of shell globs to match firmware files.
> 
>            For example:
>              - BR2_PKG_LNX_FW_IWLWIFI_GLOBS="*"
>                would match all of:
>                  iwlwifi-*.ucode
> 
>              - BR2_PKG_LNX_FW_IWLWIFI_GLOBS="Qu-* QuZ-*"
>                would match all of:
>                  iwlwifi-Qu-*.ucode
>                  iwlwifi-QuZ-*.ucode
> 
> This is way easier to do and, with the $(wildcard) clause I suggested
> above), should cover all of the iwlwifi cases, and we can coalesce the
> 22000, 22260, 3160, 3168, 3945, 4965, 5000, 6000G2A, 6000G2B, 7260,
> 7265, 7265D, 8000C, 8265, and 9XXX, into a single option with a glob.
> But that is not very user-friendly...

  I think the way it is split now is OK. Most of the other chips have just a few 
versions and are much less bad than 22000.


> Thomas, Arnout, Peter: thoughts?

  I think the current patch, if simplified as proposed by Yann, strikes a good 
middle ground.

  One way how we could improve the situation globally is to add an open-ended 
string option that allows the user to specify a list of files or directories or 
globs to include. That way, if you don't want the redundant API versions, you 
can simply only set that option and not the named ones, with exactly the 
firmware files (or globs) that you need.

  Regards,
  Arnout

> 
> Regards,
> Yann E. MORIN.
> 
_______________________________________________
buildroot mailing list
buildroot@buildroot.org
https://lists.buildroot.org/mailman/listinfo/buildroot