[PATCH v2 6/6] firmware: move umh try locks into the umh code

[Luis R. Rodriguez]

This moves the usermode helper locks into only code paths that use the
usermode helper API from the kernel. The usermode helper locks were
originally added to prevent stalling suspend, later the firmware cache
was added to help with this, and further later direct filesystem lookup
was added by Linus to completely bypass udev due to the amount of issues
the umh approach had.

The usermode helper locks were kept even when the direct filesystem lookup
mechanism is used though. A lot has changed since the original usermode
helper locks were added but the recent commit which added the code for
firmware_enabled() are intended to address any possible races cured only
as collateral by using the locks as though side consequence of code
evolution and this not being addressed any time sooner. With the
firmware_enabled() code in place we are a bit more sure to move the
usermode helper locks to UMH only code.

There is a bit of history here so let's recap a bit of it to ensure nothing
is lost and things are clear. The direct filesystem approach to loading
firmware is rather new, it was added via commit abb139e75c2cdb ("firmware:
teach the kernel to load firmware files directly from the filesystem") by
Linus merged on the v3.7 release, to enable to bypass udev.

usermodehelper_read_lock_wait() was added earlier via commit 9b78c1da60b3c
("firmware_class: Do not warn that system is not ready from async loads")
merged on v3.4, after Rafael noted that the async firmware API call
request_firmware_nowait() should not be penalized to fail if userspace is
not available yet or frozen, it'd allow for a timeout grace period before
giving up. The WARN_ON() was kept for the sync firmware API call though on
request_firmware(). At this time there was no direct filesystem lookup for
firmware though.

The original usermode helper lock came from commit a144c6a6c924a ("PM:
Print a warning if firmware is requested when tasks are frozen") merged on
the v3.0 kernel by Rafael to print a warning back when firmware requests
were used on resume(), thaw() or restore() callbacks and there was no
direct fs lookups or the firmware cache.

Signed-off-by: Luis R. Rodriguez <mcgrof@xxxxxxxxxx>
---
 drivers/base/firmware_class.c | 68 +++++++++++++++++++++++--------------------
 1 file changed, 36 insertions(+), 32 deletions(-)

diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
index 7722943025f3..b9f907eedbf7 100644
--- a/drivers/base/firmware_class.c
+++ b/drivers/base/firmware_class.c
@@ -1102,23 +1102,51 @@ static int _request_firmware_load(struct firmware_priv *fw_priv,
 
 static int fw_load_from_user_helper(struct firmware *firmware,
 				    const char *name, struct device *device,
-				    unsigned int opt_flags, long timeout)
+				    unsigned int opt_flags)
 {
 	struct firmware_priv *fw_priv;
+	long timeout;
+	int ret;
+
+	timeout = firmware_loading_timeout();
+	if (opt_flags & FW_OPT_NOWAIT) {
+		timeout = usermodehelper_read_lock_wait(timeout);
+		if (!timeout) {
+			dev_dbg(device, "firmware: %s loading timed out\n",
+				name);
+			return -EBUSY;
+		}
+	} else {
+		ret = usermodehelper_read_trylock();
+		if (WARN_ON(ret)) {
+			dev_err(device, "firmware: %s will not be loaded\n",
+				name);
+			return ret;
+		}
+	}
 
 	fw_priv = fw_create_instance(firmware, name, device, opt_flags);
-	if (IS_ERR(fw_priv))
-		return PTR_ERR(fw_priv);
+	if (IS_ERR(fw_priv)) {
+		ret = PTR_ERR(fw_priv);
+		goto out_unlock;
+	}
 
 	fw_priv->buf = firmware->priv;
-	return _request_firmware_load(fw_priv, opt_flags, timeout);
+	ret = _request_firmware_load(fw_priv, opt_flags, timeout);
+
+	if (!ret)
+		ret = assign_firmware_buf(firmware, device, opt_flags);
+
+out_unlock:
+	usermodehelper_read_unlock();
+
+	return ret;
 }
 
 #else /* CONFIG_FW_LOADER_USER_HELPER */
 static inline int
 fw_load_from_user_helper(struct firmware *firmware, const char *name,
-			 struct device *device, unsigned int opt_flags,
-			 long timeout)
+			 struct device *device, unsigned int opt_flags)
 {
 	return -ENOENT;
 }
@@ -1179,7 +1207,6 @@ _request_firmware(const struct firmware **firmware_p, const char *name,
 		  unsigned int opt_flags)
 {
 	struct firmware *fw = NULL;
-	long timeout;
 	int ret;
 
 	if (!firmware_p)
@@ -1200,25 +1227,6 @@ _request_firmware(const struct firmware **firmware_p, const char *name,
 		goto out;
 	}
 
-	ret = 0;
-	timeout = firmware_loading_timeout();
-	if (opt_flags & FW_OPT_NOWAIT) {
-		timeout = usermodehelper_read_lock_wait(timeout);
-		if (!timeout) {
-			dev_dbg(device, "firmware: %s loading timed out\n",
-				name);
-			ret = -EBUSY;
-			goto out;
-		}
-	} else {
-		ret = usermodehelper_read_trylock();
-		if (WARN_ON(ret)) {
-			dev_err(device, "firmware: %s will not be loaded\n",
-				name);
-			goto out;
-		}
-	}
-
 	ret = fw_get_filesystem_firmware(device, fw->priv);
 	if (ret) {
 		if (!(opt_flags & FW_OPT_NO_WARN))
@@ -1228,15 +1236,11 @@ _request_firmware(const struct firmware **firmware_p, const char *name,
 		if (opt_flags & FW_OPT_USERHELPER) {
 			dev_warn(device, "Falling back to user helper\n");
 			ret = fw_load_from_user_helper(fw, name, device,
-						       opt_flags, timeout);
+						       opt_flags);
 		}
-	}
-
-	if (!ret)
+	} else
 		ret = assign_firmware_buf(fw, device, opt_flags);
 
-	usermodehelper_read_unlock();
-
  out:
 	if (ret < 0) {
 		release_firmware(fw);
-- 
2.11.0