[PATCH 0/5] firmware: move UMH locks onto fallback code

[PATCH 0/5] firmware: move UMH locks onto fallback code

[Luis R. Rodriguez]

Greg,

One of the eyesores on the old API was the use of the UMH lock even when
we don't use any of the usermode helpers. It took quite a bit of git
archeology to draw up a solution which makes me feel comfortable in
moving this code out of the way given it may have added new protections
we never knew about. A resolution is to make similar protections for
direct FS lookups for now, and later after a release or so we can
consider removing them. There is further possible work to do to clean
up the UMH pollution on external code, but this can be done separately
and is more a generic kernel/kmod.c thing than firmware thing.

This changes makes subsequent patches able to fold the new driver data
API onto the older firmware API. These all go hammered with 0-day and
the firmware test scripts shipped upstream.

Luis R. Rodriguez (5):
  firmware: share fw fallback killing on reboot/suspend
  firmware: always enable the reboot notifier
  firmware: add sanity check on shutdown/suspend
  firmware: move assign_firmware_buf() further up
  firmware: move umh try locks into the umh code

 .../driver-api/firmware/request_firmware.rst       |  11 +
 drivers/base/firmware_class.c                      | 293 ++++++++++++++-------
 2 files changed, 207 insertions(+), 97 deletions(-)

-- 
2.11.0

[PATCH 1/5] firmware: share fw fallback killing on reboot/suspend

[Luis R. Rodriguez]

We kill pending fallback requests on suspend and reboot,
the only difference is that on suspend we only kill custom
fallback requests. Provide a wrapper that lets us customize
the request with a flag.

This also lets us simplify the #ifdef'ery over the calls.

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

diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
index ac350c518e0c..d2e2d83aaf26 100644
--- a/drivers/base/firmware_class.c
+++ b/drivers/base/firmware_class.c
@@ -562,16 +562,15 @@ static void fw_load_abort(struct firmware_priv *fw_priv)
 
 static LIST_HEAD(pending_fw_head);
 
-/* reboot notifier for avoid deadlock with usermode_lock */
 static int fw_shutdown_notify(struct notifier_block *unused1,
 			      unsigned long unused2, void *unused3)
 {
-	mutex_lock(&fw_lock);
-	while (!list_empty(&pending_fw_head))
-		__fw_load_abort(list_first_entry(&pending_fw_head,
-					       struct firmware_buf,
-					       pending_list));
-	mutex_unlock(&fw_lock);
+	/*
+	 * Kill all pending fallback requests to avoid both stalling shutdown,
+	 * and avoid a deadlock with the usermode_lock.
+	 */
+	kill_pending_fw_fallback_reqs(false);
+
 	return NOTIFY_DONE;
 }
 
@@ -1048,21 +1047,20 @@ static int fw_load_from_user_helper(struct firmware *firmware,
 	return _request_firmware_load(fw_priv, opt_flags, timeout);
 }
 
-#ifdef CONFIG_PM_SLEEP
-/* kill pending requests without uevent to avoid blocking suspend */
-static void kill_requests_without_uevent(void)
+static void kill_pending_fw_fallback_reqs(bool only_kill_custom)
 {
 	struct firmware_buf *buf;
 	struct firmware_buf *next;
 
 	mutex_lock(&fw_lock);
 	list_for_each_entry_safe(buf, next, &pending_fw_head, pending_list) {
-		if (!buf->need_uevent)
+		if (!only_kill_custom)
+			 __fw_load_abort(buf);
+		else if (!buf->need_uevent)
 			 __fw_load_abort(buf);
 	}
 	mutex_unlock(&fw_lock);
 }
-#endif
 
 #else /* CONFIG_FW_LOADER_USER_HELPER */
 static inline int
@@ -1073,9 +1071,7 @@ fw_load_from_user_helper(struct firmware *firmware, const char *name,
 	return -ENOENT;
 }
 
-#ifdef CONFIG_PM_SLEEP
-static inline void kill_requests_without_uevent(void) { }
-#endif
+static inline void kill_pending_fw_fallback_reqs(bool only_kill_custom) { }
 
 #endif /* CONFIG_FW_LOADER_USER_HELPER */
 
