[PATCH v7 0/4] firmware: refactor loading status

[PATCH v7 0/4] firmware: refactor loading status

[Daniel Wagner]

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

Hi,

after the lengthy discussion online and in person I decided to remove
all things which might change behavoir. So this is refactore clean up
version. I think it worth to consider it because this version gets rid
of the fw_lock dependency for protecting the loading status.

It is based on Yves-Alexis' patch "firmware: fix usermode helper
fallback loading". According this patch I fixed the type cast
error in __fw_state_wait_common(). This function returns a long
now.

The last patch could be merged into second last. I decided to
split it to make the review easier.

cheers,
daniel

changes since v6:
  - removed all controversial patches which change behavoir
  - fixed casting error in return type in __fw_state_wait_common()
  - rebased in on v4.9-rc5 + Yves-Alexis' patch.

changes since v5:
  - add enum name (enum fw_status)
  - renamed function prefix and structure fw_umh to fw_state (avoid to
    clash with enum fw_status)
  - add back patch 1 from v3
  - move unused functions for direct firmeware loading into UMH section

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



Daniel Wagner (4):
  firmware: refactor loading status
  firmware: drop bit ops in favor of simple state machine
  firmware: do not use fw_lock for fw_state protection
  firmware: move fw_state_is_done() into UHM section

 drivers/base/firmware_class.c | 156 ++++++++++++++++++++++++++----------------
 1 file changed, 96 insertions(+), 60 deletions(-)

-- 
2.7.4

[PATCH v7 1/4] firmware: refactor loading status

[Daniel Wagner]

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

The firmware loader tracks the current state of the loading process
via unsigned long status and a completion in struct
firmware_buf. Instead of open code tracking the state, introduce data
structure which encapsulate the state tracking and synchronization.

While at it also separate UHM states from direct loading states, e.g.
the loading_timeout is only defined when CONFIG_FW_LOADER_USER_HELPER.

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 | 127 +++++++++++++++++++++++++++++++-----------
 1 file changed, 93 insertions(+), 34 deletions(-)

diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
index a95e1e572697..d3b614d10d60 100644
--- a/drivers/base/firmware_class.c
+++ b/drivers/base/firmware_class.c
@@ -91,10 +91,11 @@ static inline bool fw_is_builtin_firmware(const struct firmware *fw)
 }
 #endif
 
-enum {
+enum fw_status {
+	FW_STATUS_UNKNOWN,
 	FW_STATUS_LOADING,
 	FW_STATUS_DONE,
-	FW_STATUS_ABORT,
+	FW_STATUS_ABORTED,
 };
 
 static int loading_timeout = 60;	/* In seconds */
@@ -104,6 +105,76 @@ static inline long firmware_loading_timeout(void)
 	return loading_timeout > 0 ? loading_timeout * HZ : MAX_JIFFY_OFFSET;
 }
 
