firmware loader: retry _nowait requests when userhelper is not yet available

During resume, the userhelper might not be available. However for drivers which use the request_firmware_nowait interface, this will only lead to a pointless WARNING and a device which no longer works after the resume [since it couldn't get the firmware, because the userhelper was not available to take the request].

Quoted 5 lines · On Sat, 3 Mar 2012 21:22:36 +0100 Christian Lamparter…

During resume, the userhelper might not be available. However for drivers which use the request_firmware_nowait interface, this will only lead to a pointless WARNING and a device which no longer works after the resume [since it couldn't get the firmware, because the userhelper was not available to take the request].

Can you not load the firmware on the suspend path ?

	if (skt->state & SOCKET_CARDBUS) {
		/* We can't be sure the CardBus card is the same
		 * as the one previously inserted. Therefore, remove
		 * and re-add... */
		cb_free(skt);
		cb_alloc(skt);
		return 0;
	}

During resume, the userhelper might not be available. However for drivers which use the request_firmware_nowait interface, this will only lead to a pointless WARNING and a device which no longer works after the resume [since it couldn't get the firmware, because the userhelper was not available to take the request].

In order to solve this "chicken or egg" dilemma, the code now retries _nowait requests at one second intervals until the "loading_timeout" time is up.

---
I'm aware about the previous "request_firmware* in probe" discussions. Unfortunately, the hardware needs firmware so there is no other way around it. So please, I just wanted to know what the general opinion about the idea behind this patch is.

Regards,
Christian

BTW: I'm not on the list, so please keep the 'CC'.

---
 drivers/base/firmware_class.c |   24 +++++++++++++++++++++++-
 1 files changed, 23 insertions(+), 1 deletions(-)

drivers/base/firmware_class.c +23 -1

index 6c9387d..9f70096 100644
@@ -19,6 +19,7 @@
 #include <linux/kthread.h>
 #include <linux/highmem.h>
 #include <linux/firmware.h>
+#include <linux/delay.h>
 #include <linux/slab.h>
 
 #define to_dev(obj) container_of(obj, struct device, kobj)

@@ -535,6 +536,11 @@ static int _request_firmware(const struct firmware **firmware_p,
 
 	read_lock_usermodehelper();
 
+	if (nowait && usermodehelper_is_disabled()) {
+		retval = -EBUSY;
+		goto out;
+	}
+
 	if (WARN_ON(usermodehelper_is_disabled())) {
 		dev_err(device, "firmware: %s will not be loaded\n", name);
 		retval = -EBUSY;

@@ -633,7 +639,7 @@ static int request_firmware_work_func(void *arg)
 {
 	struct firmware_work *fw_work = arg;
 	const struct firmware *fw;
-	int ret;
+	int ret, timeout = loading_timeout;
 
 	if (!arg) {
 		WARN_ON(1);

@@ -642,6 +648,22 @@ static int request_firmware_work_func(void *arg)
 
 	ret = _request_firmware(&fw, fw_work->name, fw_work->device,
 				fw_work->uevent, true);
+
+	while (ret == -EBUSY) {
+		/*
+		 * Try to retrieve the firmware within the loading timeout.
+		 * To stick with the loading timeout convention from above:
+		 *	loading_timeout = 0 means 'try forever' as well.
+		 */
+
+		msleep(1000);
+		ret = _request_firmware(&fw, fw_work->name, fw_work->device,
+					fw_work->uevent, true);
+
+		if (timeout != 0 && timeout-- == 1)
 
+			break;
 
+	};
 
+

drivers/base/firmware_class.c +0 -0

 	fw_work->cont(fw, fw_work->context);
 
 	module_put(fw_work->module);

This patch fixes a regression which was introduced by:
"PM: Print a warning if firmware is requested when tasks are frozen"

request_firmware_nowait does not stall in any system resume paths. Therefore, I think it is perfectly save to use request_firmware_nowait from at least the ->complete() callback.

Signed-off-by Christian Lamparter <[email protected]>

---
 drivers/base/firmware_class.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

drivers/base/firmware_class.c +0 -0

index 6c9387d..017e020 100644

Quoted 6 lines · Christian Lamparter

This patch fixes a regression which was introduced by:
"PM: Print a warning if firmware is requested when tasks are frozen"

request_firmware_nowait does not stall in any system resume paths. Therefore, I think it is perfectly save to use request_firmware_nowait from at least the ->complete() callback.

Is there code somewhere in the kernel that wants to do this?

Has commit a144c6a broken it somehow that this fix would resolve it?

Quoted 11 lines

Signed-off-by Christian Lamparter <[email protected]>

---
 drivers/base/firmware_class.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

drivers/base/firmware_class.c +0 -0

index 6c9387d..017e020 100644
@@ -535,7 +535,7 @@ static int _request_firmware(const struct firmware **firmware_p,
 

drivers/base/firmware_class.c static int _request_firmware(const struct firmware **firmware_p, · line 538

Quoted 4 lines

drivers/base/firmware_class.c +1 -1

 	read_lock_usermodehelper();
 
-	if (WARN_ON(usermodehelper_is_disabled())) {
+	if (WARN_ON(usermodehelper_is_disabled() && !(nowait && uevent))) {

What does uevent have to do with things here?

Quoted 6 lines · Christian Lamparter

This patch fixes a regression which was introduced by:
"PM: Print a warning if firmware is requested when tasks are frozen"

request_firmware_nowait does not stall in any system resume paths. Therefore, I think it is perfectly save to use request_firmware_nowait from at least the ->complete() callback.

Is there code somewhere in the kernel that wants to do this?  Has commit a144c6a broken it somehow that this fix would resolve it?

Quoted 12 lines

Signed-off-by Christian Lamparter <[email protected]>

---
 drivers/base/firmware_class.c |    2 +-

 1 files changed, 1 insertions(+), 1 deletions(-)

drivers/base/firmware_class.c +0 -0

index 6c9387d..017e020 100644
@@ -535,7 +535,7 @@ static int _request_firmware(const struct firmware **firmware_p,
 

drivers/base/firmware_class.c static int _request_firmware(const struct firmware **firmware_p, · line 538

Quoted 4 lines

drivers/base/firmware_class.c +1 -1

       read_lock_usermodehelper();
 
-     if (WARN_ON(usermodehelper_is_disabled())) {
+     if (WARN_ON(usermodehelper_is_disabled() && !(nowait && uevent))) {

What does uevent have to do with things here?

Quoted 23 lines · Greg KH

Quoted 6 lines · Christian Lamparter

This patch fixes a regression which was introduced by:
"PM: Print a warning if firmware is requested when tasks are frozen"

request_firmware_nowait does not stall in any system resume paths. Therefore, I think it is perfectly save to use request_firmware_nowait from at least the ->complete() callback.

Is there code somewhere in the kernel that wants to do this? Has commit a144c6a broken it somehow that this fix would resolve it?

Quoted 12 lines

Signed-off-by Christian Lamparter<[email protected]>

---
  drivers/base/firmware_class.c |    2 +-
  1 files changed, 1 insertions(+), 1 deletions(-)

drivers/base/firmware_class.c +0 -0

index 6c9387d..017e020 100644
@@ -535,7 +535,7 @@ static int _request_firmware(const struct firmware **firmware_p,
 

Quoted 6 lines

drivers/base/firmware_class.c static int _request_firmware(const struct firmware **firmware_p, · line 538

Quoted 4 lines

drivers/base/firmware_class.c +1 -1

       read_lock_usermodehelper();
 
-     if (WARN_ON(usermodehelper_is_disabled())) {
+     if (WARN_ON(usermodehelper_is_disabled()&&  !(nowait&&  uevent))) {

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.

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.

Quoted 23 lines · Greg KH

Quoted 6 lines · Christian Lamparter

This patch fixes a regression which was introduced by:
"PM: Print a warning if firmware is requested when tasks are frozen"

request_firmware_nowait does not stall in any system resume paths. Therefore, I think it is perfectly save to use request_firmware_nowait from at least the ->complete() callback.

Is there code somewhere in the kernel that wants to do this? Has commit a144c6a broken it somehow that this fix would resolve it?

Quoted 12 lines

Signed-off-by Christian Lamparter<[email protected]>

---
  drivers/base/firmware_class.c |    2 +-
  1 files changed, 1 insertions(+), 1 deletions(-)

drivers/base/firmware_class.c +0 -0

index 6c9387d..017e020 100644
@@ -535,7 +535,7 @@ static int _request_firmware(const struct firmware **firmware_p,
 

Quoted 6 lines

drivers/base/firmware_class.c static int _request_firmware(const struct firmware **firmware_p, · line 538

Quoted 4 lines

drivers/base/firmware_class.c +1 -1

       read_lock_usermodehelper();
 
-     if (WARN_ON(usermodehelper_is_disabled())) {
+     if (WARN_ON(usermodehelper_is_disabled()&&  !(nowait&&  uevent))) {

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.

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.

Quoted 35 lines · Kay Sievers

Quoted 23 lines · Greg KH

Quoted 6 lines · Christian Lamparter

This patch fixes a regression which was introduced by:
"PM: Print a warning if firmware is requested when tasks are frozen"

request_firmware_nowait does not stall in any system resume paths. Therefore, I think it is perfectly save to use request_firmware_nowait from at least the ->complete() callback.

Is there code somewhere in the kernel that wants to do this? Has commit a144c6a broken it somehow that this fix would resolve it?

Quoted 12 lines

Signed-off-by Christian Lamparter<[email protected]>

---
  drivers/base/firmware_class.c |    2 +-
  1 files changed, 1 insertions(+), 1 deletions(-)

drivers/base/firmware_class.c +0 -0

index 6c9387d..017e020 100644
@@ -535,7 +535,7 @@ static int _request_firmware(const struct firmware **firmware_p,
 

Quoted 6 lines

drivers/base/firmware_class.c static int _request_firmware(const struct firmware **firmware_p,

Quoted 4 lines

drivers/base/firmware_class.c +1 -1

       read_lock_usermodehelper();
 
-     if (WARN_ON(usermodehelper_is_disabled())) {
+     if (WARN_ON(usermodehelper_is_disabled()&&  !(nowait&&  uevent))) {

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.

Quoted 6 lines

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.

Resending again after fixing my stupid email formatting.

We (mach-msm) just happened to be looking at similar issues with request_firmware. The recent changes to request_firmware to check for usermodehelper_is_disabled() was preventing us from using request_firmware() in what I think is a valid use case. I will get to that later.

To first suggest a solution specific the problem this patch is trying to address, I think it would be better to do something like below. It's just a quick RFC to show what I mean -- haven't even compiled it. If there is an agreement on this suggestion, I can send out a cleaner patch.

firmware class: Check for usermode helper availability only
when enabled.

Signed-off-by Saravana Kannan <[email protected]>

---
  drivers/base/firmware_class.c |   15 ++++++++-------
  kernel/kmod.c                 |    2 +-
  2 files changed, 9 insertions(+), 8 deletions(-)

drivers/base/firmware_class.c +8 -7

index 06ed6b4..2a45bf7 100644
@@ -534,12 +534,6 @@ static int _request_firmware(const struct firmware 
 **firmware_p,
                 return 0;
         }
 
-       if (WARN_ON(usermodehelper_is_disabled())) {
-               dev_err(device, "firmware: %s will not be loaded\n", name);
-               retval = -EBUSY;
-               goto out;
-       }
-
         if (uevent)
                 dev_dbg(device, "firmware: requesting %s\n", name);

@@ -555,12 +549,19 @@ static int _request_firmware(const struct firmware 
 **firmware_p,
                                   round_jiffies_up(jiffies +
                                                    loading_timeout * HZ));
 
-               kobject_uevent(&fw_priv->dev.kobj, KOBJ_ADD);
+               retval = kobject_uevent(&fw_priv->dev.kobj, KOBJ_ADD);
+               if (retval) {
+                       dev_err(device, "firmware: %s will not be 
 loaded\n", name);
+                       set_bit(FW_STATUS_ABORT, &fw_priv->status);
+                       goto abort;
+               }
         }
 
         wait_for_completion(&fw_priv->completion);
 
         set_bit(FW_STATUS_DONE, &fw_priv->status);
+
+abort:
         del_timer_sync(&fw_priv->timeout);

         mutex_lock(&fw_lock);

kernel/kmod.c +1 -1

index 47613df..e733afe3 100644
@@ -422,7 +422,7 @@ int call_usermodehelper_exec(struct subprocess_info 
 *sub_info,
         if (sub_info->path[0] == '\0')
                 goto out;
 
-       if (!khelper_wq || usermodehelper_disabled) {
+       if (!khelper_wq || (uevent_helper[0] && usermodehelper_disabled)) {
                 retval = -EBUSY;
                 goto out;

         }

Now, getting to the issue we are facing -- the recent checks for usermode helper in request_firmare() is failing request_firmware() in a kthread that also activates a wake up source (or if you are familiar with Android terms -- grabs a wake lock). By activating a wakeup source, the kthread is properly indicating that a suspend shouldn't happen. So, I think it's a valid use case for request_firmware().

With the current checks, that doesn't seem to be sufficient since a kthread can coincidentally be running in parallel to the suspend sequence. The suspend sequence sets "usermodehelper_disabled" for the purpose of causing request_firmware() to fail immediately when called from the suspend ops.

But that doesn't take into account that the kthread could also be running at the same time.

If this check wasn't there, the suspend would be aborted (since the kthread has activated the wakeup source) and the request_firmware() would have succeeded.

I think the usermodehelper check in the request_firmware() flow is denying a wider swath of scenarios than it needs to. I think the real check should be to only disallow request_firmware() in all of the callbacks that are called from suspend_enter().

I was joking to my colleague (Stephen Boyd) about just walking up the stack to see if suspend_enter() is in the stack, but he seems to have ideas that would have a similar effect on the functionality without being insane code. I will leave it to him to present his ideas.

But while we are trying to figure out ways to immediately error out request_firmware() from suspend callbacks, I think we should remove the usermodehelper check in request_firmware() since it's actually preventing legitimate use cases.

All of those use cases are in fact of the "wait for user space to be thawed and then load the firmware" type, which I believe may be handled without changing that code.

Why don't you make your kthread freezable, for one example?

Why don't you use a freezable workqueue instead?

Quoted 4 lines · Rafael J. Wysocki

All of those use cases are in fact of the "wait for user space to be thawed and then load the firmware" type, which I believe may be handled without changing that code.

Quoted 4 lines

Why don't you make your kthread freezable, for one example?

Why don't you use a freezable workqueue instead?

If we put it on the freezable workqueue or make it a freezable thread will it work?

In my scenario a wakeup interrupt comes in that wakes us up from suspend. Within that wakeup handler a work item is scheduled to the freezable workqueue. That work item then calls request_firmware().

It looks like we call schedule() after thawing the workqueues and tasks so the work item could run before usermodehelpers are enabled and then request_firmware() would fail. Do we need something like this (ignore the fact that we call usermodhelper_enable() twice)?

kernel/power/process.c +0 -0

index 7e42645..61bfa95 100644

kernel/power/process.c +0 -0

        schedule();
        printk("done.\n");
 }

Is there a reason we disable usermodehelpers if
CONFIG_SUSPEND_FREEZER=n?

Should we do this instead so that
usermodehelpers are only disabled if we freeze userspace? Also what is that schedule() call in thaw_kernel_threads() for? It looks like we'll call schedule between kernel thread thawing and userspace thawing.

I pushed out the schedule() call to the callers so that we don't call schedule() until userspace is thawed.

Quoted 5 lines · Stephen Boyd

Quoted 4 lines · Rafael J. Wysocki

All of those use cases are in fact of the "wait for user space to be thawed and then load the firmware" type, which I believe may be handled without changing that code.

Quoted 7 lines

Quoted 4 lines

Why don't you make your kthread freezable, for one example?

Why don't you use a freezable workqueue instead?

If we put it on the freezable workqueue or make it a freezable thread will it work?

That depends on what exactly you want to achieve, which isn't entirely clear to me at this point.

Quoted 3 lines

In my scenario a wakeup interrupt comes in that wakes us up from suspend. Within that wakeup handler a work item is scheduled to the freezable workqueue. That work item then calls request_firmware().

That should work.

Quoted 10 lines

It looks like we call schedule() after thawing the workqueues and tasks so the work item could run before usermodehelpers are enabled and then request_firmware() would fail. Do we need something like this (ignore the fact that we call usermodhelper_enable() twice)?

kernel/power/process.c +0 -0

index 7e42645..61bfa95 100644

kernel/power/process.c void thaw_processes(void) · line 190

Quoted 4 lines

kernel/power/process.c +1 -0

@@ -187,6 +187,7 @@ void thaw_processes(void)
        } while_each_thread(g, p);
        read_unlock(&tasklist_lock);
 
+       usermodehelper_enable();

That would be a reasonable change.

Quoted 7 lines

kernel/power/process.c +0 -0

        schedule();
        printk("done.\n");
 }

Is there a reason we disable usermodehelpers if
CONFIG_SUSPEND_FREEZER=n?

Not really, but CONFIG_SUSPEND_FREEZER=n can only work reliably in a very limited set of cases, so I don't think it's even worth making the general code depend on it.

I'd actually prefer to remove CONFIG_SUSPEND_FREEZER altogether, because it's not very useful nowadays (probably isn't useful at all).

Quoted 4 lines

Should we do this instead so that
usermodehelpers are only disabled if we freeze userspace? Also what is that schedule() call in thaw_kernel_threads() for? It looks like we'll call schedule between kernel thread thawing and userspace thawing.

Which is OK, I think.

Quoted 54 lines · Rafael J. Wysocki

Quoted 11 lines · Stephen Boyd

Quoted 8 lines · Rafael J. Wysocki

All of those use cases are in fact of the "wait for user space to be thawed and then load the firmware" type, which I believe may be handled without changing that code.

Why don't you make your kthread freezable, for one example?

Why don't you use a freezable workqueue instead?

If we put it on the freezable workqueue or make it a freezable thread will it work?

That depends on what exactly you want to achieve, which isn't entirely clear to me at this point.

Quoted 3 lines

In my scenario a wakeup interrupt comes in that wakes us up from suspend. Within that wakeup handler a work item is scheduled to the freezable workqueue. That work item then calls request_firmware().

That should work.

Quoted 10 lines

It looks like we call schedule() after thawing the workqueues and tasks so the work item could run before usermodehelpers are enabled and then request_firmware() would fail. Do we need something like this (ignore the fact that we call usermodhelper_enable() twice)?

kernel/power/process.c +0 -0

index 7e42645..61bfa95 100644

kernel/power/process.c void thaw_processes(void) · line 190

Quoted 4 lines

kernel/power/process.c +1 -0

@@ -187,6 +187,7 @@ void thaw_processes(void)
        } while_each_thread(g, p);
        read_unlock(&tasklist_lock);
 
+       usermodehelper_enable();

That would be a reasonable change.

Quoted 7 lines

kernel/power/process.c +0 -0

        schedule();
        printk("done.\n");
 }

Is there a reason we disable usermodehelpers if
CONFIG_SUSPEND_FREEZER=n?

Not really, but CONFIG_SUSPEND_FREEZER=n can only work reliably in a very limited set of cases, so I don't think it's even worth making the general code depend on it.

I'd actually prefer to remove CONFIG_SUSPEND_FREEZER altogether, because it's not very useful nowadays (probably isn't useful at all).

Quoted 4 lines

Should we do this instead so that
usermodehelpers are only disabled if we freeze userspace? Also what is that schedule() call in thaw_kernel_threads() for? It looks like we'll call schedule between kernel thread thawing and userspace thawing.

Quoted 1 line

Which is OK, I think.

Moreover, thaw_kernel_threads() is _only_ called by (a) freeze_kernel_threads() on error and (b) user-space hibernate interface in kernel/power/user.c (and please read the comment in there describing what it's there for, which also explains why the schedule() call in there is necessary).

Quoted 55 lines · Rafael J. Wysocki

Quoted 54 lines · Rafael J. Wysocki

Quoted 11 lines · Stephen Boyd

Quoted 8 lines · Rafael J. Wysocki

All of those use cases are in fact of the "wait for user space to be thawed and then load the firmware" type, which I believe may be handled without changing that code.

Why don't you make your kthread freezable, for one example?

Why don't you use a freezable workqueue instead?

If we put it on the freezable workqueue or make it a freezable thread will it work?

That depends on what exactly you want to achieve, which isn't entirely clear to me at this point.

Quoted 3 lines

In my scenario a wakeup interrupt comes in that wakes us up from suspend. Within that wakeup handler a work item is scheduled to the freezable workqueue. That work item then calls request_firmware().

That should work.

Quoted 10 lines

It looks like we call schedule() after thawing the workqueues and tasks so the work item could run before usermodehelpers are enabled and then request_firmware() would fail. Do we need something like this (ignore the fact that we call usermodhelper_enable() twice)?

kernel/power/process.c +0 -0

index 7e42645..61bfa95 100644

kernel/power/process.c void thaw_processes(void) · line 190

Quoted 4 lines

kernel/power/process.c +1 -0

@@ -187,6 +187,7 @@ void thaw_processes(void)
        } while_each_thread(g, p);
        read_unlock(&tasklist_lock);
 
+       usermodehelper_enable();

That would be a reasonable change.

Quoted 7 lines

kernel/power/process.c +0 -0

        schedule();
        printk("done.\n");
 }

Is there a reason we disable usermodehelpers if
CONFIG_SUSPEND_FREEZER=n?

Not really, but CONFIG_SUSPEND_FREEZER=n can only work reliably in a very limited set of cases, so I don't think it's even worth making the general code depend on it.

I'd actually prefer to remove CONFIG_SUSPEND_FREEZER altogether, because it's not very useful nowadays (probably isn't useful at all).

Quoted 4 lines

Should we do this instead so that
usermodehelpers are only disabled if we freeze userspace? Also what is that schedule() call in thaw_kernel_threads() for? It looks like we'll call schedule between kernel thread thawing and userspace thawing.

Quoted 5 lines

Quoted 1 line

Which is OK, I think.

Moreover, thaw_kernel_threads() is _only_ called by (a) freeze_kernel_threads() on error and (b) user-space hibernate interface in kernel/power/user.c (and please read the comment in there describing what it's there for, which also explains why the schedule() call in there is necessary).

Exactly. So in case (a) when the error occurs we'll have this call flow:

usermodehelpers_disable()
suspend_freeze_processes()
freeze_processes()
freeze_kernel_threads()
try_to_freeze_tasks() <-- returns error
thaw_kernel_threads()
schedule()
thaw_processes()
usermodehelpers_enable()

Shouldn't we schedule only after we thaw all processes (not just tasks)? Otherwise we may run a kernel thread before userspace is thawed?

Quoted 20 lines · Stephen Boyd

Quoted 7 lines · Rafael J. Wysocki

Quoted 1 line · Rafael J. Wysocki

Which is OK, I think.

Moreover, thaw_kernel_threads() is _only_ called by (a) freeze_kernel_threads() on error and (b) user-space hibernate interface in kernel/power/user.c (and please read the comment in there describing what it's there for, which also explains why the schedule() call in there is necessary).

Exactly. So in case (a) when the error occurs we'll have this call flow:

usermodehelpers_disable()
suspend_freeze_processes()
freeze_processes()
freeze_kernel_threads()
try_to_freeze_tasks() <-- returns error
thaw_kernel_threads()
schedule()
thaw_processes()
usermodehelpers_enable()

Quoted 2 lines

Shouldn't we schedule only after we thaw all processes (not just tasks)? Otherwise we may run a kernel thread before userspace is thawed?

Yes, we may, but that isn't wrong, is it?

Only a few kernel threads are freezable, so definitely kernel threads can run while user space is frozen.

Quoted 21 lines · Rafael J. Wysocki

Quoted 20 lines · Stephen Boyd

Quoted 7 lines · Rafael J. Wysocki

Quoted 1 line · Rafael J. Wysocki

Which is OK, I think.

Moreover, thaw_kernel_threads() is _only_ called by (a) freeze_kernel_threads() on error and (b) user-space hibernate interface in kernel/power/user.c (and please read the comment in there describing what it's there for, which also explains why the schedule() call in there is necessary).

Exactly. So in case (a) when the error occurs we'll have this call flow:

usermodehelpers_disable()
suspend_freeze_processes()
freeze_processes()
freeze_kernel_threads()
try_to_freeze_tasks() <-- returns error
thaw_kernel_threads()
schedule()
thaw_processes()
usermodehelpers_enable()

Quoted 7 lines

Quoted 2 lines

Shouldn't we schedule only after we thaw all processes (not just tasks)? Otherwise we may run a kernel thread before userspace is thawed?

Yes, we may, but that isn't wrong, is it?

Only a few kernel threads are freezable, so definitely kernel threads can run while user space is frozen.

Yes but if someone calls request_firmware() from a kthread then they will hit the same problem where the thread runs and requests the firmware and usermodehelpers are still disabled. Currently my code is written with kthreads and that thread makes the request firmware call, so this doesn't seem far fetched (although in my case I can probably fix it).

It looks like before 379e0be (PM / Freezer: Thaw only kernel threads if freezing of kernel threads fails, 2012-02-03) schedule wasn't called until userspace was thawed. It looks like that patch was about hibernation and not suspend?

Oddly enough a work_struct was already part of the firmware_work structure but nobody was using it. Instead of creating a new kthread for each request_firmware_nowait() just schedule the work on the system workqueue. This should avoid some overhead in forking new threads when they're not strictly necessary if workqueues are available.

Signed-off-by Stephen Boyd <[email protected]>

Cc Greg Kroah-Hartman <[email protected]>

Cc Kay Sievers <[email protected]>

Cc Rafael J. Wysocki <[email protected]>

---

I saw this while looking at this problem we're having.

Quoted 13 lines · Stephen Boyd

Oddly enough a work_struct was already part of the firmware_work structure but nobody was using it. Instead of creating a new kthread for each request_firmware_nowait() just schedule the work on the system workqueue. This should avoid some overhead in forking new threads when they're not strictly necessary if workqueues are available.

Signed-off-by Stephen Boyd <[email protected]>

Cc Greg Kroah-Hartman <[email protected]>

Cc Kay Sievers <[email protected]>

Cc Rafael J. Wysocki <[email protected]>

---

Quoted 1 line

I saw this while looking at this problem we're having.

Correct me if I'm wrong, but wouldn't that stall all other global workqueue tasks for up to 60 seconds [in worst case]?

But I think we can get rid of the firmware_work work struct...

Quoted 14 lines · Christian Lamparter

Quoted 13 lines · Stephen Boyd

Oddly enough a work_struct was already part of the firmware_work structure but nobody was using it. Instead of creating a new kthread for each request_firmware_nowait() just schedule the work on the system workqueue. This should avoid some overhead in forking new threads when they're not strictly necessary if workqueues are available.

Signed-off-by Stephen Boyd <[email protected]>

Cc Greg Kroah-Hartman <[email protected]>

Cc Kay Sievers <[email protected]>

Cc Rafael J. Wysocki <[email protected]>

---

Quoted 6 lines

Quoted 1 line

I saw this while looking at this problem we're having.

Correct me if I'm wrong, but wouldn't that stall all other global workqueue tasks for up to 60 seconds [in worst case]?

But I think we can get rid of the firmware_work work struct...

My understanding is that with concurrency managed workqueues when the work item blocks another will be scheduled to run almost immediately. So before that change by Tejun workqueues would have been a bad idea because it could have blocked up to 60 second but now it should be fine because that work item will just be put to sleep and another request will run.

Quoted 19 lines · Stephen Boyd

Quoted 18 lines · Christian Lamparter

Quoted 7 lines · Stephen Boyd

Oddly enough a work_struct was already part of the firmware_work structure but nobody was using it. Instead of creating a new kthread for each request_firmware_nowait() just schedule the work on the system workqueue. This should avoid some overhead in forking new threads when they're not strictly necessary if workqueues are available.

Quoted 7 lines

Signed-off-by Stephen Boyd <[email protected]>

Cc Greg Kroah-Hartman <[email protected]>

Cc Kay Sievers <[email protected]>

Cc Rafael J. Wysocki <[email protected]>

---

I saw this while looking at this problem we're having.

Correct me if I'm wrong, but wouldn't that stall all other global workqueue tasks for up to 60 seconds [in worst case]?

Quoted 8 lines

Quoted 2 lines

But I think we can get rid of the firmware_work work struct...

My understanding is that with concurrency managed workqueues when the work item blocks another will be scheduled to run almost immediately. So before that change by Tejun workqueues would have been a bad idea because it could have blocked up to 60 second but now it should be fine because that work item will just be put to sleep and another request will run.

Please read the description of system_wq in workqueue.h.

You should have used either system_long_wq or system_nrt_wq (depending on what you really need).

Quoted 30 lines · Rafael J. Wysocki

Quoted 19 lines · Stephen Boyd

Quoted 18 lines · Christian Lamparter

Quoted 7 lines · Stephen Boyd

Oddly enough a work_struct was already part of the firmware_work structure but nobody was using it. Instead of creating a new kthread for each request_firmware_nowait() just schedule the work on the system workqueue. This should avoid some overhead in forking new threads when they're not strictly necessary if workqueues are available.

Quoted 7 lines

Signed-off-by Stephen Boyd <[email protected]>

Cc Greg Kroah-Hartman <[email protected]>

Cc Kay Sievers <[email protected]>

Cc Rafael J. Wysocki <[email protected]>

---

I saw this while looking at this problem we're having.

Correct me if I'm wrong, but wouldn't that stall all other global workqueue tasks for up to 60 seconds [in worst case]?

Quoted 8 lines

Quoted 2 lines

But I think we can get rid of the firmware_work work struct...

My understanding is that with concurrency managed workqueues when the work item blocks another will be scheduled to run almost immediately. So before that change by Tejun workqueues would have been a bad idea because it could have blocked up to 60 second but now it should be fine because that work item will just be put to sleep and another request will run.

Please read the description of system_wq in workqueue.h.

Quoted 4 lines

You should have used either system_long_wq or system_nrt_wq (depending on what you really need).

Thanks. I think we can use system_nrt_wq then? Or maybe even the unbounded workqueue system_unbound_wq?

Quoted 35 lines · Stephen Boyd

Quoted 34 lines · Rafael J. Wysocki

Quoted 19 lines · Stephen Boyd

Quoted 18 lines · Christian Lamparter

Quoted 7 lines · Stephen Boyd

Oddly enough a work_struct was already part of the firmware_work structure but nobody was using it. Instead of creating a new kthread for each request_firmware_nowait() just schedule the work on the system workqueue. This should avoid some overhead in forking new threads when they're not strictly necessary if workqueues are available.

Quoted 7 lines

Signed-off-by Stephen Boyd <[email protected]>

Cc Greg Kroah-Hartman <[email protected]>

Cc Kay Sievers <[email protected]>

Cc Rafael J. Wysocki <[email protected]>

---

I saw this while looking at this problem we're having.

Correct me if I'm wrong, but wouldn't that stall all other global workqueue tasks for up to 60 seconds [in worst case]?

Quoted 8 lines

Quoted 2 lines

But I think we can get rid of the firmware_work work struct...

My understanding is that with concurrency managed workqueues when the work item blocks another will be scheduled to run almost immediately. So before that change by Tejun workqueues would have been a bad idea because it could have blocked up to 60 second but now it should be fine because that work item will just be put to sleep and another request will run.

Please read the description of system_wq in workqueue.h.

You should have used either system_long_wq or system_nrt_wq (depending on what you really need).

Quoted 2 lines

Thanks. I think we can use system_nrt_wq then? Or maybe even the unbounded workqueue system_unbound_wq?

Hmm. Can you please remind me what the exact role of that work item is?

It loads the device's firmware, but I'm not sure in what situations that's supposed to happen.

Quoted 36 lines · Rafael J. Wysocki

Quoted 35 lines · Stephen Boyd

Quoted 34 lines · Rafael J. Wysocki

Quoted 19 lines · Stephen Boyd

Quoted 18 lines · Christian Lamparter

Quoted 7 lines · Stephen Boyd

Oddly enough a work_struct was already part of the firmware_work structure but nobody was using it. Instead of creating a new kthread for each request_firmware_nowait() just schedule the work on the system workqueue. This should avoid some overhead in forking new threads when they're not strictly necessary if workqueues are available.

Quoted 7 lines

Signed-off-by Stephen Boyd <[email protected]>

Cc Greg Kroah-Hartman <[email protected]>

Cc Kay Sievers <[email protected]>

Cc Rafael J. Wysocki <[email protected]>

---

I saw this while looking at this problem we're having.

Correct me if I'm wrong, but wouldn't that stall all other global workqueue tasks for up to 60 seconds [in worst case]?

Quoted 8 lines

Quoted 2 lines

But I think we can get rid of the firmware_work work struct...

My understanding is that with concurrency managed workqueues when the work item blocks another will be scheduled to run almost immediately. So before that change by Tejun workqueues would have been a bad idea because it could have blocked up to 60 second but now it should be fine because that work item will just be put to sleep and another request will run.

Please read the description of system_wq in workqueue.h.

You should have used either system_long_wq or system_nrt_wq (depending on what you really need).

Quoted 7 lines

Quoted 2 lines

Thanks. I think we can use system_nrt_wq then? Or maybe even the unbounded workqueue system_unbound_wq?

Hmm. Can you please remind me what the exact role of that work item is?

It loads the device's firmware, but I'm not sure in what situations that's supposed to happen.

request_firmware_nowait() is used by code that wants to get the firmware asynchronously. Callers pass in a callback function which is called once the firmware is retrieved. The work item will correspond to one call to request_firmware_nowait(), where the work item will handle the sysfs entry generation, uevent generation, and wait_for_completion() calls that _request_firmware() does.

The work item also executes the callback function the caller passes in which could do probably anything and could take an arbitrarily long time. It looks like some drivers even chain request_firmware_nowait() together by calling request_firmware_nowait() from the callback functions.

Quoted 52 lines · Stephen Boyd

Quoted 36 lines · Rafael J. Wysocki

Quoted 35 lines · Stephen Boyd

Quoted 34 lines · Rafael J. Wysocki

Quoted 19 lines · Stephen Boyd

Quoted 18 lines · Christian Lamparter

Quoted 7 lines · Stephen Boyd

Oddly enough a work_struct was already part of the firmware_work structure but nobody was using it. Instead of creating a new kthread for each request_firmware_nowait() just schedule the work on the system workqueue. This should avoid some overhead in forking new threads when they're not strictly necessary if workqueues are available.

Quoted 7 lines

Signed-off-by Stephen Boyd <[email protected]>

Cc Greg Kroah-Hartman <[email protected]>

Cc Kay Sievers <[email protected]>

Cc Rafael J. Wysocki <[email protected]>

---

I saw this while looking at this problem we're having.

Correct me if I'm wrong, but wouldn't that stall all other global workqueue tasks for up to 60 seconds [in worst case]?

Quoted 8 lines

Quoted 2 lines

But I think we can get rid of the firmware_work work struct...

My understanding is that with concurrency managed workqueues when the work item blocks another will be scheduled to run almost immediately. So before that change by Tejun workqueues would have been a bad idea because it could have blocked up to 60 second but now it should be fine because that work item will just be put to sleep and another request will run.

Please read the description of system_wq in workqueue.h.

You should have used either system_long_wq or system_nrt_wq (depending on what you really need).

Quoted 7 lines

Quoted 2 lines

Thanks. I think we can use system_nrt_wq then? Or maybe even the unbounded workqueue system_unbound_wq?

Hmm. Can you please remind me what the exact role of that work item is?

It loads the device's firmware, but I'm not sure in what situations that's supposed to happen.

request_firmware_nowait() is used by code that wants to get the firmware asynchronously. Callers pass in a callback function which is called once the firmware is retrieved. The work item will correspond to one call to request_firmware_nowait(), where the work item will handle the sysfs entry generation, uevent generation, and wait_for_completion() calls that _request_firmware() does.

Quoted 4 lines

The work item also executes the callback function the caller passes in which could do probably anything and could take an arbitrarily long time. It looks like some drivers even chain request_firmware_nowait() together by calling request_firmware_nowait() from the callback functions.

So it looks like an unbound workqueue would be suitable for that, but perhaps it may even be an ordered one?

Quoted 58 lines · On Friday 16 March 2012 22:45:52 Rafael J. Wysocki wrote:

Quoted 56 lines · Stephen Boyd

Quoted 36 lines · Rafael J. Wysocki

Quoted 35 lines · Stephen Boyd

Quoted 34 lines · Rafael J. Wysocki

Quoted 19 lines · Stephen Boyd

Quoted 18 lines · Christian Lamparter

Quoted 7 lines · Stephen Boyd

Oddly enough a work_struct was already part of the firmware_work structure but nobody was using it. Instead of creating a new kthread for each request_firmware_nowait() just schedule the work on the system workqueue. This should avoid some overhead in forking new threads when they're not strictly necessary if workqueues are available.

Quoted 7 lines

Signed-off-by Stephen Boyd <[email protected]>

Cc Greg Kroah-Hartman <[email protected]>

Cc Kay Sievers <[email protected]>

Cc Rafael J. Wysocki <[email protected]>

---

I saw this while looking at this problem we're having.

Correct me if I'm wrong, but wouldn't that stall all other global workqueue tasks for up to 60 seconds [in worst case]?

Quoted 8 lines

Quoted 2 lines

But I think we can get rid of the firmware_work work struct...

My understanding is that with concurrency managed workqueues when the work item blocks another will be scheduled to run almost immediately. So before that change by Tejun workqueues would have been a bad idea because it could have blocked up to 60 second but now it should be fine because that work item will just be put to sleep and another request will run.

Please read the description of system_wq in workqueue.h.

You should have used either system_long_wq or system_nrt_wq (depending on what you really need).

Quoted 7 lines

Quoted 2 lines

Thanks. I think we can use system_nrt_wq then? Or maybe even the unbounded workqueue system_unbound_wq?

Hmm. Can you please remind me what the exact role of that work item is?

It loads the device's firmware, but I'm not sure in what situations that's supposed to happen.

request_firmware_nowait() is used by code that wants to get the firmware asynchronously. Callers pass in a callback function which is called once the firmware is retrieved. The work item will correspond to one call to request_firmware_nowait(), where the work item will handle the sysfs entry generation, uevent generation, and wait_for_completion() calls that _request_firmware() does.

The work item also executes the callback function the caller passes in which could do probably anything and could take an arbitrarily long time. It looks like some drivers even chain request_firmware_nowait() together by calling request_firmware_nowait() from the callback functions.

Quoted 2 lines

So it looks like an unbound workqueue would be suitable for that, but perhaps it may even be an ordered one?

Just a note:
kernel/kmod.c currently defines a workqueue "khelper"... So since we are already "using" kmod's usermodehelper_*, then why not the mess with the workqueue as well?

Quoted 29 lines · Stephen Boyd

Quoted 28 lines · Rafael J. Wysocki

Quoted 20 lines · Stephen Boyd

Quoted 7 lines · Rafael J. Wysocki

Quoted 1 line · Rafael J. Wysocki

Which is OK, I think.

Moreover, thaw_kernel_threads() is _only_ called by (a) freeze_kernel_threads() on error and (b) user-space hibernate interface in kernel/power/user.c (and please read the comment in there describing what it's there for, which also explains why the schedule() call in there is necessary).

Exactly. So in case (a) when the error occurs we'll have this call flow:

usermodehelpers_disable()
suspend_freeze_processes()
freeze_processes()
freeze_kernel_threads()
try_to_freeze_tasks() <-- returns error
thaw_kernel_threads()
schedule()
thaw_processes()
usermodehelpers_enable()

Quoted 2 lines

Shouldn't we schedule only after we thaw all processes (not just tasks)? Otherwise we may run a kernel thread before userspace is thawed?

Yes, we may, but that isn't wrong, is it?

Only a few kernel threads are freezable, so definitely kernel threads can run while user space is frozen.

Quoted 6 lines

Yes but if someone calls request_firmware() from a kthread then they will hit the same problem where the thread runs and requests the firmware and usermodehelpers are still disabled. Currently my code is written with kthreads and that thread makes the request firmware call, so this doesn't seem far fetched (although in my case I can probably fix it).

So again, please consider using suspend/resume notifiers to synchronize your kthread with system power transitions.

-       if (WARN_ON(usermodehelper_is_disabled())) {
+       if (is_sleep_task()) {
               dev_err(device, "firmware: %s will not be loaded\n", name);

drivers/base/firmware_class.c static int _request_firmware(const struct firmware **firmware_p, · line 537

Quoted 3 lines · Stephen Boyd

drivers/base/firmware_class.c +1 -1

-       if (WARN_ON(usermodehelper_is_disabled())) {
+       if (is_sleep_task()) {
               dev_err(device, "firmware: %s will not be loaded\n", name);

Stop these idiotic games already!

It's very simple: you cannot load firmware while the system is not readt. Your moronic "let's change the test to something else" is entirely and utterly misguided and totally misses the point.

It's not about sleeping, and it's not about anything even *remotely* about that. Stop the idiocy already.

How hard is it to understand? How many times do people have to tell you?

That warning is very much valid during bootup, and that warning has been *seen* during bootup. For example, try to compile in most wireless drivers as non-modular, and that warning *has* to trigger.

Rafael, please consider everything along these *IDIOTIC* lines completely NAK'ed.

drivers/base/firmware_class.c static int _request_firmware(const struct firmware **firmware_p, · line 537

Quoted 3 lines · Stephen Boyd

drivers/base/firmware_class.c +1 -1

-       if (WARN_ON(usermodehelper_is_disabled())) {
+       if (is_sleep_task()) {
               dev_err(device, "firmware: %s will not be loaded\n", name);

Stop these idiotic games already!

It's very simple: you cannot load firmware while the system is not readt. Your moronic "let's change the test to something else" is entirely and utterly misguided and totally misses the point.

It's not about sleeping, and it's not about anything even *remotely* about that. Stop the idiocy already.

How hard is it to understand? How many times do people have to tell you?

That warning is very much valid during bootup, and that warning has been *seen* during bootup. For example, try to compile in most wireless drivers as non-modular, and that warning *has* to trigger.

Quoted 10 lines · Linus Torvalds

drivers/base/firmware_class.c static int _request_firmware(const struct firmware **firmware_p, · line 537

Quoted 3 lines · Stephen Boyd

drivers/base/firmware_class.c +1 -1

-       if (WARN_ON(usermodehelper_is_disabled())) {
+       if (is_sleep_task()) {
               dev_err(device, "firmware: %s will not be loaded\n", name);

Stop these idiotic games already!

It's very simple: you cannot load firmware while the system is not readt. Your moronic "let's change the test to something else" is entirely and utterly misguided and totally misses the point.

Quoted 8 lines

It's not about sleeping, and it's not about anything even *remotely* about that. Stop the idiocy already.

How hard is it to understand? How many times do people have to tell you?

That warning is very much valid during bootup, and that warning has been *seen* during bootup. For example, try to compile in most wireless drivers as non-modular, and that warning *has* to trigger.

Ok. I like where nowait() is going in the other part of the thread but I'm still confused about when request_firmware() is correct to use. It seems that the function is inherently racy with freezing. Does every user of request_firmware() need to synchronize with freezing?

For example, if one CPU is in the middle of a driver probe that makes a request_firmware() call and another CPU is starting to suspend we will have a race between usermodehelpers being disabled and the request_firmware() call acquiring the usermodehelper rwsem. If the suspending CPU wins the race it will disable usermodehelpers and the request_firmware() call will return -EBUSY and warn.

Quoted 5 lines · Stephen Boyd

Ok. I like where nowait() is going in the other part of the thread but I'm still confused about when request_firmware() is correct to use. It seems that the function is inherently racy with freezing. Does every user of request_firmware() need to synchronize with freezing?

Quoted 6 lines

For example, if one CPU is in the middle of a driver probe that makes a request_firmware() call and another CPU is starting to suspend we will have a race between usermodehelpers being disabled and the request_firmware() call acquiring the usermodehelper rwsem. If the suspending CPU wins the race it will disable usermodehelpers and the request_firmware() call will return -EBUSY and warn.

Yes, it will.

Quoted 13 lines · Rafael J. Wysocki

Quoted 11 lines · Stephen Boyd

Ok. I like where nowait() is going in the other part of the thread but I'm still confused about when request_firmware() is correct to use. It seems that the function is inherently racy with freezing. Does every user of request_firmware() need to synchronize with freezing?

For example, if one CPU is in the middle of a driver probe that makes a request_firmware() call and another CPU is starting to suspend we will have a race between usermodehelpers being disabled and the request_firmware() call acquiring the usermodehelper rwsem. If the suspending CPU wins the race it will disable usermodehelpers and the request_firmware() call will return -EBUSY and warn.

Quoted 1 line

Yes, it will.

That sounds wrong then, no? Why don't we have request_firmware() do a read_lock on the usermodehelpers sem and then have suspend do a write lock, disable usermodehelpers, wait for any users to finish, freeze processes and then unlock the write lock? That way we don't hit a case where request_firmware() races with suspend, and we don't have to change the warning or conditional.

Quoted 14 lines · Stephen Boyd

Quoted 13 lines · Rafael J. Wysocki

Quoted 11 lines · Stephen Boyd

Ok. I like where nowait() is going in the other part of the thread but I'm still confused about when request_firmware() is correct to use. It seems that the function is inherently racy with freezing. Does every user of request_firmware() need to synchronize with freezing?

For example, if one CPU is in the middle of a driver probe that makes a request_firmware() call and another CPU is starting to suspend we will have a race between usermodehelpers being disabled and the request_firmware() call acquiring the usermodehelper rwsem. If the suspending CPU wins the race it will disable usermodehelpers and the request_firmware() call will return -EBUSY and warn.

Quoted 8 lines

Quoted 1 line

Yes, it will.

That sounds wrong then, no? Why don't we have request_firmware() do a read_lock on the usermodehelpers sem and then have suspend do a write lock, disable usermodehelpers, wait for any users to finish, freeze processes and then unlock the write lock? That way we don't hit a case where request_firmware() races with suspend, and we don't have to change the warning or conditional.

So, you're postulating that the freezing of tasks be done under umhelper_sem write-locked, right?

That would lead to freezing failures if a user space task waited in request_firmware() for umhelper_sem to become available for read-locking and unfortunately we don't have an interruptible variant of down_read().

However, we may catch request_firmware() and try to freeze the task calling it instead. I'll try to prototype something along these lines later today (on top of the three "firmware_class" patches I posted yesterday).

 drivers/base/firmware_class.c |   57 +++++++++++++++++++++++++++++-------------
 1 file changed, 40 insertions(+), 17 deletions(-)

Index: linux/drivers/base/firmware_class.c
===================================================================

drivers/base/firmware_class.c +1 -1

@@ -435,7 +435,7 @@ static void firmware_class_timeout(u_lon
 }
 
 static struct firmware_priv *
-fw_create_instance(struct firmware *firmware, const char *fw_name,
+fw_create_instance(const struct firmware *firmware, const char *fw_name,
 		   struct device *device, bool uevent, bool nowait)
 {
 	struct firmware_priv *fw_priv;

Quoted 20 lines · Rafael J. Wysocki

 drivers/base/firmware_class.c |   57 +++++++++++++++++++++++++++++-------------
 1 file changed, 40 insertions(+), 17 deletions(-)

Index: linux/drivers/base/firmware_class.c
===================================================================

drivers/base/firmware_class.c +1 -1

@@ -435,7 +435,7 @@ static void firmware_class_timeout(u_lon
 }
 
 static struct firmware_priv *
-fw_create_instance(struct firmware *firmware, const char *fw_name,
+fw_create_instance(const struct firmware *firmware, const char *fw_name,
 		   struct device *device, bool uevent, bool nowait)
 {
 	struct firmware_priv *fw_priv;

@@ -449,7 +449,7 @@ fw_create_instance(struct firmware *firm
 		goto err_out;
 	}
 

drivers/base/firmware_class.c fw_create_instance(struct firmware *firm · line 455

Quoted 5 lines

drivers/base/firmware_class.c +1 -1

-	fw_priv->fw = firmware;
+	fw_priv->fw = (struct firmware *)firmware;
 	fw_priv->nowait = nowait;
 	strcpy(fw_priv->fw_id, fw_name);
 	init_completion(&fw_priv->completion);

Can we avoid this cast? If we do some parts of fw_create_instance() during the setup phase I think we can avoid it.

Quoted 21 lines · Stephen Boyd

Quoted 20 lines · Rafael J. Wysocki

 drivers/base/firmware_class.c |   57 +++++++++++++++++++++++++++++-------------
 1 file changed, 40 insertions(+), 17 deletions(-)

Index: linux/drivers/base/firmware_class.c
===================================================================

drivers/base/firmware_class.c +1 -1

@@ -435,7 +435,7 @@ static void firmware_class_timeout(u_lon
 }
 
 static struct firmware_priv *
-fw_create_instance(struct firmware *firmware, const char *fw_name,
+fw_create_instance(const struct firmware *firmware, const char *fw_name,
 		   struct device *device, bool uevent, bool nowait)
 {
 	struct firmware_priv *fw_priv;

@@ -449,7 +449,7 @@ fw_create_instance(struct firmware *firm
 		goto err_out;
 	}
 

Quoted 8 lines

drivers/base/firmware_class.c fw_create_instance(const struct firmware *firmware, const char *fw_name, · line 453

Quoted 5 lines

drivers/base/firmware_class.c +1 -1

-	fw_priv->fw = firmware;
+	fw_priv->fw = (struct firmware *)firmware;
 	fw_priv->nowait = nowait;
 	strcpy(fw_priv->fw_id, fw_name);
 	init_completion(&fw_priv->completion);

Can we avoid this cast? If we do some parts of fw_create_instance() during the setup phase I think we can avoid it.

Oops. With the other patch you can squash this in to do what I originally talked about.

-----8<-----

drivers/base/firmware_class.c +1 -1

index fbe98a8..113d37d 100644
@@ -435,7 +435,7 @@ static void firmware_class_timeout(u_long data)
 }
 
 static struct firmware_priv *
-fw_create_instance(const struct firmware *firmware, const char *fw_name,
+fw_create_instance(struct firmware *firmware, const char *fw_name,
                   struct device *device, bool uevent, bool nowait)
 {
        struct firmware_priv *fw_priv;

Hmm, what other patch? The one you send previously?

Quoted 20 lines · Rafael J. Wysocki

 drivers/base/firmware_class.c |   57 +++++++++++++++++++++++++++++-------------
 1 file changed, 40 insertions(+), 17 deletions(-)

Index: linux/drivers/base/firmware_class.c
===================================================================

drivers/base/firmware_class.c +1 -1

@@ -435,7 +435,7 @@ static void firmware_class_timeout(u_lon
 }
 
 static struct firmware_priv *
-fw_create_instance(struct firmware *firmware, const char *fw_name,
+fw_create_instance(const struct firmware *firmware, const char *fw_name,
 		   struct device *device, bool uevent, bool nowait)
 {
 	struct firmware_priv *fw_priv;

@@ -449,7 +449,7 @@ fw_create_instance(struct firmware *firm
 		goto err_out;
 	}
 

drivers/base/firmware_class.c fw_create_instance(struct firmware *firm · line 455

Quoted 5 lines

drivers/base/firmware_class.c +1 -1

-	fw_priv->fw = firmware;
+	fw_priv->fw = (struct firmware *)firmware;
 	fw_priv->nowait = nowait;
 	strcpy(fw_priv->fw_id, fw_name);
 	init_completion(&fw_priv->completion);

Can we avoid this cast? If we do some parts of fw_create_instance() during the setup phase I think we can avoid it.

drivers/base/firmware_class.c static int request_firmware_work_func(vo · line 660

Quoted 8 lines

drivers/base/firmware_class.c +3 -1

@@ -639,8 +655,15 @@ static int request_firmware_work_func(vo
 		return 0;
 	}
 
-	ret = _request_firmware(&fw, fw_work->name, fw_work->device,
+	ret = _request_firmware_prepare(&fw, fw_work->name, fw_work->device);
+	if (ret <= 0)
+		return ret;

This needs to jump to the cont function so that users know loading failed or that the firmware was builtin.

Quoted 8 lines

drivers/base/firmware_class.c +5 -0

+
+	ret = _request_firmware(fw, fw_work->name, fw_work->device,
 				fw_work->uevent, true);
+	if (ret)
+		_request_firmware_cleanup(&fw);
+
 	fw_work->cont(fw, fw_work->context);
 

drivers/base/firmware_class.c static int request_firmware_work_func(vo · line 669

Quoted 2 lines

drivers/base/firmware_class.c +0 -0

 	module_put(fw_work->module);

Here's a compile only tested patch on top. The idea is return a fw_priv pointer from the setup function and use that to indicate failure (error pointer), builtin (NULL) or loadable (non-NULL). Then we can move all the uevent/device stuff to the loading phase

----8<-----

drivers/base/firmware_class.c +61 -62

index 5db254d..fbe98a8 100644
@@ -440,13 +440,11 @@ fw_create_instance(const struct firmware *firmware, const char *fw_name,
 {
 	struct firmware_priv *fw_priv;
 	struct device *f_dev;
-	int error;
 
 	fw_priv = kzalloc(sizeof(*fw_priv) + strlen(fw_name) + 1 , GFP_KERNEL);
 	if (!fw_priv) {
 		dev_err(device, "%s: kmalloc failed\n", __func__);
-		error = -ENOMEM;
-		goto err_out;
+		return ERR_PTR(-ENOMEM);
 	}
 
 	fw_priv->fw = (struct firmware *)firmware;

@@ -463,42 +461,7 @@ fw_create_instance(const struct firmware *firmware, const char *fw_name,
 	f_dev->parent = device;
 	f_dev->class = &firmware_class;
 
-	dev_set_uevent_suppress(f_dev, true);
-
-	/* Need to pin this module until class device is destroyed */
-	__module_get(THIS_MODULE);
-
-	error = device_add(f_dev);
-	if (error) {
-		dev_err(device, "%s: device_register failed\n", __func__);
-		goto err_put_dev;
-	}
-
-	error = device_create_bin_file(f_dev, &firmware_attr_data);
-	if (error) {
-		dev_err(device, "%s: sysfs_create_bin_file failed\n", __func__);
-		goto err_del_dev;
-	}
-
-	error = device_create_file(f_dev, &dev_attr_loading);
-	if (error) {
-		dev_err(device, "%s: device_create_file failed\n", __func__);
-		goto err_del_bin_attr;
-	}
-
-	if (uevent)
-		dev_set_uevent_suppress(f_dev, false);
-
 	return fw_priv;
-
-err_del_bin_attr:
-	device_remove_bin_file(f_dev, &firmware_attr_data);
-err_del_dev:
-	device_del(f_dev);
-err_put_dev:
-	put_device(f_dev);
-err_out:
-	return ERR_PTR(error);
 }
 
 static void fw_destroy_instance(struct firmware_priv *fw_priv)

@@ -510,27 +473,34 @@ static void fw_destroy_instance(struct firmware_priv *fw_priv)
 	device_unregister(f_dev);
 }
 
-static int _request_firmware_prepare(const struct firmware **firmware_p,
-				     const char *name, struct device *device)
+static struct firmware_priv *
+_request_firmware_prepare(const struct firmware **firmware_p, const char *name,
+			  struct device *device, bool uevent, bool nowait)
 {
 	struct firmware *firmware;
+	struct firmware_priv *fw_priv;
 
 	if (!firmware_p)
-		return -EINVAL;
+		return ERR_PTR(-EINVAL);
 
 	*firmware_p = firmware = kzalloc(sizeof(*firmware), GFP_KERNEL);
 	if (!firmware) {
 		dev_err(device, "%s: kmalloc(struct firmware) failed\n",
 			__func__);
-		return -ENOMEM;
+		return ERR_PTR(-ENOMEM);
 	}
 
 	if (fw_get_builtin_firmware(firmware, name)) {
 		dev_dbg(device, "firmware: using built-in firmware %s\n", name);
-		return 0;
+		return NULL;
 	}
 
-	return 1;
+	fw_priv = fw_create_instance(firmware, name, device, uevent, nowait);
+	if (IS_ERR(fw_priv)) {
+		release_firmware(firmware);
+		*firmware_p = NULL;
+	}
+	return fw_priv;
 }
 
 static void _request_firmware_cleanup(const struct firmware **firmware_p)

@@ -539,29 +509,44 @@ static void _request_firmware_cleanup(const struct firmware **firmware_p)
 	*firmware_p = NULL;
 }
 
-static int _request_firmware(const struct firmware *firmware,
-			     const char *name, struct device *device,
-			     bool uevent, bool nowait)
+static int _request_firmware_load(struct firmware_priv *fw_priv, bool uevent)
 {
-	struct firmware_priv *fw_priv;
 	int retval;
+	struct device *f_dev = &fw_priv->dev;
 
 	retval = usermodehelper_read_trylock();
 	if (WARN_ON(retval)) {
-		dev_err(device, "firmware: %s will not be loaded\n", name);
+		dev_err(f_dev, "firmware: %s will not be loaded\n",
+				fw_priv->fw_id);
 		return retval;
 	}
 
-	if (uevent)
-		dev_dbg(device, "firmware: requesting %s\n", name);
+	dev_set_uevent_suppress(f_dev, true);
 
-	fw_priv = fw_create_instance(firmware, name, device, uevent, nowait);
-	if (IS_ERR(fw_priv)) {
-		retval = PTR_ERR(fw_priv);
-		goto out;
+	/* Need to pin this module until class device is destroyed */
+	__module_get(THIS_MODULE);
+
+	retval = device_add(f_dev);
+	if (retval) {
+		dev_err(f_dev, "%s: device_register failed\n", __func__);
+		goto err_put_dev;
+	}
+
+	retval = device_create_bin_file(f_dev, &firmware_attr_data);
+	if (retval) {
+		dev_err(f_dev, "%s: sysfs_create_bin_file failed\n", __func__);
+		goto err_del_dev;
+	}
+
+	retval = device_create_file(f_dev, &dev_attr_loading);
+	if (retval) {
+		dev_err(f_dev, "%s: device_create_file failed\n", __func__);
+		goto err_del_bin_attr;
 	}
 
 	if (uevent) {
+		dev_set_uevent_suppress(f_dev, false);
+		dev_dbg(f_dev, "firmware: requesting %s\n", fw_priv->fw_id);
 		if (loading_timeout > 0)
 			mod_timer(&fw_priv->timeout,
 				  round_jiffies_up(jiffies +

@@ -586,6 +571,14 @@ static int _request_firmware(const struct firmware *firmware,
 out:
 	usermodehelper_read_unlock();
 	return retval;
+
+err_del_bin_attr:
+	device_remove_bin_file(f_dev, &firmware_attr_data);
+err_del_dev:
+	device_del(f_dev);
+err_put_dev:
+	put_device(f_dev);
+	goto out;
 }
 
 /**

@@ -607,13 +600,15 @@ int
 request_firmware(const struct firmware **firmware_p, const char *name,
                  struct device *device)
 {
+	struct firmware_priv *fw_priv;
 	int ret;
 
-	ret = _request_firmware_prepare(firmware_p, name, device);
-	if (ret <= 0)
-		return ret;
+	fw_priv = _request_firmware_prepare(firmware_p, name, device, true,
+					    false);
+	if (IS_ERR_OR_NULL(fw_priv))
+		return PTR_RET(fw_priv);
 
-	ret = _request_firmware(*firmware_p, name, device, true, false);
+	ret = _request_firmware_load(fw_priv, true);
 	if (ret)
 		_request_firmware_cleanup(firmware_p);
 

@@ -648,6 +643,7 @@ static int request_firmware_work_func(void *arg)
 {
 	struct firmware_work *fw_work = arg;
 	const struct firmware *fw;
+	struct firmware_priv *fw_priv;
 	int ret;
 
 	if (!arg) {

@@ -655,15 +651,18 @@ static int request_firmware_work_func(void *arg)
 		return 0;
 	}
 
-	ret = _request_firmware_prepare(&fw, fw_work->name, fw_work->device);
-	if (ret <= 0)
-		return ret;
+	fw_priv = _request_firmware_prepare(&fw, fw_work->name, fw_work->device,
+			fw_work->uevent, true);
+	if (IS_ERR_OR_NULL(fw_priv)) {
+		ret = PTR_RET(fw_priv);
+		goto out;
+	}
 

Quoted 11 lines · Stephen Boyd

drivers/base/firmware_class.c static int request_firmware_work_func(vo · line 660

Quoted 8 lines · Rafael J. Wysocki

drivers/base/firmware_class.c +3 -1

@@ -639,8 +655,15 @@ static int request_firmware_work_func(vo
 		return 0;
 	}
 
-	ret = _request_firmware(&fw, fw_work->name, fw_work->device,
+	ret = _request_firmware_prepare(&fw, fw_work->name, fw_work->device);
+	if (ret <= 0)
+		return ret;

This needs to jump to the cont function so that users know loading failed or that the firmware was builtin.

You're right, sorry. That should have been

	if (ret > 0) {
		ret = _request_firmware(fw, fw_work->name, fw_work->device,
			fw_work->uevent, true);
		if (ret)
			_request_firmware_cleanup(&fw);
	}

but actually using a jump makes the next patch look better.

Updated patch is appended.
---
From: Rafael J. Wysocki <[email protected]>
Subject: firmware_class: Split _request_firmware() into three functions, v2

Split _request_firmware() into three functions, _request_firmware_prepare() doing preparatory work that need not be done under umhelper_sem, _request_firmware_cleanup() doing the post-error cleanup and _request_firmware() carrying out the remaining operations.

This change is requisite for moving the acquisition of umhelper_sem from _request_firmware() to the callers, which is going to be done subsequently.

Signed-off-by Rafael J. Wysocki <[email protected]>

---

Quoted 34 lines · Rafael J. Wysocki

Quoted 11 lines · Stephen Boyd

drivers/base/firmware_class.c static int request_firmware_work_func(vo · line 660

Quoted 8 lines · Rafael J. Wysocki

drivers/base/firmware_class.c +3 -1

@@ -639,8 +655,15 @@ static int request_firmware_work_func(vo
 		return 0;
 	}
 
-	ret = _request_firmware(&fw, fw_work->name, fw_work->device,
+	ret = _request_firmware_prepare(&fw, fw_work->name, fw_work->device);
+	if (ret <= 0)
+		return ret;

This needs to jump to the cont function so that users know loading failed or that the firmware was builtin.

You're right, sorry. That should have been

	if (ret > 0) {
		ret = _request_firmware(fw, fw_work->name, fw_work->device,
			fw_work->uevent, true);
		if (ret)
			_request_firmware_cleanup(&fw);
	}

but actually using a jump makes the next patch look better.

Updated patch is appended.
---
From: Rafael J. Wysocki <[email protected]>
Subject: firmware_class: Split _request_firmware() into three functions, v2

Split _request_firmware() into three functions, _request_firmware_prepare() doing preparatory work that need not be done under umhelper_sem, _request_firmware_cleanup() doing the post-error cleanup and _request_firmware() carrying out the remaining operations.

Quoted 3 lines

This change is requisite for moving the acquisition of umhelper_sem from _request_firmware() to the callers, which is going to be done subsequently.

Reviewed-by Stephen Boyd <[email protected]>

kernel/power/process.c int freeze_processes(void) · line 141

Quoted 12 lines · Rafael J. Wysocki

Index: linux/kernel/power/process.c
===================================================================

kernel/power/process.c +1 -0

@@ -135,6 +135,7 @@ int freeze_processes(void)
 	error = try_to_freeze_tasks(true);
 	if (!error) {
 		printk("done.");
+		__usermodehelper_reset(UMH_DISABLED);
 		oom_killer_disable();
 	}
 	printk("\n");

nitpick: This doesn't seem to be doing a reset, more like a set. Perhaps this function should be called __usermodehelper_set()?

Quoted 15 lines · Stephen Boyd

kernel/power/process.c int freeze_processes(void) · line 141

Quoted 12 lines · Rafael J. Wysocki

Index: linux/kernel/power/process.c
===================================================================

kernel/power/process.c +1 -0

@@ -135,6 +135,7 @@ int freeze_processes(void)
 	error = try_to_freeze_tasks(true);
 	if (!error) {
 		printk("done.");
+		__usermodehelper_reset(UMH_DISABLED);
 		oom_killer_disable();
 	}
 	printk("\n");

nitpick: This doesn't seem to be doing a reset, more like a set. Perhaps this function should be called __usermodehelper_set()?

Yes, you are right. I changed that before, but somehow managed to send an old patch. The new version follows.

---
From: Rafael J. Wysocki <[email protected]>
Subject: PM / Sleep: Mitigate race between the freezer and request_firmware()

There is a race condition between the freezer and request_firmware() such that if request_firmware() is run on one CPU and freeze_processes() is run on another CPU and usermodehelper_disable() called by it succeeds to grab umhelper_sem for writing before usermodehelper_read_trylock() called from request_firmware() acquires it for reading, the request_firmware() will fail and trigger a WARN_ON() complaining that it was called at a wrong time. However, in fact, it wasn't called at a wrong time and freeze_processes() simply happened to be executed simultaneously.

To avoid this race, at least in some cases, modify usermodehelper_read_trylock() so that it doesn't fail if the freezing of tasks has just started and hasn't been completed yet. Instead, during the freezing of tasks, it will try to freeze the task that has called it so that it can wait until user space is thawed without triggering the scary warning.

For this purpose, change usermodehelper_disabled so that it can take three different values, UMH_ENABLED (0), UMH_FREEZING and UMH_DISABLED. The first one means that usermode helpers are enabled, the last one means "hard disable" (i.e. the system is not ready for usermode helpers to be used) and the second one is reserved for the freezer. Namely, when freeze_processes() is started, it sets usermodehelper_disabled to UMH_FREEZING which tells usermodehelper_read_trylock() that it shouldn't fail just yet and should call try_to_freeze() if woken up and cannot return immediately. This way all freezable tasks that happen to call request_firmware() right before freeze_processes() is started and lose the race for umhelper_sem with it will be frozen and will sleep until thaw_processes() unsets usermodehelper_disabled. [For the non-freezable callers of request_firmware() the race for umhelper_sem against freeze_processes() is unfortunately unavoidable.]

Signed-off-by Rafael J. Wysocki <[email protected]>

---