[PATCH] firmware_class: encapsulate firmware loading status

[PATCH] firmware_class: encapsulate firmware loading status

[Daniel Wagner]

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

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

The fw_lock is not needed to protect the status update anymore. We don't
do any RMW operations. Instead we just do a write or a read, not both at
the same time.

Signed-off-by: Daniel Wagner <daniel.wagner@bmw-carit.de>
Cc: Ming Lei <ming.lei@canonical.com>
Cc: Luis R. Rodriguez <mcgrof@kernel.org>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---

Hi,

In [0] we have a discussion on how the firmware_class API might be
changed to improve the current handling of firmware loading. This
patch was part of the orignal RFC which triggered the discussion.

I think it is worth taking this one anyway. Maybe as I suggested it
could be part of the series from Luis.

It cleans up the code base (okay my opinion) and removes the
complete_all() call which is problematic for -rt. complete_all() can
be used in any context including IRQ. That could lead to unbounded
work in the IRQ context and that is a no go for -rt. So here the
attempt to reduce the number of complete_all() calls where possible. I
have left this argument out in the commit message because I was told
'-rt' arguments don't count for inclusion.

cheers,
daniel

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

drivers/base/firmware_class.c | 148 ++++++++++++++++++++++++------------------
 1 file changed, 85 insertions(+), 63 deletions(-)

diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
index 773fc30..665a803 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,19 +86,78 @@ static inline bool fw_is_builtin_firmware(const struct firmware *fw)
 }
 #endif
 
+static int loading_timeout = 60;	/* In seconds */
+
+static inline long firmware_loading_timeout(void)
+{
+	return loading_timeout > 0 ? loading_timeout * HZ : MAX_JIFFY_OFFSET;
+}
+
 enum {
+	FW_STATUS_UNKNOWN,
 	FW_STATUS_LOADING,
 	FW_STATUS_DONE,
-	FW_STATUS_ABORT,
+	FW_STATUS_ABORTED,
 };
 
-static int loading_timeout = 60;	/* In seconds */
+struct fw_status {
+	unsigned long status;
+	struct swait_queue_head wq;
+};
 
-static inline long firmware_loading_timeout(void)
+
+static void fw_status_init(struct fw_status *fw_st)
 {
-	return loading_timeout > 0 ? loading_timeout * HZ : MAX_JIFFY_OFFSET;
+	fw_st->status = FW_STATUS_UNKNOWN;
+	init_swait_queue_head(&fw_st->wq);
+}
+
+static unsigned long __fw_status_get(struct fw_status *fw_st)
+{
+	return READ_ONCE(fw_st->status);
+}
+
+static inline bool is_fw_status_done(unsigned long status)
+{
+	return status == FW_STATUS_DONE ||
+	       status == FW_STATUS_ABORTED;
+}
+
+static int fw_status_wait_timeout(struct fw_status *fw_st, long timeout)
+{
+	int err;
+	err = swait_event_interruptible_timeout(fw_st->wq,
+				is_fw_status_done(READ_ONCE(fw_st->status)),
+				timeout);
+	if (err == 0 && fw_st->status == FW_STATUS_ABORTED)
+		return -ENOENT;
+
+	return err;
 }
 
+static void __fw_status_set(struct fw_status *fw_st,
+			  unsigned long status)
+{
+	WRITE_ONCE(fw_st->status, status);
+
+	if (status == FW_STATUS_DONE ||
+			status == FW_STATUS_ABORTED)
+		swake_up(&fw_st->wq);
+}
+
+#define fw_status_start(fw_st)					\
+	__fw_status_set(fw_st, FW_STATUS_LOADING)
+#define fw_status_done(fw_st)					\
+	__fw_status_set(fw_st, FW_STATUS_DONE)
+#define fw_status_aborted(fw_st)				\
+	__fw_status_set(fw_st, FW_STATUS_ABORTED)
+#define fw_status_is_loading(fw_st)				\
+	(__fw_status_get(fw_st) == FW_STATUS_LOADING)
+#define fw_status_is_done(fw_st)				\
+	(__fw_status_get(fw_st) == FW_STATUS_DONE)
+#define fw_status_is_aborted(fw_st)				\
+	(__fw_status_get(fw_st) == FW_STATUS_ABORTED)
+
 /* firmware behavior options */
 #define FW_OPT_UEVENT	(1U << 0)
 #define FW_OPT_NOWAIT	(1U << 1)
