[PATCH v5 0/5] firmware: encapsulate firmware loading status

[PATCH v5 0/5] firmware: encapsulate firmware loading status

[Daniel Wagner]

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

Hi,

The firmware user helper code tracks the current state of the loading
process via an member of struct firmware_buf and a completion. Let's
encapsulate this simple state machine into struct fw_status. The aim is
to increase readability and reduce the usage of the fw_lock.

Luis asked by to add a few CC for getting a wider audience. If you
haven't seen this before the initial version of this series has some
more details on the motivation for this series:

http://www.spinics.net/lists/linux-wireless/msg153005.html

I tested this series with fw_userhelper.sh and fw_filesystem.sh under
kvm and also let it run on real hardware. The series is also available
here:

https://git.kernel.org/cgit/linux/kernel/git/wagi/linux.git/log/?h=firmware_async-7

I had pushed earlier version of series there and haven't got any 0-day
bug reports so far.

cheers,
daniel

This series depends on Luis' "firmware: add SmPL grammar to avoid issues"
series:

http://marc.info/?l=linux-kernel&m=147320896231418&w=2

changes since v4:
  - replaced "firmware: Move umh locking code into fw_load_from_user_helper()"
    with "firmware: document user mode helper lock usage"
  - changed prefix fw_status_ to fw_umh_
  - fixed a couple of bux pointed out by Ming
  - changed type of fw_umh::status to u8 and updated commit message to
    point out that all states are exclusive
    

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

Daniel Wagner (5):
  firmware: document user mode helper lock usage
  firmware: encapsulate firmware loading status
  firmware: rename fw_load_from_user_helper() and
    _request_firmware_load()
  firmware: drop bit ops in favor of simple state machine
  firmware: do not use fw_lock for fw_umh protection

 drivers/base/firmware_class.c | 194 +++++++++++++++++++++++++-----------------
 1 file changed, 118 insertions(+), 76 deletions(-)

-- 
2.7.4

[PATCH v5 1/5] firmware: document user mode helper lock usage

[Daniel Wagner]

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

The lock is also used to generate warnings when a direct
firmware load is requested too early.

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

As Luis points out:

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

After some discussion with Ming it was decided to put a comment to the
code to document the usage of the umh and a TODO to resolve this by
having some generic means to detect ongoing freezing operations.

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

diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
index 960f8f7..8eba1fb 100644
--- a/drivers/base/firmware_class.c
+++ b/drivers/base/firmware_class.c
@@ -1150,6 +1150,19 @@ _request_firmware(const struct firmware **firmware_p, const char *name,
 	if (ret <= 0) /* error or already assigned */
 		goto out;
 
+	/*
+	 * The usermode helper lock is taken to serialize the firmware
+	 * loading even when no usermoder mode helper is used at all.
+	 *
+	 * Some drivers may not benefit from firmware loading cache
+	 * when requesting loading in .resume(). In the situation of
+	 * suspend vs. resume, it is still too early for direct
+	 * loading. With UMH lock, we can get a warning or avoid the
+	 * issue.
+	 *
+	 * TODO: Taking the UMH lock is a bit missleading and it makes
+	 * sense to generalize this to a common freezer check.
+	 */
 	ret = 0;
 	timeout = firmware_loading_timeout();
 	if (opt_flags & FW_OPT_NOWAIT) {
-- 
2.7.4

[PATCH v5 2/5] 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.

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

diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
index 8eba1fb..821babe 100644
--- a/drivers/base/firmware_class.c
+++ b/drivers/base/firmware_class.c
@@ -91,12 +91,6 @@ static inline bool fw_is_builtin_firmware(const struct firmware *fw)
 }
 #endif
 
-enum {
-	FW_STATUS_LOADING,
-	FW_STATUS_DONE,
-	FW_STATUS_ABORT,
-};
-
 static int loading_timeout = 60;	/* In seconds */
 
 static inline long firmware_loading_timeout(void)
@@ -104,6 +98,77 @@ static inline long firmware_loading_timeout(void)
 	return loading_timeout > 0 ? loading_timeout * HZ : MAX_JIFFY_OFFSET;
 }
 
+#ifdef CONFIG_FW_LOADER_USER_HELPER
+
+enum {
+	FW_UMH_UNKNOWN,
+	FW_UMH_LOADING,
+	FW_UMH_DONE,
+	FW_UMH_ABORTED,
+};
+
+struct fw_umh {
+	struct completion completion;
+	unsigned long status;
+};
+
+static void fw_umh_init(struct fw_umh *fw_umh)
+{
+	init_completion(&fw_umh->completion);
+	fw_umh->status = FW_UMH_UNKNOWN;
+}
+
+static int __fw_umh_check(struct fw_umh *fw_umh, unsigned long status)
+{
+	return test_bit(status, &fw_umh->status);
+}
+
+static int fw_umh_wait_timeout(struct fw_umh *fw_umh, long timeout)
+{
+	int ret;
+
+	ret = wait_for_completion_interruptible_timeout(&fw_umh->completion,
+							timeout);
+	if (ret != 0 && test_bit(FW_UMH_ABORTED, &fw_umh->status))
+		return -ENOENT;
+
+	return ret;
+}
+
+static void __fw_umh_set(struct fw_umh *fw_umh,
+			  unsigned long status)
+{
+	set_bit(status, &fw_umh->status);
+
+	if (status == FW_UMH_DONE || status == FW_UMH_ABORTED) {
+		clear_bit(FW_UMH_LOADING, &fw_umh->status);
+		complete_all(&fw_umh->completion);
+	}
+}
+
+#define fw_umh_start(fw_umh)					\
+	__fw_umh_set(fw_umh, FW_UMH_LOADING)
+#define fw_umh_done(fw_umh)					\
+	__fw_umh_set(fw_umh, FW_UMH_DONE)
+#define fw_umh_aborted(fw_umh)					\
+	__fw_umh_set(fw_umh, FW_UMH_ABORTED)
+#define fw_umh_is_loading(fw_umh)				\
+	__fw_umh_check(fw_umh, FW_UMH_LOADING)
+#define fw_umh_is_done(fw_umh)					\
+	__fw_umh_check(fw_umh, FW_UMH_DONE)
+#define fw_umh_is_aborted(fw_umh)				\
+	__fw_umh_check(fw_umh, FW_UMH_ABORTED)
+
+#else /* CONFIG_FW_LOADER_USER_HELPER */
+
+#define fw_umh_wait_timeout(fw_st, long)	0
+
+#define fw_umh_done(fw_st)
+#define fw_umh_is_done(fw_st)			true
+#define fw_umh_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 +210,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_umh fw_umh;
 	bool is_paged_buf;
 	bool need_uevent;
 	struct page **pages;
@@ -205,8 +269,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_umh_init(&buf->fw_umh);
 	INIT_LIST_HEAD(&buf->pending_list);
 #endif
 
@@ -309,8 +373,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_umh_done(&buf->fw_umh);
 	mutex_unlock(&fw_lock);
 }
 
@@ -478,12 +541,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_umh_is_done(&buf->fw_umh))
 		return;
 
 	list_del_init(&buf->pending_list);
-	set_bit(FW_STATUS_ABORT, &buf->status);
-	complete_all(&buf->completion);
+	fw_umh_aborted(&buf->fw_umh);
 }
 
 static void fw_load_abort(struct firmware_priv *fw_priv)
@@ -496,9 +558,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 +657,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_umh_is_loading(&fw_priv->buf->fw_umh);
 	mutex_unlock(&fw_lock);
 
 	return sprintf(buf, "%d\n", loading);
@@ -653,23 +712,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_umh_is_done(&fw_buf->fw_umh)) {
 			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_umh_start(&fw_buf->fw_umh);
 		}
 		break;
 	case 0:
-		if (test_bit(FW_STATUS_LOADING, &fw_buf->status)) {
+		if (fw_umh_is_loading(&fw_buf->fw_umh)) {
 			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 +747,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_umh_aborted(&fw_buf->fw_umh);
 				written = rc;
+			} else {
+				fw_umh_done(&fw_buf->fw_umh);
 			}
-			complete_all(&fw_buf->completion);
 			break;
 		}
 		/* fallthrough */
@@ -755,7 +812,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_umh_is_done(&buf->fw_umh)) {
 		ret_count = -ENODEV;
 		goto out;
 	}
@@ -842,7 +899,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_umh_is_done(&buf->fw_umh)) {
 		retval = -ENODEV;
 		goto out;
 	}
@@ -955,8 +1012,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_umh_wait_timeout(&buf->fw_umh, timeout);
 	if (retval == -ERESTARTSYS || !retval) {
 		mutex_lock(&fw_lock);
 		fw_load_abort(fw_priv);
@@ -965,7 +1021,7 @@ static int _request_firmware_load(struct firmware_priv *fw_priv,
 		retval = 0;
 	}
 
-	if (is_fw_load_aborted(buf))
+	if (fw_umh_is_aborted(&buf->fw_umh))
 		retval = -EAGAIN;
 	else if (buf->is_paged_buf && !buf->data)
 		retval = -ENOMEM;
@@ -1015,29 +1071,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_umh_is_done(&buf->fw_umh)) {
+		if (fw_umh_is_aborted(&buf->fw_umh)) {
 			ret = -ENOENT;
 			break;
 		}
 		mutex_unlock(&fw_lock);
-		ret = wait_for_completion_interruptible(&buf->completion);
+		ret = fw_umh_wait_timeout(&buf->fw_umh, 0);
 		mutex_lock(&fw_lock);
 	}
 	mutex_unlock(&fw_lock);
@@ -1095,7 +1147,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_umh_is_aborted(&buf->fw_umh)) {
 		mutex_unlock(&fw_lock);
 		return -ENOENT;
 	}
-- 
2.7.4

[PATCH v5 3/5] firmware: rename fw_load_from_user_helper() and _request_firmware_load()

[Daniel Wagner]

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

fw_load_from_user_helper() and _request_firmware_load() are used when
CONFIG_FW_LOADER_USER_HELPER is enabled. In order to clearly mark which
part of the code is depending on UMH we stream line these functions to
match with the rest of the code, e.g. fw_umh_done().

Suggested by Luis.

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

diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
index 821babe..5e38c27 100644
--- a/drivers/base/firmware_class.c
+++ b/drivers/base/firmware_class.c
@@ -979,9 +979,8 @@ fw_create_instance(struct firmware *firmware, const char *fw_name,
 	return fw_priv;
 }
 
