From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753802AbcIHBjv (ORCPT ); Wed, 7 Sep 2016 21:39:51 -0400 Received: from mx2.suse.de ([195.135.220.15]:57523 "EHLO mx2.suse.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752396AbcIHBjn (ORCPT ); Wed, 7 Sep 2016 21:39:43 -0400 Date: Thu, 8 Sep 2016 03:39:40 +0200 From: "Luis R. Rodriguez" To: Daniel Wagner , Takashi Iwai Cc: linux-kernel@vger.kernel.org, Daniel Wagner , Ming Lei , "Luis R . Rodriguez" , Greg Kroah-Hartman Subject: Re: [PATCH v4 2/4] firmware: encapsulate firmware loading status Message-ID: <20160908013940.GA3296@wotan.suse.de> References: <1473237908-20989-1-git-send-email-wagi@monom.org> <1473237908-20989-3-git-send-email-wagi@monom.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1473237908-20989-3-git-send-email-wagi@monom.org> User-Agent: Mutt/1.5.21 (2010-09-15) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Wed, Sep 07, 2016 at 10:45:06AM +0200, Daniel Wagner wrote: > From: Daniel Wagner > > 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 > Cc: Ming Lei > Cc: Luis R. Rodriguez > Cc: Greg Kroah-Hartman > --- > 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; > + > + 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 */ Note, this all does *nothing* when we have CONFIG_FW_LOADER_USER_HELPER disabled. Might as well then rename these with a fw_umh_ prefix so we can easily tell that this mess is for that. So how about fw_umh_done(), fw_umh_is_done() fw_umh_is_aborted() and fw_umh_wait()? > + > +#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; Likewise fw_umh_status ? > 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, Maybe rename also _request_firmware_load() to something to implicate this is all umh related. > timeout = MAX_JIFFY_OFFSET; > } > > - retval = wait_for_completion_interruptible_timeout(&buf->completion, > - timeout); > + retval = fw_status_wait_timeout(&buf->fw_st, timeout); Note this is one of the users for fw_status_wait_timeout(). This makes sense for this fw_status_wait_timeout, as its umh related. > 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); Now this one -- I am not sure of. That it, it is not clear why we would need fw_status_wait_timeout() here, and the code history does not reveal that either. Obviously this is a no-op for for non UMH kenrels *but* even for UMH -- why do we need it? Perhaps Takashi might know... Luis