Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S937085AbcKQKBX (ORCPT ); Thu, 17 Nov 2016 05:01:23 -0500 Received: from atlantic540.startdedicated.de ([188.138.9.77]:49603 "EHLO atlantic540.startdedicated.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S936256AbcKQKBS (ORCPT ); Thu, 17 Nov 2016 05:01:18 -0500 From: Daniel Wagner To: linux-kernel@vger.kernel.org Cc: Ming Lei , "Luis R . Rodriguez" , Greg Kroah-Hartman , "Rafael J . Wysocki" , Daniel Vetter , Takashi Iwai , Bjorn Andersson , Arend van Spriel , Yves-Alexis Perez , Daniel Wagner Subject: [PATCH v7 3/4] firmware: do not use fw_lock for fw_state protection Date: Thu, 17 Nov 2016 11:00:50 +0100 Message-Id: <1479376851-2307-4-git-send-email-wagi@monom.org> X-Mailer: git-send-email 2.7.4 In-Reply-To: <1479376851-2307-1-git-send-email-wagi@monom.org> References: <1479376851-2307-1-git-send-email-wagi@monom.org> MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 4904 Lines: 151 From: Daniel Wagner 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_state can be used without needing the fw_lock to protect its state transition and wake ups. fw_state 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. Note there some more cleanups possible after with this change. For example for !CONFIG_FW_LOADER_USER_HELPER we don't check for the state anymore. Let's to this in the next patch instead mingling to many changes into this one. And yes you get a gcc warning "‘__fw_state_check’ defined but not used [-Wunused-function] code." for the time beeing. Cc: Ming Lei Cc: Luis R. Rodriguez Cc: Greg Kroah-Hartman Signed-off-by: Daniel Wagner --- drivers/base/firmware_class.c | 51 +++++++++++++------------------------------ 1 file changed, 15 insertions(+), 36 deletions(-) diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c index 1fda674f5219..0208a37cd2ff 100644 --- a/drivers/base/firmware_class.c +++ b/drivers/base/firmware_class.c @@ -30,6 +30,7 @@ #include #include #include +#include #include @@ -111,13 +112,13 @@ static inline long firmware_loading_timeout(void) * state of the firmware loading. */ struct fw_state { - struct completion completion; + struct swait_queue_head wq; enum fw_status status; }; static void fw_state_init(struct fw_state *fw_st) { - init_completion(&fw_st->completion); + init_swait_queue_head(&fw_st->wq); fw_st->status = FW_STATUS_UNKNOWN; } @@ -126,13 +127,19 @@ static int __fw_state_check(struct fw_state *fw_st, enum fw_status status) return fw_st->status == status; } +static inline bool __fw_state_is_done(enum fw_status status) +{ + return status == FW_STATUS_DONE || status == FW_STATUS_ABORTED; +} + static long __fw_state_wait_common(struct fw_state *fw_st, long timeout) { long ret; - ret = wait_for_completion_interruptible_timeout(&fw_st->completion, - timeout); - if (ret != 0 && READ_ONCE(fw_st->status) == FW_STATUS_ABORTED) + ret = swait_event_interruptible_timeout(fw_st->wq, + __fw_state_is_done(READ_ONCE(fw_st->status)), + timeout); + if (ret != 0 && fw_st->status == FW_STATUS_ABORTED) return -ENOENT; return ret; @@ -144,7 +151,7 @@ static void __fw_state_set(struct fw_state *fw_st, WRITE_ONCE(fw_st->status, status); if (status == FW_STATUS_DONE || status == FW_STATUS_ABORTED) - complete_all(&fw_st->completion); + swake_up(&fw_st->wq); } #define fw_state_start(fw_st) \ @@ -373,14 +380,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_state_done(&buf->fw_st); - mutex_unlock(&fw_lock); -} - static int fw_get_filesystem_firmware(struct device *device, struct firmware_buf *buf) { @@ -427,7 +426,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_state_done(&buf->fw_st); break; } __putname(path); @@ -1082,26 +1081,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_state_is_done(&buf->fw_st)) { - if (fw_state_is_aborted(&buf->fw_st)) { - ret = -ENOENT; - break; - } - mutex_unlock(&fw_lock); - ret = fw_state_wait(&buf->fw_st); - 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 @@ -1135,7 +1114,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_state_wait(&buf->fw_st); if (!ret) { fw_set_page_data(buf, firmware); return 0; /* assigned */ -- 2.7.4