@@ -1724,7 +1720,11 @@ static int fw_pm_notify(struct notifier_block *notify_block,
 	case PM_HIBERNATION_PREPARE:
 	case PM_SUSPEND_PREPARE:
 	case PM_RESTORE_PREPARE:
-		kill_requests_without_uevent();
+		/*
+		 * kill pending fallback requests with a custom fallback
+		 * to avoid stalling suspend.
+		 */
+		kill_pending_fw_fallback_reqs(true);
 		device_cache_fw_images();
 		break;
 
-- 
2.11.0

[PATCH 2/5] firmware: always enable the reboot notifier

[Luis R. Rodriguez]

Now that we've have proper wrappers for the fallback mechanism
we can easily share the reboot notifier for the firmware_class
at all times.

This change will make subsequent modifications to the reboot
notifier easier to review.

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

diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
index d2e2d83aaf26..e8edf22536c1 100644
--- a/drivers/base/firmware_class.c
+++ b/drivers/base/firmware_class.c
@@ -562,22 +562,6 @@ static void fw_load_abort(struct firmware_priv *fw_priv)
 
 static LIST_HEAD(pending_fw_head);
 
-static int fw_shutdown_notify(struct notifier_block *unused1,
-			      unsigned long unused2, void *unused3)
-{
-	/*
-	 * Kill all pending fallback requests to avoid both stalling shutdown,
-	 * and avoid a deadlock with the usermode_lock.
-	 */
-	kill_pending_fw_fallback_reqs(false);
-
-	return NOTIFY_DONE;
-}
-
-static struct notifier_block fw_shutdown_nb = {
-	.notifier_call = fw_shutdown_notify,
-};
-
 static ssize_t timeout_show(struct class *class, struct class_attribute *attr,
 			    char *buf)
 {
@@ -1783,11 +1767,27 @@ static void __init fw_cache_init(void)
 #endif
 }
 
+static int fw_shutdown_notify(struct notifier_block *unused1,
+			      unsigned long unused2, void *unused3)
+{
+	/*
+	 * Kill all pending fallback requests to avoid both stalling shutdown,
+	 * and avoid a deadlock with the usermode_lock.
+	 */
+	kill_pending_fw_fallback_reqs(false);
+
+	return NOTIFY_DONE;
+}
+
+static struct notifier_block fw_shutdown_nb = {
+	.notifier_call = fw_shutdown_notify,
+};
+
 static int __init firmware_class_init(void)
 {
 	fw_cache_init();
-#ifdef CONFIG_FW_LOADER_USER_HELPER
 	register_reboot_notifier(&fw_shutdown_nb);
+#ifdef CONFIG_FW_LOADER_USER_HELPER
 	return class_register(&firmware_class);
 #else
 	return 0;
@@ -1800,8 +1800,8 @@ static void __exit firmware_class_exit(void)
 	unregister_syscore_ops(&fw_syscore_ops);
 	unregister_pm_notifier(&fw_cache.pm_notify);
 #endif
-#ifdef CONFIG_FW_LOADER_USER_HELPER
 	unregister_reboot_notifier(&fw_shutdown_nb);
+#ifdef CONFIG_FW_LOADER_USER_HELPER
 	class_unregister(&firmware_class);
 #endif
 }
-- 
2.11.0

[PATCH 3/5] firmware: add sanity check on shutdown/suspend

[Luis R. Rodriguez]

The firmware API should not be used after we go to suspend
and after we reboot/halt. The suspend/resume case is a bit
complex, so this documents that so things are clearer.

We want to know about users of the API in incorrect places so
that their callers are corrected, so this also adds a warn
for those cases.

Signed-off-by: Luis R. Rodriguez <mcgrof@kernel.org>
---
 .../driver-api/firmware/request_firmware.rst       | 11 +++
 drivers/base/firmware_class.c                      | 96 ++++++++++++++++++++++
 2 files changed, 107 insertions(+)

diff --git a/Documentation/driver-api/firmware/request_firmware.rst b/Documentation/driver-api/firmware/request_firmware.rst
index cc0aea880824..1c2c4967cd43 100644
--- a/Documentation/driver-api/firmware/request_firmware.rst
+++ b/Documentation/driver-api/firmware/request_firmware.rst
@@ -44,6 +44,17 @@ request_firmware_nowait
 .. kernel-doc:: drivers/base/firmware_class.c
    :functions: request_firmware_nowait
 
+Considerations for suspend and resume
+=====================================
+
+During suspend and resume only the built-in firmware and the firmware cache
+elements of the firmware API can be used. This is managed by fw_pm_notify().
+
+fw_pm_notify
+------------
+.. kernel-doc:: drivers/base/firmware_class.c
+   :functions: fw_pm_notify
+
 request firmware API expected driver use
 ========================================
 
diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
index e8edf22536c1..f7ac619635b4 100644
--- a/drivers/base/firmware_class.c
+++ b/drivers/base/firmware_class.c
@@ -260,6 +260,38 @@ static int fw_cache_piggyback_on_request(const char *name);
  * guarding for corner cases a global lock should be OK */
 static DEFINE_MUTEX(fw_lock);
 
+static bool __enable_firmware = false;
+
+static void enable_firmware(void)
+{
+	mutex_lock(&fw_lock);
+	__enable_firmware = true;
+	mutex_unlock(&fw_lock);
+}
+
+static void disable_firmware(void)
+{
+	mutex_lock(&fw_lock);
+	__enable_firmware = false;
+	mutex_unlock(&fw_lock);
+}
+
+/*
+ * When disabled only the built-in firmware and the firmware cache will be
+ * used to look for firmware.
+ */
+static bool firmware_enabled(void)
+{
+	bool enabled = false;
+
+	mutex_lock(&fw_lock);
+	if (__enable_firmware)
+		enabled = true;
+	mutex_unlock(&fw_lock);
+
+	return enabled;
+}
+
 static struct firmware_cache fw_cache;
 
 static struct firmware_buf *__allocate_fw_buf(const char *fw_name,
@@ -1165,6 +1197,12 @@ _request_firmware(const struct firmware **firmware_p, const char *name,
 	if (ret <= 0) /* error or already assigned */
 		goto out;
 
+	if (!firmware_enabled()) {
+		WARN(1, "firmware request while host is not available\n");
+		ret = -EHOSTDOWN;
+		goto out;
+	}
+
 	ret = 0;
 	timeout = firmware_loading_timeout();
 	if (opt_flags & FW_OPT_NOWAIT) {
@@ -1697,6 +1735,59 @@ static void device_uncache_fw_images_delay(unsigned long delay)
 			   msecs_to_jiffies(delay));
 }
 
+/**
+ * fw_pm_notify - notifier for suspend/resume
+ *
+ * Used to modify the firmware_class state as we move in between states.
+ * The firmware_class implements a firmware cache to enable device driver
+ * to fetch firmware upon resume before the root filesystem is ready. We
+ * disable API calls which do not use the built-in firmware or the firmware
+ * cache when we know these calls will not work.
+ *
+ * The inner logic behind all this is a bit complex so it is worth summarizing
+ * the kernel's own suspend/resume process with context and focus on how this
+ * can impact the firmware API.
+ *
+ * First a review on how we go to suspend:
+ *
+ * pm_suspend() --> enter_state() -->
+ *	sys_sync()
+ *	suspend_prepare() -->
+ *		__pm_notifier_call_chain(PM_SUSPEND_PREPARE, ...);
+ *		suspend_freeze_processes() -->
+ *			freeze_processes() -->
+ *				__usermodehelper_set_disable_depth(UMH_DISABLED);
+ *				freeze all tasks ...
+ *			freeze_kernel_threads()
+ *	suspend_devices_and_enter() -->
+ *		dpm_suspend_start() -->
+ *				dpm_prepare()
+ *				dpm_suspend()
+ *		suspend_enter()  -->
+ *			platform_suspend_prepare()
+ *			dpm_suspend_late()
+ *			freeze_enter()
+ *			syscore_suspend()
+ *
+ * When we resume we bail out of a loop from suspend_devices_and_enter() and
+ * unwind back out to the caller enter_state() where we were before as follows:
+ *
+ * enter_state() -->
+ *	suspend_devices_and_enter() --> (bail from loop)
+ *		dpm_resume_end() -->
+ *			dpm_resume()
+ *			dpm_complete()
+ *	suspend_finish() -->
+ *		suspend_thaw_processes() -->
+ *			thaw_processes() -->
+ *				__usermodehelper_set_disable_depth(UMH_FREEZING);
+ *				thaw_workqueues();
+ *				thaw all processes ...
+ *				usermodehelper_enable();
+ *		pm_notifier_call_chain(PM_POST_SUSPEND);
+ *
+ * fw_pm_notify() works through pm_notifier_call_chain().
+ */
 static int fw_pm_notify(struct notifier_block *notify_block,
 			unsigned long mode, void *unused)
 {
@@ -1710,6 +1801,7 @@ static int fw_pm_notify(struct notifier_block *notify_block,
 		 */
 		kill_pending_fw_fallback_reqs(true);
 		device_cache_fw_images();
+		disable_firmware();
 		break;
 
 	case PM_POST_SUSPEND:
@@ -1722,6 +1814,7 @@ static int fw_pm_notify(struct notifier_block *notify_block,
 		mutex_lock(&fw_lock);
 		fw_cache.state = FW_LOADER_NO_CACHE;
 		mutex_unlock(&fw_lock);
+		enable_firmware();
 
 		device_uncache_fw_images_delay(10 * MSEC_PER_SEC);
 		break;
@@ -1770,6 +1863,7 @@ static void __init fw_cache_init(void)
 static int fw_shutdown_notify(struct notifier_block *unused1,
 			      unsigned long unused2, void *unused3)
 {
+	disable_firmware();
 	/*
 	 * Kill all pending fallback requests to avoid both stalling shutdown,
 	 * and avoid a deadlock with the usermode_lock.
@@ -1785,6 +1879,7 @@ static struct notifier_block fw_shutdown_nb = {
 
 static int __init firmware_class_init(void)
 {
+	enable_firmware();
 	fw_cache_init();
 	register_reboot_notifier(&fw_shutdown_nb);
 #ifdef CONFIG_FW_LOADER_USER_HELPER
@@ -1796,6 +1891,7 @@ static int __init firmware_class_init(void)
 
 static void __exit firmware_class_exit(void)
 {
+	disable_firmware();
 #ifdef CONFIG_PM_SLEEP
 	unregister_syscore_ops(&fw_syscore_ops);
 	unregister_pm_notifier(&fw_cache.pm_notify);
-- 
2.11.0

[PATCH 4/5] firmware: move assign_firmware_buf() further up

[Luis R. Rodriguez]

This will make subsequent changes easier to read.

Signed-off-by: Luis R. Rodriguez <mcgrof@kernel.org>
---
 drivers/base/firmware_class.c | 77 +++++++++++++++++++++----------------------
 1 file changed, 38 insertions(+), 39 deletions(-)

diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
index f7ac619635b4..f28fbfab9c94 100644
--- a/drivers/base/firmware_class.c
+++ b/drivers/base/firmware_class.c
@@ -555,6 +555,44 @@ static int fw_add_devm_name(struct device *dev, const char *name)
 }
 #endif
 
+static int assign_firmware_buf(struct firmware *fw, struct device *device,
+			       unsigned int opt_flags)
+{
+	struct firmware_buf *buf = fw->priv;
+
+	mutex_lock(&fw_lock);
+	if (!buf->size || fw_state_is_aborted(&buf->fw_st)) {
+		mutex_unlock(&fw_lock);
+		return -ENOENT;
+	}
+
+	/*
+	 * add firmware name into devres list so that we can auto cache
+	 * and uncache firmware for device.
+	 *
+	 * device may has been deleted already, but the problem
+	 * should be fixed in devres or driver core.
+	 */
+	/* don't cache firmware handled without uevent */
+	if (device && (opt_flags & FW_OPT_UEVENT) &&
+	    !(opt_flags & FW_OPT_NOCACHE))
+		fw_add_devm_name(device, buf->fw_id);
+
+	/*
+	 * After caching firmware image is started, let it piggyback
+	 * on request firmware.
+	 */
+	if (!(opt_flags & FW_OPT_NOCACHE) &&
+	    buf->fwc->state == FW_LOADER_START_CACHE) {
+		if (fw_cache_piggyback_on_request(buf->fw_id))
+			kref_get(&buf->ref);
+	}
+
+	/* pass the pages buffer to driver at the last minute */
+	fw_set_page_data(buf, fw);
+	mutex_unlock(&fw_lock);
+	return 0;
+}
 
 /*
  * user-mode helper code
@@ -1136,45 +1174,6 @@ _request_firmware_prepare(struct firmware **firmware_p, const char *name,
 	return 1; /* need to load */
 }
 
-static int assign_firmware_buf(struct firmware *fw, struct device *device,
-			       unsigned int opt_flags)
-{
-	struct firmware_buf *buf = fw->priv;
-
-	mutex_lock(&fw_lock);
-	if (!buf->size || fw_state_is_aborted(&buf->fw_st)) {
-		mutex_unlock(&fw_lock);
-		return -ENOENT;
-	}
-
-	/*
-	 * add firmware name into devres list so that we can auto cache
-	 * and uncache firmware for device.
-	 *
-	 * device may has been deleted already, but the problem
-	 * should be fixed in devres or driver core.
-	 */
-	/* don't cache firmware handled without uevent */
-	if (device && (opt_flags & FW_OPT_UEVENT) &&
-	    !(opt_flags & FW_OPT_NOCACHE))
-		fw_add_devm_name(device, buf->fw_id);
-
-	/*
-	 * After caching firmware image is started, let it piggyback
-	 * on request firmware.
-	 */
-	if (!(opt_flags & FW_OPT_NOCACHE) &&
-	    buf->fwc->state == FW_LOADER_START_CACHE) {
-		if (fw_cache_piggyback_on_request(buf->fw_id))
-			kref_get(&buf->ref);
-	}
-
-	/* pass the pages buffer to driver at the last minute */
-	fw_set_page_data(buf, fw);
-	mutex_unlock(&fw_lock);
-	return 0;
-}
-
 /* called from request_firmware() and request_firmware_work_func() */
 static int
 _request_firmware(const struct firmware **firmware_p, const char *name,
-- 
2.11.0

[PATCH 5/5] 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@kernel.org>
---
 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 f28fbfab9c94..54fc4c42f126 100644
--- a/drivers/base/firmware_class.c
+++ b/drivers/base/firmware_class.c
@@ -1089,16 +1089,45 @@ 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;
 }
 
 static void kill_pending_fw_fallback_reqs(bool only_kill_custom)
@@ -1119,8 +1148,7 @@ static void kill_pending_fw_fallback_reqs(bool only_kill_custom)
 #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;
 }
@@ -1181,7 +1209,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)
@@ -1202,25 +1229,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))
@@ -1230,15 +1238,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

