[PATCH v2 0/2] firmware_class: encapsulate firmware loading status

[PATCH v2 0/2] firmware_class: encapsulate firmware loading status

[Daniel Wagner]

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

Hi,

As requested an splited up version.

cheers,
daniel

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 (2):
  firmware_class: encapsulate firmware loading status
  firmware_class: Use swait instead of completion

 drivers/base/firmware_class.c | 163 +++++++++++++++++++++++++-----------------
 1 file changed, 98 insertions(+), 65 deletions(-)

-- 
2.7.4

[PATCH v2 1/2] firmware_class: 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 an member of struct firmware_buf and a completion. Let's
encapsulate this simple state machine into struct fw_status.

We don't need the fw_lock to protect the state machine anymore. First of
all we don't do any RWM operations instead we do either a write or read
to the status variable. Reading the status variable via READ_ONCE
ensures we see the new state whenever we get woken. We are not allowed
to use the complete_all() more than once so we need to filter out all
state transition but the last. That is okay since only the final state
DONE|ABORTED is of interest.

Besides that we also safe a few bytes:

cris etrax-100lx_defconfig

$ bloat-o-meter firmware_class.o.old firmware_class.o.new
add/remove: 0/0 grow/shrink: 0/1 up/down: 0/-234 (-234)
function                                     old     new   delta
_request_firmware                           1228     994    -234
Total: Before=2179, After=1945, chg -10.74%

x86_64 FW_LOADER !FW_LOADER_USER_HELPER_FALLBACK

$ bloat-o-meter firmware_class.o.old firmware_class.o.new
add/remove: 0/0 grow/shrink: 0/1 up/down: 0/-194 (-194)
function                                     old     new   delta
_request_firmware                           2118    1924    -194
Total: Before=6811, After=6617, chg -2.85%

x86_64 FW_LOADER FW_LOADER_USER_HELPER_FALLBACK

$ bloat-o-meter firmware_class.o.old firmware_class.o.new
add/remove: 0/0 grow/shrink: 2/5 up/down: 4/-193 (-189)
function                                     old     new   delta
fw_shutdown_notify                            81      83      +2
firmware_data_read                           162     164      +2
fw_pm_notify                                 396     394      -2
firmware_loading_store                       458     441     -17
firmware_data_write                          504     475     -29
firmware_loading_show                         92      61     -31
_request_firmware                           2931    2817    -114
Total: Before=10328, After=10139, chg -1.83%

Note the variable loading_timeout and the inline function
firmware_loadint_timeout() are used also when no user helper is enabled.
So they need to be moved out side of the ifdef section.

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

diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
index 22d1760..d3dcf87 100644
--- a/drivers/base/firmware_class.c
+++ b/drivers/base/firmware_class.c
@@ -91,19 +91,88 @@ static inline bool fw_is_builtin_firmware(const struct firmware *fw)
 }
 #endif
 
+static int loading_timeout = 60;	/* In seconds */
+
+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_STATUS_UNKNOWN,
 	FW_STATUS_LOADING,
 	FW_STATUS_DONE,
-	FW_STATUS_ABORT,
+	FW_STATUS_ABORTED,
 };
 
-static int loading_timeout = 60;	/* In seconds */
+struct fw_status {
+	unsigned long status;
+	struct completion completion;
+};
 
-static inline long firmware_loading_timeout(void)
+static void fw_status_init(struct fw_status *fw_st)
 {
-	return loading_timeout > 0 ? loading_timeout * HZ : MAX_JIFFY_OFFSET;
+	fw_st->status = FW_STATUS_UNKNOWN;
+	init_completion(&fw_st->completion);
+}
+
+static unsigned long __fw_status_get(struct fw_status *fw_st)
+{
+	return READ_ONCE(fw_st->status);
+}
+
+static int fw_status_wait_timeout(struct fw_status *fw_st, long timeout)
+{
+	unsigned long status;
+	int err;
+
+	err = wait_for_completion_interruptible_timeout(&fw_st->completion,
+							timeout);
+	status = READ_ONCE(fw_st->status);
+	if (err == 0 && status == FW_STATUS_ABORTED)
+		return -ENOENT;
+
+	return err;
+}
+
+static void __fw_status_set(struct fw_status *fw_st,
+			  unsigned long status)
+{
+	WRITE_ONCE(fw_st->status, status);
+
+	if (status == FW_STATUS_DONE ||
+			status == FW_STATUS_ABORTED)
+		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_get(fw_st) == FW_STATUS_LOADING)
+#define fw_status_is_done(fw_st)				\
+	(__fw_status_get(fw_st) == FW_STATUS_DONE)
+#define fw_status_is_aborted(fw_st)				\
+	(__fw_status_get(fw_st) == FW_STATUS_ABORTED)
+
+#else /* CONFIG_FW_LOADER_USER_HELPER */
+
+#define fw_status_wait_timeout(fw_st, long)	0
+
+#define fw_status_start(fw_st)
+#define fw_status_done(fw_st)
+#define fw_status_aborted(fw_st)
+#define fw_status_is_loading(fw_st)
+#define fw_status_is_done(fw_st)
+#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 +214,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 +273,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
 
