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

["Srivatsa S. Bhat" <srivatsa.bhat@linux.vnet.ibm.com>]

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

 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;