On Tuesday, May 03, 2011, Valdis.Kletnieks@xxxxxx wrote:
> On Tue, 03 May 2011 00:44:51 +0200, "Rafael J. Wysocki" said:
>
> > + if (WARN_ON(usermodehelper_is_disabled()))
> > + return -EBUSY;
> > +
>
> Since this is a "no user serviceable parts inside" type of error, so I guess
> WARN_ON rather than a printk(KERN_WARNING is a good idea so we get
> a traceback pointing out the offending driver.
>
> I have to wonder 2 things though:
>
> 1) What percent of the time the missing firmware (or other issues, like the
> display not being resumed yet) will prevent the WARN_ON output from making it
> to the display *anyhow*, so the user *still* hits the power button to try again?
Although the WARN_ON output will probably not make it to the user's screen
immediately, the resume will continue without the delay, because error code
will be returned as soon as the WARN_ON triggers.
The user will likely notice the device not working after the resume and will
look at dmesg output. :-)
> 2) What percent of the time the WARN_ON output will itself make the user
> think the resume has died rather than just being slow, causing them to power
> cycle and hope for a clean boot?
>
> Maybe something like this instead?
>
> if (WARN_ON(usermodehelper_is_disable()))) {
> printk(KERN_WARNING "Resume continuing, but firmware for %s not loaded", device);
It's useful, but I'd rather do
dev_info(device, "firmware: %s will not be loaded\n", name);
> return -EBUSY;
> }
>
> (or whatever that %s actually needs to work)
>
> All the same, it still looks better than what we're doing now.
OK, updated patch is appended.
Thanks,
Rafael
---
From: Rafael J. Wysocki <rjw@xxxxxxx>
Subject: PM: Print a warning if firmware is requested when tasks are frozen
Some drivers erroneously use request_firmware() from their ->resume()
(or ->thaw(), or ->restore()) callbacks, which is not going to work
unless the firmware has been built in. This causes system resume to
stall until the firmware-loading timeout expires, which makes users
think that the resume has failed and reboot their machines
unnecessarily. For this reason, make _request_firmware() print a
warning and return immediately with error code if it has been called
when tasks are frozen and it's impossible to start any new usermode
helpers.
Signed-off-by: Rafael J. Wysocki <rjw@xxxxxxx>
---
drivers/base/firmware_class.c | 5 +++++
include/linux/kmod.h | 1 +
kernel/kmod.c | 8 ++++++++
3 files changed, 14 insertions(+)
Index: linux-2.6/include/linux/kmod.h
===================================================================
--- linux-2.6.orig/include/linux/kmod.h
+++ linux-2.6/include/linux/kmod.h
@@ -113,5 +113,6 @@ extern void usermodehelper_init(void);
extern int usermodehelper_disable(void);
extern void usermodehelper_enable(void);
+extern bool usermodehelper_is_disabled(void);
#endif /* __LINUX_KMOD_H__ */
Index: linux-2.6/kernel/kmod.c
===================================================================
--- linux-2.6.orig/kernel/kmod.c
+++ linux-2.6/kernel/kmod.c
@@ -301,6 +301,14 @@ void usermodehelper_enable(void)
usermodehelper_disabled = 0;
}
+/**
+ * usermodehelper_is_disabled - check if new helpers are allowed to be started
+ */
+bool usermodehelper_is_disabled(void)
+{
+ return usermodehelper_disabled;
+}
+
static void helper_lock(void)
{
atomic_inc(&running_helpers);
Index: linux-2.6/drivers/base/firmware_class.c
===================================================================
--- linux-2.6.orig/drivers/base/firmware_class.c
+++ linux-2.6/drivers/base/firmware_class.c
@@ -521,6 +521,11 @@ static int _request_firmware(const struc
if (!firmware_p)
return -EINVAL;
+ if (WARN_ON(usermodehelper_is_disabled())) {
+ dev_info(device, "firmware: %s will not be loaded\n", name);
+ return -EBUSY;
+ }
+
*firmware_p = firmware = kzalloc(sizeof(*firmware), GFP_KERNEL);
if (!firmware) {
dev_err(device, "%s: kmalloc(struct firmware) failed\n",
_______________________________________________
linux-pm mailing list
linux-pm@xxxxxxxxxxxxxxxxxxxxxxxxxx
https://lists.linux-foundation.org/mailman/listinfo/linux-pm
