[RFC v0 0/8] Reuse firmware loader helpers

[RFC v0 0/8] Reuse firmware loader helpers

[Daniel Wagner]

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

Hi,

While reviewing all the complete_all() users, I realized there is
recouring pattern how the completion API is used to synchronize the
stages of the firmware loading. Since firmware_class.c contains a
fairly complete implemetation for synching the loading, it worthwhile
to export it and reuse it in drivers, At the same time one
complete_all() user is gone which is a good thing for -rt (*).

The first 2 patches are bug fixes for the test script.

Patch 3 adds the new API, and the following patches update a few
drivers.

I haven't updated all the drivers because I wanted to see first if
this is going into the right direction. Since naming is a difficult, I
am more than please to pick a better name if you have one.

cheers,
daniel

(*) Under -rt waking all waiters via complete_all() is not good. If
the complete_all() call happens in IRQ context we have an unbound
latency there. Therefore the aim is to reduce the complete_all() users
and get rid of them where possible.

Daniel Wagner (8):
  selftests: firmware: do not abort test too early
  selftests: firmware: do not clutter output
  firmware: Factor out firmware load helpers
  Input: goodix: use firmware_stat instead of completion
  ath9k_htc: use firmware_stat instead of completion
  remoteproc: use firmware_stat instead of completion
  Input: ims-pcu: use firmware_stat instead of completion
  iwl4965: use firmware_stat instead of completion

 drivers/base/firmware_class.c                     | 112 ++++++++++------------
 drivers/input/misc/ims-pcu.c                      |  10 +-
 drivers/input/touchscreen/goodix.c                |  10 +-
 drivers/net/wireless/ath/ath9k/hif_usb.c          |  10 +-
 drivers/net/wireless/ath/ath9k/hif_usb.h          |   2 +-
 drivers/net/wireless/intel/iwlegacy/4965-mac.c    |   8 +-
 drivers/net/wireless/intel/iwlegacy/common.h      |   3 +-
 drivers/remoteproc/remoteproc_core.c              |  10 +-
 drivers/soc/ti/wkup_m3_ipc.c                      |   2 +-
 include/linux/firmware.h                          |  71 ++++++++++++++
 include/linux/remoteproc.h                        |   6 +-
 tools/testing/selftests/firmware/fw_filesystem.sh |   6 +-
 tools/testing/selftests/firmware/fw_userhelper.sh |   2 +-
 13 files changed, 158 insertions(+), 94 deletions(-)

-- 
2.7.4

[RFC v0 1/8] selftests: firmware: do not abort test too early

[Daniel Wagner]

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

When running the test script you will get:

kselftest/firmware/fw_userhelper.sh: line 69: echo: write error: Resource temporarily unavailable

and stops right there. Because the script runs with the '-e' option
which will stop the script at any error.

We should stop there because we are trying to something wrong and get an
error reported back. Instead, ignore the error message and make sure we
do not stop the script by setting the last error code to true.

Signed-off-by: Daniel Wagner <daniel.wagner@bmw-carit.de>
---
 tools/testing/selftests/firmware/fw_userhelper.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/testing/selftests/firmware/fw_userhelper.sh b/tools/testing/selftests/firmware/fw_userhelper.sh
index b9983f8..2d244a7 100755
--- a/tools/testing/selftests/firmware/fw_userhelper.sh
+++ b/tools/testing/selftests/firmware/fw_userhelper.sh
@@ -66,7 +66,7 @@ NAME=$(basename "$FW")
 
 # Test failure when doing nothing (timeout works).
 echo 1 >/sys/class/firmware/timeout
-echo -n "$NAME" >"$DIR"/trigger_request
+echo -n "$NAME" >"$DIR"/trigger_request 2> /dev/null || true
 if diff -q "$FW" /dev/test_firmware >/dev/null ; then
 	echo "$0: firmware was not expected to match" >&2
 	exit 1
-- 
2.7.4

[RFC v0 2/8] selftests: firmware: do not clutter output

[Daniel Wagner]

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

Some of the test are supposed to fail but we get a few ouptput lines:

./fw_filesystem.sh: line 51: printf: write error: Invalid argument
./fw_filesystem.sh: line 56: printf: write error: No such device
./fw_filesystem.sh: line 62: echo: write error: No such file or directory

Let's silence them so that the selftest output is clean if all is fine.

Signed-off-by: Daniel Wagner <daniel.wagner@bmw-carit.de>
---
 tools/testing/selftests/firmware/fw_filesystem.sh | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/tools/testing/selftests/firmware/fw_filesystem.sh b/tools/testing/selftests/firmware/fw_filesystem.sh
index 5c495ad..d8ac9ba 100755
--- a/tools/testing/selftests/firmware/fw_filesystem.sh
+++ b/tools/testing/selftests/firmware/fw_filesystem.sh
@@ -48,18 +48,18 @@ echo "ABCD0123" >"$FW"
 
 NAME=$(basename "$FW")
 
-if printf '\000' >"$DIR"/trigger_request; then
+if printf '\000' >"$DIR"/trigger_request 2> /dev/null; then
 	echo "$0: empty filename should not succeed" >&2
 	exit 1
 fi
 
-if printf '\000' >"$DIR"/trigger_async_request; then
+if printf '\000' >"$DIR"/trigger_async_request 2> /dev/null; then
 	echo "$0: empty filename should not succeed (async)" >&2
 	exit 1
 fi
 
 # Request a firmware that doesn't exist, it should fail.
-if echo -n "nope-$NAME" >"$DIR"/trigger_request; then
+if echo -n "nope-$NAME" >"$DIR"/trigger_request 2> /dev/null; then
 	echo "$0: firmware shouldn't have loaded" >&2
 	exit 1
 fi
-- 
2.7.4

[RFC v0 3/8] firmware: Factor out firmware load helpers

[Daniel Wagner]

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

Factor out the firmware loading synchronization code in order to allow
drivers to reuse it. This also documents more clearly what is
happening. This is especial useful the drivers which will be converted
afterwards. Not everyone has to come with yet another way to handle it.

We use swait instead completion. complete() would only wake up one
waiter, so complete_all() is used. complete_all() wakes max MAX_UINT/2
waiters which is plenty in all cases. Though withh swait we just wait
until the exptected status shows up and wake any waiter.

Signed-off-by: Daniel Wagner <daniel.wagner@bmw-carit.de>
---
 drivers/base/firmware_class.c | 112 +++++++++++++++++++-----------------------
 include/linux/firmware.h      |  71 ++++++++++++++++++++++++++
 2 files changed, 122 insertions(+), 61 deletions(-)

diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
index 773fc30..bf1ca70 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>
 
@@ -85,11 +86,36 @@ static inline bool fw_is_builtin_firmware(const struct firmware *fw)
 }
 #endif
 
-enum {
-	FW_STATUS_LOADING,
-	FW_STATUS_DONE,
-	FW_STATUS_ABORT,
-};
+
+#if defined(CONFIG_FW_LOADER) || (defined(CONFIG_FW_LOADER_MODULE) && defined(MODULE))
+
+static inline bool is_fw_sync_done(unsigned long status)
+{
+	return status == FW_STATUS_LOADED || status == FW_STATUS_ABORT;
+}
+
+int __firmware_stat_wait(struct firmware_stat *fwst,
+				long timeout)
+{
+	int err;
+	err = swait_event_interruptible_timeout(fwst->wq,
+				is_fw_sync_done(READ_ONCE(fwst->status)),
+				timeout);
+	if (err == 0 && fwst->status == FW_STATUS_ABORT)
+		return -ENOENT;
+
+	return err;
+}
+EXPORT_SYMBOL(__firmware_stat_wait);
+
+void __firmware_stat_set(struct firmware_stat *fwst, unsigned long status)
+{
+	WRITE_ONCE(fwst->status, status);
+	swake_up(&fwst->wq);
+}
+EXPORT_SYMBOL(__firmware_stat_set);
+
+#endif
 
 static int loading_timeout = 60;	/* In seconds */
 
@@ -138,9 +164,8 @@ struct firmware_cache {
 struct firmware_buf {
 	struct kref ref;
 	struct list_head list;
-	struct completion completion;
+	struct firmware_stat fwst;
 	struct firmware_cache *fwc;
-	unsigned long status;
 	void *data;
 	size_t size;
 #ifdef CONFIG_FW_LOADER_USER_HELPER
@@ -194,7 +219,7 @@ static struct firmware_buf *__allocate_fw_buf(const char *fw_name,
 
 	kref_init(&buf->ref);
 	buf->fwc = fwc;
-	init_completion(&buf->completion);
+	firmware_stat_init(&buf->fwst);
 #ifdef CONFIG_FW_LOADER_USER_HELPER
 	INIT_LIST_HEAD(&buf->pending_list);
 #endif
@@ -292,15 +317,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)
 {
@@ -339,7 +355,7 @@ static int fw_get_filesystem_firmware(struct device *device,
 		}
 		dev_dbg(device, "direct-loading %s\n", buf->fw_id);
 		buf->size = size;
-		fw_finish_direct_load(device, buf);
+		fw_loading_done(buf->fwst);
 		break;
 	}
 	__putname(path);
@@ -457,12 +473,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 (is_fw_loading_done(buf->fwst))
 		return;
 
 	list_del_init(&buf->pending_list);
-	set_bit(FW_STATUS_ABORT, &buf->status);
-	complete_all(&buf->completion);
+	fw_loading_abort(buf->fwst);
 }
 
 static void fw_load_abort(struct firmware_priv *fw_priv)
@@ -475,9 +490,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 */
@@ -577,7 +589,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 = is_fw_loading(fw_priv->buf->fwst);
 	mutex_unlock(&fw_lock);
 
 	return sprintf(buf, "%d\n", loading);
@@ -632,23 +644,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 (!is_fw_loading_done(fw_buf->fwst)) {
 			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_loading_start(fw_buf->fwst);
 		}
 		break;
 	case 0:
-		if (test_bit(FW_STATUS_LOADING, &fw_buf->status)) {
+		if (is_fw_loading(fw_buf->fwst)) {
 			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
@@ -670,10 +679,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_loading_abort(fw_buf->fwst);
 				written = rc;
+			} else {
+				fw_loading_done(fw_buf->fwst);
 			}
-			complete_all(&fw_buf->completion);
 			break;
 		}
 		/* fallthrough */
@@ -681,7 +691,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_loading_abort(fw_buf->fwst);
 		break;
 	}
 out:
@@ -702,7 +712,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 || is_fw_loading_done(buf->fwst)) {
 		ret_count = -ENODEV;
 		goto out;
 	}
@@ -799,7 +809,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 || is_fw_loading_done(buf->fwst)) {
 		retval = -ENODEV;
 		goto out;
 	}
