* [PATCH v2 0/2] firmware_class: encapsulate firmware loading status @ 2016-08-23 9:00 Daniel Wagner 2016-08-23 9:00 ` [PATCH v2 1/2] " Daniel Wagner 2016-08-23 9:00 ` [PATCH v2 2/2] firmware_class: Use swait instead of completion Daniel Wagner 0 siblings, 2 replies; 5+ messages in thread From: Daniel Wagner @ 2016-08-23 9:00 UTC (permalink / raw) To: linux-kernel Cc: Daniel Wagner, Ming Lei, Luis R . Rodriguez, Greg Kroah-Hartman From: Daniel Wagner <daniel.wagner@bmw-carit.de> Hi, As requested an splited up version. cheers, daniel 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 Cc: Ming Lei <ming.lei@canonical.com> Cc: Luis R. Rodriguez <mcgrof@kernel.org> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> Daniel Wagner (2): firmware_class: encapsulate firmware loading status firmware_class: Use swait instead of completion drivers/base/firmware_class.c | 163 +++++++++++++++++++++++++----------------- 1 file changed, 98 insertions(+), 65 deletions(-) -- 2.7.4 ^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH v2 1/2] firmware_class: encapsulate firmware loading status 2016-08-23 9:00 [PATCH v2 0/2] firmware_class: encapsulate firmware loading status Daniel Wagner @ 2016-08-23 9:00 ` Daniel Wagner 2016-08-23 23:55 ` Luis R. Rodriguez 2016-08-23 9:00 ` [PATCH v2 2/2] firmware_class: Use swait instead of completion Daniel Wagner 1 sibling, 1 reply; 5+ messages in thread From: Daniel Wagner @ 2016-08-23 9:00 UTC (permalink / raw) To: linux-kernel Cc: Daniel Wagner, Ming Lei, Luis R . Rodriguez, Greg Kroah-Hartman 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. We don't need the fw_lock to protect the state machine anymore. First of all we don't do any RWM operations instead we do either a write or read to the status variable. Reading the status variable via READ_ONCE ensures we see the new state whenever we get woken. We are not allowed to use the complete_all() more than once so we need to filter out all state transition but the last. That is okay since only the final state DONE|ABORTED is of interest. Besides that we also safe a few bytes: cris etrax-100lx_defconfig $ bloat-o-meter firmware_class.o.old firmware_class.o.new add/remove: 0/0 grow/shrink: 0/1 up/down: 0/-234 (-234) function old new delta _request_firmware 1228 994 -234 Total: Before=2179, After=1945, chg -10.74% x86_64 FW_LOADER !FW_LOADER_USER_HELPER_FALLBACK $ bloat-o-meter firmware_class.o.old firmware_class.o.new add/remove: 0/0 grow/shrink: 0/1 up/down: 0/-194 (-194) function old new delta _request_firmware 2118 1924 -194 Total: Before=6811, After=6617, chg -2.85% x86_64 FW_LOADER FW_LOADER_USER_HELPER_FALLBACK $ bloat-o-meter firmware_class.o.old firmware_class.o.new add/remove: 0/0 grow/shrink: 2/5 up/down: 4/-193 (-189) function old new delta fw_shutdown_notify 81 83 +2 firmware_data_read 162 164 +2 fw_pm_notify 396 394 -2 firmware_loading_store 458 441 -17 firmware_data_write 504 475 -29 firmware_loading_show 92 61 -31 _request_firmware 2931 2817 -114 Total: Before=10328, After=10139, chg -1.83% Note the variable loading_timeout and the inline function firmware_loadint_timeout() are used also when no user helper is enabled. So they need to be moved out side of the ifdef section. 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 | 158 +++++++++++++++++++++++++----------------- 1 file changed, 93 insertions(+), 65 deletions(-) diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c index 22d1760..d3dcf87 100644 --- a/drivers/base/firmware_class.c +++ b/drivers/base/firmware_class.c @@ -91,19 +91,88 @@ 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; +} + +#ifdef CONFIG_FW_LOADER_USER_HELPER + 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 completion completion; +}; -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_completion(&fw_st->completion); +} + +static unsigned long __fw_status_get(struct fw_status *fw_st) +{ + return READ_ONCE(fw_st->status); +} + +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) + 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) + complete_all(&fw_st->completion); } +#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 /* CONFIG_FW_LOADER_USER_HELPER */ + +#define fw_status_wait_timeout(fw_st, long) 0 + +#define fw_status_start(fw_st) +#define fw_status_done(fw_st) +#define fw_status_aborted(fw_st) +#define fw_status_is_loading(fw_st) +#define fw_status_is_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 +214,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 +273,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 +373,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 +419,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 +537,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 +554,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 +651,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 +706,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 +741,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 +753,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 +806,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 +893,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 +1006,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 +1015,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 +1065,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 +1104,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 +1123,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 ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v2 1/2] firmware_class: encapsulate firmware loading status 2016-08-23 9:00 ` [PATCH v2 1/2] " Daniel Wagner @ 2016-08-23 23:55 ` Luis R. Rodriguez 0 siblings, 0 replies; 5+ messages in thread From: Luis R. Rodriguez @ 2016-08-23 23:55 UTC (permalink / raw) To: Daniel Wagner Cc: linux-kernel, Daniel Wagner, Ming Lei, Luis R . Rodriguez, Greg Kroah-Hartman On Tue, Aug 23, 2016 at 11:00:19AM +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. You can specify the members, the unsigned long status and the completion. > Let's > encapsulate this simple state machine into struct fw_status. Please rephrase instead to: "We only need this for the usermode helper as such we can encapsulate all this data into its own data structure." > We don't need the fw_lock to protect the state machine anymore. You can the instead follow up explaining, "Doing this helps us compartamentalize the usermode helper code further." Can you cut that out into a separate patch? Then another patch can be: "The usermode helper code only needs access to a status variable, no RWM operations are done on it, so we can replace the use of the global lock with READ_ONCE() and WRITE_ONCE()." > First of > all we don't do any RWM operations instead we do either a write or read > to the status variable. Reading the status variable via READ_ONCE > ensures we see the new state whenever we get woken. We are not allowed > to use the complete_all() more than once so we need to filter out all > state transition but the last. That is okay since only the final state > DONE|ABORTED is of interest. > > Besides that we also safe a few bytes: > > cris etrax-100lx_defconfig > > $ bloat-o-meter firmware_class.o.old firmware_class.o.new > add/remove: 0/0 grow/shrink: 0/1 up/down: 0/-234 (-234) > function old new delta > _request_firmware 1228 994 -234 > Total: Before=2179, After=1945, chg -10.74% > > x86_64 FW_LOADER !FW_LOADER_USER_HELPER_FALLBACK > > $ bloat-o-meter firmware_class.o.old firmware_class.o.new > add/remove: 0/0 grow/shrink: 0/1 up/down: 0/-194 (-194) > function old new delta > _request_firmware 2118 1924 -194 > Total: Before=6811, After=6617, chg -2.85% > > x86_64 FW_LOADER FW_LOADER_USER_HELPER_FALLBACK > > $ bloat-o-meter firmware_class.o.old firmware_class.o.new > add/remove: 0/0 grow/shrink: 2/5 up/down: 4/-193 (-189) > function old new delta > fw_shutdown_notify 81 83 +2 > firmware_data_read 162 164 +2 > fw_pm_notify 396 394 -2 > firmware_loading_store 458 441 -17 > firmware_data_write 504 475 -29 > firmware_loading_show 92 61 -31 > _request_firmware 2931 2817 -114 > Total: Before=10328, After=10139, chg -1.83% > > Note the variable loading_timeout and the inline function > firmware_loadint_timeout() are used also when no user helper is enabled. > So they need to be moved out side of the ifdef section. > > 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 | 158 +++++++++++++++++++++++++----------------- > 1 file changed, 93 insertions(+), 65 deletions(-) > > diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c > index 22d1760..d3dcf87 100644 > --- a/drivers/base/firmware_class.c > +++ b/drivers/base/firmware_class.c > @@ -91,19 +91,88 @@ 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; > +} All this code is being kept when CONFIG_FW_LOADER_USER_HELPER is not enabled, that seems odd given firmware_loading_timeout() is used when CONFIG_FW_LOADER_USER_HELPER is disabled in two places, in one case its not needed at all, in the second its in this piece of code: retval = fw_status_wait_timeout(&buf->fw_st, timeout); if (retval == -ERESTARTSYS || !retval) { ... } I'd rather instead have a wrapper for a user mode helper timeout check and have that return 0 for when CONFIG_FW_LOADER_USER_HELPER is not selected. This way the global variable loading_timeout and anything that accesses it is just left inside CONFIG_FW_LOADER_USER_HELPER. > + > +#ifdef CONFIG_FW_LOADER_USER_HELPER > + > 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 completion completion; > +}; > > -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_completion(&fw_st->completion); > +} > + > +static unsigned long __fw_status_get(struct fw_status *fw_st) > +{ > + return READ_ONCE(fw_st->status); > +} > + > +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) > + 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) > + complete_all(&fw_st->completion); > } > > +#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 /* CONFIG_FW_LOADER_USER_HELPER */ > + > +#define fw_status_wait_timeout(fw_st, long) 0 > + > +#define fw_status_start(fw_st) You don't need this, in fact if you had it as-is and the code for when CONFIG_FW_LOADER_USER_HELPER is disabled would use it you may end up in a compile error. > +#define fw_status_done(fw_st) You need this for both. > +#define fw_status_aborted(fw_st) > +#define fw_status_is_loading(fw_st) > +#define fw_status_is_done(fw_st) All these are no longer needed for when CONFIG_FW_LOADER_USER_HELPER is disabled. > +#define fw_status_is_aborted(fw_st) false This is needed for both. Given all this it seems to be best to rebrand these helpers as usermode helper related. In fact the next step is to see if this looks best to just stuffing all this user mode helper code into its own C file. But we can do that later. Other than that looks good! Luis ^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH v2 2/2] firmware_class: Use swait instead of completion 2016-08-23 9:00 [PATCH v2 0/2] firmware_class: encapsulate firmware loading status Daniel Wagner 2016-08-23 9:00 ` [PATCH v2 1/2] " Daniel Wagner @ 2016-08-23 9:00 ` Daniel Wagner 2016-08-23 23:57 ` Luis R. Rodriguez 1 sibling, 1 reply; 5+ messages in thread From: Daniel Wagner @ 2016-08-23 9:00 UTC (permalink / raw) To: linux-kernel Cc: Daniel Wagner, Ming Lei, Luis R . Rodriguez, Greg Kroah-Hartman 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 ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v2 2/2] firmware_class: Use swait instead of completion 2016-08-23 9:00 ` [PATCH v2 2/2] firmware_class: Use swait instead of completion Daniel Wagner @ 2016-08-23 23:57 ` Luis R. Rodriguez 0 siblings, 0 replies; 5+ messages in thread From: Luis R. Rodriguez @ 2016-08-23 23:57 UTC (permalink / raw) To: Daniel Wagner Cc: linux-kernel, Daniel Wagner, Ming Lei, Luis R . Rodriguez, Greg Kroah-Hartman 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 ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2016-08-23 23:57 UTC | newest] Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2016-08-23 9:00 [PATCH v2 0/2] firmware_class: encapsulate firmware loading status Daniel Wagner 2016-08-23 9:00 ` [PATCH v2 1/2] " Daniel Wagner 2016-08-23 23:55 ` Luis R. Rodriguez 2016-08-23 9:00 ` [PATCH v2 2/2] firmware_class: Use swait instead of completion Daniel Wagner 2016-08-23 23:57 ` Luis R. Rodriguez
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox
all inboxes | Powered by JetHome®