Re: [PATCH] iwlwifi: cfg: Add missing MODULE_FIRMWARE() for *.pnvm

[Takashi Iwai]

On Tue, 11 Apr 2023 13:10:34 +0200,
Johannes Berg wrote:
> 
> [removing Luca, he no longer works on wifi]
> 
> Hi,
> 
> On Wed, 2023-04-05 at 08:35 +0200, Takashi Iwai wrote:
> > A few models require *.pnvm files while we don't declare them via
> > MODULE_FIRMWARE().  This resulted in the breakage of WiFi on the
> > system that relies on the information from modinfo (e.g. openSUSE
> > installer image).
> > 
> > This patch adds those missing MODULE_FIRMWARE() entries for *.pnvm
> > files.
> 
> Makes sense. They (may) also exist the previous generation of devices,
> but weren't strictly required then.
> 
> > The fix is obviously ad hoc.
> 
> Yeah. Maybe we'll merge it anyway though? Do you think this should still
> go to 6.3? Pretty close I guess.

It's a long-standing issue, so no urgent fix is needed.
It can be postponed to 6.4 merge.

> > Here I added the lines with the explicit string since *_PRE definition
> > contains the tailing dash and can't be used for *.pnvm file.
> 
> Yeah, we thought about changing that - but I have a larger set of rework
> in this area just done a short while ago, which would make it a bit hard
> to do. Hence maybe we should merge this for 6.3/6.4 and do the larger
> rework plus getting rid of the dash in the *_PRE definitions in 6.5,
> what do you think?

Agreed.

> > Alternatively, we may put a single line
> >  MODULE_FIRMWARE("iwlwifi-*.pnvm");
> > to catch all, too.
> > 
> 
> Unrelated discussion, but ... I didn't even know that was possible.
> 
> Maybe this gives us a way out of something else I was thinking about
> recently - the MODULE_FIRMWARE() here in iwlwifi usually only states the
> latest version that the driver accepts, however:
> 
>  * the driver might be ahead of the firmware releases - in fact that's
>    how it usually should be, just due to various issues we haven't been
>    upstreaming as quickly as we'd like
>  * sometimes we (have to) skip firmware releases due to other issues
>  * etc.
> 
> So it could be that 6.4 kernel will state e.g. the max version is 78,
> but we end up never even releasing 78 firmware. The MODULE_FIRMWARE()
> would then state 78, but that file would never exist.
> 
> Have we just been very lucky with never running into any of these
> issues, and the distro kernels being "old enough" that usually all the
> max version firmware was already released by the time it was used? Or
> did you work around this in some other way?

Heh, we had occasionally a "hot fix" patch for avoiding to point to
the non-existing versions for distro kernels.

> Anyway, if we can use wildcards, maybe instead of stating the max API
> version number of the filename, we should have a wildcard there for the
> number? OTOH, iwlwifi *already* comes with a *lot* of firmware files for
> all the various families of devices and radios, and making the API
> version a wildcard would make it much bigger again, to the point where
> we might as well state something like
> 
>   MODULE_FIRMWARE("iwlwifi-*")
> 
> which is a lot of files ...

Right, this may end up with too many files.
Although there were recent actions in linux-firmware tree to drop the
unused versions, there are still quite many in total, and iwlwifi
firmware files are relatively large, unfortunately.

> Did you see any issues with this versioning thing in the past? And what
> would you think (from a distro POV) about making this a wildcard on the
> version, i.e. having, in things like
> 
> #define IWL_QU_B_HR_B_MODULE_FIRMWARE(api) \
>         IWL_QU_B_HR_B_FW_PRE __stringify(api) ".ucode"
> 
> 
> "*" instead of __stringify(api).
> 
> 
> Some input on this would be nice.
> 
> Thanks,
> johannes

As of now, using the wildcard for matching all iwlwifi firmware files
would be worse for us, as it'll lead to drag all those files into the
openSUSE/SUSE installer image and grow the image size significantly.
From that POV, the current MODULE_FIRMWARE() has been working "good
enough"; as already mentioned, we occasionally fix in the downstream
side to point to the existing firmware version, but that's OK-ish.


thanks,

Takashi