Re: [PATCH] brcmfmac: expose firmware config files through modinfo

[Hans de Goede]

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@xxxxxxxxxx wrote:
> > > From: Matthias Brugger <mbrugger@xxxxxxxx>
> > >
> > > 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@xxxxxxxx>
> > >
> > > ---
> > >
> > >   .../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),