Re: [RFC] firmware loader: retry _nowait requests when userhelper is not yet available

[Christian Lamparter <chunkeey@googlemail.com>]

On Friday 16 March 2012 23:57:10 Rafael J. Wysocki wrote:
> On Friday, March 16, 2012, Christian Lamparter wrote:
> > On Friday 16 March 2012 23:19:53 Rafael J. Wysocki wrote:
> > > > On 03/04/2012 01:52 AM, Christian Lamparter wrote:
> > > > > During resume, the userhelper might not be available. However for
> > > > > drivers which use the request_firmware_nowait interface, this will
> > > > > only lead to a pointless WARNING and a device which no longer works
> > > > > after the resume [since it couldn't get the firmware, because the
> > > > > userhelper was not available to take the request].
> > > > > 
> > > > > In order to solve this "chicken or egg" dilemma, the code now
> > > > > retries _nowait requests at one second intervals until the
> > > > > "loading_timeout" time is up.
> > > 
> > > BTW, I wonder what comments on this patch were posted?
> > Only Alan Cox was kind enough to drop me a few words.
> > 
> > Why? Do you think it is actually sane from a specific POV?
> > [Don't tell me you do :D !]
> 
> I don't think it's really wrong.
Of course, I've tested both patches [the RFC and the other one].
The RFC might not be "really wrong" but the concept of busy
msleeping strikes me as a bit insane. [Maybe, that's because I
write drivers and I hate it when IO needs msleeps... brrr]
 
> I agree that the WARN_ON() isn't really useful in the request_firmware_nowait()
> case, because the user of that doesn't really know when exactly the firmware is
> going to be requested, so it can't really do anything about the warning.
> 
> Moreover, failures of request_firmware_nowait() just because it happens to
> race with system suspend (or something of that kind), just because of "bad"
> timing, aren't really useful either.
> 
If it's just about "waiting until the firmware can be loaded" then why not go
with the "easy approach in the [PATCH] don't cancel...". This just queues the
request in the _nowait case. And once the userspace helper is running, it will
pick up all backlogged firmware requests [of course, only the ones that have
not been timeouted yet] and life goes on!

Anyway, I know that I don't really have a say in what will be accepted.
So, it's all up to you guys! But I'll happily test any patches.

> So, I think it makes sense for it to wait until the firmware can be loaded.
> 
> I'd do that a bit differently, though, for example like in the appended patch
> (untested).
Just one comment. [see below] 

And now, I'm off to bed.

Good night,
	Chr

> ---
>  drivers/base/firmware_class.c |   31 +++++++++++++++++++++++++++----
>  1 file changed, 27 insertions(+), 4 deletions(-)


> Index: linux/drivers/base/firmware_class.c
> ===================================================================
> --- linux.orig/drivers/base/firmware_class.c
> +++ linux/drivers/base/firmware_class.c
> @@ -20,6 +20,7 @@
>  #include <linux/highmem.h>
>  #include <linux/firmware.h>
>  #include <linux/slab.h>
> +#include <linux/delay.h>
>  
>  #define to_dev(obj) container_of(obj, struct device, kobj)
>  
> @@ -535,10 +536,31 @@ static int _request_firmware(const struc
>  
>  	read_lock_usermodehelper();
>  
> -	if (WARN_ON(usermodehelper_is_disabled())) {
> +	if (nowait) {
> +		int limit = loading_timeout * MSEC_PER_SEC;
> +		int timeout = 10;  /* in msec */
> +
> +		while (usermodehelper_is_disabled()) {
> +			read_unlock_usermodehelper();
> +
> +			msleep(timeout);
timeout is 10 ms, right?

so this might apply:
Documentation/timers/timers-howto.txt
	- Why not msleep for (1ms - 20ms)?
		Explained originally here:
			http://lkml.org/lkml/2007/8/3/250
		msleep(1~20) may not do what the caller intends, and
		will often sleep longer (~20 ms actual sleep for any
		value given in the 1~20ms range). In many cases this
		is not the desired behavior.

Of course, that's of little importance.