Messages in this thread | | | From | "Rafael J. Wysocki" <> | Subject | Re: [PATCH] firmware loader: don't cancel _nowait requests when helper is not yet available | Date | Wed, 14 Mar 2012 23:54:04 +0100 |
| |
On Wednesday, March 14, 2012, Srivatsa S. Bhat wrote: > On 03/14/2012 05:40 AM, Rafael J. Wysocki wrote: > > > On Wednesday, March 14, 2012, Kay Sievers wrote: > >> On Tue, Mar 13, 2012 at 20:42, Rafael J. Wysocki <rjw@sisk.pl> wrote: > >>> On Sunday, March 11, 2012, Kay Sievers wrote: > >>>> On Sat, Mar 10, 2012 at 00:36, Greg KH <gregkh@linuxfoundation.org> wrote: > >> > >>>>> What does uevent have to do with things here? > >>>> > >>>> I don't think that the firmware loader should care about the > >>>> usermodehelper at all, and that stuff fiddling should just be removed > >>>> from the firmware class. > >>> > >>> It's there to warn people that their drivers do stupid things like > >>> loading frimware during system resume, which is guaranteed not to work. > >>> > >>> IOW, it's there very much on purpose. > >> > >> Using the /sbin/hotplug is no case that needs any warning. It' such a > >> broken model these days, that firmware loading is the least problem > >> that occurs with it. > >> > >>>> Forking /sbin/hotplug is disabled by default, it is a broken concept, > >>>> and it cannot work reliably on today's systems. > >>>> > >>>> Firmware is not loaded by /sbin/hotplug since many years, but by udev > >>>> or whatever service handles uevents, like ueventd on android. > >>> > >>> Which I'm not sure why is relevant here. > >> > >> It is relevant in the sense that the firmware loader should not even > >> know that a uevent *can* cause a usermodehelper exec() if it runs in > >> legacy mode. The firmware loader just has no business in fiddling with > >> the details of driver core legacy stuff. I don't think his warning > >> makes much sense. > > > > But that warning actually triggers for drivers that attempt to use > > request_firmware() during system resume, even though /sbin/hotplug isn't > > used any more. > > > > > I agree with Rafael about why the warning and the bail out is required, > including the part about the races with freezer which he explained in his > other mail. These problems have already been well documented too. > (See Documentation/power/freezing-of-tasks.txt). > > > usermodehelper_is_disabled() means "we are in the middle of system power > > transition" rather than anything else (I agree it should be called > > suspend_in_progress() or something similar these days). > > > > > How about this patch then? > > --- > > From: Srivatsa S. Bhat <srivatsa.bhat@linux.vnet.ibm.com> > Subject: PM/firmware loader: Use better name for usermodehelper_is_disabled() > > Rafael J. Wysocki <rjw@sisk.pl> wrote: > > | usermodehelper_is_disabled() means "we are in the middle of system power > | transition" rather than anything else (I agree it should be called > | suspend_in_progress() or something similar these days). > > > But simply renaming usermodehelper_is_disabled() to suspend_in_progress() > isn't the best thing to do since that would be misleading because suspend > transitions are begun much before usermodehelpers are disabled. > > Apart from that, we don't want people to suddenly start abusing this function > in future in a totally different context to check if suspend is in progress. > > So, add an alias specific to firmware loaders alone, that will internally > call usermodehelpers_is_disabled(). > > Signed-off-by: Srivatsa S. Bhat <srivatsa.bhat@linux.vnet.ibm.com> > --- > > drivers/base/firmware_class.c | 12 +++++++++++- > 1 files changed, 11 insertions(+), 1 deletions(-) > > diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c > index 6c9387d..9e401e1 100644 > --- a/drivers/base/firmware_class.c > +++ b/drivers/base/firmware_class.c > @@ -510,6 +510,8 @@ static void fw_destroy_instance(struct firmware_priv *fw_priv) > device_unregister(f_dev); > } > > +#define suspend_in_progress() usermodehelper_is_disabled()
This looks like an overstretch to me. I think a comment would be sufficient.
> + > static int _request_firmware(const struct firmware **firmware_p, > const char *name, struct device *device, > bool uevent, bool nowait) > @@ -535,7 +537,15 @@ static int _request_firmware(const struct firmware **firmware_p, > > read_lock_usermodehelper(); > > - if (WARN_ON(usermodehelper_is_disabled())) { > + /* > + * It is wrong to request firmware when the system is suspended, > + * because it simply won't work reliably.
In fact, it won't work at all.
> + Also, it can cause races with > + * the freezer, leading to freezing failures.
It actually is worse than that too. It may cause a user space process to run when we think we have frozen user space and _that_ may lead to all kinds of interesting breakage.
> * So check if the system is > + * in a state which is unsuitable for requesting firmware (because the > + * system is suspended or not yet fully resumed) and bail out early if > + * needed.
And here I'd explain why usermodehelper_is_disabled() is used for that.
> + */ > + if (WARN_ON(suspend_in_progress())) { > dev_err(device, "firmware: %s will not be loaded\n", name); > retval = -EBUSY; > goto out;
Thanks, Rafael
| |