| Message ID | 20230405063546.12439-1-tiwai@suse.de (mailing list archive) |
|---|---|
| State | Accepted |
| Delegated to: | Miri Korenblit |
| Headers | show |
| Series | iwlwifi: cfg: Add missing MODULE_FIRMWARE() for *.pnvm | expand |
[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
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
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");
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(+)