* [PATCH] brcmfmac: expose firmware config files through modinfo @ 2020-07-01 15:31 matthias.bgg 2020-07-01 15:38 ` Hans de Goede 2020-07-14 9:37 ` Kalle Valo 0 siblings, 2 replies; 7+ messages in thread From: matthias.bgg @ 2020-07-01 15:31 UTC (permalink / raw) To: Jakub Kicinski, Kalle Valo, David S . Miller, hdegoede Cc: Pali Rohár, Guenter Roeck, Chi-Hsien Lin, Franky Lin, Chung-Hsien Hsu, Jean-Philippe Brucker, Double Lo, Frank Kao, linux-wireless, brcm80211-dev-list.pdl, Arend van Spriel, Gustavo A . R . Silva, netdev, Rafał Miłecki, Hante Meuleman, Wright Feng, Matthias Brugger, Saravanan Shanmugham, brcm80211-dev-list, linux-kernel, Ulf Hansson, Soeren Moch From: Matthias Brugger <mbrugger@suse.com> Apart from a firmware binary the chip needs a config file used by the FW. Add the config files to modinfo so that they can be read by userspace. Signed-off-by: Matthias Brugger <mbrugger@suse.com> --- .../wireless/broadcom/brcm80211/brcmfmac/sdio.c | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c index 310d8075f5d7..ba18df6d8d94 100644 --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c @@ -624,6 +624,22 @@ BRCMF_FW_DEF(4359, "brcmfmac4359-sdio"); BRCMF_FW_DEF(4373, "brcmfmac4373-sdio"); BRCMF_FW_DEF(43012, "brcmfmac43012-sdio"); +/* firmware config files */ +MODULE_FIRMWARE(BRCMF_FW_DEFAULT_PATH "brcm/brcmfmac4330-sdio.Prowise-PT301.txt"); +MODULE_FIRMWARE(BRCMF_FW_DEFAULT_PATH "brcm/brcmfmac43340-sdio.meegopad-t08.txt"); +MODULE_FIRMWARE(BRCMF_FW_DEFAULT_PATH "brcm/brcmfmac43340-sdio.pov-tab-p1006w-data.txt"); +MODULE_FIRMWARE(BRCMF_FW_DEFAULT_PATH "brcm/brcmfmac43362-sdio.cubietech,cubietruck.txt"); +MODULE_FIRMWARE(BRCMF_FW_DEFAULT_PATH "brcm/brcmfmac43430a0-sdio.jumper-ezpad-mini3.txt"); +MODULE_FIRMWARE(BRCMF_FW_DEFAULT_PATH "brcm/brcmfmac43430a0-sdio.ONDA-V80 PLUS.txt"); +MODULE_FIRMWARE(BRCMF_FW_DEFAULT_PATH "brcm/brcmfmac43430-sdio.AP6212.txt"); +MODULE_FIRMWARE(BRCMF_FW_DEFAULT_PATH "brcm/brcmfmac43430-sdio.Hampoo-D2D3_Vi8A1.txt"); +MODULE_FIRMWARE(BRCMF_FW_DEFAULT_PATH "brcm/brcmfmac43430-sdio.MUR1DX.txt"); +MODULE_FIRMWARE(BRCMF_FW_DEFAULT_PATH "brcm/brcmfmac43430-sdio.raspberrypi,3-model-b.txt"); +MODULE_FIRMWARE(BRCMF_FW_DEFAULT_PATH "brcm/brcmfmac43455-sdio.MINIX-NEO Z83-4.txt"); +MODULE_FIRMWARE(BRCMF_FW_DEFAULT_PATH "brcm/brcmfmac43455-sdio.raspberrypi,3-model-b-plus.txt"); +MODULE_FIRMWARE(BRCMF_FW_DEFAULT_PATH "brcm/brcmfmac43455-sdio.raspberrypi,4-model-b.txt"); +MODULE_FIRMWARE(BRCMF_FW_DEFAULT_PATH "brcm/brcmfmac4356-pcie.gpd-win-pocket.txt"); + static const struct brcmf_firmware_mapping brcmf_sdio_fwnames[] = { BRCMF_FW_ENTRY(BRCM_CC_43143_CHIP_ID, 0xFFFFFFFF, 43143), BRCMF_FW_ENTRY(BRCM_CC_43241_CHIP_ID, 0x0000001F, 43241B0), -- 2.27.0 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] brcmfmac: expose firmware config files through modinfo 2020-07-01 15:31 [PATCH] brcmfmac: expose firmware config files through modinfo matthias.bgg @ 2020-07-01 15:38 ` Hans de Goede 2020-07-01 15:46 ` Matthias Brugger 2020-07-14 9:37 ` Kalle Valo 1 sibling, 1 reply; 7+ messages in thread From: Hans de Goede @ 2020-07-01 15:38 UTC (permalink / raw) To: matthias.bgg, Jakub Kicinski, Kalle Valo, David S . Miller Cc: Pali Rohár, Guenter Roeck, Chi-Hsien Lin, Franky Lin, Chung-Hsien Hsu, Jean-Philippe Brucker, Double Lo, Frank Kao, linux-wireless, brcm80211-dev-list.pdl, Arend van Spriel, Gustavo A . R . Silva, netdev, Rafał Miłecki, Hante Meuleman, Wright Feng, Matthias Brugger, Saravanan Shanmugham, brcm80211-dev-list, linux-kernel, Ulf Hansson, Soeren Moch Hi, On 7/1/20 5:31 PM, matthias.bgg@kernel.org wrote: > From: Matthias Brugger <mbrugger@suse.com> > > Apart from a firmware binary the chip needs a config file used by the > FW. Add the config files to modinfo so that they can be read by > userspace. The configfile firmware filename is dynamically generated, just adding the list of all currently shipped ones is not really helpful and this is going to get out of sync with what we actually have in linux-firmware. I must honestly say that I'm not a fan of this, I guess you are trying to get some tool which builds a minimal image, such as an initrd generator to add these files to the image ? I do not immediately have a better idea, but IMHO the solution this patch proposes is not a good one, so nack from me for this change. Regards, Hans > > Signed-off-by: Matthias Brugger <mbrugger@suse.com> > > --- > > .../wireless/broadcom/brcm80211/brcmfmac/sdio.c | 16 ++++++++++++++++ > 1 file changed, 16 insertions(+) > > diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c > index 310d8075f5d7..ba18df6d8d94 100644 > --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c > +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c > @@ -624,6 +624,22 @@ BRCMF_FW_DEF(4359, "brcmfmac4359-sdio"); > BRCMF_FW_DEF(4373, "brcmfmac4373-sdio"); > BRCMF_FW_DEF(43012, "brcmfmac43012-sdio"); > > +/* firmware config files */ > +MODULE_FIRMWARE(BRCMF_FW_DEFAULT_PATH "brcm/brcmfmac4330-sdio.Prowise-PT301.txt"); > +MODULE_FIRMWARE(BRCMF_FW_DEFAULT_PATH "brcm/brcmfmac43340-sdio.meegopad-t08.txt"); > +MODULE_FIRMWARE(BRCMF_FW_DEFAULT_PATH "brcm/brcmfmac43340-sdio.pov-tab-p1006w-data.txt"); > +MODULE_FIRMWARE(BRCMF_FW_DEFAULT_PATH "brcm/brcmfmac43362-sdio.cubietech,cubietruck.txt"); > +MODULE_FIRMWARE(BRCMF_FW_DEFAULT_PATH "brcm/brcmfmac43430a0-sdio.jumper-ezpad-mini3.txt"); > +MODULE_FIRMWARE(BRCMF_FW_DEFAULT_PATH "brcm/brcmfmac43430a0-sdio.ONDA-V80 PLUS.txt"); > +MODULE_FIRMWARE(BRCMF_FW_DEFAULT_PATH "brcm/brcmfmac43430-sdio.AP6212.txt"); > +MODULE_FIRMWARE(BRCMF_FW_DEFAULT_PATH "brcm/brcmfmac43430-sdio.Hampoo-D2D3_Vi8A1.txt"); > +MODULE_FIRMWARE(BRCMF_FW_DEFAULT_PATH "brcm/brcmfmac43430-sdio.MUR1DX.txt"); > +MODULE_FIRMWARE(BRCMF_FW_DEFAULT_PATH "brcm/brcmfmac43430-sdio.raspberrypi,3-model-b.txt"); > +MODULE_FIRMWARE(BRCMF_FW_DEFAULT_PATH "brcm/brcmfmac43455-sdio.MINIX-NEO Z83-4.txt"); > +MODULE_FIRMWARE(BRCMF_FW_DEFAULT_PATH "brcm/brcmfmac43455-sdio.raspberrypi,3-model-b-plus.txt"); > +MODULE_FIRMWARE(BRCMF_FW_DEFAULT_PATH "brcm/brcmfmac43455-sdio.raspberrypi,4-model-b.txt"); > +MODULE_FIRMWARE(BRCMF_FW_DEFAULT_PATH "brcm/brcmfmac4356-pcie.gpd-win-pocket.txt"); > + > static const struct brcmf_firmware_mapping brcmf_sdio_fwnames[] = { > BRCMF_FW_ENTRY(BRCM_CC_43143_CHIP_ID, 0xFFFFFFFF, 43143), > BRCMF_FW_ENTRY(BRCM_CC_43241_CHIP_ID, 0x0000001F, 43241B0), > ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] brcmfmac: expose firmware config files through modinfo 2020-07-01 15:38 ` Hans de Goede @ 2020-07-01 15:46 ` Matthias Brugger 2020-07-02 18:00 ` Hans de Goede 0 siblings, 1 reply; 7+ messages in thread From: Matthias Brugger @ 2020-07-01 15:46 UTC (permalink / raw) To: Hans de Goede, matthias.bgg, Jakub Kicinski, Kalle Valo, David S . Miller Cc: Pali Rohár, Guenter Roeck, Chi-Hsien Lin, Franky Lin, Chung-Hsien Hsu, Jean-Philippe Brucker, Double Lo, Frank Kao, linux-wireless, brcm80211-dev-list.pdl, Arend van Spriel, Gustavo A . R . Silva, netdev, Rafał Miłecki, Hante Meuleman, Wright Feng, Saravanan Shanmugham, brcm80211-dev-list, linux-kernel, Ulf Hansson, Soeren Moch Hi Hans, On 01/07/2020 17:38, Hans de Goede wrote: > Hi, > > On 7/1/20 5:31 PM, matthias.bgg@kernel.org wrote: >> From: Matthias Brugger <mbrugger@suse.com> >> >> Apart from a firmware binary the chip needs a config file used by the >> FW. Add the config files to modinfo so that they can be read by >> userspace. > > The configfile firmware filename is dynamically generated, just adding the list > of all currently shipped ones is not really helpful and this is going to get > out of sync with what we actually have in linux-firmware. I'm aware of this, and I agree. > > I must honestly say that I'm not a fan of this, I guess you are trying to > get some tool which builds a minimal image, such as an initrd generator > to add these files to the image ? > Yes exactly. > I do not immediately have a better idea, but IMHO the solution > this patch proposes is not a good one, so nack from me for this change. > Another path we could go is add a wildcard string instead, for example: MODULE_FIRMWARE("brcm/brcmfmac43455-sdio.*.txt"); AFAIK there is no driver in the kernel that does this. I checked with our dracut developer and right now dracut can't cope with that. But he will try to implement that in the future. So my idea was to maintain that list for now and switch to the wildcard approach once we have dracut support that. Regards, Matthias > Regards, > > Hans > > > >> >> Signed-off-by: Matthias Brugger <mbrugger@suse.com> >> >> --- >> >> .../wireless/broadcom/brcm80211/brcmfmac/sdio.c | 16 ++++++++++++++++ >> 1 file changed, 16 insertions(+) >> >> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c >> b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c >> index 310d8075f5d7..ba18df6d8d94 100644 >> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c >> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c >> @@ -624,6 +624,22 @@ BRCMF_FW_DEF(4359, "brcmfmac4359-sdio"); >> BRCMF_FW_DEF(4373, "brcmfmac4373-sdio"); >> BRCMF_FW_DEF(43012, "brcmfmac43012-sdio"); >> +/* firmware config files */ >> +MODULE_FIRMWARE(BRCMF_FW_DEFAULT_PATH >> "brcm/brcmfmac4330-sdio.Prowise-PT301.txt"); >> +MODULE_FIRMWARE(BRCMF_FW_DEFAULT_PATH >> "brcm/brcmfmac43340-sdio.meegopad-t08.txt"); >> +MODULE_FIRMWARE(BRCMF_FW_DEFAULT_PATH >> "brcm/brcmfmac43340-sdio.pov-tab-p1006w-data.txt"); >> +MODULE_FIRMWARE(BRCMF_FW_DEFAULT_PATH >> "brcm/brcmfmac43362-sdio.cubietech,cubietruck.txt"); >> +MODULE_FIRMWARE(BRCMF_FW_DEFAULT_PATH >> "brcm/brcmfmac43430a0-sdio.jumper-ezpad-mini3.txt"); >> +MODULE_FIRMWARE(BRCMF_FW_DEFAULT_PATH "brcm/brcmfmac43430a0-sdio.ONDA-V80 >> PLUS.txt"); >> +MODULE_FIRMWARE(BRCMF_FW_DEFAULT_PATH "brcm/brcmfmac43430-sdio.AP6212.txt"); >> +MODULE_FIRMWARE(BRCMF_FW_DEFAULT_PATH >> "brcm/brcmfmac43430-sdio.Hampoo-D2D3_Vi8A1.txt"); >> +MODULE_FIRMWARE(BRCMF_FW_DEFAULT_PATH "brcm/brcmfmac43430-sdio.MUR1DX.txt"); >> +MODULE_FIRMWARE(BRCMF_FW_DEFAULT_PATH >> "brcm/brcmfmac43430-sdio.raspberrypi,3-model-b.txt"); >> +MODULE_FIRMWARE(BRCMF_FW_DEFAULT_PATH "brcm/brcmfmac43455-sdio.MINIX-NEO >> Z83-4.txt"); >> +MODULE_FIRMWARE(BRCMF_FW_DEFAULT_PATH >> "brcm/brcmfmac43455-sdio.raspberrypi,3-model-b-plus.txt"); >> +MODULE_FIRMWARE(BRCMF_FW_DEFAULT_PATH >> "brcm/brcmfmac43455-sdio.raspberrypi,4-model-b.txt"); >> +MODULE_FIRMWARE(BRCMF_FW_DEFAULT_PATH >> "brcm/brcmfmac4356-pcie.gpd-win-pocket.txt"); >> + >> static const struct brcmf_firmware_mapping brcmf_sdio_fwnames[] = { >> BRCMF_FW_ENTRY(BRCM_CC_43143_CHIP_ID, 0xFFFFFFFF, 43143), >> BRCMF_FW_ENTRY(BRCM_CC_43241_CHIP_ID, 0x0000001F, 43241B0), >> > ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] brcmfmac: expose firmware config files through modinfo 2020-07-01 15:46 ` Matthias Brugger @ 2020-07-02 18:00 ` Hans de Goede 2020-07-03 14:01 ` Matthias Brugger 0 siblings, 1 reply; 7+ messages in thread From: Hans de Goede @ 2020-07-02 18:00 UTC (permalink / raw) To: Matthias Brugger, matthias.bgg, Jakub Kicinski, Kalle Valo, David S . Miller Cc: Pali Rohár, Guenter Roeck, Chi-Hsien Lin, Franky Lin, Chung-Hsien Hsu, Jean-Philippe Brucker, Double Lo, Frank Kao, linux-wireless, brcm80211-dev-list.pdl, Arend van Spriel, Gustavo A . R . Silva, netdev, Rafał Miłecki, Hante Meuleman, Wright Feng, Saravanan Shanmugham, brcm80211-dev-list, linux-kernel, Ulf Hansson, Soeren Moch Hi, On 7/1/20 5:46 PM, Matthias Brugger wrote: > Hi Hans, > > On 01/07/2020 17:38, Hans de Goede wrote: >> Hi, >> >> On 7/1/20 5:31 PM, matthias.bgg@kernel.org wrote: >>> From: Matthias Brugger <mbrugger@suse.com> >>> >>> Apart from a firmware binary the chip needs a config file used by the >>> FW. Add the config files to modinfo so that they can be read by >>> userspace. >> >> The configfile firmware filename is dynamically generated, just adding the list >> of all currently shipped ones is not really helpful and this is going to get >> out of sync with what we actually have in linux-firmware. > > I'm aware of this, and I agree. > >> >> I must honestly say that I'm not a fan of this, I guess you are trying to >> get some tool which builds a minimal image, such as an initrd generator >> to add these files to the image ? >> > > Yes exactly. > >> I do not immediately have a better idea, but IMHO the solution >> this patch proposes is not a good one, so nack from me for this change. >> > > Another path we could go is add a wildcard string instead, for example: > MODULE_FIRMWARE("brcm/brcmfmac43455-sdio.*.txt"); I was thinking about the same lines, but I'm afraid some user-space utils may blow up if we introduce this, which is why I did not suggest it in my previous email. > AFAIK there is no driver in the kernel that does this. I checked with our dracut > developer and right now dracut can't cope with that. Can't cope as in tries to add "/lib/firmware/brcm/brcmfmac43455-sdio.*.txt" and then skips it (as it does for other missing firmware files); or can't cope as in blows-up and aborts without leaving a valid initrd behind. If is the former, that is fine, if it is the latter that is a problem. > But he will try to > implement that in the future. > > So my idea was to maintain that list for now and switch to the wildcard approach > once we have dracut support that. So lets assume that the wildcard approach is ok and any initrd tools looking at the MODULE_FIRMWARE metadata either accidentally do what we want; or fail gracefully. Then if we temporarily add the long MODULE_FIRMWARE list now, those which fail gracefully will start doing the right thing (except they add too much firmware), and later on we cannot remove all the non wildcard MODULE_FIRMWARE list entries because that will cause a regression. Because of this I'm not a fan of temporarily fixing this like this. Using wifi inside the initrd is very much a cornercase anyways, so I think users can use a workaround by dropping an /etc/dracut.conf.d file adding the necessary config file for now. As for the long run, I was thinking that even with regular firmware files we are adding too much firmware to host-specific initrds since we add all the firmwares listed with MODULE_FIRMWARE, and typically only a few are actually necessary. We could modify the firmware_loader code under drivers/base/firmware_loader to keep a list of all files loaded since boot; and export that somewhere under /sys, then dracut could use that list in host-only mode and we get a smaller initrd. One challenge with this approach though is firmware files which are necessary for a new kernel, but not used by the running kernel ... I'm afraid I do not have a good answer to that. Regards, Hans >>> Signed-off-by: Matthias Brugger <mbrugger@suse.com> >>> >>> --- >>> >>> .../wireless/broadcom/brcm80211/brcmfmac/sdio.c | 16 ++++++++++++++++ >>> 1 file changed, 16 insertions(+) >>> >>> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c >>> b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c >>> index 310d8075f5d7..ba18df6d8d94 100644 >>> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c >>> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c >>> @@ -624,6 +624,22 @@ BRCMF_FW_DEF(4359, "brcmfmac4359-sdio"); >>> BRCMF_FW_DEF(4373, "brcmfmac4373-sdio"); >>> BRCMF_FW_DEF(43012, "brcmfmac43012-sdio"); >>> +/* firmware config files */ >>> +MODULE_FIRMWARE(BRCMF_FW_DEFAULT_PATH >>> "brcm/brcmfmac4330-sdio.Prowise-PT301.txt"); >>> +MODULE_FIRMWARE(BRCMF_FW_DEFAULT_PATH >>> "brcm/brcmfmac43340-sdio.meegopad-t08.txt"); >>> +MODULE_FIRMWARE(BRCMF_FW_DEFAULT_PATH >>> "brcm/brcmfmac43340-sdio.pov-tab-p1006w-data.txt"); >>> +MODULE_FIRMWARE(BRCMF_FW_DEFAULT_PATH >>> "brcm/brcmfmac43362-sdio.cubietech,cubietruck.txt"); >>> +MODULE_FIRMWARE(BRCMF_FW_DEFAULT_PATH >>> "brcm/brcmfmac43430a0-sdio.jumper-ezpad-mini3.txt"); >>> +MODULE_FIRMWARE(BRCMF_FW_DEFAULT_PATH "brcm/brcmfmac43430a0-sdio.ONDA-V80 >>> PLUS.txt"); >>> +MODULE_FIRMWARE(BRCMF_FW_DEFAULT_PATH "brcm/brcmfmac43430-sdio.AP6212.txt"); >>> +MODULE_FIRMWARE(BRCMF_FW_DEFAULT_PATH >>> "brcm/brcmfmac43430-sdio.Hampoo-D2D3_Vi8A1.txt"); >>> +MODULE_FIRMWARE(BRCMF_FW_DEFAULT_PATH "brcm/brcmfmac43430-sdio.MUR1DX.txt"); >>> +MODULE_FIRMWARE(BRCMF_FW_DEFAULT_PATH >>> "brcm/brcmfmac43430-sdio.raspberrypi,3-model-b.txt"); >>> +MODULE_FIRMWARE(BRCMF_FW_DEFAULT_PATH "brcm/brcmfmac43455-sdio.MINIX-NEO >>> Z83-4.txt"); >>> +MODULE_FIRMWARE(BRCMF_FW_DEFAULT_PATH >>> "brcm/brcmfmac43455-sdio.raspberrypi,3-model-b-plus.txt"); >>> +MODULE_FIRMWARE(BRCMF_FW_DEFAULT_PATH >>> "brcm/brcmfmac43455-sdio.raspberrypi,4-model-b.txt"); >>> +MODULE_FIRMWARE(BRCMF_FW_DEFAULT_PATH >>> "brcm/brcmfmac4356-pcie.gpd-win-pocket.txt"); >>> + >>> static const struct brcmf_firmware_mapping brcmf_sdio_fwnames[] = { >>> BRCMF_FW_ENTRY(BRCM_CC_43143_CHIP_ID, 0xFFFFFFFF, 43143), >>> BRCMF_FW_ENTRY(BRCM_CC_43241_CHIP_ID, 0x0000001F, 43241B0), >>> >> > ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] brcmfmac: expose firmware config files through modinfo 2020-07-02 18:00 ` Hans de Goede @ 2020-07-03 14:01 ` Matthias Brugger 2020-07-03 14:10 ` Hans de Goede 0 siblings, 1 reply; 7+ messages in thread From: Matthias Brugger @ 2020-07-03 14:01 UTC (permalink / raw) To: Hans de Goede, Matthias Brugger, matthias.bgg, Jakub Kicinski, Kalle Valo, David S . Miller Cc: Pali Rohár, Guenter Roeck, Chi-Hsien Lin, Franky Lin, Chung-Hsien Hsu, Jean-Philippe Brucker, Double Lo, Frank Kao, linux-wireless, brcm80211-dev-list.pdl, Arend van Spriel, Gustavo A . R . Silva, netdev, Rafał Miłecki, Hante Meuleman, Wright Feng, Saravanan Shanmugham, brcm80211-dev-list, linux-kernel, Ulf Hansson, Soeren Moch On 02/07/2020 20:00, Hans de Goede wrote: > Hi, > > On 7/1/20 5:46 PM, Matthias Brugger wrote: >> Hi Hans, >> >> On 01/07/2020 17:38, Hans de Goede wrote: >>> Hi, >>> >>> On 7/1/20 5:31 PM, matthias.bgg@kernel.org wrote: >>>> From: Matthias Brugger <mbrugger@suse.com> >>>> >>>> Apart from a firmware binary the chip needs a config file used by the >>>> FW. Add the config files to modinfo so that they can be read by >>>> userspace. >>> >>> The configfile firmware filename is dynamically generated, just adding the list >>> of all currently shipped ones is not really helpful and this is going to get >>> out of sync with what we actually have in linux-firmware. >> >> I'm aware of this, and I agree. >> >>> >>> I must honestly say that I'm not a fan of this, I guess you are trying to >>> get some tool which builds a minimal image, such as an initrd generator >>> to add these files to the image ? >>> >> >> Yes exactly. >> >>> I do not immediately have a better idea, but IMHO the solution >>> this patch proposes is not a good one, so nack from me for this change. >>> >> >> Another path we could go is add a wildcard string instead, for example: >> MODULE_FIRMWARE("brcm/brcmfmac43455-sdio.*.txt"); > > I was thinking about the same lines, but I'm afraid some user-space > utils may blow up if we introduce this, which is why I did not suggest > it in my previous email. > >> AFAIK there is no driver in the kernel that does this. I checked with our dracut >> developer and right now dracut can't cope with that. > > Can't cope as in tries to add "/lib/firmware/brcm/brcmfmac43455-sdio.*.txt" > and then skips it (as it does for other missing firmware files); or can't > cope as in blows-up and aborts without leaving a valid initrd behind. > > If is the former, that is fine, if it is the latter that is a problem. > >> But he will try to >> implement that in the future. >> >> So my idea was to maintain that list for now and switch to the wildcard approach >> once we have dracut support that. > > So lets assume that the wildcard approach is ok and any initrd tools looking at > the MODULE_FIRMWARE metadata either accidentally do what we want; or fail > gracefully. Then if we temporarily add the long MODULE_FIRMWARE list now, those > which fail gracefully will start doing the right thing (except they add too > much firmware), and later on we cannot remove all the non wildcard > MODULE_FIRMWARE list entries because that will cause a regression. > > Because of this I'm not a fan of temporarily fixing this like this. Using wifi > inside the initrd is very much a cornercase anyways, so I think users can > use a workaround by dropping an /etc/dracut.conf.d file adding the necessary > config file for now. > > As for the long run, I was thinking that even with regular firmware files > we are adding too much firmware to host-specific initrds since we add all > the firmwares listed with MODULE_FIRMWARE, and typically only a few are > actually necessary. > > We could modify the firmware_loader code under drivers/base/firmware_loader > to keep a list of all files loaded since boot; and export that somewhere > under /sys, then dracut could use that list in host-only mode and we get > a smaller initrd. One challenge with this approach though is firmware files > which are necessary for a new kernel, but not used by the running kernel ... > I'm afraid I do not have a good answer to that. > That would work for creating a new minimal initrd from a working image. But it would not help in bootstrapping an image. My understanding is that for bootstrapping an image we will need to support wildcards in MODULE_FIRMWARE() strings. Regards, Matthias > Regards, > > Hans > > > > > > > >>>> Signed-off-by: Matthias Brugger <mbrugger@suse.com> >>>> >>>> --- >>>> >>>> .../wireless/broadcom/brcm80211/brcmfmac/sdio.c | 16 ++++++++++++++++ >>>> 1 file changed, 16 insertions(+) >>>> >>>> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c >>>> b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c >>>> index 310d8075f5d7..ba18df6d8d94 100644 >>>> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c >>>> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c >>>> @@ -624,6 +624,22 @@ BRCMF_FW_DEF(4359, "brcmfmac4359-sdio"); >>>> BRCMF_FW_DEF(4373, "brcmfmac4373-sdio"); >>>> BRCMF_FW_DEF(43012, "brcmfmac43012-sdio"); >>>> +/* firmware config files */ >>>> +MODULE_FIRMWARE(BRCMF_FW_DEFAULT_PATH >>>> "brcm/brcmfmac4330-sdio.Prowise-PT301.txt"); >>>> +MODULE_FIRMWARE(BRCMF_FW_DEFAULT_PATH >>>> "brcm/brcmfmac43340-sdio.meegopad-t08.txt"); >>>> +MODULE_FIRMWARE(BRCMF_FW_DEFAULT_PATH >>>> "brcm/brcmfmac43340-sdio.pov-tab-p1006w-data.txt"); >>>> +MODULE_FIRMWARE(BRCMF_FW_DEFAULT_PATH >>>> "brcm/brcmfmac43362-sdio.cubietech,cubietruck.txt"); >>>> +MODULE_FIRMWARE(BRCMF_FW_DEFAULT_PATH >>>> "brcm/brcmfmac43430a0-sdio.jumper-ezpad-mini3.txt"); >>>> +MODULE_FIRMWARE(BRCMF_FW_DEFAULT_PATH "brcm/brcmfmac43430a0-sdio.ONDA-V80 >>>> PLUS.txt"); >>>> +MODULE_FIRMWARE(BRCMF_FW_DEFAULT_PATH "brcm/brcmfmac43430-sdio.AP6212.txt"); >>>> +MODULE_FIRMWARE(BRCMF_FW_DEFAULT_PATH >>>> "brcm/brcmfmac43430-sdio.Hampoo-D2D3_Vi8A1.txt"); >>>> +MODULE_FIRMWARE(BRCMF_FW_DEFAULT_PATH "brcm/brcmfmac43430-sdio.MUR1DX.txt"); >>>> +MODULE_FIRMWARE(BRCMF_FW_DEFAULT_PATH >>>> "brcm/brcmfmac43430-sdio.raspberrypi,3-model-b.txt"); >>>> +MODULE_FIRMWARE(BRCMF_FW_DEFAULT_PATH "brcm/brcmfmac43455-sdio.MINIX-NEO >>>> Z83-4.txt"); >>>> +MODULE_FIRMWARE(BRCMF_FW_DEFAULT_PATH >>>> "brcm/brcmfmac43455-sdio.raspberrypi,3-model-b-plus.txt"); >>>> +MODULE_FIRMWARE(BRCMF_FW_DEFAULT_PATH >>>> "brcm/brcmfmac43455-sdio.raspberrypi,4-model-b.txt"); >>>> +MODULE_FIRMWARE(BRCMF_FW_DEFAULT_PATH >>>> "brcm/brcmfmac4356-pcie.gpd-win-pocket.txt"); >>>> + >>>> static const struct brcmf_firmware_mapping brcmf_sdio_fwnames[] = { >>>> BRCMF_FW_ENTRY(BRCM_CC_43143_CHIP_ID, 0xFFFFFFFF, 43143), >>>> BRCMF_FW_ENTRY(BRCM_CC_43241_CHIP_ID, 0x0000001F, 43241B0), >>>> >>> >> > ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] brcmfmac: expose firmware config files through modinfo 2020-07-03 14:01 ` Matthias Brugger @ 2020-07-03 14:10 ` Hans de Goede 0 siblings, 0 replies; 7+ messages in thread From: Hans de Goede @ 2020-07-03 14:10 UTC (permalink / raw) To: Matthias Brugger, Matthias Brugger, matthias.bgg, Jakub Kicinski, Kalle Valo, David S . Miller Cc: Pali Rohár, Guenter Roeck, Chi-Hsien Lin, Franky Lin, Chung-Hsien Hsu, Jean-Philippe Brucker, Double Lo, Frank Kao, linux-wireless, brcm80211-dev-list.pdl, Arend van Spriel, Gustavo A . R . Silva, netdev, Rafał Miłecki, Hante Meuleman, Wright Feng, Saravanan Shanmugham, brcm80211-dev-list, linux-kernel, Ulf Hansson, Soeren Moch Hi, On 7/3/20 4:01 PM, Matthias Brugger wrote: > > > On 02/07/2020 20:00, Hans de Goede wrote: >> Hi, >> >> On 7/1/20 5:46 PM, Matthias Brugger wrote: >>> Hi Hans, >>> >>> On 01/07/2020 17:38, Hans de Goede wrote: >>>> Hi, >>>> >>>> On 7/1/20 5:31 PM, matthias.bgg@kernel.org wrote: >>>>> From: Matthias Brugger <mbrugger@suse.com> >>>>> >>>>> Apart from a firmware binary the chip needs a config file used by the >>>>> FW. Add the config files to modinfo so that they can be read by >>>>> userspace. >>>> >>>> The configfile firmware filename is dynamically generated, just adding the list >>>> of all currently shipped ones is not really helpful and this is going to get >>>> out of sync with what we actually have in linux-firmware. >>> >>> I'm aware of this, and I agree. >>> >>>> >>>> I must honestly say that I'm not a fan of this, I guess you are trying to >>>> get some tool which builds a minimal image, such as an initrd generator >>>> to add these files to the image ? >>>> >>> >>> Yes exactly. >>> >>>> I do not immediately have a better idea, but IMHO the solution >>>> this patch proposes is not a good one, so nack from me for this change. >>>> >>> >>> Another path we could go is add a wildcard string instead, for example: >>> MODULE_FIRMWARE("brcm/brcmfmac43455-sdio.*.txt"); >> >> I was thinking about the same lines, but I'm afraid some user-space >> utils may blow up if we introduce this, which is why I did not suggest >> it in my previous email. >> >>> AFAIK there is no driver in the kernel that does this. I checked with our dracut >>> developer and right now dracut can't cope with that. >> >> Can't cope as in tries to add "/lib/firmware/brcm/brcmfmac43455-sdio.*.txt" >> and then skips it (as it does for other missing firmware files); or can't >> cope as in blows-up and aborts without leaving a valid initrd behind. >> >> If is the former, that is fine, if it is the latter that is a problem. >> >>> But he will try to >>> implement that in the future. >>> >>> So my idea was to maintain that list for now and switch to the wildcard approach >>> once we have dracut support that. >> >> So lets assume that the wildcard approach is ok and any initrd tools looking at >> the MODULE_FIRMWARE metadata either accidentally do what we want; or fail >> gracefully. Then if we temporarily add the long MODULE_FIRMWARE list now, those >> which fail gracefully will start doing the right thing (except they add too >> much firmware), and later on we cannot remove all the non wildcard >> MODULE_FIRMWARE list entries because that will cause a regression. >> >> Because of this I'm not a fan of temporarily fixing this like this. Using wifi >> inside the initrd is very much a cornercase anyways, so I think users can >> use a workaround by dropping an /etc/dracut.conf.d file adding the necessary >> config file for now. >> >> As for the long run, I was thinking that even with regular firmware files >> we are adding too much firmware to host-specific initrds since we add all >> the firmwares listed with MODULE_FIRMWARE, and typically only a few are >> actually necessary. >> >> We could modify the firmware_loader code under drivers/base/firmware_loader >> to keep a list of all files loaded since boot; and export that somewhere >> under /sys, then dracut could use that list in host-only mode and we get >> a smaller initrd. One challenge with this approach though is firmware files >> which are necessary for a new kernel, but not used by the running kernel ... >> I'm afraid I do not have a good answer to that. >> > > That would work for creating a new minimal initrd from a working image. But it > would not help in bootstrapping an image. My understanding is that for > bootstrapping an image we will need to support wildcards in MODULE_FIRMWARE() > strings. Yes, I agree. Regards, Hans ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] brcmfmac: expose firmware config files through modinfo 2020-07-01 15:31 [PATCH] brcmfmac: expose firmware config files through modinfo matthias.bgg 2020-07-01 15:38 ` Hans de Goede @ 2020-07-14 9:37 ` Kalle Valo 1 sibling, 0 replies; 7+ messages in thread From: Kalle Valo @ 2020-07-14 9:37 UTC (permalink / raw) To: matthias.bgg Cc: Jakub Kicinski, David S . Miller, hdegoede, Pali Rohár, Guenter Roeck, Chi-Hsien Lin, Franky Lin, Chung-Hsien Hsu, Jean-Philippe Brucker, Double Lo, Frank Kao, linux-wireless, brcm80211-dev-list.pdl, Arend van Spriel, Gustavo A . R . Silva, netdev, Rafał Miłecki, Hante Meuleman, Wright Feng, Matthias Brugger, Saravanan Shanmugham, brcm80211-dev-list, linux-kernel, Ulf Hansson, Soeren Moch matthias.bgg@kernel.org wrote: > From: Matthias Brugger <mbrugger@suse.com> > > Apart from a firmware binary the chip needs a config file used by the > FW. Add the config files to modinfo so that they can be read by > userspace. > > Signed-off-by: Matthias Brugger <mbrugger@suse.com> I agree with Hans, this does not look a good solution for the problem. Patch set to Changes Requested. -- https://patchwork.kernel.org/patch/11636715/ https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2020-07-14 9:37 UTC | newest] Thread overview: 7+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2020-07-01 15:31 [PATCH] brcmfmac: expose firmware config files through modinfo matthias.bgg 2020-07-01 15:38 ` Hans de Goede 2020-07-01 15:46 ` Matthias Brugger 2020-07-02 18:00 ` Hans de Goede 2020-07-03 14:01 ` Matthias Brugger 2020-07-03 14:10 ` Hans de Goede 2020-07-14 9:37 ` Kalle Valo
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox