Messages in this thread | | | Subject | Re: [PATCH] brcmfmac: expose firmware config files through modinfo | From | Hans de Goede <> | Date | Fri, 3 Jul 2020 16:10:09 +0200 |
| |
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
| |