Re: [PATCH 1/5] firmware: share fw fallback killing on reboot/suspend

[Coelho, Luciano]

Hi Luis,


On Wed, 2017-03-29 at 20:24 -0700, Luis R. Rodriguez wrote:
> We kill pending fallback requests on suspend and reboot,
> the only difference is that on suspend we only kill custom
> fallback requests. Provide a wrapper that lets us customize
> the request with a flag.
> 
> This also lets us simplify the #ifdef'ery over the calls.
> 
> Signed-off-by: Luis R. Rodriguez <mcgrof@kernel.org>
> ---
>  drivers/base/firmware_class.c | 32 ++++++++++++++++----------------
>  1 file changed, 16 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
> index ac350c518e0c..d2e2d83aaf26 100644
> --- a/drivers/base/firmware_class.c
> +++ b/drivers/base/firmware_class.c
> @@ -562,16 +562,15 @@ static void fw_load_abort(struct firmware_priv *fw_priv)
>  
>  static LIST_HEAD(pending_fw_head);
>  
> -/* reboot notifier for avoid deadlock with usermode_lock */
>  static int fw_shutdown_notify(struct notifier_block *unused1,
>  			      unsigned long unused2, void *unused3)
>  {
> -	mutex_lock(&fw_lock);
> -	while (!list_empty(&pending_fw_head))
> -		__fw_load_abort(list_first_entry(&pending_fw_head,
> -					       struct firmware_buf,
> -					       pending_list));
> -	mutex_unlock(&fw_lock);
> +	/*
> +	 * Kill all pending fallback requests to avoid both stalling shutdown,
> +	 * and avoid a deadlock with the usermode_lock.
> +	 */
> +	kill_pending_fw_fallback_reqs(false);

You are calling this function before you declare it, this won't compile
when CONFIG_FW_LOADER_USER_HELPER is not set.



> +
>  	return NOTIFY_DONE;
>  }
>  
> @@ -1048,21 +1047,20 @@ static int fw_load_from_user_helper(struct firmware *firmware,
>  	return _request_firmware_load(fw_priv, opt_flags, timeout);
>  }
>  
> -#ifdef CONFIG_PM_SLEEP
> -/* kill pending requests without uevent to avoid blocking suspend */
> -static void kill_requests_without_uevent(void)
> +static void kill_pending_fw_fallback_reqs(bool only_kill_custom)
>  {
>  	struct firmware_buf *buf;
>  	struct firmware_buf *next;
>  
>  	mutex_lock(&fw_lock);
>  	list_for_each_entry_safe(buf, next, &pending_fw_head, pending_list) {
> -		if (!buf->need_uevent)
> +		if (!only_kill_custom)
> +			 __fw_load_abort(buf);
> +		else if (!buf->need_uevent)
>  			 __fw_load_abort(buf);
Why not use this?

	if (!only_kill_custom || !buf->need_uevent)
		__fw_load_abort(buf);

--
Cheers,
Luca.

Re: [PATCH 1/5] firmware: share fw fallback killing on reboot/suspend

[Luis R. Rodriguez]

On Thu, Apr 06, 2017 at 06:38:47AM +0000, Coelho, Luciano wrote:
> Hi Luis,
> > diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
> > index ac350c518e0c..d2e2d83aaf26 100644
> > --- a/drivers/base/firmware_class.c
> > +++ b/drivers/base/firmware_class.c
> > @@ -562,16 +562,15 @@ static void fw_load_abort(struct firmware_priv *fw_priv)
> >  
> >  static LIST_HEAD(pending_fw_head);
> >  
> > -/* reboot notifier for avoid deadlock with usermode_lock */
> >  static int fw_shutdown_notify(struct notifier_block *unused1,
> >  			      unsigned long unused2, void *unused3)
> >  {
> > -	mutex_lock(&fw_lock);
> > -	while (!list_empty(&pending_fw_head))
> > -		__fw_load_abort(list_first_entry(&pending_fw_head,
> > -					       struct firmware_buf,
> > -					       pending_list));
> > -	mutex_unlock(&fw_lock);
> > +	/*
> > +	 * Kill all pending fallback requests to avoid both stalling shutdown,
> > +	 * and avoid a deadlock with the usermode_lock.
> > +	 */
> > +	kill_pending_fw_fallback_reqs(false);
> 
> You are calling this function before you declare it, this won't compile
> when CONFIG_FW_LOADER_USER_HELPER is not set.

Actually allnoconfig compiles fine given the code in question is needed only when
CONFIG_FW_LOADER_USER_HELPER is loaded, however it does fail to compile when
CONFIG_FW_LOADER_USER_HELPER is set, and its odd that 0-day did not pick that up.

I've fixed this by moving kill_pending_fw_fallback_reqs() up above in a
separate patch first.

> > @@ -1048,21 +1047,20 @@ static int fw_load_from_user_helper(struct firmware *firmware,
> >  	return _request_firmware_load(fw_priv, opt_flags, timeout);
> >  }
> >  
> > -#ifdef CONFIG_PM_SLEEP
> > -/* kill pending requests without uevent to avoid blocking suspend */
> > -static void kill_requests_without_uevent(void)
> > +static void kill_pending_fw_fallback_reqs(bool only_kill_custom)
> >  {
> >  	struct firmware_buf *buf;
> >  	struct firmware_buf *next;
> >  
> >  	mutex_lock(&fw_lock);
> >  	list_for_each_entry_safe(buf, next, &pending_fw_head, pending_list) {
> > -		if (!buf->need_uevent)
> > +		if (!only_kill_custom)
> > +			 __fw_load_abort(buf);
> > +		else if (!buf->need_uevent)
> >  			 __fw_load_abort(buf);
> Why not use this?
> 
> 	if (!only_kill_custom || !buf->need_uevent)
> 		__fw_load_abort(buf);

Sure thing! Thanks for the review !

  Luis

[PATCH v2 0/6] firmware: move UMH locks onto fallback code

[Luis R. Rodriguez]

Greg,

This v2 follows the last iteration submitted [0] and addresses one
build failure case which for some reason was not caught by 0-day, and
it also adjusts some documentation to make 'make htmldocs' happy. This
and the next series are available in git form on my linux-next
20170501-driver-data-try2 branch [1]. In case folks want to play with
a more stable tree I've rebased this and the next series onto v4.11-rc8,
and is available on my linux [2] v4.11-rc8-driver-data-try3 branch.

I realize the merge window has opened -- so this can wait. We can just roll
with this once the that closes out and we're ready for new developemnt, no
critical fixes here. This is just paving the way for the next series which
introduces new features.

[0] https://lkml.kernel.org/r/20170330032450.17121-1-mcgrof@kernel.org
[1] https://git.kernel.org/pub/scm/linux/kernel/git/mcgrof/linux-next.git/log/?h=20170501-driver-data-try2
[2] https://git.kernel.org/pub/scm/linux/kernel/git/mcgrof/linux-next.git/log/?h=v4.11-rc8-driver-data-try3

Luis R. Rodriguez (6):
  firmware: move kill_requests_without_uevent() up above
  firmware: share fw fallback killing on reboot/suspend
  firmware: always enable the reboot notifier
  firmware: add sanity check on shutdown/suspend
  firmware: move assign_firmware_buf() further up
  firmware: move umh try locks into the umh code

 .../driver-api/firmware/request_firmware.rst       |  11 +
 drivers/base/firmware_class.c                      | 302 ++++++++++++++-------
 2 files changed, 212 insertions(+), 101 deletions(-)

-- 
2.11.0

[PATCH v2 1/6] firmware: move kill_requests_without_uevent() up above

[Luis R. Rodriguez]

This routine will used in functions declared earlier next. This
code shift has no functional changes, it will make subsequent
changes easier to read.

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

diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
index ac350c518e0c..900b139668e8 100644
--- a/drivers/base/firmware_class.c
+++ b/drivers/base/firmware_class.c
@@ -562,6 +562,22 @@ static void fw_load_abort(struct firmware_priv *fw_priv)
 
 static LIST_HEAD(pending_fw_head);
 
+#ifdef CONFIG_PM_SLEEP
+/* kill pending requests without uevent to avoid blocking suspend */
+static void kill_requests_without_uevent(void)
+{
+	struct firmware_buf *buf;
+	struct firmware_buf *next;
+
+	mutex_lock(&fw_lock);
+	list_for_each_entry_safe(buf, next, &pending_fw_head, pending_list) {
+		if (!buf->need_uevent)
+			 __fw_load_abort(buf);
+	}
+	mutex_unlock(&fw_lock);
+}
+#endif
+
 /* reboot notifier for avoid deadlock with usermode_lock */
 static int fw_shutdown_notify(struct notifier_block *unused1,
 			      unsigned long unused2, void *unused3)
@@ -1048,22 +1064,6 @@ static int fw_load_from_user_helper(struct firmware *firmware,
 	return _request_firmware_load(fw_priv, opt_flags, timeout);
 }
 
-#ifdef CONFIG_PM_SLEEP
-/* kill pending requests without uevent to avoid blocking suspend */
-static void kill_requests_without_uevent(void)
-{
-	struct firmware_buf *buf;
-	struct firmware_buf *next;
-
-	mutex_lock(&fw_lock);
-	list_for_each_entry_safe(buf, next, &pending_fw_head, pending_list) {
-		if (!buf->need_uevent)
-			 __fw_load_abort(buf);
-	}
-	mutex_unlock(&fw_lock);
-}
-#endif
-
 #else /* CONFIG_FW_LOADER_USER_HELPER */
 static inline int
 fw_load_from_user_helper(struct firmware *firmware, const char *name,
-- 
2.11.0

[PATCH v2 2/6] firmware: share fw fallback killing on reboot/suspend

[Luis R. Rodriguez]

We kill pending fallback requests on suspend and reboot,
the only difference is that on suspend we only kill custom
fallback requests. Provide a wrapper that lets us customize
the request with a flag.

This also lets us simplify the #ifdef'ery over the calls.

Signed-off-by: Luis R. Rodriguez <mcgrof@kernel.org>
---
 drivers/base/firmware_class.c | 29 ++++++++++++++---------------
 1 file changed, 14 insertions(+), 15 deletions(-)

diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
index 900b139668e8..58b661df6d34 100644
--- a/drivers/base/firmware_class.c
+++ b/drivers/base/firmware_class.c
@@ -562,32 +562,29 @@ static void fw_load_abort(struct firmware_priv *fw_priv)
 
 static LIST_HEAD(pending_fw_head);
 
-#ifdef CONFIG_PM_SLEEP
-/* kill pending requests without uevent to avoid blocking suspend */
-static void kill_requests_without_uevent(void)
+static void kill_pending_fw_fallback_reqs(bool only_kill_custom)
 {
 	struct firmware_buf *buf;
 	struct firmware_buf *next;
 
 	mutex_lock(&fw_lock);
 	list_for_each_entry_safe(buf, next, &pending_fw_head, pending_list) {
-		if (!buf->need_uevent)
+		if (!buf->need_uevent || !only_kill_custom)
 			 __fw_load_abort(buf);
 	}
 	mutex_unlock(&fw_lock);
 }
-#endif
 
 /* reboot notifier for avoid deadlock with usermode_lock */
 static int fw_shutdown_notify(struct notifier_block *unused1,
 			      unsigned long unused2, void *unused3)
 {
-	mutex_lock(&fw_lock);
-	while (!list_empty(&pending_fw_head))
-		__fw_load_abort(list_first_entry(&pending_fw_head,
-					       struct firmware_buf,
-					       pending_list));
-	mutex_unlock(&fw_lock);
+	/*
+	 * Kill all pending fallback requests to avoid both stalling shutdown,
+	 * and avoid a deadlock with the usermode_lock.
+	 */
+	kill_pending_fw_fallback_reqs(false);
+
 	return NOTIFY_DONE;
 }
 
@@ -1073,9 +1070,7 @@ fw_load_from_user_helper(struct firmware *firmware, const char *name,
 	return -ENOENT;
 }
 
-#ifdef CONFIG_PM_SLEEP
-static inline void kill_requests_without_uevent(void) { }
-#endif
+static inline void kill_pending_fw_fallback_reqs(bool only_kill_custom) { }
 
 #endif /* CONFIG_FW_LOADER_USER_HELPER */
 
@@ -1724,7 +1719,11 @@ static int fw_pm_notify(struct notifier_block *notify_block,
 	case PM_HIBERNATION_PREPARE:
 	case PM_SUSPEND_PREPARE:
 	case PM_RESTORE_PREPARE:
-		kill_requests_without_uevent();
+		/*
+		 * kill pending fallback requests with a custom fallback
+		 * to avoid stalling suspend.
+		 */
+		kill_pending_fw_fallback_reqs(true);
 		device_cache_fw_images();
 		break;
 
-- 
2.11.0

[PATCH v2 3/6] firmware: always enable the reboot notifier

[Luis R. Rodriguez]

Now that we've have proper wrappers for the fallback mechanism
we can easily share the reboot notifier for the firmware_class
at all times.

This change will make subsequent modifications to the reboot
notifier easier to review.

Signed-off-by: Luis R. Rodriguez <mcgrof@kernel.org>
---
 drivers/base/firmware_class.c | 37 ++++++++++++++++++-------------------
 1 file changed, 18 insertions(+), 19 deletions(-)

diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
index 58b661df6d34..dd9b7f3d0927 100644
--- a/drivers/base/firmware_class.c
+++ b/drivers/base/firmware_class.c
@@ -575,23 +575,6 @@ static void kill_pending_fw_fallback_reqs(bool only_kill_custom)
 	mutex_unlock(&fw_lock);
 }
 
-/* reboot notifier for avoid deadlock with usermode_lock */
-static int fw_shutdown_notify(struct notifier_block *unused1,
-			      unsigned long unused2, void *unused3)
-{
-	/*
-	 * Kill all pending fallback requests to avoid both stalling shutdown,
-	 * and avoid a deadlock with the usermode_lock.
-	 */
-	kill_pending_fw_fallback_reqs(false);
-
-	return NOTIFY_DONE;
-}
-
-static struct notifier_block fw_shutdown_nb = {
-	.notifier_call = fw_shutdown_notify,
-};
-
 static ssize_t timeout_show(struct class *class, struct class_attribute *attr,
 			    char *buf)
 {
@@ -1782,11 +1765,27 @@ static void __init fw_cache_init(void)
 #endif
 }
 
+static int fw_shutdown_notify(struct notifier_block *unused1,
+			      unsigned long unused2, void *unused3)
+{
+	/*
+	 * Kill all pending fallback requests to avoid both stalling shutdown,
+	 * and avoid a deadlock with the usermode_lock.
+	 */
+	kill_pending_fw_fallback_reqs(false);
+
+	return NOTIFY_DONE;
+}
+
+static struct notifier_block fw_shutdown_nb = {
+	.notifier_call = fw_shutdown_notify,
+};
+
 static int __init firmware_class_init(void)
 {
 	fw_cache_init();
-#ifdef CONFIG_FW_LOADER_USER_HELPER
 	register_reboot_notifier(&fw_shutdown_nb);
+#ifdef CONFIG_FW_LOADER_USER_HELPER
 	return class_register(&firmware_class);
 #else
 	return 0;
@@ -1799,8 +1798,8 @@ static void __exit firmware_class_exit(void)
 	unregister_syscore_ops(&fw_syscore_ops);
 	unregister_pm_notifier(&fw_cache.pm_notify);
 #endif
-#ifdef CONFIG_FW_LOADER_USER_HELPER
 	unregister_reboot_notifier(&fw_shutdown_nb);
+#ifdef CONFIG_FW_LOADER_USER_HELPER
 	class_unregister(&firmware_class);
 #endif
 }
-- 
2.11.0

[PATCH v2 4/6] firmware: add sanity check on shutdown/suspend

[Luis R. Rodriguez]

The firmware API should not be used after we go to suspend
and after we reboot/halt. The suspend/resume case is a bit
complex, so this documents that so things are clearer.

We want to know about users of the API in incorrect places so
that their callers are corrected, so this also adds a warn
for those cases.

Signed-off-by: Luis R. Rodriguez <mcgrof@kernel.org>
---
 .../driver-api/firmware/request_firmware.rst       | 11 +++
 drivers/base/firmware_class.c                      | 99 ++++++++++++++++++++++
 2 files changed, 110 insertions(+)

diff --git a/Documentation/driver-api/firmware/request_firmware.rst b/Documentation/driver-api/firmware/request_firmware.rst
index cc0aea880824..1c2c4967cd43 100644
--- a/Documentation/driver-api/firmware/request_firmware.rst
+++ b/Documentation/driver-api/firmware/request_firmware.rst
@@ -44,6 +44,17 @@ request_firmware_nowait
 .. kernel-doc:: drivers/base/firmware_class.c
    :functions: request_firmware_nowait
 
+Considerations for suspend and resume
+=====================================
+
+During suspend and resume only the built-in firmware and the firmware cache
+elements of the firmware API can be used. This is managed by fw_pm_notify().
+
+fw_pm_notify
+------------
+.. kernel-doc:: drivers/base/firmware_class.c
+   :functions: fw_pm_notify
+
 request firmware API expected driver use
 ========================================
 
diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
index dd9b7f3d0927..a272c8662212 100644
--- a/drivers/base/firmware_class.c
+++ b/drivers/base/firmware_class.c
@@ -260,6 +260,38 @@ static int fw_cache_piggyback_on_request(const char *name);
  * guarding for corner cases a global lock should be OK */
 static DEFINE_MUTEX(fw_lock);
 
+static bool __enable_firmware = false;
+
+static void enable_firmware(void)
+{
+	mutex_lock(&fw_lock);
+	__enable_firmware = true;
+	mutex_unlock(&fw_lock);
+}
+
+static void disable_firmware(void)
+{
+	mutex_lock(&fw_lock);
+	__enable_firmware = false;
+	mutex_unlock(&fw_lock);
+}
+
+/*
+ * When disabled only the built-in firmware and the firmware cache will be
+ * used to look for firmware.
+ */
+static bool firmware_enabled(void)
+{
+	bool enabled = false;
+
+	mutex_lock(&fw_lock);
+	if (__enable_firmware)
+		enabled = true;
+	mutex_unlock(&fw_lock);
+
+	return enabled;
+}
+
 static struct firmware_cache fw_cache;
 
 static struct firmware_buf *__allocate_fw_buf(const char *fw_name,
@@ -1163,6 +1195,12 @@ _request_firmware(const struct firmware **firmware_p, const char *name,
 	if (ret <= 0) /* error or already assigned */
 		goto out;
 
+	if (!firmware_enabled()) {
+		WARN(1, "firmware request while host is not available\n");
+		ret = -EHOSTDOWN;
+		goto out;
+	}
+
 	ret = 0;
 	timeout = firmware_loading_timeout();
 	if (opt_flags & FW_OPT_NOWAIT) {
@@ -1695,6 +1733,62 @@ static void device_uncache_fw_images_delay(unsigned long delay)
 			   msecs_to_jiffies(delay));
 }
 
+/**
+ * fw_pm_notify - notifier for suspend/resume
+ * @notify_block: unused
+ * @mode: mode we are switching to
+ * @unused: unused
+ *
+ * Used to modify the firmware_class state as we move in between states.
+ * The firmware_class implements a firmware cache to enable device driver
+ * to fetch firmware upon resume before the root filesystem is ready. We
+ * disable API calls which do not use the built-in firmware or the firmware
+ * cache when we know these calls will not work.
+ *
+ * The inner logic behind all this is a bit complex so it is worth summarizing
+ * the kernel's own suspend/resume process with context and focus on how this
+ * can impact the firmware API.
+ *
+ * First a review on how we go to suspend::
+ *
+ *	pm_suspend() --> enter_state() -->
+ *	sys_sync()
+ *	suspend_prepare() -->
+ *		__pm_notifier_call_chain(PM_SUSPEND_PREPARE, ...);
+ *		suspend_freeze_processes() -->
+ *			freeze_processes() -->
+ *				__usermodehelper_set_disable_depth(UMH_DISABLED);
+ *				freeze all tasks ...
+ *			freeze_kernel_threads()
+ *	suspend_devices_and_enter() -->
+ *		dpm_suspend_start() -->
+ *				dpm_prepare()
+ *				dpm_suspend()
+ *		suspend_enter()  -->
+ *			platform_suspend_prepare()
+ *			dpm_suspend_late()
+ *			freeze_enter()
+ *			syscore_suspend()
+ *
+ * When we resume we bail out of a loop from suspend_devices_and_enter() and
+ * unwind back out to the caller enter_state() where we were before as follows::
+ *
+ * 	enter_state() -->
+ *	suspend_devices_and_enter() --> (bail from loop)
+ *		dpm_resume_end() -->
+ *			dpm_resume()
+ *			dpm_complete()
+ *	suspend_finish() -->
+ *		suspend_thaw_processes() -->
+ *			thaw_processes() -->
+ *				__usermodehelper_set_disable_depth(UMH_FREEZING);
+ *				thaw_workqueues();
+ *				thaw all processes ...
+ *				usermodehelper_enable();
+ *		pm_notifier_call_chain(PM_POST_SUSPEND);
+ *
+ * fw_pm_notify() works through pm_notifier_call_chain().
+ */
 static int fw_pm_notify(struct notifier_block *notify_block,
 			unsigned long mode, void *unused)
 {
@@ -1708,6 +1802,7 @@ static int fw_pm_notify(struct notifier_block *notify_block,
 		 */
 		kill_pending_fw_fallback_reqs(true);
 		device_cache_fw_images();
+		disable_firmware();
 		break;
 
 	case PM_POST_SUSPEND:
@@ -1720,6 +1815,7 @@ static int fw_pm_notify(struct notifier_block *notify_block,
 		mutex_lock(&fw_lock);
 		fw_cache.state = FW_LOADER_NO_CACHE;
 		mutex_unlock(&fw_lock);
+		enable_firmware();
 
 		device_uncache_fw_images_delay(10 * MSEC_PER_SEC);
 		break;
@@ -1768,6 +1864,7 @@ static void __init fw_cache_init(void)
 static int fw_shutdown_notify(struct notifier_block *unused1,
 			      unsigned long unused2, void *unused3)
 {
+	disable_firmware();
 	/*
 	 * Kill all pending fallback requests to avoid both stalling shutdown,
 	 * and avoid a deadlock with the usermode_lock.
@@ -1783,6 +1880,7 @@ static struct notifier_block fw_shutdown_nb = {
 
 static int __init firmware_class_init(void)
 {
+	enable_firmware();
 	fw_cache_init();
 	register_reboot_notifier(&fw_shutdown_nb);
 #ifdef CONFIG_FW_LOADER_USER_HELPER
@@ -1794,6 +1892,7 @@ static int __init firmware_class_init(void)
 
 static void __exit firmware_class_exit(void)
 {
+	disable_firmware();
 #ifdef CONFIG_PM_SLEEP
 	unregister_syscore_ops(&fw_syscore_ops);
 	unregister_pm_notifier(&fw_cache.pm_notify);
-- 
2.11.0

[PATCH v2 5/6] firmware: move assign_firmware_buf() further up

[Luis R. Rodriguez]

This will make subsequent changes easier to read.

Signed-off-by: Luis R. Rodriguez <mcgrof@kernel.org>
---
 drivers/base/firmware_class.c | 77 +++++++++++++++++++++----------------------
 1 file changed, 38 insertions(+), 39 deletions(-)

diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
index a272c8662212..7722943025f3 100644
--- a/drivers/base/firmware_class.c
+++ b/drivers/base/firmware_class.c
@@ -555,6 +555,44 @@ static int fw_add_devm_name(struct device *dev, const char *name)
 }
 #endif
 
+static int assign_firmware_buf(struct firmware *fw, struct device *device,
+			       unsigned int opt_flags)
+{
+	struct firmware_buf *buf = fw->priv;
+
+	mutex_lock(&fw_lock);
+	if (!buf->size || fw_state_is_aborted(&buf->fw_st)) {
+		mutex_unlock(&fw_lock);
+		return -ENOENT;
+	}
+
+	/*
+	 * add firmware name into devres list so that we can auto cache
+	 * and uncache firmware for device.
+	 *
+	 * device may has been deleted already, but the problem
+	 * should be fixed in devres or driver core.
+	 */
+	/* don't cache firmware handled without uevent */
+	if (device && (opt_flags & FW_OPT_UEVENT) &&
+	    !(opt_flags & FW_OPT_NOCACHE))
+		fw_add_devm_name(device, buf->fw_id);
+
+	/*
+	 * After caching firmware image is started, let it piggyback
+	 * on request firmware.
+	 */
+	if (!(opt_flags & FW_OPT_NOCACHE) &&
+	    buf->fwc->state == FW_LOADER_START_CACHE) {
+		if (fw_cache_piggyback_on_request(buf->fw_id))
+			kref_get(&buf->ref);
+	}
+
+	/* pass the pages buffer to driver at the last minute */
+	fw_set_page_data(buf, fw);
+	mutex_unlock(&fw_lock);
+	return 0;
+}
 
 /*
  * user-mode helper code
@@ -1134,45 +1172,6 @@ _request_firmware_prepare(struct firmware **firmware_p, const char *name,
 	return 1; /* need to load */
 }
 
-static int assign_firmware_buf(struct firmware *fw, struct device *device,
-			       unsigned int opt_flags)
-{
-	struct firmware_buf *buf = fw->priv;
-
-	mutex_lock(&fw_lock);
-	if (!buf->size || fw_state_is_aborted(&buf->fw_st)) {
-		mutex_unlock(&fw_lock);
-		return -ENOENT;
-	}
-
-	/*
-	 * add firmware name into devres list so that we can auto cache
-	 * and uncache firmware for device.
-	 *
-	 * device may has been deleted already, but the problem
-	 * should be fixed in devres or driver core.
-	 */
-	/* don't cache firmware handled without uevent */
-	if (device && (opt_flags & FW_OPT_UEVENT) &&
-	    !(opt_flags & FW_OPT_NOCACHE))
-		fw_add_devm_name(device, buf->fw_id);
-
-	/*
-	 * After caching firmware image is started, let it piggyback
-	 * on request firmware.
-	 */
-	if (!(opt_flags & FW_OPT_NOCACHE) &&
-	    buf->fwc->state == FW_LOADER_START_CACHE) {
-		if (fw_cache_piggyback_on_request(buf->fw_id))
-			kref_get(&buf->ref);
-	}
-
-	/* pass the pages buffer to driver at the last minute */
-	fw_set_page_data(buf, fw);
-	mutex_unlock(&fw_lock);
-	return 0;
-}
-
 /* called from request_firmware() and request_firmware_work_func() */
 static int
 _request_firmware(const struct firmware **firmware_p, const char *name,
-- 
2.11.0

[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@kernel.org>
---
 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