Re: [RFC] ath10k: silence firmware file probing warnings

[Stanislaw Gruszka <sgruszka@redhat.com>]

On Fri, Jul 22, 2016 at 10:38:24AM +0200, Arend Van Spriel wrote:
> + Luis
> 
> On 21-7-2016 13:51, Stanislaw Gruszka wrote:
> > (cc: firmware and brcmfmac maintainers)
> > 
> > On Thu, Jul 21, 2016 at 06:23:11AM -0400, Prarit Bhargava wrote:
> >>
> >>
> >> On 07/21/2016 04:05 AM, Stanislaw Gruszka wrote:
> >>> On Thu, Jul 21, 2016 at 10:36:42AM +0300, Emmanuel Grumbach wrote:
> >>>> On Thu, Jul 21, 2016 at 10:09 AM, Stanislaw Gruszka <sgruszka@redhat.com> wrote:
> >>>>> On Tue, Jul 19, 2016 at 03:00:37PM +0200, Michal Kazior wrote:
> >>>>>> Firmware files are versioned to prevent older
> >>>>>> driver instances to load unsupported firmware
> >>>>>> blobs. This is reflected with a fallback logic
> >>>>>> which attempts to load several firmware files.
> >>>>>>
> >>>>>> This however produced a lot of unnecessary
> >>>>>> warnings sometimes confusing users and leading
> >>>>>> them to rename firmware files making things even
> >>>>>> more confusing.
> >>>>>
> >>>>> This happens on kernels configured with
> >>>>> CONFIG_FW_LOADER_USER_HELPER_FALLBACK and cause not only ugly warnings,
> >>>>> but also 60 seconds delay before loading next firmware version.
> >>>>> For some reason RHEL kernel needs above config option, so this
> >>>>> patch is very welcome from my perspective.
> >>>>>
> >>>>
> >>>> Sorry for my ignorance but how does the firmware loading work if not
> >>>> with udev's help?
> >>>
> >>> I'm not sure exactly, but I think kernel VFS layer is capable to copy
> >>> file data directly from mounted filesystem without user space helper.
> >>
> >> Here's the situation: request_firmware() waits 60 seconds for udev to do its
> >> loading magic via a "usermode helper".  This delay is there to allow, for
> >> example, userspace to unpack or download a new firmware image or verify the
> >> firmware image *in userspace* before providing it to the driver to apply to the HW.
> >>
> >> Why 60 seconds?  It is arbitrary and there is no way for udev & the kernel to
> >> handshake on completion.
> >>
> >>>
> >>>> As you can imagine, iwlwifi is suffering from the
> >>>> same problem and I would be interested in applying the same change,
> >>>> but I'd love to understand a bit more :)
> >>>
> >>> Yes, iwlwifi (and some other drivers) suffer from this. However this
> >>> happen when the newest firmware version is not installed on the system
> >>> and CONFIG_FW_LOADER_USER_HELPER_FALLBACK is enabled. What I suppose
> >>> it's not common.
> >>
> >> request_firmware_direct() was introduced at my request because (as you've
> >> noticed) when CONFIG_FW_LOADER_USER_HELPER_FALLBACK=y drivers may stall for long
> >> periods of time when starting.  The bug that this introduced was a 60 second
> >> delay per logical cpu when starting a system.  On a 64 cpu system that meant the
> >> boot would complete in a little over one hour.
> >>
> >>>
> >>> I started to see this currently, because that option was enabled on 
> >>> RHEL kernel. BTW: I think Prarit iwlwifi thermal_zone problem was
> >>> happened because of that, i.e. thermal device was not functional
> >>> because f/w wasn't loaded due to big delay.
> >>>
> >>> I'm not sure if replacing to request_firmware_direct() is a good
> >>> fix though. For example I can see this problem also on brcmfmac, which
> >>> use request_firmware_nowait(). I think I would rather prefer special
> >>> helper for firmware drivers that needs user helper and have
> >>> request_firmware() be direct as default.
> >>>
> >>
> >> The difference between request_firmware_direct() and request_firmware() is that
> >> the _direct() version does not wait the 60 seconds for udev interaction.  The
> >> only userspace check performed is to see if the file is there, and if the file
> >> does exist it is provided to the driver to be applied to the hardware.
> >>
> >> So the real question to ask here is whether or not the ath10k, brcmfmac, and
> >> iwlwifi require udev to do anything beyond checking for the existence and
> >> loading the firmware image.  If they don't, then it is better to use
> >> request_firmware_direct().
> > 
> > They don't need that, like 99% of the drivers I think, hence changing the
> > default seems to be more reasonable. However changing 3 drivers would work
> > for me as well, and that change do not introduce risk of broking drivers
> > that require udev fw download.
> > 
> > iwlwifi and ath10k are trivial, bcrmfmac is a bit more complex as it
> > use request_firmware_nowait(), so it first need to be converted to
> > ordinary request_firmware(), but this should be doable and I can do
> > that.
> 
> I am going bonkers here. This is the Nth time a discussion pops up on
> firmware API usage. I stopped counting N :-( So the first issue was that
> the INIT was taking to long as we were requesting firmware during probe
> which was executed in the INIT context. So we added a worker and
> register the driver from there. There was probably a reason for
> switching to _no_wait() as well, but I do not recall the details. The
> things is I don't know if I need user-space or not. I just need firmware
> to get the device up and running. We have changed our driver a couple of
> times now to accommodate something that in my opinion should have been
> abstracted behind the firmware API in the first place and now here is
> another proposal to change the drivers. Come on!

I understand you dislike that :-) Just to clarify the issue here:

Some drivers (including brcmfmac) request new firmware images, which are
not yet available (i.e. development F/W versions) and then fall-back
to older firmware version and works perfectly fine.

However with CONFIG_FW_LOADER_USER_HELPER_FALLBACK=y configured, in case
of missing F/W image, request firmware involve user space helper and
waits 60s (loading_timeout value from drivers/base/firmware_class.c),
what delays creating network interface and confuse users.

For brcmfmac this looks like this:

[   15.160923] brcmfmac 0000:03:00.0: Direct firmware load for brcm/brcmfmac4356-pcie.txt failed with error -2
[   15.170759] brcmfmac 0000:03:00.0: Falling back to user helper
<snip>
[   75.709397] brcmfmac: brcmf_c_preinit_dcmds: Firmware version = wl0: Oct 22 2015 06:16:41 version 7.35.180.119 (r594535) FWID 01-1a5c4016
[   75.736941] brcmfmac: brcmf_cfg80211_reg_notifier: not a ISO3166 code (0x30 0x30)

Without CONFIG_FW_LOADER_USER_HELPER_FALLBACK first firmware request
silently fail and then instantly next F/W image is loaded.

Another option to solve to problem would be stop requesting not
available publicly firmware. However, I assume some drivers would
like to preserve that option.

> > However I wonder if changing that will not broke the case when
> > driver is build-in in the kernel and f/w is not yet available when
> > driver start to initialize. Or maybe nowadays this is not the case
> > any longer, i.e. the MODULE_FIRMWARE macros assure proper f/w 
> > images are build-in in the kernel or copied to initramfs?
> 
> That is a nice idea, but I have not seen any change in that area. Could
> have missed it.

I believe this is how the things are already done, IOW switching to
request_firmware_direct() in the driver should be no harm.

Stanislaw