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

Groups > linux.kernel > #1479929 > unrolled thread

[PATCH v5 0/5] firmware: encapsulate firmware loading status

Back to article view | Back to linux.kernel

Contents

  [PATCH v5 0/5] firmware: encapsulate firmware loading status Daniel Wagner <wagi@monom.org> - 2016-09-09 14:20 +0200
    [PATCH v5 3/5] firmware: rename fw_load_from_user_helper() and _request_firmware_load() Daniel Wagner <wagi@monom.org> - 2016-09-09 14:20 +0200
      Re: [PATCH v5 3/5] firmware: rename fw_load_from_user_helper() and  _request_firmware_load() "Luis R. Rodriguez" <mcgrof@kernel.org> - 2016-09-10 00:20 +0200
    [PATCH v5 1/5] firmware: document user mode helper lock usage Daniel Wagner <wagi@monom.org> - 2016-09-09 14:20 +0200
      Re: [PATCH v5 1/5] firmware: document user mode helper lock usage "Luis R. Rodriguez" <mcgrof@kernel.org> - 2016-09-10 00:20 +0200
        Re: [PATCH v5 1/5] firmware: document user mode helper lock usage Ming Lei <ming.lei@canonical.com> - 2016-09-22 04:40 +0200
    [PATCH v5 4/5] firmware: drop bit ops in favor of simple state machine Daniel Wagner <wagi@monom.org> - 2016-09-09 14:20 +0200
      Re: [PATCH v5 4/5] firmware: drop bit ops in favor of simple state  machine "Luis R. Rodriguez" <mcgrof@kernel.org> - 2016-09-10 00:40 +0200
    [PATCH v5 2/5] firmware: encapsulate firmware loading status Daniel Wagner <wagi@monom.org> - 2016-09-09 14:20 +0200
      Re: [PATCH v5 2/5] firmware: encapsulate firmware loading status "Luis R. Rodriguez" <mcgrof@kernel.org> - 2016-09-10 00:20 +0200
      Re: [PATCH v5 2/5] firmware: encapsulate firmware loading status "Luis R. Rodriguez" <mcgrof@kernel.org> - 2016-09-10 00:30 +0200
      Re: [PATCH v5 2/5] firmware: encapsulate firmware loading status Daniel Wagner <daniel.wagner@bmw-carit.de> - 2016-09-13 11:50 +0200
    [PATCH v5 5/5] firmware: do not use fw_lock for fw_umh protection Daniel Wagner <wagi@monom.org> - 2016-09-09 14:20 +0200
    Re: [PATCH v5 0/5] firmware: encapsulate firmware loading status "Luis R. Rodriguez" <mcgrof@kernel.org> - 2016-09-09 19:40 +0200

#1479929 — [PATCH v5 0/5] firmware: encapsulate firmware loading status

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

Hi,

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 increase readability and reduce the usage of the fw_lock.

Luis asked by to add a few CC for getting a wider audience. If you
haven't seen this before the initial version of this series has some
more details on the motivation for this series:

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

I tested this series with fw_userhelper.sh and fw_filesystem.sh under
kvm and also let it run on real hardware. The series is also available
here:

https://git.kernel.org/cgit/linux/kernel/git/wagi/linux.git/log/?h=firmware_async-7

I had pushed earlier version of series there and haven't got any 0-day
bug reports so far.

cheers,
daniel

This series depends on Luis' "firmware: add SmPL grammar to avoid issues"
series:

http://marc.info/?l=linux-kernel&m=147320896231418&w=2

changes since v4:
  - replaced "firmware: Move umh locking code into fw_load_from_user_helper()"
    with "firmware: document user mode helper lock usage"
  - changed prefix fw_status_ to fw_umh_
  - fixed a couple of bux pointed out by Ming
  - changed type of fw_umh::status to u8 and updated commit message to
    point out that all states are exclusive
    

changes since v3:
  - added 'firmware: Move umh locking code into
    fw_load_from_user_helper()'
  - dropped loading_tiemout and firmware_loading_time() for
    !CONFG_FW_LOADER_USER_HELPER
  - rebased on Luis patches

changes since v2:
  - more splitting out
    - first patch factors out all the bit ops into fw_status
    - second patch gets rid of the bit ops
    - third get rid of fw_lock by using swait

changes since v1:
  - moved swait change into its own patch
  - added ifdef section for FW_LOADER_USER_HELPER_FALLBACK
  - updated commit message highlighting the mutex usage drop a bit

  https://lkml.org/lkml/2016/8/4/239

Daniel Wagner (5):
  firmware: document user mode helper lock usage
  firmware: encapsulate firmware loading status
  firmware: rename fw_load_from_user_helper() and
    _request_firmware_load()
  firmware: drop bit ops in favor of simple state machine
  firmware: do not use fw_lock for fw_umh protection

 drivers/base/firmware_class.c | 194 +++++++++++++++++++++++++-----------------
 1 file changed, 118 insertions(+), 76 deletions(-)