-/* load a firmware via user helper */
-static int _request_firmware_load(struct firmware_priv *fw_priv,
-				  unsigned int opt_flags, long timeout)
+static int _request_firmware_umh(struct firmware_priv *fw_priv,
+				 unsigned int opt_flags, long timeout)
 {
 	int retval = 0;
 	struct device *f_dev = &fw_priv->dev;
@@ -1032,9 +1031,9 @@ static int _request_firmware_load(struct firmware_priv *fw_priv,
 	return retval;
 }
 
-static int fw_load_from_user_helper(struct firmware *firmware,
-				    const char *name, struct device *device,
-				    unsigned int opt_flags, long timeout)
+static int fw_get_umh_firmware(struct firmware *firmware, const char *name,
+			struct device *device, unsigned int opt_flags,
+			long timeout)
 {
 	struct firmware_priv *fw_priv;
 
@@ -1043,7 +1042,7 @@ static int fw_load_from_user_helper(struct firmware *firmware,
 		return PTR_ERR(fw_priv);
 
 	fw_priv->buf = firmware->priv;
-	return _request_firmware_load(fw_priv, opt_flags, timeout);
+	return _request_firmware_umh(fw_priv, opt_flags, timeout);
 }
 
 #ifdef CONFIG_PM_SLEEP
@@ -1064,9 +1063,9 @@ static void kill_requests_without_uevent(void)
 
 #else /* CONFIG_FW_LOADER_USER_HELPER */
 static inline int
-fw_load_from_user_helper(struct firmware *firmware, const char *name,
-			 struct device *device, unsigned int opt_flags,
-			 long timeout)
+fw_get_umh_firmware(struct firmware *firmware, const char *name,
+		    struct device *device, unsigned int opt_flags,
+		    long timeout)
 {
 	return -ENOENT;
 }
@@ -1242,8 +1241,8 @@ _request_firmware(const struct firmware **firmware_p, const char *name,
 				 name, ret);
 		if (opt_flags & FW_OPT_USERHELPER) {
 			dev_warn(device, "Falling back to user helper\n");
-			ret = fw_load_from_user_helper(fw, name, device,
-						       opt_flags, timeout);
+			ret = fw_get_umh_firmware(fw, name, device,
+						  opt_flags, timeout);
 		}
 	}
 
-- 
2.7.4

[PATCH v5 4/5] 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 they are all mutual exclusive there are
only a few simple state transition we can model this simplify.

	   UNKNOWN -> LOADING -> DONE | ABORTED

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

diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
index 5e38c27..8f5838c 100644
--- a/drivers/base/firmware_class.c
+++ b/drivers/base/firmware_class.c
@@ -109,7 +109,7 @@ enum {
 
 struct fw_umh {
 	struct completion completion;
-	unsigned long status;
+	u8 status;
 };
 
 static void fw_umh_init(struct fw_umh *fw_umh)
@@ -120,7 +120,7 @@ static void fw_umh_init(struct fw_umh *fw_umh)
 
 static int __fw_umh_check(struct fw_umh *fw_umh, unsigned long status)
 {
-	return test_bit(status, &fw_umh->status);
+	return fw_umh->status == status;
 }
 
 static int fw_umh_wait_timeout(struct fw_umh *fw_umh, long timeout)
@@ -129,7 +129,7 @@ static int fw_umh_wait_timeout(struct fw_umh *fw_umh, long timeout)
 
 	ret = wait_for_completion_interruptible_timeout(&fw_umh->completion,
 							timeout);
-	if (ret != 0 && test_bit(FW_UMH_ABORTED, &fw_umh->status))
+	if (ret != 0 && READ_ONCE(fw_umh->status) == FW_UMH_ABORTED)
 		return -ENOENT;
 
 	return ret;
@@ -138,12 +138,10 @@ static int fw_umh_wait_timeout(struct fw_umh *fw_umh, long timeout)
 static void __fw_umh_set(struct fw_umh *fw_umh,
 			  unsigned long status)
 {
-	set_bit(status, &fw_umh->status);
+	WRITE_ONCE(fw_umh->status, status);
 
-	if (status == FW_UMH_DONE || status == FW_UMH_ABORTED) {
-		clear_bit(FW_UMH_LOADING, &fw_umh->status);
+	if (status == FW_UMH_DONE || status == FW_UMH_ABORTED)
 		complete_all(&fw_umh->completion);
-	}
 }
 
 #define fw_umh_start(fw_umh)					\
-- 
2.7.4

[PATCH v5 5/5] firmware: do not use fw_lock for fw_umh 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_umh can be used without needing the fw_lock to protect
its state transition and wake ups.

fw_umh is holds the state in status and the completion is used to
wake up all waiters (in this case that is the user land helper so only
one). This operation has to be 'atomic' to avoid races.  We can do this
by using swait which takes care we don't miss any wake up.

We use also swait instead of wait because don't need all the additional
features wait provides.

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

diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
index 8f5838c..1a28070 100644
--- a/drivers/base/firmware_class.c
+++ b/drivers/base/firmware_class.c
@@ -30,6 +30,7 @@
 #include <linux/syscore_ops.h>
 #include <linux/reboot.h>
 #include <linux/security.h>
+#include <linux/swait.h>
 
 #include <generated/utsrelease.h>
 
@@ -108,13 +109,13 @@ enum {
 };
 
 struct fw_umh {
-	struct completion completion;
+	struct swait_queue_head wq;
 	u8 status;
 };
 
 static void fw_umh_init(struct fw_umh *fw_umh)
 {
-	init_completion(&fw_umh->completion);
+	init_swait_queue_head(&fw_umh->wq);
 	fw_umh->status = FW_UMH_UNKNOWN;
 }
 
@@ -123,13 +124,19 @@ static int __fw_umh_check(struct fw_umh *fw_umh, unsigned long status)
 	return fw_umh->status == status;
 }
 
+static inline bool __fw_umh_is_done(unsigned long status)
+{
+	return status == FW_UMH_DONE || status == FW_UMH_ABORTED;
+}
+
 static int fw_umh_wait_timeout(struct fw_umh *fw_umh, long timeout)
 {
 	int ret;
 
-	ret = wait_for_completion_interruptible_timeout(&fw_umh->completion,
-							timeout);
-	if (ret != 0 && READ_ONCE(fw_umh->status) == FW_UMH_ABORTED)
+	ret = swait_event_interruptible_timeout(fw_umh->wq,
+				__fw_umh_is_done(READ_ONCE(fw_umh->status)),
+				timeout);
+	if (ret != 0 && fw_umh->status == FW_UMH_ABORTED)
 		return -ENOENT;
 
 	return ret;
@@ -141,7 +148,7 @@ static void __fw_umh_set(struct fw_umh *fw_umh,
 	WRITE_ONCE(fw_umh->status, status);
 
 	if (status == FW_UMH_DONE || status == FW_UMH_ABORTED)
-		complete_all(&fw_umh->completion);
+		swake_up(&fw_umh->wq);
 }
 
 #define fw_umh_start(fw_umh)					\
@@ -367,14 +374,6 @@ static const char * const fw_path[] = {
 module_param_string(path, fw_path_para, sizeof(fw_path_para), 0644);
 MODULE_PARM_DESC(path, "customized firmware image search path with a higher priority than default path");
 
-static void fw_finish_direct_load(struct device *device,
-				  struct firmware_buf *buf)
-{
-	mutex_lock(&fw_lock);
-	fw_umh_done(&buf->fw_umh);
-	mutex_unlock(&fw_lock);
-}
-
 static int
 fw_get_filesystem_firmware(struct device *device, struct firmware_buf *buf)
 {
@@ -421,7 +420,7 @@ fw_get_filesystem_firmware(struct device *device, struct firmware_buf *buf)
 		}
 		dev_dbg(device, "direct-loading %s\n", buf->fw_id);
 		buf->size = size;
-		fw_finish_direct_load(device, buf);
+		fw_umh_done(&buf->fw_umh);
 		break;
 	}
 	__putname(path);
@@ -1074,25 +1073,6 @@ static inline void kill_requests_without_uevent(void) { }
 
 #endif /* CONFIG_FW_LOADER_USER_HELPER */
 
-/* wait until the shared firmware_buf becomes ready (or error) */
-static int sync_cached_firmware_buf(struct firmware_buf *buf)
-{
-	int ret = 0;
-
-	mutex_lock(&fw_lock);
-	while (!fw_umh_is_done(&buf->fw_umh)) {
-		if (fw_umh_is_aborted(&buf->fw_umh)) {
-			ret = -ENOENT;
-			break;
-		}
-		mutex_unlock(&fw_lock);
-		ret = fw_umh_wait_timeout(&buf->fw_umh, 0);
-		mutex_lock(&fw_lock);
-	}
-	mutex_unlock(&fw_lock);
-	return ret;
-}
-
 /* prepare firmware and firmware_buf structs;
  * return 0 if a firmware is already assigned, 1 if need to load one,
  * or a negative error code
@@ -1126,7 +1106,7 @@ _request_firmware_prepare(struct firmware **firmware_p, const char *name,
 	firmware->priv = buf;
 
 	if (ret > 0) {
-		ret = sync_cached_firmware_buf(buf);
+		ret = fw_umh_wait_timeout(&buf->fw_umh, 0);
 		if (!ret) {
 			fw_set_page_data(buf, firmware);
 			return 0; /* assigned */
-- 
2.7.4

Re: [PATCH v5 0/5] firmware: encapsulate firmware loading status

[Luis R. Rodriguez]

On Fri, Sep 09, 2016 at 02:12:19PM +0200, Daniel Wagner wrote:
> From: Daniel Wagner <daniel.wagner@bmw-carit.de>
> 
> Hi,
> 
> The firmware user helper code tracks the current state of the loading
> process via an member of struct firmware_buf and a completion. Let's
> encapsulate this simple state machine into struct fw_status. The aim is
> to increase readability and reduce the usage of the fw_lock.
> 
> Luis asked by to add a few CC for getting a wider audience. If you
> haven't seen this before the initial version of this series has some
> more details on the motivation for this series:
> 
> http://www.spinics.net/lists/linux-wireless/msg153005.html
> 
> I tested this series with fw_userhelper.sh and fw_filesystem.sh under
> kvm and also let it run on real hardware. The series is also available
> here:
> 
> https://git.kernel.org/cgit/linux/kernel/git/wagi/linux.git/log/?h=firmware_async-7

Ah, this series is looking so much nicer now thanks!

> I had pushed earlier version of series there and haven't got any 0-day
> bug reports so far.

0-day eventually sends a complete success report if you're good. That's
when you can get a warm fuzzy over your series. Found just one more issue.
Since that means another respin I'll provide some other nitpicks.


  Luis

Re: [PATCH v5 1/5] firmware: document user mode helper lock usage

[Luis R. Rodriguez]

On Fri, Sep 09, 2016 at 02:12:20PM +0200, Daniel Wagner wrote:
> From: Daniel Wagner <daniel.wagner@bmw-carit.de>
> 
> The lock is also used to generate warnings when a direct
> firmware load is requested too early.

I've determined the firmware cache lets us bail out of this
consideration now. If Ming agrees with the logic we don't need this
patch and you can continue as you had intended. Sorry for the trouble.

  Luis

Re: [PATCH v5 3/5] firmware: rename fw_load_from_user_helper() and _request_firmware_load()

[Luis R. Rodriguez]

On Fri, Sep 09, 2016 at 02:12:22PM +0200, Daniel Wagner wrote:
> From: Daniel Wagner <daniel.wagner@bmw-carit.de>
> 
> fw_load_from_user_helper() and _request_firmware_load() are used when
> CONFIG_FW_LOADER_USER_HELPER is enabled. In order to clearly mark which
> part of the code is depending on UMH we stream line these functions to
> match with the rest of the code, e.g. fw_umh_done().
> 
> Suggested by Luis.
> 
> Cc: Ming Lei <ming.lei@canonical.com>
> Cc: Luis R. Rodriguez <mcgrof@kernel.org>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Signed-off-by: Daniel Wagner <daniel.wagner@bmw-carit.de>

Acked-by: Luis R. Rodriguez <mcgrof@kernel.org>

  Luis

Re: [PATCH v5 2/5] firmware: encapsulate firmware loading status

[Luis R. Rodriguez]

On Fri, Sep 09, 2016 at 02:12:21PM +0200, Daniel Wagner wrote:
> +#else /* CONFIG_FW_LOADER_USER_HELPER */
> +
> +#define fw_umh_wait_timeout(fw_st, long)	0
> +
> +#define fw_umh_done(fw_st)
> +#define fw_umh_is_done(fw_st)			true
> +#define fw_umh_is_aborted(fw_st)		false
> +

If we do proceed with compartmentalizing the UMH timeout crap
then my hope is this piece of code will not require the
fw_umh_wait_timeout() def.

  Luis

Re: [PATCH v5 2/5] firmware: encapsulate firmware loading status

[Luis R. Rodriguez]

On Fri, Sep 09, 2016 at 02:12:21PM +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.
> 
> Cc: Ming Lei <ming.lei@canonical.com>
> Cc: Luis R. Rodriguez <mcgrof@kernel.org>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Signed-off-by: Daniel Wagner <daniel.wagner@bmw-carit.de>
> ---
>  drivers/base/firmware_class.c | 130 +++++++++++++++++++++++++++++-------------
>  1 file changed, 91 insertions(+), 39 deletions(-)
> 
> diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
> index 8eba1fb..821babe 100644
> --- a/drivers/base/firmware_class.c
> +++ b/drivers/base/firmware_class.c
> @@ -91,12 +91,6 @@ static inline bool fw_is_builtin_firmware(const struct firmware *fw)
>  }
>  #endif
>  
> -enum {
> -	FW_STATUS_LOADING,
> -	FW_STATUS_DONE,
> -	FW_STATUS_ABORT,
> -};
> -
>  static int loading_timeout = 60;	/* In seconds */
>  
>  static inline long firmware_loading_timeout(void)
> @@ -104,6 +98,77 @@ static inline long firmware_loading_timeout(void)
>  	return loading_timeout > 0 ? loading_timeout * HZ : MAX_JIFFY_OFFSET;
>  }
>  
> +#ifdef CONFIG_FW_LOADER_USER_HELPER
> +
> +enum {
> +	FW_UMH_UNKNOWN,
> +	FW_UMH_LOADING,
> +	FW_UMH_DONE,
> +	FW_UMH_ABORTED,
> +};

