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

[Srivatsa S. Bhat]

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@xxxxxxx> wrote:
>>> On Sunday, March 11, 2012, Kay Sievers wrote:
>>>> On Sat, Mar 10, 2012 at 00:36, Greg KH <gregkh@xxxxxxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxxxxx>
Subject: PM/firmware loader: Use better name for usermodehelper_is_disabled()

Rafael J. Wysocki <rjw@xxxxxxx> 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@xxxxxxxxxxxxxxxxxx>
---

 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()
+
 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. Also, it can cause races with
+	 * the freezer, leading to freezing failures. 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.
+	 */
+	if (WARN_ON(suspend_in_progress())) {
 		dev_err(device, "firmware: %s will not be loaded\n", name);
 		retval = -EBUSY;
 		goto out;


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/