Re: Re: [PATCH] firmware loader: don't cancel _nowait requests whenhelper is not yet available
From: Saravana Kannan
Date: Tue Mar 13 2012 - 05:37:31 EST
On 01/-10/37 11:59, Kay Sievers wrote:
On Sat, Mar 10, 2012 at 00:36, Greg KH<gregkh@xxxxxxxxxxxxxxxxxxx> wrote:
On Fri, Mar 09, 2012 at 11:30:24PM +0100, Christian Lamparter wrote:
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?
Signed-off-by: Christian Lamparter<chunkeey@xxxxxxxxxxxxxx>
---
drivers/base/firmware_class.c | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
index 6c9387d..017e020 100644
--- a/drivers/base/firmware_class.c
+++ b/drivers/base/firmware_class.c
@@ -535,7 +535,7 @@ static int _request_firmware(const struct firmware **firmware_p,
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.
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 <skannan@xxxxxxxxxxxxxx>
---
drivers/base/firmware_class.c | 15 ++++++++-------
kernel/kmod.c | 2 +-
2 files changed, 9 insertions(+), 8 deletions(-)
diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
index 06ed6b4..2a45bf7 100644
--- a/drivers/base/firmware_class.c
+++ b/drivers/base/firmware_class.c
@@ -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);
diff --git a/kernel/kmod.c b/kernel/kmod.c
index 47613df..e733afe3 100644
--- a/kernel/kmod.c
+++ b/kernel/kmod.c
@@ -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.
Thanks,
Saravana
--
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.
--
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/