@@ -917,8 +927,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_loading_wait_timeout(buf->fwst, timeout);
 	if (retval == -ERESTARTSYS || !retval) {
 		mutex_lock(&fw_lock);
 		fw_load_abort(fw_priv);
@@ -927,7 +936,7 @@ static int _request_firmware_load(struct firmware_priv *fw_priv,
 		retval = 0;
 	}
 
-	if (is_fw_load_aborted(buf))
+	if (is_fw_loading_aborted(buf->fwst))
 		retval = -EAGAIN;
 	else if (!buf->data)
 		retval = -ENOMEM;
@@ -986,26 +995,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 (!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
@@ -1039,7 +1028,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_loading_wait_timeout(buf->fwst,
+					      firmware_loading_timeout());
 		if (!ret) {
 			fw_set_page_data(buf, firmware);
 			return 0; /* assigned */
@@ -1057,7 +1047,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 || is_fw_loading_aborted(buf->fwst)) {
 		mutex_unlock(&fw_lock);
 		return -ENOENT;
 	}
diff --git a/include/linux/firmware.h b/include/linux/firmware.h
index 5c41c5e..f584160 100644
--- a/include/linux/firmware.h
+++ b/include/linux/firmware.h
@@ -4,10 +4,17 @@
 #include <linux/types.h>
 #include <linux/compiler.h>
 #include <linux/gfp.h>
+#include <linux/swait.h>
 
 #define FW_ACTION_NOHOTPLUG 0
 #define FW_ACTION_HOTPLUG 1
 
+enum {
+	FW_STATUS_LOADING,
+	FW_STATUS_LOADED,
+	FW_STATUS_ABORT,
+};
+
 struct firmware {
 	size_t size;
 	const u8 *data;
@@ -17,6 +24,11 @@ struct firmware {
 	void *priv;
 };
 
+struct firmware_stat {
+	unsigned long status;
+	struct swait_queue_head wq;
+};
+
 struct module;
 struct device;
 
@@ -49,6 +61,36 @@ int request_firmware_direct(const struct firmware **fw, const char *name,
 			    struct device *device);
 
 void release_firmware(const struct firmware *fw);
+
+static inline void firmware_stat_init(struct firmware_stat *fwst)
+{
+	init_swait_queue_head(&fwst->wq);
+}
+
+static inline unsigned long __firmware_stat_get(struct firmware_stat *fwst)
+{
+	return READ_ONCE(fwst->status);
+}
+void __firmware_stat_set(struct firmware_stat *fwst, unsigned long status);
+int __firmware_stat_wait(struct firmware_stat *fwst, long timeout);
+
+#define fw_loading_start(fwst)					\
+	__firmware_stat_set(&fwst, FW_STATUS_LOADING)
+#define fw_loading_done(fwst)					\
+	__firmware_stat_set(&fwst, FW_STATUS_LOADED)
+#define fw_loading_abort(fwst)					\
+	__firmware_stat_set(&fwst, FW_STATUS_ABORT)
+#define fw_loading_wait(fwst)					\
+	__firmware_stat_wait(&fwst, 0)
+#define fw_loading_wait_timeout(fwst, timeout)			\
+	__firmware_stat_wait(&fwst, timeout)
+#define is_fw_loading(fwst)					\
+	(__firmware_stat_get(&fwst) == FW_STATUS_LOADING)
+#define is_fw_loading_done(fwst)				\
+	(__firmware_stat_get(&fwst) == FW_STATUS_LOADED)
+#define is_fw_loading_aborted(fwst)				\
+	(__firmware_stat_get(&fwst) == FW_STATUS_ABORT)
+
 #else
 static inline int request_firmware(const struct firmware **fw,
 				   const char *name,
@@ -75,5 +117,34 @@ static inline int request_firmware_direct(const struct firmware **fw,
 	return -EINVAL;
 }
 
+static inline void firmware_stat_init(struct firmware_stat *fwst)
+{
+}
+
+static inline unsigned long __firmware_stat_get(struct firmware_stat *fwst)
+{
+	return -EINVAL;
+}
+
+static inline void __firmware_stat_set(struct firmware_stat *fwst,
+				       unsigned long status)
+{
+}
+
+static inline int __firmware_stat_wait(struct firmware_stat *fwst,
+				       long timeout)
+{
+	return -EINVAL;
+}
+
+#define fw_loading_start(fwst)
+#define fw_loading_done(fwst)
+#define fw_loading_abort(fwst)
+#define fw_loading_wait(fwst)
+#define fw_loading_wait_timeout(fwst, timeout)
+#define is_fw_loading(fwst)		0
+#define is_fw_loading_done(fwst)	0
+#define is_fw_loading_aborted(fwst)	0
+
 #endif
 #endif
-- 
2.7.4

Re: [RFC v0 3/8] firmware: Factor out firmware load helpers

[Dan Williams]

On Thu, 2016-07-28 at 09:55 +0200, Daniel Wagner wrote:
> From: Daniel Wagner <daniel.wagner@bmw-carit.de>
> 
> Factor out the firmware loading synchronization code in order to
> allow
> drivers to reuse it. This also documents more clearly what is
> happening. This is especial useful the drivers which will be
> converted
> afterwards. Not everyone has to come with yet another way to handle
> it.

It's somewhat odd to me that the structure is "firmware_stat" but most
of the functions are "fw_loading_*".  That seems inconsistent for a
structure that is (currently) only used by these functions.

I would personally do either:

a) "struct fw_load_status" and "fw_load_*()"

or

b) "struct firmware_load_stat" and "firmware_load_*()"

I'd also change the function names from "loading" -> "load", similar to
how userland has read(2), not reading(2).

Dan

> We use swait instead completion. complete() would only wake up one
> waiter, so complete_all() is used. complete_all() wakes max
> MAX_UINT/2
> waiters which is plenty in all cases. Though withh swait we just wait
> until the exptected status shows up and wake any waiter.
> 
> Signed-off-by: Daniel Wagner <daniel.wagner@bmw-carit.de>
> ---
>  drivers/base/firmware_class.c | 112 +++++++++++++++++++-------------
> ----------
>  include/linux/firmware.h      |  71 ++++++++++++++++++++++++++
>  2 files changed, 122 insertions(+), 61 deletions(-)
> 
> diff --git a/drivers/base/firmware_class.c
> b/drivers/base/firmware_class.c
> index 773fc30..bf1ca70 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>
>  
> @@ -85,11 +86,36 @@ static inline bool fw_is_builtin_firmware(const
> struct firmware *fw)
>  }
>  #endif
>  
> -enum {
> -	FW_STATUS_LOADING,
> -	FW_STATUS_DONE,
> -	FW_STATUS_ABORT,
> -};
> +
> +#if defined(CONFIG_FW_LOADER) || (defined(CONFIG_FW_LOADER_MODULE)
> && defined(MODULE))
> +
> +static inline bool is_fw_sync_done(unsigned long status)
> +{
> +	return status == FW_STATUS_LOADED || status ==
> FW_STATUS_ABORT;
> +}
> +
> +int __firmware_stat_wait(struct firmware_stat *fwst,
> +				long timeout)
> +{
> +	int err;
> +	err = swait_event_interruptible_timeout(fwst->wq,
> +				is_fw_sync_done(READ_ONCE(fwst-
> >status)),
> +				timeout);
> +	if (err == 0 && fwst->status == FW_STATUS_ABORT)
> +		return -ENOENT;
> +
> +	return err;
> +}
> +EXPORT_SYMBOL(__firmware_stat_wait);
> +
> +void __firmware_stat_set(struct firmware_stat *fwst, unsigned long
> status)
> +{
> +	WRITE_ONCE(fwst->status, status);
> +	swake_up(&fwst->wq);
> +}
> +EXPORT_SYMBOL(__firmware_stat_set);
> +
> +#endif
>  
>  static int loading_timeout = 60;	/* In seconds */
>  
> @@ -138,9 +164,8 @@ struct firmware_cache {
>  struct firmware_buf {
>  	struct kref ref;
>  	struct list_head list;
> -	struct completion completion;
> +	struct firmware_stat fwst;
>  	struct firmware_cache *fwc;
> -	unsigned long status;
>  	void *data;
>  	size_t size;
>  #ifdef CONFIG_FW_LOADER_USER_HELPER
> @@ -194,7 +219,7 @@ static struct firmware_buf
> *__allocate_fw_buf(const char *fw_name,
>  
>  	kref_init(&buf->ref);
>  	buf->fwc = fwc;
> -	init_completion(&buf->completion);
> +	firmware_stat_init(&buf->fwst);
>  #ifdef CONFIG_FW_LOADER_USER_HELPER
>  	INIT_LIST_HEAD(&buf->pending_list);
>  #endif
> @@ -292,15 +317,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)
>  {
> @@ -339,7 +355,7 @@ static int fw_get_filesystem_firmware(struct
> device *device,
>  		}
>  		dev_dbg(device, "direct-loading %s\n", buf->fw_id);
>  		buf->size = size;
> -		fw_finish_direct_load(device, buf);
> +		fw_loading_done(buf->fwst);
>  		break;
>  	}
>  	__putname(path);
> @@ -457,12 +473,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 (is_fw_loading_done(buf->fwst))
>  		return;
>  
>  	list_del_init(&buf->pending_list);
> -	set_bit(FW_STATUS_ABORT, &buf->status);
> -	complete_all(&buf->completion);
> +	fw_loading_abort(buf->fwst);
>  }
>  
>  static void fw_load_abort(struct firmware_priv *fw_priv)
> @@ -475,9 +490,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 */
> @@ -577,7 +589,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 = is_fw_loading(fw_priv->buf->fwst);
>  	mutex_unlock(&fw_lock);
>  
>  	return sprintf(buf, "%d\n", loading);
> @@ -632,23 +644,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 (!is_fw_loading_done(fw_buf->fwst)) {
>  			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_loading_start(fw_buf->fwst);
>  		}
>  		break;
>  	case 0:
> -		if (test_bit(FW_STATUS_LOADING, &fw_buf->status)) {
> +		if (is_fw_loading(fw_buf->fwst)) {
>  			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
> @@ -670,10 +679,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_loading_abort(fw_buf->fwst);
>  				written = rc;
> +			} else {
> +				fw_loading_done(fw_buf->fwst);
>  			}
> -			complete_all(&fw_buf->completion);
>  			break;
>  		}
>  		/* fallthrough */
> @@ -681,7 +691,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_loading_abort(fw_buf->fwst);
>  		break;
>  	}
>  out:
> @@ -702,7 +712,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 || is_fw_loading_done(buf->fwst)) {
>  		ret_count = -ENODEV;
>  		goto out;
>  	}
> @@ -799,7 +809,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 || is_fw_loading_done(buf->fwst)) {
>  		retval = -ENODEV;
>  		goto out;
>  	}
> @@ -917,8 +927,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_loading_wait_timeout(buf->fwst, timeout);
>  	if (retval == -ERESTARTSYS || !retval) {
>  		mutex_lock(&fw_lock);
>  		fw_load_abort(fw_priv);
> @@ -927,7 +936,7 @@ static int _request_firmware_load(struct
> firmware_priv *fw_priv,
>  		retval = 0;
>  	}
>  
> -	if (is_fw_load_aborted(buf))
> +	if (is_fw_loading_aborted(buf->fwst))
>  		retval = -EAGAIN;
>  	else if (!buf->data)
>  		retval = -ENOMEM;
> @@ -986,26 +995,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 (!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
> @@ -1039,7 +1028,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_loading_wait_timeout(buf->fwst,
> +					      firmware_loading_timeo
> ut());
>  		if (!ret) {
>  			fw_set_page_data(buf, firmware);
>  			return 0; /* assigned */
> @@ -1057,7 +1047,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 || is_fw_loading_aborted(buf->fwst)) {
>  		mutex_unlock(&fw_lock);
>  		return -ENOENT;
>  	}
> diff --git a/include/linux/firmware.h b/include/linux/firmware.h
> index 5c41c5e..f584160 100644
> --- a/include/linux/firmware.h
> +++ b/include/linux/firmware.h
> @@ -4,10 +4,17 @@
>  #include <linux/types.h>
>  #include <linux/compiler.h>
>  #include <linux/gfp.h>
> +#include <linux/swait.h>
>  
>  #define FW_ACTION_NOHOTPLUG 0
>  #define FW_ACTION_HOTPLUG 1
>  
> +enum {
> +	FW_STATUS_LOADING,
> +	FW_STATUS_LOADED,
> +	FW_STATUS_ABORT,
> +};
> +
>  struct firmware {
>  	size_t size;
>  	const u8 *data;
> @@ -17,6 +24,11 @@ struct firmware {
>  	void *priv;
>  };
>  
> +struct firmware_stat {
> +	unsigned long status;
> +	struct swait_queue_head wq;
> +};
> +
>  struct module;
>  struct device;
>  
> @@ -49,6 +61,36 @@ int request_firmware_direct(const struct firmware
> **fw, const char *name,
>  			    struct device *device);
>  
>  void release_firmware(const struct firmware *fw);
> +
> +static inline void firmware_stat_init(struct firmware_stat *fwst)
> +{
> +	init_swait_queue_head(&fwst->wq);
> +}
> +
> +static inline unsigned long __firmware_stat_get(struct firmware_stat
> *fwst)
> +{
> +	return READ_ONCE(fwst->status);
> +}
> +void __firmware_stat_set(struct firmware_stat *fwst, unsigned long
> status);
> +int __firmware_stat_wait(struct firmware_stat *fwst, long timeout);
> +
> +#define fw_loading_start(fwst)					
> \
> +	__firmware_stat_set(&fwst, FW_STATUS_LOADING)
> +#define fw_loading_done(fwst)					
> \
> +	__firmware_stat_set(&fwst, FW_STATUS_LOADED)
> +#define fw_loading_abort(fwst)					
> \
> +	__firmware_stat_set(&fwst, FW_STATUS_ABORT)
> +#define fw_loading_wait(fwst)					
> \
> +	__firmware_stat_wait(&fwst, 0)
> +#define fw_loading_wait_timeout(fwst, timeout)			
> \
> +	__firmware_stat_wait(&fwst, timeout)
> +#define is_fw_loading(fwst)					\
> +	(__firmware_stat_get(&fwst) == FW_STATUS_LOADING)
> +#define is_fw_loading_done(fwst)				\
> +	(__firmware_stat_get(&fwst) == FW_STATUS_LOADED)
> +#define is_fw_loading_aborted(fwst)				\
> +	(__firmware_stat_get(&fwst) == FW_STATUS_ABORT)
> +
>  #else
>  static inline int request_firmware(const struct firmware **fw,
>  				   const char *name,
> @@ -75,5 +117,34 @@ static inline int request_firmware_direct(const
> struct firmware **fw,
>  	return -EINVAL;
>  }
>  
> +static inline void firmware_stat_init(struct firmware_stat *fwst)
> +{
> +}
> +
> +static inline unsigned long __firmware_stat_get(struct firmware_stat
> *fwst)
> +{
> +	return -EINVAL;
> +}
> +
> +static inline void __firmware_stat_set(struct firmware_stat *fwst,
> +				       unsigned long status)
> +{
> +}
> +
> +static inline int __firmware_stat_wait(struct firmware_stat *fwst,
> +				       long timeout)
> +{
> +	return -EINVAL;
> +}
> +
> +#define fw_loading_start(fwst)
> +#define fw_loading_done(fwst)
> +#define fw_loading_abort(fwst)
> +#define fw_loading_wait(fwst)
> +#define fw_loading_wait_timeout(fwst, timeout)
> +#define is_fw_loading(fwst)		0
> +#define is_fw_loading_done(fwst)	0
> +#define is_fw_loading_aborted(fwst)	0
> +
>  #endif
>  #endif
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC v0 3/8] firmware: Factor out firmware load helpers

[Daniel Wagner]

> It's somewhat odd to me that the structure is "firmware_stat" but most
> of the functions are "fw_loading_*".  That seems inconsistent for a
> structure that is (currently) only used by these functions.

I agree, my proposal is odd.

> I would personally do either:
>
> a) "struct fw_load_status" and "fw_load_*()"
>
> or
>
> b) "struct firmware_load_stat" and "firmware_load_*()"
>
> I'd also change the function names from "loading" -> "load", similar to
> how userland has read(2), not reading(2).

a) sounds good to me, because fw_load_ should be long enough prefix.

cheers,
daniel

Re: [RFC v0 3/8] firmware: Factor out firmware load helpers

[Dmitry Torokhov]

