Re: [PATCH v4 2/4] firmware: encapsulate firmware loading status

["Luis R. Rodriguez" <mcgrof@kernel.org>]

On Wed, Sep 07, 2016 at 10:45:06AM +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 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;
> +
> +	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