-- 
2.7.4

[toc] | [next] | [standalone]

#1479930 — [PATCH v5 3/5] firmware: rename fw_load_from_user_helper() and _request_firmware_load()

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

fw_load_from_user_helper() and _request_firmware_load() are used when
CONFIG_FW_LOADER_USER_HELPER is enabled. In order to clearly mark which
part of the code is depending on UMH we stream line these functions to
match with the rest of the code, e.g. fw_umh_done().

Suggested by Luis.

Cc: Ming Lei <ming.lei@canonical.com>
Cc: Luis R. Rodriguez <mcgrof@kernel.org>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Signed-off-by: Daniel Wagner <daniel.wagner@bmw-carit.de>
---
 drivers/base/firmware_class.c | 23 +++++++++++------------
 1 file changed, 11 insertions(+), 12 deletions(-)

diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
index 821babe..5e38c27 100644
--- a/drivers/base/firmware_class.c
+++ b/drivers/base/firmware_class.c
@@ -979,9 +979,8 @@ fw_create_instance(struct firmware *firmware, const char *fw_name,
 	return fw_priv;
 }
 
-/* load a firmware via user helper */
-static int _request_firmware_load(struct firmware_priv *fw_priv,
-				  unsigned int opt_flags, long timeout)
+static int _request_firmware_umh(struct firmware_priv *fw_priv,
+				 unsigned int opt_flags, long timeout)
 {
 	int retval = 0;
 	struct device *f_dev = &fw_priv->dev;
@@ -1032,9 +1031,9 @@ static int _request_firmware_load(struct firmware_priv *fw_priv,
 	return retval;
 }
 
-static int fw_load_from_user_helper(struct firmware *firmware,
-				    const char *name, struct device *device,
-				    unsigned int opt_flags, long timeout)
+static int fw_get_umh_firmware(struct firmware *firmware, const char *name,
+			struct device *device, unsigned int opt_flags,
+			long timeout)
 {
 	struct firmware_priv *fw_priv;
 
@@ -1043,7 +1042,7 @@ static int fw_load_from_user_helper(struct firmware *firmware,
 		return PTR_ERR(fw_priv);
 
 	fw_priv->buf = firmware->priv;
-	return _request_firmware_load(fw_priv, opt_flags, timeout);
+	return _request_firmware_umh(fw_priv, opt_flags, timeout);
 }
 
 #ifdef CONFIG_PM_SLEEP
@@ -1064,9 +1063,9 @@ static void kill_requests_without_uevent(void)
 
 #else /* CONFIG_FW_LOADER_USER_HELPER */
 static inline int
-fw_load_from_user_helper(struct firmware *firmware, const char *name,
-			 struct device *device, unsigned int opt_flags,
-			 long timeout)
+fw_get_umh_firmware(struct firmware *firmware, const char *name,
+		    struct device *device, unsigned int opt_flags,
+		    long timeout)
 {
 	return -ENOENT;
 }
@@ -1242,8 +1241,8 @@ _request_firmware(const struct firmware **firmware_p, const char *name,
 				 name, ret);
 		if (opt_flags & FW_OPT_USERHELPER) {
 			dev_warn(device, "Falling back to user helper\n");
-			ret = fw_load_from_user_helper(fw, name, device,
-						       opt_flags, timeout);
+			ret = fw_get_umh_firmware(fw, name, device,
+						  opt_flags, timeout);
 		}
 	}
 
-- 
2.7.4

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

#1480389 — Re: [PATCH v5 3/5] firmware: rename fw_load_from_user_helper() and _request_firmware_load()

On Fri, Sep 09, 2016 at 02:12:22PM +0200, Daniel Wagner wrote:
> From: Daniel Wagner <daniel.wagner@bmw-carit.de>
> 
> fw_load_from_user_helper() and _request_firmware_load() are used when
> CONFIG_FW_LOADER_USER_HELPER is enabled. In order to clearly mark which
> part of the code is depending on UMH we stream line these functions to
> match with the rest of the code, e.g. fw_umh_done().
> 
> Suggested by Luis.
> 
> Cc: Ming Lei <ming.lei@canonical.com>
> Cc: Luis R. Rodriguez <mcgrof@kernel.org>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Signed-off-by: Daniel Wagner <daniel.wagner@bmw-carit.de>

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

  Luis

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

#1479933 — [PATCH v5 1/5] firmware: document user mode helper lock usage

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

The lock is also used to generate warnings when a direct
firmware load is requested too early.

The usermodehelper locking code was added by b298d289c792 ("PM / Sleep:
Fix freezer failures due to racy usermodehelper_is_disabled()").

As Luis points out:

"Reviewing commit 247bc03742545 ("PM / Sleep: Mitigate race between the freezer
and request_firmware()") which originally extended umh state machine from
just being enabled/disabled, with the concepts of UMH_ENABLED, UMH_FREEZING,
UMH_DISABLED -- its goal was to prevent UMH uses during suspend. So -- the
"UMH lock" on firmware was actually added to help avoid races between freezing
and request_firmware(). We should not re-use UMH status notifiers when the
firmware UMH is disabled for the same concepts -- if we needed such a concept
then we should take this out from UMH code and generalize it."

After some discussion with Ming it was decided to put a comment to the
code to document the usage of the umh and a TODO to resolve this by
having some generic means to detect ongoing freezing operations.

Cc: Ming Lei <ming.lei@canonical.com>
Cc: Luis R. Rodriguez <mcgrof@kernel.org>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Signed-off-by: Daniel Wagner <daniel.wagner@bmw-carit.de>
---
 drivers/base/firmware_class.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
index 960f8f7..8eba1fb 100644
--- a/drivers/base/firmware_class.c
+++ b/drivers/base/firmware_class.c
@@ -1150,6 +1150,19 @@ _request_firmware(const struct firmware **firmware_p, const char *name,
 	if (ret <= 0) /* error or already assigned */
 		goto out;
 
+	/*
+	 * The usermode helper lock is taken to serialize the firmware
+	 * loading even when no usermoder mode helper is used at all.
+	 *
+	 * Some drivers may not benefit from firmware loading cache
+	 * when requesting loading in .resume(). In the situation of
+	 * suspend vs. resume, it is still too early for direct
+	 * loading. With UMH lock, we can get a warning or avoid the
+	 * issue.
+	 *
+	 * TODO: Taking the UMH lock is a bit missleading and it makes
+	 * sense to generalize this to a common freezer check.
+	 */
 	ret = 0;
 	timeout = firmware_loading_timeout();
 	if (opt_flags & FW_OPT_NOWAIT) {
-- 
2.7.4

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

#1480390 — Re: [PATCH v5 1/5] firmware: document user mode helper lock usage

On Fri, Sep 09, 2016 at 02:12:20PM +0200, Daniel Wagner wrote:
> From: Daniel Wagner <daniel.wagner@bmw-carit.de>
> 
> The lock is also used to generate warnings when a direct
> firmware load is requested too early.

I've determined the firmware cache lets us bail out of this
consideration now. If Ming agrees with the logic we don't need this
patch and you can continue as you had intended. Sorry for the trouble.

  Luis

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

#1488511 — Re: [PATCH v5 1/5] firmware: document user mode helper lock usage

On Sat, Sep 10, 2016 at 6:14 AM, Luis R. Rodriguez <mcgrof@kernel.org> wrote:
> On Fri, Sep 09, 2016 at 02:12:20PM +0200, Daniel Wagner wrote:
>> From: Daniel Wagner <daniel.wagner@bmw-carit.de>
>>
>> The lock is also used to generate warnings when a direct
>> firmware load is requested too early.
>
> I've determined the firmware cache lets us bail out of this
> consideration now. If Ming agrees with the logic we don't need this
> patch and you can continue as you had intended. Sorry for the trouble.

IMO it is helpful to add comment about using the lock for direct loading,
and we can sort it out in future if anyone want to improve it.

So for this patch, I am fine.

Thanks,

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

#1479934 — [PATCH v5 4/5] firmware: drop bit ops in favor of simple state machine

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

We track the state of the loading with bit ops. Since the state machine
has only a couple of states and they are all mutual exclusive there are
only a few simple state transition we can model this simplify.

	   UNKNOWN -> LOADING -> DONE | ABORTED

Cc: Ming Lei <ming.lei@canonical.com>
Cc: Luis R. Rodriguez <mcgrof@kernel.org>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Signed-off-by: Daniel Wagner <daniel.wagner@bmw-carit.de>
---
 drivers/base/firmware_class.c | 12 +++++-------
 1 file changed, 5 insertions(+), 7 deletions(-)

diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
index 5e38c27..8f5838c 100644
--- a/drivers/base/firmware_class.c
+++ b/drivers/base/firmware_class.c
@@ -109,7 +109,7 @@ enum {
 
 struct fw_umh {
 	struct completion completion;
-	unsigned long status;
+	u8 status;
 };
 
 static void fw_umh_init(struct fw_umh *fw_umh)
@@ -120,7 +120,7 @@ static void fw_umh_init(struct fw_umh *fw_umh)
 
 static int __fw_umh_check(struct fw_umh *fw_umh, unsigned long status)
 {
-	return test_bit(status, &fw_umh->status);
+	return fw_umh->status == status;
 }
 
 static int fw_umh_wait_timeout(struct fw_umh *fw_umh, long timeout)
@@ -129,7 +129,7 @@ static int fw_umh_wait_timeout(struct fw_umh *fw_umh, long timeout)
 
 	ret = wait_for_completion_interruptible_timeout(&fw_umh->completion,
 							timeout);
-	if (ret != 0 && test_bit(FW_UMH_ABORTED, &fw_umh->status))
+	if (ret != 0 && READ_ONCE(fw_umh->status) == FW_UMH_ABORTED)
 		return -ENOENT;
 
 	return ret;
@@ -138,12 +138,10 @@ static int fw_umh_wait_timeout(struct fw_umh *fw_umh, long timeout)
 static void __fw_umh_set(struct fw_umh *fw_umh,
 			  unsigned long status)
 {
-	set_bit(status, &fw_umh->status);
+	WRITE_ONCE(fw_umh->status, status);
 
-	if (status == FW_UMH_DONE || status == FW_UMH_ABORTED) {
-		clear_bit(FW_UMH_LOADING, &fw_umh->status);
+	if (status == FW_UMH_DONE || status == FW_UMH_ABORTED)
 		complete_all(&fw_umh->completion);
-	}
 }
 
 #define fw_umh_start(fw_umh)					\
-- 
2.7.4

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

#1480421 — Re: [PATCH v5 4/5] firmware: drop bit ops in favor of simple state machine

On Fri, Sep 09, 2016 at 02:12:23PM +0200, Daniel Wagner wrote:
> From: Daniel Wagner <daniel.wagner@bmw-carit.de>
> 
> We track the state of the loading with bit ops. Since the state machine

We track the state of the firmware usermode helper loading with bit ops.

> has only a couple of states and they are all mutual exclusive there are
> only a few simple state transition we can model this simplify.
> 
> 	   UNKNOWN -> LOADING -> DONE | ABORTED

If you also do the change suggested below you'd have to annotate that change in the
commit log as well.

> 
> Cc: Ming Lei <ming.lei@canonical.com>
> Cc: Luis R. Rodriguez <mcgrof@kernel.org>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Signed-off-by: Daniel Wagner <daniel.wagner@bmw-carit.de>
> ---
>  drivers/base/firmware_class.c | 12 +++++-------
>  1 file changed, 5 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
> index 5e38c27..8f5838c 100644
> --- a/drivers/base/firmware_class.c
> +++ b/drivers/base/firmware_class.c
> @@ -109,7 +109,7 @@ enum {
>  
>  struct fw_umh {
>  	struct completion completion;
> -	unsigned long status;
> +	u8 status;
 
Sorry I know I suggested the u8 but below you end up still using unsigned long status.
Instead of fixing this please consider changing:

  struct fw_umh {
       struct completion completion;
 -     unsigned long status;
 +     enum fw_umh_status status;

Then you can use the enum fw_umh_status status in function arguments, I've used this
trick in other codebases to ensure that the data type for the status passed then
matches the same one expected, *and* if you use a switch() statement the compiler
will complain and moan about missing values (unless a default switch statement
is present). For such simple state machines then this is better practice.

>  };
>  
>  static void fw_umh_init(struct fw_umh *fw_umh)
> @@ -120,7 +120,7 @@ static void fw_umh_init(struct fw_umh *fw_umh)
>  
>  static int __fw_umh_check(struct fw_umh *fw_umh, unsigned long status)
>  {
> -	return test_bit(status, &fw_umh->status);
> +	return fw_umh->status == status;

Why does this not use READ_ONCE(fw_umh->status) ?

  Luis

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

#1479935 — [PATCH v5 2/5] firmware: 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 unsigned long status and a complection in struct
firmware_buf. We only need this for the usermode helper as such we can
encapsulate all this data into its own data structure.

Cc: Ming Lei <ming.lei@canonical.com>
Cc: Luis R. Rodriguez <mcgrof@kernel.org>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Signed-off-by: Daniel Wagner <daniel.wagner@bmw-carit.de>
---
 drivers/base/firmware_class.c | 130 +++++++++++++++++++++++++++++-------------
 1 file changed, 91 insertions(+), 39 deletions(-)

diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
index 8eba1fb..821babe 100644
--- a/drivers/base/firmware_class.c
+++ b/drivers/base/firmware_class.c
@@ -91,12 +91,6 @@ static inline bool fw_is_builtin_firmware(const struct firmware *fw)
 }
 #endif
 
-enum {
-	FW_STATUS_LOADING,
-	FW_STATUS_DONE,
-	FW_STATUS_ABORT,
-};
-
 static int loading_timeout = 60;	/* In seconds */
 
 static inline long firmware_loading_timeout(void)
@@ -104,6 +98,77 @@ static inline long firmware_loading_timeout(void)
 	return loading_timeout > 0 ? loading_timeout * HZ : MAX_JIFFY_OFFSET;
 }
 
+#ifdef CONFIG_FW_LOADER_USER_HELPER
+
+enum {
+	FW_UMH_UNKNOWN,
+	FW_UMH_LOADING,
+	FW_UMH_DONE,
+	FW_UMH_ABORTED,
+};
+
+struct fw_umh {
+	struct completion completion;
+	unsigned long status;
+};
+
+static void fw_umh_init(struct fw_umh *fw_umh)
+{
+	init_completion(&fw_umh->completion);
+	fw_umh->status = FW_UMH_UNKNOWN;
+}
+
+static int __fw_umh_check(struct fw_umh *fw_umh, unsigned long status)
+{
+	return test_bit(status, &fw_umh->status);
+}
+
+static int fw_umh_wait_timeout(struct fw_umh *fw_umh, long timeout)
+{
+	int ret;
+
+	ret = wait_for_completion_interruptible_timeout(&fw_umh->completion,
+							timeout);
+	if (ret != 0 && test_bit(FW_UMH_ABORTED, &fw_umh->status))
+		return -ENOENT;
+
+	return ret;
+}
+
+static void __fw_umh_set(struct fw_umh *fw_umh,
+			  unsigned long status)
+{
+	set_bit(status, &fw_umh->status);
+
+	if (status == FW_UMH_DONE || status == FW_UMH_ABORTED) {
+		clear_bit(FW_UMH_LOADING, &fw_umh->status);
+		complete_all(&fw_umh->completion);
+	}
+}
+
+#define fw_umh_start(fw_umh)					\
+	__fw_umh_set(fw_umh, FW_UMH_LOADING)
+#define fw_umh_done(fw_umh)					\
+	__fw_umh_set(fw_umh, FW_UMH_DONE)
+#define fw_umh_aborted(fw_umh)					\
+	__fw_umh_set(fw_umh, FW_UMH_ABORTED)
+#define fw_umh_is_loading(fw_umh)				\
+	__fw_umh_check(fw_umh, FW_UMH_LOADING)
+#define fw_umh_is_done(fw_umh)					\
+	__fw_umh_check(fw_umh, FW_UMH_DONE)
+#define fw_umh_is_aborted(fw_umh)				\
+	__fw_umh_check(fw_umh, FW_UMH_ABORTED)
+
+#else /* CONFIG_FW_LOADER_USER_HELPER */
+
+#define fw_umh_wait_timeout(fw_st, long)	0
+
+#define fw_umh_done(fw_st)
+#define fw_umh_is_done(fw_st)			true
+#define fw_umh_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_umh fw_umh;
 	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_umh_init(&buf->fw_umh);
 	INIT_LIST_HEAD(&buf->pending_list);
 #endif
 
@@ -309,8 +373,7 @@ static void fw_finish_direct_load(struct device *device,
 				  struct firmware_buf *buf)
 {
 	mutex_lock(&fw_lock);
-	set_bit(FW_STATUS_DONE, &buf->status);
-	complete_all(&buf->completion);
+	fw_umh_done(&buf->fw_umh);
 	mutex_unlock(&fw_lock);
 }
 
@@ -478,12 +541,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_umh_is_done(&buf->fw_umh))
 		return;
 
 	list_del_init(&buf->pending_list);
-	set_bit(FW_STATUS_ABORT, &buf->status);
-	complete_all(&buf->completion);
+	fw_umh_aborted(&buf->fw_umh);
 }
 
 static void fw_load_abort(struct firmware_priv *fw_priv)
@@ -496,9 +558,6 @@ static void fw_load_abort(struct firmware_priv *fw_priv)
 	fw_priv->buf = NULL;
 }
 
-#define is_fw_load_aborted(buf)	\
-	test_bit(FW_STATUS_ABORT, &(buf)->status)
-
 static LIST_HEAD(pending_fw_head);
 
 /* reboot notifier for avoid deadlock with usermode_lock */
@@ -598,7 +657,7 @@ static ssize_t firmware_loading_show(struct device *dev,
 
 	mutex_lock(&fw_lock);
 	if (fw_priv->buf)
-		loading = test_bit(FW_STATUS_LOADING, &fw_priv->buf->status);
+		loading = fw_umh_is_loading(&fw_priv->buf->fw_umh);
 	mutex_unlock(&fw_lock);
 
 	return sprintf(buf, "%d\n", loading);
@@ -653,23 +712,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_umh_is_done(&fw_buf->fw_umh)) {
 			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_umh_start(&fw_buf->fw_umh);
 		}
 		break;
 	case 0:
-		if (test_bit(FW_STATUS_LOADING, &fw_buf->status)) {
+		if (fw_umh_is_loading(&fw_buf->fw_umh)) {
 			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 +747,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_umh_aborted(&fw_buf->fw_umh);
 				written = rc;
+			} else {
+				fw_umh_done(&fw_buf->fw_umh);
 			}
-			complete_all(&fw_buf->completion);
 			break;
 		}
 		/* fallthrough */
@@ -755,7 +812,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_umh_is_done(&buf->fw_umh)) {
 		ret_count = -ENODEV;
 		goto out;
 	}
@@ -842,7 +899,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_umh_is_done(&buf->fw_umh)) {
 		retval = -ENODEV;
 		goto out;
 	}
@@ -955,8 +1012,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_umh_wait_timeout(&buf->fw_umh, timeout);
 	if (retval == -ERESTARTSYS || !retval) {
 		mutex_lock(&fw_lock);
 		fw_load_abort(fw_priv);
@@ -965,7 +1021,7 @@ static int _request_firmware_load(struct firmware_priv *fw_priv,
 		retval = 0;
 	}
 
-	if (is_fw_load_aborted(buf))
+	if (fw_umh_is_aborted(&buf->fw_umh))
 		retval = -EAGAIN;
 	else if (buf->is_paged_buf && !buf->data)
 		retval = -ENOMEM;
@@ -1015,29 +1071,25 @@ 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)) {
+	while (!fw_umh_is_done(&buf->fw_umh)) {
+		if (fw_umh_is_aborted(&buf->fw_umh)) {
 			ret = -ENOENT;
 			break;
 		}
 		mutex_unlock(&fw_lock);
-		ret = wait_for_completion_interruptible(&buf->completion);
+		ret = fw_umh_wait_timeout(&buf->fw_umh, 0);
 		mutex_lock(&fw_lock);
 	}
 	mutex_unlock(&fw_lock);
@@ -1095,7 +1147,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_umh_is_aborted(&buf->fw_umh)) {
 		mutex_unlock(&fw_lock);
 		return -ENOENT;
 	}