+/*
+ * Concurrent request_firmware() for the same firmware need to be
+ * serialized.  struct fw_state is simple state machine which hold the
+ * state of the firmware loading.
+ */
+struct fw_state {
+	struct completion completion;
+	unsigned long status;
+};
+
+static void fw_state_init(struct fw_state *fw_st)
+{
+	init_completion(&fw_st->completion);
+	fw_st->status = FW_STATUS_UNKNOWN;
+}
+
+static int __fw_state_check(struct fw_state *fw_st, enum fw_status status)
+{
+	return test_bit(status, &fw_st->status);
+}
+
+static long __fw_state_wait_common(struct fw_state *fw_st, long timeout)
+{
+	long 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_state_set(struct fw_state *fw_st,
+			   enum fw_status 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_state_start(fw_st)					\
+	__fw_state_set(fw_st, FW_STATUS_LOADING)
+#define fw_state_done(fw_st)					\
+	__fw_state_set(fw_st, FW_STATUS_DONE)
+#define fw_state_is_done(fw_st)					\
+	__fw_state_check(fw_st, FW_STATUS_DONE)
+#define fw_state_wait(fw_st)					\
+	__fw_state_wait_common(fw_st, MAX_SCHEDULE_TIMEOUT)
+
+#ifndef CONFIG_FW_LOADER_USER_HELPER
+
+#define fw_state_is_aborted(fw_st)	false
+
+#else /* CONFIG_FW_LOADER_USER_HELPER */
+
+#define fw_state_aborted(fw_st)					\
+	__fw_state_set(fw_st, FW_STATUS_ABORTED)
+#define fw_state_is_loading(fw_st)				\
+	__fw_state_check(fw_st, FW_STATUS_LOADING)
+#define fw_state_is_aborted(fw_st)				\
+	__fw_state_check(fw_st, FW_STATUS_ABORTED)
+#define fw_state_wait_timeout(fw_st, timeout)			\
+	__fw_state_wait_common(fw_st, timeout)
+
+#endif /* CONFIG_FW_LOADER_USER_HELPER */
+
 /* firmware behavior options */
 #define FW_OPT_UEVENT	(1U << 0)
 #define FW_OPT_NOWAIT	(1U << 1)
@@ -145,9 +216,8 @@ struct firmware_cache {
 struct firmware_buf {
 	struct kref ref;
 	struct list_head list;
-	struct completion completion;
 	struct firmware_cache *fwc;
-	unsigned long status;
+	struct fw_state fw_st;
 	void *data;
 	size_t size;
 	size_t allocated_size;
@@ -205,7 +275,7 @@ 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);
+	fw_state_init(&buf->fw_st);
 #ifdef CONFIG_FW_LOADER_USER_HELPER
 	INIT_LIST_HEAD(&buf->pending_list);
 #endif
@@ -309,8 +379,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_state_done(&buf->fw_st);
 	mutex_unlock(&fw_lock);
 }
 
@@ -478,12 +547,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_state_is_done(&buf->fw_st))
 		return;
 
 	list_del_init(&buf->pending_list);
-	set_bit(FW_STATUS_ABORT, &buf->status);
-	complete_all(&buf->completion);
+	fw_state_aborted(&buf->fw_st);
 }
 
 static void fw_load_abort(struct firmware_priv *fw_priv)
@@ -496,9 +564,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 +663,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_state_is_loading(&fw_priv->buf->fw_st);
 	mutex_unlock(&fw_lock);
 
 	return sprintf(buf, "%d\n", loading);
@@ -653,23 +718,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_state_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_state_start(&fw_buf->fw_st);
 		}
 		break;
 	case 0:
-		if (test_bit(FW_STATUS_LOADING, &fw_buf->status)) {
+		if (fw_state_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 +753,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_state_aborted(&fw_buf->fw_st);
 				written = rc;
+			} else {
+				fw_state_done(&fw_buf->fw_st);
 			}
-			complete_all(&fw_buf->completion);
 			break;
 		}
 		/* fallthrough */
@@ -755,7 +818,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_state_is_done(&buf->fw_st)) {
 		ret_count = -ENODEV;
 		goto out;
 	}
@@ -842,7 +905,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_state_is_done(&buf->fw_st)) {
 		retval = -ENODEV;
 		goto out;
 	}
@@ -955,8 +1018,7 @@ static int _request_firmware_load(struct firmware_priv *fw_priv,
 		timeout = MAX_JIFFY_OFFSET;
 	}
 
-	timeout = wait_for_completion_interruptible_timeout(&buf->completion,
-			timeout);
+	timeout = fw_state_wait_timeout(&buf->fw_st, timeout);
 	if (timeout == -ERESTARTSYS || !timeout) {
 		retval = timeout;
 		mutex_lock(&fw_lock);
@@ -966,7 +1028,7 @@ static int _request_firmware_load(struct firmware_priv *fw_priv,
 		retval = 0;
 	}
 
-	if (is_fw_load_aborted(buf))
+	if (fw_state_is_aborted(&buf->fw_st))
 		retval = -EAGAIN;
 	else if (buf->is_paged_buf && !buf->data)
 		retval = -ENOMEM;
@@ -1016,9 +1078,6 @@ 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
@@ -1032,13 +1091,13 @@ 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_state_is_done(&buf->fw_st)) {
+		if (fw_state_is_aborted(&buf->fw_st)) {
 			ret = -ENOENT;
 			break;
 		}
 		mutex_unlock(&fw_lock);
-		ret = wait_for_completion_interruptible(&buf->completion);
+		ret = fw_state_wait(&buf->fw_st);
 		mutex_lock(&fw_lock);
 	}
 	mutex_unlock(&fw_lock);
@@ -1096,7 +1155,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_state_is_aborted(&buf->fw_st)) {
 		mutex_unlock(&fw_lock);
 		return -ENOENT;
 	}
