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

[Stanislaw Gruszka <sgruszka@redhat.com>]

(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.

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?

Thanks
Stanislaw