-- 
2.7.4

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

#1480391 — Re: [PATCH v5 2/5] firmware: encapsulate firmware loading status

On Fri, Sep 09, 2016 at 02:12:21PM +0200, Daniel Wagner wrote:
> +#else /* CONFIG_FW_LOADER_USER_HELPER */
> +
> +#define fw_umh_wait_timeout(fw_st, long)	0
> +
> +#define fw_umh_done(fw_st)
> +#define fw_umh_is_done(fw_st)			true
> +#define fw_umh_is_aborted(fw_st)		false
> +

If we do proceed with compartmentalizing the UMH timeout crap
then my hope is this piece of code will not require the
fw_umh_wait_timeout() def.

  Luis

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

#1480400 — Re: [PATCH v5 2/5] firmware: encapsulate firmware loading status

On Fri, Sep 09, 2016 at 02:12:21PM +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 unsigned long status and a complection in struct
> firmware_buf. We only need this for the usermode helper as such we can
> encapsulate all this data into its own data structure.
> 
> Cc: Ming Lei <ming.lei@canonical.com>
> Cc: Luis R. Rodriguez <mcgrof@kernel.org>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Signed-off-by: Daniel Wagner <daniel.wagner@bmw-carit.de>
> ---
>  drivers/base/firmware_class.c | 130 +++++++++++++++++++++++++++++-------------
>  1 file changed, 91 insertions(+), 39 deletions(-)
> 
> diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
> index 8eba1fb..821babe 100644
> --- a/drivers/base/firmware_class.c
> +++ b/drivers/base/firmware_class.c
> @@ -91,12 +91,6 @@ static inline bool fw_is_builtin_firmware(const struct firmware *fw)
>  }
>  #endif
>  
> -enum {
> -	FW_STATUS_LOADING,
> -	FW_STATUS_DONE,
> -	FW_STATUS_ABORT,
> -};
> -
>  static int loading_timeout = 60;	/* In seconds */
>  
>  static inline long firmware_loading_timeout(void)
> @@ -104,6 +98,77 @@ static inline long firmware_loading_timeout(void)
>  	return loading_timeout > 0 ? loading_timeout * HZ : MAX_JIFFY_OFFSET;
>  }
>  
> +#ifdef CONFIG_FW_LOADER_USER_HELPER
> +
> +enum {
> +	FW_UMH_UNKNOWN,
> +	FW_UMH_LOADING,
> +	FW_UMH_DONE,
> +	FW_UMH_ABORTED,
> +};

