| Message ID | 201105030044.51661.rjw@sisk.pl (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
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
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
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
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.
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
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
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",