lkml.org 
[lkml]   [2016]   [Sep]   [9]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    From
    Subject[PATCH v5 5/5] firmware: do not use fw_lock for fw_umh protection
    Date
    From: Daniel Wagner <daniel.wagner@bmw-carit.de>

    fw_lock is to use to protect 'corner cases' inside firmware_class. It
    is not exactly clear what those corner cases are nor what it exactly
    protects. fw_umh can be used without needing the fw_lock to protect
    its state transition and wake ups.

    fw_umh is holds the state in status and the completion is used to
    wake up all waiters (in this case that is the user land helper so only
    one). This operation has to be 'atomic' to avoid races. We can do this
    by using swait which takes care we don't miss any wake up.

    We use also swait instead of wait because don't need all the additional
    features wait provides.

    Cc: Ming Lei <ming.lei@canonical.com>
    Cc: Luis R. Rodriguez <mcgrof@kernel.org>
    Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
    Signed-off-by: Daniel Wagner <daniel.wagner@bmw-carit.de>
    ---
    drivers/base/firmware_class.c | 50 +++++++++++++------------------------------
    1 file changed, 15 insertions(+), 35 deletions(-)

    diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
    index 8f5838c..1a28070 100644
    --- a/drivers/base/firmware_class.c
    +++ b/drivers/base/firmware_class.c
    @@ -30,6 +30,7 @@
    #include <linux/syscore_ops.h>
    #include <linux/reboot.h>
    #include <linux/security.h>
    +#include <linux/swait.h>

    #include <generated/utsrelease.h>

    @@ -108,13 +109,13 @@ enum {
    };

    struct fw_umh {
    - struct completion completion;
    + struct swait_queue_head wq;
    u8 status;
    };

    static void fw_umh_init(struct fw_umh *fw_umh)
    {
    - init_completion(&fw_umh->completion);
    + init_swait_queue_head(&fw_umh->wq);
    fw_umh->status = FW_UMH_UNKNOWN;
    }

    @@ -123,13 +124,19 @@ static int __fw_umh_check(struct fw_umh *fw_umh, unsigned long status)
    return fw_umh->status == status;
    }

    +static inline bool __fw_umh_is_done(unsigned long status)
    +{
    + return status == FW_UMH_DONE || status == FW_UMH_ABORTED;
    +}
    +
    static int fw_umh_wait_timeout(struct fw_umh *fw_umh, long timeout)
    {
    int ret;

    - ret = wait_for_completion_interruptible_timeout(&fw_umh->completion,
    - timeout);
    - if (ret != 0 && READ_ONCE(fw_umh->status) == FW_UMH_ABORTED)
    + ret = swait_event_interruptible_timeout(fw_umh->wq,
    + __fw_umh_is_done(READ_ONCE(fw_umh->status)),
    + timeout);
    + if (ret != 0 && fw_umh->status == FW_UMH_ABORTED)
    return -ENOENT;

    return ret;
    @@ -141,7 +148,7 @@ static void __fw_umh_set(struct fw_umh *fw_umh,
    WRITE_ONCE(fw_umh->status, status);

    if (status == FW_UMH_DONE || status == FW_UMH_ABORTED)
    - complete_all(&fw_umh->completion);
    + swake_up(&fw_umh->wq);
    }

    #define fw_umh_start(fw_umh) \
    @@ -367,14 +374,6 @@ static const char * const fw_path[] = {
    module_param_string(path, fw_path_para, sizeof(fw_path_para), 0644);
    MODULE_PARM_DESC(path, "customized firmware image search path with a higher priority than default path");

    -static void fw_finish_direct_load(struct device *device,
    - struct firmware_buf *buf)
    -{
    - mutex_lock(&fw_lock);
    - fw_umh_done(&buf->fw_umh);
    - mutex_unlock(&fw_lock);
    -}
    -
    static int
    fw_get_filesystem_firmware(struct device *device, struct firmware_buf *buf)
    {
    @@ -421,7 +420,7 @@ fw_get_filesystem_firmware(struct device *device, struct firmware_buf *buf)
    }
    dev_dbg(device, "direct-loading %s\n", buf->fw_id);
    buf->size = size;
    - fw_finish_direct_load(device, buf);
    + fw_umh_done(&buf->fw_umh);
    break;
    }
    __putname(path);
    @@ -1074,25 +1073,6 @@ static inline void kill_requests_without_uevent(void) { }

    #endif /* CONFIG_FW_LOADER_USER_HELPER */

    -/* wait until the shared firmware_buf becomes ready (or error) */
    -static int sync_cached_firmware_buf(struct firmware_buf *buf)
    -{
    - int ret = 0;
    -
    - mutex_lock(&fw_lock);
    - while (!fw_umh_is_done(&buf->fw_umh)) {
    - if (fw_umh_is_aborted(&buf->fw_umh)) {
    - ret = -ENOENT;
    - break;
    - }
    - mutex_unlock(&fw_lock);
    - ret = fw_umh_wait_timeout(&buf->fw_umh, 0);
    - mutex_lock(&fw_lock);
    - }
    - mutex_unlock(&fw_lock);
    - return ret;
    -}
    -
    /* prepare firmware and firmware_buf structs;
    * return 0 if a firmware is already assigned, 1 if need to load one,
    * or a negative error code
    @@ -1126,7 +1106,7 @@ _request_firmware_prepare(struct firmware **firmware_p, const char *name,
    firmware->priv = buf;

    if (ret > 0) {
    - ret = sync_cached_firmware_buf(buf);
    + ret = fw_umh_wait_timeout(&buf->fw_umh, 0);
    if (!ret) {
    fw_set_page_data(buf, firmware);
    return 0; /* assigned */
    --
    2.7.4
    \
     
     \ /
      Last update: 2016-09-17 09:59    [W:16.593 / U:0.020 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site