On Thu, Jul 28, 2016 at 09:55:07AM +0200, Daniel Wagner wrote:
> +int __firmware_stat_wait(struct firmware_stat *fwst,
> +				long timeout)
> +{
> +	int err;
> +	err = swait_event_interruptible_timeout(fwst->wq,
> +				is_fw_sync_done(READ_ONCE(fwst->status)),
> +				timeout);
> +	if (err == 0 && fwst->status == FW_STATUS_ABORT)
> +		return -ENOENT;
> +
> +	return err;
> +}
> +EXPORT_SYMBOL(__firmware_stat_wait);
> +
> +void __firmware_stat_set(struct firmware_stat *fwst, unsigned long status)
> +{
> +	WRITE_ONCE(fwst->status, status);
> +	swake_up(&fwst->wq);

Do we need to notify everyone for FW_STATUS_LOADING status?

The driver users do not care for sure.

Thanks.

-- 
Dmitry

Re: [RFC v0 3/8] firmware: Factor out firmware load helpers

[Daniel Wagner]

On 07/28/2016 07:57 PM, Dmitry Torokhov wrote:
> On Thu, Jul 28, 2016 at 09:55:07AM +0200, Daniel Wagner wrote:
>> +int __firmware_stat_wait(struct firmware_stat *fwst,
>> +				long timeout)
>> +{
>> +	int err;
>> +	err = swait_event_interruptible_timeout(fwst->wq,
>> +				is_fw_sync_done(READ_ONCE(fwst->status)),
>> +				timeout);
>> +	if (err == 0 && fwst->status == FW_STATUS_ABORT)
>> +		return -ENOENT;
>> +
>> +	return err;
>> +}
>> +EXPORT_SYMBOL(__firmware_stat_wait);
>> +
>> +void __firmware_stat_set(struct firmware_stat *fwst, unsigned long status)
>> +{
>> +	WRITE_ONCE(fwst->status, status);
>> +	swake_up(&fwst->wq);
>
> Do we need to notify everyone for FW_STATUS_LOADING status?

Hmm, I don't think so. In the end drivers are probably only interested 
in the final result which is either success or fail.

cheers,
daniel
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[RFC v0 4/8] Input: goodix: use firmware_stat instead of completion

[Daniel Wagner]

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

Loading firmware is an operation many drivers implement in various ways
around the completion API. And most of them do it almost in the same
way. Let's reuse the firmware_stat API which is used also by the
firmware_class loader. Apart of streamlining the firmware loading states
we also document it slightly better.

Signed-off-by: Daniel Wagner <daniel.wagner@bmw-carit.de>
---
 drivers/input/touchscreen/goodix.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/input/touchscreen/goodix.c b/drivers/input/touchscreen/goodix.c
index 240b16f..67158d3 100644
--- a/drivers/input/touchscreen/goodix.c
+++ b/drivers/input/touchscreen/goodix.c
@@ -47,7 +47,7 @@ struct goodix_ts_data {
 	u16 id;
 	u16 version;
 	const char *cfg_name;
-	struct completion firmware_loading_complete;
+	struct firmware_stat fwst;
 	unsigned long irq_flags;
 };
 
@@ -683,7 +683,7 @@ static void goodix_config_cb(const struct firmware *cfg, void *ctx)
 
 err_release_cfg:
 	release_firmware(cfg);
-	complete_all(&ts->firmware_loading_complete);
+	fw_loading_done(ts->fwst);
 }
 
 static int goodix_ts_probe(struct i2c_client *client,
@@ -705,7 +705,7 @@ static int goodix_ts_probe(struct i2c_client *client,
 
 	ts->client = client;
 	i2c_set_clientdata(client, ts);
-	init_completion(&ts->firmware_loading_complete);
+	firmware_stat_init(&ts->fwst);
 
 	error = goodix_get_gpio_config(ts);
 	if (error)
@@ -766,7 +766,7 @@ static int goodix_ts_remove(struct i2c_client *client)
 	struct goodix_ts_data *ts = i2c_get_clientdata(client);
 
 	if (ts->gpiod_int && ts->gpiod_rst)
-		wait_for_completion(&ts->firmware_loading_complete);
+		fw_loading_wait(ts->fwst);
 
 	return 0;
 }
@@ -781,7 +781,7 @@ static int __maybe_unused goodix_suspend(struct device *dev)
 	if (!ts->gpiod_int || !ts->gpiod_rst)
 		return 0;
 
-	wait_for_completion(&ts->firmware_loading_complete);
+	fw_loading_wait(ts->fwst);
 
 	/* Free IRQ as IRQ pin is used as output in the suspend sequence */
 	goodix_free_irq(ts);
-- 
2.7.4

Re: [RFC v0 4/8] Input: goodix: use firmware_stat instead of completion

[Bastien Nocera]

On Thu, 2016-07-28 at 09:55 +0200, Daniel Wagner wrote:
> From: Daniel Wagner <daniel.wagner@bmw-carit.de>
> 
> Loading firmware is an operation many drivers implement in various
> ways
> around the completion API. And most of them do it almost in the same
> way. Let's reuse the firmware_stat API which is used also by the
> firmware_class loader. Apart of streamlining the firmware loading
> states
> we also document it slightly better.
> 
> Signed-off-by: Daniel Wagner <daniel.wagner@bmw-carit.de>

Irina added and tested that feature, so best for her to comment on
this, as I don't have any hardware that would use this feature.

Cheers

Re: [RFC v0 4/8] Input: goodix: use firmware_stat instead of completion

[Daniel Wagner]

On 07/28/2016 01:22 PM, Bastien Nocera wrote:
> On Thu, 2016-07-28 at 09:55 +0200, Daniel Wagner wrote:
>> From: Daniel Wagner <daniel.wagner@bmw-carit.de>
>>
>> Loading firmware is an operation many drivers implement in various
>> ways
>> around the completion API. And most of them do it almost in the same
>> way. Let's reuse the firmware_stat API which is used also by the
>> firmware_class loader. Apart of streamlining the firmware loading
>> states
>> we also document it slightly better.
>>
>> Signed-off-by: Daniel Wagner <daniel.wagner@bmw-carit.de>
>
> Irina added and tested that feature, so best for her to comment on
> this, as I don't have any hardware that would use this feature.

In case you have any comments on the API, let me know. I'll add Irina to 
the Cc list in the next version.

cheers,
daniel

Re: [RFC v0 4/8] Input: goodix: use firmware_stat instead of completion

[Bastien Nocera]

On Thu, 2016-07-28 at 13:59 +0200, Daniel Wagner wrote:
> On 07/28/2016 01:22 PM, Bastien Nocera wrote:
> > On Thu, 2016-07-28 at 09:55 +0200, Daniel Wagner wrote:
> > > From: Daniel Wagner <daniel.wagner@bmw-carit.de>
> > > 
> > > Loading firmware is an operation many drivers implement in
> > > various
> > > ways
> > > around the completion API. And most of them do it almost in the
> > > same
> > > way. Let's reuse the firmware_stat API which is used also by the
> > > firmware_class loader. Apart of streamlining the firmware loading
> > > states
> > > we also document it slightly better.
> > > 
> > > Signed-off-by: Daniel Wagner <daniel.wagner@bmw-carit.de>
> > 
> > Irina added and tested that feature, so best for her to comment on
> > this, as I don't have any hardware that would use this feature.
> 
> In case you have any comments on the API, let me know. I'll add Irina
> to 
> the Cc list in the next version.

Looking at the API, I really don't like the mixing of namespaces.
Either it's fw_ or it's firmware_ but not a mix of both.

Also looks like fw_loading_start() would do nothing as the struct is
likely zero initialised, and FW_STATUS_LOADING == 0. Maybe you need an
UNSET enum member?

FW_STATUS_ABORT <- FW_STATUS_ABORTED, to match the adjective suffixes
of the other members. Ditto fw_loading_abort() which doesn't abort
firmware loading but sets the status.

Cheers

Re: [RFC v0 4/8] Input: goodix: use firmware_stat instead of completion

[Daniel Wagner]

> Looking at the API, I really don't like the mixing of namespaces.
> Either it's fw_ or it's firmware_ but not a mix of both.

I agree, that was not really clever.

	struct fw_loading { ... }
	__fw_loading_*()
	fw_loading_*()

Would that make more sense? firmware_loading_ as prefix is a bit long IMO.

> Also looks like fw_loading_start() would do nothing as the struct is
> likely zero initialised, and FW_STATUS_LOADING == 0. Maybe you need an
> UNSET enum member?

Good point, I cut a corner here a bit :). In the spirit of making things 
more clear fw_loading_start() should be used.

> FW_STATUS_ABORT <- FW_STATUS_ABORTED, to match the adjective suffixes
> of the other members. Ditto fw_loading_abort() which doesn't abort
> firmware loading but sets the status.

Okay.

Thanks a lot for the feedback.

cheers,
daniel

[RFC v0 5/8] ath9k_htc: use firmware_stat instead of completion

[Daniel Wagner]

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

Loading firmware is an operation many drivers implement in various ways
around the completion API. And most of them do it almost in the same
way. Let's reuse the firmware_stat API which is used also by the
firmware_class loader. Apart of streamlining the firmware loading states
we also document it slightly better.

Signed-off-by: Daniel Wagner <daniel.wagner@bmw-carit.de>
---
 drivers/net/wireless/ath/ath9k/hif_usb.c | 10 +++++-----
 drivers/net/wireless/ath/ath9k/hif_usb.h |  2 +-
 2 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/net/wireless/ath/ath9k/hif_usb.c b/drivers/net/wireless/ath/ath9k/hif_usb.c
index e1c338c..0a05d68 100644
--- a/drivers/net/wireless/ath/ath9k/hif_usb.c
+++ b/drivers/net/wireless/ath/ath9k/hif_usb.c
@@ -1067,7 +1067,7 @@ static void ath9k_hif_usb_firmware_fail(struct hif_device_usb *hif_dev)
 	struct device *dev = &hif_dev->udev->dev;
 	struct device *parent = dev->parent;
 
-	complete_all(&hif_dev->fw_done);
+	fw_loading_abort(hif_dev->fw_st);
 
 	if (parent)
 		device_lock(parent);
@@ -1192,7 +1192,7 @@ static void ath9k_hif_usb_firmware_cb(const struct firmware *fw, void *context)
 
 	release_firmware(fw);
 	hif_dev->flags |= HIF_USB_READY;
-	complete_all(&hif_dev->fw_done);
+	fw_loading_done(hif_dev->fw_st);
 
 	return;
 
@@ -1287,7 +1287,7 @@ static int ath9k_hif_usb_probe(struct usb_interface *interface,
 #endif
 	usb_set_intfdata(interface, hif_dev);
 
-	init_completion(&hif_dev->fw_done);
+	firmware_stat_init(&hif_dev->fw_st);
 
 	ret = ath9k_hif_request_firmware(hif_dev, true);
 	if (ret)
@@ -1330,7 +1330,7 @@ static void ath9k_hif_usb_disconnect(struct usb_interface *interface)
 	if (!hif_dev)
 		return;
 
-	wait_for_completion(&hif_dev->fw_done);
+	fw_loading_wait(hif_dev->fw_st);
 
 	if (hif_dev->flags & HIF_USB_READY) {
 		ath9k_htc_hw_deinit(hif_dev->htc_handle, unplugged);
@@ -1363,7 +1363,7 @@ static int ath9k_hif_usb_suspend(struct usb_interface *interface,
 	if (!(hif_dev->flags & HIF_USB_START))
 		ath9k_htc_suspend(hif_dev->htc_handle);
 
-	wait_for_completion(&hif_dev->fw_done);
+	fw_loading_wait(hif_dev->fw_st);
 
 	if (hif_dev->flags & HIF_USB_READY)
 		ath9k_hif_usb_dealloc_urbs(hif_dev);
diff --git a/drivers/net/wireless/ath/ath9k/hif_usb.h b/drivers/net/wireless/ath/ath9k/hif_usb.h
index 7c2ef7e..0af9fe4 100644
--- a/drivers/net/wireless/ath/ath9k/hif_usb.h
+++ b/drivers/net/wireless/ath/ath9k/hif_usb.h
@@ -111,7 +111,7 @@ struct hif_device_usb {
 	const struct usb_device_id *usb_device_id;
 	const void *fw_data;
 	size_t fw_size;
-	struct completion fw_done;
+	struct firmware_stat fw_st;
 	struct htc_target *htc_handle;
 	struct hif_usb_tx tx;
 	struct usb_anchor regout_submitted;
-- 
2.7.4

[RFC v0 6/8] remoteproc: use firmware_stat instead of completion

[Daniel Wagner]

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

Loading firmware is an operation many drivers implement in various ways
around the completion API. And most of them do it almost in the same
way. Let's reuse the firmware_stat API which is used also by the
firmware_class loader. Apart of streamlining the firmware loading states
we also document it slightly better.

Signed-off-by: Daniel Wagner <daniel.wagner@bmw-carit.de>
---
 drivers/remoteproc/remoteproc_core.c | 10 +++++-----
 drivers/soc/ti/wkup_m3_ipc.c         |  2 +-
 include/linux/remoteproc.h           |  6 ++++--
 3 files changed, 10 insertions(+), 8 deletions(-)

diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
index db3958b..3b158f7 100644
--- a/drivers/remoteproc/remoteproc_core.c
+++ b/drivers/remoteproc/remoteproc_core.c
@@ -936,7 +936,7 @@ static void rproc_fw_config_virtio(const struct firmware *fw, void *context)
 out:
 	release_firmware(fw);
 	/* allow rproc_del() contexts, if any, to proceed */
-	complete_all(&rproc->firmware_loading_complete);
+	fw_loading_done(rproc->fw_st);
 }
 
 static int rproc_add_virtio_devices(struct rproc *rproc)
@@ -944,7 +944,7 @@ static int rproc_add_virtio_devices(struct rproc *rproc)
 	int ret;
 
 	/* rproc_del() calls must wait until async loader completes */
-	init_completion(&rproc->firmware_loading_complete);
+	firmware_stat_init(&rproc->fw_st);
 
 	/*
 	 * We must retrieve early virtio configuration info from
@@ -959,7 +959,7 @@ static int rproc_add_virtio_devices(struct rproc *rproc)
 				      rproc, rproc_fw_config_virtio);
 	if (ret < 0) {
 		dev_err(&rproc->dev, "request_firmware_nowait err: %d\n", ret);
-		complete_all(&rproc->firmware_loading_complete);
+		fw_loading_abort(rproc->fw_st);
 	}
 
 	return ret;
@@ -1089,7 +1089,7 @@ static int __rproc_boot(struct rproc *rproc, bool wait)
 
 	/* if rproc virtio is not yet configured, wait */
 	if (wait)
-		wait_for_completion(&rproc->firmware_loading_complete);
+		fw_loading_wait(rproc->fw_st);
 
 	ret = rproc_fw_boot(rproc, firmware_p);
 
@@ -1447,7 +1447,7 @@ int rproc_del(struct rproc *rproc)
 		return -EINVAL;
 
 	/* if rproc is just being registered, wait */
-	wait_for_completion(&rproc->firmware_loading_complete);
+	fw_loading_wait(rproc->fw_st);
 
 	/* clean up remote vdev entries */
 	list_for_each_entry_safe(rvdev, tmp, &rproc->rvdevs, node)
diff --git a/drivers/soc/ti/wkup_m3_ipc.c b/drivers/soc/ti/wkup_m3_ipc.c
index 8823cc8..14c6396 100644
--- a/drivers/soc/ti/wkup_m3_ipc.c
+++ b/drivers/soc/ti/wkup_m3_ipc.c
@@ -370,7 +370,7 @@ static void wkup_m3_rproc_boot_thread(struct wkup_m3_ipc *m3_ipc)
 	struct device *dev = m3_ipc->dev;
 	int ret;
 
-	wait_for_completion(&m3_ipc->rproc->firmware_loading_complete);
+	fw_loading_wait(m3_ipc->rproc->fw_st);
 
 	init_completion(&m3_ipc->sync_complete);
 
diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
index 1c457a8..b8e7ff4 100644
--- a/include/linux/remoteproc.h
+++ b/include/linux/remoteproc.h
@@ -41,6 +41,7 @@
 #include <linux/completion.h>
 #include <linux/idr.h>
 #include <linux/of.h>
+#include <linux/firmware.h>
 
 /**
  * struct resource_table - firmware resource table header
@@ -397,7 +398,7 @@ enum rproc_crash_type {
  * @num_traces: number of trace buffers
  * @carveouts: list of physically contiguous memory allocations
  * @mappings: list of iommu mappings we initiated, needed on shutdown
- * @firmware_loading_complete: marks e/o asynchronous firmware loading
+ * @fw_sync: marks e/o asynchronous firmware loading
  * @bootaddr: address of first instruction to boot rproc with (optional)
  * @rvdevs: list of remote virtio devices
  * @notifyids: idr for dynamically assigning rproc-wide unique notify ids
@@ -429,7 +430,7 @@ struct rproc {
 	int num_traces;
 	struct list_head carveouts;
 	struct list_head mappings;
-	struct completion firmware_loading_complete;
+	struct firmware_stat fw_st;
 	u32 bootaddr;
 	struct list_head rvdevs;
 	struct idr notifyids;
@@ -479,6 +480,7 @@ struct rproc_vring {
  * @vring: the vrings for this vdev
  * @rsc_offset: offset of the vdev's resource entry
  */
+
 struct rproc_vdev {
 	struct list_head node;
 	struct rproc *rproc;
-- 
2.7.4

[RFC v0 7/8] Input: ims-pcu: use firmware_stat instead of completion

[Daniel Wagner]

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

Loading firmware is an operation many drivers implement in various ways
around the completion API. And most of them do it almost in the same
way. Let's reuse the firmware_stat API which is used also by the
firmware_class loader. Apart of streamlining the firmware loading states
we also document it slightly better.

Signed-off-by: Daniel Wagner <daniel.wagner@bmw-carit.de>
---
 drivers/input/misc/ims-pcu.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/input/misc/ims-pcu.c b/drivers/input/misc/ims-pcu.c
index 9c0ea36..cda1fbf 100644
--- a/drivers/input/misc/ims-pcu.c
+++ b/drivers/input/misc/ims-pcu.c
@@ -109,7 +109,7 @@ struct ims_pcu {
 
 	u32 fw_start_addr;
 	u32 fw_end_addr;
-	struct completion async_firmware_done;
+	struct firmware_stat fw_st;
 
 	struct ims_pcu_buttons buttons;
 	struct ims_pcu_gamepad *gamepad;
@@ -940,7 +940,7 @@ static void ims_pcu_process_async_firmware(const struct firmware *fw,
 	release_firmware(fw);
 
 out:
-	complete(&pcu->async_firmware_done);
+	fw_loading_done(pcu->fw_st);
 }
 
 /*********************************************************************
@@ -1967,7 +1967,7 @@ static int ims_pcu_init_bootloader_mode(struct ims_pcu *pcu)
 					ims_pcu_process_async_firmware);
 	if (error) {
 		/* This error is not fatal, let userspace have another chance */
-		complete(&pcu->async_firmware_done);
+		fw_loading_abort(pcu->fw_st);
 	}
 
 	return 0;
@@ -1976,7 +1976,7 @@ static int ims_pcu_init_bootloader_mode(struct ims_pcu *pcu)
 static void ims_pcu_destroy_bootloader_mode(struct ims_pcu *pcu)
 {
 	/* Make sure our initial firmware request has completed */
-	wait_for_completion(&pcu->async_firmware_done);
+	fw_loading_wait(pcu->fw_st);
 }
 
 #define IMS_PCU_APPLICATION_MODE	0
@@ -2000,7 +2000,7 @@ static int ims_pcu_probe(struct usb_interface *intf,
 	pcu->bootloader_mode = id->driver_info == IMS_PCU_BOOTLOADER_MODE;
 	mutex_init(&pcu->cmd_mutex);
 	init_completion(&pcu->cmd_done);
-	init_completion(&pcu->async_firmware_done);
+	firmware_stat_init(&pcu->fw_st);
 
 	error = ims_pcu_parse_cdc_data(intf, pcu);
 	if (error)
-- 
2.7.4

Re: [RFC v0 7/8] Input: ims-pcu: use firmware_stat instead of completion

[Dmitry Torokhov]

On Thu, Jul 28, 2016 at 09:55:11AM +0200, Daniel Wagner wrote:
> From: Daniel Wagner <daniel.wagner@bmw-carit.de>
> 
> Loading firmware is an operation many drivers implement in various ways
> around the completion API. And most of them do it almost in the same
> way. Let's reuse the firmware_stat API which is used also by the
> firmware_class loader. Apart of streamlining the firmware loading states
> we also document it slightly better.
> 
> Signed-off-by: Daniel Wagner <daniel.wagner@bmw-carit.de>
> ---
>  drivers/input/misc/ims-pcu.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/input/misc/ims-pcu.c b/drivers/input/misc/ims-pcu.c
> index 9c0ea36..cda1fbf 100644
> --- a/drivers/input/misc/ims-pcu.c
> +++ b/drivers/input/misc/ims-pcu.c
> @@ -109,7 +109,7 @@ struct ims_pcu {
>  
>  	u32 fw_start_addr;
>  	u32 fw_end_addr;
> -	struct completion async_firmware_done;
> +	struct firmware_stat fw_st;
>  
>  	struct ims_pcu_buttons buttons;
>  	struct ims_pcu_gamepad *gamepad;
> @@ -940,7 +940,7 @@ static void ims_pcu_process_async_firmware(const struct firmware *fw,
>  	release_firmware(fw);
>  
>  out:
> -	complete(&pcu->async_firmware_done);
> +	fw_loading_done(pcu->fw_st);

Why does the driver have to do it? If firmware loader manages this, then
it should let waiters know when callback finishes. 

>  }
>  
>  /*********************************************************************
> @@ -1967,7 +1967,7 @@ static int ims_pcu_init_bootloader_mode(struct ims_pcu *pcu)
>  					ims_pcu_process_async_firmware);
>  	if (error) {
>  		/* This error is not fatal, let userspace have another chance */
> -		complete(&pcu->async_firmware_done);
> +		fw_loading_abort(pcu->fw_st);

Why should the driver signal abort if it does not manage completion in
this case?

>  	}
>  
>  	return 0;
> @@ -1976,7 +1976,7 @@ static int ims_pcu_init_bootloader_mode(struct ims_pcu *pcu)
>  static void ims_pcu_destroy_bootloader_mode(struct ims_pcu *pcu)
>  {
>  	/* Make sure our initial firmware request has completed */
> -	wait_for_completion(&pcu->async_firmware_done);
> +	fw_loading_wait(pcu->fw_st);
>  }
>  
>  #define IMS_PCU_APPLICATION_MODE	0
> @@ -2000,7 +2000,7 @@ static int ims_pcu_probe(struct usb_interface *intf,
>  	pcu->bootloader_mode = id->driver_info == IMS_PCU_BOOTLOADER_MODE;
>  	mutex_init(&pcu->cmd_mutex);
>  	init_completion(&pcu->cmd_done);
> -	init_completion(&pcu->async_firmware_done);
> +	firmware_stat_init(&pcu->fw_st);

Do not quite like it... I'd rather asynchronous request give out a
firmware status pointer that could be used later on.

	pcu->fw_st = request_firmware_async(IMS_PCU_FIRMWARE_NAME,
					    pcu,
					    ims_pcu_process_async_firmware);
	if (IS_ERR(pcu->fw_st))
		return PTR_ERR(pcu->fw_st);

	....

	fw_loading_wait(pcu->fw_st);

Thanks.

-- 
Dmitry

Re: [RFC v0 7/8] Input: ims-pcu: use firmware_stat instead of completion

[Bjorn Andersson]

On Thu 28 Jul 11:33 PDT 2016, Dmitry Torokhov wrote:

> On Thu, Jul 28, 2016 at 09:55:11AM +0200, Daniel Wagner wrote:
> > From: Daniel Wagner <daniel.wagner@bmw-carit.de>
> > 
[..]
> 
> Do not quite like it... I'd rather asynchronous request give out a
> firmware status pointer that could be used later on.
> 
> 	pcu->fw_st = request_firmware_async(IMS_PCU_FIRMWARE_NAME,
> 					    pcu,
> 					    ims_pcu_process_async_firmware);
> 	if (IS_ERR(pcu->fw_st))
> 		return PTR_ERR(pcu->fw_st);
> 
> 	....
> 
> 	fw_loading_wait(pcu->fw_st);
> 

In the remoteproc case (patch 6) this would clean up the code, rather
than replacing the completion API 1 to 1. I like it!

Regards,
Bjorn

Re: [RFC v0 7/8] Input: ims-pcu: use firmware_stat instead of completion

[Daniel Wagner]

On 07/28/2016 09:01 PM, Bjorn Andersson wrote:
> On Thu 28 Jul 11:33 PDT 2016, Dmitry Torokhov wrote:
>
>> On Thu, Jul 28, 2016 at 09:55:11AM +0200, Daniel Wagner wrote:
>>> From: Daniel Wagner <daniel.wagner-98C5kh4wR6ohFhg+JK9F0w@public.gmane.org>
>>>
> [..]
>>
>> Do not quite like it... I'd rather asynchronous request give out a
>> firmware status pointer that could be used later on.
>>
>> 	pcu->fw_st = request_firmware_async(IMS_PCU_FIRMWARE_NAME,
>> 					    pcu,
>> 					    ims_pcu_process_async_firmware);
>> 	if (IS_ERR(pcu->fw_st))
>> 		return PTR_ERR(pcu->fw_st);
>>
>> 	....
>>
>> 	fw_loading_wait(pcu->fw_st);
>>
>
> In the remoteproc case (patch 6) this would clean up the code, rather
> than replacing the completion API 1 to 1. I like it!

IIRC most drivers do it the same way. So request_firmware_async() indeed 
would be good thing to have. Let me try that.

Thanks for the excellent feedback.

cheers,
daniel
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC v0 7/8] Input: ims-pcu: use firmware_stat instead of completion

[Arend van Spriel]

+ Luis (again) ;-)

On 29-07-16 08:13, Daniel Wagner wrote:
> On 07/28/2016 09:01 PM, Bjorn Andersson wrote:
>> On Thu 28 Jul 11:33 PDT 2016, Dmitry Torokhov wrote:
>>
>>> On Thu, Jul 28, 2016 at 09:55:11AM +0200, Daniel Wagner wrote:
>>>> From: Daniel Wagner <daniel.wagner@bmw-carit.de>
>>>>
>> [..]
>>>
>>> Do not quite like it... I'd rather asynchronous request give out a
>>> firmware status pointer that could be used later on.

Excellent. Why not get rid of the callback function as well and have
fw_loading_wait() return result (0 = firmware available, < 0 = fail).
Just to confirm, you are proposing a new API function next to
request_firmware_nowait(), right?

>>>     pcu->fw_st = request_firmware_async(IMS_PCU_FIRMWARE_NAME,
>>> -                       pcu,
>>> -                       ims_pcu_process_async_firmware);
    +			    pcu);
>>>     if (IS_ERR(pcu->fw_st))
>>>         return PTR_ERR(pcu->fw_st);
>>>
>>>     ....
>>>
>>>     err = fw_loading_wait(pcu->fw_st);
	if (err)
		return err;

	fw = fwstat_get_firmware(pcu->fw_st);

Or whatever consistent prefix it is going to be.

>>>
>>
>> In the remoteproc case (patch 6) this would clean up the code, rather
>> than replacing the completion API 1 to 1. I like it!
> 
> IIRC most drivers do it the same way. So request_firmware_async() indeed
> would be good thing to have. Let me try that.

While the idea behind this series is a good one I am wondering about the
need for these drivers to use the asynchronous API. The historic reason
might be to avoid timeout caused by user-mode helper, but that may no
longer apply and these drivers could be better off using
request_firmware_direct().

There have been numerous discussions about the firmware API. Here most
recent one:

http://www.spinics.net/lists/linux-wireless/index.html#152755

Regards,
Arend

> Thanks for the excellent feedback.
> 
> cheers,
> daniel
> -- 
> To unsubscribe from this list: send the line "unsubscribe
> linux-wireless" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC v0 7/8] Input: ims-pcu: use firmware_stat instead of completion

[Luis R. Rodriguez]

On Sat, Jul 30, 2016 at 02:42:41PM +0200, Arend van Spriel wrote:
> + Luis (again) ;-)
> 
> On 29-07-16 08:13, Daniel Wagner wrote:
> > On 07/28/2016 09:01 PM, Bjorn Andersson wrote:
> >> On Thu 28 Jul 11:33 PDT 2016, Dmitry Torokhov wrote:
> >>
> >>> On Thu, Jul 28, 2016 at 09:55:11AM +0200, Daniel Wagner wrote:
> >>>> From: Daniel Wagner <daniel.wagner@bmw-carit.de>
> >>>>
> >> [..]
> >>>
> >>> Do not quite like it... I'd rather asynchronous request give out a
> >>> firmware status pointer that could be used later on.
> 
> Excellent. Why not get rid of the callback function as well and have
> fw_loading_wait() return result (0 = firmware available, < 0 = fail).
> Just to confirm, you are proposing a new API function next to
> request_firmware_nowait(), right?

If proposing new firmware_class patches please bounce / Cc me, I've
recently asked for me to be added to MAINTAINERS so I get these
e-mails as I'm working on a new flexible API which would allow us
to extend the firmware API without having to care about the old
stupid usermode helper at all.

> >>>     pcu->fw_st = request_firmware_async(IMS_PCU_FIRMWARE_NAME,
> >>> -                       pcu,
> >>> -                       ims_pcu_process_async_firmware);
>     +			    pcu);
> >>>     if (IS_ERR(pcu->fw_st))
> >>>         return PTR_ERR(pcu->fw_st);
> >>>
> >>>     ....
> >>>
> >>>     err = fw_loading_wait(pcu->fw_st);
> 	if (err)
> 		return err;
> 
> 	fw = fwstat_get_firmware(pcu->fw_st);
> 
> Or whatever consistent prefix it is going to be.
> 
> >>>
> >>
> >> In the remoteproc case (patch 6) this would clean up the code, rather
> >> than replacing the completion API 1 to 1. I like it!
> > 
> > IIRC most drivers do it the same way. So request_firmware_async() indeed
> > would be good thing to have. Let me try that.
> 
> While the idea behind this series is a good one I am wondering about the
> need for these drivers to use the asynchronous API. The historic reason
> might be to avoid timeout caused by user-mode helper, but that may no
> longer apply and these drivers could be better off using
> request_firmware_direct().

BTW I have in my queue for the sysdata API something like firmware_request_direct()
but with async support. The only thing left to do I think is just add the devm
helpers so drivers no longer need to worry about the release of the firmware.

> There have been numerous discussions about the firmware API. Here most
> recent one:
> 
> http://www.spinics.net/lists/linux-wireless/index.html#152755

And more importantly, the sysdata API queue:

https://git.kernel.org/cgit/linux/kernel/git/mcgrof/linux-next.git/log/?h=20160616-sysdata-v2

  Luis

Re: [RFC v0 7/8] Input: ims-pcu: use firmware_stat instead of completion

[Dmitry Torokhov]

On July 30, 2016 9:58:17 AM PDT, "Luis R. Rodriguez" <mcgrof@kernel.org> wrote:
>On Sat, Jul 30, 2016 at 02:42:41PM +0200, Arend van Spriel wrote:
>> + Luis (again) ;-)
>> 
>> On 29-07-16 08:13, Daniel Wagner wrote:
>> > On 07/28/2016 09:01 PM, Bjorn Andersson wrote:
>> >> On Thu 28 Jul 11:33 PDT 2016, Dmitry Torokhov wrote:
>> >>
>> >>> On Thu, Jul 28, 2016 at 09:55:11AM +0200, Daniel Wagner wrote:
>> >>>> From: Daniel Wagner <daniel.wagner@bmw-carit.de>
>> >>>>
>> >> [..]
>> >>>
>> >>> Do not quite like it... I'd rather asynchronous request give out
>a
>> >>> firmware status pointer that could be used later on.
>> 
>> Excellent. Why not get rid of the callback function as well and have
>> fw_loading_wait() return result (0 = firmware available, < 0 = fail).
>> Just to confirm, you are proposing a new API function next to
>> request_firmware_nowait(), right?
>
>If proposing new firmware_class patches please bounce / Cc me, I've
>recently asked for me to be added to MAINTAINERS so I get these
>e-mails as I'm working on a new flexible API which would allow us
>to extend the firmware API without having to care about the old
>stupid usermode helper at all.

I am not sure why we started calling usermode helper "stupid". We only had to implement direct kernel firmware loading because udev/stsremd folks had "interesting" ideas how events should be handled; but having userspace to feed us data is not stupid.

If we want to overhaul firmware loading support we need to figure out how to support case when a driver want to [asynchronously] request firmware/config/blob and the rest of the system is not ready. Even if we want kernel to do read/load the data we need userspace to tell kernel when firmware partition is available, until then the kernel should not fail the request.


Thanks.

-- 
Dmitry

Re: [RFC v0 7/8] Input: ims-pcu: use firmware_stat instead of completion

[Daniel Wagner]

On 07/31/2016 09:23 AM, Dmitry Torokhov wrote:
> On July 30, 2016 9:58:17 AM PDT, "Luis R. Rodriguez" <mcgrof-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:
>> On Sat, Jul 30, 2016 at 02:42:41PM +0200, Arend van Spriel wrote:
>>> On 29-07-16 08:13, Daniel Wagner wrote:
>>>> On 07/28/2016 09:01 PM, Bjorn Andersson wrote:
>>>>> On Thu 28 Jul 11:33 PDT 2016, Dmitry Torokhov wrote:

>>> + Luis (again) ;-)

