[PATCH v4 0/4] firmware: encapsulate firmware loading status

[PATCH v4 0/4] firmware: encapsulate firmware loading status

[Daniel Wagner]

From: Daniel Wagner <daniel.wagner@bmw-carit.de>

Hi,

This is version depends on Luis' '[PATCH v4 0/5] firmware: add SmPL
grammar to avoid issues' series [1]. Because of that I was able to get
rid of loading_timeout and firmware_loading_timeout() in the
!FW_LOADER_USER_HELPER_FALLBACK case.

cheers,
daniel

This series depends on
[1] http://marc.info/?l=linux-kernel&m=147320896231418&w=2

changes since v3:
  - added 'firmware: Move umh locking code into
    fw_load_from_user_helper()'
  - dropped loading_tiemout and firmware_loading_time() for
    !CONFG_FW_LOADER_USER_HELPER
  - rebased on Luis patches

changes since v2:
  - more splitting out
    - first patch factors out all the bit ops into fw_status
    - second patch gets rid of the bit ops
    - third get rid of fw_lock by using swait

changes since v1:
  - moved swait change into its own patch
  - added ifdef section for FW_LOADER_USER_HELPER_FALLBACK
  - updated commit message highlighting the mutex usage drop a bit

  https://lkml.org/lkml/2016/8/4/239

Cc: Ming Lei <ming.lei@canonical.com>
Cc: Luis R. Rodriguez <mcgrof@kernel.org>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

Daniel Wagner (4):
  firmware: Move umh locking code into fw_load_from_user_helper()
  firmware: encapsulate firmware loading status
  firmware: Drop bit ops in favor of simple state machine
  firmware: Do not use fw_lock for fw_status protection

 drivers/base/firmware_class.c | 202 +++++++++++++++++++++++++-----------------
 1 file changed, 119 insertions(+), 83 deletions(-)

-- 
2.7.4

[PATCH v4 1/4] firmware: Move umh locking code into fw_load_from_user_helper()

[Daniel Wagner]

From: Daniel Wagner <daniel.wagner@bmw-carit.de>

When we load the firmware directly we don't need to take the umh
lock. So move this part inside fw_load_from_user_helper which is only
available when CONFIG_FW_LOADER_USER_HELPER is set.

This avoids a dependency on firmware_loading_timeout() even when
!CONFIG_FW_LOADER_UER_HELPER.

The usermodehelper locking code was added by b298d289c792 ("PM / Sleep:
Fix freezer failures due to racy usermodehelper_is_disabled()").

Signed-off-by: Daniel Wagner <daniel.wagner@bmw-carit.de>
Cc: Ming Lei <ming.lei@canonical.com>
Cc: Luis R. Rodriguez <mcgrof@kernel.org>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
 drivers/base/firmware_class.c | 52 +++++++++++++++++++++++--------------------
 1 file changed, 28 insertions(+), 24 deletions(-)

diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
index 960f8f7..d4fee06 100644
--- a/drivers/base/firmware_class.c
+++ b/drivers/base/firmware_class.c
@@ -981,13 +981,38 @@ static int fw_load_from_user_helper(struct firmware *firmware,
 				    unsigned int opt_flags, long timeout)
 {
 	struct firmware_priv *fw_priv;
+	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 release_lock;
+	}
 
 	fw_priv->buf = firmware->priv;
-	return _request_firmware_load(fw_priv, opt_flags, timeout);
+	ret = _request_firmware_load(fw_priv, opt_flags, timeout);
+
+release_lock:
+	usermodehelper_read_unlock();
+
+	return ret;
 }
 
 #ifdef CONFIG_PM_SLEEP
@@ -1150,25 +1175,6 @@ _request_firmware(const struct firmware **firmware_p, const char *name,
 	if (ret <= 0) /* error or already assigned */
 		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))
@@ -1185,8 +1191,6 @@ _request_firmware(const struct firmware **firmware_p, const char *name,
 	if (!ret)
 		ret = assign_firmware_buf(fw, device, opt_flags);
 
-	usermodehelper_read_unlock();
-
  out:
 	if (ret < 0) {
 		release_firmware(fw);
-- 
2.7.4

[PATCH v4 2/4] firmware: encapsulate firmware loading status

[Daniel Wagner]

From: Daniel Wagner <daniel.wagner@bmw-carit.de>

The firmware user helper code tracks the current state of the loading
process via unsigned long status and a complection in struct
firmware_buf. We only need this for the usermode helper as such we can
encapsulate all this data into its own data structure.

Signed-off-by: Daniel Wagner <daniel.wagner@bmw-carit.de>
Cc: Ming Lei <ming.lei@canonical.com>
Cc: Luis R. Rodriguez <mcgrof@kernel.org>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
 drivers/base/firmware_class.c | 123 ++++++++++++++++++++++++++++++------------
 1 file changed, 88 insertions(+), 35 deletions(-)

diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
index d4fee06..b11fbb0 100644
--- a/drivers/base/firmware_class.c
+++ b/drivers/base/firmware_class.c
@@ -91,10 +91,13 @@ static inline bool fw_is_builtin_firmware(const struct firmware *fw)
 }
 #endif
 
+#ifdef CONFIG_FW_LOADER_USER_HELPER
+
 enum {
+	FW_STATUS_UNKNOWN,
 	FW_STATUS_LOADING,
 	FW_STATUS_DONE,
-	FW_STATUS_ABORT,
+	FW_STATUS_ABORTED,
 };
 
 static int loading_timeout = 60;	/* In seconds */
@@ -104,6 +107,69 @@ static inline long firmware_loading_timeout(void)
 	return loading_timeout > 0 ? loading_timeout * HZ : MAX_JIFFY_OFFSET;
 }
 
+struct fw_status {
+	unsigned long status;
+	struct completion completion;
+};
+
+static void fw_status_init(struct fw_status *fw_st)
+{
+	fw_st->status = FW_STATUS_UNKNOWN;
+	init_completion(&fw_st->completion);
+}
+
+static int __fw_status_check(struct fw_status *fw_st, unsigned long status)
+{
+	return test_bit(status, &fw_st->status);
+}
+
+static int fw_status_wait_timeout(struct fw_status *fw_st, long timeout)
+{
+	int ret;
+
+	ret = wait_for_completion_interruptible_timeout(&fw_st->completion,
+							timeout);
+	if (ret == 0 && test_bit(FW_STATUS_ABORTED, &fw_st->status))
+		return -ENOENT;
+
+	return ret;
+}
+
+static void __fw_status_set(struct fw_status *fw_st,
+			  unsigned long status)
+{
+	set_bit(status, &fw_st->status);
+
+	if (status == FW_STATUS_DONE ||
+			status == FW_STATUS_ABORTED) {
+		clear_bit(FW_STATUS_LOADING, &fw_st->status);
+		complete_all(&fw_st->completion);
+	}
+}
+
+#define fw_status_start(fw_st)					\
+	__fw_status_set(fw_st, FW_STATUS_LOADING)
+#define fw_status_done(fw_st)					\
+	__fw_status_set(fw_st, FW_STATUS_DONE)
+#define fw_status_aborted(fw_st)				\
+	__fw_status_set(fw_st, FW_STATUS_ABORTED)
+#define fw_status_is_loading(fw_st)				\
+	__fw_status_check(fw_st, FW_STATUS_LOADING)
+#define fw_status_is_done(fw_st)				\
+	__fw_status_check(fw_st, FW_STATUS_DONE)
+#define fw_status_is_aborted(fw_st)				\
+	__fw_status_check(fw_st, FW_STATUS_ABORTED)
+
+#else /* CONFIG_FW_LOADER_USER_HELPER */
+
+#define fw_status_wait_timeout(fw_st, long)	0
+
+#define fw_status_done(fw_st)
+#define fw_status_is_done(fw_st)		true
+#define fw_status_is_aborted(fw_st)		false
+
+#endif /* !CONFIG_FW_LOADER_USER_HELPER */
+
 /* firmware behavior options */
 #define FW_OPT_UEVENT	(1U << 0)
 #define FW_OPT_NOWAIT	(1U << 1)
