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

Groups > linux.kernel > #1468414 > unrolled thread

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

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH v2 2/2] firmware_class: Use swait instead of completion Daniel Wagner <wagi@monom.org> - 2016-08-23 11:10 +0200
    Re: [PATCH v2 2/2] firmware_class: Use swait instead of completion "Luis R. Rodriguez" <mcgrof@kernel.org> - 2016-08-24 02:00 +0200

#1468414 — [PATCH v2 2/2] firmware_class: Use swait instead of completion

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

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

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

diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
index d3dcf87..029b829 100644
--- a/drivers/base/firmware_class.c
+++ b/drivers/base/firmware_class.c
@@ -30,6 +30,7 @@
 #include <linux/syscore_ops.h>
 #include <linux/reboot.h>
 #include <linux/security.h>
+#include <linux/swait.h>
 
 #include <generated/utsrelease.h>
 
@@ -109,13 +110,13 @@ enum {
 
 struct fw_status {
 	unsigned long status;
-	struct completion completion;
+	struct swait_queue_head wq;
 };
 
 static void fw_status_init(struct fw_status *fw_st)
 {
 	fw_st->status = FW_STATUS_UNKNOWN;
-	init_completion(&fw_st->completion);
+	init_swait_queue_head(&fw_st->wq);
 }
 
 static unsigned long __fw_status_get(struct fw_status *fw_st)
@@ -123,15 +124,19 @@ static unsigned long __fw_status_get(struct fw_status *fw_st)
 	return READ_ONCE(fw_st->status);
 }
 
+static inline bool is_fw_status_done(unsigned long status)
+{
+	return status == FW_STATUS_DONE ||
+	       status == FW_STATUS_ABORTED;
+}
+
 static int fw_status_wait_timeout(struct fw_status *fw_st, long timeout)
 {
-	unsigned long status;
 	int err;
-
-	err = wait_for_completion_interruptible_timeout(&fw_st->completion,
-							timeout);
-	status = READ_ONCE(fw_st->status);
-	if (err == 0 && status == FW_STATUS_ABORTED)
+	err = swait_event_interruptible_timeout(fw_st->wq,
+				is_fw_status_done(READ_ONCE(fw_st->status)),
+				timeout);
+	if (err == 0 && fw_st->status == FW_STATUS_ABORTED)
 		return -ENOENT;
 
 	return err;
@@ -144,7 +149,7 @@ static void __fw_status_set(struct fw_status *fw_st,
 
 	if (status == FW_STATUS_DONE ||
 			status == FW_STATUS_ABORTED)
-		complete_all(&fw_st->completion);
+		swake_up(&fw_st->wq);
 }
 
 #define fw_status_start(fw_st)					\
-- 
2.7.4

[toc] | [next] | [standalone]

#1468982

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

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

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

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

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

  Luis

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web