From 5b029624948d6864053166da1263df74c0c443df Mon Sep 17 00:00:00 2001 From: Daniel Wagner Date: Thu, 17 Nov 2016 11:00:50 +0100 Subject: [PATCH] firmware: do not use fw_lock for fw_state protection MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 Signed-off-by: Daniel Wagner Acked-by: Luis R. Rodriguez Signed-off-by: Greg Kroah-Hartman --- 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 45a20dd31c25..95f566c10a55 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); @@ -1084,26 +1083,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 @@ -1137,7 +1116,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 */