Note the enum here is anonymous still. That's a bit of a lost opportunity. If we name
it we can then just use it in arguments, but that can be done later in your other
patch. So say we name it enum fw_umh_status or whatever.

  Luis

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

#1482316 — Re: [PATCH v5 2/5] firmware: encapsulate firmware loading status

Hi Luis,


On 09/09/2016 02:12 PM, Daniel Wagner wrote:
> The firmware user helper code tracks the current state of the loading
> process via unsigned long status and a complection in struct
> firmware_buf. We only need this for the usermode helper as such we can
> encapsulate all this data into its own data structure.

I don't think we are able to move the completion code into a 
CONFIG_FW_LOADER_HELPER section. The direct loading path uses completion 
as well.

> +#ifdef CONFIG_FW_LOADER_USER_HELPER
> +
> +enum {
> +	FW_UMH_UNKNOWN,
> +	FW_UMH_LOADING,
> +	FW_UMH_DONE,
> +	FW_UMH_ABORTED,
> +};

The direct loading path just uses two states, LOADING and DONE. ABORTED 
is not used.

> +struct fw_umh {
> +	struct completion completion;
> +	unsigned long status;
> +};
> +
> +static void fw_umh_init(struct fw_umh *fw_umh)
> +{
> +	init_completion(&fw_umh->completion);
> +	fw_umh->status = FW_UMH_UNKNOWN;
> +}
> +
> +static int __fw_umh_check(struct fw_umh *fw_umh, unsigned long status)
> +{
> +	return test_bit(status, &fw_umh->status);
> +}
> +
> +static int fw_umh_wait_timeout(struct fw_umh *fw_umh, long timeout)
> +{
> +	int ret;
> +
> +	ret = wait_for_completion_interruptible_timeout(&fw_umh->completion,
> +							timeout);
> +	if (ret != 0 && test_bit(FW_UMH_ABORTED, &fw_umh->status))
> +		return -ENOENT;
> +
> +	return ret;
> +}
> +
> +static void __fw_umh_set(struct fw_umh *fw_umh,
> +			  unsigned long status)
> +{
> +	set_bit(status, &fw_umh->status);
> +
> +	if (status == FW_UMH_DONE || status == FW_UMH_ABORTED) {
> +		clear_bit(FW_UMH_LOADING, &fw_umh->status);
> +		complete_all(&fw_umh->completion);
> +	}
> +}
> +
> +#define fw_umh_start(fw_umh)					\
> +	__fw_umh_set(fw_umh, FW_UMH_LOADING)
> +#define fw_umh_done(fw_umh)					\
> +	__fw_umh_set(fw_umh, FW_UMH_DONE)
> +#define fw_umh_aborted(fw_umh)					\
> +	__fw_umh_set(fw_umh, FW_UMH_ABORTED)
> +#define fw_umh_is_loading(fw_umh)				\
> +	__fw_umh_check(fw_umh, FW_UMH_LOADING)
> +#define fw_umh_is_done(fw_umh)					\
> +	__fw_umh_check(fw_umh, FW_UMH_DONE)
> +#define fw_umh_is_aborted(fw_umh)				\
> +	__fw_umh_check(fw_umh, FW_UMH_ABORTED)
> +
> +#else /* CONFIG_FW_LOADER_USER_HELPER */
> +
> +#define fw_umh_wait_timeout(fw_st, long)	0
> +
> +#define fw_umh_done(fw_st)
> +#define fw_umh_is_done(fw_st)			true
> +#define fw_umh_is_aborted(fw_st)		false