That was not on purpose :) My attempt to keep the Cc list a bit shorter 
was a failure.

>>>>>> Do not quite like it... I'd rather asynchronous request give out
>>>>>> firmware status pointer that could be used later on.
>>>
>>> Excellent. Why not get rid of the callback function as well and have
>>> fw_loading_wait() return result (0 = firmware available, < 0 = fail).
>>> Just to confirm, you are proposing a new API function next to
>>> request_firmware_nowait(), right?
>>
>> If proposing new firmware_class patches please bounce / Cc me, I've
>> recently asked for me to be added to MAINTAINERS so I get these
>> e-mails as I'm working on a new flexible API which would allow us
>> to extend the firmware API without having to care about the old
>> stupid usermode helper at all.

These patches here are a first attempt to clean up a bit of the code 
around the completion API. As Dmitry correctly pointed out, it makes 
more sense to go bit further and make the async loading a bit more 
convenient for the drivers.

> I am not sure why we started calling usermode helper "stupid". We
> only  had to implement direct kernel firmware loading because udev/stsremd
> folks had "interesting" ideas how events should be handled; but having
> userspace to feed us data is not stupid.

I was ignorant on all the nasty details around the firmware loading. If 
I parse Luis' patches correctly they introduce an API which calls 
kernel_read_file_from_path() asynchronously:

sysdata_file_request_async(..., &cookie)
   *coookie = async_schedule_domain(request_sysdata_file_work_func(), ..)

   request_sysdata_file_work_fun()
     _sysdata_file_request()
       fw_get_filesystem_firmware()
	kernel_read_file_from_path()

sysdata_synchronize_request(&cookie);

Doesn't look like what your asking for.

> If we want to overhaul firmware loading support we need to figure
> out  how to support case when a driver want to [asynchronously] request
> firmware/config/blob and the rest of the system is not ready. Even if we
> want kernel to do read/load the data we need userspace to tell kernel
> when firmware partition is available, until then the kernel should not
> fail the request.

I gather from Luis' blog post and comments that he is on the quest on 
removing userspace support completely.

Maybe this attempt here could be a step before. Step 1 would be changing 
request_firmware_nowait() to request_firmware_async so drivers don't 
have to come up with their own sync primitives, e.g.

   cookie = request_firmware_async()
   fw_load_wait(cookie)

Step 2 could be something more towards sysdata approach.

cheers,
daniel
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC v0 7/8] Input: ims-pcu: use firmware_stat instead of completion

[Luis R. Rodriguez]

On Mon, Aug 01, 2016 at 02:26:04PM +0200, Daniel Wagner wrote:
> On 07/31/2016 09:23 AM, Dmitry Torokhov wrote:
> >On July 30, 2016 9:58:17 AM PDT, "Luis R. Rodriguez" <mcgrof-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:
> >>On Sat, Jul 30, 2016 at 02:42:41PM +0200, Arend van Spriel wrote:
> >>>On 29-07-16 08:13, Daniel Wagner wrote:
> >>>>On 07/28/2016 09:01 PM, Bjorn Andersson wrote:
> >>>>>On Thu 28 Jul 11:33 PDT 2016, Dmitry Torokhov wrote:
> 
> >>>+ Luis (again) ;-)
> 
> That was not on purpose :) My attempt to keep the Cc list a bit
> shorter was a failure.
> 
> >>>>>>Do not quite like it... I'd rather asynchronous request give out
> >>>>>>firmware status pointer that could be used later on.
> >>>
> >>>Excellent. Why not get rid of the callback function as well and have
> >>>fw_loading_wait() return result (0 = firmware available, < 0 = fail).
> >>>Just to confirm, you are proposing a new API function next to
> >>>request_firmware_nowait(), right?
> >>
> >>If proposing new firmware_class patches please bounce / Cc me, I've
> >>recently asked for me to be added to MAINTAINERS so I get these
> >>e-mails as I'm working on a new flexible API which would allow us
> >>to extend the firmware API without having to care about the old
> >>stupid usermode helper at all.
> 
> These patches here are a first attempt to clean up a bit of the code
> around the completion API. As Dmitry correctly pointed out, it makes
> more sense to go bit further and make the async loading a bit more
> convenient for the drivers.
> 
> >I am not sure why we started calling usermode helper "stupid". We
> >only  had to implement direct kernel firmware loading because udev/stsremd
> >folks had "interesting" ideas how events should be handled; but having
> >userspace to feed us data is not stupid.
> 
> I was ignorant on all the nasty details around the firmware loading.
> If I parse Luis' patches correctly they introduce an API which calls
> kernel_read_file_from_path() asynchronously:
> 
> sysdata_file_request_async(..., &cookie)
>   *coookie = async_schedule_domain(request_sysdata_file_work_func(), ..)
> 
>   request_sysdata_file_work_fun()
>     _sysdata_file_request()
>       fw_get_filesystem_firmware()
> 	kernel_read_file_from_path()
> 
> sysdata_synchronize_request(&cookie);
> 
> Doesn't look like what your asking for.

No, but its also a generic kernel read issue as I noted in my last
reply.

> >If we want to overhaul firmware loading support we need to figure
> >out  how to support case when a driver want to [asynchronously] request
> >firmware/config/blob and the rest of the system is not ready. Even if we
> >want kernel to do read/load the data we need userspace to tell kernel
> >when firmware partition is available, until then the kernel should not
> >fail the request.
> 
> I gather from Luis' blog post and comments that he is on the quest
> on removing userspace support completely.

No, I explained in my last proposed documentation patch series that we cannot
get rid of the usermode helper. Its not well understood why so I explained and
documented why. Best we can do is compartamentalize its uses.

The sysdata API's main goal rather is to provide a flexible API first,
compartamentalizing the usermode helper was secondary. But now it seems
I may just also add devm support too to help simplify code further.

What Dmitry notes is an existential issue with kernel_read_file_from_path()
and we need a common solution for it.

> Maybe this attempt here could be a step before. Step 1 would be
> changing request_firmware_nowait() to request_firmware_async so
> drivers don't have to come up with their own sync primitives, e.g.
> 
>   cookie = request_firmware_async()
>   fw_load_wait(cookie)

That's one of the features already part of async mechanism of the sysdata API :)

  Luis
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC v0 7/8] Input: ims-pcu: use firmware_stat instead of completion

[Daniel Wagner]

Hi Luis,

>> I was ignorant on all the nasty details around the firmware loading.
>> If I parse Luis' patches correctly they introduce an API which calls
>> kernel_read_file_from_path() asynchronously:
>>
>> sysdata_file_request_async(..., &cookie)
>>   *coookie = async_schedule_domain(request_sysdata_file_work_func(), ..)
>>
>>   request_sysdata_file_work_fun()
>>     _sysdata_file_request()
>>       fw_get_filesystem_firmware()
>> 	kernel_read_file_from_path()
>>
>> sysdata_synchronize_request(&cookie);
>>
>> Doesn't look like what your asking for.
>
> No, but its also a generic kernel read issue as I noted in my last
> reply.

Okay, got it.

