[PATCH v5 5/5] firmware: do not use fw_lock for fw_umh protection

[Daniel Wagner]

From: Daniel Wagner <daniel.wagner@xxxxxxxxxxxx>

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@xxxxxxxxxxxxx>
Cc: Luis R. Rodriguez <mcgrof@xxxxxxxxxx>
Cc: Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>
Signed-off-by: Daniel Wagner <daniel.wagner@xxxxxxxxxxxx>
---
 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