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

[Hans de Goede <hdegoede@redhat.com>]

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),
>>>
>>
>