lkml.org 
[lkml]   [2012]   [Mar]   [13]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patches in this message
    /
    Date
    From
    SubjectRe: Re: [PATCH] firmware loader: don't cancel _nowait requests when helper is not yet available
    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.
    >

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


    \
     
     \ /
      Last update: 2012-03-13 10:47    [W:4.070 / U:0.472 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site