>>> If we want to overhaul firmware loading support we need to figure
>>> out  how to support case when a driver want to [asynchronously] request
>>> firmware/config/blob and the rest of the system is not ready. Even if we
>>> want kernel to do read/load the data we need userspace to tell kernel
>>> when firmware partition is available, until then the kernel should not
>>> fail the request.
>>
>> I gather from Luis' blog post and comments that he is on the quest
>> on removing userspace support completely.
>
> No, I explained in my last proposed documentation patch series that we cannot
> get rid of the usermode helper.

I stand corrected.

> Its not well understood why so I explained and documented why.

Obviously, I got lost somewhere there :)

> Best we can do is compartamentalize its uses.

Sounds like a plan.

> The sysdata API's main goal rather is to provide a flexible API first,
> compartamentalizing the usermode helper was secondary. But now it seems
> I may just also add devm support too to help simplify code further.

I missed the point that you plan to add usermode helper support to the 
sysdata API.

> What Dmitry notes is an existential issue with kernel_read_file_from_path()
> and we need a common solution for it.

Understood. I guess best thing to keep that discussion in the other 
subthread.

>> Maybe this attempt here could be a step before. Step 1 would be
>> changing request_firmware_nowait() to request_firmware_async so
>> drivers don't have to come up with their own sync primitives, e.g.
>>
>>   cookie = request_firmware_async()
>>   fw_load_wait(cookie)
>
> That's one of the features already part of async mechanism of the sysdata API :)

Yes, I realized that too :)

cheers,
daniel

Thanks for the feedback.

Re: [RFC v0 7/8] Input: ims-pcu: use firmware_stat instead of completion

[Luis R. Rodriguez]

On Tue, Aug 02, 2016 at 07:49:19AM +0200, Daniel Wagner wrote:
> >The sysdata API's main goal rather is to provide a flexible API first,
> >compartamentalizing the usermode helper was secondary. But now it seems
> >I may just also add devm support too to help simplify code further.
> 
> I missed the point that you plan to add usermode helper support to
> the sysdata API.

I had no such plans, when I have asked folks so far about "hey are you
really in need for it, OK what for? " and "what extended uses do you
envision?" so I far I have not gotten any replies at all. So -- instead
sysdata currently ignores it.

  Luis

Re: [RFC v0 7/8] Input: ims-pcu: use firmware_stat instead of completion

[Daniel Wagner]

On 08/02/2016 08:34 AM, Luis R. Rodriguez wrote:
> On Tue, Aug 02, 2016 at 07:49:19AM +0200, Daniel Wagner wrote:
>>> The sysdata API's main goal rather is to provide a flexible API first,
>>> compartamentalizing the usermode helper was secondary. But now it seems
>>> I may just also add devm support too to help simplify code further.
>>
>> I missed the point that you plan to add usermode helper support to
>> the sysdata API.
>
> I had no such plans, when I have asked folks so far about "hey are you
> really in need for it, OK what for? " and "what extended uses do you
> envision?" so I far I have not gotten any replies at all. So -- instead
> sysdata currently ignores it.

So you argue for the remoteproc use case with 100+ MB firmware that if 
there is a way to load after pivot_root() (or other additional firmware 
partition shows up) then there is no need at all for usermode helper?
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC v0 7/8] Input: ims-pcu: use firmware_stat instead of completion

[Luis R. Rodriguez]

On Tue, Aug 02, 2016 at 08:53:55AM +0200, Daniel Wagner wrote:
> On 08/02/2016 08:34 AM, Luis R. Rodriguez wrote:
> >On Tue, Aug 02, 2016 at 07:49:19AM +0200, Daniel Wagner wrote:
> >>>The sysdata API's main goal rather is to provide a flexible API first,
> >>>compartamentalizing the usermode helper was secondary. But now it seems
> >>>I may just also add devm support too to help simplify code further.
> >>
> >>I missed the point that you plan to add usermode helper support to
> >>the sysdata API.
> >
> >I had no such plans, when I have asked folks so far about "hey are you
> >really in need for it, OK what for? " and "what extended uses do you
> >envision?" so I far I have not gotten any replies at all. So -- instead
> >sysdata currently ignores it.
> 
> So you argue for the remoteproc use case with 100+ MB firmware that
> if there is a way to load after pivot_root() (or other additional
> firmware partition shows up) then there is no need at all for
> usermode helper?

No, I'm saying I'd like to hear valid uses cases for the usermode helper and so
far I have only found using coccinelle grammar 2 explicit users, that's it. My
patch series (not yet merge) then annotates these as valid as I've verified
through their documentation they have some quirky requirement.

Other than these two drivers I'd like hear to valid requirements for it.

The existential issue is a real issue but it does not look impossible to
resolve. It may be a solution to bloat up the kernel with 100+ MB size just to
stuff built-in firmware to avoid this issue, but it does not mean a solution
is not possible.

Remind me -- why can remoteproc not stuff the firmware in initramfs ?

Anyway, here's a simple suggestion: fs/exec.c gets a sentinel file monitor
support per enum kernel_read_file_id. For instance we'd have one for
READING_FIRMWARE, one for READING_KEXEC_IMAGE, perhaps READING_POLICY, and this
would in turn be used as the system configurable deterministic file for
which to wait for to be present before enabling each enum kernel_read_file_id
type read.

Thoughts ?

  Luis
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC v0 7/8] Input: ims-pcu: use firmware_stat instead of completion

[Daniel Wagner]

On 08/02/2016 09:41 AM, Luis R. Rodriguez wrote:
> On Tue, Aug 02, 2016 at 08:53:55AM +0200, Daniel Wagner wrote:
>> On 08/02/2016 08:34 AM, Luis R. Rodriguez wrote:
>>> On Tue, Aug 02, 2016 at 07:49:19AM +0200, Daniel Wagner wrote:
>> So you argue for the remoteproc use case with 100+ MB firmware that
>> if there is a way to load after pivot_root() (or other additional
>> firmware partition shows up) then there is no need at all for
>> usermode helper?
>
> No, I'm saying I'd like to hear valid uses cases for the usermode helper and so
> far I have only found using coccinelle grammar 2 explicit users, that's it. My
> patch series (not yet merge) then annotates these as valid as I've verified
> through their documentation they have some quirky requirement.

I got that question wrong. It should read something like 'for the 
remoteproc 100+MB there is no need for the user help?'. I've gone 
through your patches and they make perfectly sense too. Maybe I can 
convince you to take a better version of my patch 3 into your queue. And 
I help you converting the exiting drivers. Obviously if you like my help 
at all.

> Other than these two drivers I'd like hear to valid requirements for it.
>
> The existential issue is a real issue but it does not look impossible to
> resolve. It may be a solution to bloat up the kernel with 100+ MB size just to
> stuff built-in firmware to avoid this issue, but it does not mean a solution
> is not possible.
>
> Remind me -- why can remoteproc not stuff the firmware in initramfs ?

I don't know. I was just bringing it up with the hope that Bjorn will 
defend it. It seems my tactics didn't work out :)

> Anyway, here's a simple suggestion: fs/exec.c gets a sentinel file monitor
> support per enum kernel_read_file_id. For instance we'd have one for
> READING_FIRMWARE, one for READING_KEXEC_IMAGE, perhaps READING_POLICY, and this
> would in turn be used as the system configurable deterministic file for
> which to wait for to be present before enabling each enum kernel_read_file_id
> type read.
>
> Thoughts ?

Not sure if I get you here correctly. Is the 'system configurable 
deterministic file' is a knob which controlled by user space? Or it this 
something you define at compile time?

Hmm, so it would allow to decided to ask a userspace helper or load the 
firmware directly (to be more precised the kernel_read_file_id type). If 
yes, than it is what currently already have just integrated nicely into 
the new sysdata API.

cheers,
daniel

Re: [RFC v0 7/8] Input: ims-pcu: use firmware_stat instead of completion

[Luis R. Rodriguez]

On Wed, Aug 03, 2016 at 08:57:09AM +0200, Daniel Wagner wrote:
> On 08/02/2016 09:41 AM, Luis R. Rodriguez wrote:
> >On Tue, Aug 02, 2016 at 08:53:55AM +0200, Daniel Wagner wrote:
> >>On 08/02/2016 08:34 AM, Luis R. Rodriguez wrote:
> >>>On Tue, Aug 02, 2016 at 07:49:19AM +0200, Daniel Wagner wrote:
> >>So you argue for the remoteproc use case with 100+ MB firmware that
> >>if there is a way to load after pivot_root() (or other additional
> >>firmware partition shows up) then there is no need at all for
> >>usermode helper?
> >
> >No, I'm saying I'd like to hear valid uses cases for the usermode helper and so
> >far I have only found using coccinelle grammar 2 explicit users, that's it. My
> >patch series (not yet merge) then annotates these as valid as I've verified
> >through their documentation they have some quirky requirement.
> 
> I got that question wrong. It should read something like 'for the
> remoteproc 100+MB there is no need for the user help?'.

That's not a question for me but for those who say that the usermode helper
is needed for remoteproc, so far from what folks are saying it seems the only
reason for the usermodehelper was to try to avoid the deterministic issue,
but I suggested a way to resolve that without the usermode helper now so
would be curious to hear if there are any more reasons for it.

> I've gone
> through your patches and they make perfectly sense too. Maybe I can
> convince you to take a better version of my patch 3 into your queue.
> And I help you converting the exiting drivers. Obviously if you like
> my help at all.

I accept all help and would be glad to make enhancements instead of
the old API through new API. The biggest thing here first I think is
adding devm support, that I think should address what seemed to be
the need to add more code for a transformation into the API. I'd
personally only want to add that and be done with an introduction
of the sysdata API. Further changes IMHO are best done atomically
after that on top of it, but I'm happy to queue in the changes.

> >Other than these two drivers I'd like hear to valid requirements for it.
> >
> >The existential issue is a real issue but it does not look impossible to
> >resolve. It may be a solution to bloat up the kernel with 100+ MB size just to
> >stuff built-in firmware to avoid this issue, but it does not mean a solution
> >is not possible.
> >
> >Remind me -- why can remoteproc not stuff the firmware in initramfs ?
> 
> I don't know. I was just bringing it up with the hope that Bjorn
> will defend it. It seems my tactics didn't work out :)

OK.

> >Anyway, here's a simple suggestion: fs/exec.c gets a sentinel file monitor
> >support per enum kernel_read_file_id. For instance we'd have one for
> >READING_FIRMWARE, one for READING_KEXEC_IMAGE, perhaps READING_POLICY, and this
> >would in turn be used as the system configurable deterministic file for
> >which to wait for to be present before enabling each enum kernel_read_file_id
> >type read.
> >
> >Thoughts ?
> 
> Not sure if I get you here correctly. Is the 'system configurable
> deterministic file' is a knob which controlled by user space? Or it
> this something you define at compile time?

I meant at compile time on the kernel. So CONFIG_READ_READY_SENTINEL
or something like this, and it be a string, which if set then when
the kernel read APIs are used, then a new API could be introduced
that would *only* enable reading through once that sentinel has
been detected by the kernel to allowed through reads. Doing this
per mount / target filesystem is rather cumbersome given possible
overlaps in mounts and also pivot_root() being possible, so instead
targeting simply the fs/exec.c enum kernel_read_file_id would seem
more efficient and clean but we would need a decided upon set of
paths per enum kernel_read_file_id as base (or just one path per
enum kernel_read_file_id). For number of paths I mean the number
of target directories to look for the sentinel per enum kernel_read_file_id,
so for instance for READING_FIRMWARE perhaps just deciding on /lib/firmware/
would suffice, but if this supported multiple paths another option may be
for the sentinel to also be looked for in /lib/firmware/updates/,
/lib/firmware/" UTS_RELEASE -- etc. It would *stop* after finding one
sentinel on any of these paths.

If a system has has CONFIG_READ_READY_SENTINEL it would mean an agreed upon
system configuration has been decided so that at any point in time reads
against READING_FIRMWARE using a new kernel_read_file_from_path_sentinel()
(or something like it) would only allow the read to go through once
the sentinel has been found for READING_FIRMWARE on the agreed upon
paths.

The benefit of the sentintel approach is it avoids complexities with
pivot_root(), and makes the deterministic aspect of the target left
only to a system-configuration enabled target path / file.

This is just an idea. I'd like some FS folks to review.

> Hmm, so it would allow to decided to ask a userspace helper or load
> the firmware directly (to be more precised the kernel_read_file_id
> type). If yes, than it is what currently already have just
> integrated nicely into the new sysdata API.

Sorry, no, the above description is better of what I meant. This
actually would not need to go into the sysdata API, unless of
course we wanted it just as a new "feature" of it, but I don't
think that's needed unless it has some implications behind the
scenes. Given that firmware_class now uses a common core kernel
API for reading files kernel_read_file_from_path() we could
for instance add kernel_read_file_from_sentintel() and only
if CONFIG_READ_READY_SENTINEL() would it block and wait until
the sentinel clears. This should mean being able to make the
change for both the old API and the new proposed sysdata API.
Likewise for other kernel_read_file*() users -- they'd benefit
from it as well.

  Luis
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC v0 7/8] Input: ims-pcu: use firmware_stat instead of completion

[Dmitry Torokhov]

On Wed, Aug 03, 2016 at 05:55:40PM +0200, Luis R. Rodriguez wrote:
> 
> I accept all help and would be glad to make enhancements instead of
> the old API through new API. The biggest thing here first I think is
> adding devm support, that I think should address what seemed to be
> the need to add more code for a transformation into the API. I'd

I am confused. Why do we need devm support, given that devm is only
valid in probe() paths[*] and we do know that we do not want to load
firmware in probe() paths because it may cause blocking?

[*] Yes, I know there are calls to devm* outside of probe() but I am
pretty sure they are buggy unless they explicitly freed with devm* as
well and then there is no point. IN all other cases it is likely wrong
as it messes up with order of freeing resources.

Thanks.

-- 
Dmitry

Re: [RFC v0 7/8] Input: ims-pcu: use firmware_stat instead of completion

[Luis R. Rodriguez]

On Wed, Aug 03, 2016 at 09:18:21AM -0700, Dmitry Torokhov wrote:
> On Wed, Aug 03, 2016 at 05:55:40PM +0200, Luis R. Rodriguez wrote:
> > 
> > I accept all help and would be glad to make enhancements instead of
> > the old API through new API. The biggest thing here first I think is
> > adding devm support, that I think should address what seemed to be
> > the need to add more code for a transformation into the API. I'd
> 
> I am confused. Why do we need devm support, given that devm is only
> valid in probe() paths[*] and we do know that we do not want to load
> firmware in probe() paths because it may cause blocking?

Its a good point, I hadn't gone on to implement devm support on the sysdata API
yet here so this requirement was not known to me. This certainly would put a
limitation to the idea of using devm then to deal with the firmware for you,
given that not all users of firmware are on probe, and as you note we want to
by default avoid firmware calls on probe since init+probe are called serially
by default unless a driver is using the new async probe. Nevertheless, even if
we had userspace or the driver always asking for async probe, most users of the
firmware API are not on probe anyway, so the gains of using devm to help with
freeing the firmware for the driver on probe would be very limited.

With that in mind, in retrospect then the current sysdata approach to require a
callback for synchronous calls would seem to work around this issue and
generalize a solution given we'd have:

For the sync case:

const struct sysdata_file_desc sysdata_desc = {
	SYSDATA_DEFAULT_SYNC(driver_sync_req_cb, dev),
	.keep = false, /* not explicitly needed as default is false */
};
ret = sysdata_file_request();
...

Behind the scenes firmware_class would call driver_sync_req_cb(),
since that's where we know the firmware will be consumed and since
the driver has explicitly asked that it no longer needs to keep the
firmware around (keep == false), it will free it on behalf of the
driver.

Since current synchronous calls for firmware do not have a callback
this would mean a driver changing to the sysdata API if it wanted
to take advantage of this feature of letting firmware_class free
the firmware for you, you'd need a bit more code than before.

For the asynchronous case this is a bit different given that the
current async firmware API requires a callback, so if keep == false
on the async sysdata API we just remove the release_firmware()
calls when converting over.

