Re: [PATCH] firmware loader: don't cancel _nowait requests when helper is not yet available

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

On Friday, March 16, 2012, Srivatsa S. Bhat wrote:
> On 03/15/2012 04:24 AM, Rafael J. Wysocki wrote:
> 
> > 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.
> 
> 
> On second thoughts... I agree, a comment is good enough.
> 
> > 
> >> +
> >>  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.
> > 
> 
> 
> Oh, yes! That would be really dreadful!
> 
> >> 	 * 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.
> >
> 
> 
> OK
>  
> 
> >> +	 */
> >> +	if (WARN_ON(suspend_in_progress())) {
> >>  		dev_err(device, "firmware: %s will not be loaded\n", name);
> >>  		retval = -EBUSY;
> >>  		goto out;
> > 
> 
> 
> So here is the updated patch:
> (I know its a bit verbose, but given that it is causing a considerable amount of
> confusion, may be a proper comment with good explanation is worthwhile).
> 
> ---
> 
> From: Srivatsa S. Bhat <srivatsa.bhat@linux.vnet.ibm.com>
> Subject: PM/firmware loader: Explain why usermodehelper_is_disabled() check is used
> 
> 
> 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 instead of renaming usermodehelper_is_disabled(), add a comment
> explaining its importance and also why the warning and bail out at
> _request_firmware() makes sense.
> 
> Signed-off-by: Srivatsa S. Bhat <srivatsa.bhat@linux.vnet.ibm.com>

That's fine by me.

If no one objects, I'll apply it.

Thanks,
Rafael


> ---
> 
>  drivers/base/firmware_class.c |   16 ++++++++++++++++
>  1 files changed, 16 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
> index 6c9387d..9199e3e 100644
> --- a/drivers/base/firmware_class.c
> +++ b/drivers/base/firmware_class.c
> @@ -535,6 +535,22 @@ static int _request_firmware(const struct firmware **firmware_p,
>  
>  	read_lock_usermodehelper();
>  
> +	/*
> +	 * It is wrong to request firmware when the system is suspended,
> +	 * because it simply won't work. Also, it can cause races with
> +	 * the freezer, leading to freezing failures. Worse than that,
> +	 * it may even cause a user space process to run when we think
> +	 * we have frozen the user space! - and that can lead to all kinds
> +	 * of interesting breakage..
> +	 *
> +	 * So check if the system is in a state which is unsuitable for
> +	 * requesting firmware (because it is suspended or not yet fully
> +	 * resumed) and bail out early if needed.
> +	 * Usermodehelpers are disabled at the beginning of suspend, before
> +	 * freezing tasks and re-enabled only towards the end of resume, after
> +	 * thawing tasks, when it is safe. So all we need to do here is ensure
> +	 * that we don't request firmware when usermodehelpers are disabled.
> +	 */
>  	if (WARN_ON(usermodehelper_is_disabled())) {
>  		dev_err(device, "firmware: %s will not be loaded\n", name);
>  		retval = -EBUSY;
> 
> 
> 
>