We still need the implementation for fw_umh_wait_timeout() and 
fw_umh_start(), fw_umh_done() etc. fw_umh_is_aborted() is not needed.


> @@ -309,8 +373,7 @@ static void fw_finish_direct_load(struct device *device,
>  				  struct firmware_buf *buf)
>  {
>  	mutex_lock(&fw_lock);
> -	set_bit(FW_STATUS_DONE, &buf->status);
> -	complete_all(&buf->completion);
> +	fw_umh_done(&buf->fw_umh);
>  	mutex_unlock(&fw_lock);
>  }

Here we signal that we have loaded the firmware

>  /* 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)) {
> +	while (!fw_umh_is_done(&buf->fw_umh)) {
> +		if (fw_umh_is_aborted(&buf->fw_umh)) {
>  			ret = -ENOENT;
>  			break;
>  		}
>  		mutex_unlock(&fw_lock);
> -		ret = wait_for_completion_interruptible(&buf->completion);
> +		ret = fw_umh_wait_timeout(&buf->fw_umh, 0);
>  		mutex_lock(&fw_lock);
>  	}

and here we here we wait for it.

So I suggest to rename it back to fw_status_ and don't move it inside a 
CONFIG_FW_LOADER_HELPER section. Drop the special handling of the 
ABORTED and add instead a comment that the ABORTED state is not used for 
direct loading. This special handling makes unnecessary more complex. 
This is a slowpath and this micro optimization is helping to maintain 
the code.

cheers,
daniel

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

#1479938 — [PATCH v5 5/5] firmware: do not use fw_lock for fw_umh protection

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

fw_lock is to use to protect 'corner cases' inside firmware_class. It
is not exactly clear what those corner cases are nor what it exactly
protects. fw_umh can be used without needing the fw_lock to protect
its state transition and wake ups.

fw_umh is holds the state in status and the completion is used to
wake up all waiters (in this case that is the user land helper so only
one). This operation has to be 'atomic' to avoid races.  We can do this
by using swait which takes care we don't miss any wake up.

We use also swait instead of wait because don't need all the additional
features wait provides.

Cc: Ming Lei <ming.lei@canonical.com>
Cc: Luis R. Rodriguez <mcgrof@kernel.org>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Signed-off-by: Daniel Wagner <daniel.wagner@bmw-carit.de>
---
 drivers/base/firmware_class.c | 50 +++++++++++++------------------------------
 1 file changed, 15 insertions(+), 35 deletions(-)

diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
index 8f5838c..1a28070 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>
 
@@ -108,13 +109,13 @@ enum {
 };
 
 struct fw_umh {
-	struct completion completion;
+	struct swait_queue_head wq;
 	u8 status;
 };
 
 static void fw_umh_init(struct fw_umh *fw_umh)
 {
-	init_completion(&fw_umh->completion);
+	init_swait_queue_head(&fw_umh->wq);
 	fw_umh->status = FW_UMH_UNKNOWN;
 }
 
@@ -123,13 +124,19 @@ static int __fw_umh_check(struct fw_umh *fw_umh, unsigned long status)
 	return fw_umh->status == status;
 }
 
+static inline bool __fw_umh_is_done(unsigned long status)
+{
+	return status == FW_UMH_DONE || status == FW_UMH_ABORTED;
+}
+
 static int fw_umh_wait_timeout(struct fw_umh *fw_umh, long timeout)
 {
 	int ret;
 
-	ret = wait_for_completion_interruptible_timeout(&fw_umh->completion,
-							timeout);
-	if (ret != 0 && READ_ONCE(fw_umh->status) == FW_UMH_ABORTED)
+	ret = swait_event_interruptible_timeout(fw_umh->wq,
+				__fw_umh_is_done(READ_ONCE(fw_umh->status)),
+				timeout);
+	if (ret != 0 && fw_umh->status == FW_UMH_ABORTED)
 		return -ENOENT;
 
 	return ret;
@@ -141,7 +148,7 @@ static void __fw_umh_set(struct fw_umh *fw_umh,
 	WRITE_ONCE(fw_umh->status, status);
 
 	if (status == FW_UMH_DONE || status == FW_UMH_ABORTED)
-		complete_all(&fw_umh->completion);
+		swake_up(&fw_umh->wq);
 }
 
 #define fw_umh_start(fw_umh)					\
@@ -367,14 +374,6 @@ static const char * const fw_path[] = {
 module_param_string(path, fw_path_para, sizeof(fw_path_para), 0644);
 MODULE_PARM_DESC(path, "customized firmware image search path with a higher priority than default path");
 
-static void fw_finish_direct_load(struct device *device,
-				  struct firmware_buf *buf)
-{
-	mutex_lock(&fw_lock);
-	fw_umh_done(&buf->fw_umh);
-	mutex_unlock(&fw_lock);
-}
-
 static int
 fw_get_filesystem_firmware(struct device *device, struct firmware_buf *buf)
 {
@@ -421,7 +420,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_umh_done(&buf->fw_umh);
 		break;
 	}
 	__putname(path);
@@ -1074,25 +1073,6 @@ static inline void kill_requests_without_uevent(void) { }
 
 #endif /* CONFIG_FW_LOADER_USER_HELPER */
 
