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

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

[Takashi Iwai]

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.

Link: https://bugzilla.opensuse.org/show_bug.cgi?id=1207553
Signed-off-by: Takashi Iwai <tiwai@suse.de>
---

The fix is obviously ad hoc.

Here I added the lines with the explicit string since *_PRE definition
contains the tailing dash and can't be used for *.pnvm file.

Alternatively, we may put a single line
 MODULE_FIRMWARE("iwlwifi-*.pnvm");
to catch all, too.

 drivers/net/wireless/intel/iwlwifi/cfg/22000.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/net/wireless/intel/iwlwifi/cfg/22000.c b/drivers/net/wireless/intel/iwlwifi/cfg/22000.c
index 3bdd6774716d..3c6dc3601784 100644
--- a/drivers/net/wireless/intel/iwlwifi/cfg/22000.c
+++ b/drivers/net/wireless/intel/iwlwifi/cfg/22000.c
@@ -1065,3 +1065,7 @@ MODULE_FIRMWARE(IWL_BNJ_A_HR_B_MODULE_FIRMWARE(IWL_22000_UCODE_API_MAX));
 MODULE_FIRMWARE(IWL_BZ_A_FM4_A_MODULE_FIRMWARE(IWL_22000_UCODE_API_MAX));
 MODULE_FIRMWARE(IWL_GL_B_FM_B_MODULE_FIRMWARE(IWL_22000_UCODE_API_MAX));
 MODULE_FIRMWARE(IWL_BNJ_B_FM_B_MODULE_FIRMWARE(IWL_22000_UCODE_API_MAX));
+
+MODULE_FIRMWARE("iwlwifi-so-a0-gf4-a0.pnvm");
+MODULE_FIRMWARE("iwlwifi-so-a0-gf-a0.pnvm");
+MODULE_FIRMWARE("iwlwifi-ty-a0-gf-a0.pnvm");
-- 
2.35.3

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

[Johannes Berg]

[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.

> 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?

> 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?

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

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

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