Re: [RFC][PATCH] PM: Print a warning if firmware is requested when tasks are frozen

["Rafael J. Wysocki" <rjw@sisk.pl>]

On Tuesday, May 03, 2011, Valdis.Kletnieks@vt.edu 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@sisk.pl>
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@sisk.pl>
---
 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",