Date:   Fri, 3 Jul 2020 16:10:09 +0200
From:   Hans de Goede <hdegoede@...hat.com>
To:     Matthias Brugger <matthias.bgg@...il.com>,
        Matthias Brugger <mbrugger@...e.com>, matthias.bgg@...nel.org,
        Jakub Kicinski <kuba@...nel.org>,
        Kalle Valo <kvalo@...eaurora.org>,
        "David S . Miller" <davem@...emloft.net>
Cc:     Pali Rohár <pali@...nel.org>,
        Guenter Roeck <linux@...ck-us.net>,
        Chi-Hsien Lin <chi-hsien.lin@...ress.com>,
        Franky Lin <franky.lin@...adcom.com>,
        Chung-Hsien Hsu <stanley.hsu@...ress.com>,
        Jean-Philippe Brucker <jean-philippe@...aro.org>,
        Double Lo <double.lo@...ress.com>,
        Frank Kao <frank.kao@...ress.com>,
        linux-wireless@...r.kernel.org,
        brcm80211-dev-list.pdl@...adcom.com,
        Arend van Spriel <arend.vanspriel@...adcom.com>,
        "Gustavo A . R . Silva" <gustavo@...eddedor.com>,
        netdev@...r.kernel.org,
        Rafał Miłecki <rafal@...ecki.pl>,
        Hante Meuleman <hante.meuleman@...adcom.com>,
        Wright Feng <wright.feng@...ress.com>,
        Saravanan Shanmugham <saravanan.shanmugham@...ress.com>,
        brcm80211-dev-list@...ress.com, linux-kernel@...r.kernel.org,
        Ulf Hansson <ulf.hansson@...aro.org>,
        Soeren Moch <smoch@....de>
Subject: Re: [PATCH] brcmfmac: expose firmware config files through modinfo

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@...nel.org wrote:
>>>>> From: Matthias Brugger <mbrugger@...e.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

Powered by blists - more mailing lists