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

[Prarit Bhargava <prarit@redhat.com>]

On 07/22/2016 08:21 AM, Arend Van Spriel wrote:
> On 22-7-2016 12:26, Stanislaw Gruszka wrote:
>> 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.
> 
> Actually, this is not the case with brcmfmac. We do need a firmware
> file, ie. brcm/brcmfmac4356-pcie.bin, and also request for a nvram file,
> ie. brcm/brcmfmac4356-pcie.txt. The latter is optional and the device
> works fine without it.
> 
> What is still unclear to me is when request_firmware_direct() would fail
> and in what circumstances the udev helper is a valid callback. Can you
> explain such a scenario. Another question I have is what the reasons are
> behind the 60 seconds timeout.

request_firmware_direct() will fail when the specified FW file is not present.
This is different from request_firmware() which implements a usermode helper to
potentially download firmware, or unpack a firmware image.

Re: 60 second timeout ... The 60 second timeout with request_firmware() is
completely arbitrary.  There is no way for udev to signal back to the kernel
that userspace helper has not completed its actions, so the kernel has a 60 dead
man timer-ish delay.

> 
>>>> 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.
> 
> Ok. What are the consequences when:
> - driver is built-in.
> - driver+firmware present on initramfs.
> - driver on initramfs, firmware only present on rootfs.
> - driver+firmware only on rootfs.
> 
> I assume the third one would be considered a configuration issue.

I think your question here can be answered by reading drivers/base/Kconfig:88,
and reading about those 4 config options.  I could paraphrase it butI think the
Kconfig notes are better than I could explain it.  Note that this is how things
currently work with request_firmware_nowait().  IIRC request_firmware_nowait()
is just an asynchronous version of request_firmware().

HTH,

P.