Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1456264 > unrolled thread

[PATCH] firmware_class: encapsulate firmware loading status

Back to article view | Back to linux.kernel

Contents

  [PATCH] firmware_class: encapsulate firmware loading status Daniel Wagner <wagi@monom.org> - 2016-08-04 11:30 +0200
    Re: [PATCH] firmware_class: encapsulate firmware loading status kbuild test robot <lkp@intel.com> - 2016-08-04 12:40 +0200
      [PATCH v1] firmware_class: encapsulate firmware loading status Daniel Wagner <wagi@monom.org> - 2016-08-04 14:30 +0200
        Re: [PATCH v1] firmware_class: encapsulate firmware loading status "Luis R. Rodriguez" <mcgrof@kernel.org> - 2016-08-10 03:20 +0200
          Re: [PATCH v1] firmware_class: encapsulate firmware loading status Daniel Wagner <wagi@monom.org> - 2016-08-10 20:20 +0200
            Re: [PATCH v1] firmware_class: encapsulate firmware loading status "Luis R. Rodriguez" <mcgrof@kernel.org> - 2016-08-10 21:40 +0200

#1456264 — [PATCH] firmware_class: encapsulate firmware loading status

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

[toc] | [next] | [standalone]

#1456316

[Multipart message — attachments visible in raw view] — view raw

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

[toc] | [prev] | [next] | [standalone]

#1456370 — [PATCH v1] firmware_class: encapsulate firmware loading status

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

[toc] | [prev] | [next] | [standalone]

#1459234 — Re: [PATCH v1] firmware_class: encapsulate firmware loading status

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

[toc] | [prev] | [next] | [standalone]

#1459355 — Re: [PATCH v1] firmware_class: encapsulate firmware loading status

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

[toc] | [prev] | [next] | [standalone]

#1459691 — Re: [PATCH v1] firmware_class: encapsulate firmware loading status

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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web