Note the enum here is anonymous still. That's a bit of a lost opportunity. If we name
it we can then just use it in arguments, but that can be done later in your other
patch. So say we name it enum fw_umh_status or whatever.

  Luis

Re: [PATCH v5 4/5] firmware: drop bit ops in favor of simple state machine

[Luis R. Rodriguez]

On Fri, Sep 09, 2016 at 02:12:23PM +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

We track the state of the firmware usermode helper loading with bit ops.

> has only a couple of states and they are all mutual exclusive there are
> only a few simple state transition we can model this simplify.
> 
> 	   UNKNOWN -> LOADING -> DONE | ABORTED

If you also do the change suggested below you'd have to annotate that change in the
commit log as well.

> 
> Cc: Ming Lei <ming.lei@canonical.com>
> Cc: Luis R. Rodriguez <mcgrof@kernel.org>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Signed-off-by: Daniel Wagner <daniel.wagner@bmw-carit.de>
> ---
>  drivers/base/firmware_class.c | 12 +++++-------
>  1 file changed, 5 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
> index 5e38c27..8f5838c 100644
> --- a/drivers/base/firmware_class.c
> +++ b/drivers/base/firmware_class.c
> @@ -109,7 +109,7 @@ enum {
>  
>  struct fw_umh {
>  	struct completion completion;
> -	unsigned long status;
> +	u8 status;
 
Sorry I know I suggested the u8 but below you end up still using unsigned long status.
Instead of fixing this please consider changing:

  struct fw_umh {
       struct completion completion;
 -     unsigned long status;
 +     enum fw_umh_status status;

Then you can use the enum fw_umh_status status in function arguments, I've used this
trick in other codebases to ensure that the data type for the status passed then
matches the same one expected, *and* if you use a switch() statement the compiler
will complain and moan about missing values (unless a default switch statement
is present). For such simple state machines then this is better practice.

>  };
>  
>  static void fw_umh_init(struct fw_umh *fw_umh)
> @@ -120,7 +120,7 @@ static void fw_umh_init(struct fw_umh *fw_umh)
>  
>  static int __fw_umh_check(struct fw_umh *fw_umh, unsigned long status)
>  {
> -	return test_bit(status, &fw_umh->status);
> +	return fw_umh->status == status;

Why does this not use READ_ONCE(fw_umh->status) ?

  Luis

Re: [PATCH v5 2/5] firmware: encapsulate firmware loading status

[Daniel Wagner]

Hi Luis,


On 09/09/2016 02:12 PM, Daniel Wagner wrote:
> 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.

I don't think we are able to move the completion code into a 
CONFIG_FW_LOADER_HELPER section. The direct loading path uses completion 
as well.

> +#ifdef CONFIG_FW_LOADER_USER_HELPER
> +
> +enum {
> +	FW_UMH_UNKNOWN,
> +	FW_UMH_LOADING,
> +	FW_UMH_DONE,
> +	FW_UMH_ABORTED,
> +};

The direct loading path just uses two states, LOADING and DONE. ABORTED 
is not used.

> +struct fw_umh {
> +	struct completion completion;
> +	unsigned long status;
> +};
> +
> +static void fw_umh_init(struct fw_umh *fw_umh)
> +{
> +	init_completion(&fw_umh->completion);
> +	fw_umh->status = FW_UMH_UNKNOWN;
> +}
> +
> +static int __fw_umh_check(struct fw_umh *fw_umh, unsigned long status)
> +{
> +	return test_bit(status, &fw_umh->status);
> +}
> +
> +static int fw_umh_wait_timeout(struct fw_umh *fw_umh, long timeout)
> +{
> +	int ret;
> +
> +	ret = wait_for_completion_interruptible_timeout(&fw_umh->completion,
> +							timeout);
> +	if (ret != 0 && test_bit(FW_UMH_ABORTED, &fw_umh->status))
> +		return -ENOENT;
> +
> +	return ret;
> +}
> +
> +static void __fw_umh_set(struct fw_umh *fw_umh,
> +			  unsigned long status)
> +{
> +	set_bit(status, &fw_umh->status);
> +
> +	if (status == FW_UMH_DONE || status == FW_UMH_ABORTED) {
> +		clear_bit(FW_UMH_LOADING, &fw_umh->status);
> +		complete_all(&fw_umh->completion);
> +	}
> +}
> +
> +#define fw_umh_start(fw_umh)					\
> +	__fw_umh_set(fw_umh, FW_UMH_LOADING)
> +#define fw_umh_done(fw_umh)					\
> +	__fw_umh_set(fw_umh, FW_UMH_DONE)
> +#define fw_umh_aborted(fw_umh)					\
> +	__fw_umh_set(fw_umh, FW_UMH_ABORTED)
> +#define fw_umh_is_loading(fw_umh)				\
> +	__fw_umh_check(fw_umh, FW_UMH_LOADING)
> +#define fw_umh_is_done(fw_umh)					\
> +	__fw_umh_check(fw_umh, FW_UMH_DONE)
> +#define fw_umh_is_aborted(fw_umh)				\
> +	__fw_umh_check(fw_umh, FW_UMH_ABORTED)
> +
> +#else /* CONFIG_FW_LOADER_USER_HELPER */
> +
> +#define fw_umh_wait_timeout(fw_st, long)	0
> +
> +#define fw_umh_done(fw_st)
> +#define fw_umh_is_done(fw_st)			true
> +#define fw_umh_is_aborted(fw_st)		false

We still need the implementation for fw_umh_wait_timeout() and 
fw_umh_start(), fw_umh_done() etc. fw_umh_is_aborted() is not needed.


> @@ -309,8 +373,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_umh_done(&buf->fw_umh);
>  	mutex_unlock(&fw_lock);
>  }

Here we signal that we have loaded the firmware

>  /* 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_umh_is_done(&buf->fw_umh)) {
> +		if (fw_umh_is_aborted(&buf->fw_umh)) {
>  			ret = -ENOENT;
>  			break;
>  		}
>  		mutex_unlock(&fw_lock);
> -		ret = wait_for_completion_interruptible(&buf->completion);
> +		ret = fw_umh_wait_timeout(&buf->fw_umh, 0);
>  		mutex_lock(&fw_lock);
>  	}

and here we here we wait for it.

So I suggest to rename it back to fw_status_ and don't move it inside a 
CONFIG_FW_LOADER_HELPER section. Drop the special handling of the 
ABORTED and add instead a comment that the ABORTED state is not used for 
direct loading. This special handling makes unnecessary more complex. 
This is a slowpath and this micro optimization is helping to maintain 
the code.

cheers,
daniel

Re: [PATCH v5 1/5] firmware: document user mode helper lock usage

[Ming Lei]

On Sat, Sep 10, 2016 at 6:14 AM, Luis R. Rodriguez <mcgrof@kernel.org> wrote:
> On Fri, Sep 09, 2016 at 02:12:20PM +0200, Daniel Wagner wrote:
>> From: Daniel Wagner <daniel.wagner@bmw-carit.de>
>>
>> The lock is also used to generate warnings when a direct
>> firmware load is requested too early.
>
> I've determined the firmware cache lets us bail out of this
> consideration now. If Ming agrees with the logic we don't need this
> patch and you can continue as you had intended. Sorry for the trouble.

IMO it is helpful to add comment about using the lock for direct loading,
and we can sort it out in future if anyone want to improve it.

So for this patch, I am fine.

Thanks,

Re: [PATCH v5 2/5] firmware: encapsulate firmware loading status

[Luis R. Rodriguez]

On Tue, Sep 13, 2016 at 11:47:08AM +0200, Daniel Wagner wrote:
> Hi Luis,
> 
> 
> On 09/09/2016 02:12 PM, Daniel Wagner wrote:
> >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.
> 
> I don't think we are able to move the completion code into a
> CONFIG_FW_LOADER_HELPER section. The direct loading path uses
> completion as well.

Where?

> >+#ifdef CONFIG_FW_LOADER_USER_HELPER
> >+
> >+enum {
> >+	FW_UMH_UNKNOWN,
> >+	FW_UMH_LOADING,
> >+	FW_UMH_DONE,
> >+	FW_UMH_ABORTED,
> >+};
> 
> The direct loading path just uses two states, LOADING and DONE.
> ABORTED is not used.
> 
> >+struct fw_umh {
> >+	struct completion completion;
> >+	unsigned long status;
> >+};
> >+
> >+static void fw_umh_init(struct fw_umh *fw_umh)
> >+{
> >+	init_completion(&fw_umh->completion);
> >+	fw_umh->status = FW_UMH_UNKNOWN;
> >+}
> >+
> >+static int __fw_umh_check(struct fw_umh *fw_umh, unsigned long status)
> >+{
> >+	return test_bit(status, &fw_umh->status);
> >+}
> >+
> >+static int fw_umh_wait_timeout(struct fw_umh *fw_umh, long timeout)
> >+{
> >+	int ret;
> >+
> >+	ret = wait_for_completion_interruptible_timeout(&fw_umh->completion,
> >+							timeout);
> >+	if (ret != 0 && test_bit(FW_UMH_ABORTED, &fw_umh->status))
> >+		return -ENOENT;
> >+
> >+	return ret;
> >+}
> >+
> >+static void __fw_umh_set(struct fw_umh *fw_umh,
> >+			  unsigned long status)
> >+{
> >+	set_bit(status, &fw_umh->status);
> >+
> >+	if (status == FW_UMH_DONE || status == FW_UMH_ABORTED) {
> >+		clear_bit(FW_UMH_LOADING, &fw_umh->status);
> >+		complete_all(&fw_umh->completion);
> >+	}
> >+}
> >+
> >+#define fw_umh_start(fw_umh)					\
> >+	__fw_umh_set(fw_umh, FW_UMH_LOADING)
> >+#define fw_umh_done(fw_umh)					\
> >+	__fw_umh_set(fw_umh, FW_UMH_DONE)
> >+#define fw_umh_aborted(fw_umh)					\
> >+	__fw_umh_set(fw_umh, FW_UMH_ABORTED)
> >+#define fw_umh_is_loading(fw_umh)				\
> >+	__fw_umh_check(fw_umh, FW_UMH_LOADING)
> >+#define fw_umh_is_done(fw_umh)					\
> >+	__fw_umh_check(fw_umh, FW_UMH_DONE)
> >+#define fw_umh_is_aborted(fw_umh)				\
> >+	__fw_umh_check(fw_umh, FW_UMH_ABORTED)
> >+
> >+#else /* CONFIG_FW_LOADER_USER_HELPER */
> >+
> >+#define fw_umh_wait_timeout(fw_st, long)	0
> >+
> >+#define fw_umh_done(fw_st)
> >+#define fw_umh_is_done(fw_st)			true
> >+#define fw_umh_is_aborted(fw_st)		false
> 
> We still need the implementation for fw_umh_wait_timeout() and
> fw_umh_start(), fw_umh_done() etc.

Why?

> fw_umh_is_aborted() is not
> needed.
> 
> 
> >@@ -309,8 +373,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_umh_done(&buf->fw_umh);
> > 	mutex_unlock(&fw_lock);
> > }
> 
> Here we signal that we have loaded the firmware

The struct firmware_buf is only used for the sysfs stuff no?

> > /* 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_umh_is_done(&buf->fw_umh)) {
> >+		if (fw_umh_is_aborted(&buf->fw_umh)) {
> > 			ret = -ENOENT;
> > 			break;
> > 		}
> > 		mutex_unlock(&fw_lock);
> >-		ret = wait_for_completion_interruptible(&buf->completion);
> >+		ret = fw_umh_wait_timeout(&buf->fw_umh, 0);
> > 		mutex_lock(&fw_lock);
> > 	}
> 
> and here we here we wait for it.

Likewise.

  Luis

Re: [PATCH v5 1/5] firmware: document user mode helper lock usage

[Luis R. Rodriguez]

On Thu, Sep 22, 2016 at 10:36:39AM +0800, Ming Lei wrote:
> On Sat, Sep 10, 2016 at 6:14 AM, Luis R. Rodriguez <mcgrof@kernel.org> wrote:
> > On Fri, Sep 09, 2016 at 02:12:20PM +0200, Daniel Wagner wrote:
> >> From: Daniel Wagner <daniel.wagner@bmw-carit.de>
> >>
> >> The lock is also used to generate warnings when a direct
> >> firmware load is requested too early.
> >
> > I've determined the firmware cache lets us bail out of this
> > consideration now. If Ming agrees with the logic we don't need this
> > patch and you can continue as you had intended. Sorry for the trouble.
> 
> IMO it is helpful to add comment about using the lock for direct loading,
> and we can sort it out in future if anyone want to improve it.
> 
> So for this patch, I am fine.

Yeah that's fine too. Thew new API, the driver data API will not use the UMH
lock, so we can just skip the lock there if we feel mushy about removing
it from the existing API on direct firmware loading. From what I've determined
though its simply not needed.

  Luis

Re: [PATCH v5 1/5] firmware: document user mode helper lock usage

[Luis R. Rodriguez]

On Fri, Sep 23, 2016 at 10:13:44AM +0200, Daniel Wagner wrote:
> On 09/22/2016 04:36 AM, Ming Lei wrote:
> >On Sat, Sep 10, 2016 at 6:14 AM, Luis R. Rodriguez <mcgrof@kernel.org> wrote:
> >>On Fri, Sep 09, 2016 at 02:12:20PM +0200, Daniel Wagner wrote:
> >>>From: Daniel Wagner <daniel.wagner@bmw-carit.de>
> >>>
> >>>The lock is also used to generate warnings when a direct
> >>>firmware load is requested too early.
> >>
> >>I've determined the firmware cache lets us bail out of this
> >>consideration now. If Ming agrees with the logic we don't need this
> >>patch and you can continue as you had intended. Sorry for the trouble.
> >
> >IMO it is helpful to add comment about using the lock for direct loading,
> >and we can sort it out in future if anyone want to improve it.
> >
> >So for this patch, I am fine.
> 
> Sorry, I am a bit confused now. What is the consensus here:
> 
>  a) add a comment to _request_firmware() as in this patch #1 v5

The adding a comment note from Daniel was that the UMH lock is *not*
needed on the direct firmware loading case, he's lazy to remove it
now so he'll just add a comment.

>  b) move the umh locking to fw_load_from_user_helper() as in
>     patch #1 v4

This is fine and IMHO the non-lazy approach.

To be clear -- I did my own vetting of the removal of the lock by
inspecting the original purpose of the UMH lock being added on the
history Linux git tree, having a secondary review of that would be
appreciated as well.

  Luis

Re: [PATCH v5 2/5] firmware: encapsulate firmware loading status

[Daniel Wagner]

Hi Luis,

On 10/05/2016 10:27 PM, Luis R. Rodriguez wrote:
> On Tue, Sep 13, 2016 at 11:47:08AM +0200, Daniel Wagner wrote:
>> On 09/09/2016 02:12 PM, Daniel Wagner wrote:
>>> 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.
>>
>> I don't think we are able to move the completion code into a
>> CONFIG_FW_LOADER_HELPER section. The direct loading path uses
>> completion as well.
>
> Where?

If you look at the current code (not these patches) you have dependency 
via the firmware_buf for two concurrent _request_firmware() calls:


1nd request (waker context)

_request_firmware()
   _request_firmware_prepare()
     fw_lookup_and_allocate_buf()   # no pendending request
                                    # returns 0 -> load firmware

   fw_get_fileystem_firmware()
     fw_finish_direct_load()
       complete_all()


2nd request (waiter context)

_request_firmware()
   _request_firmware_prepare()
      fw_lookup_allocate_buf()      # finds previously allocated buf
                                    # returns 1 -> wait for loading
      sync_cached_firmware_buf()
         wait_for_completion_interruptible_timeout()


>>> +#else /* CONFIG_FW_LOADER_USER_HELPER */
>>> +
>>> +#define fw_umh_wait_timeout(fw_st, long)	0
>>> +
>>> +#define fw_umh_done(fw_st)
>>> +#define fw_umh_is_done(fw_st)			true
>>> +#define fw_umh_is_aborted(fw_st)		false
>>
>> We still need the implementation for fw_umh_wait_timeout() and
>> fw_umh_start(), fw_umh_done() etc.
>
> Why?

See above.

>>> @@ -309,8 +373,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_umh_done(&buf->fw_umh);
>>> 	mutex_unlock(&fw_lock);
>>> }
>>
>> Here we signal that we have loaded the firmware
>
> The struct firmware_buf is only used for the sysfs stuff no?