-- 
2.7.4

[PATCH v7 2/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 firmware loading with bit ops.  Since the
state machine has only a few states and they are all mutual exclusive
there are only a few simple state transition we can model this simplify.

	   UNKNOWN -> LOADING -> DONE | ABORTED

Because we don't use any bit ops on fw_state::status anymore we are able
to change the data type to enum fw_status and update the function
arguments accordingly.

READ_ONCE() and WRITE_ONCE() are propably not needed because there are a
lot of load and stores around fw_st->status. But let's make it explicit
and not be sorry later.

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 d3b614d10d60..1fda674f5219 100644
--- a/drivers/base/firmware_class.c
+++ b/drivers/base/firmware_class.c
@@ -112,7 +112,7 @@ static inline long firmware_loading_timeout(void)
  */
 struct fw_state {
 	struct completion completion;
-	unsigned long status;
+	enum fw_status status;
 };
 
 static void fw_state_init(struct fw_state *fw_st)
@@ -123,7 +123,7 @@ static void fw_state_init(struct fw_state *fw_st)
 
 static int __fw_state_check(struct fw_state *fw_st, enum fw_status status)
 {
-	return test_bit(status, &fw_st->status);
+	return fw_st->status == status;
 }
 
 static long __fw_state_wait_common(struct fw_state *fw_st, long timeout)
@@ -132,7 +132,7 @@ static long __fw_state_wait_common(struct fw_state *fw_st, long timeout)
 
 	ret = wait_for_completion_interruptible_timeout(&fw_st->completion,
 							timeout);
-	if (ret != 0 && test_bit(FW_STATUS_ABORTED, &fw_st->status))
+	if (ret != 0 && READ_ONCE(fw_st->status) == FW_STATUS_ABORTED)
 		return -ENOENT;
 
 	return ret;
@@ -141,12 +141,10 @@ static long __fw_state_wait_common(struct fw_state *fw_st, long timeout)
 static void __fw_state_set(struct fw_state *fw_st,
 			   enum fw_status 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);
+	if (status == FW_STATUS_DONE || status == FW_STATUS_ABORTED)
 		complete_all(&fw_st->completion);
-	}
 }
 
 #define fw_state_start(fw_st)					\
-- 
2.7.4

[PATCH v7 3/4] firmware: do not use fw_lock for fw_state 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_state can be used without needing the fw_lock to protect
its state transition and wake ups.

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

Note there some more cleanups possible after with this change. For
example for !CONFIG_FW_LOADER_USER_HELPER we don't check for the state
anymore.  Let's to this in the next patch instead mingling to many
changes into this one. And yes you get a gcc warning "‘__fw_state_check’
defined but not used [-Wunused-function] code." for the time beeing.

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 | 51 +++++++++++++------------------------------
 1 file changed, 15 insertions(+), 36 deletions(-)

diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
index 1fda674f5219..0208a37cd2ff 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>
 
@@ -111,13 +112,13 @@ static inline long firmware_loading_timeout(void)
  * state of the firmware loading.
  */
 struct fw_state {
-	struct completion completion;
+	struct swait_queue_head wq;
 	enum fw_status status;
 };
 
 static void fw_state_init(struct fw_state *fw_st)
 {
-	init_completion(&fw_st->completion);
+	init_swait_queue_head(&fw_st->wq);
 	fw_st->status = FW_STATUS_UNKNOWN;
 }
 
@@ -126,13 +127,19 @@ static int __fw_state_check(struct fw_state *fw_st, enum fw_status status)
 	return fw_st->status == status;
 }
 
