diff mbox series

brcmfmac: expose firmware config files through modinfo

Message ID 20200701153123.25602-1-matthias.bgg@kernel.org (mailing list archive)
State Changes Requested
Delegated to: Kalle Valo
Headers show
Series brcmfmac: expose firmware config files through modinfo | expand

Commit Message

Matthias Brugger July 1, 2020, 3:31 p.m. UTC 
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(+)

Comments

Hans de Goede July 1, 2020, 3:38 p.m. UTC | #1 
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),
>
Matthias Brugger July 1, 2020, 3:46 p.m. UTC | #2 
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),
>>
>
Hans de Goede July 2, 2020, 6 p.m. UTC | #3 
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),
>>>
>>
>
Matthias Brugger July 3, 2020, 2:01 p.m. UTC | #4 
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),
>>>>
>>>
>>
>
Hans de Goede July 3, 2020, 2:10 p.m. UTC | #5 
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
Kalle Valo July 14, 2020, 9:37 a.m. UTC | #6 
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.
diff mbox series

Patch

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