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

[Matthias Brugger <matthias.bgg@gmail.com>]

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