+static inline bool __fw_state_is_done(enum fw_status status)
+{
+	return status == FW_STATUS_DONE || status == FW_STATUS_ABORTED;
+}
+
 static long __fw_state_wait_common(struct fw_state *fw_st, long timeout)
 {
 	long ret;
 
-	ret = wait_for_completion_interruptible_timeout(&fw_st->completion,
-							timeout);
-	if (ret != 0 && READ_ONCE(fw_st->status) == FW_STATUS_ABORTED)
+	ret = swait_event_interruptible_timeout(fw_st->wq,
+				__fw_state_is_done(READ_ONCE(fw_st->status)),
+				timeout);
+	if (ret != 0 && fw_st->status == FW_STATUS_ABORTED)
 		return -ENOENT;
 
 	return ret;
@@ -144,7 +151,7 @@ static void __fw_state_set(struct fw_state *fw_st,
 	WRITE_ONCE(fw_st->status, status);
 
 	if (status == FW_STATUS_DONE || status == FW_STATUS_ABORTED)
-		complete_all(&fw_st->completion);
+		swake_up(&fw_st->wq);
 }
 
 #define fw_state_start(fw_st)					\
@@ -373,14 +380,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_state_done(&buf->fw_st);
-	mutex_unlock(&fw_lock);
-}
-
 static int
 fw_get_filesystem_firmware(struct device *device, struct firmware_buf *buf)
 {
@@ -427,7 +426,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_state_done(&buf->fw_st);
 		break;
 	}
 	__putname(path);
@@ -1082,26 +1081,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_state_is_done(&buf->fw_st)) {
-		if (fw_state_is_aborted(&buf->fw_st)) {
-			ret = -ENOENT;
-			break;
-		}
-		mutex_unlock(&fw_lock);
-		ret = fw_state_wait(&buf->fw_st);
-		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
@@ -1135,7 +1114,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_state_wait(&buf->fw_st);
 		if (!ret) {
 			fw_set_page_data(buf, firmware);
 			return 0; /* assigned */
-- 
2.7.4

[PATCH v7 4/4] firmware: move fw_state_is_done() into UHM section

[Daniel Wagner]

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

fw_state_is_done() is only used for UHM so moved into that section.

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 | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
index 0208a37cd2ff..fe1cc42512a9 100644
--- a/drivers/base/firmware_class.c
+++ b/drivers/base/firmware_class.c
@@ -122,11 +122,6 @@ static void fw_state_init(struct fw_state *fw_st)
 	fw_st->status = FW_STATUS_UNKNOWN;
 }
 
-static int __fw_state_check(struct fw_state *fw_st, enum fw_status status)
-{
-	return fw_st->status == status;
-}
-
 static inline bool __fw_state_is_done(enum fw_status status)
 {
 	return status == FW_STATUS_DONE || status == FW_STATUS_ABORTED;
@@ -158,8 +153,6 @@ static void __fw_state_set(struct fw_state *fw_st,
 	__fw_state_set(fw_st, FW_STATUS_LOADING)
 #define fw_state_done(fw_st)					\
 	__fw_state_set(fw_st, FW_STATUS_DONE)
-#define fw_state_is_done(fw_st)					\
-	__fw_state_check(fw_st, FW_STATUS_DONE)
 #define fw_state_wait(fw_st)					\
 	__fw_state_wait_common(fw_st, MAX_SCHEDULE_TIMEOUT)
 
@@ -169,8 +162,15 @@ static void __fw_state_set(struct fw_state *fw_st,
 
 #else /* CONFIG_FW_LOADER_USER_HELPER */
 
+static int __fw_state_check(struct fw_state *fw_st, enum fw_status status)
+{
+	return fw_st->status == status;
+}
+
 #define fw_state_aborted(fw_st)					\
 	__fw_state_set(fw_st, FW_STATUS_ABORTED)
+#define fw_state_is_done(fw_st)					\
+	__fw_state_check(fw_st, FW_STATUS_DONE)
 #define fw_state_is_loading(fw_st)				\
 	__fw_state_check(fw_st, FW_STATUS_LOADING)
 #define fw_state_is_aborted(fw_st)				\
-- 
2.7.4

Re: [PATCH v7 0/4] firmware: refactor loading status

[Luis R. Rodriguez]

On Thu, Nov 17, 2016 at 11:00:47AM +0100, Daniel Wagner wrote:
> From: Daniel Wagner <daniel.wagner@bmw-carit.de>
> 
> Hi,
> 
> after the lengthy discussion online and in person I decided to remove
> all things which might change behavoir. So this is refactore clean up
> version. I think it worth to consider it because this version gets rid
> of the fw_lock dependency for protecting the loading status.
> 
> It is based on Yves-Alexis' patch "firmware: fix usermode helper
> fallback loading". According this patch I fixed the type cast
> error in __fw_state_wait_common(). This function returns a long
> now.
> 
> The last patch could be merged into second last. I decided to
> split it to make the review easier.

Thanks for all this work and your patience during review!

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

  Luis