Given this, other than the bikeshedding aspects [0] ("sysdata", "driver data",
"firmware), perhaps the sysdata API is done then.

[0] http://phk.freebsd.dk/sagas/bikeshed.html

> [*] Yes, I know there are calls to devm* outside of probe() but I am
> pretty sure they are buggy unless they explicitly freed with devm* as
> well and then there is no point.

Really ? If so that's good to know.. and it should mean grammar could
be used to hunt this down, specially since we have now some grammar
basics to help us check for calls on probe or init. On the grammar
we'd just only complain if a call was used not in a probe path.

> IN all other cases it is likely wrong
> as it messes up with order of freeing resources.

Good to know, thanks. Hopefully the above semantics of the driver
using keep should suffice. Which gets me to think, what if devm
had something similar to white-list uses outside of probe so that
if a keep (or another flag name) was set then its vetting that
the order of freeing of resources is understood and fine.

  Luis

Re: [RFC v0 7/8] Input: ims-pcu: use firmware_stat instead of completion

[Luis R. Rodriguez]

On Wed, Aug 03, 2016 at 05:55:40PM +0200, Luis R. Rodriguez wrote:
> On Wed, Aug 03, 2016 at 08:57:09AM +0200, Daniel Wagner wrote:
> > On 08/02/2016 09:41 AM, Luis R. Rodriguez wrote:
> > >On Tue, Aug 02, 2016 at 08:53:55AM +0200, Daniel Wagner wrote:
> > >>On 08/02/2016 08:34 AM, Luis R. Rodriguez wrote:
> > >>>On Tue, Aug 02, 2016 at 07:49:19AM +0200, Daniel Wagner wrote:
> > >>So you argue for the remoteproc use case with 100+ MB firmware that
> > >>if there is a way to load after pivot_root() (or other additional
> > >>firmware partition shows up) then there is no need at all for
> > >>usermode helper?
> > >
> > >No, I'm saying I'd like to hear valid uses cases for the usermode helper and so
> > >far I have only found using coccinelle grammar 2 explicit users, that's it. My
> > >patch series (not yet merge) then annotates these as valid as I've verified
> > >through their documentation they have some quirky requirement.
> > 
> > I got that question wrong. It should read something like 'for the
> > remoteproc 100+MB there is no need for the user help?'.
> 
> That's not a question for me but for those who say that the usermode helper
> is needed for remoteproc, so far from what folks are saying it seems the only
> reason for the usermodehelper was to try to avoid the deterministic issue,
> but I suggested a way to resolve that without the usermode helper now so
> would be curious to hear if there are any more reasons for it.
> 
> > I've gone
> > through your patches and they make perfectly sense too. Maybe I can
> > convince you to take a better version of my patch 3 into your queue.
> > And I help you converting the exiting drivers. Obviously if you like
> > my help at all.
> 
> I accept all help and would be glad to make enhancements instead of
> the old API through new API. The biggest thing here first I think is
> adding devm support, that I think should address what seemed to be
> the need to add more code for a transformation into the API. I'd
> personally only want to add that and be done with an introduction
> of the sysdata API. Further changes IMHO are best done atomically
> after that on top of it, but I'm happy to queue in the changes.
> 
> > >Other than these two drivers I'd like hear to valid requirements for it.
> > >
> > >The existential issue is a real issue but it does not look impossible to
> > >resolve. It may be a solution to bloat up the kernel with 100+ MB size just to
> > >stuff built-in firmware to avoid this issue, but it does not mean a solution
> > >is not possible.
> > >
> > >Remind me -- why can remoteproc not stuff the firmware in initramfs ?
> > 
> > I don't know. I was just bringing it up with the hope that Bjorn
> > will defend it. It seems my tactics didn't work out :)
> 
> OK.
> 
> > >Anyway, here's a simple suggestion: fs/exec.c gets a sentinel file monitor
> > >support per enum kernel_read_file_id. For instance we'd have one for
> > >READING_FIRMWARE, one for READING_KEXEC_IMAGE, perhaps READING_POLICY, and this
> > >would in turn be used as the system configurable deterministic file for
> > >which to wait for to be present before enabling each enum kernel_read_file_id
> > >type read.
> > >
> > >Thoughts ?
> > 
> > Not sure if I get you here correctly. Is the 'system configurable
> > deterministic file' is a knob which controlled by user space? Or it
> > this something you define at compile time?
> 
> I meant at compile time on the kernel. So CONFIG_READ_READY_SENTINEL
> or something like this, and it be a string, which if set then when
> the kernel read APIs are used, then a new API could be introduced
> that would *only* enable reading through once that sentinel has
> been detected by the kernel to allowed through reads. Doing this
> per mount / target filesystem is rather cumbersome given possible
> overlaps in mounts and also pivot_root() being possible, so instead
> targeting simply the fs/exec.c enum kernel_read_file_id would seem
> more efficient and clean but we would need a decided upon set of
> paths per enum kernel_read_file_id as base (or just one path per
> enum kernel_read_file_id). For number of paths I mean the number
> of target directories to look for the sentinel per enum kernel_read_file_id,
> so for instance for READING_FIRMWARE perhaps just deciding on /lib/firmware/
> would suffice, but if this supported multiple paths another option may be
> for the sentinel to also be looked for in /lib/firmware/updates/,
> /lib/firmware/" UTS_RELEASE -- etc. It would *stop* after finding one
> sentinel on any of these paths.
> 
> If a system has has CONFIG_READ_READY_SENTINEL it would mean an agreed upon
> system configuration has been decided so that at any point in time reads
> against READING_FIRMWARE using a new kernel_read_file_from_path_sentinel()
> (or something like it) would only allow the read to go through once
> the sentinel has been found for READING_FIRMWARE on the agreed upon
> paths.
> 
> The benefit of the sentintel approach is it avoids complexities with
> pivot_root(), and makes the deterministic aspect of the target left
> only to a system-configuration enabled target path / file.
> 
> This is just an idea. I'd like some FS folks to review.
> 
> > Hmm, so it would allow to decided to ask a userspace helper or load
> > the firmware directly (to be more precised the kernel_read_file_id
> > type). If yes, than it is what currently already have just
> > integrated nicely into the new sysdata API.
> 
> Sorry, no, the above description is better of what I meant. This
> actually would not need to go into the sysdata API, unless of
> course we wanted it just as a new "feature" of it, but I don't
> think that's needed unless it has some implications behind the
> scenes. Given that firmware_class now uses a common core kernel
> API for reading files kernel_read_file_from_path() we could
> for instance add kernel_read_file_from_sentintel() and only
> if CONFIG_READ_READY_SENTINEL() would it block and wait until
> the sentinel clears. This should mean being able to make the
> change for both the old API and the new proposed sysdata API.
> Likewise for other kernel_read_file*() users -- they'd benefit
> from it as well.

A file sentinel would implicate a file namespace thing being used on the
filesystem -- to me this just means the Linux distribution / system integrator
would add this per filesystem, but agree this is pretty hacky.  Furthermore
we'd wait forever if the Linux distribution / system integrator forgot to set
the sentinel file. That's not good. To avoid that a generic "root fs ready"
event could be sent from userspace to know when to clear stale reads... but if
that's going to be done best just replace all sentintels with a simple "root fs
ready" which would mean all reads from the kernel are ready. If we wanted
further granularity I suppose we could further have one event per enum
kernel_read_file_id, and a generic all-is-ready one.

To start off with then a simple event from userspace should suffice. But do keep
in mind that granularity might help given that a big iron system might have some
large array of disks to mount during bootup and that may take a long while, and
you likely want to read /lib/firmware way before that filesystem is ready.

Not sure if granularity fixated by enum kernel_read_file_id should suffice,
perhaps given its also enough for LSMs...

This indeed would mean a kernelspace and userspace change, but it would mean 
not having to deal with the usermode helper crap anymore.

Anyway -- these are just ideas, patches welcomed !

  Luis
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC v0 7/8] Input: ims-pcu: use firmware_stat instead of completion

[Bjorn Andersson]

On Wed 03 Aug 08:55 PDT 2016, Luis R. Rodriguez wrote:

> On Wed, Aug 03, 2016 at 08:57:09AM +0200, Daniel Wagner wrote:
> > On 08/02/2016 09:41 AM, Luis R. Rodriguez wrote:
[..]
> > Not sure if I get you here correctly. Is the 'system configurable
> > deterministic file' is a knob which controlled by user space? Or it
> > this something you define at compile time?
> 
> I meant at compile time on the kernel. So CONFIG_READ_READY_SENTINEL
> or something like this, and it be a string, which if set then when
> the kernel read APIs are used, then a new API could be introduced
> that would *only* enable reading through once that sentinel has
> been detected by the kernel to allowed through reads. Doing this
> per mount / target filesystem is rather cumbersome given possible
> overlaps in mounts and also pivot_root() being possible, so instead
> targeting simply the fs/exec.c enum kernel_read_file_id would seem
> more efficient and clean but we would need a decided upon set of
> paths per enum kernel_read_file_id as base (or just one path per
> enum kernel_read_file_id). For number of paths I mean the number
> of target directories to look for the sentinel per enum kernel_read_file_id,
> so for instance for READING_FIRMWARE perhaps just deciding on /lib/firmware/
> would suffice, but if this supported multiple paths another option may be
> for the sentinel to also be looked for in /lib/firmware/updates/,
> /lib/firmware/" UTS_RELEASE -- etc. It would *stop* after finding one
> sentinel on any of these paths.
> 
> If a system has has CONFIG_READ_READY_SENTINEL it would mean an agreed upon
> system configuration has been decided so that at any point in time reads
> against READING_FIRMWARE using a new kernel_read_file_from_path_sentinel()
> (or something like it) would only allow the read to go through once
> the sentinel has been found for READING_FIRMWARE on the agreed upon
> paths.
> 
> The benefit of the sentintel approach is it avoids complexities with
> pivot_root(), and makes the deterministic aspect of the target left
> only to a system-configuration enabled target path / file.
> 

This sounds reasonable, it could be configured to wait for a certain
static file or userspace could generate this file once it reaches some
checkpoint.


Just to provide some additional input to "will rootfs mounted be enough
of a sentinel".

In an Android device you have a initramfs that will read an fstab file
and mount /system that holds most firmware, for some platforms
additional firmware will come from a third partition (in the Qualcomm
case mounted in /persist by the same mechanism).

With the sentinel approach one could configure the system either point
it at a file in the last file system to be mounted or have init generate
a file once its done with this; or in the generic configuration just
wait for /lib/firmware to show up.

I like this approach.

> This is just an idea. I'd like some FS folks to review.
> 

Regards,
Bjorn

Re: [RFC v0 7/8] Input: ims-pcu: use firmware_stat instead of completion

[Dmitry Torokhov]

On Tue, Aug 2, 2016 at 12:41 AM, Luis R. Rodriguez <mcgrof@kernel.org> wrote:
> On Tue, Aug 02, 2016 at 08:53:55AM +0200, Daniel Wagner wrote:
>> On 08/02/2016 08:34 AM, Luis R. Rodriguez wrote:
>> >On Tue, Aug 02, 2016 at 07:49:19AM +0200, Daniel Wagner wrote:
>> >>>The sysdata API's main goal rather is to provide a flexible API first,
>> >>>compartamentalizing the usermode helper was secondary. But now it seems
>> >>>I may just also add devm support too to help simplify code further.
>> >>
>> >>I missed the point that you plan to add usermode helper support to
>> >>the sysdata API.
>> >
>> >I had no such plans, when I have asked folks so far about "hey are you
>> >really in need for it, OK what for? " and "what extended uses do you
>> >envision?" so I far I have not gotten any replies at all. So -- instead
>> >sysdata currently ignores it.
>>
>> So you argue for the remoteproc use case with 100+ MB firmware that
>> if there is a way to load after pivot_root() (or other additional
>> firmware partition shows up) then there is no need at all for
>> usermode helper?
>
> No, I'm saying I'd like to hear valid uses cases for the usermode helper and so
> far I have only found using coccinelle grammar 2 explicit users, that's it. My
> patch series (not yet merge) then annotates these as valid as I've verified
> through their documentation they have some quirky requirement.

In certain configurations (embedded) people do not want to use
initramfs nor modules nor embed firmware into the kernel. In this case
usermode helper + firmware calss timeout handling provides necessary
wait for the root filesystem to be mounted.

If we solve waiting for rootfs (or something else that may contain
firmware) then these cases will not need to use usermode helper.

Thanks.

-- 
Dmitry

Re: [RFC v0 7/8] Input: ims-pcu: use firmware_stat instead of completion

[Arend van Spriel]

On 03-08-16 09:42, Dmitry Torokhov wrote:
> On Tue, Aug 2, 2016 at 12:41 AM, Luis R. Rodriguez <mcgrof@kernel.org> wrote:
>> On Tue, Aug 02, 2016 at 08:53:55AM +0200, Daniel Wagner wrote:
>>> On 08/02/2016 08:34 AM, Luis R. Rodriguez wrote:
>>>> On Tue, Aug 02, 2016 at 07:49:19AM +0200, Daniel Wagner wrote:
>>>>>> The sysdata API's main goal rather is to provide a flexible API first,
>>>>>> compartamentalizing the usermode helper was secondary. But now it seems
>>>>>> I may just also add devm support too to help simplify code further.
>>>>>
>>>>> I missed the point that you plan to add usermode helper support to
>>>>> the sysdata API.
>>>>
>>>> I had no such plans, when I have asked folks so far about "hey are you
>>>> really in need for it, OK what for? " and "what extended uses do you
>>>> envision?" so I far I have not gotten any replies at all. So -- instead
>>>> sysdata currently ignores it.
>>>
>>> So you argue for the remoteproc use case with 100+ MB firmware that
>>> if there is a way to load after pivot_root() (or other additional
>>> firmware partition shows up) then there is no need at all for
>>> usermode helper?
>>
>> No, I'm saying I'd like to hear valid uses cases for the usermode helper and so
>> far I have only found using coccinelle grammar 2 explicit users, that's it. My
>> patch series (not yet merge) then annotates these as valid as I've verified
>> through their documentation they have some quirky requirement.
> 
> In certain configurations (embedded) people do not want to use
> initramfs nor modules nor embed firmware into the kernel. In this case
> usermode helper + firmware calss timeout handling provides necessary
> wait for the root filesystem to be mounted.

And there are people who don't have a usermode helper running at all in
their configuration, but I guess they should disable the helper.

In my opinion the kernel should provide functionality to user-space and
user-space providing functionality to the kernel should be avoided.

> If we solve waiting for rootfs (or something else that may contain
> firmware) then these cases will not need to use usermode helper.

If firmware (or whatever) API could get notification of mount syscall it
could be used to retry firmware loading instead of periodic polling.
That leaves the question raised by you about when to stop trying. The
initlevel stuff is probably a user-space only concept, right? So no
ideas how the kernel itself could decide except for a "long" timeout.

Regards,
Arend

Re: [RFC v0 7/8] Input: ims-pcu: use firmware_stat instead of completion

[Luis R. Rodriguez]

On Wed, Aug 03, 2016 at 01:43:31PM +0200, Arend van Spriel wrote:
> On 03-08-16 09:42, Dmitry Torokhov wrote:
> > On Tue, Aug 2, 2016 at 12:41 AM, Luis R. Rodriguez <mcgrof@kernel.org> wrote:
> >> On Tue, Aug 02, 2016 at 08:53:55AM +0200, Daniel Wagner wrote:
> >>> On 08/02/2016 08:34 AM, Luis R. Rodriguez wrote:
> >>>> On Tue, Aug 02, 2016 at 07:49:19AM +0200, Daniel Wagner wrote:
> >>>>>> The sysdata API's main goal rather is to provide a flexible API first,
> >>>>>> compartamentalizing the usermode helper was secondary. But now it seems
> >>>>>> I may just also add devm support too to help simplify code further.
> >>>>>
> >>>>> I missed the point that you plan to add usermode helper support to
> >>>>> the sysdata API.
> >>>>
> >>>> I had no such plans, when I have asked folks so far about "hey are you
> >>>> really in need for it, OK what for? " and "what extended uses do you
> >>>> envision?" so I far I have not gotten any replies at all. So -- instead
> >>>> sysdata currently ignores it.
> >>>
> >>> So you argue for the remoteproc use case with 100+ MB firmware that
> >>> if there is a way to load after pivot_root() (or other additional
> >>> firmware partition shows up) then there is no need at all for
> >>> usermode helper?
> >>
> >> No, I'm saying I'd like to hear valid uses cases for the usermode helper and so
> >> far I have only found using coccinelle grammar 2 explicit users, that's it. My
> >> patch series (not yet merge) then annotates these as valid as I've verified
> >> through their documentation they have some quirky requirement.
> > 
> > In certain configurations (embedded) people do not want to use
> > initramfs nor modules nor embed firmware into the kernel. In this case
> > usermode helper + firmware calss timeout handling provides necessary
> > wait for the root filesystem to be mounted.
> 
> And there are people who don't have a usermode helper running at all 

Correction: most Linux distributions these days have
CONFIG_FW_LOADER_USER_HELPER but disable CONFIG_FW_LOADER_USER_HELPER_FALLBACK.

And then let me re-iterate from my patches then the implications:

+When CONFIG_FW_LOADER_USER_HELPER_FALLBACK is enabled request_firmware()
+*always* calls the usermode helpers. When CONFIG_FW_LOADER_USER_HELPER is
+enabled, only if you specifically ask for it will the usermode helper be
+called. If CONFIG_FW_LOADER_USER_HELPER_FALLBACK is disabled drivers can
+still require the usermode helper by using request_firmware_nowait().

The issues with CONFIG_FW_LOADER_USER_HELPER_FALLBACK are so much so
(the kmod timeout was an engineering mishap refer to my post with
details about it [0]) that these days most distributions disable it
and there have been huge efforts to ensure its not even called
explicitly, to the point now we only have TWO explicit users for it.
Keep in mind as well that in my series of patches to help clarify the
situation with the usermode helper I also proposed a fix to the firmware_class
to avoid the usermode helper completely when the firmware cache, this is
because the firmware cache purposely *kills* any pending usermode helpers
on the way to suspend anyway, so calling a new instance would be an error
and just delay suspend further.