@@ -305,15 +373,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);
-	set_bit(FW_STATUS_DONE, &buf->status);
-	complete_all(&buf->completion);
-	mutex_unlock(&fw_lock);
-}
-
 static int
 fw_get_filesystem_firmware(struct device *device, struct firmware_buf *buf)
 {
@@ -360,7 +419,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);
@@ -478,12 +537,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 +554,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 */
@@ -596,10 +651,8 @@ static ssize_t firmware_loading_show(struct device *dev,
 	struct firmware_priv *fw_priv = to_firmware_priv(dev);
 	int loading = 0;
 
-	mutex_lock(&fw_lock);
 	if (fw_priv->buf)
-		loading = test_bit(FW_STATUS_LOADING, &fw_priv->buf->status);
-	mutex_unlock(&fw_lock);
+		loading = fw_status_is_loading(&fw_priv->buf->fw_st);
 
 	return sprintf(buf, "%d\n", loading);
 }
@@ -653,23 +706,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 +741,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 +753,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 +806,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 +893,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 +1006,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 +1015,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;
@@ -1015,35 +1065,12 @@ 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)) {
-			ret = -ENOENT;
-			break;
-		}
-		mutex_unlock(&fw_lock);
-		ret = wait_for_completion_interruptible(&buf->completion);
-		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
@@ -1077,7 +1104,8 @@ _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,
+					     firmware_loading_timeout());
 		if (!ret) {
 			fw_set_page_data(buf, firmware);
 			return 0; /* assigned */
@@ -1095,7 +1123,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 v2 2/2] firmware_class: Use swait instead of completion

[Daniel Wagner]

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

complete_all() can only be issued once before it needs to be
reinitialized. To ensure we never call complete_all() twice we use
swait and make the code here a bit more robust.

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

diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
index d3dcf87..029b829 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 @@ enum {
 
 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 unsigned long __fw_status_get(struct fw_status *fw_st)
@@ -123,15 +124,19 @@ static unsigned long __fw_status_get(struct fw_status *fw_st)
 	return READ_ONCE(fw_st->status);
 }
 
+static inline bool is_fw_status_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 err;
-
-	err = wait_for_completion_interruptible_timeout(&fw_st->completion,
-							timeout);
-	status = READ_ONCE(fw_st->status);
-	if (err == 0 && status == FW_STATUS_ABORTED)
+	err = swait_event_interruptible_timeout(fw_st->wq,
+				is_fw_status_done(READ_ONCE(fw_st->status)),
+				timeout);
+	if (err == 0 && fw_st->status == FW_STATUS_ABORTED)
 		return -ENOENT;
 
 	return err;
@@ -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)					\
-- 
2.7.4

Re: [PATCH v2 1/2] firmware_class: encapsulate firmware loading status

[Luis R. Rodriguez]

On Tue, Aug 23, 2016 at 11:00:19AM +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 an member of struct firmware_buf and a completion.

You can specify the members, the unsigned long status and the completion.

> Let's
> encapsulate this simple state machine into struct fw_status.

Please rephrase instead to:

"We only need this for the usermode helper as such we can encapsulate
all this data into its own data structure."

> We don't need the fw_lock to protect the state machine anymore.

You can the instead follow up explaining, "Doing this helps us compartamentalize
the usermode helper code further."

Can you cut that out into a separate patch?

Then another patch can be: "The usermode helper code only needs access to a
status variable, no RWM operations are done on it, so we can replace the
use of the global lock with READ_ONCE() and WRITE_ONCE()."

> First of
> all we don't do any RWM operations instead we do either a write or read
> to the status variable. Reading the status variable via READ_ONCE
> ensures we see the new state whenever we get woken. We are not allowed
> to use the complete_all() more than once so we need to filter out all
> state transition but the last. That is okay since only the final state
> DONE|ABORTED is of interest.
> 
> Besides that we also safe a few bytes:
> 
> cris etrax-100lx_defconfig
> 
> $ bloat-o-meter firmware_class.o.old firmware_class.o.new
> add/remove: 0/0 grow/shrink: 0/1 up/down: 0/-234 (-234)
> function                                     old     new   delta
> _request_firmware                           1228     994    -234
> Total: Before=2179, After=1945, chg -10.74%
> 
> x86_64 FW_LOADER !FW_LOADER_USER_HELPER_FALLBACK
> 
> $ bloat-o-meter firmware_class.o.old firmware_class.o.new
> add/remove: 0/0 grow/shrink: 0/1 up/down: 0/-194 (-194)
> function                                     old     new   delta
> _request_firmware                           2118    1924    -194
> Total: Before=6811, After=6617, chg -2.85%
> 
> x86_64 FW_LOADER FW_LOADER_USER_HELPER_FALLBACK
> 
> $ bloat-o-meter firmware_class.o.old firmware_class.o.new
> add/remove: 0/0 grow/shrink: 2/5 up/down: 4/-193 (-189)
> function                                     old     new   delta
> fw_shutdown_notify                            81      83      +2
> firmware_data_read                           162     164      +2
> fw_pm_notify                                 396     394      -2
> firmware_loading_store                       458     441     -17
> firmware_data_write                          504     475     -29
> firmware_loading_show                         92      61     -31
> _request_firmware                           2931    2817    -114
> Total: Before=10328, After=10139, chg -1.83%
> 
> Note the variable loading_timeout and the inline function
> firmware_loadint_timeout() are used also when no user helper is enabled.
> So they need to be moved out side of the ifdef section.
> 
> 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 | 158 +++++++++++++++++++++++++-----------------
>  1 file changed, 93 insertions(+), 65 deletions(-)
> 
> diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
> index 22d1760..d3dcf87 100644
> --- a/drivers/base/firmware_class.c
> +++ b/drivers/base/firmware_class.c
> @@ -91,19 +91,88 @@ static inline bool fw_is_builtin_firmware(const struct firmware *fw)
>  }
>  #endif
>  
> +static int loading_timeout = 60;	/* In seconds */
> +
> +static inline long firmware_loading_timeout(void)
> +{
> +	return loading_timeout > 0 ? loading_timeout * HZ : MAX_JIFFY_OFFSET;
> +}