I don't know, I was looking at the code in firmware_class.c not any 
users. Why is that important?

>>> /* 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_umh_is_done(&buf->fw_umh)) {
>>> +		if (fw_umh_is_aborted(&buf->fw_umh)) {
>>> 			ret = -ENOENT;
>>> 			break;
>>> 		}
>>> 		mutex_unlock(&fw_lock);
>>> -		ret = wait_for_completion_interruptible(&buf->completion);
>>> +		ret = fw_umh_wait_timeout(&buf->fw_umh, 0);
>>> 		mutex_lock(&fw_lock);
>>> 	}
>>
>> and here we here we wait for it.
>
> Likewise.

As I tried to explain above the buffering code is depending on completion.

cheers,
daniel

Re: [PATCH v5 1/5] firmware: document user mode helper lock usage

[Luis R. Rodriguez]

On Fri, Oct 07, 2016 at 08:16:29AM +0200, Daniel Wagner wrote:
> > > Sorry, I am a bit confused now. What is the consensus here:
> > > 
> > >  a) add a comment to _request_firmware() as in this patch #1 v5
> > 
> > The adding a comment note from Daniel was that the UMH lock is *not*
> > needed on the direct firmware loading case, he's lazy to remove it
> > now so he'll just add a comment.
> 
> IIRC, we hadn't really settle on what the right solution is or I couldn't
> parse it. That is why I am asking specifically which version is the right
> thing. Don't worry I don't want to shortcut here :)

The removal of the lock from the general case is the right thing, I however
wanted Ming to also acknowledge this change, I suppose he can do so if you
supply a respin, or you can wait.

  Luis

Re: [PATCH v5 2/5] firmware: encapsulate firmware loading status

[Luis R. Rodriguez]

On Fri, Oct 07, 2016 at 01:41:21PM +0200, Daniel Wagner wrote:
> Hi Luis,
> 
> On 10/05/2016 10:27 PM, Luis R. Rodriguez wrote:
> > On Tue, Sep 13, 2016 at 11:47:08AM +0200, Daniel Wagner wrote:
> > > On 09/09/2016 02:12 PM, Daniel Wagner wrote:
> > > > The firmware user helper code tracks the current state of the loading
> > > > process via unsigned long status and a completion 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.
> > > 
> > > I don't think we are able to move the completion code into a
> > > CONFIG_FW_LOADER_HELPER section. The direct loading path uses
> > > completion as well.
> > 
> > Where?
> 
> If you look at the current code (not these patches) you have dependency via
> the firmware_buf for two concurrent _request_firmware() calls:
> 
> 
> 1nd request (waker context)
> 
> _request_firmware()
>   _request_firmware_prepare()
>     fw_lookup_and_allocate_buf()   # no pendending request
>                                    # returns 0 -> load firmware

"no pending request" is an invalid association with what fw_lookup_and_allocate_buf()
does, its also why I have asked for this to be renamed, it looks for the firmware
in the fw cache, if it finds it it returns 1. Otherwise it creates a new buf
entry and adds it to the fw cache, and returns 0.

> 
>   fw_get_fileystem_firmware()
>     fw_finish_direct_load()
>       complete_all()
> 
> 
> 2nd request (waiter context)
> 
> _request_firmware()
>   _request_firmware_prepare()
>      fw_lookup_allocate_buf()      # finds previously allocated buf
>                                    # returns 1 -> wait for loading
>      sync_cached_firmware_buf()
>         wait_for_completion_interruptible_timeout()

No, that's wait_for_completion_interruptible() not
           wait_for_completion_interruptible_timeout()

Also note that we only call sync_cached_firmware_buf()
*iff* fw_lookup_and_allocate_buf() returned the 1 -- I mentioned
when this happens above. That happens only if we already had the entry on
the fw cache. As it stands -- concurrent calls against the same fw name
could cause a clash here, as such, the wait_for_completion_interruptible()
is indeed still needed.

Further optimizations can be considered later but for indeed, agreed
that completion is needed even for the direct fw load case. The timeout
though, I don't see a reason for it.

> > > > +#else /* CONFIG_FW_LOADER_USER_HELPER */
> > > > +
> > > > +#define fw_umh_wait_timeout(fw_st, long)	0
> > > > +
> > > > +#define fw_umh_done(fw_st)
> > > > +#define fw_umh_is_done(fw_st)			true
> > > > +#define fw_umh_is_aborted(fw_st)		false
> > > 
> > > We still need the implementation for fw_umh_wait_timeout() and
> > > fw_umh_start(), fw_umh_done() etc.
> > 
> > Why?
> 
> See above.

