Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S934672AbcJTJwr (ORCPT ); Thu, 20 Oct 2016 05:52:47 -0400 Received: from atlantic540.startdedicated.de ([188.138.9.77]:47008 "EHLO atlantic540.startdedicated.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S933891AbcJTJw3 (ORCPT ); Thu, 20 Oct 2016 05:52:29 -0400 From: Daniel Wagner To: linux-kernel@vger.kernel.org Cc: Ming Lei , "Luis R . Rodriguez" , Greg Kroah-Hartman , "Srivatsa S . Bhat" , "Rafael J . Wysocki" , Daniel Vetter , Takashi Iwai , Bjorn Andersson , Arend van Spriel , Daniel Wagner Subject: [PATCH v6 5/6] firmware: do not use fw_lock for fw_state protection Date: Thu, 20 Oct 2016 11:52:11 +0200 Message-Id: <1476957132-27818-6-git-send-email-wagi@monom.org> X-Mailer: git-send-email 2.7.4 In-Reply-To: <1476957132-27818-1-git-send-email-wagi@monom.org> References: <1476957132-27818-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: 4894 Lines: 155 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. Signed-off-by: Daniel Wagner Cc: Ming Lei Cc: Luis R. Rodriguez Cc: Greg Kroah-Hartman --- drivers/base/firmware_class.c | 52 +++++++++++++------------------------------ 1 file changed, 16 insertions(+), 36 deletions(-) diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c index a1dab6a792f1..4f1755537314 100644 --- a/drivers/base/firmware_class.c +++ b/drivers/base/firmware_class.c @@ -30,6 +30,7 @@ #include #include #include +#include #include @@ -105,13 +106,13 @@ enum fw_status { * firmwareq. */ 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; } @@ -120,25 +121,31 @@ 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 int __fw_state_wait_common(struct fw_state *fw_st, long timeout) { int 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; } static void __fw_state_set(struct fw_state *fw_st, - enum fw_status status) + enum fw_status status) { 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) \ @@ -374,14 +381,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) { @@ -428,7 +427,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); @@ -1106,25 +1105,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 @@ -1158,7 +1138,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