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

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

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rjw@sisk.pl>

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 when 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 |    3 +++
 include/linux/kmod.h          |    1 +
 kernel/kmod.c                 |    8 ++++++++
 3 files changed, 12 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,9 @@ static int _request_firmware(const struc
 	if (!firmware_p)
 		return -EINVAL;
 
+	if (WARN_ON(usermodehelper_is_disabled()))
+		return -EBUSY;
+
 	*firmware_p = firmware = kzalloc(sizeof(*firmware), GFP_KERNEL);
 	if (!firmware) {
 		dev_err(device, "%s: kmalloc(struct firmware) failed\n",

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

[Greg KH]

On Tue, May 03, 2011 at 12:44:51AM +0200, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rjw@sisk.pl>
> 
> 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 when 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 |    3 +++
>  include/linux/kmod.h          |    1 +
>  kernel/kmod.c                 |    8 ++++++++
>  3 files changed, 12 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,9 @@ static int _request_firmware(const struc
>  	if (!firmware_p)
>  		return -EINVAL;
>  
> +	if (WARN_ON(usermodehelper_is_disabled()))
> +		return -EBUSY;

But you can safely call this function with nowait set, and this warning
should not be triggered, right?

thanks,

greg k-h

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

[Linus Torvalds]

On Mon, May 2, 2011 at 4:12 PM, Greg KH <gregkh@suse.de> wrote:
>
> But you can safely call this function with nowait set, and this warning
> should not be triggered, right?

Why would you want that?

It's _always_ wrong to ask for firmware during resume. "nowait" or not
is totally irrelevant. A driver that depends on the firmware being
built in to the kernel is a buggy driver, why would you want to
silently allow that kind of crap? It's just a timebomb waiting for
somebody to compile the kernel differently.

                      Linus

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

[Greg KH]

On Mon, May 02, 2011 at 04:21:09PM -0700, Linus Torvalds wrote:
> On Mon, May 2, 2011 at 4:12 PM, Greg KH <gregkh@suse.de> wrote:
> >
> > But you can safely call this function with nowait set, and this warning
> > should not be triggered, right?
> 
> Why would you want that?
> 
> It's _always_ wrong to ask for firmware during resume. "nowait" or not
> is totally irrelevant. A driver that depends on the firmware being
> built in to the kernel is a buggy driver, why would you want to
> silently allow that kind of crap? It's just a timebomb waiting for
> somebody to compile the kernel differently.

A driver that does not rely on the firmware being built in would be
correct in calling request_firmware_nowait() on resume, then when
userspace is properly woken up, the firmware would be sent to the
device, then the driver would be notified, load it, and handle things
as part of its resume sequence from that notification.

Isn't that ideally what we want to have happen?

Or am I missing something else?

thanks,

greg k-h

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

[Linus Torvalds]

On Mon, May 2, 2011 at 4:28 PM, Greg KH <gregkh@suse.de> wrote:
>
> A driver that does not rely on the firmware being built in would be
> correct in calling request_firmware_nowait() on resume, then when
> userspace is properly woken up, the firmware would be sent to the
> device, then the driver would be notified, load it, and handle things
> as part of its resume sequence from that notification.
>
> Isn't that ideally what we want to have happen?

No. Absolutely not.

What we ideally want to happen is for the driver to not be a stupid
piece of sh*t.

A driver that needs firmware loading at resume time IS A BROKEN DRIVER.

It really is that simple.

> Or am I missing something else?

The only correct thing to do is for a driver that is active over a
suspend to cache the firmware in RAM.

There is absolutely no other possible solution. You can't rely on
user-space loading, because the user space may well require that
driver to load things in the first place. And even if it doesn't, it
leaves a huge gaping window where the driver is useless because it has
no firmware, and then any other (unrelated to firmware loading or udev
or anythign else) user space that wants to access the device - because
it was accessing it before the suspend - will be hosed.

Seriously. Anything else is pure idiocy. The only sane model is for
drivers to load the firmware before the suspend even happens.
Preferably by simply never unloading the firmware in the first place.

No amount of crazy hacks will ever solve the problem if a driver
doesn't do that.  Not "nowait", not "reload it from udev when user
space comes back".

                        Linus

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

[Greg KH]

On Mon, May 02, 2011 at 05:59:22PM -0700, Linus Torvalds wrote:
> On Mon, May 2, 2011 at 4:28 PM, Greg KH <gregkh@suse.de> wrote:
> >
> > A driver that does not rely on the firmware being built in would be
> > correct in calling request_firmware_nowait() on resume, then when
> > userspace is properly woken up, the firmware would be sent to the
> > device, then the driver would be notified, load it, and handle things
> > as part of its resume sequence from that notification.
> >
> > Isn't that ideally what we want to have happen?
> 
> No. Absolutely not.
> 
> What we ideally want to happen is for the driver to not be a stupid
> piece of sh*t.

Well, we all want that :)

> A driver that needs firmware loading at resume time IS A BROKEN DRIVER.
> 
> It really is that simple.

Ok, fair enough, then I have no objection to this patch.  It will let us
catch those shitty drivers and fix them up to not do this.

thanks,

greg k-h

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

[Valdis.Kletnieks]

[-- Attachment #1: Type: text/plain, Size: 1079 bytes --]

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?

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);
		return -EBUSY;
	}

(or whatever that %s actually needs to work)

All the same, it still looks better than what we're doing now.


[-- Attachment #2: Type: application/pgp-signature, Size: 227 bytes --]

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

[Rafael J. Wysocki]

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",

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

[Greg KH]

On Tue, May 03, 2011 at 01:02:08PM +0200, Rafael J. Wysocki wrote:
> 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>

I'm guessing you will take this through your tree?  If so, that's great:

Acked-by: Greg Kroah-Hartman <gregkh@suse.de>

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

[Rafael J. Wysocki]

On Tuesday, May 03, 2011, Greg KH wrote:
> On Tue, May 03, 2011 at 01:02:08PM +0200, Rafael J. Wysocki wrote:
> > 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>
> 
> I'm guessing you will take this through your tree?

Yes, I will.

> If so, that's great:
> 
> Acked-by: Greg Kroah-Hartman <gregkh@suse.de>

Thanks a lot!

Rafael

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

[Valdis.Kletnieks]

[-- Attachment #1: Type: text/plain, Size: 1020 bytes --]

On Tue, 03 May 2011 13:02:08 +0200, "Rafael J. Wysocki" said:

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

Good point. I obviously didn't have enough caffeine to think it through. ;)

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

That works too.  :)

> OK, updated patch is appended.

Looks good to me.


[-- Attachment #2: Type: application/pgp-signature, Size: 227 bytes --]

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

[Joe Perches]

On Tue, 2011-05-03 at 11:47 -0400, Valdis.Kletnieks@vt.edu wrote:
> On Tue, 03 May 2011 13:02:08 +0200, "Rafael J. Wysocki" said:
> > It's useful, but I'd rather do
> >  		dev_info(device, "firmware: %s will not be loaded\n", name);
> That works too.  :)

A higher KERN_<LEVEL> like dev_alert or dev_err might be better.

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

[Rafael J. Wysocki]

On Tuesday, May 03, 2011, Joe Perches wrote:
> On Tue, 2011-05-03 at 11:47 -0400, Valdis.Kletnieks@vt.edu wrote:
> > On Tue, 03 May 2011 13:02:08 +0200, "Rafael J. Wysocki" said:
> > > It's useful, but I'd rather do
> > >  		dev_info(device, "firmware: %s will not be loaded\n", name);
> > That works too.  :)
> 
> A higher KERN_<LEVEL> like dev_alert or dev_err might be better.

Hmm, fair enough.  I'll use dev_err() in the final version.

Thanks,
Rafael

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

[Valdis.Kletnieks]

[-- Attachment #1: Type: text/plain, Size: 508 bytes --]

On Tue, 03 May 2011 19:14:18 +0200, you said:
> On Tuesday, May 03, 2011, Valdis.Kletnieks@vt.edu wrote:
> > > It's useful, but I'd rather do
> > >
> > >  		dev_info(device, "firmware: %s will not be loaded\n", name);
> >
> > That works too.  :)
> >
> > > OK, updated patch is appended.
> >
> > Looks good to me.
>
> May I take that as an ACK?

Feel free to stick either an Acked-By: or Reviewed-By: Valdis Kletnieks <valdis.kletnieks@vt.edu>
on the version with dev_err.  I'm not sure which one applies.




[-- Attachment #2: Type: application/pgp-signature, Size: 227 bytes --]