[0] http://www.do-not-panic.com/2015/12/linux-asynchronous-probe.html

> in
> their configuration, but I guess they should disable the helper.

Yes. I have gone to good lengths to ask why the hell its needed and so 
far I have not gotten any technical reason for it.

> > If we solve waiting for rootfs (or something else that may contain
> > firmware) then these cases will not need to use usermode helper.
> 
> If firmware (or whatever) API could get notification of mount syscall it
> could be used to retry firmware loading instead of periodic polling.
> That leaves the question raised by you about when to stop trying. The
> initlevel stuff is probably a user-space only concept, right?

No init levels here refer to include/linux/init.h init calls that one
wraps the kernel module inits, or subsystem inits. If it wasn't for
pivot_root() existing and also the fact that systems can be complex
I would have suggested simply that we make firmware_class move to
late_initcall() -- this however would not solve all races given how
complex systems could be set up with a partition.

> So no
> ideas how the kernel itself could decide except for a "long" timeout.

If we wanted to learn from history -- the timeout for this would be a bad idea
so this makes it a bit more complex.  I proposed a technical idea here which
would avoid this and I think enable making this deterministic as well for new
systems that want that.

  Luis

Re: [RFC v0 7/8] Input: ims-pcu: use firmware_stat instead of completion

[Dmitry Torokhov]

On Wed, Aug 03, 2016 at 01:43:31PM +0200, Arend van Spriel wrote:
> On 03-08-16 09:42, Dmitry Torokhov wrote:
> > On Tue, Aug 2, 2016 at 12:41 AM, Luis R. Rodriguez <mcgrof@kernel.org> wrote:
> >> On Tue, Aug 02, 2016 at 08:53:55AM +0200, Daniel Wagner wrote:
> >>> On 08/02/2016 08:34 AM, Luis R. Rodriguez wrote:
> >>>> On Tue, Aug 02, 2016 at 07:49:19AM +0200, Daniel Wagner wrote:
> >>>>>> The sysdata API's main goal rather is to provide a flexible API first,
> >>>>>> compartamentalizing the usermode helper was secondary. But now it seems
> >>>>>> I may just also add devm support too to help simplify code further.
> >>>>>
> >>>>> I missed the point that you plan to add usermode helper support to
> >>>>> the sysdata API.
> >>>>
> >>>> I had no such plans, when I have asked folks so far about "hey are you
> >>>> really in need for it, OK what for? " and "what extended uses do you
> >>>> envision?" so I far I have not gotten any replies at all. So -- instead
> >>>> sysdata currently ignores it.
> >>>
> >>> So you argue for the remoteproc use case with 100+ MB firmware that
> >>> if there is a way to load after pivot_root() (or other additional
> >>> firmware partition shows up) then there is no need at all for
> >>> usermode helper?
> >>
> >> No, I'm saying I'd like to hear valid uses cases for the usermode helper and so
> >> far I have only found using coccinelle grammar 2 explicit users, that's it. My
> >> patch series (not yet merge) then annotates these as valid as I've verified
> >> through their documentation they have some quirky requirement.
> > 
> > In certain configurations (embedded) people do not want to use
> > initramfs nor modules nor embed firmware into the kernel. In this case
> > usermode helper + firmware calss timeout handling provides necessary
> > wait for the root filesystem to be mounted.
> 
> And there are people who don't have a usermode helper running at all in
> their configuration, but I guess they should disable the helper.

Right, if they don't that means their system is misconfigured.

> 
> In my opinion the kernel should provide functionality to user-space and
> user-space providing functionality to the kernel should be avoided.

Why? We have bunch of stuff running in userspace for the kernel. Fuse
for example. I am sure there are more.

> 
> > If we solve waiting for rootfs (or something else that may contain
> > firmware) then these cases will not need to use usermode helper.
> 
> If firmware (or whatever) API could get notification of mount syscall it
> could be used to retry firmware loading instead of periodic polling.
> That leaves the question raised by you about when to stop trying. The
> initlevel stuff is probably a user-space only concept, right? So no
> ideas how the kernel itself could decide except for a "long" timeout.

The kernel really does not know, it can only guess. The firmware may get
delivered by motorized carrier pidgeons. But distribution does know how
they set up, so they are in position to tell the kernel "go" or "give
up".

Thanks.

-- 
Dmitry

Re: [RFC v0 7/8] Input: ims-pcu: use firmware_stat instead of completion

[Arend van Spriel]

On 03-08-16 17:35, Dmitry Torokhov wrote:
>> In my opinion the kernel should provide functionality to user-space and
>> > user-space providing functionality to the kernel should be avoided.
> Why? We have bunch of stuff running in userspace for the kernel. Fuse
> for example. I am sure there are more.

To me "running in user-space" is not the same as providing
functionality, but I see your point given below.

>> > 
>>> > > If we solve waiting for rootfs (or something else that may contain
>>> > > firmware) then these cases will not need to use usermode helper.
>> > 
>> > If firmware (or whatever) API could get notification of mount syscall it
>> > could be used to retry firmware loading instead of periodic polling.
>> > That leaves the question raised by you about when to stop trying. The
>> > initlevel stuff is probably a user-space only concept, right? So no
>> > ideas how the kernel itself could decide except for a "long" timeout.
> The kernel really does not know, it can only guess. The firmware may get
> delivered by motorized carrier pidgeons. But distribution does know how
> they set up, so they are in position to tell the kernel "go" or "give
> up".

What distro employs pidgeons. Like to give it a spin ;-)

Maybe the latest idea from Luis is a viable option.

Regards,
Arend
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC v0 7/8] Input: ims-pcu: use firmware_stat instead of completion

[Luis R. Rodriguez]

On Wed, Aug 03, 2016 at 12:42:14AM -0700, Dmitry Torokhov wrote:
> On Tue, Aug 2, 2016 at 12:41 AM, Luis R. Rodriguez <mcgrof@kernel.org> wrote:
> > On Tue, Aug 02, 2016 at 08:53:55AM +0200, Daniel Wagner wrote:
> >> On 08/02/2016 08:34 AM, Luis R. Rodriguez wrote:
> >> >On Tue, Aug 02, 2016 at 07:49:19AM +0200, Daniel Wagner wrote:
> >> >>>The sysdata API's main goal rather is to provide a flexible API first,
> >> >>>compartamentalizing the usermode helper was secondary. But now it seems
> >> >>>I may just also add devm support too to help simplify code further.
> >> >>
> >> >>I missed the point that you plan to add usermode helper support to
> >> >>the sysdata API.
> >> >
> >> >I had no such plans, when I have asked folks so far about "hey are you
> >> >really in need for it, OK what for? " and "what extended uses do you
> >> >envision?" so I far I have not gotten any replies at all. So -- instead
> >> >sysdata currently ignores it.
> >>
> >> So you argue for the remoteproc use case with 100+ MB firmware that
> >> if there is a way to load after pivot_root() (or other additional
> >> firmware partition shows up) then there is no need at all for
> >> usermode helper?
> >
> > No, I'm saying I'd like to hear valid uses cases for the usermode helper and so
> > far I have only found using coccinelle grammar 2 explicit users, that's it. My
> > patch series (not yet merge) then annotates these as valid as I've verified
> > through their documentation they have some quirky requirement.
> 
> In certain configurations (embedded) people do not want to use
> initramfs nor modules nor embed firmware into the kernel. In this case
> usermode helper + firmware calss timeout handling provides necessary
> wait for the root filesystem to be mounted.
> 
> If we solve waiting for rootfs (or something else that may contain
> firmware) then these cases will not need to use usermode helper.

Given most distributions already disable FW_LOADER_USER_HELPER_FALLBACK and
grammar shows we only have 2 explicit users of the usermode helper I'd
prefer if we indeed could just compartamentalize the usermode helper
and not rely on it further. Furthermore I think its possible address
this issue, and suggested at least one idea how for now. With a bit
further review I'm in hope we can address this well, not only
for the firmware API but for all other kernel_read_file*() users.

  Luis

Re: [RFC v0 7/8] Input: ims-pcu: use firmware_stat instead of completion

[Bjorn Andersson]

On Tue 02 Aug 00:41 PDT 2016, Luis R. Rodriguez wrote:

> On Tue, Aug 02, 2016 at 08:53:55AM +0200, Daniel Wagner wrote:
> > On 08/02/2016 08:34 AM, Luis R. Rodriguez wrote:
> > >On Tue, Aug 02, 2016 at 07:49:19AM +0200, Daniel Wagner wrote:
> > >>>The sysdata API's main goal rather is to provide a flexible API first,
> > >>>compartamentalizing the usermode helper was secondary. But now it seems
> > >>>I may just also add devm support too to help simplify code further.
> > >>
> > >>I missed the point that you plan to add usermode helper support to
> > >>the sysdata API.
> > >
> > >I had no such plans, when I have asked folks so far about "hey are you
> > >really in need for it, OK what for? " and "what extended uses do you
> > >envision?" so I far I have not gotten any replies at all. So -- instead
> > >sysdata currently ignores it.
> > 
> > So you argue for the remoteproc use case with 100+ MB firmware that
> > if there is a way to load after pivot_root() (or other additional
> > firmware partition shows up) then there is no need at all for
> > usermode helper?
> 
> No, I'm saying I'd like to hear valid uses cases for the usermode helper and so
> far I have only found using coccinelle grammar 2 explicit users, that's it. My
> patch series (not yet merge) then annotates these as valid as I've verified
> through their documentation they have some quirky requirement.
> 

I think we're on the same page, but just to make sure; I do not want the
usermode helper, I only want a way to wait for the firmware files to
become available.

> Other than these two drivers I'd like hear to valid requirements for it.
> 
> The existential issue is a real issue but it does not look impossible to
> resolve. It may be a solution to bloat up the kernel with 100+ MB size just to
> stuff built-in firmware to avoid this issue, but it does not mean a solution
> is not possible.
> 
> Remind me -- why can remoteproc not stuff the firmware in initramfs ?
> 

RAM usage:
Storing the files in initramfs would consume 100MB RAM, we would then
allocate 100MB RAM for buffers during firmware loading and then we have
the reserved 100MB for the peripherals. The buffers could be easily be
removed with a mechanism for providing a buffer to the load operation,
but we would still double the RAM consumption.

Boot time:
Enlarging the kernel by 100MB will give noticeable addition to boot
times.

Development issues:
I have numerous concerns related to this, e.g. not being able to side
load the firmware files without rebuilding the initramfs. But most of
these are not technical issues, but rather a matter of convenience.

One large issue would be how to figure out how large to make the boot
partition in your Android phone, to cope with potential future growth in
firmware size - which has already proven to be a mess.

Legal matters:
Some of these firmware files are not redistributable, making it
impossible for end users to rebuild their kernel without loosing
functionality. There are even cases where these files are not allowed to
share partition with GPL binaries.


Most of these are just a major inconveniences to the developer but some
are show stoppers; especially the legal matters. So if we wave this off
as something people can live without then every downstream will sit on
their own solution to reimplement it.

> Anyway, here's a simple suggestion: fs/exec.c gets a sentinel file monitor
> support per enum kernel_read_file_id. For instance we'd have one for
> READING_FIRMWARE, one for READING_KEXEC_IMAGE, perhaps READING_POLICY, and this
> would in turn be used as the system configurable deterministic file for
> which to wait for to be present before enabling each enum kernel_read_file_id
> type read.
> 
> Thoughts ?

That does sound like a good generic solution for our problem and for the
other types of files as well. Do you have any ideas (patches?) on how
each sentinel would be triggered?

The only concern I can think of right now is that the
firmware_class.path might point to a separate partition; but based on
how the signaling of the sentinels are implemented this might not be an
issue.

Regards,
Bjorn
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC v0 7/8] Input: ims-pcu: use firmware_stat instead of completion

[Luis R. Rodriguez]

On Wed, Aug 03, 2016 at 10:39:55AM -0700, Bjorn Andersson wrote:
> On Tue 02 Aug 00:41 PDT 2016, Luis R. Rodriguez wrote:
> 
> > On Tue, Aug 02, 2016 at 08:53:55AM +0200, Daniel Wagner wrote:
> > > On 08/02/2016 08:34 AM, Luis R. Rodriguez wrote:
> > > >On Tue, Aug 02, 2016 at 07:49:19AM +0200, Daniel Wagner wrote:
> > > >>>The sysdata API's main goal rather is to provide a flexible API first,
> > > >>>compartamentalizing the usermode helper was secondary. But now it seems
> > > >>>I may just also add devm support too to help simplify code further.
> > > >>
> > > >>I missed the point that you plan to add usermode helper support to
> > > >>the sysdata API.
> > > >
> > > >I had no such plans, when I have asked folks so far about "hey are you
> > > >really in need for it, OK what for? " and "what extended uses do you
> > > >envision?" so I far I have not gotten any replies at all. So -- instead
> > > >sysdata currently ignores it.
> > > 
> > > So you argue for the remoteproc use case with 100+ MB firmware that
> > > if there is a way to load after pivot_root() (or other additional
> > > firmware partition shows up) then there is no need at all for
> > > usermode helper?
> > 
> > No, I'm saying I'd like to hear valid uses cases for the usermode helper and so
> > far I have only found using coccinelle grammar 2 explicit users, that's it. My
> > patch series (not yet merge) then annotates these as valid as I've verified
> > through their documentation they have some quirky requirement.
> > 
> 
> I think we're on the same page, but just to make sure; I do not want the
> usermode helper,

Yay.

> I only want a way to wait for the firmware files to
> become available.

Sure.

> > Other than these two drivers I'd like hear to valid requirements for it.
> > 
> > The existential issue is a real issue but it does not look impossible to
> > resolve. It may be a solution to bloat up the kernel with 100+ MB size just to
> > stuff built-in firmware to avoid this issue, but it does not mean a solution
> > is not possible.
> > 
> > Remind me -- why can remoteproc not stuff the firmware in initramfs ?
> > 
> 
> RAM usage:
> Storing the files in initramfs would consume 100MB RAM, we would then
> allocate 100MB RAM for buffers during firmware loading and then we have
> the reserved 100MB for the peripherals. The buffers could be easily be
> removed with a mechanism for providing a buffer to the load operation,
> but we would still double the RAM consumption.
> 
> Boot time:
> Enlarging the kernel by 100MB will give noticeable addition to boot
> times.

Right I see.. Since we read the full kernel...

> Development issues:
> I have numerous concerns related to this, e.g. not being able to side
> load the firmware files without rebuilding the initramfs. But most of
> these are not technical issues, but rather a matter of convenience.
> 
> One large issue would be how to figure out how large to make the boot
> partition in your Android phone, to cope with potential future growth in
> firmware size - which has already proven to be a mess.
> 
> Legal matters:
> Some of these firmware files are not redistributable, making it
> impossible for end users to rebuild their kernel without loosing
> functionality. There are even cases where these files are not allowed to
> share partition with GPL binaries.
> 
> 
> Most of these are just a major inconveniences to the developer but some
> are show stoppers; especially the legal matters. So if we wave this off
> as something people can live without then every downstream will sit on
> their own solution to reimplement it.

Thanks I'll document these into the firmware_class.

> > Anyway, here's a simple suggestion: fs/exec.c gets a sentinel file monitor
> > support per enum kernel_read_file_id. For instance we'd have one for
> > READING_FIRMWARE, one for READING_KEXEC_IMAGE, perhaps READING_POLICY, and this
> > would in turn be used as the system configurable deterministic file for
> > which to wait for to be present before enabling each enum kernel_read_file_id
> > type read.
> > 
> > Thoughts ?
> 
> That does sound like a good generic solution for our problem and for the
> other types of files as well. Do you have any ideas (patches?) on how
> each sentinel would be triggered?
> 
> The only concern I can think of right now is that the
> firmware_class.path might point to a separate partition; but based on
> how the signaling of the sentinels are implemented this might not be an
> issue.

There's another simpler suggestion I'm getting too, will post in the other
thread.

  Luis

Re: [RFC v0 7/8] Input: ims-pcu: use firmware_stat instead of completion

[Luis R. Rodriguez]

On Sun, Jul 31, 2016 at 12:23:09AM -0700, Dmitry Torokhov wrote:
> On July 30, 2016 9:58:17 AM PDT, "Luis R. Rodriguez" <mcgrof@kernel.org> wrote:
> >On Sat, Jul 30, 2016 at 02:42:41PM +0200, Arend van Spriel wrote:
> >> + Luis (again) ;-)
> >> 
> >> On 29-07-16 08:13, Daniel Wagner wrote:
> >> > On 07/28/2016 09:01 PM, Bjorn Andersson wrote:
> >> >> On Thu 28 Jul 11:33 PDT 2016, Dmitry Torokhov wrote:
> >> >>
> >> >>> On Thu, Jul 28, 2016 at 09:55:11AM +0200, Daniel Wagner wrote:
> >> >>>> From: Daniel Wagner <daniel.wagner@bmw-carit.de>
> >> >>>>
> >> >> [..]
> >> >>>
> >> >>> Do not quite like it... I'd rather asynchronous request give out
> >a
> >> >>> firmware status pointer that could be used later on.
> >> 
> >> Excellent. Why not get rid of the callback function as well and have
> >> fw_loading_wait() return result (0 = firmware available, < 0 = fail).
> >> Just to confirm, you are proposing a new API function next to
> >> request_firmware_nowait(), right?
> >
> >If proposing new firmware_class patches please bounce / Cc me, I've
> >recently asked for me to be added to MAINTAINERS so I get these
> >e-mails as I'm working on a new flexible API which would allow us
> >to extend the firmware API without having to care about the old
> >stupid usermode helper at all.
> 
> I am not sure why we started calling usermode helper "stupid".

