Messages in this thread | | | From | Ming Lei <> | Date | Thu, 8 Sep 2016 19:26:56 +0800 | Subject | Re: [PATCH v4 2/4] firmware: encapsulate firmware loading status |
| |
On Wed, Sep 7, 2016 at 4:45 PM, Daniel Wagner <wagi@monom.org> 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. > > 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 | 123 ++++++++++++++++++++++++++++++------------ > 1 file changed, 88 insertions(+), 35 deletions(-) > > diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c > index d4fee06..b11fbb0 100644 > --- a/drivers/base/firmware_class.c > +++ b/drivers/base/firmware_class.c > @@ -91,10 +91,13 @@ static inline bool fw_is_builtin_firmware(const struct firmware *fw) > } > #endif > > +#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 */ > @@ -104,6 +107,69 @@ static inline long firmware_loading_timeout(void) > return loading_timeout > 0 ? loading_timeout * HZ : MAX_JIFFY_OFFSET; > } > > +struct fw_status { > + unsigned long status; > + struct completion completion; > +}; > + > +static void fw_status_init(struct fw_status *fw_st) > +{ > + fw_st->status = FW_STATUS_UNKNOWN; > + init_completion(&fw_st->completion); > +} > + > +static int __fw_status_check(struct fw_status *fw_st, unsigned long status) > +{ > + return test_bit(status, &fw_st->status); > +} > + > +static int fw_status_wait_timeout(struct fw_status *fw_st, long timeout) > +{ > + int ret; > + > + ret = wait_for_completion_interruptible_timeout(&fw_st->completion, > + timeout); > + if (ret == 0 && test_bit(FW_STATUS_ABORTED, &fw_st->status)) > + return -ENOENT;
I guess the check should have been OR instead of AND, right?
> + > + return ret; > +} > + > +static void __fw_status_set(struct fw_status *fw_st, > + unsigned long status) > +{ > + set_bit(status, &fw_st->status); > + > + if (status == FW_STATUS_DONE || > + status == FW_STATUS_ABORTED) { > + clear_bit(FW_STATUS_LOADING, &fw_st->status); > + 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_check(fw_st, FW_STATUS_LOADING) > +#define fw_status_is_done(fw_st) \ > + __fw_status_check(fw_st, FW_STATUS_DONE) > +#define fw_status_is_aborted(fw_st) \ > + __fw_status_check(fw_st, FW_STATUS_ABORTED) > + > +#else /* CONFIG_FW_LOADER_USER_HELPER */ > + > +#define fw_status_wait_timeout(fw_st, long) 0 > + > +#define fw_status_done(fw_st) > +#define fw_status_is_done(fw_st) true > +#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 +211,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 +270,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 > > @@ -309,8 +374,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_status_done(&buf->fw_st); > mutex_unlock(&fw_lock); > } > > @@ -478,12 +542,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 +559,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 +658,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_status_is_loading(&fw_priv->buf->fw_st); > mutex_unlock(&fw_lock); > > return sprintf(buf, "%d\n", loading); > @@ -653,23 +713,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 +748,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 +760,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 +813,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 +900,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 +1013,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 +1022,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; > @@ -1040,29 +1097,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_status_is_done(&buf->fw_st)) { > + if (fw_status_is_aborted(&buf->fw_st)) { > ret = -ENOENT; > break; > } > mutex_unlock(&fw_lock); > - ret = wait_for_completion_interruptible(&buf->completion); > + ret = fw_status_wait_timeout(&buf->fw_st, 0); > mutex_lock(&fw_lock); > } > mutex_unlock(&fw_lock); > @@ -1120,7 +1173,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
| |