Sure, but note how the timeout is not used.

> > > > @@ -309,8 +373,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_umh_done(&buf->fw_umh);
> > > > 	mutex_unlock(&fw_lock);
> > > > }
> > > 
> > > Here we signal that we have loaded the firmware
> > 
> > The struct firmware_buf is only used for the sysfs stuff no?
> 
> I don't know, I was looking at the code in firmware_class.c not any users.
> Why is that important?

Sorry I meant struct firmware_priv is used by sysfs stuff only, the sysfs stuff
is only used for the FW UMH.

> > > > /* 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_umh_is_done(&buf->fw_umh)) {
> > > > +		if (fw_umh_is_aborted(&buf->fw_umh)) {
> > > > 			ret = -ENOENT;
> > > > 			break;
> > > > 		}
> > > > 		mutex_unlock(&fw_lock);
> > > > -		ret = wait_for_completion_interruptible(&buf->completion);
> > > > +		ret = fw_umh_wait_timeout(&buf->fw_umh, 0);
> > > > 		mutex_lock(&fw_lock);
> > > > 	}
> > > 
> > > and here we here we wait for it.
> > 
> > Likewise.
> 
> As I tried to explain above the buffering code is depending on completion.

OK sure agreed. The timeout, no though, unless I missed something?

  Luis

Re: [PATCH v5 2/5] firmware: encapsulate firmware loading status

[Daniel Wagner]

On 10/10/2016 10:37 PM, Luis R. Rodriguez wrote:
> On Fri, Oct 07, 2016 at 01:41:21PM +0200, Daniel Wagner wrote:
>> On 10/05/2016 10:27 PM, Luis R. Rodriguez wrote:
>>> On Tue, Sep 13, 2016 at 11:47:08AM +0200, Daniel Wagner wrote:
>>>> On 09/09/2016 02:12 PM, Daniel Wagner wrote:
>>>>> The firmware user helper code tracks the current state of the loading
>>>>> process via unsigned long status and a completion 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.
>>>>
>>>> I don't think we are able to move the completion code into a
>>>> CONFIG_FW_LOADER_HELPER section. The direct loading path uses
>>>> completion as well.
>>>
>>> Where?
>>
>> If you look at the current code (not these patches) you have dependency via
>> the firmware_buf for two concurrent _request_firmware() calls:
>>
>>
>> 1nd request (waker context)
>>
>> _request_firmware()
>>   _request_firmware_prepare()
>>     fw_lookup_and_allocate_buf()   # no pendending request
>>                                    # returns 0 -> load firmware
>
> "no pending request" is an invalid association with what fw_lookup_and_allocate_buf()
> does, its also why I have asked for this to be renamed, it looks for the firmware
> in the fw cache, if it finds it it returns 1. Otherwise it creates a new buf
> entry and adds it to the fw cache, and returns 0.

I used 'no pending request' for what you describe below with first call. 
So yes, either the buffer is allocated for the given name or it's 
missing. It doesn't tell anything about the load status of the firmware.

>>   fw_get_fileystem_firmware()
>>     fw_finish_direct_load()
>>       complete_all()
>>
>>
>> 2nd request (waiter context)
>>
>> _request_firmware()
>>   _request_firmware_prepare()
>>      fw_lookup_allocate_buf()      # finds previously allocated buf
>>                                    # returns 1 -> wait for loading
>>      sync_cached_firmware_buf()
>>         wait_for_completion_interruptible_timeout()
>
> No, that's wait_for_completion_interruptible() not
>            wait_for_completion_interruptible_timeout()

I confused that one from _request_firmware_load().

> Also note that we only call sync_cached_firmware_buf()
> *iff* fw_lookup_and_allocate_buf() returned the 1 -- I mentioned
> when this happens above. That happens only if we already had the entry on
> the fw cache. As it stands -- concurrent calls against the same fw name
> could cause a clash here, as such, the wait_for_completion_interruptible()
> is indeed still needed.
>
> Further optimizations can be considered later but for indeed, agreed
> that completion is needed even for the direct fw load case. The timeout
> though, I don't see a reason for it.

So I think I found the source of the confusion about 
fw_umh_wait_timeout(). When providing a timeout value of 0 you get the 
wait_for_completion_interruptible() version.

>>>>> +#else /* CONFIG_FW_LOADER_USER_HELPER */
>>>>> +
>>>>> +#define fw_umh_wait_timeout(fw_st, long)	0
>>>>> +
>>>>> +#define fw_umh_done(fw_st)
>>>>> +#define fw_umh_is_done(fw_st)			true
>>>>> +#define fw_umh_is_aborted(fw_st)		false
>>>>
>>>> We still need the implementation for fw_umh_wait_timeout() and
>>>> fw_umh_start(), fw_umh_done() etc.
>>>
>>> Why?
>>
>> See above.
>
> Sure, but note how the timeout is not used.