@@ -145,13 +211,12 @@ struct firmware_cache {
 struct firmware_buf {
 	struct kref ref;
 	struct list_head list;
-	struct completion completion;
 	struct firmware_cache *fwc;
-	unsigned long status;
 	void *data;
 	size_t size;
 	size_t allocated_size;
 #ifdef CONFIG_FW_LOADER_USER_HELPER
+	struct fw_status fw_st;
 	bool is_paged_buf;
 	bool need_uevent;
 	struct page **pages;
@@ -205,8 +270,8 @@ static struct firmware_buf *__allocate_fw_buf(const char *fw_name,
 	buf->fwc = fwc;
 	buf->data = dbuf;
 	buf->allocated_size = size;
-	init_completion(&buf->completion);
 #ifdef CONFIG_FW_LOADER_USER_HELPER
+	fw_status_init(&buf->fw_st);
 	INIT_LIST_HEAD(&buf->pending_list);
 #endif
 
@@ -309,8 +374,7 @@ static void fw_finish_direct_load(struct device *device,
 				  struct firmware_buf *buf)
 {
 	mutex_lock(&fw_lock);
-	set_bit(FW_STATUS_DONE, &buf->status);
-	complete_all(&buf->completion);
+	fw_status_done(&buf->fw_st);
 	mutex_unlock(&fw_lock);
 }
 
@@ -478,12 +542,11 @@ static void __fw_load_abort(struct firmware_buf *buf)
 	 * There is a small window in which user can write to 'loading'
 	 * between loading done and disappearance of 'loading'
 	 */
-	if (test_bit(FW_STATUS_DONE, &buf->status))
+	if (fw_status_is_done(&buf->fw_st))
 		return;
 
 	list_del_init(&buf->pending_list);
-	set_bit(FW_STATUS_ABORT, &buf->status);
-	complete_all(&buf->completion);
+	fw_status_aborted(&buf->fw_st);
 }
 
 static void fw_load_abort(struct firmware_priv *fw_priv)
@@ -496,9 +559,6 @@ static void fw_load_abort(struct firmware_priv *fw_priv)
 	fw_priv->buf = NULL;
 }
 
-#define is_fw_load_aborted(buf)	\
-	test_bit(FW_STATUS_ABORT, &(buf)->status)
-
 static LIST_HEAD(pending_fw_head);
 
 /* reboot notifier for avoid deadlock with usermode_lock */
@@ -598,7 +658,7 @@ static ssize_t firmware_loading_show(struct device *dev,
 
 	mutex_lock(&fw_lock);
 	if (fw_priv->buf)
-		loading = test_bit(FW_STATUS_LOADING, &fw_priv->buf->status);
+		loading = fw_status_is_loading(&fw_priv->buf->fw_st);
 	mutex_unlock(&fw_lock);
 
 	return sprintf(buf, "%d\n", loading);
@@ -653,23 +713,20 @@ static ssize_t firmware_loading_store(struct device *dev,
 	switch (loading) {
 	case 1:
 		/* discarding any previous partial load */
-		if (!test_bit(FW_STATUS_DONE, &fw_buf->status)) {
+		if (!fw_status_is_done(&fw_buf->fw_st)) {
 			for (i = 0; i < fw_buf->nr_pages; i++)
 				__free_page(fw_buf->pages[i]);
 			vfree(fw_buf->pages);
 			fw_buf->pages = NULL;
 			fw_buf->page_array_size = 0;
 			fw_buf->nr_pages = 0;
-			set_bit(FW_STATUS_LOADING, &fw_buf->status);
+			fw_status_start(&fw_buf->fw_st);
 		}
 		break;
 	case 0:
-		if (test_bit(FW_STATUS_LOADING, &fw_buf->status)) {
+		if (fw_status_is_loading(&fw_buf->fw_st)) {
 			int rc;
 
-			set_bit(FW_STATUS_DONE, &fw_buf->status);
-			clear_bit(FW_STATUS_LOADING, &fw_buf->status);
-
 			/*
 			 * Several loading requests may be pending on
 			 * one same firmware buf, so let all requests
@@ -691,10 +748,11 @@ static ssize_t firmware_loading_store(struct device *dev,
 			 */
 			list_del_init(&fw_buf->pending_list);
 			if (rc) {
-				set_bit(FW_STATUS_ABORT, &fw_buf->status);
+				fw_status_aborted(&fw_buf->fw_st);
 				written = rc;
+			} else {
+				fw_status_done(&fw_buf->fw_st);
 			}
-			complete_all(&fw_buf->completion);
 			break;
 		}
 		/* fallthrough */
@@ -702,7 +760,7 @@ static ssize_t firmware_loading_store(struct device *dev,
 		dev_err(dev, "%s: unexpected value (%d)\n", __func__, loading);
 		/* fallthrough */
 	case -1:
-		fw_load_abort(fw_priv);
+		fw_status_aborted(&fw_buf->fw_st);
 		break;
 	}
 out:
@@ -755,7 +813,7 @@ static ssize_t firmware_data_read(struct file *filp, struct kobject *kobj,
 
 	mutex_lock(&fw_lock);
 	buf = fw_priv->buf;
-	if (!buf || test_bit(FW_STATUS_DONE, &buf->status)) {
+	if (!buf || fw_status_is_done(&buf->fw_st)) {
 		ret_count = -ENODEV;
 		goto out;
 	}
@@ -842,7 +900,7 @@ static ssize_t firmware_data_write(struct file *filp, struct kobject *kobj,
 
 	mutex_lock(&fw_lock);
 	buf = fw_priv->buf;
-	if (!buf || test_bit(FW_STATUS_DONE, &buf->status)) {
+	if (!buf || fw_status_is_done(&buf->fw_st)) {
 		retval = -ENODEV;
 		goto out;
 	}
@@ -955,8 +1013,7 @@ static int _request_firmware_load(struct firmware_priv *fw_priv,
 		timeout = MAX_JIFFY_OFFSET;
 	}
 
-	retval = wait_for_completion_interruptible_timeout(&buf->completion,
-			timeout);
+	retval = fw_status_wait_timeout(&buf->fw_st, timeout);
 	if (retval == -ERESTARTSYS || !retval) {
 		mutex_lock(&fw_lock);
 		fw_load_abort(fw_priv);
@@ -965,7 +1022,7 @@ static int _request_firmware_load(struct firmware_priv *fw_priv,
 		retval = 0;
 	}
 
-	if (is_fw_load_aborted(buf))
+	if (fw_status_is_aborted(&buf->fw_st))
 		retval = -EAGAIN;
 	else if (buf->is_paged_buf && !buf->data)
 		retval = -ENOMEM;
@@ -1040,29 +1097,25 @@ fw_load_from_user_helper(struct firmware *firmware, const char *name,
 	return -ENOENT;
 }
 
-/* No abort during direct loading */
-#define is_fw_load_aborted(buf) false
-
 #ifdef CONFIG_PM_SLEEP
 static inline void kill_requests_without_uevent(void) { }
 #endif
 
 #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 (!test_bit(FW_STATUS_DONE, &buf->status)) {
-		if (is_fw_load_aborted(buf)) {
+	while (!fw_status_is_done(&buf->fw_st)) {
+		if (fw_status_is_aborted(&buf->fw_st)) {
 			ret = -ENOENT;
 			break;
 		}
 		mutex_unlock(&fw_lock);
-		ret = wait_for_completion_interruptible(&buf->completion);
+		ret = fw_status_wait_timeout(&buf->fw_st, 0);
 		mutex_lock(&fw_lock);
 	}
 	mutex_unlock(&fw_lock);
@@ -1120,7 +1173,7 @@ static int assign_firmware_buf(struct firmware *fw, struct device *device,
 	struct firmware_buf *buf = fw->priv;
 
 	mutex_lock(&fw_lock);
-	if (!buf->size || is_fw_load_aborted(buf)) {
+	if (!buf->size || fw_status_is_aborted(&buf->fw_st)) {
 		mutex_unlock(&fw_lock);
 		return -ENOENT;
 	}
-- 
2.7.4

[PATCH v4 3/4] firmware: Drop bit ops in favor of simple state machine

[Daniel Wagner]

From: Daniel Wagner <daniel.wagner@bmw-carit.de>

We track the state of the loading with bit ops. Since the state machine
has only a couple of states and there are only a few simple state
transition we can model this simplify.

	   UNKNOWN -> LOADING -> DONE | ABORTED

Signed-off-by: Daniel Wagner <daniel.wagner@bmw-carit.de>
Cc: Ming Lei <ming.lei@canonical.com>
Cc: Luis R. Rodriguez <mcgrof@kernel.org>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
 drivers/base/firmware_class.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
index b11fbb0..7757c03 100644
--- a/drivers/base/firmware_class.c
+++ b/drivers/base/firmware_class.c
@@ -120,16 +120,18 @@ static void fw_status_init(struct fw_status *fw_st)
 
 static int __fw_status_check(struct fw_status *fw_st, unsigned long status)
 {
-	return test_bit(status, &fw_st->status);
+	return fw_st->status == status;
 }
 
 static int fw_status_wait_timeout(struct fw_status *fw_st, long timeout)
 {
+	unsigned long status;
 	int ret;
 
 	ret = wait_for_completion_interruptible_timeout(&fw_st->completion,
 							timeout);
-	if (ret == 0 && test_bit(FW_STATUS_ABORTED, &fw_st->status))
+	status = READ_ONCE(fw_st->status);
+	if (ret == 0 && status == FW_STATUS_ABORTED)
 		return -ENOENT;
 
 	return ret;
@@ -138,13 +140,11 @@ static int fw_status_wait_timeout(struct fw_status *fw_st, long timeout)
 static void __fw_status_set(struct fw_status *fw_st,
 			  unsigned long status)
 {
-	set_bit(status, &fw_st->status);
+	WRITE_ONCE(fw_st->status, status);
 
 	if (status == FW_STATUS_DONE ||
-			status == FW_STATUS_ABORTED) {
-		clear_bit(FW_STATUS_LOADING, &fw_st->status);
+			status == FW_STATUS_ABORTED)
 		complete_all(&fw_st->completion);
-	}
 }
 
 #define fw_status_start(fw_st)					\
-- 
2.7.4

[PATCH v4 4/4] firmware: Do not use fw_lock for fw_status protection

[Daniel Wagner]

From: Daniel Wagner <daniel.wagner@bmw-carit.de>

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_status can be used without needing the fw_lock to protect
its state transition and wake ups.

fw_status 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.

Signed-off-by: Daniel Wagner <daniel.wagner@bmw-carit.de>
Cc: Ming Lei <ming.lei@canonical.com>
Cc: Luis R. Rodriguez <mcgrof@kernel.org>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
 drivers/base/firmware_class.c | 53 +++++++++++++------------------------------
 1 file changed, 16 insertions(+), 37 deletions(-)

diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
index 7757c03..05fe6ec 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>
 
@@ -109,13 +110,13 @@ static inline long firmware_loading_timeout(void)
 
 struct fw_status {
 	unsigned long status;
-	struct completion completion;
+	struct swait_queue_head wq;
 };
 
 static void fw_status_init(struct fw_status *fw_st)
 {
 	fw_st->status = FW_STATUS_UNKNOWN;
-	init_completion(&fw_st->completion);
+	init_swait_queue_head(&fw_st->wq);
 }
 
 static int __fw_status_check(struct fw_status *fw_st, unsigned long status)
@@ -123,15 +124,19 @@ static int __fw_status_check(struct fw_status *fw_st, unsigned long status)
 	return fw_st->status == status;
 }
 
+static inline bool __fw_status_is_done(unsigned long status)
+{
+	return status == FW_STATUS_DONE ||
+	       status == FW_STATUS_ABORTED;
+}
+
 static int fw_status_wait_timeout(struct fw_status *fw_st, long timeout)
 {
-	unsigned long status;
 	int ret;
-
-	ret = wait_for_completion_interruptible_timeout(&fw_st->completion,
-							timeout);
-	status = READ_ONCE(fw_st->status);
-	if (ret == 0 && status == FW_STATUS_ABORTED)
+	ret = swait_event_interruptible_timeout(fw_st->wq,
+				__fw_status_is_done(READ_ONCE(fw_st->status)),
+				timeout);
+	if (ret == 0 && fw_st->status == FW_STATUS_ABORTED)
 		return -ENOENT;
 
 	return ret;
@@ -144,7 +149,7 @@ static void __fw_status_set(struct fw_status *fw_st,
 
 	if (status == FW_STATUS_DONE ||
 			status == FW_STATUS_ABORTED)
-		complete_all(&fw_st->completion);
+		swake_up(&fw_st->wq);
 }
 
 #define fw_status_start(fw_st)					\
@@ -370,14 +375,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_status_done(&buf->fw_st);
-	mutex_unlock(&fw_lock);
-}
-
 static int
 fw_get_filesystem_firmware(struct device *device, struct firmware_buf *buf)
 {
@@ -424,7 +421,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_status_done(&buf->fw_st);
 		break;
 	}
 	__putname(path);
@@ -1103,24 +1100,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_status_is_done(&buf->fw_st)) {
-		if (fw_status_is_aborted(&buf->fw_st)) {
-			ret = -ENOENT;
-			break;
-		}
-		mutex_unlock(&fw_lock);
-		ret = fw_status_wait_timeout(&buf->fw_st, 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,
@@ -1155,7 +1134,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_status_wait_timeout(&buf->fw_st, 0);
 		if (!ret) {
 			fw_set_page_data(buf, firmware);
 			return 0; /* assigned */
-- 
2.7.4

Re: [PATCH v4 1/4] firmware: Move umh locking code into fw_load_from_user_helper()

[Luis R. Rodriguez]

On Wed, Sep 07, 2016 at 10:45:05AM +0200, Daniel Wagner wrote:
> From: Daniel Wagner <daniel.wagner@bmw-carit.de>
> 
> When we load the firmware directly we don't need to take the umh
> lock. So move this part inside fw_load_from_user_helper which is only
> available when CONFIG_FW_LOADER_USER_HELPER is set.
> 
> This avoids a dependency on firmware_loading_timeout() even when
> !CONFIG_FW_LOADER_UER_HELPER.

Great work! Just one issue found, noted below.

> The usermodehelper locking code was added by b298d289c792 ("PM / Sleep:
> Fix freezer failures due to racy usermodehelper_is_disabled()").

Thanks, this helps to give some perspective, I'll note that commit also refers
to commit a144c6a (PM: Print a warning if firmware is requested when tasks are
frozen) by Srivatsa a long time ago which added a warning print if a driver
requested firmware when tasks are frozen. That commit log further clarifies
that the issues is that some drivers erroneously use request_firmware() in
their driver's ->resume() (or ->thaw(), or ->restore()) callbacks, it further
clarifies that is not going to work unless the firmware has been built in.
It did not explain *why* it wouldn't work though. But note it also mentioned
how drivers that do have request_firmware() calls on resume stall resume --
the reason for the stalling is the stupid usermode helper. The kernel now
"fixed" these by returning an error in such cases, it does this by checking
kernel user mode helper is disabled, this is why it would not work. But note
that we should be disabling the usermode helper on suspend too, and likely
the reason we never ran into an issue with the cache stuff is we would fail
if the usermode helper was disabled anyway. This is a long winded way of
saying that these commits further confirm removal of using the usermode helper
from the firmware cache work for suspend/resume.

> Signed-off-by: Daniel Wagner <daniel.wagner@bmw-carit.de>
> Cc: Ming Lei <ming.lei@canonical.com>
> Cc: Luis R. Rodriguez <mcgrof@kernel.org>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> ---
>  drivers/base/firmware_class.c | 52 +++++++++++++++++++++++--------------------
>  1 file changed, 28 insertions(+), 24 deletions(-)
> 
> diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
> index 960f8f7..d4fee06 100644
> --- a/drivers/base/firmware_class.c
> +++ b/drivers/base/firmware_class.c
> @@ -981,13 +981,38 @@ static int fw_load_from_user_helper(struct firmware *firmware,
>  				    unsigned int opt_flags, long timeout)
>  {
>  	struct firmware_priv *fw_priv;
> +	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_load_from_user_helper() no longer needs the timeout parameter then.
Given this fact I'll chime in with some other, IMHO cosmetic things for
this series. This however is the just the biggest issue for this series
that I've found. That and testing this at run time didn't boot on my
system, it could be an issue with linux-next next-20160907 booting
on my system, I hadn't tried that yet. I did put your series through
0-day though and it went through fine though.

Since you will need a respin I'd appreciate if you can Cc Takashi,
Bjorn, Daniel Vetter, and Arend van Spriel on these series as some
of them have expressed interest in the umh stuff, so best to get wider
review as well. While at it please Cc Rafael and Srivatsa.

  Luis

Re: [PATCH v4 2/4] firmware: encapsulate firmware loading status

[Luis R. Rodriguez]

On Wed, Sep 07, 2016 at 10:45:06AM +0200, Daniel Wagner wrote:
> From: Daniel Wagner <daniel.wagner@bmw-carit.de>
> 
> The firmware user helper code tracks the current state of the loading
> process via unsigned long status and a complection in struct
> firmware_buf. We only need this for the usermode helper as such we can
> encapsulate all this data into its own data structure.
> 
> Signed-off-by: Daniel Wagner <daniel.wagner@bmw-carit.de>
> Cc: Ming Lei <ming.lei@canonical.com>
> Cc: Luis R. Rodriguez <mcgrof@kernel.org>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> ---
>  drivers/base/firmware_class.c | 123 ++++++++++++++++++++++++++++++------------
>  1 file changed, 88 insertions(+), 35 deletions(-)
> 
> diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
> index d4fee06..b11fbb0 100644
> --- a/drivers/base/firmware_class.c
> +++ b/drivers/base/firmware_class.c
> @@ -91,10 +91,13 @@ static inline bool fw_is_builtin_firmware(const struct firmware *fw)
>  }
>  #endif
>  
> +#ifdef CONFIG_FW_LOADER_USER_HELPER
> +
>  enum {
> +	FW_STATUS_UNKNOWN,
>  	FW_STATUS_LOADING,
>  	FW_STATUS_DONE,
> -	FW_STATUS_ABORT,
> +	FW_STATUS_ABORTED,
>  };
>  
>  static int loading_timeout = 60;	/* In seconds */
> @@ -104,6 +107,69 @@ static inline long firmware_loading_timeout(void)
>  	return loading_timeout > 0 ? loading_timeout * HZ : MAX_JIFFY_OFFSET;
>  }
>  
> +struct fw_status {
> +	unsigned long status;
> +	struct completion completion;
> +};
> +
> +static void fw_status_init(struct fw_status *fw_st)
> +{
> +	fw_st->status = FW_STATUS_UNKNOWN;
> +	init_completion(&fw_st->completion);
> +}
> +
> +static int __fw_status_check(struct fw_status *fw_st, unsigned long status)
> +{
> +	return test_bit(status, &fw_st->status);
> +}
> +
> +static int fw_status_wait_timeout(struct fw_status *fw_st, long timeout)
> +{
> +	int ret;
> +
> +	ret = wait_for_completion_interruptible_timeout(&fw_st->completion,
> +							timeout);
> +	if (ret == 0 && test_bit(FW_STATUS_ABORTED, &fw_st->status))
> +		return -ENOENT;
> +
> +	return ret;
> +}
> +
> +static void __fw_status_set(struct fw_status *fw_st,
> +			  unsigned long status)
> +{
> +	set_bit(status, &fw_st->status);
> +
> +	if (status == FW_STATUS_DONE ||
> +			status == FW_STATUS_ABORTED) {
> +		clear_bit(FW_STATUS_LOADING, &fw_st->status);
> +		complete_all(&fw_st->completion);
> +	}
> +}
> +
> +#define fw_status_start(fw_st)					\
> +	__fw_status_set(fw_st, FW_STATUS_LOADING)
> +#define fw_status_done(fw_st)					\
> +	__fw_status_set(fw_st, FW_STATUS_DONE)
> +#define fw_status_aborted(fw_st)				\
> +	__fw_status_set(fw_st, FW_STATUS_ABORTED)
> +#define fw_status_is_loading(fw_st)				\
> +	__fw_status_check(fw_st, FW_STATUS_LOADING)
> +#define fw_status_is_done(fw_st)				\
> +	__fw_status_check(fw_st, FW_STATUS_DONE)
> +#define fw_status_is_aborted(fw_st)				\
> +	__fw_status_check(fw_st, FW_STATUS_ABORTED)
> +
> +#else /* CONFIG_FW_LOADER_USER_HELPER */

Note, this all does *nothing* when we have CONFIG_FW_LOADER_USER_HELPER
disabled. Might as well then rename these with a fw_umh_ prefix so we can
easily tell that this mess is for that.

So how about fw_umh_done(), fw_umh_is_done() fw_umh_is_aborted() and fw_umh_wait()?

> +
> +#define fw_status_wait_timeout(fw_st, long)	0
> +
> +#define fw_status_done(fw_st)
> +#define fw_status_is_done(fw_st)		true
> +#define fw_status_is_aborted(fw_st)		false
> +
> +#endif /* !CONFIG_FW_LOADER_USER_HELPER */
> +
>  /* firmware behavior options */
>  #define FW_OPT_UEVENT	(1U << 0)
>  #define FW_OPT_NOWAIT	(1U << 1)
> @@ -145,13 +211,12 @@ struct firmware_cache {
>  struct firmware_buf {
>  	struct kref ref;
>  	struct list_head list;
> -	struct completion completion;
>  	struct firmware_cache *fwc;
> -	unsigned long status;
>  	void *data;
>  	size_t size;
>  	size_t allocated_size;
>  #ifdef CONFIG_FW_LOADER_USER_HELPER
> +	struct fw_status fw_st;

Likewise fw_umh_status ?

>  	bool is_paged_buf;
>  	bool need_uevent;
>  	struct page **pages;
> @@ -205,8 +270,8 @@ static struct firmware_buf *__allocate_fw_buf(const char *fw_name,
>  	buf->fwc = fwc;
>  	buf->data = dbuf;
>  	buf->allocated_size = size;
> -	init_completion(&buf->completion);
>  #ifdef CONFIG_FW_LOADER_USER_HELPER
> +	fw_status_init(&buf->fw_st);
>  	INIT_LIST_HEAD(&buf->pending_list);
>  #endif
>  
> @@ -309,8 +374,7 @@ static void fw_finish_direct_load(struct device *device,
>  				  struct firmware_buf *buf)
>  {
>  	mutex_lock(&fw_lock);
> -	set_bit(FW_STATUS_DONE, &buf->status);
> -	complete_all(&buf->completion);
> +	fw_status_done(&buf->fw_st);
>  	mutex_unlock(&fw_lock);
>  }
>  
> @@ -478,12 +542,11 @@ static void __fw_load_abort(struct firmware_buf *buf)
>  	 * There is a small window in which user can write to 'loading'
>  	 * between loading done and disappearance of 'loading'
>  	 */
> -	if (test_bit(FW_STATUS_DONE, &buf->status))
> +	if (fw_status_is_done(&buf->fw_st))
>  		return;
>  
>  	list_del_init(&buf->pending_list);
> -	set_bit(FW_STATUS_ABORT, &buf->status);
> -	complete_all(&buf->completion);
> +	fw_status_aborted(&buf->fw_st);
>  }
>  
>  static void fw_load_abort(struct firmware_priv *fw_priv)
> @@ -496,9 +559,6 @@ static void fw_load_abort(struct firmware_priv *fw_priv)
>  	fw_priv->buf = NULL;
>  }
>  
> -#define is_fw_load_aborted(buf)	\
> -	test_bit(FW_STATUS_ABORT, &(buf)->status)
> -
>  static LIST_HEAD(pending_fw_head);
>  
>  /* reboot notifier for avoid deadlock with usermode_lock */
> @@ -598,7 +658,7 @@ static ssize_t firmware_loading_show(struct device *dev,
>  
>  	mutex_lock(&fw_lock);
>  	if (fw_priv->buf)
> -		loading = test_bit(FW_STATUS_LOADING, &fw_priv->buf->status);
> +		loading = fw_status_is_loading(&fw_priv->buf->fw_st);
>  	mutex_unlock(&fw_lock);
>  
>  	return sprintf(buf, "%d\n", loading);
> @@ -653,23 +713,20 @@ static ssize_t firmware_loading_store(struct device *dev,
>  	switch (loading) {
>  	case 1:
>  		/* discarding any previous partial load */
> -		if (!test_bit(FW_STATUS_DONE, &fw_buf->status)) {
> +		if (!fw_status_is_done(&fw_buf->fw_st)) {
>  			for (i = 0; i < fw_buf->nr_pages; i++)
>  				__free_page(fw_buf->pages[i]);
>  			vfree(fw_buf->pages);
>  			fw_buf->pages = NULL;
>  			fw_buf->page_array_size = 0;
>  			fw_buf->nr_pages = 0;
> -			set_bit(FW_STATUS_LOADING, &fw_buf->status);
> +			fw_status_start(&fw_buf->fw_st);
>  		}
>  		break;
>  	case 0:
> -		if (test_bit(FW_STATUS_LOADING, &fw_buf->status)) {
> +		if (fw_status_is_loading(&fw_buf->fw_st)) {
>  			int rc;
>  
> -			set_bit(FW_STATUS_DONE, &fw_buf->status);
> -			clear_bit(FW_STATUS_LOADING, &fw_buf->status);
> -
>  			/*
>  			 * Several loading requests may be pending on
>  			 * one same firmware buf, so let all requests
> @@ -691,10 +748,11 @@ static ssize_t firmware_loading_store(struct device *dev,
>  			 */
>  			list_del_init(&fw_buf->pending_list);
>  			if (rc) {
> -				set_bit(FW_STATUS_ABORT, &fw_buf->status);
> +				fw_status_aborted(&fw_buf->fw_st);
>  				written = rc;
> +			} else {
> +				fw_status_done(&fw_buf->fw_st);
>  			}
> -			complete_all(&fw_buf->completion);
>  			break;
>  		}
>  		/* fallthrough */
> @@ -702,7 +760,7 @@ static ssize_t firmware_loading_store(struct device *dev,
>  		dev_err(dev, "%s: unexpected value (%d)\n", __func__, loading);
>  		/* fallthrough */
>  	case -1:
> -		fw_load_abort(fw_priv);
> +		fw_status_aborted(&fw_buf->fw_st);
>  		break;
>  	}
>  out:
> @@ -755,7 +813,7 @@ static ssize_t firmware_data_read(struct file *filp, struct kobject *kobj,
>  
>  	mutex_lock(&fw_lock);
>  	buf = fw_priv->buf;
> -	if (!buf || test_bit(FW_STATUS_DONE, &buf->status)) {
> +	if (!buf || fw_status_is_done(&buf->fw_st)) {
>  		ret_count = -ENODEV;
>  		goto out;
>  	}
> @@ -842,7 +900,7 @@ static ssize_t firmware_data_write(struct file *filp, struct kobject *kobj,
>  
>  	mutex_lock(&fw_lock);
>  	buf = fw_priv->buf;
> -	if (!buf || test_bit(FW_STATUS_DONE, &buf->status)) {
> +	if (!buf || fw_status_is_done(&buf->fw_st)) {
>  		retval = -ENODEV;
>  		goto out;
>  	}
> @@ -955,8 +1013,7 @@ static int _request_firmware_load(struct firmware_priv *fw_priv,

Maybe rename also _request_firmware_load() to something to implicate this is all
umh related.

>  		timeout = MAX_JIFFY_OFFSET;
>  	}
>  
> -	retval = wait_for_completion_interruptible_timeout(&buf->completion,
> -			timeout);
> +	retval = fw_status_wait_timeout(&buf->fw_st, timeout);

Note this is one of the users for fw_status_wait_timeout(). This makes sense
for this fw_status_wait_timeout, as its umh related.

>  	if (retval == -ERESTARTSYS || !retval) {
>  		mutex_lock(&fw_lock);
>  		fw_load_abort(fw_priv);
> @@ -965,7 +1022,7 @@ static int _request_firmware_load(struct firmware_priv *fw_priv,
>  		retval = 0;
>  	}
>  
> -	if (is_fw_load_aborted(buf))
> +	if (fw_status_is_aborted(&buf->fw_st))
>  		retval = -EAGAIN;
>  	else if (buf->is_paged_buf && !buf->data)
>  		retval = -ENOMEM;
> @@ -1040,29 +1097,25 @@ fw_load_from_user_helper(struct firmware *firmware, const char *name,
>  	return -ENOENT;
>  }
>  
> -/* No abort during direct loading */
> -#define is_fw_load_aborted(buf) false
> -
>  #ifdef CONFIG_PM_SLEEP
>  static inline void kill_requests_without_uevent(void) { }
>  #endif
>  
>  #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 (!test_bit(FW_STATUS_DONE, &buf->status)) {
> -		if (is_fw_load_aborted(buf)) {
> +	while (!fw_status_is_done(&buf->fw_st)) {
> +		if (fw_status_is_aborted(&buf->fw_st)) {
>  			ret = -ENOENT;
>  			break;
>  		}
>  		mutex_unlock(&fw_lock);
> -		ret = wait_for_completion_interruptible(&buf->completion);
> +		ret = fw_status_wait_timeout(&buf->fw_st, 0);

Now this one -- I am not sure of. That it, it is not clear why we would
need fw_status_wait_timeout() here, and the code history does not reveal
that either. Obviously this is a no-op for for non UMH kenrels *but*
even for UMH -- why do we need it?

Perhaps Takashi might know...

  Luis

Re: [PATCH v4 3/4] firmware: Drop bit ops in favor of simple state machine

[Luis R. Rodriguez]

On Wed, Sep 07, 2016 at 10:45:07AM +0200, Daniel Wagner wrote:
> From: Daniel Wagner <daniel.wagner@bmw-carit.de>
> 
> We track the state of the loading with bit ops. Since the state machine
> has only a couple of states and there are only a few simple state
> transition 

And they are all mutually exclusive ?

> we can model this simplify.
> 
> 	   UNKNOWN -> LOADING -> DONE | ABORTED

So why unsigned long ? Why not a u8?

> 
> Signed-off-by: Daniel Wagner <daniel.wagner@bmw-carit.de>
> Cc: Ming Lei <ming.lei@canonical.com>
> Cc: Luis R. Rodriguez <mcgrof@kernel.org>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> ---
>  drivers/base/firmware_class.c | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
> index b11fbb0..7757c03 100644
> --- a/drivers/base/firmware_class.c
> +++ b/drivers/base/firmware_class.c
> @@ -120,16 +120,18 @@ static void fw_status_init(struct fw_status *fw_st)
>  
>  static int __fw_status_check(struct fw_status *fw_st, unsigned long status)
>  {
> -	return test_bit(status, &fw_st->status);
> +	return fw_st->status == status;
>  }
>  
>  static int fw_status_wait_timeout(struct fw_status *fw_st, long timeout)
>  {
> +	unsigned long status;
>  	int ret;
>  
>  	ret = wait_for_completion_interruptible_timeout(&fw_st->completion,
>  							timeout);
> -	if (ret == 0 && test_bit(FW_STATUS_ABORTED, &fw_st->status))
> +	status = READ_ONCE(fw_st->status);
> +	if (ret == 0 && status == FW_STATUS_ABORTED)
>  		return -ENOENT;
>  
>  	return ret;
> @@ -138,13 +140,11 @@ static int fw_status_wait_timeout(struct fw_status *fw_st, long timeout)
>  static void __fw_status_set(struct fw_status *fw_st,
>  			  unsigned long status)
>  {
> -	set_bit(status, &fw_st->status);
> +	WRITE_ONCE(fw_st->status, status);
>  
>  	if (status == FW_STATUS_DONE ||
> -			status == FW_STATUS_ABORTED) {
> -		clear_bit(FW_STATUS_LOADING, &fw_st->status);
> +			status == FW_STATUS_ABORTED)
>  		complete_all(&fw_st->completion);
> -	}
>  }
>  
>  #define fw_status_start(fw_st)					\

See if all the above were prefixed with fw_umh or something like it
it would make this easier to read and tell this is all umh related.

  Luis

Re: [PATCH v4 2/4] firmware: encapsulate firmware loading status

[Daniel Wagner]

On 09/08/2016 03:39 AM, Luis R. Rodriguez wrote:
> On Wed, Sep 07, 2016 at 10:45:06AM +0200, Daniel Wagner wrote:
>> From: Daniel Wagner <daniel.wagner@bmw-carit.de>
>>  		timeout = MAX_JIFFY_OFFSET;
>>  	}
>>
>> -	retval = wait_for_completion_interruptible_timeout(&buf->completion,
>> -			timeout);
>> +	retval = fw_status_wait_timeout(&buf->fw_st, timeout);
>
> Note this is one of the users for fw_status_wait_timeout(). This makes sense
> for this fw_status_wait_timeout, as its umh related.
>
>>  	if (retval == -ERESTARTSYS || !retval) {
>>  		mutex_lock(&fw_lock);
>>  		fw_load_abort(fw_priv);
>> @@ -965,7 +1022,7 @@ static int _request_firmware_load(struct firmware_priv *fw_priv,
>>  		retval = 0;
>>  	}
>>
>> -	if (is_fw_load_aborted(buf))
>> +	if (fw_status_is_aborted(&buf->fw_st))
>>  		retval = -EAGAIN;
>>  	else if (buf->is_paged_buf && !buf->data)
>>  		retval = -ENOMEM;
>> @@ -1040,29 +1097,25 @@ fw_load_from_user_helper(struct firmware *firmware, const char *name,
>>  	return -ENOENT;
>>  }
>>
>> -/* No abort during direct loading */
>> -#define is_fw_load_aborted(buf) false
>> -
>>  #ifdef CONFIG_PM_SLEEP
>>  static inline void kill_requests_without_uevent(void) { }
>>  #endif
>>
>>  #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 (!test_bit(FW_STATUS_DONE, &buf->status)) {
>> -		if (is_fw_load_aborted(buf)) {
>> +	while (!fw_status_is_done(&buf->fw_st)) {
>> +		if (fw_status_is_aborted(&buf->fw_st)) {
>>  			ret = -ENOENT;
>>  			break;
>>  		}
>>  		mutex_unlock(&fw_lock);
>> -		ret = wait_for_completion_interruptible(&buf->completion);
>> +		ret = fw_status_wait_timeout(&buf->fw_st, 0);
>
> Now this one -- I am not sure of. That it, it is not clear why we would
> need fw_status_wait_timeout() here, and the code history does not reveal
> that either. Obviously this is a no-op for for non UMH kenrels *but*
> even for UMH -- why do we need it?


This while loop was originally a goto loop:

1f2b79599ee8 ("firmware loader: always let firmware_buf own the pages 
buffer")

I don't think the code doesn't do what it was indented to do. The reason 
is that calling complete_all() the wait_for_completion() will not block 
again until it has 'eaten up' the done counter. That is around UMAX/2 
loops. So there is an reinit_completion() missing in this case.

Before the above commit, the completion was used to wait inside
_request_firmware_load() till the UML had done its job. The answer for 
your question is probably in 1f2b79599ee8 ("firmware loader: always let 
firmware_buf own the pages buffer").

/me starts reading.

cheers,
daniel

Re: [PATCH v4 2/4] firmware: encapsulate firmware loading status

[Daniel Wagner]

On 09/08/2016 10:05 AM, Daniel Wagner wrote:
> This while loop was originally a goto loop:
>
> 1f2b79599ee8 ("firmware loader: always let firmware_buf own the pages
> buffer")
>
> I don't think the code doesn't do what it was indented to do. The reason
> is that calling complete_all() the wait_for_completion() will not block
> again until it has 'eaten up' the done counter. That is around UMAX/2
> loops. So there is an reinit_completion() missing in this case.

Ah, I think I got it. It looks like kind of optimization. Instead doing

	wait_for_completion()
	if (done)
		do_this()
	if (abort)
		do_that()

we have

check_again:
	if (done) {
		do_this()
		goto out
	}
	if (abort) {
		do_that()
		goto out
	}
	wait_for_completion()
	goto check_again
out:

> Before the above commit, the completion was used to wait inside
> _request_firmware_load() till the UML had done its job. The answer for
> your question is probably in 1f2b79599ee8 ("firmware loader: always let
> firmware_buf own the pages buffer").

So it still does the same job after this change. Check if we have the 
firmware loaded already in buf if not kick off the umh loader and wait 
for the result.

Re: [PATCH v4 2/4] firmware: encapsulate firmware loading status

[Ming Lei]

On Wed, Sep 7, 2016 at 4:45 PM, Daniel Wagner <wagi@monom.org> wrote:
> From: Daniel Wagner <daniel.wagner@bmw-carit.de>
>
> The firmware user helper code tracks the current state of the loading
> process via unsigned long status and a complection in struct
> firmware_buf. We only need this for the usermode helper as such we can
> encapsulate all this data into its own data structure.
>
> Signed-off-by: Daniel Wagner <daniel.wagner@bmw-carit.de>
> Cc: Ming Lei <ming.lei@canonical.com>
> Cc: Luis R. Rodriguez <mcgrof@kernel.org>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> ---
>  drivers/base/firmware_class.c | 123 ++++++++++++++++++++++++++++++------------
>  1 file changed, 88 insertions(+), 35 deletions(-)
>
> diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
> index d4fee06..b11fbb0 100644
> --- a/drivers/base/firmware_class.c
> +++ b/drivers/base/firmware_class.c
> @@ -91,10 +91,13 @@ static inline bool fw_is_builtin_firmware(const struct firmware *fw)
>  }
>  #endif
>
> +#ifdef CONFIG_FW_LOADER_USER_HELPER
> +
>  enum {
> +       FW_STATUS_UNKNOWN,
>         FW_STATUS_LOADING,
>         FW_STATUS_DONE,
> -       FW_STATUS_ABORT,
> +       FW_STATUS_ABORTED,
>  };
>
>  static int loading_timeout = 60;       /* In seconds */
> @@ -104,6 +107,69 @@ static inline long firmware_loading_timeout(void)
>         return loading_timeout > 0 ? loading_timeout * HZ : MAX_JIFFY_OFFSET;
>  }
>
> +struct fw_status {
> +       unsigned long status;
> +       struct completion completion;
> +};
> +
> +static void fw_status_init(struct fw_status *fw_st)
> +{
> +       fw_st->status = FW_STATUS_UNKNOWN;
> +       init_completion(&fw_st->completion);
> +}
> +
> +static int __fw_status_check(struct fw_status *fw_st, unsigned long status)
> +{
> +       return test_bit(status, &fw_st->status);
> +}
> +
> +static int fw_status_wait_timeout(struct fw_status *fw_st, long timeout)
> +{
> +       int ret;
> +
> +       ret = wait_for_completion_interruptible_timeout(&fw_st->completion,
> +                                                       timeout);
> +       if (ret == 0 && test_bit(FW_STATUS_ABORTED, &fw_st->status))
> +               return -ENOENT;

I guess the check should have been OR instead of AND, right?

> +
> +       return ret;
> +}
> +
> +static void __fw_status_set(struct fw_status *fw_st,
> +                         unsigned long status)
> +{
> +       set_bit(status, &fw_st->status);
> +
> +       if (status == FW_STATUS_DONE ||
> +                       status == FW_STATUS_ABORTED) {
> +               clear_bit(FW_STATUS_LOADING, &fw_st->status);
> +               complete_all(&fw_st->completion);
> +       }
> +}
> +
> +#define fw_status_start(fw_st)                                 \
> +       __fw_status_set(fw_st, FW_STATUS_LOADING)
> +#define fw_status_done(fw_st)                                  \
> +       __fw_status_set(fw_st, FW_STATUS_DONE)
> +#define fw_status_aborted(fw_st)                               \
> +       __fw_status_set(fw_st, FW_STATUS_ABORTED)
> +#define fw_status_is_loading(fw_st)                            \
> +       __fw_status_check(fw_st, FW_STATUS_LOADING)
> +#define fw_status_is_done(fw_st)                               \
> +       __fw_status_check(fw_st, FW_STATUS_DONE)
> +#define fw_status_is_aborted(fw_st)                            \
> +       __fw_status_check(fw_st, FW_STATUS_ABORTED)
> +
> +#else /* CONFIG_FW_LOADER_USER_HELPER */
> +
> +#define fw_status_wait_timeout(fw_st, long)    0
> +
> +#define fw_status_done(fw_st)
> +#define fw_status_is_done(fw_st)               true
> +#define fw_status_is_aborted(fw_st)            false
> +
> +#endif /* !CONFIG_FW_LOADER_USER_HELPER */
> +
>  /* firmware behavior options */
>  #define FW_OPT_UEVENT  (1U << 0)
>  #define FW_OPT_NOWAIT  (1U << 1)
> @@ -145,13 +211,12 @@ struct firmware_cache {
>  struct firmware_buf {
>         struct kref ref;
>         struct list_head list;
> -       struct completion completion;
>         struct firmware_cache *fwc;
> -       unsigned long status;
>         void *data;
>         size_t size;
>         size_t allocated_size;
>  #ifdef CONFIG_FW_LOADER_USER_HELPER
> +       struct fw_status fw_st;
>         bool is_paged_buf;
>         bool need_uevent;
>         struct page **pages;
> @@ -205,8 +270,8 @@ static struct firmware_buf *__allocate_fw_buf(const char *fw_name,
>         buf->fwc = fwc;
>         buf->data = dbuf;
>         buf->allocated_size = size;
> -       init_completion(&buf->completion);
>  #ifdef CONFIG_FW_LOADER_USER_HELPER
> +       fw_status_init(&buf->fw_st);
>         INIT_LIST_HEAD(&buf->pending_list);
>  #endif
>
> @@ -309,8 +374,7 @@ static void fw_finish_direct_load(struct device *device,
>                                   struct firmware_buf *buf)
>  {
>         mutex_lock(&fw_lock);
> -       set_bit(FW_STATUS_DONE, &buf->status);
> -       complete_all(&buf->completion);
> +       fw_status_done(&buf->fw_st);
>         mutex_unlock(&fw_lock);
>  }
>
> @@ -478,12 +542,11 @@ static void __fw_load_abort(struct firmware_buf *buf)
>          * There is a small window in which user can write to 'loading'
>          * between loading done and disappearance of 'loading'
>          */
> -       if (test_bit(FW_STATUS_DONE, &buf->status))
> +       if (fw_status_is_done(&buf->fw_st))
>                 return;
>
>         list_del_init(&buf->pending_list);
> -       set_bit(FW_STATUS_ABORT, &buf->status);
> -       complete_all(&buf->completion);
> +       fw_status_aborted(&buf->fw_st);
>  }
>
>  static void fw_load_abort(struct firmware_priv *fw_priv)
> @@ -496,9 +559,6 @@ static void fw_load_abort(struct firmware_priv *fw_priv)
>         fw_priv->buf = NULL;
>  }
>
> -#define is_fw_load_aborted(buf)        \
> -       test_bit(FW_STATUS_ABORT, &(buf)->status)
> -
>  static LIST_HEAD(pending_fw_head);
>
>  /* reboot notifier for avoid deadlock with usermode_lock */
> @@ -598,7 +658,7 @@ static ssize_t firmware_loading_show(struct device *dev,
>
>         mutex_lock(&fw_lock);
>         if (fw_priv->buf)
> -               loading = test_bit(FW_STATUS_LOADING, &fw_priv->buf->status);
> +               loading = fw_status_is_loading(&fw_priv->buf->fw_st);
>         mutex_unlock(&fw_lock);
>
>         return sprintf(buf, "%d\n", loading);
> @@ -653,23 +713,20 @@ static ssize_t firmware_loading_store(struct device *dev,
>         switch (loading) {
>         case 1:
>                 /* discarding any previous partial load */
> -               if (!test_bit(FW_STATUS_DONE, &fw_buf->status)) {
> +               if (!fw_status_is_done(&fw_buf->fw_st)) {
>                         for (i = 0; i < fw_buf->nr_pages; i++)
>                                 __free_page(fw_buf->pages[i]);
>                         vfree(fw_buf->pages);
>                         fw_buf->pages = NULL;
>                         fw_buf->page_array_size = 0;
>                         fw_buf->nr_pages = 0;
> -                       set_bit(FW_STATUS_LOADING, &fw_buf->status);
> +                       fw_status_start(&fw_buf->fw_st);
>                 }
>                 break;
>         case 0:
> -               if (test_bit(FW_STATUS_LOADING, &fw_buf->status)) {
> +               if (fw_status_is_loading(&fw_buf->fw_st)) {
>                         int rc;
>
> -                       set_bit(FW_STATUS_DONE, &fw_buf->status);
> -                       clear_bit(FW_STATUS_LOADING, &fw_buf->status);
> -
>                         /*
>                          * Several loading requests may be pending on
>                          * one same firmware buf, so let all requests
> @@ -691,10 +748,11 @@ static ssize_t firmware_loading_store(struct device *dev,
>                          */
>                         list_del_init(&fw_buf->pending_list);
>                         if (rc) {
> -                               set_bit(FW_STATUS_ABORT, &fw_buf->status);
> +                               fw_status_aborted(&fw_buf->fw_st);
>                                 written = rc;
> +                       } else {
> +                               fw_status_done(&fw_buf->fw_st);
>                         }
> -                       complete_all(&fw_buf->completion);
>                         break;
>                 }
>                 /* fallthrough */
> @@ -702,7 +760,7 @@ static ssize_t firmware_loading_store(struct device *dev,
>                 dev_err(dev, "%s: unexpected value (%d)\n", __func__, loading);
>                 /* fallthrough */
>         case -1:
> -               fw_load_abort(fw_priv);
> +               fw_status_aborted(&fw_buf->fw_st);
>                 break;
>         }
>  out:
> @@ -755,7 +813,7 @@ static ssize_t firmware_data_read(struct file *filp, struct kobject *kobj,
>
>         mutex_lock(&fw_lock);
>         buf = fw_priv->buf;
> -       if (!buf || test_bit(FW_STATUS_DONE, &buf->status)) {
> +       if (!buf || fw_status_is_done(&buf->fw_st)) {
>                 ret_count = -ENODEV;
>                 goto out;
>         }
> @@ -842,7 +900,7 @@ static ssize_t firmware_data_write(struct file *filp, struct kobject *kobj,
>
>         mutex_lock(&fw_lock);
>         buf = fw_priv->buf;
> -       if (!buf || test_bit(FW_STATUS_DONE, &buf->status)) {
> +       if (!buf || fw_status_is_done(&buf->fw_st)) {
>                 retval = -ENODEV;
>                 goto out;
>         }
> @@ -955,8 +1013,7 @@ static int _request_firmware_load(struct firmware_priv *fw_priv,
>                 timeout = MAX_JIFFY_OFFSET;
>         }
>
> -       retval = wait_for_completion_interruptible_timeout(&buf->completion,
> -                       timeout);
> +       retval = fw_status_wait_timeout(&buf->fw_st, timeout);
>         if (retval == -ERESTARTSYS || !retval) {
>                 mutex_lock(&fw_lock);
>                 fw_load_abort(fw_priv);
> @@ -965,7 +1022,7 @@ static int _request_firmware_load(struct firmware_priv *fw_priv,
>                 retval = 0;
>         }
>
> -       if (is_fw_load_aborted(buf))
> +       if (fw_status_is_aborted(&buf->fw_st))
>                 retval = -EAGAIN;
>         else if (buf->is_paged_buf && !buf->data)
>                 retval = -ENOMEM;
> @@ -1040,29 +1097,25 @@ fw_load_from_user_helper(struct firmware *firmware, const char *name,
>         return -ENOENT;
>  }
>
> -/* No abort during direct loading */
> -#define is_fw_load_aborted(buf) false
> -
>  #ifdef CONFIG_PM_SLEEP
>  static inline void kill_requests_without_uevent(void) { }
>  #endif
>
>  #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 (!test_bit(FW_STATUS_DONE, &buf->status)) {
> -               if (is_fw_load_aborted(buf)) {
> +       while (!fw_status_is_done(&buf->fw_st)) {
> +               if (fw_status_is_aborted(&buf->fw_st)) {
>                         ret = -ENOENT;
>                         break;
>                 }
>                 mutex_unlock(&fw_lock);
> -               ret = wait_for_completion_interruptible(&buf->completion);
> +               ret = fw_status_wait_timeout(&buf->fw_st, 0);
>                 mutex_lock(&fw_lock);
>         }
>         mutex_unlock(&fw_lock);
> @@ -1120,7 +1173,7 @@ static int assign_firmware_buf(struct firmware *fw, struct device *device,
>         struct firmware_buf *buf = fw->priv;
>
>         mutex_lock(&fw_lock);
> -       if (!buf->size || is_fw_load_aborted(buf)) {
> +       if (!buf->size || fw_status_is_aborted(&buf->fw_st)) {
>                 mutex_unlock(&fw_lock);
>                 return -ENOENT;
>         }
> --
> 2.7.4

Re: [PATCH v4 2/4] firmware: encapsulate firmware loading status

[Daniel Wagner]

Hi Ming,

On 09/08/2016 01:26 PM, Ming Lei wrote:
> On Wed, Sep 7, 2016 at 4:45 PM, Daniel Wagner <wagi@monom.org> wrote:
>> From: Daniel Wagner <daniel.wagner@bmw-carit.de>
>> +static int fw_status_wait_timeout(struct fw_status *fw_st, long timeout)
>> +{
>> +       int ret;
>> +
>> +       ret = wait_for_completion_interruptible_timeout(&fw_st->completion,
>> +                                                       timeout);
>> +       if (ret == 0 && test_bit(FW_STATUS_ABORTED, &fw_st->status))
>> +               return -ENOENT;
>
> I guess the check should have been OR instead of AND, right?


Good catch. It should be

	if (ret != 0 && test_bit(...))
		return -ENOENT;

in case where we abort the operation instead of timing out.

cheers,
daniel

Re: [PATCH v4 2/4] firmware: encapsulate firmware loading status

[Daniel Wagner]

On 09/07/2016 10:45 AM, Daniel Wagner wrote:
> @@ -702,7 +760,7 @@ static ssize_t firmware_loading_store(struct device *dev,
>  		dev_err(dev, "%s: unexpected value (%d)\n", __func__, loading);
>  		/* fallthrough */
>  	case -1:
> -		fw_load_abort(fw_priv);
> +		fw_status_aborted(&fw_buf->fw_st);
>  		break;
>  	}

And that change one is wrong. Spinning a new version.

Re: [PATCH v4 1/4] firmware: Move umh locking code into fw_load_from_user_helper()

[Daniel Wagner]

On 09/08/2016 01:33 AM, Luis R. Rodriguez wrote:
>> The usermodehelper locking code was added by b298d289c792 ("PM / Sleep:
>> Fix freezer failures due to racy usermodehelper_is_disabled()").
>
> Thanks, this helps to give some perspective, I'll note that commit also refers
> to commit a144c6a (PM: Print a warning if firmware is requested when tasks are
> frozen) by Srivatsa a long time ago which added a warning print if a driver
> requested firmware when tasks are frozen. That commit log further clarifies
> that the issues is that some drivers erroneously use request_firmware() in
> their driver's ->resume() (or ->thaw(), or ->restore()) callbacks, it further
> clarifies that is not going to work unless the firmware has been built in.
> It did not explain *why* it wouldn't work though. But note it also mentioned
> how drivers that do have request_firmware() calls on resume stall resume --
> the reason for the stalling is the stupid usermode helper. The kernel now
> "fixed" these by returning an error in such cases, it does this by checking
> kernel user mode helper is disabled, this is why it would not work. But note
> that we should be disabling the usermode helper on suspend too, and likely
> the reason we never ran into an issue with the cache stuff is we would fail
> if the usermode helper was disabled anyway. This is a long winded way of
> saying that these commits further confirm removal of using the usermode helper
> from the firmware cache work for suspend/resume.

Okay, so let's finish this round of refactoring first. I prefer going in 
smaller steps and see if there are any regressions with those changes.


>> Signed-off-by: Daniel Wagner <daniel.wagner@bmw-carit.de>
>> Cc: Ming Lei <ming.lei@canonical.com>
>> Cc: Luis R. Rodriguez <mcgrof@kernel.org>
>> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
>> ---
>>  drivers/base/firmware_class.c | 52 +++++++++++++++++++++++--------------------
>>  1 file changed, 28 insertions(+), 24 deletions(-)
>>
>> diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
>> index 960f8f7..d4fee06 100644
>> --- a/drivers/base/firmware_class.c
>> +++ b/drivers/base/firmware_class.c
>> @@ -981,13 +981,38 @@ static int fw_load_from_user_helper(struct firmware *firmware,
>>  				    unsigned int opt_flags, long timeout)
>>  {
>>  	struct firmware_priv *fw_priv;
>> +	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_load_from_user_helper() no longer needs the timeout parameter then.

Updated the patch accordingly.

> Given this fact I'll chime in with some other, IMHO cosmetic things for
> this series. This however is the just the biggest issue for this series
> that I've found. That and testing this at run time didn't boot on my
> system, it could be an issue with linux-next next-20160907 booting
> on my system, I hadn't tried that yet. I did put your series through
> 0-day though and it went through fine though.

So far I have it tested with kvm. I'll give it a spin on real hardware. 
Good point.

> Since you will need a respin I'd appreciate if you can Cc Takashi,
> Bjorn, Daniel Vetter, and Arend van Spriel on these series as some
> of them have expressed interest in the umh stuff, so best to get wider
> review as well. While at it please Cc Rafael and Srivatsa.

Will do.

cheers,
daniel

Re: [PATCH v4 3/4] firmware: Drop bit ops in favor of simple state machine

[Daniel Wagner]

On 09/08/2016 03:45 AM, Luis R. Rodriguez wrote:
> On Wed, Sep 07, 2016 at 10:45:07AM +0200, Daniel Wagner wrote:
>> From: Daniel Wagner <daniel.wagner@bmw-carit.de>
>>
>> We track the state of the loading with bit ops. Since the state machine
>> has only a couple of states and there are only a few simple state
>> transition
>
> And they are all mutually exclusive ?

Yes, I'll updated the commit message

>
>> we can model this simplify.
>>
>> 	   UNKNOWN -> LOADING -> DONE | ABORTED
>
> So why unsigned long ? Why not a u8?

No reason, only a leftover. Changed it to u8.

>  	return ret;
>> @@ -138,13 +140,11 @@ static int fw_status_wait_timeout(struct fw_status *fw_st, long timeout)
>>  static void __fw_status_set(struct fw_status *fw_st,
>>  			  unsigned long status)
>>  {
>> -	set_bit(status, &fw_st->status);
>> +	WRITE_ONCE(fw_st->status, status);
>>
>>  	if (status == FW_STATUS_DONE ||
>> -			status == FW_STATUS_ABORTED) {
>> -		clear_bit(FW_STATUS_LOADING, &fw_st->status);
>> +			status == FW_STATUS_ABORTED)
>>  		complete_all(&fw_st->completion);
>> -	}
>>  }
>>
>>  #define fw_status_start(fw_st)					\
>
> See if all the above were prefixed with fw_umh or something like it
> it would make this easier to read and tell this is all umh related.

Changed the prefix to fm_umh. Looks much better.

cheers,
daniel

Re: [PATCH v4 1/4] firmware: Move umh locking code into fw_load_from_user_helper()

[Luis R. Rodriguez]

On Thu, Sep 08, 2016 at 02:41:33PM +0200, Daniel Wagner wrote:
> So far I have it tested with kvm. I'll give it a spin on real
> hardware. Good point.

If you can also include testing with the test_firmware.ko driver
and describe your tests in the cover letter that would be appreciated.
Bonus points if you also indicate you've had 0-day have full success
with your delta. To do that just poke Fenguang that you need testing
for a branch, and then push to your branch. I noted I gave your changes
a test spin and it came back OK, so it should be good, but still, this
is rather good practice for any changes sent out.

  Luis

Re: [PATCH v4 1/4] firmware: Move umh locking code into fw_load_from_user_helper()

[Ming Lei]

On Wed, Sep 7, 2016 at 4:45 PM, Daniel Wagner <wagi@monom.org> wrote:
> From: Daniel Wagner <daniel.wagner@bmw-carit.de>
>
> When we load the firmware directly we don't need to take the umh
> lock.

I am wondering if it can be wrong.

Actually in case of firmware loading, the usermode helper lock doesn't
only mean the user helper is usable, and it also may serve to mark the
filesystem/block device is ready for firmware loading, and of couse direct
loading need fs/block to be ready too.

> So move this part inside fw_load_from_user_helper which is only
> available when CONFIG_FW_LOADER_USER_HELPER is set.
>
> This avoids a dependency on firmware_loading_timeout() even when
> !CONFIG_FW_LOADER_UER_HELPER.
>
> The usermodehelper locking code was added by b298d289c792 ("PM / Sleep:
> Fix freezer failures due to racy usermodehelper_is_disabled()").
>
> Signed-off-by: Daniel Wagner <daniel.wagner@bmw-carit.de>
> Cc: Ming Lei <ming.lei@canonical.com>
> Cc: Luis R. Rodriguez <mcgrof@kernel.org>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> ---
>  drivers/base/firmware_class.c | 52 +++++++++++++++++++++++--------------------
>  1 file changed, 28 insertions(+), 24 deletions(-)
>
> diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
> index 960f8f7..d4fee06 100644
> --- a/drivers/base/firmware_class.c
> +++ b/drivers/base/firmware_class.c
> @@ -981,13 +981,38 @@ static int fw_load_from_user_helper(struct firmware *firmware,
>                                     unsigned int opt_flags, long timeout)
>  {
>         struct firmware_priv *fw_priv;
> +       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 release_lock;
> +       }
>
>         fw_priv->buf = firmware->priv;
> -       return _request_firmware_load(fw_priv, opt_flags, timeout);
> +       ret = _request_firmware_load(fw_priv, opt_flags, timeout);
> +
> +release_lock:
> +       usermodehelper_read_unlock();
> +
> +       return ret;
>  }
>
>  #ifdef CONFIG_PM_SLEEP
> @@ -1150,25 +1175,6 @@ _request_firmware(const struct firmware **firmware_p, const char *name,
>         if (ret <= 0) /* error or already assigned */
>                 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))
> @@ -1185,8 +1191,6 @@ _request_firmware(const struct firmware **firmware_p, const char *name,
>         if (!ret)
>                 ret = assign_firmware_buf(fw, device, opt_flags);
>
> -       usermodehelper_read_unlock();
> -
>   out:
>         if (ret < 0) {
>                 release_firmware(fw);
> --
> 2.7.4

Re: [PATCH v4 2/4] firmware: encapsulate firmware loading status

[Ming Lei]

On Thu, Sep 8, 2016 at 8:26 PM, Daniel Wagner <wagi@monom.org> wrote:
> Hi Ming,
>
> On 09/08/2016 01:26 PM, Ming Lei wrote:
>>
>> On Wed, Sep 7, 2016 at 4:45 PM, Daniel Wagner <wagi@monom.org> wrote:
>>>
>>> From: Daniel Wagner <daniel.wagner@bmw-carit.de>
>>> +static int fw_status_wait_timeout(struct fw_status *fw_st, long timeout)
>>> +{
>>> +       int ret;
>>> +
>>> +       ret =
>>> wait_for_completion_interruptible_timeout(&fw_st->completion,
>>> +                                                       timeout);
>>> +       if (ret == 0 && test_bit(FW_STATUS_ABORTED, &fw_st->status))
>>> +               return -ENOENT;
>>
>>
>> I guess the check should have been OR instead of AND, right?
>
>
>
> Good catch. It should be
>
>         if (ret != 0 && test_bit(...))
>                 return -ENOENT;

Another question, why do you want to return -ENOENT when
userspace aborts the load? And looks it will always be override as
-EAGAIN.

>
> in case where we abort the operation instead of timing out.
>
> cheers,
> daniel

Re: [PATCH v4 1/4] firmware: Move umh locking code into fw_load_from_user_helper()

[Luis R. Rodriguez]

On Thu, Sep 08, 2016 at 11:37:54PM +0800, Ming Lei wrote:
> On Wed, Sep 7, 2016 at 4:45 PM, Daniel Wagner <wagi@monom.org> wrote:
> > From: Daniel Wagner <daniel.wagner@bmw-carit.de>
> >
> > When we load the firmware directly we don't need to take the umh
> > lock.
> 
> I am wondering if it can be wrong.

If you disable the firmware UMH why would we need to lock if the lock is being
shown only used for the firmare UMH ?

> Actually in case of firmware loading, the usermode helper lock doesn't
> only mean the user helper is usable, and it also may serve to mark the
> filesystem/block device is ready for firmware loading, and of couse direct
> loading need fs/block to be ready too.

Yes but that's a race I've identified a while ago present even if you use initramfs *and*
use kernel_read_file_from_path() on any part of the kernel [0], I proposed a possible
solution recently [1], given tons of folks were *complaining* about this but despite there
being one solution proposed [2] it was still incorrect, as only *userspace* can know
for sure when your critical filesystems are mounted. Since we now have a shared
"read file fs" helper kernel_read_file_from_path() it implicates the race is possible
elsewhere as well.

The race is present given you simply cannot know when the root filesystem
(consider a series of pivot_root() calls, hey -- anyone can do that, who are we to
tell them they can't), or in this particular case importance, only considering firmware,
when /lib/firmware/ is ready.  In short I am saying this whole race thing is a
bigger issue, and as much as Linus hates my proposed solution I have not heard
any proposal alternatives to address this properly, not even clarifications on
what our expectations for drivers then are if they use kernel_read_file_from_path()
early on their driver code.

The original goal of the usermode helper lock came can be traced back in
time via a144c6a ("PM: Print a warning if firmware is requested when task
are frozen) when Rafael noticed drivers were calling to request firmware
on *resume* calls! Why would that be an issue? It was because of the stupid
delays incurred on resume when firmware *was not found* __due__ to the stupid
user mode helper timeout delay and people believing this often meant users
confusing resume *stalling* for resume was *broken! Later b298d289c7
("PM / Sleep: Fix freezer failures due to racy usermodehelper_is_disabled()")
addressed races. That code in turn was later massaged into shape to better
accept direct FS loading via commit 4e0c92d015235 ("firmware: Refactoring for
splitting user-mode helper code").

So for a while we've been nagging driver developers to not add request_firmware()
calls on resume calls. In fact the drivers/base/firmware_class.c code *kills*
firmware UMH calls when we go to suspend for the firmware cache, as such even
on suspend its a stupid idea to use the UMH, not only because it can
stall suspend but *also* because we kill these UMH calls. Its why I recently
proposed removing the possibility to call the firmware UMH from the firmware
cache [3]. If this patch is wrong please do chime in!

So, as I see it the user mode helper lock should have *nothing* to do with
the race between reading files from the kernel and having a path ready. If
it was *used* for that, that was papering over the real issue. Its no
different of a hack for instance as if a driver using request_firmware() tried
to use async probe to avoid the same race. Yes it will help, but no, it does
not mean it is deterministically safe.

Reviewing commit 247bc03742545 ("PM / Sleep: Mitigate race between the freezer
and request_firmware()") which originally extended umh state machine from
just being enabled/disabled, with the concepts of UMH_ENABLED, UMH_FREEZING,
UMH_DISABLED -- its goal was to prevent UMH uses during suspend. So -- the
"UMH lock" on firmware was actually added to help avoid races between freezing
and request_firmware(). We should not re-use UMH status notifiers when the
firmware UMH is disabled for the same concepts -- if we needed such a concept
then we should take this out from UMH code and generalize it.

In fact this shows there's still an issue here for UMH code as well. The
usermodehelper_enable() call 

rest_init() --> kernel_thread(kernel_init, NULL, CLONE_FS); -->
	kernel_init --> kernel_init_freeable() --> wait_for_completion(&kthreadd_done);

So after kernel_init_freeable() kthreadd_done should be done but note that
only userspace will know what partition set up to use. Shortly after
this though in kernel_init_freeable() we call prepare_namespace() so if
initramfs was set up its set up after.

Meanwhile driver's inits are called via do_initcalls() called from do_basic_setup()
and that is called within kernel_init_freeable() *prior* to prepare_namespace().

So calling usermodehelper_enable() doesn't necessarily ensure that the paths
for the UMH used will actually work either. This path also has a race.

We need to address both things then:

  1) *freeze* races / stalls
  2)  userspace paths ready for whatever the kernel needs to read off of the
      filesystem

These issues are not particular to firmware -- this applies to all
kernel_read_file_from_path() and as is being revealed now the UMH code. It gets
me wondering if we have any shared code possible between UMH and
kernel_read_file_from_path().

For 1) we could just generalize the code.

For 2) I proposed a solution as RFC although some hate it, my latest preference
would be either *declare* these uses early reads clearly as not supported or
have a proper init level that does guarantee to be safe against the userspace
paths.

I'm all ears for alternatives.

[0] https://marc.info/?t=147286207700002&r=1&w=2
[1] http://lkml.kernel.org/r/CAB=NE6UBRa0K7=PomJzKxsoj4GzAqkYrkp=O+UfVvu2fwM25pA@mail.gmail.com
[2] https://lkml.org/lkml/2014/6/15/47
[3] https://lkml.kernel.org/r/1473208930-6835-6-git-send-email-mcgrof@kernel.org

  Luis

Re: [PATCH v4 1/4] firmware: Move umh locking code into fw_load_from_user_helper()

[Ming Lei]

On Fri, Sep 9, 2016 at 4:11 AM, Luis R. Rodriguez <mcgrof@kernel.org> wrote:
> On Thu, Sep 08, 2016 at 11:37:54PM +0800, Ming Lei wrote:
>> On Wed, Sep 7, 2016 at 4:45 PM, Daniel Wagner <wagi@monom.org> wrote:
>> > From: Daniel Wagner <daniel.wagner@bmw-carit.de>
>> >
>> > When we load the firmware directly we don't need to take the umh
>> > lock.
>>
>> I am wondering if it can be wrong.
>
> If you disable the firmware UMH why would we need to lock if the lock is being
> shown only used for the firmare UMH ?
>
>> Actually in case of firmware loading, the usermode helper lock doesn't
>> only mean the user helper is usable, and it also may serve to mark the
>> filesystem/block device is ready for firmware loading, and of couse direct
>> loading need fs/block to be ready too.
>
> Yes but that's a race I've identified a while ago present even if you use initramfs *and*
> use kernel_read_file_from_path() on any part of the kernel [0], I proposed a possible

Actualy I mean the situation of suspend vs. resume, and some drivers
still may not benefit from firmware loading cache when requesting loading
in .resume(), at that time it is still too early for direct loading.
With UMH lock,
we can get a warning or avoid the issue.


> solution recently [1], given tons of folks were *complaining* about this but despite there
> being one solution proposed [2] it was still incorrect, as only *userspace* can know

In case of initramfs and built-in drivers, it isn't a surprise to see this race,
and that shouldn't be a common case. But for suspend vs. resume, it can be
true.


> for sure when your critical filesystems are mounted. Since we now have a shared
> "read file fs" helper kernel_read_file_from_path() it implicates the race is possible
> elsewhere as well.
>
> The race is present given you simply cannot know when the root filesystem
> (consider a series of pivot_root() calls, hey -- anyone can do that, who are we to
> tell them they can't), or in this particular case importance, only considering firmware,
> when /lib/firmware/ is ready.  In short I am saying this whole race thing is a
> bigger issue, and as much as Linus hates my proposed solution I have not heard
> any proposal alternatives to address this properly, not even clarifications on
> what our expectations for drivers then are if they use kernel_read_file_from_path()
> early on their driver code.
>
> The original goal of the usermode helper lock came can be traced back in
> time via a144c6a ("PM: Print a warning if firmware is requested when task
> are frozen) when Rafael noticed drivers were calling to request firmware
> on *resume* calls! Why would that be an issue? It was because of the stupid
> delays incurred on resume when firmware *was not found* __due__ to the stupid
> user mode helper timeout delay and people believing this often meant users
> confusing resume *stalling* for resume was *broken! Later b298d289c7
> ("PM / Sleep: Fix freezer failures due to racy usermodehelper_is_disabled()")
> addressed races. That code in turn was later massaged into shape to better
> accept direct FS loading via commit 4e0c92d015235 ("firmware: Refactoring for
> splitting user-mode helper code").
>
> So for a while we've been nagging driver developers to not add request_firmware()
> calls on resume calls. In fact the drivers/base/firmware_class.c code *kills*
> firmware UMH calls when we go to suspend for the firmware cache, as such even
> on suspend its a stupid idea to use the UMH, not only because it can
> stall suspend but *also* because we kill these UMH calls. Its why I recently
> proposed removing the possibility to call the firmware UMH from the firmware
> cache [3]. If this patch is wrong please do chime in!
>
> So, as I see it the user mode helper lock should have *nothing* to do with
> the race between reading files from the kernel and having a path ready. If
> it was *used* for that, that was papering over the real issue. Its no
> different of a hack for instance as if a driver using request_firmware() tried
> to use async probe to avoid the same race. Yes it will help, but no, it does
> not mean it is deterministically safe.
>
> Reviewing commit 247bc03742545 ("PM / Sleep: Mitigate race between the freezer
> and request_firmware()") which originally extended umh state machine from
> just being enabled/disabled, with the concepts of UMH_ENABLED, UMH_FREEZING,
> UMH_DISABLED -- its goal was to prevent UMH uses during suspend. So -- the
> "UMH lock" on firmware was actually added to help avoid races between freezing
> and request_firmware(). We should not re-use UMH status notifiers when the
> firmware UMH is disabled for the same concepts -- if we needed such a concept
> then we should take this out from UMH code and generalize it.
>
> In fact this shows there's still an issue here for UMH code as well. The
> usermodehelper_enable() call
>
> rest_init() --> kernel_thread(kernel_init, NULL, CLONE_FS); -->
>         kernel_init --> kernel_init_freeable() --> wait_for_completion(&kthreadd_done);
>
> So after kernel_init_freeable() kthreadd_done should be done but note that
> only userspace will know what partition set up to use. Shortly after
> this though in kernel_init_freeable() we call prepare_namespace() so if
> initramfs was set up its set up after.
>
> Meanwhile driver's inits are called via do_initcalls() called from do_basic_setup()
> and that is called within kernel_init_freeable() *prior* to prepare_namespace().
>
> So calling usermodehelper_enable() doesn't necessarily ensure that the paths
> for the UMH used will actually work either. This path also has a race.
>
> We need to address both things then:
>
>   1) *freeze* races / stalls
>   2)  userspace paths ready for whatever the kernel needs to read off of the
>       filesystem
>
> These issues are not particular to firmware -- this applies to all
> kernel_read_file_from_path() and as is being revealed now the UMH code. It gets
> me wondering if we have any shared code possible between UMH and
> kernel_read_file_from_path().
>
> For 1) we could just generalize the code.
>
> For 2) I proposed a solution as RFC although some hate it, my latest preference
> would be either *declare* these uses early reads clearly as not supported or
> have a proper init level that does guarantee to be safe against the userspace
> paths.
>
> I'm all ears for alternatives.
>
> [0] https://marc.info/?t=147286207700002&r=1&w=2
> [1] http://lkml.kernel.org/r/CAB=NE6UBRa0K7=PomJzKxsoj4GzAqkYrkp=O+UfVvu2fwM25pA@mail.gmail.com
> [2] https://lkml.org/lkml/2014/6/15/47
> [3] https://lkml.kernel.org/r/1473208930-6835-6-git-send-email-mcgrof@kernel.org
>
>   Luis

Re: [PATCH v4 1/4] firmware: Move umh locking code into fw_load_from_user_helper()

[Ming Lei]

On Fri, Sep 9, 2016 at 11:39 AM, Luis R. Rodriguez <mcgrof@kernel.org> wrote:
> On Sep 8, 2016 6:22 PM, "Ming Lei" <ming.lei@canonical.com> wrote:
>>
>> On Fri, Sep 9, 2016 at 4:11 AM, Luis R. Rodriguez <mcgrof@kernel.org>
>> wrote:
>> > On Thu, Sep 08, 2016 at 11:37:54PM +0800, Ming Lei wrote:
>> >> On Wed, Sep 7, 2016 at 4:45 PM, Daniel Wagner <wagi@monom.org> wrote:
>> >> > From: Daniel Wagner <daniel.wagner@bmw-carit.de>
>> >> >
>> >> > When we load the firmware directly we don't need to take the umh
>> >> > lock.
>> >>
>> >> I am wondering if it can be wrong.
>> >
>> > If you disable the firmware UMH why would we need to lock if the lock is
>> > being
>> > shown only used for the firmare UMH ?
>> >
>> >> Actually in case of firmware loading, the usermode helper lock doesn't
>> >> only mean the user helper is usable, and it also may serve to mark the
>> >> filesystem/block device is ready for firmware loading, and of couse
>> >> direct
>> >> loading need fs/block to be ready too.
>> >
>> > Yes but that's a race I've identified a while ago present even if you
>> > use initramfs *and*
>> > use kernel_read_file_from_path() on any part of the kernel [0], I
>> > proposed a possible
>>
>> Actualy I mean the situation of suspend vs. resume, and some drivers
>> still may not benefit from firmware loading cache when requesting loading
>> in .resume(), at that time it is still too early for direct loading.
>> With UMH lock,
>> we can get a warning or avoid the issue.
>
> Agreed, but that would seem odd and perhaps misleading to have a try lock
> for UMH when no firmware UMH code is enabled. This should probably made
> clear in comments for now as to why we have it then and we should just mark

That is very helpful, :-)

> a TODO item to generalize this to a common freezer check. Surprised we don't
> have one yet. Rafael ?
>
>   Luis

Re: [PATCH v4 2/4] firmware: encapsulate firmware loading status

[Daniel Wagner]

On 09/08/2016 05:49 PM, Ming Lei wrote:
> On Thu, Sep 8, 2016 at 8:26 PM, Daniel Wagner <wagi@monom.org> wrote:
>>
>>         if (ret != 0 && test_bit(...))
>>                 return -ENOENT;
>
> Another question, why do you want to return -ENOENT when
> userspace aborts the load? And looks it will always be override as
> -EAGAIN.

I took the -ENOENT from sync_cached_firmware_buf():

  /* 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 (!test_bit(FW_STATUS_DONE, &buf->status)) {
-		if (is_fw_load_aborted(buf)) {
+	while (!fw_status_is_done(&buf->fw_st)) {
+		if (fw_status_is_aborted(&buf->fw_st)) {
  			ret = -ENOENT;
  			break;
  		}
  		mutex_unlock(&fw_lock);
-		ret = wait_for_completion_interruptible(&buf->completion);
+		ret = fw_status_wait_timeout(&buf->fw_st, 0);
  		mutex_lock(&fw_lock);


There is a path where it not overwritten by -EAGAIN:

   request_firmware()
     _request_firmware()
       _request_firmware_prepare
         sync_cached_firmware_buf()
   	  fw_status_wait_timeout()
	    return -ENOENT

If you want me to change it to EAGAIN, I'll do it. I just tried hard not 
to have any changes in behavior.

cheer,
daniel

Re: [PATCH v4 1/4] firmware: Move umh locking code into fw_load_from_user_helper()

[Luis R. Rodriguez]

On Fri, Sep 09, 2016 at 12:19:38PM +0800, Ming Lei wrote:
> On Fri, Sep 9, 2016 at 11:39 AM, Luis R. Rodriguez <mcgrof@kernel.org> wrote:
> > On Sep 8, 2016 6:22 PM, "Ming Lei" <ming.lei@canonical.com> wrote:
> >>
> >> On Fri, Sep 9, 2016 at 4:11 AM, Luis R. Rodriguez <mcgrof@kernel.org>
> >> wrote:
> >> > On Thu, Sep 08, 2016 at 11:37:54PM +0800, Ming Lei wrote:
> >> >> On Wed, Sep 7, 2016 at 4:45 PM, Daniel Wagner <wagi@monom.org> wrote:
> >> >> > From: Daniel Wagner <daniel.wagner@bmw-carit.de>
> >> >> >
> >> >> > When we load the firmware directly we don't need to take the umh
> >> >> > lock.
> >> >>
> >> >> I am wondering if it can be wrong.
> >> >
> >> > If you disable the firmware UMH why would we need to lock if the lock is
> >> > being
> >> > shown only used for the firmare UMH ?
> >> >
> >> >> Actually in case of firmware loading, the usermode helper lock doesn't
> >> >> only mean the user helper is usable, and it also may serve to mark the
> >> >> filesystem/block device is ready for firmware loading, and of couse
> >> >> direct
> >> >> loading need fs/block to be ready too.
> >> >
> >> > Yes but that's a race I've identified a while ago present even if you
> >> > use initramfs *and*
> >> > use kernel_read_file_from_path() on any part of the kernel [0], I
> >> > proposed a possible
> >>
> >> Actualy I mean the situation of suspend vs. resume, and some drivers
> >> still may not benefit from firmware loading cache when requesting loading
> >> in .resume(), at that time it is still too early for direct loading.
> >> With UMH lock,
> >> we can get a warning or avoid the issue.
> >
> > Agreed, but that would seem odd and perhaps misleading to have a try lock
> > for UMH when no firmware UMH code is enabled. This should probably made
> > clear in comments for now as to why we have it then and we should just mark
> 
> That is very helpful, :-)

Upon a closer look at how we use usermodehelper_read_trylock() and
usermodehelper_read_lock_wait() for the non-CONFIG_FW_LOADER_USER_HELPER kernels,
it seems we have to change things a bit more if we wanted to keep this *and*
I've personally come to the determination we can stuff all this crap under
CONFIG_FW_LOADER_USER_HELPER due to the firmware cache. Here's why:

As Daniel has it, the usermodehelper_read_lock_wait() case this unfolds into a:

schedule_timeout(0) when the usermodehelper_disabled is true, since after
the freezer was introduced this can be any of these values:

enum umh_disable_depth {                                                        
        UMH_ENABLED = 0,                                                        
        UMH_FREEZING,                                                           
        UMH_DISABLED,                                                           
};

It means we schedule_timeout(0) anytime usermodehelper_disabled is either
UMH_FREEZING or UMH_DISABLED. Contrary to usermodehelper_read_trylock() it
will also *not* call try_to_freeze().

Given how we setup timer for this:

        expire = timeout + jiffies;                                             
                                                                                
        setup_timer_on_stack(&timer, process_timeout, (unsigned long)current);  
        __mod_timer(&timer, expire, false);                                     
        schedule();                                                             
        del_singleshot_timer_sync(&timer);

We will naturally *always* timeout on schedule_timeout(0), so this return 0
always.

As Daniel has it when CONFIG_FW_LOADER_USER_HELPER is enabled we do have a
timeout and that just ensures we schedule_timeout(timeout) and as such can run
in the loop for usermodehelper_read_trylock() trying to see if the
usermodehelper_disabled has finally flipped to UMH_ENABLED, after which we'd
grant entry, say you can pass Go, and collect $200.

Since we use schedule_timeout(0) for !CONFIG_FW_LOADER_USER_HELPER it means
we *always* break out of the loop immediately, and return 0. If we are
in !CONFIG_FW_LOADER_USER_HELPER and the usermodehelper_disabled is set
to UMH_ENABLED we *immediately* bail and return 0 as well.

So effectively this code:

_request_firmware()
{
	...
                timeout = usermodehelper_read_lock_wait(timeout);               
                if (!timeout) {                                                 
                        dev_dbg(device, "firmware: %s loading timed out\n",     
                                name);                                          
                        ret = -EBUSY;                                           
                        goto out;                                               
                }   
	...
}

Will always returns -EBUSY if timeout is 0.

Our current default is 60 even for !CONFIG_FW_LOADER_USER_HELPER kernels, in
that case what this code does is only on FW_OPT_NOWAIT calls
(request_firmware_nowait()) it will wait for 60 seconds for the freezer to
clear.

In Daniel's code right now this would change the default timeout to 0, but that
would effectively break all request_firmware_nowait() calls. If we want to
keep the same functionality we'd have to keep the default timeout set to 60
then for !CONFIG_FW_LOADER_USER_HELPER.

For the non-FW_OPT_NOWAIT (regular sync request_firmware() calls) cases we have:

_request_firmware()
{
	...
                ret = usermodehelper_read_trylock();                            
                if (WARN_ON(ret)) {                                             
                        dev_err(device, "firmware: %s will not be loaded\n",    
                                name);                                          
                        goto out;                                               
                }      
	...
}

This will do the same loop but if we are freezing (UMH_FREEZING,) it *will*
take the time to call try_to_freeze(). If we've hit UMH_DISABLED we bail
with -EAGAIN. Contrary to the async case this doesn't time out, so it
just loops forever and schedules()'s in hopes we eventually change state.

So.. this would have to be kept if we want to keep the same behaviour.

Now that this is all groked the next question is do we really need this
crap ? My interest in this is not just for this patch series -- the
"sysdata" flexible API (which I'll re-brand to "driverdata" for lack of
any better alternative recommendations) also did away with this usermode
lock crap completely as none of the above was documented, and that API was
to *avoid* the fw usermode helper at all costs.

If we *really* need something like this it should mean all users of
kernel_read_file_from_path() might also need this. Note though:

 a) we *warn* about calls during resume callbacks -- we want to avoid them,
    note this was added via a144c6a ("PM: Print a warning if firmware is
    requested when tasks are frozen"). That warn is currently only applicable
    on sync requests. Note that the UMH code and commits indicate that
    we actually cannot put tasks to sleep that are not interruptible so
    that race will always exist, as such its rather fatal to use these
    calls on resume, but we don't warn there.

 b) if we've entered UMH_DISABLED (this means freeze_processes() is done)
    we kill all requests until thaw_processes() is called when we are
    coming out of suspend.

*But*

 c) commit 07646d9c0938d ("firmware loader: cache devices firmware during
    suspend/resume cycle") added a clever firmware cache solution to
    *enable* drivers to call request firmware on resume ! The trick
    to this and why we currently *do not get warnings on resume* calls
    is the warning is part of _request_firmware() but only for the sync
    case where we use usermodehelper_read_trylock(), and note :

      the cache is checked earlier via __fw_lookup_buf()!

Effectively the firmware cache does aways with the stupid issues
with calling request_firmware() on resume, its effective so long
as you don't use the firmware usermode helper given that we otherwise
kill these on our way at suspend anyway. The firmware cache *does not*
help if you have are using the firmware usermode helper then, and
as such I can only think of the usermodehelper_read_trylock() and
usermodehelper_read_lock_wait() as effective *iff* you *are* using
the firmware usermode helper given that we have a solution for
suspend/resume without it. Additionally if we needed any type of
additional suspend/resume check for the native filesystem loader
we should implement it properly using the fw_pm_notify and/or
the struct syscore_ops fw_syscore_ops.

The complexities with using the UMH and suspend/resume should send
chills to anyone still considering that solution.

The flexible driverdata API I'll add would just *skip* the UMH calls
completely, for now best we can do is just compartmentalize it completley
as much as possible as Daniel has been doing.

> > a TODO item to generalize this to a common freezer check. Surprised we don't
> > have one yet. Rafael ?

The above determination of using the proper struct syscore_ops IMHO would
be the appropriate solution.

Some other notes for Daniel:

  o feel free to rename __fw_lookup_buf() and related calls so something easier to
    understand that this is looking at the *firmware cache*

  o If Ming agrees with the above findings I think you should proceed with
    stuffing the UMH lock crap into the FW UMH code as you had intended

  Luis

Re: [PATCH v4 1/4] firmware: Move umh locking code into fw_load_from_user_helper()

[Luis R. Rodriguez]

On Mon, Sep 12, 2016 at 02:09:10PM +0200, Daniel Wagner wrote:
> On 09/10/2016 12:10 AM, Luis R. Rodriguez wrote:
> > I've personally come to the determination we can stuff all this crap under
> > CONFIG_FW_LOADER_USER_HELPER due to the firmware cache. Here's why:
> > 
> > As Daniel has it, the usermodehelper_read_lock_wait() case this unfolds into a:
> >
> > schedule_timeout(0) when the usermodehelper_disabled is true, since after
> > the freezer was introduced this can be any of these values:
> > 
> > enum umh_disable_depth {                                                        
> >         UMH_ENABLED = 0,                                                        
> >         UMH_FREEZING,                                                           
> >         UMH_DISABLED,                                                           
> > };
> > 
> > It means we schedule_timeout(0) anytime usermodehelper_disabled is either
> > UMH_FREEZING or UMH_DISABLED. Contrary to usermodehelper_read_trylock() it
> > will also *not* call try_to_freeze().
> > 
> > Given how we setup timer for this:
> > 
> >         expire = timeout + jiffies;                                             
> >                                                                                 
> >         setup_timer_on_stack(&timer, process_timeout, (unsigned long)current);  
> >         __mod_timer(&timer, expire, false);                                     
> >         schedule();                                                             
> >         del_singleshot_timer_sync(&timer);
> > 
> > We will naturally *always* timeout on schedule_timeout(0), so this return 0
> > always.
> 
> The smallest possible timeout is 1 * HZ because:
> 
> static inline long firmware_loading_timeout(void)
> {
> 	return loading_timeout > 0 ? loading_timeout * HZ : MAX_JIFFY_OFFSET;
> }

Sorry, I confused your setting:

+#define fw_status_wait_timeout(fw_st, long)    0

for replacing the abvove firmware_loading_timeout() to be 0 when
!CONFIG_FW_LOADER_USER_HELPER 

> > As Daniel has it when CONFIG_FW_LOADER_USER_HELPER is enabled we do have a
> > timeout and that just ensures we schedule_timeout(timeout) and as such can run
> > in the loop for usermodehelper_read_trylock() trying to see if the
> > usermodehelper_disabled has finally flipped to UMH_ENABLED, after which we'd
> > grant entry, say you can pass Go, and collect $200.
> > 
> > Since we use schedule_timeout(0) for !CONFIG_FW_LOADER_USER_HELPER it means
> > we *always* break out of the loop immediately, and return 0. If we are
> > in !CONFIG_FW_LOADER_USER_HELPER and the usermodehelper_disabled is set
> > to UMH_ENABLED we *immediately* bail and return 0 as well.
> 
> The mainline version will use 60s timeout for the usermode helper
> for !CONFIG_FW_LOADER_USER_HELPER. In case of the 
> CONFIG_FW_LOADER_USER_HELPER we should be the range of 1 * HZ to
> MAX_JIFFY_OFFSET. I say should because we don't check if
> 'loading_timeout * HZ' overflows. 

Indeed.

> > So effectively this code:
> > 
> > _request_firmware()
> > {
> > 	...
> >                 timeout = usermodehelper_read_lock_wait(timeout);               
> >                 if (!timeout) {                                                 
> >                         dev_dbg(device, "firmware: %s loading timed out\n",     
> >                                 name);                                          
> >                         ret = -EBUSY;                                           
> >                         goto out;                                               
> >                 }   
> > 	...
> > }
> > 
> > Will always returns -EBUSY if timeout is 0.
> 
> But timeout is never 0 unless I got that wrong. 

No indeed, my mistake.

> > Our current default is 60 even for !CONFIG_FW_LOADER_USER_HELPER kernels, in
> > that case what this code does is only on FW_OPT_NOWAIT calls
> > (request_firmware_nowait()) it will wait for 60 seconds for the freezer to
> > clear.
> > 
> > In Daniel's code right now this would change the default timeout to 0,
> 
> No, it shouldn't. With this patch 1, the usermode helper code
> is not involved anymore, that's the main difference.

Indeed. I meant to say that if the timeout was 0 it would not be effectively
for using the umh lock for what we though we wanted before. As you clarify
though your changes do not set the default timeout to 0 though.

> > but that
> > would effectively break all request_firmware_nowait() calls. If we want to
> > keep the same functionality we'd have to keep the default timeout set to 60
> > then for !CONFIG_FW_LOADER_USER_HELPER.
> > 
> > For the non-FW_OPT_NOWAIT (regular sync request_firmware() calls) cases we have:
> > 
> > _request_firmware()
> > {
> > 	...
> >                 ret = usermodehelper_read_trylock();                            
> >                 if (WARN_ON(ret)) {                                             
> >                         dev_err(device, "firmware: %s will not be loaded\n",    
> >                                 name);                                          
> >                         goto out;                                               
> >                 }      
> > 	...
> > }
> > 
> > This will do the same loop but if we are freezing (UMH_FREEZING,) it *will*
> > take the time to call try_to_freeze(). If we've hit UMH_DISABLED we bail
> > with -EAGAIN. Contrary to the async case this doesn't time out, so it
> > just loops forever and schedules()'s in hopes we eventually change state.
> > 
> > So.. this would have to be kept if we want to keep the same behaviour.
> 
> With !CONFIG_FW_LOADER_HELPER this series is missing warning when it is
> still too early for direct loading.

Well no, the commit that added the umh lock to firmware_class mentions the
purpose was to warn for users of the firmware API on resume. My point is that
folks should not rely on this lock to assume rootfs is ready, its simply not
true. As I noted in a proposed RFC and followed by criticism -- this requires
more work. For now the SmPL grammar rule should start helping us hunt for users
of these APIs on init/probe.

PS. I'll be gone on vacation for 2 weeks now.

  Luis