OK Fair, how systemd implemented kmod timeout for the usermode helper
was stupid and it affected tons of systems.

> We only had to
> implement direct kernel firmware loading because udev/stsremd folks had
> "interesting" ideas how events should be handled; but having userspace to
> feed us data is not stupid.

It really should just be an option. The problem was the collateral due to the
way it was handled in userspace.

> If we want to overhaul firmware loading support we need to figure out how to
> support case when a driver want to [asynchronously] request
> firmware/config/blob and the rest of the system is not ready.

There's a lot of issues. So let's break them down.

1) First the sysdata API came about to help avoid the required set of collateral
evolutions whenever the firmware API was expanded. A new argument added meant
requiring to modify all drivers with the new argument, or a new exported
symbol. The sysdata API makes the API flexible, enabling extensions by just
expanding on the descriptor passed. The few features I've added to sysdata
(like avoiding drivers having to declare their own completions, waits) are
just small enhancements, but it seems now supporting devm may be desirable
as well.

2) The usermode helper cannot be removed, however we can compartamentalize it.
The sysdata API aims at helping with that, it doesn't touch it. There are
only 2 explicit users of the usermode helper now in the kernel. If there are
further users that really want it, I'd like to hear about them.

3) The firmware API having its own kernel read thing was fine but there were
other places in the kernel doing the same, this begged sharing that code
and also allowing then two LSM hooks to take care of handling doing whatever
with a kernel read, a pre-read hook and post-read hook. Mimi completed this
work and we now have the firmware API using kernel_read_file_from_path().

4) The asynchronous firmware loading issue you describe above is just *one*
issue, but it becomes more of an generic issue if you consider 3) above...
because naturally there could potentially be other users of kernel_read_file()
or kernel_read_file_from_path() and whereby a race also can happen. We may
decide that this is up to the subsystem/user to figure out. If that's the
case lets discuss that. It however occurs to me that it could just be as
simple as adding another fs/exec.c helper like kernel_read_file_from_path_wait()
which passes a completion and fs/exec.c just signals back when its ready.
If we have this both the old firmware_class and new sysdata API could benefit
from this behind the scenes -- no new API extension would be needed, this
would just be a firmware_class fix to use a more deterministic kernel
read API.

> Even if we want kernel to do read/load the data we need userspace to tell
> kernel when firmware partition is available, until then the kernel should not
> fail the request.

pivot_root() is possible as well, so exactly when the *real* partition is
ready is very system specific -- best I think we can do is perhaps just
see when the *first* file path becomes available and signal back. Problem
with this is we would still need a way to know -- *everything is ready*
as a limit condition for waiting.

  Luis

Re: [RFC v0 7/8] Input: ims-pcu: use firmware_stat instead of completion

[Bjorn Andersson]

On Sat 30 Jul 09:58 PDT 2016, Luis R. Rodriguez wrote:

> On Sat, Jul 30, 2016 at 02:42:41PM +0200, Arend van Spriel wrote:
> > + Luis (again) ;-)
> > 
> > On 29-07-16 08:13, Daniel Wagner wrote:
> > > On 07/28/2016 09:01 PM, Bjorn Andersson wrote:
> > >> On Thu 28 Jul 11:33 PDT 2016, Dmitry Torokhov wrote:
> > >>
> > >>> On Thu, Jul 28, 2016 at 09:55:11AM +0200, Daniel Wagner wrote:
> > >>>> From: Daniel Wagner <daniel.wagner@bmw-carit.de>
> > >>>>
> > >> [..]
> > >>>
> > >>> Do not quite like it... I'd rather asynchronous request give out a
> > >>> firmware status pointer that could be used later on.
> > 
> > Excellent. Why not get rid of the callback function as well and have
> > fw_loading_wait() return result (0 = firmware available, < 0 = fail).
> > Just to confirm, you are proposing a new API function next to
> > request_firmware_nowait(), right?
> 
> If proposing new firmware_class patches please bounce / Cc me, I've
> recently asked for me to be added to MAINTAINERS so I get these
> e-mails as I'm working on a new flexible API which would allow us
> to extend the firmware API without having to care about the old
> stupid usermode helper at all.
> 

In the remoteproc world there are several systems where we see 100+MB of
firmware being loaded. It's unfeasible to have these files in an
initramfs, so we need a way to indicate to the kernel that the
second/primary (or a dedicated firmware partition) is mounted.

We're currently loading these files with request_firmware_nowait(), so
either one can either use kernel modules or the user-helper fallback to
delay the loading until the files are available. (I don't like to
enforce the usage of kernel modules)

I'm open to alternative ways of having firmware loading wait on
secondary filesystems to be mounted, but have not yet tried to tackle
this problem. I do believe something that waits and retries the firmware
load as additional file systems gets mounted would be prettier than
forcing user space to tell us it's time to move on.



Due to the size of these firmware files we release the firmware as soon
as we have copied the content into the appropriate memory segments, so
we're not utilizing the caching mechanisms of the current fw loader.

Regards,
Bjorn

Re: [RFC v0 7/8] Input: ims-pcu: use firmware_stat instead of completion

[Luis R. Rodriguez]

On Mon, Aug 01, 2016 at 10:19:51AM -0700, Bjorn Andersson wrote:
> On Sat 30 Jul 09:58 PDT 2016, Luis R. Rodriguez wrote:
> 
> > On Sat, Jul 30, 2016 at 02:42:41PM +0200, Arend van Spriel wrote:
> > > + Luis (again) ;-)
> > > 
> > > On 29-07-16 08:13, Daniel Wagner wrote:
> > > > On 07/28/2016 09:01 PM, Bjorn Andersson wrote:
> > > >> On Thu 28 Jul 11:33 PDT 2016, Dmitry Torokhov wrote:
> > > >>
> > > >>> On Thu, Jul 28, 2016 at 09:55:11AM +0200, Daniel Wagner wrote:
> > > >>>> From: Daniel Wagner <daniel.wagner@bmw-carit.de>
> > > >>>>
> > > >> [..]
> > > >>>
> > > >>> Do not quite like it... I'd rather asynchronous request give out a
> > > >>> firmware status pointer that could be used later on.
> > > 
> > > Excellent. Why not get rid of the callback function as well and have
> > > fw_loading_wait() return result (0 = firmware available, < 0 = fail).
> > > Just to confirm, you are proposing a new API function next to
> > > request_firmware_nowait(), right?
> > 
> > If proposing new firmware_class patches please bounce / Cc me, I've
> > recently asked for me to be added to MAINTAINERS so I get these
> > e-mails as I'm working on a new flexible API which would allow us
> > to extend the firmware API without having to care about the old
> > stupid usermode helper at all.
> > 
> 
> In the remoteproc world there are several systems where we see 100+MB of
> firmware being loaded. It's unfeasible to have these files in an
> initramfs, so we need a way to indicate to the kernel that the
> second/primary (or a dedicated firmware partition) is mounted.
> 
> We're currently loading these files with request_firmware_nowait(), so
> either one can either use kernel modules or the user-helper fallback to
> delay the loading until the files are available. (I don't like to
> enforce the usage of kernel modules)

Now that the firmware API is sharing the same API call to read files
the existential issue of the file is not unique issue of firmware,
but also to any other caller of it.

> I'm open to alternative ways of having firmware loading wait on
> secondary filesystems to be mounted, but have not yet tried to tackle
> this problem. I do believe something that waits and retries the firmware
> load as additional file systems gets mounted would be prettier than
> forcing user space to tell us it's time to move on.

Agreed. We simply have not addressed this problem yet. Let's discuss a
possible solution on the other reply thread I provided with more details
to Dmitry.

> Due to the size of these firmware files we release the firmware as soon
> as we have copied the content into the appropriate memory segments, so
> we're not utilizing the caching mechanisms of the current fw loader.

BTW my goal with the sysdata API is to automatically free this for you too :)

  Luis

Re: [RFC v0 7/8] Input: ims-pcu: use firmware_stat instead of completion

[Dmitry Torokhov]

On Mon, Aug 01, 2016 at 10:19:51AM -0700, Bjorn Andersson wrote:
> On Sat 30 Jul 09:58 PDT 2016, Luis R. Rodriguez wrote:
> 
> > On Sat, Jul 30, 2016 at 02:42:41PM +0200, Arend van Spriel wrote:
> > > + Luis (again) ;-)
> > > 
> > > On 29-07-16 08:13, Daniel Wagner wrote:
> > > > On 07/28/2016 09:01 PM, Bjorn Andersson wrote:
> > > >> On Thu 28 Jul 11:33 PDT 2016, Dmitry Torokhov wrote:
> > > >>
> > > >>> On Thu, Jul 28, 2016 at 09:55:11AM +0200, Daniel Wagner wrote:
> > > >>>> From: Daniel Wagner <daniel.wagner@bmw-carit.de>
> > > >>>>
> > > >> [..]
> > > >>>
> > > >>> Do not quite like it... I'd rather asynchronous request give out a
> > > >>> firmware status pointer that could be used later on.
> > > 
> > > Excellent. Why not get rid of the callback function as well and have
> > > fw_loading_wait() return result (0 = firmware available, < 0 = fail).
> > > Just to confirm, you are proposing a new API function next to
> > > request_firmware_nowait(), right?
> > 
> > If proposing new firmware_class patches please bounce / Cc me, I've
> > recently asked for me to be added to MAINTAINERS so I get these
> > e-mails as I'm working on a new flexible API which would allow us
> > to extend the firmware API without having to care about the old
> > stupid usermode helper at all.
> > 
> 
> In the remoteproc world there are several systems where we see 100+MB of
> firmware being loaded. It's unfeasible to have these files in an
> initramfs, so we need a way to indicate to the kernel that the
> second/primary (or a dedicated firmware partition) is mounted.
> 
> We're currently loading these files with request_firmware_nowait(), so
> either one can either use kernel modules or the user-helper fallback to
> delay the loading until the files are available. (I don't like to
> enforce the usage of kernel modules)
> 
> I'm open to alternative ways of having firmware loading wait on
> secondary filesystems to be mounted, but have not yet tried to tackle
> this problem. I do believe something that waits and retries the firmware
> load as additional file systems gets mounted would be prettier than
> forcing user space to tell us it's time to move on.

Hmm, but when do you stop? How do you know that you are not going to get
yet another fs mounted and that one will finally have the firmware you
are looking for? The kernel does now, but distribution/product
integrator does. That is why I think the most reliable way is to see if
firmware is built-in, otherwise wait and let userspace do its thing.

Thanks.

-- 
Dmitry

Re: [RFC v0 7/8] Input: ims-pcu: use firmware_stat instead of completion

[Dmitry Torokhov]

On July 30, 2016 5:42:41 AM PDT, Arend van Spriel <arend.vanspriel@broadcom.com> wrote:
>+ Luis (again) ;-)
>
>On 29-07-16 08:13, Daniel Wagner wrote:
>> On 07/28/2016 09:01 PM, Bjorn Andersson wrote:
>>> On Thu 28 Jul 11:33 PDT 2016, Dmitry Torokhov wrote:
>>>
>>>> On Thu, Jul 28, 2016 at 09:55:11AM +0200, Daniel Wagner wrote:
>>>>> From: Daniel Wagner <daniel.wagner@bmw-carit.de>
>>>>>
>>> [..]
>>>>
>>>> Do not quite like it... I'd rather asynchronous request give out a
>>>> firmware status pointer that could be used later on.
>
>Excellent. Why not get rid of the callback function as well and have
>fw_loading_wait() return result (0 = firmware available, < 0 = fail).
>Just to confirm, you are proposing a new API function next to
>request_firmware_nowait(), right?

Yes, that would be a new API call. Maybe we could replace old API with the new at some point.


>>>>     pcu->fw_st = request_firmware_async(IMS_PCU_FIRMWARE_NAME,
>>>> -                       pcu,
>>>> -                       ims_pcu_process_async_firmware);
>    +			    pcu);
>>>>     if (IS_ERR(pcu->fw_st))
>>>>         return PTR_ERR(pcu->fw_st);
>>>>
>>>>     ....
>>>>
>>>>     err = fw_loading_wait(pcu->fw_st);
>	if (err)
>		return err;
>
>	fw = fwstat_get_firmware(pcu->fw_st);
>
>Or whatever consistent prefix it is going to be.
>
>>>>
>>>
>>> In the remoteproc case (patch 6) this would clean up the code,
>rather
>>> than replacing the completion API 1 to 1. I like it!
>> 
>> IIRC most drivers do it the same way. So request_firmware_async()
>indeed
>> would be good thing to have. Let me try that.
>
>While the idea behind this series is a good one I am wondering about
>the
>need for these drivers to use the asynchronous API. The historic reason
>might be to avoid timeout caused by user-mode helper, but that may no
>longer apply and these drivers could be better off using
>request_firmware_direct().

Actually systems using this driver rely on usermode helper to provide necessary delay and load the firmware from storage once root partition is mounted. Converting to request_firmware_direct() would break them.


Thanks.

-- 
Dmitry

[RFC v0 8/8] iwl4965: use firmware_stat instead of completion

[Daniel Wagner]

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

Loading firmware is an operation many drivers implement in various ways
around the completion API. And most of them do it almost in the same
way. Let's reuse the firmware_stat API which is used also by the
firmware_class loader. Apart of streamlining the firmware loading states
we also document it slightly better.

Signed-off-by: Daniel Wagner <daniel.wagner@bmw-carit.de>
---
 drivers/net/wireless/intel/iwlegacy/4965-mac.c | 8 ++++----
 drivers/net/wireless/intel/iwlegacy/common.h   | 3 ++-
 2 files changed, 6 insertions(+), 5 deletions(-)

diff --git a/drivers/net/wireless/intel/iwlegacy/4965-mac.c b/drivers/net/wireless/intel/iwlegacy/4965-mac.c
index a91d170..d5e5808 100644
--- a/drivers/net/wireless/intel/iwlegacy/4965-mac.c
+++ b/drivers/net/wireless/intel/iwlegacy/4965-mac.c
@@ -5005,7 +5005,7 @@ il4965_ucode_callback(const struct firmware *ucode_raw, void *context)
 
 	/* We have our copies now, allow OS release its copies */
 	release_firmware(ucode_raw);
-	complete(&il->_4965.firmware_loading_complete);
+	fw_loading_done(il->_4965.fw_st);
 	return;
 
 try_again:
@@ -5019,7 +5019,7 @@ err_pci_alloc:
 	IL_ERR("failed to allocate pci memory\n");
 	il4965_dealloc_ucode_pci(il);
 out_unbind:
-	complete(&il->_4965.firmware_loading_complete);
+	fw_loading_done(il->_4965.fw_st);
 	device_release_driver(&il->pci_dev->dev);
 	release_firmware(ucode_raw);
 }
@@ -6678,7 +6678,7 @@ il4965_pci_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
 
 	il_power_initialize(il);
 
-	init_completion(&il->_4965.firmware_loading_complete);
+	firmware_stat_init(&il->_4965.fw_st);
 
 	err = il4965_request_firmware(il, true);
 	if (err)
@@ -6716,7 +6716,7 @@ il4965_pci_remove(struct pci_dev *pdev)
 	if (!il)
 		return;
 
-	wait_for_completion(&il->_4965.firmware_loading_complete);
+	fw_loading_wait(il->_4965.fw_st);
 
 	D_INFO("*** UNLOAD DRIVER ***\n");
 
diff --git a/drivers/net/wireless/intel/iwlegacy/common.h b/drivers/net/wireless/intel/iwlegacy/common.h
index 726ede3..94af7b7 100644
--- a/drivers/net/wireless/intel/iwlegacy/common.h
+++ b/drivers/net/wireless/intel/iwlegacy/common.h
@@ -32,6 +32,7 @@
 #include <linux/leds.h>
 #include <linux/wait.h>
 #include <linux/io.h>
+#include <linux/firmware.h>
 #include <net/mac80211.h>
 #include <net/ieee80211_radiotap.h>
 
@@ -1357,7 +1358,7 @@ struct il_priv {
 			bool last_phy_res_valid;
 			u32 ampdu_ref;
 
-			struct completion firmware_loading_complete;
+			struct firmware_stat fw_st;
 
 			/*
 			 * chain noise reset and gain commands are the
-- 
2.7.4