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

[Saravana Kannan <skannan@codeaurora.org>]

On 01/-10/37 11:59, Kay Sievers wrote:
> On Sat, Mar 10, 2012 at 00:36, Greg KH<gregkh@linuxfoundation.org>  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@googlemail.com>
>>> ---
>>>   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@codeaurora.org>
---
  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.