On Wed, Sep 07, 2016 at 10:45:06AM +0200, Daniel Wagner wrote:
> From: Daniel Wagner <[email protected]>
>
> 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 <[email protected]>
> Cc: Ming Lei <[email protected]>
> Cc: Luis R. Rodriguez <[email protected]>
> Cc: Greg Kroah-Hartman <[email protected]>
> ---
> 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