All this code is being kept when CONFIG_FW_LOADER_USER_HELPER is not enabled, that
seems odd given firmware_loading_timeout() is used when CONFIG_FW_LOADER_USER_HELPER
is disabled in two places, in one case its not needed at all, in the second its in this
piece of code:

        retval = fw_status_wait_timeout(&buf->fw_st, timeout);                  
        if (retval == -ERESTARTSYS || !retval) {      
		...
	 }
I'd rather instead have a wrapper for a user mode helper timeout check and
have that return 0 for when CONFIG_FW_LOADER_USER_HELPER is not selected.
This way the global variable loading_timeout and anything that accesses it
is just left inside CONFIG_FW_LOADER_USER_HELPER.

> +
> +#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 */
> +struct fw_status {
> +	unsigned long status;
> +	struct completion completion;
> +};
>  
> -static inline long firmware_loading_timeout(void)
> +static void fw_status_init(struct fw_status *fw_st)
>  {
> -	return loading_timeout > 0 ? loading_timeout * HZ : MAX_JIFFY_OFFSET;
> +	fw_st->status = FW_STATUS_UNKNOWN;
> +	init_completion(&fw_st->completion);
> +}
> +
> +static unsigned long __fw_status_get(struct fw_status *fw_st)
> +{
> +	return READ_ONCE(fw_st->status);
> +}
> +
> +static int fw_status_wait_timeout(struct fw_status *fw_st, long timeout)
> +{
> +	unsigned long status;
> +	int err;
> +
> +	err = wait_for_completion_interruptible_timeout(&fw_st->completion,
> +							timeout);
> +	status = READ_ONCE(fw_st->status);
> +	if (err == 0 && status == FW_STATUS_ABORTED)
> +		return -ENOENT;
> +
> +	return err;
> +}
> +
> +static void __fw_status_set(struct fw_status *fw_st,
> +			  unsigned long status)
> +{
> +	WRITE_ONCE(fw_st->status, status);
> +
> +	if (status == FW_STATUS_DONE ||
> +			status == FW_STATUS_ABORTED)
> +		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_get(fw_st) == FW_STATUS_LOADING)
> +#define fw_status_is_done(fw_st)				\
> +	(__fw_status_get(fw_st) == FW_STATUS_DONE)
> +#define fw_status_is_aborted(fw_st)				\
> +	(__fw_status_get(fw_st) == FW_STATUS_ABORTED)
> +
> +#else /* CONFIG_FW_LOADER_USER_HELPER */
> +
> +#define fw_status_wait_timeout(fw_st, long)	0
> +
> +#define fw_status_start(fw_st)

You don't need this, in fact if you had it as-is and the code for when
CONFIG_FW_LOADER_USER_HELPER is disabled would use it you may end up in
a compile error.

> +#define fw_status_done(fw_st)

You need this for both.

> +#define fw_status_aborted(fw_st)
> +#define fw_status_is_loading(fw_st)
> +#define fw_status_is_done(fw_st)

All these are no longer needed for when CONFIG_FW_LOADER_USER_HELPER is
disabled.

> +#define fw_status_is_aborted(fw_st)		false

This is needed for both.

Given all this it seems to be best to rebrand these helpers as usermode helper
related. In fact the next step is to see if this looks best to just stuffing all
this user mode helper code into its own C file. But we can do that later.

Other than that looks good!

  Luis

Re: [PATCH v2 2/2] firmware_class: Use swait instead of completion

[Luis R. Rodriguez]

On Tue, Aug 23, 2016 at 11:00:20AM +0200, Daniel Wagner wrote:
> From: Daniel Wagner <daniel.wagner@bmw-carit.de>
> 
> complete_all() can only be issued once before it needs to be
> reinitialized. 

What are the chances this ever happened? If it could not happen its
worth explaining why.

> To ensure we never call complete_all() twice we use
> swait and make the code here a bit more robust.

The real benefit here though is not that is it? Isn't the benefit that
we don't need all the API functionality provided by wait, we just need
something light weight.

Thanks for splitting this up, it makes it for a much easier review.

  Luis