-/* wait until the shared firmware_buf becomes ready (or error) */
-static int sync_cached_firmware_buf(struct firmware_buf *buf)
-{
-	int ret = 0;
-
-	mutex_lock(&fw_lock);
-	while (!fw_umh_is_done(&buf->fw_umh)) {
-		if (fw_umh_is_aborted(&buf->fw_umh)) {
-			ret = -ENOENT;
-			break;
-		}
-		mutex_unlock(&fw_lock);
-		ret = fw_umh_wait_timeout(&buf->fw_umh, 0);
-		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
@@ -1126,7 +1106,7 @@ _request_firmware_prepare(struct firmware **firmware_p, const char *name,
 	firmware->priv = buf;
 
 	if (ret > 0) {
-		ret = sync_cached_firmware_buf(buf);
+		ret = fw_umh_wait_timeout(&buf->fw_umh, 0);
 		if (!ret) {
 			fw_set_page_data(buf, firmware);
 			return 0; /* assigned */
-- 
2.7.4

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

#1480222

On Fri, Sep 09, 2016 at 02:12:19PM +0200, Daniel Wagner wrote:
> From: Daniel Wagner <daniel.wagner@bmw-carit.de>
> 
> Hi,
> 
> 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 increase readability and reduce the usage of the fw_lock.
> 
> Luis asked by to add a few CC for getting a wider audience. If you
> haven't seen this before the initial version of this series has some
> more details on the motivation for this series:
> 
> http://www.spinics.net/lists/linux-wireless/msg153005.html
> 
> I tested this series with fw_userhelper.sh and fw_filesystem.sh under
> kvm and also let it run on real hardware. The series is also available
> here:
> 
> https://git.kernel.org/cgit/linux/kernel/git/wagi/linux.git/log/?h=firmware_async-7

Ah, this series is looking so much nicer now thanks!

> I had pushed earlier version of series there and haven't got any 0-day
> bug reports so far.

0-day eventually sends a complete success report if you're good. That's
when you can get a warm fuzzy over your series. Found just one more issue.
Since that means another respin I'll provide some other nitpicks.


  Luis

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web