@@ -138,9 +198,8 @@ struct firmware_cache {
 struct firmware_buf {
 	struct kref ref;
 	struct list_head list;
-	struct completion completion;
+	struct fw_status fw_st;
 	struct firmware_cache *fwc;
-	unsigned long status;
 	void *data;
 	size_t size;
 #ifdef CONFIG_FW_LOADER_USER_HELPER
@@ -194,7 +253,7 @@ static struct firmware_buf *__allocate_fw_buf(const char *fw_name,
 
 	kref_init(&buf->ref);
 	buf->fwc = fwc;
-	init_completion(&buf->completion);
+	fw_status_init(&buf->fw_st);
 #ifdef CONFIG_FW_LOADER_USER_HELPER
 	INIT_LIST_HEAD(&buf->pending_list);
 #endif
@@ -292,15 +351,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 +389,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_status_done(&buf->fw_st);
 		break;
 	}
 	__putname(path);
@@ -457,12 +507,11 @@ static void __fw_load_abort(struct firmware_buf *buf)
 	 * There is a small window in which user can write to 'loading'
 	 * between loading done and disappearance of 'loading'
 	 */
-	if (test_bit(FW_STATUS_DONE, &buf->status))
+	if (fw_status_is_done(&buf->fw_st))
 		return;
 
 	list_del_init(&buf->pending_list);
-	set_bit(FW_STATUS_ABORT, &buf->status);
-	complete_all(&buf->completion);
+	fw_status_aborted(&buf->fw_st);
 }
 
 static void fw_load_abort(struct firmware_priv *fw_priv)
@@ -475,9 +524,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 */
@@ -575,10 +621,8 @@ static ssize_t firmware_loading_show(struct device *dev,
 	struct firmware_priv *fw_priv = to_firmware_priv(dev);
 	int loading = 0;
 
-	mutex_lock(&fw_lock);
 	if (fw_priv->buf)
-		loading = test_bit(FW_STATUS_LOADING, &fw_priv->buf->status);
-	mutex_unlock(&fw_lock);
+		loading = fw_status_is_loading(&fw_priv->buf->fw_st);
 
 	return sprintf(buf, "%d\n", loading);
 }
@@ -632,23 +676,20 @@ static ssize_t firmware_loading_store(struct device *dev,
 	switch (loading) {
 	case 1:
 		/* discarding any previous partial load */
-		if (!test_bit(FW_STATUS_DONE, &fw_buf->status)) {
+		if (!fw_status_is_done(&fw_buf->fw_st)) {
 			for (i = 0; i < fw_buf->nr_pages; i++)
 				__free_page(fw_buf->pages[i]);
 			vfree(fw_buf->pages);
 			fw_buf->pages = NULL;
 			fw_buf->page_array_size = 0;
 			fw_buf->nr_pages = 0;
-			set_bit(FW_STATUS_LOADING, &fw_buf->status);
+			fw_status_start(&fw_buf->fw_st);
 		}
 		break;
 	case 0:
-		if (test_bit(FW_STATUS_LOADING, &fw_buf->status)) {
+		if (fw_status_is_loading(&fw_buf->fw_st)) {
 			int rc;
 
-			set_bit(FW_STATUS_DONE, &fw_buf->status);
-			clear_bit(FW_STATUS_LOADING, &fw_buf->status);
-
 			/*
 			 * Several loading requests may be pending on
 			 * one same firmware buf, so let all requests
@@ -670,10 +711,11 @@ static ssize_t firmware_loading_store(struct device *dev,
 			 */
 			list_del_init(&fw_buf->pending_list);
 			if (rc) {
-				set_bit(FW_STATUS_ABORT, &fw_buf->status);
+				fw_status_aborted(&fw_buf->fw_st);
 				written = rc;
+			} else {
+				fw_status_done(&fw_buf->fw_st);
 			}
-			complete_all(&fw_buf->completion);
 			break;
 		}
 		/* fallthrough */
@@ -681,7 +723,7 @@ static ssize_t firmware_loading_store(struct device *dev,
 		dev_err(dev, "%s: unexpected value (%d)\n", __func__, loading);
 		/* fallthrough */
 	case -1:
-		fw_load_abort(fw_priv);
+		fw_status_aborted(&fw_buf->fw_st);
 		break;
 	}
 out:
@@ -702,7 +744,7 @@ static ssize_t firmware_data_read(struct file *filp, struct kobject *kobj,
 
 	mutex_lock(&fw_lock);
 	buf = fw_priv->buf;
-	if (!buf || test_bit(FW_STATUS_DONE, &buf->status)) {
+	if (!buf || fw_status_is_done(&buf->fw_st)) {
 		ret_count = -ENODEV;
 		goto out;
 	}
@@ -799,7 +841,7 @@ static ssize_t firmware_data_write(struct file *filp, struct kobject *kobj,
 
 	mutex_lock(&fw_lock);
 	buf = fw_priv->buf;
-	if (!buf || test_bit(FW_STATUS_DONE, &buf->status)) {
+	if (!buf || fw_status_is_done(&buf->fw_st)) {
 		retval = -ENODEV;
 		goto out;
 	}
@@ -917,8 +959,7 @@ static int _request_firmware_load(struct firmware_priv *fw_priv,
 		timeout = MAX_JIFFY_OFFSET;
 	}
 
-	retval = wait_for_completion_interruptible_timeout(&buf->completion,
-			timeout);
+	retval = fw_status_wait_timeout(&buf->fw_st, timeout);
 	if (retval == -ERESTARTSYS || !retval) {
 		mutex_lock(&fw_lock);
 		fw_load_abort(fw_priv);
@@ -927,7 +968,7 @@ static int _request_firmware_load(struct firmware_priv *fw_priv,
 		retval = 0;
 	}
 
-	if (is_fw_load_aborted(buf))
+	if (fw_status_is_aborted(&buf->fw_st))
 		retval = -EAGAIN;
 	else if (!buf->data)
 		retval = -ENOMEM;
@@ -978,7 +1019,7 @@ fw_load_from_user_helper(struct firmware *firmware, const char *name,
 }
 
 /* No abort during direct loading */
-#define is_fw_load_aborted(buf) false
+#define fw_status_is_aborted(buf) false
 
 #ifdef CONFIG_PM_SLEEP
 static inline void kill_requests_without_uevent(void) { }
@@ -986,26 +1027,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 +1060,8 @@ _request_firmware_prepare(struct firmware **firmware_p, const char *name,
 	firmware->priv = buf;
 
 	if (ret > 0) {
-		ret = sync_cached_firmware_buf(buf);
+		ret = fw_status_wait_timeout(&buf->fw_st,
+					   firmware_loading_timeout());
 		if (!ret) {
 			fw_set_page_data(buf, firmware);
 			return 0; /* assigned */
@@ -1057,7 +1079,7 @@ static int assign_firmware_buf(struct firmware *fw, struct device *device,
 	struct firmware_buf *buf = fw->priv;
 
 	mutex_lock(&fw_lock);
-	if (!buf->size || is_fw_load_aborted(buf)) {
+	if (!buf->size || fw_status_is_aborted(&buf->fw_st)) {
 		mutex_unlock(&fw_lock);
 		return -ENOENT;
 	}
-- 
2.7.4

Re: [PATCH] firmware_class: encapsulate firmware loading status

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 1948 bytes --]

Hi Daniel,

[auto build test WARNING on v4.7-rc7]
[cannot apply to next-20160803]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Daniel-Wagner/firmware_class-encapsulate-firmware-loading-status/20160804-173551
config: cris-etrax-100lx_v2_defconfig (attached as .config)
compiler: cris-linux-gcc (GCC) 4.6.3
reproduce:
        wget https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        make.cross ARCH=cris 

All warnings (new ones prefixed by >>):

>> drivers/base/firmware_class.c:1022:0: warning: "fw_status_is_aborted" redefined [enabled by default]
   drivers/base/firmware_class.c:158:0: note: this is the location of the previous definition
   drivers/base/firmware_class.c:115:22: warning: '__fw_status_get' defined but not used [-Wunused-function]

vim +/fw_status_is_aborted +1022 drivers/base/firmware_class.c

  1006				 __fw_load_abort(buf);
  1007		}
  1008		mutex_unlock(&fw_lock);
  1009	}
  1010	#endif
  1011	
  1012	#else /* CONFIG_FW_LOADER_USER_HELPER */
  1013	static inline int
  1014	fw_load_from_user_helper(struct firmware *firmware, const char *name,
  1015				 struct device *device, unsigned int opt_flags,
  1016				 long timeout)
  1017	{
  1018		return -ENOENT;
  1019	}
  1020	
  1021	/* No abort during direct loading */
> 1022	#define fw_status_is_aborted(buf) false
  1023	
  1024	#ifdef CONFIG_PM_SLEEP
  1025	static inline void kill_requests_without_uevent(void) { }
  1026	#endif
  1027	
  1028	#endif /* CONFIG_FW_LOADER_USER_HELPER */
  1029	
  1030	/* prepare firmware and firmware_buf structs;

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/octet-stream, Size: 8254 bytes --]

[PATCH v1] firmware_class: encapsulate firmware loading status

[Daniel Wagner]

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

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

The fw_lock is not needed to protect the status update anymore. We don't
do any RMW operations. Instead we just do a write or a read, not both at
the same time.

[v1: moved fw_status into !CONFIG_FW_LOADER_USER_HELPER section,
     reported by 0day kbuild]

Signed-off-by: Daniel Wagner <daniel.wagner@bmw-carit.de>
Cc: Ming Lei <ming.lei@canonical.com>
Cc: Luis R. Rodriguez <mcgrof@kernel.org>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---

Hi,

In [0] we have a discussion on how the firmware_class API might be
changed to improve the current handling of firmware loading. This
patch was part of the orignal RFC which triggered the discussion.

I think it is worth taking this one anyway. Maybe as I suggested it
could be part of the series from Luis.

It cleans up the code base (okay my opinion) and removes the
complete_all() call which is problematic for -rt. complete_all() can
be used in any context including IRQ. That could lead to unbounded
work in the IRQ context and that is a no go for -rt. So here the
attempt to reduce the number of complete_all() calls where possible. I
have left this argument out in the commit message because I was told
'-rt' arguments don't count for inclusion.

cheers,
daniel

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

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

diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
index 22d1760..33eb372 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>
 
@@ -91,19 +92,83 @@ static inline bool fw_is_builtin_firmware(const struct firmware *fw)
 }
 #endif
 
+static int loading_timeout = 60;	/* In seconds */
+
+static inline long firmware_loading_timeout(void)
+{
+	return loading_timeout > 0 ? loading_timeout * HZ : MAX_JIFFY_OFFSET;
+}
+
 enum {
+	FW_STATUS_UNKNOWN,
 	FW_STATUS_LOADING,
 	FW_STATUS_DONE,
-	FW_STATUS_ABORT,
+	FW_STATUS_ABORTED,
 };
 
-static int loading_timeout = 60;	/* In seconds */
+#ifdef CONFIG_FW_LOADER_USER_HELPER
+struct fw_status {
+	unsigned long status;
+	struct swait_queue_head wq;
+};
 
-static inline long firmware_loading_timeout(void)
+static void fw_status_init(struct fw_status *fw_st)
 {
-	return loading_timeout > 0 ? loading_timeout * HZ : MAX_JIFFY_OFFSET;
+	fw_st->status = FW_STATUS_UNKNOWN;
+	init_swait_queue_head(&fw_st->wq);
+}
+
+static inline bool is_fw_status_done(unsigned long status)
+{
+	return status == FW_STATUS_DONE ||
+	       status == FW_STATUS_ABORTED;
+}
+
+static int fw_status_wait_timeout(struct fw_status *fw_st, long timeout)
+{
+	int err;
+	err = swait_event_interruptible_timeout(fw_st->wq,
+				is_fw_status_done(READ_ONCE(fw_st->status)),
+				timeout);
+	if (err == 0 && fw_st->status == FW_STATUS_ABORTED)
+		return -ENOENT;
+
+	return err;
+}
+
+static void __fw_status_set(struct fw_status *fw_st,
+			  unsigned long status)
+{
+	WRITE_ONCE(fw_st->status, status);
+
+	if (status == FW_STATUS_DONE ||
+			status == FW_STATUS_ABORTED)
+		swake_up(&fw_st->wq);
+}
+
+static unsigned long __fw_status_get(struct fw_status *fw_st)
+{
+	return READ_ONCE(fw_st->status);
 }
 
+#define fw_status_start(fw_st)					\
+	__fw_status_set(fw_st, FW_STATUS_LOADING)
+#define fw_status_done(fw_st)					\
+	__fw_status_set(fw_st, FW_STATUS_DONE)
+#define fw_status_aborted(fw_st)				\
+	__fw_status_set(fw_st, FW_STATUS_ABORTED)
+#define fw_status_is_loading(fw_st)				\
+	(__fw_status_get(fw_st) == FW_STATUS_LOADING)
+#define fw_status_is_done(fw_st)				\
+	(__fw_status_get(fw_st) == FW_STATUS_DONE)
+#define fw_status_is_aborted(fw_st)				\
+	(__fw_status_get(fw_st) == FW_STATUS_ABORTED)
+#else
+#define fw_status_wait_timeout(fw_st, timeout)	0
+#define fw_status_done(fw_st)
+#define fw_status_is_aborted(fw_st)		false
+#endif /* CONFIG_FW_LOADER_USER_HELPER */
+
 /* firmware behavior options */
 #define FW_OPT_UEVENT	(1U << 0)
 #define FW_OPT_NOWAIT	(1U << 1)
@@ -145,13 +210,12 @@ struct firmware_cache {
 struct firmware_buf {
 	struct kref ref;
 	struct list_head list;
-	struct completion completion;
 	struct firmware_cache *fwc;
-	unsigned long status;
 	void *data;
 	size_t size;
 	size_t allocated_size;
 #ifdef CONFIG_FW_LOADER_USER_HELPER
+	struct fw_status fw_st;
 	bool is_paged_buf;
 	bool need_uevent;
 	struct page **pages;
@@ -205,8 +269,8 @@ static struct firmware_buf *__allocate_fw_buf(const char *fw_name,
 	buf->fwc = fwc;
 	buf->data = dbuf;
 	buf->allocated_size = size;
-	init_completion(&buf->completion);
 #ifdef CONFIG_FW_LOADER_USER_HELPER
+	fw_status_init(&buf->fw_st);
 	INIT_LIST_HEAD(&buf->pending_list);
 #endif
 
@@ -305,15 +369,6 @@ static const char * const fw_path[] = {
 module_param_string(path, fw_path_para, sizeof(fw_path_para), 0644);
 MODULE_PARM_DESC(path, "customized firmware image search path with a higher priority than default path");
 
-static void fw_finish_direct_load(struct device *device,
-				  struct firmware_buf *buf)
-{
-	mutex_lock(&fw_lock);
-	set_bit(FW_STATUS_DONE, &buf->status);
-	complete_all(&buf->completion);
-	mutex_unlock(&fw_lock);
-}
-
 static int
 fw_get_filesystem_firmware(struct device *device, struct firmware_buf *buf)
 {
@@ -360,7 +415,7 @@ fw_get_filesystem_firmware(struct device *device, struct firmware_buf *buf)
 		}
 		dev_dbg(device, "direct-loading %s\n", buf->fw_id);
 		buf->size = size;
-		fw_finish_direct_load(device, buf);
+		fw_status_done(&buf->fw_st);
 		break;
 	}
 	__putname(path);
@@ -478,12 +533,11 @@ static void __fw_load_abort(struct firmware_buf *buf)
 	 * There is a small window in which user can write to 'loading'
 	 * between loading done and disappearance of 'loading'
 	 */
-	if (test_bit(FW_STATUS_DONE, &buf->status))
+	if (fw_status_is_done(&buf->fw_st))
 		return;
 
 	list_del_init(&buf->pending_list);
-	set_bit(FW_STATUS_ABORT, &buf->status);
-	complete_all(&buf->completion);
+	fw_status_aborted(&buf->fw_st);
 }
 
 static void fw_load_abort(struct firmware_priv *fw_priv)
@@ -496,9 +550,6 @@ static void fw_load_abort(struct firmware_priv *fw_priv)
 	fw_priv->buf = NULL;
 }
 
-#define is_fw_load_aborted(buf)	\
-	test_bit(FW_STATUS_ABORT, &(buf)->status)
-
 static LIST_HEAD(pending_fw_head);
 
 /* reboot notifier for avoid deadlock with usermode_lock */
@@ -596,10 +647,8 @@ static ssize_t firmware_loading_show(struct device *dev,
 	struct firmware_priv *fw_priv = to_firmware_priv(dev);
 	int loading = 0;
 
-	mutex_lock(&fw_lock);
 	if (fw_priv->buf)
-		loading = test_bit(FW_STATUS_LOADING, &fw_priv->buf->status);
-	mutex_unlock(&fw_lock);
+		loading = fw_status_is_loading(&fw_priv->buf->fw_st);
 
 	return sprintf(buf, "%d\n", loading);
 }
@@ -653,23 +702,20 @@ static ssize_t firmware_loading_store(struct device *dev,
 	switch (loading) {
 	case 1:
 		/* discarding any previous partial load */
-		if (!test_bit(FW_STATUS_DONE, &fw_buf->status)) {
+		if (!fw_status_is_done(&fw_buf->fw_st)) {
 			for (i = 0; i < fw_buf->nr_pages; i++)
 				__free_page(fw_buf->pages[i]);
 			vfree(fw_buf->pages);
 			fw_buf->pages = NULL;
 			fw_buf->page_array_size = 0;
 			fw_buf->nr_pages = 0;
-			set_bit(FW_STATUS_LOADING, &fw_buf->status);
+			fw_status_start(&fw_buf->fw_st);
 		}
 		break;
 	case 0:
-		if (test_bit(FW_STATUS_LOADING, &fw_buf->status)) {
+		if (fw_status_is_loading(&fw_buf->fw_st)) {
 			int rc;
 
-			set_bit(FW_STATUS_DONE, &fw_buf->status);
-			clear_bit(FW_STATUS_LOADING, &fw_buf->status);
-
 			/*
 			 * Several loading requests may be pending on
 			 * one same firmware buf, so let all requests
@@ -691,10 +737,11 @@ static ssize_t firmware_loading_store(struct device *dev,
 			 */
 			list_del_init(&fw_buf->pending_list);
 			if (rc) {
-				set_bit(FW_STATUS_ABORT, &fw_buf->status);
+				fw_status_aborted(&fw_buf->fw_st);
 				written = rc;
+			} else {
+				fw_status_done(&fw_buf->fw_st);
 			}
-			complete_all(&fw_buf->completion);
 			break;
 		}
 		/* fallthrough */
@@ -702,7 +749,7 @@ static ssize_t firmware_loading_store(struct device *dev,
 		dev_err(dev, "%s: unexpected value (%d)\n", __func__, loading);
 		/* fallthrough */
 	case -1:
-		fw_load_abort(fw_priv);
+		fw_status_aborted(&fw_buf->fw_st);
 		break;
 	}
 out:
@@ -755,7 +802,7 @@ static ssize_t firmware_data_read(struct file *filp, struct kobject *kobj,
 
 	mutex_lock(&fw_lock);
 	buf = fw_priv->buf;
-	if (!buf || test_bit(FW_STATUS_DONE, &buf->status)) {
+	if (!buf || fw_status_is_done(&buf->fw_st)) {
 		ret_count = -ENODEV;
 		goto out;
 	}
@@ -842,7 +889,7 @@ static ssize_t firmware_data_write(struct file *filp, struct kobject *kobj,
 
 	mutex_lock(&fw_lock);
 	buf = fw_priv->buf;
-	if (!buf || test_bit(FW_STATUS_DONE, &buf->status)) {
+	if (!buf || fw_status_is_done(&buf->fw_st)) {
 		retval = -ENODEV;
 		goto out;
 	}
@@ -955,8 +1002,7 @@ static int _request_firmware_load(struct firmware_priv *fw_priv,
 		timeout = MAX_JIFFY_OFFSET;
 	}
 
-	retval = wait_for_completion_interruptible_timeout(&buf->completion,
-			timeout);
+	retval = fw_status_wait_timeout(&buf->fw_st, timeout);
 	if (retval == -ERESTARTSYS || !retval) {
 		mutex_lock(&fw_lock);
 		fw_load_abort(fw_priv);
@@ -965,7 +1011,7 @@ static int _request_firmware_load(struct firmware_priv *fw_priv,
 		retval = 0;
 	}
 
-	if (is_fw_load_aborted(buf))
+	if (fw_status_is_aborted(&buf->fw_st))
 		retval = -EAGAIN;
 	else if (buf->is_paged_buf && !buf->data)
 		retval = -ENOMEM;
@@ -1015,35 +1061,12 @@ fw_load_from_user_helper(struct firmware *firmware, const char *name,
 	return -ENOENT;
 }
 
-/* No abort during direct loading */
-#define is_fw_load_aborted(buf) false
-
 #ifdef CONFIG_PM_SLEEP
 static inline void kill_requests_without_uevent(void) { }
 #endif
 
 #endif /* CONFIG_FW_LOADER_USER_HELPER */
 
-
-/* wait until the shared firmware_buf becomes ready (or error) */
-static int sync_cached_firmware_buf(struct firmware_buf *buf)
-{
-	int ret = 0;
-
-	mutex_lock(&fw_lock);
-	while (!test_bit(FW_STATUS_DONE, &buf->status)) {
-		if (is_fw_load_aborted(buf)) {
-			ret = -ENOENT;
-			break;
-		}
-		mutex_unlock(&fw_lock);
-		ret = wait_for_completion_interruptible(&buf->completion);
-		mutex_lock(&fw_lock);
-	}
-	mutex_unlock(&fw_lock);
-	return ret;
-}
-
 /* prepare firmware and firmware_buf structs;
  * return 0 if a firmware is already assigned, 1 if need to load one,
  * or a negative error code
@@ -1077,7 +1100,8 @@ _request_firmware_prepare(struct firmware **firmware_p, const char *name,
 	firmware->priv = buf;
 
 	if (ret > 0) {
-		ret = sync_cached_firmware_buf(buf);
+		ret = fw_status_wait_timeout(&buf->fw_st,
+					   firmware_loading_timeout());
 		if (!ret) {
 			fw_set_page_data(buf, firmware);
 			return 0; /* assigned */
@@ -1095,7 +1119,7 @@ static int assign_firmware_buf(struct firmware *fw, struct device *device,
 	struct firmware_buf *buf = fw->priv;
 
 	mutex_lock(&fw_lock);
-	if (!buf->size || is_fw_load_aborted(buf)) {
+	if (!buf->size || fw_status_is_aborted(&buf->fw_st)) {
 		mutex_unlock(&fw_lock);
 		return -ENOENT;
 	}
-- 
2.7.4

Re: [PATCH v1] firmware_class: encapsulate firmware loading status

[Luis R. Rodriguez]

On Thu, Aug 04, 2016 at 02:27:16PM +0200, Daniel Wagner wrote:
> From: Daniel Wagner <daniel.wagner@bmw-carit.de>
> 
> The firmware user helper code tracks the current state of the loading
> process via an member of struct firmware_buf and a completion. Let's
> encapsulate this simple state machine into struct fw_status. The aim is
> to encrease readiblity and reduce the usage of the fw_lock.

Great, emphasis, reduce use of fw_lock, good stuff!

> The fw_lock is not needed to protect the status update anymore. We don't
> do any RMW operations. Instead we just do a write or a read, not both at
> the same time.
> 
> [v1: moved fw_status into !CONFIG_FW_LOADER_USER_HELPER section,
>      reported by 0day kbuild]
> 
> Signed-off-by: Daniel Wagner <daniel.wagner@bmw-carit.de>
> Cc: Ming Lei <ming.lei@canonical.com>
> Cc: Luis R. Rodriguez <mcgrof@kernel.org>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> ---
> 
> Hi,
> 
> In [0] we have a discussion on how the firmware_class API might be
> changed to improve the current handling of firmware loading. This
> patch was part of the orignal RFC which triggered the discussion.
> 
> I think it is worth taking this one anyway. Maybe as I suggested it
> could be part of the series from Luis.

I'm happy to queue it in on my end however your changes are a bit orthogonal
as you help optimize us away from the usermode helper, I just compartamentalize
that whole API away into a new one so this can go in separately. In terms of
coordination -- sure order will help to get right so I can queue it in, in
that sense. But we're not yet sure if sysdsata will go in first, and I'm happy
for this to go in first as it does not conflict as its slightly orthogonal.

So order here does not interfere with my series -- lets just review this and
its good lets let it go in.

What you do is strip us further from the user mode helper and that
is a good thing.

My review below.

> It cleans up the code base (okay my opinion) 

You do little to sell this. In fact, if this is OK, it does a good
compartamentalization of a completion and a lock and implicates the
wait stuff only onto the usermoder helper, indeed that's a win
worth documenting on the commit log.

> and removes the
> complete_all() call which is problematic for -rt. complete_all() can
> be used in any context including IRQ.

I see. But in this case the code in question should never run in IRQ context?

> That could lead to unbounded
> work in the IRQ context and that is a no go for -rt.

Is the fear of the call to be used in IRQ context or the waiters to
somehow work in IRQ context somehow. The waiters were sleeping.. so
that I think leaves only the call site of the complete_all() to worry
about, but I can't see that happening in IRQ context. Please
correct me if I'm wrong.

> So here the
> attempt to reduce the number of complete_all() calls where possible.

OK so this is the real motivation.

> I have left this argument out in the commit message because I was told '-rt'
> arguments don't count for inclusion.

Sure, but I appreciate this explanation, thanks for that !

Can you provide a set of commits accepted upstream or on linux-next
where such conversion has been done and accepted as well elsewhere
in the kernel ?

I know its just pending patches for review but this has me thinking, is
the use of async functionality in the sysdata patches kosher for RT ?

> cheers,
> daniel
> 
> [0] http://www.spinics.net/lists/linux-wireless/msg153005.html
> 
> drivers/base/firmware_class.c | 154 ++++++++++++++++++++++++------------------
>  1 file changed, 89 insertions(+), 65 deletions(-)
> 
> diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
> index 22d1760..33eb372 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>
>  
> @@ -91,19 +92,83 @@ static inline bool fw_is_builtin_firmware(const struct firmware *fw)
>  }
>  #endif
>  
> +static int loading_timeout = 60;	/* In seconds */
> +
> +static inline long firmware_loading_timeout(void)
> +{
> +	return loading_timeout > 0 ? loading_timeout * HZ : MAX_JIFFY_OFFSET;
> +}

Seems like we can wrap the above loading_timeout and firmware_loading_timeout onto
CONFIG_FW_LOADER_USER_HELPER -- or provide a helper that returns some
static nonsense value that works for !CONFIG_FW_LOADER_USER_HELPER.

The move of the code above also makes this change harder to review.

> +
>  enum {
> +	FW_STATUS_UNKNOWN,
>  	FW_STATUS_LOADING,
>  	FW_STATUS_DONE,
> -	FW_STATUS_ABORT,
> +	FW_STATUS_ABORTED,
>  };

Come to think of it, even if CONFIG_FW_LOADER_USER_HELPER is enabled
we should only have a need to use this wait crap if an explicit
caller in the kernel requested to use the usermode helper, and as
my patches show there are only 2 of those cases left in the kernel.

To be clear if CONFIG_FW_LOADER_USER_HELPER_FALLBACK (not many distros left) is
set we're stuck and always have to use this, if you only have
CONFIG_FW_LOADER_USER_HELPER (most distros) then only explicit calls
will need this. I really don't want to be adding further crap for the
the user mode helper but since you are optimizing this I think a
compromise here can be to extend the status further for when an explicit
caller with usermode helper is passed, FW_STATUS_WAIT, then we only
really need to wait CONFIG_FW_LOADER_USER_HELPER_FALLBACK or if
FW_STATUS_WAIT is set.

For all intents and purposes this is 0 times for most distros these days
given we have only 2 drivers using this crap explicitly right now. But
I think this can be a separate atomic change.

> -static int loading_timeout = 60;	/* In seconds */
> +#ifdef CONFIG_FW_LOADER_USER_HELPER
> +struct fw_status {
> +	unsigned long status;
> +	struct swait_queue_head wq;
> +};
>  
> -static inline long firmware_loading_timeout(void)
> +static void fw_status_init(struct fw_status *fw_st)
>  {
> -	return loading_timeout > 0 ? loading_timeout * HZ : MAX_JIFFY_OFFSET;
> +	fw_st->status = FW_STATUS_UNKNOWN;
> +	init_swait_queue_head(&fw_st->wq);
> +}
> +
> +static inline bool is_fw_status_done(unsigned long status)
> +{
> +	return status == FW_STATUS_DONE ||
> +	       status == FW_STATUS_ABORTED;
> +}
> +
> +static int fw_status_wait_timeout(struct fw_status *fw_st, long timeout)
> +{
> +	int err;
> +	err = swait_event_interruptible_timeout(fw_st->wq,
> +				is_fw_status_done(READ_ONCE(fw_st->status)),
> +				timeout);
> +	if (err == 0 && fw_st->status == FW_STATUS_ABORTED)
> +		return -ENOENT;
> +
> +	return err;
> +}
> +
> +static void __fw_status_set(struct fw_status *fw_st,
> +			  unsigned long status)
> +{
> +	WRITE_ONCE(fw_st->status, status);
> +
> +	if (status == FW_STATUS_DONE ||
> +			status == FW_STATUS_ABORTED)
> +		swake_up(&fw_st->wq);
> +}
> +
> +static unsigned long __fw_status_get(struct fw_status *fw_st)
> +{
> +	return READ_ONCE(fw_st->status);
>  }
>  
> +#define fw_status_start(fw_st)					\
> +	__fw_status_set(fw_st, FW_STATUS_LOADING)
> +#define fw_status_done(fw_st)					\
> +	__fw_status_set(fw_st, FW_STATUS_DONE)
> +#define fw_status_aborted(fw_st)				\
> +	__fw_status_set(fw_st, FW_STATUS_ABORTED)
> +#define fw_status_is_loading(fw_st)				\
> +	(__fw_status_get(fw_st) == FW_STATUS_LOADING)
> +#define fw_status_is_done(fw_st)				\
> +	(__fw_status_get(fw_st) == FW_STATUS_DONE)
> +#define fw_status_is_aborted(fw_st)				\
> +	(__fw_status_get(fw_st) == FW_STATUS_ABORTED)
> +#else
> +#define fw_status_wait_timeout(fw_st, timeout)	0
> +#define fw_status_done(fw_st)
> +#define fw_status_is_aborted(fw_st)		false
> +#endif /* CONFIG_FW_LOADER_USER_HELPER */
> +
>  /* firmware behavior options */
>  #define FW_OPT_UEVENT	(1U << 0)
>  #define FW_OPT_NOWAIT	(1U << 1)
> @@ -145,13 +210,12 @@ struct firmware_cache {
>  struct firmware_buf {
>  	struct kref ref;
>  	struct list_head list;
> -	struct completion completion;
>  	struct firmware_cache *fwc;
> -	unsigned long status;
>  	void *data;
>  	size_t size;
>  	size_t allocated_size;
>  #ifdef CONFIG_FW_LOADER_USER_HELPER
> +	struct fw_status fw_st;
>  	bool is_paged_buf;
>  	bool need_uevent;
>  	struct page **pages;
> @@ -205,8 +269,8 @@ static struct firmware_buf *__allocate_fw_buf(const char *fw_name,
>  	buf->fwc = fwc;
>  	buf->data = dbuf;
>  	buf->allocated_size = size;
> -	init_completion(&buf->completion);
>  #ifdef CONFIG_FW_LOADER_USER_HELPER
> +	fw_status_init(&buf->fw_st);
>  	INIT_LIST_HEAD(&buf->pending_list);
>  #endif
>  
> @@ -305,15 +369,6 @@ static const char * const fw_path[] = {
>  module_param_string(path, fw_path_para, sizeof(fw_path_para), 0644);
>  MODULE_PARM_DESC(path, "customized firmware image search path with a higher priority than default path");
>  
> -static void fw_finish_direct_load(struct device *device,
> -				  struct firmware_buf *buf)
> -{
> -	mutex_lock(&fw_lock);
> -	set_bit(FW_STATUS_DONE, &buf->status);
> -	complete_all(&buf->completion);
> -	mutex_unlock(&fw_lock);
> -}
> -
>  static int
>  fw_get_filesystem_firmware(struct device *device, struct firmware_buf *buf)
>  {
> @@ -360,7 +415,7 @@ fw_get_filesystem_firmware(struct device *device, struct firmware_buf *buf)
>  		}
>  		dev_dbg(device, "direct-loading %s\n", buf->fw_id);
>  		buf->size = size;
> -		fw_finish_direct_load(device, buf);
> +		fw_status_done(&buf->fw_st);
>  		break;
>  	}
>  	__putname(path);
> @@ -478,12 +533,11 @@ static void __fw_load_abort(struct firmware_buf *buf)
>  	 * There is a small window in which user can write to 'loading'
>  	 * between loading done and disappearance of 'loading'
>  	 */
> -	if (test_bit(FW_STATUS_DONE, &buf->status))
> +	if (fw_status_is_done(&buf->fw_st))
>  		return;
>  
>  	list_del_init(&buf->pending_list);
> -	set_bit(FW_STATUS_ABORT, &buf->status);
> -	complete_all(&buf->completion);
> +	fw_status_aborted(&buf->fw_st);
>  }
>  
>  static void fw_load_abort(struct firmware_priv *fw_priv)
> @@ -496,9 +550,6 @@ static void fw_load_abort(struct firmware_priv *fw_priv)
>  	fw_priv->buf = NULL;
>  }
>  
> -#define is_fw_load_aborted(buf)	\
> -	test_bit(FW_STATUS_ABORT, &(buf)->status)
> -
>  static LIST_HEAD(pending_fw_head);
>  
>  /* reboot notifier for avoid deadlock with usermode_lock */
> @@ -596,10 +647,8 @@ static ssize_t firmware_loading_show(struct device *dev,
>  	struct firmware_priv *fw_priv = to_firmware_priv(dev);
>  	int loading = 0;
>  
> -	mutex_lock(&fw_lock);
>  	if (fw_priv->buf)
> -		loading = test_bit(FW_STATUS_LOADING, &fw_priv->buf->status);
> -	mutex_unlock(&fw_lock);
> +		loading = fw_status_is_loading(&fw_priv->buf->fw_st);
>  
>  	return sprintf(buf, "%d\n", loading);
>  }
> @@ -653,23 +702,20 @@ static ssize_t firmware_loading_store(struct device *dev,
>  	switch (loading) {
>  	case 1:
>  		/* discarding any previous partial load */
> -		if (!test_bit(FW_STATUS_DONE, &fw_buf->status)) {
> +		if (!fw_status_is_done(&fw_buf->fw_st)) {
>  			for (i = 0; i < fw_buf->nr_pages; i++)
>  				__free_page(fw_buf->pages[i]);
>  			vfree(fw_buf->pages);
>  			fw_buf->pages = NULL;
>  			fw_buf->page_array_size = 0;
>  			fw_buf->nr_pages = 0;
> -			set_bit(FW_STATUS_LOADING, &fw_buf->status);
> +			fw_status_start(&fw_buf->fw_st);
>  		}
>  		break;
>  	case 0:
> -		if (test_bit(FW_STATUS_LOADING, &fw_buf->status)) {
> +		if (fw_status_is_loading(&fw_buf->fw_st)) {
>  			int rc;
>  
> -			set_bit(FW_STATUS_DONE, &fw_buf->status);
> -			clear_bit(FW_STATUS_LOADING, &fw_buf->status);
> -
>  			/*
>  			 * Several loading requests may be pending on
>  			 * one same firmware buf, so let all requests
> @@ -691,10 +737,11 @@ static ssize_t firmware_loading_store(struct device *dev,
>  			 */
>  			list_del_init(&fw_buf->pending_list);
>  			if (rc) {
> -				set_bit(FW_STATUS_ABORT, &fw_buf->status);
> +				fw_status_aborted(&fw_buf->fw_st);
>  				written = rc;
> +			} else {
> +				fw_status_done(&fw_buf->fw_st);
>  			}
> -			complete_all(&fw_buf->completion);
>  			break;
>  		}
>  		/* fallthrough */
> @@ -702,7 +749,7 @@ static ssize_t firmware_loading_store(struct device *dev,
>  		dev_err(dev, "%s: unexpected value (%d)\n", __func__, loading);
>  		/* fallthrough */
>  	case -1:
> -		fw_load_abort(fw_priv);
> +		fw_status_aborted(&fw_buf->fw_st);
>  		break;
>  	}
>  out:
> @@ -755,7 +802,7 @@ static ssize_t firmware_data_read(struct file *filp, struct kobject *kobj,
>  
>  	mutex_lock(&fw_lock);
>  	buf = fw_priv->buf;
> -	if (!buf || test_bit(FW_STATUS_DONE, &buf->status)) {
> +	if (!buf || fw_status_is_done(&buf->fw_st)) {
>  		ret_count = -ENODEV;
>  		goto out;
>  	}
> @@ -842,7 +889,7 @@ static ssize_t firmware_data_write(struct file *filp, struct kobject *kobj,
>  
>  	mutex_lock(&fw_lock);
>  	buf = fw_priv->buf;
> -	if (!buf || test_bit(FW_STATUS_DONE, &buf->status)) {
> +	if (!buf || fw_status_is_done(&buf->fw_st)) {
>  		retval = -ENODEV;
>  		goto out;
>  	}
> @@ -955,8 +1002,7 @@ static int _request_firmware_load(struct firmware_priv *fw_priv,
>  		timeout = MAX_JIFFY_OFFSET;
>  	}
>  
> -	retval = wait_for_completion_interruptible_timeout(&buf->completion,
> -			timeout);
> +	retval = fw_status_wait_timeout(&buf->fw_st, timeout);
>  	if (retval == -ERESTARTSYS || !retval) {
>  		mutex_lock(&fw_lock);
>  		fw_load_abort(fw_priv);
> @@ -965,7 +1011,7 @@ static int _request_firmware_load(struct firmware_priv *fw_priv,
>  		retval = 0;
>  	}
>  
> -	if (is_fw_load_aborted(buf))
> +	if (fw_status_is_aborted(&buf->fw_st))
>  		retval = -EAGAIN;
>  	else if (buf->is_paged_buf && !buf->data)
>  		retval = -ENOMEM;
> @@ -1015,35 +1061,12 @@ fw_load_from_user_helper(struct firmware *firmware, const char *name,
>  	return -ENOENT;
>  }
>  
> -/* No abort during direct loading */
> -#define is_fw_load_aborted(buf) false
> -
>  #ifdef CONFIG_PM_SLEEP
>  static inline void kill_requests_without_uevent(void) { }
>  #endif
>  
>  #endif /* CONFIG_FW_LOADER_USER_HELPER */
>  
> -
> -/* wait until the shared firmware_buf becomes ready (or error) */
> -static int sync_cached_firmware_buf(struct firmware_buf *buf)
> -{
> -	int ret = 0;
> -
> -	mutex_lock(&fw_lock);
> -	while (!test_bit(FW_STATUS_DONE, &buf->status)) {
> -		if (is_fw_load_aborted(buf)) {
> -			ret = -ENOENT;
> -			break;
> -		}
> -		mutex_unlock(&fw_lock);
> -		ret = wait_for_completion_interruptible(&buf->completion);
> -		mutex_lock(&fw_lock);
> -	}
> -	mutex_unlock(&fw_lock);
> -	return ret;
> -}
> -
>  /* prepare firmware and firmware_buf structs;
>   * return 0 if a firmware is already assigned, 1 if need to load one,
>   * or a negative error code
> @@ -1077,7 +1100,8 @@ _request_firmware_prepare(struct firmware **firmware_p, const char *name,
>  	firmware->priv = buf;
>  
>  	if (ret > 0) {
> -		ret = sync_cached_firmware_buf(buf);
> +		ret = fw_status_wait_timeout(&buf->fw_st,
> +					   firmware_loading_timeout());
>  		if (!ret) {
>  			fw_set_page_data(buf, firmware);
>  			return 0; /* assigned */
> @@ -1095,7 +1119,7 @@ static int assign_firmware_buf(struct firmware *fw, struct device *device,
>  	struct firmware_buf *buf = fw->priv;
>  
>  	mutex_lock(&fw_lock);
> -	if (!buf->size || is_fw_load_aborted(buf)) {
> +	if (!buf->size || fw_status_is_aborted(&buf->fw_st)) {
>  		mutex_unlock(&fw_lock);
>  		return -ENOENT;
>  	}

Other than that this looks nice so far. Can you please run the  tests

tools/testing/selftests/firmware/fw_filesystem.sh
tools/testing/selftests/firmware/fw_userhelper.sh

against lib/test_firmware.c

  Luis

Re: [PATCH v1] firmware_class: encapsulate firmware loading status

[Daniel Wagner]

On 10.08.2016 03:10, Luis R. Rodriguez wrote:
> On Thu, Aug 04, 2016 at 02:27:16PM +0200, Daniel Wagner wrote:
>> From: Daniel Wagner <daniel.wagner@bmw-carit.de>

>> In [0] we have a discussion on how the firmware_class API might be
>> changed to improve the current handling of firmware loading. This
>> patch was part of the orignal RFC which triggered the discussion.
>>
>> I think it is worth taking this one anyway. Maybe as I suggested it
>> could be part of the series from Luis.
> 
> I'm happy to queue it in on my end however your changes are a bit orthogonal
> as you help optimize us away from the usermode helper, I just compartamentalize
> that whole API away into a new one so this can go in separately. 

Okay.

> In terms of
> coordination -- sure order will help to get right so I can queue it in, in
> that sense. But we're not yet sure if sysdsata will go in first, and I'm happy
> for this to go in first as it does not conflict as its slightly orthogonal.
> 
> So order here does not interfere with my series -- lets just review this and
> its good lets let it go in.

Alright, let's do this then first.

> What you do is strip us further from the user mode helper and that
> is a good thing.
> 
> My review below.
> 
>> It cleans up the code base (okay my opinion) 
> 
> You do little to sell this. In fact, if this is OK, it does a good
> compartamentalization of a completion and a lock and implicates the
> wait stuff only onto the usermoder helper, indeed that's a win
> worth documenting on the commit log.

My selling skill has still a lot of room to grow. I'll add your points
to the commit message.

>> and removes the
>> complete_all() call which is problematic for -rt. complete_all() can
>> be used in any context including IRQ.
> 
> I see. But in this case the code in question should never run in IRQ context?

No, this code is not running in IRQ context. See below.

>> That could lead to unbounded
>> work in the IRQ context and that is a no go for -rt.
> 
> Is the fear of the call to be used in IRQ context or the waiters to
> somehow work in IRQ context somehow. The waiters were sleeping.. so
> that I think leaves only the call site of the complete_all() to worry
> about, but I can't see that happening in IRQ context. Please
> correct me if I'm wrong.

The only problem for -rt is if the complete_all() happens in IRQ
context. If that happens the waker wakes up all waiters in one go (in
IRQ). That leads to the 'unbounded work' which can't be preempted. There
is no further restriction for -rt on waiters or wakers.

>> So here the
>> attempt to reduce the number of complete_all() calls where possible.
> 
> OK so this is the real motivation.

Yes, this is more ore less a clean up work :)

>> I have left this argument out in the commit message because I was told '-rt'
>> arguments don't count for inclusion.
> 
> Sure, but I appreciate this explanation, thanks for that !
> 
> Can you provide a set of commits accepted upstream or on linux-next
> where such conversion has been done and accepted as well elsewhere
> in the kernel ?

Not so far. I have started to send out patches last week. It seems most
people are enjoying holiday.

https://lkml.org/lkml/2016/8/4/264
https://patchwork.kernel.org/project/linux-amlogic/list/?submitter=47731

> I know its just pending patches for review but this has me thinking, is
> the use of async functionality in the sysdata patches kosher for RT ?

I haven't looked into deeply but from what I saw there is no obvious
show stopper. I'll give it another look next week. I am AFK till next
week ;)


> 
>> cheers,
>> daniel
>>
>> [0] http://www.spinics.net/lists/linux-wireless/msg153005.html
>>
>> drivers/base/firmware_class.c | 154 ++++++++++++++++++++++++------------------
>>  1 file changed, 89 insertions(+), 65 deletions(-)
>>
>> diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
>> index 22d1760..33eb372 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>
>>  
>> @@ -91,19 +92,83 @@ static inline bool fw_is_builtin_firmware(const struct firmware *fw)
>>  }
>>  #endif
>>  
>> +static int loading_timeout = 60;	/* In seconds */
>> +
>> +static inline long firmware_loading_timeout(void)
>> +{
>> +	return loading_timeout > 0 ? loading_timeout * HZ : MAX_JIFFY_OFFSET;
>> +}
> 
> Seems like we can wrap the above loading_timeout and firmware_loading_timeout onto
> CONFIG_FW_LOADER_USER_HELPER -- or provide a helper that returns some
> static nonsense value that works for !CONFIG_FW_LOADER_USER_HELPER.
> 
> The move of the code above also makes this change harder to review.

Yeah, I moved it up because in one version I had a dependency to
firmware_loading_timeout. Hmm, maybe it can be removed or integrated a
bit more tightly.

[...]

> Other than that this looks nice so far. Can you please run the  tests
> 
> tools/testing/selftests/firmware/fw_filesystem.sh
> tools/testing/selftests/firmware/fw_userhelper.sh
> 
> against lib/test_firmware.c

I've done this. After this change, they are still passing. Obviously,
after patching those two tests. I came up with almost the identical
patches as you have in your queue.

cheers,
daniel

Re: [PATCH v1] firmware_class: encapsulate firmware loading status

[Luis R. Rodriguez]

On Wed, Aug 10, 2016 at 09:02:08AM +0200, Daniel Wagner wrote:
> On 10.08.2016 03:10, Luis R. Rodriguez wrote:
> > On Thu, Aug 04, 2016 at 02:27:16PM +0200, Daniel Wagner wrote:
> >> From: Daniel Wagner <daniel.wagner@bmw-carit.de>
> > I see. But in this case the code in question should never run in IRQ context?
> 
> No, this code is not running in IRQ context. See below.

OK so even for RT this is not needed then. Is that right ?

If this is true there must be some gains of swait over old wait
API even if its not important for -rt, what are the selling points,
in summary ?

> >> That could lead to unbounded
> >> work in the IRQ context and that is a no go for -rt.
> > 
> > Is the fear of the call to be used in IRQ context or the waiters to
> > somehow work in IRQ context somehow. The waiters were sleeping.. so
> > that I think leaves only the call site of the complete_all() to worry
> > about, but I can't see that happening in IRQ context. Please
> > correct me if I'm wrong.
> 
> The only problem for -rt is if the complete_all() happens in IRQ
> context. If that happens the waker wakes up all waiters in one go (in
> IRQ). That leads to the 'unbounded work' which can't be preempted. There
> is no further restriction for -rt on waiters or wakers.

In that case, even when -rt, this is not needed. However the compartamentalizing
of usermode sleep crap to usermode helper only seems worthy endeavor and I
wonder if we can split the work in this patch to 2, one which splits the
stuff, and the other one that makes then the conversion from old wait to
the new swait. If this is possible there are three gains:

 o makes code easier to review
 o makes each change atomically justifiable
 o once you have only a conversion from old wait to new swait you can
   inspect the delta and try to write SmPL grammar to see if you can
   generalize the change, so grammar can do the change for other
   use cases. Of course, you'd need first to look for the IRQ context,
   and I wonder if that's possible. If there are however generic
   benefits of swait over old wait when complete_all() is used (is
   live patching one?) then this will be very handy.

> >> So here the
> >> attempt to reduce the number of complete_all() calls where possible.
> > 
> > OK so this is the real motivation.
> 
> Yes, this is more ore less a clean up work :)
> 
> >> I have left this argument out in the commit message because I was told '-rt'
> >> arguments don't count for inclusion.
> > 
> > Sure, but I appreciate this explanation, thanks for that !
> > 
> > Can you provide a set of commits accepted upstream or on linux-next
> > where such conversion has been done and accepted as well elsewhere
> > in the kernel ?
> 
> Not so far. I have started to send out patches last week. It seems most
> people are enjoying holiday.
> 
> https://lkml.org/lkml/2016/8/4/264
> https://patchwork.kernel.org/project/linux-amlogic/list/?submitter=47731

OK thanks do we have a kselftest for swait ?

> > I know its just pending patches for review but this has me thinking, is
> > the use of async functionality in the sysdata patches kosher for RT ?
> 
> I haven't looked into deeply but from what I saw there is no obvious
> show stopper. I'll give it another look next week. I am AFK till next
> week ;)

OK thanks.

> >> cheers,
> >> daniel
> >>
> >> [0] http://www.spinics.net/lists/linux-wireless/msg153005.html
> >>
> >> drivers/base/firmware_class.c | 154 ++++++++++++++++++++++++------------------
> >>  1 file changed, 89 insertions(+), 65 deletions(-)
> >>
> >> diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
> >> index 22d1760..33eb372 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>
> >>  
> >> @@ -91,19 +92,83 @@ static inline bool fw_is_builtin_firmware(const struct firmware *fw)
> >>  }
> >>  #endif
> >>  
> >> +static int loading_timeout = 60;	/* In seconds */
> >> +
> >> +static inline long firmware_loading_timeout(void)
> >> +{
> >> +	return loading_timeout > 0 ? loading_timeout * HZ : MAX_JIFFY_OFFSET;
> >> +}
> > 
> > Seems like we can wrap the above loading_timeout and firmware_loading_timeout onto
> > CONFIG_FW_LOADER_USER_HELPER -- or provide a helper that returns some
> > static nonsense value that works for !CONFIG_FW_LOADER_USER_HELPER.
> > 
> > The move of the code above also makes this change harder to review.
> 
> Yeah, I moved it up because in one version I had a dependency to
> firmware_loading_timeout. Hmm, maybe it can be removed or integrated a
> bit more tightly.
> 
> [...]
> 
> > Other than that this looks nice so far. Can you please run the  tests
> > 
> > tools/testing/selftests/firmware/fw_filesystem.sh
> > tools/testing/selftests/firmware/fw_userhelper.sh
> > 
> > against lib/test_firmware.c
> 
> I've done this. After this change, they are still passing. Obviously,
> after patching those two tests. I came up with almost the identical
> patches as you have in your queue.

OK great. Thanks. In that case just the above observations are pending.

  Luis

Re: [PATCH v1] firmware_class: encapsulate firmware loading status

[Daniel Wagner]

Hi Luis,

On 08/10/2016 08:52 PM, Luis R. Rodriguez wrote:
> On Wed, Aug 10, 2016 at 09:02:08AM +0200, Daniel Wagner wrote:
>> On 10.08.2016 03:10, Luis R. Rodriguez wrote:
>>> On Thu, Aug 04, 2016 at 02:27:16PM +0200, Daniel Wagner wrote:
>>>> From: Daniel Wagner <daniel.wagner@bmw-carit.de>
>>> I see. But in this case the code in question should never run in IRQ context?
>>
>> No, this code is not running in IRQ context. See below.
>
> OK so even for RT this is not needed then. Is that right ?
>
> If this is true there must be some gains of swait over old wait
> API even if its not important for -rt, what are the selling points,
> in summary ?

Clearly I need to improve my commit message writing. Your observation is 
correct.

The current 'state machine' uses three variables to handle the state and 
the transitions.

struct completion {
	unsigned int done;
	wait_queue_head_t wait;
};

struct firmware_buf {
	...
	struct completion completion;
	unsigned long status;
	...
};

Obviously, the variable 'status' holds the state. 'wait' and 'done' 
handles the synchronization. 'done' remembers how many waiters will be 
woken at max. complete_all() sets it to UMAX/2. That should be enough in 
most of the cases. So any future wait_for_completion() call will not block.

The patch just drops the 'done' completely because it is not necessary. 
We have a waiter queue for all those pending waiters and as soon the 
final state is reached we just wake them up. The future waiters will 
never be queued because we just check for the state first.

wait vs swait: The main difference between the two APIs is the 
implementation. So it is pretty simple to switch from one to the other. 
So why swait, I hear you asking. The swait implentation is pretty simple 
for the price that you can't do all the stuff what wait offers. As long 
you don't need the extra features of wait just go with swait.

While the above points are nice side effect the real reason is the 
cleanup of the code and getting rid of the mutex operations.

>>>> That could lead to unbounded
>>>> work in the IRQ context and that is a no go for -rt.
>>>
>>> Is the fear of the call to be used in IRQ context or the waiters to
>>> somehow work in IRQ context somehow. The waiters were sleeping.. so
>>> that I think leaves only the call site of the complete_all() to worry
>>> about, but I can't see that happening in IRQ context. Please
>>> correct me if I'm wrong.
>>
>> The only problem for -rt is if the complete_all() happens in IRQ
>> context. If that happens the waker wakes up all waiters in one go (in
>> IRQ). That leads to the 'unbounded work' which can't be preempted. There
>> is no further restriction for -rt on waiters or wakers.
>
> In that case, even when -rt, this is not needed. However the compartamentalizing
> of usermode sleep crap to usermode helper only seems worthy endeavor and I
> wonder if we can split the work in this patch to 2, one which splits the
> stuff, and the other one that makes then the conversion from old wait to
> the new swait. If this is possible there are three gains:
>
>  o makes code easier to review
>  o makes each change atomically justifiable

I can try to split the patch into two steps. Let's see how this works 
out. But I wouldn't mind if we go with this version :)

>  o once you have only a conversion from old wait to new swait you can
>    inspect the delta and try to write SmPL grammar to see if you can
>    generalize the change, so grammar can do the change for other
>    use cases. Of course, you'd need first to look for the IRQ context,
>    and I wonder if that's possible. If there are however generic
>    benefits of swait over old wait when complete_all() is used (is
>    live patching one?) then this will be very handy.

 From my attempts to figure out the execution context with SmPL I fear 
that is rather hard to achieve because you need to create a call graph 
and track the state.

>>>> So here the
>>>> attempt to reduce the number of complete_all() calls where possible.
>>>
>>> OK so this is the real motivation.
>>
>> Yes, this is more ore less a clean up work :)
>>
>>>> I have left this argument out in the commit message because I was told '-rt'
>>>> arguments don't count for inclusion.
>>>
>>> Sure, but I appreciate this explanation, thanks for that !
>>>
>>> Can you provide a set of commits accepted upstream or on linux-next
>>> where such conversion has been done and accepted as well elsewhere
>>> in the kernel ?
>>
>> Not so far. I have started to send out patches last week. It seems most
>> people are enjoying holiday.
>>
>> https://lkml.org/lkml/2016/8/4/264
>> https://patchwork.kernel.org/project/linux-amlogic/list/?submitter=47731
>
> OK thanks do we have a kselftest for swait ?

No. A quick grep didn't show any test for wait either. I should still 
have some test code around for swait while hacking on it. I'll add it to 
my todo list if you think that is a worthwhile exercise.

cheers,
daniel

Re: [PATCH v1] firmware_class: encapsulate firmware loading status

[Luis R. Rodriguez]

On Wed, Aug 17, 2016 at 08:47:24AM +0200, Daniel Wagner wrote:
> Hi Luis,
> 
> On 08/10/2016 08:52 PM, Luis R. Rodriguez wrote:
> >On Wed, Aug 10, 2016 at 09:02:08AM +0200, Daniel Wagner wrote:
> >>On 10.08.2016 03:10, Luis R. Rodriguez wrote:
> >>>On Thu, Aug 04, 2016 at 02:27:16PM +0200, Daniel Wagner wrote:
> >>>>From: Daniel Wagner <daniel.wagner@bmw-carit.de>
> >>>I see. But in this case the code in question should never run in IRQ context?
> >>
> >>No, this code is not running in IRQ context. See below.
> >
> >OK so even for RT this is not needed then. Is that right ?
> >
> >If this is true there must be some gains of swait over old wait
> >API even if its not important for -rt, what are the selling points,
> >in summary ?
> 
> Clearly I need to improve my commit message writing. Your
> observation is correct.
> 
> The current 'state machine' uses three variables to handle the state
> and the transitions.
> 
> struct completion {
> 	unsigned int done;
> 	wait_queue_head_t wait;
> };
> 
> struct firmware_buf {
> 	...
> 	struct completion completion;
> 	unsigned long status;
> 	...
> };
> 
> Obviously, the variable 'status' holds the state. 'wait' and 'done'
> handles the synchronization. 'done' remembers how many waiters will
> be woken at max. complete_all() sets it to UMAX/2. That should be
> enough in most of the cases. 

Thanks, this helps and makes sense. How many data structures
in comparison does the new swait require ? Is it smaller ? If
so that is a nice simplification indeed, however we should make
sure we have no compromises then.

> So any future wait_for_completion() call will not block.

This I don't get, do you mean that if we have already UMAX/2
waiters on a completion and another one comes in, it will not
wait at all ?

Is this documented well ? Either way clarifying exactly what is
done here would be of huge help understanding the striking
differences between a switch to the new API.

> The patch just drops the 'done' completely because it is not
> necessary. We have a waiter queue for all those pending waiters 

So there is no limit to waiters with the new API ?

> and
> as soon the final state is reached we just wake them up. The future
> waiters will never be queued because we just check for the state
> first.

I do not follow what this means, I take it here we are talking about
possible race conditions between a wait and some work about to be
done?

> wait vs swait: The main difference between the two APIs is the
> implementation. So it is pretty simple to switch from one to the
> other. So why swait, I hear you asking. The swait implentation is
> pretty simple for the price that you can't do all the stuff what
> wait offers. As long you don't need the extra features of wait just
> go with swait.

OK so wait offers more features and its a kitchen sink of stuff,
we only require a simple wait and swait is better and more light
weight. The above number of waiters is still something I'd like
a bit clarification on.

> While the above points are nice side effect the real reason is the
> cleanup of the code and getting rid of the mutex operations.

This indeed is huge and this can better be reflected on the commit log.
In fact I wonder if its possible to do the switch without the change
to swait, and do the conversion to swait as a secondary step.

> >>>>That could lead to unbounded
> >>>>work in the IRQ context and that is a no go for -rt.
> >>>
> >>>Is the fear of the call to be used in IRQ context or the waiters to
> >>>somehow work in IRQ context somehow. The waiters were sleeping.. so
> >>>that I think leaves only the call site of the complete_all() to worry
> >>>about, but I can't see that happening in IRQ context. Please
> >>>correct me if I'm wrong.
> >>
> >>The only problem for -rt is if the complete_all() happens in IRQ
> >>context. If that happens the waker wakes up all waiters in one go (in
> >>IRQ). That leads to the 'unbounded work' which can't be preempted. There
> >>is no further restriction for -rt on waiters or wakers.
> >
> >In that case, even when -rt, this is not needed. However the compartamentalizing
> >of usermode sleep crap to usermode helper only seems worthy endeavor and I
> >wonder if we can split the work in this patch to 2, one which splits the
> >stuff, and the other one that makes then the conversion from old wait to
> >the new swait. If this is possible there are three gains:
> >
> > o makes code easier to review
> > o makes each change atomically justifiable
> 
> I can try to split the patch into two steps. Let's see how this
> works out. But I wouldn't mind if we go with this version :)

I understand -- however I have to ask as if its possible it makes
things easier to review and makes two logical changes split up. This
would in turn be easier to debug if there are issues.

> > o once you have only a conversion from old wait to new swait you can
> >   inspect the delta and try to write SmPL grammar to see if you can
> >   generalize the change, so grammar can do the change for other
> >   use cases. Of course, you'd need first to look for the IRQ context,
> >   and I wonder if that's possible. If there are however generic
> >   benefits of swait over old wait when complete_all() is used (is
> >   live patching one?) then this will be very handy.
> 
> From my attempts to figure out the execution context with SmPL I
> fear that is rather hard to achieve because you need to create a
> call graph and track the state.

OK..

> >>>>So here the
> >>>>attempt to reduce the number of complete_all() calls where possible.
> >>>
> >>>OK so this is the real motivation.
> >>
> >>Yes, this is more ore less a clean up work :)
> >>
> >>>>I have left this argument out in the commit message because I was told '-rt'
> >>>>arguments don't count for inclusion.
> >>>
> >>>Sure, but I appreciate this explanation, thanks for that !
> >>>
> >>>Can you provide a set of commits accepted upstream or on linux-next
> >>>where such conversion has been done and accepted as well elsewhere
> >>>in the kernel ?
> >>
> >>Not so far. I have started to send out patches last week. It seems most
> >>people are enjoying holiday.
> >>
> >>https://lkml.org/lkml/2016/8/4/264
> >>https://patchwork.kernel.org/project/linux-amlogic/list/?submitter=47731
> >
> >OK thanks do we have a kselftest for swait ?
> 
> No. A quick grep didn't show any test for wait either. I should
> still have some test code around for swait while hacking on it. I'll
> add it to my todo list if you think that is a worthwhile exercise.

Definitely!

  Luis

Re: [PATCH v1] firmware_class: encapsulate firmware loading status

[Daniel Wagner]

On 18.08.2016 18:30, Luis R. Rodriguez wrote:
> On Wed, Aug 17, 2016 at 08:47:24AM +0200, Daniel Wagner wrote:
>> On 08/10/2016 08:52 PM, Luis R. Rodriguez wrote:
>> The current 'state machine' uses three variables to handle the state
>> and the transitions.
>>
>> struct completion {
>> 	unsigned int done;
>> 	wait_queue_head_t wait;
>> };
>>
>> struct firmware_buf {
>> 	...
>> 	struct completion completion;
>> 	unsigned long status;
>> 	...
>> };
>>
>> Obviously, the variable 'status' holds the state. 'wait' and 'done'
>> handles the synchronization. 'done' remembers how many waiters will
>> be woken at max. complete_all() sets it to UMAX/2. That should be
>> enough in most of the cases. 
> 
> Thanks, this helps and makes sense. How many data structures
> in comparison does the new swait require ? Is it smaller ? If
> so that is a nice simplification indeed, however we should make
> sure we have no compromises then.

Yes we save one 'unsigned int', that is the done member of struct
completion. For an earlier version of this patch I did check the size
changes. While we save a little on the data section, the code section
increased slightly, IIRC it was around 60 bytes. Will do another
measurement.

>> So any future wait_for_completion() call will not block.
> 
> This I don't get, do you mean that if we have already UMAX/2
> waiters on a completion and another one comes in, it will not
> wait at all ?

Sorry, I think I just confused you here with a implementation detail.
Whenever wait_for_completion() is woken it checks if done > 0. Then it
will decrement the counter. complete() increases the counter and then
wakes the waiter. Basically it is comparable with semamphore put and get
operation. complete_all() just sets done to max almost infinite value :)

> Is this documented well ? Either way clarifying exactly what is
> done here would be of huge help understanding the striking
> differences between a switch to the new API.

Obviously, there is Documentation/scheduler/completion.txt but the small
detail on UMAX/2 is not mentioned. I don't think it was considered to be
a real problem. I guess before you run into the problem of waking 2
billion threads you see other scaling issues first.

Note this has nothing to do with wait vs swait.

>> The patch just drops the 'done' completely because it is not
>> necessary. We have a waiter queue for all those pending waiters 
> 
> So there is no limit to waiters with the new API ?

Correct, the limit is gone, though I don't expect that there are so many
firmware user helper waiting that we hit the UMAX/2 limit ever.

>> and
>> as soon the final state is reached we just wake them up. The future
>> waiters will never be queued because we just check for the state
>> first.
> 
> I do not follow what this means, I take it here we are talking about
> possible race conditions between a wait and some work about to be
> done?

Let me reword that. I was not really concerned about race condition
here. I was just trying to point out that we just check for the condition.

Either we have reached FW_STATUS_{DONE|ABORTED} and just continue or we
put the thread to sleep and wait for the wake call. Because we check for
the a single condition (status == FW_STATUS_{DONE|ABORTED} in
swait_event_interruptable_timeout() we don't need any addition
synchronization. Come to think about it, that is why the mutex can be
removed.

>> wait vs swait: The main difference between the two APIs is the
>> implementation. So it is pretty simple to switch from one to the
>> other. So why swait, I hear you asking. The swait implentation is
>> pretty simple for the price that you can't do all the stuff what
>> wait offers. As long you don't need the extra features of wait just
>> go with swait.
> 
> OK so wait offers more features and its a kitchen sink of stuff,
> we only require a simple wait and swait is better and more light
> weight.

Yes, that summarized it pretty good.

> The above number of waiters is still something I'd like
> a bit clarification on.

As I understand the firmware loader helper userland API there is only
one waiter.

>> While the above points are nice side effect the real reason is the
>> cleanup of the code and getting rid of the mutex operations.
> 
> This indeed is huge and this can better be reflected on the commit log.
> In fact I wonder if its possible to do the switch without the change
> to swait, and do the conversion to swait as a secondary step.

Not sure about it because 'status' and the operation of completion need
to be synchronized. I'll give it a try just haven't had time yet. It is
not about wait or swait, it's about completion vs s/wait.

>> I can try to split the patch into two steps. Let's see how this
>> works out. But I wouldn't mind if we go with this version :)
> 
> I understand -- however I have to ask as if its possible it makes
> things easier to review and makes two logical changes split up. This
> would in turn be easier to debug if there are issues.

Sure, I completely understand. BTW, I just updated the patch and avoided
the moving of the loading_timeout. Now it doesn't contain any hard to
read section anymore.

>>> o once you have only a conversion from old wait to new swait you can
>>>   inspect the delta and try to write SmPL grammar to see if you can
>>>   generalize the change, so grammar can do the change for other
>>>   use cases. Of course, you'd need first to look for the IRQ context,
>>>   and I wonder if that's possible. If there are however generic
>>>   benefits of swait over old wait when complete_all() is used (is
>>>   live patching one?) then this will be very handy.
>>
>> From my attempts to figure out the execution context with SmPL I
>> fear that is rather hard to achieve because you need to create a
>> call graph and track the state.
> 
> OK..

I know you have a far better understanding. We need to discuss this over
a beer :)

cheers,
daniel