See above.

>>>>> @@ -309,8 +373,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_umh_done(&buf->fw_umh);
>>>>> 	mutex_unlock(&fw_lock);
>>>>> }
>>>>
>>>> Here we signal that we have loaded the firmware
>>>
>>> The struct firmware_buf is only used for the sysfs stuff no?
>>
>> I don't know, I was looking at the code in firmware_class.c not any users.
>> Why is that important?
>
> Sorry I meant struct firmware_priv is used by sysfs stuff only, the sysfs stuff
> is only used for the FW UMH.

Yes, even 'struct firmware_priv' is only available when 
CONFIG_FW_LOADER_USER_HELPER. Though fw_finish_direct_load() is used in 
the !UHM section. I think I still miss your point here.


>>>>> /* 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_umh_is_done(&buf->fw_umh)) {
>>>>> +		if (fw_umh_is_aborted(&buf->fw_umh)) {
>>>>> 			ret = -ENOENT;
>>>>> 			break;
>>>>> 		}
>>>>> 		mutex_unlock(&fw_lock);
>>>>> -		ret = wait_for_completion_interruptible(&buf->completion);
>>>>> +		ret = fw_umh_wait_timeout(&buf->fw_umh, 0);
>>>>> 		mutex_lock(&fw_lock);
>>>>> 	}
>>>>
>>>> and here we here we wait for it.
>>>
>>> Likewise.
>>
>> As I tried to explain above the buffering code is depending on completion.
>
> OK sure agreed. The timeout, no though, unless I missed something?

I don't think so. The only thing is the value of the fw_uhm_wait_timeout().

cheers,
daniel

Re: [PATCH v5 2/5] firmware: encapsulate firmware loading status

[Luis R. Rodriguez]

On Tue, Oct 18, 2016 at 03:30:45PM +0200, Daniel Wagner wrote:
> On 10/10/2016 10:37 PM, Luis R. Rodriguez wrote:
> 
> > >   fw_get_fileystem_firmware()
> > >     fw_finish_direct_load()
> > >       complete_all()
> > > 
> > > 
> > > 2nd request (waiter context)
> > > 
> > > _request_firmware()
> > >   _request_firmware_prepare()
> > >      fw_lookup_allocate_buf()      # finds previously allocated buf
> > >                                    # returns 1 -> wait for loading
> > >      sync_cached_firmware_buf()
> > >         wait_for_completion_interruptible_timeout()
> > 
> > No, that's wait_for_completion_interruptible() not
> >            wait_for_completion_interruptible_timeout()
> 
> I confused that one from _request_firmware_load().

Right and wait_for_completion_interruptible() has no timeout.

> > Also note that we only call sync_cached_firmware_buf()
> > *iff* fw_lookup_and_allocate_buf() returned the 1 -- I mentioned
> > when this happens above. That happens only if we already had the entry on
> > the fw cache. As it stands -- concurrent calls against the same fw name
> > could cause a clash here, as such, the wait_for_completion_interruptible()
> > is indeed still needed.
> > 
> > Further optimizations can be considered later but for indeed, agreed
> > that completion is needed even for the direct fw load case. The timeout
> > though, I don't see a reason for it.
> 
> So I think I found the source of the confusion about fw_umh_wait_timeout().
> When providing a timeout value of 0 you get the
> wait_for_completion_interruptible() version.

I fail to see that, how so? Note that 0 does is not allowed anyway:

static inline long firmware_loading_timeout(void)
{
        return loading_timeout > 0 ? loading_timeout * HZ : MAX_JIFFY_OFFSET;
}

  Luis

Re: [PATCH v5 2/5] firmware: encapsulate firmware loading status

[Daniel Wagner]

On 10/18/2016 11:54 PM, Luis R. Rodriguez wrote:
> On Tue, Oct 18, 2016 at 03:30:45PM +0200, Daniel Wagner wrote:
>> On 10/10/2016 10:37 PM, Luis R. Rodriguez wrote:
>>
>>>>   fw_get_fileystem_firmware()
>>>>     fw_finish_direct_load()
>>>>       complete_all()
>>>>
>>>>
>>>> 2nd request (waiter context)
>>>>
>>>> _request_firmware()
>>>>   _request_firmware_prepare()
>>>>      fw_lookup_allocate_buf()      # finds previously allocated buf
>>>>                                    # returns 1 -> wait for loading
>>>>      sync_cached_firmware_buf()
>>>>         wait_for_completion_interruptible_timeout()
>>>
>>> No, that's wait_for_completion_interruptible() not
>>>            wait_for_completion_interruptible_timeout()
>>
>> I confused that one from _request_firmware_load().
> 
> Right and wait_for_completion_interruptible() has no timeout.

All wait_for_completion_*() function are small wrappers around
a common implemention. I thought that would be a clever idea to
reuse here, but from our discussion I see it isn't. My bad.

static int fw_umh_wait_timeout(struct fw_umh *fw_umh, long timeout)
{
	int ret;

	ret = wait_for_completion_interruptible_timeout(&fw_umh->completion,
							timeout);
	if (ret != 0 && test_bit(FW_UMH_ABORTED, &fw_umh->status))
		return -ENOENT;

	return ret;
}


long __sched
wait_for_completion_interruptible_timeout(struct completion *x,
					  unsigned long timeout)
{
	return wait_for_common(x, timeout, TASK_INTERRUPTIBLE);
}

int __sched wait_for_completion_interruptible(struct completion *x)
{
	long t = wait_for_common(x, MAX_SCHEDULE_TIMEOUT, TASK_INTERRUPTIBLE);
	if (t == -ERESTARTSYS)
		return t;
	return 0;
}


I think it is far better to do something like:

static __fw_umh_wait_common(struct fw_umh *fw_umh, long timeout) { ... }

#define fw_umh_wait(fw_umh) __fw_umh_wait_common(fw_umh, MAX_SCHEDULE_TIMEOUT)
#define fw_umh_wait_timeout(fw_umh, timeout) __fw_umh_wait_common(fw_umh, timeout)

(The function prefixes will be different, since umh isn't right as discussed.)

>>> Also note that we only call sync_cached_firmware_buf()
>>> *iff* fw_lookup_and_allocate_buf() returned the 1 -- I mentioned
>>> when this happens above. That happens only if we already had the entry on
>>> the fw cache. As it stands -- concurrent calls against the same fw name
>>> could cause a clash here, as such, the wait_for_completion_interruptible()
>>> is indeed still needed.
>>>
>>> Further optimizations can be considered later but for indeed, agreed
>>> that completion is needed even for the direct fw load case. The timeout
>>> though, I don't see a reason for it.
>>
>> So I think I found the source of the confusion about fw_umh_wait_timeout().
>> When providing a timeout value of 0 you get the
>> wait_for_completion_interruptible() version.
> 
> I fail to see that, how so? Note that 0 does is not allowed anyway:
> 
> static inline long firmware_loading_timeout(void)
> {
>         return loading_timeout > 0 ? loading_timeout * HZ : MAX_JIFFY_OFFSET;
> }

Correct. The fw_umh_wait_timeout(0) is hard coded in sync_cached_firmware_buf().
fw_umh_wait_timeout(fw_umh, firmware_loading_timeout()) is used
_request_firmware_load().

I'll update the series and hopefully we get this all sorted out in the new
version. 

cheers,
daniel