lkml.org 
[lkml]   [2012]   [Mar]   [16]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
From
SubjectRe: [RFC] firmware loader: retry _nowait requests when userhelper is not yet available
Date
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.
> > > >
> > > > ---
> > > > I'm aware about the previous "request_firmware* in probe" discussions.
> > > > Unfortunately, the hardware needs firmware so there is no other way
> > > > around it. So please, I just wanted to know what the general opinion
> > > > about the idea behind this patch is.
> >
> > 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.

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.

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

Thanks,
Rafael

---
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);
+ if (loading_timeout > 0) {
+ limit -= timeout;
+ if (limit <= 0) {
+ retval = -EBUSY;
+ goto out;
+ }
+ }
+ timeout += timeout;
+ if (loading_timeout > 0 && timeout > limit)
+ timeout = limit;
+
+ read_lock_usermodehelper();
+ }
+ } else if (WARN_ON(usermodehelper_is_disabled())) {
dev_err(device, "firmware: %s will not be loaded\n", name);
retval = -EBUSY;
- goto out;
+ goto unlock;
}

if (uevent)
@@ -547,7 +569,7 @@ static int _request_firmware(const struc
fw_priv = fw_create_instance(firmware, name, device, uevent, nowait);
if (IS_ERR(fw_priv)) {
retval = PTR_ERR(fw_priv);
- goto out;
+ goto unlock;
}

if (uevent) {
@@ -572,9 +594,10 @@ static int _request_firmware(const struc

fw_destroy_instance(fw_priv);

-out:
+unlock:
read_unlock_usermodehelper();

+out:
if (retval) {
release_firmware(firmware);
*firmware_p = NULL;

\
 
 \ /
  Last update: 2012-03-16 23:55    [from the cache]
©2003-2011 Jasper Spaans