Date:	Thu, 28 Jul 2016 10:01:01 -0500
From:	Dan Williams <dcbw@...hat.com>
To:	Daniel Wagner <wagi@...om.org>, Bastien Nocera <hadess@...ess.net>,
	Bjorn Andersson <bjorn.andersson@...aro.org>,
	Dmitry Torokhov <dmitry.torokhov@...il.com>,
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	Johannes Berg <johannes.berg@...el.com>,
	Kalle Valo <kvalo@...eaurora.org>,
	Ohad Ben-Cohen <ohad@...ery.com>
Cc:	linux-input@...r.kernel.org, linux-kselftest@...r.kernel.org,
	linux-wireless@...r.kernel.org, linux-kernel@...r.kernel.org,
	Daniel Wagner <daniel.wagner@...-carit.de>
Subject: Re: [RFC v0 3/8] firmware: Factor out firmware load helpers

On Thu, 2016-07-28 at 09:55 +0200, Daniel Wagner wrote:
> From: Daniel Wagner <daniel.wagner@...-carit.de>
> 
> Factor out the firmware loading synchronization code in order to
> allow
> drivers to reuse it. This also documents more clearly what is
> happening. This is especial useful the drivers which will be
> converted
> afterwards. Not everyone has to come with yet another way to handle
> it.

It's somewhat odd to me that the structure is "firmware_stat" but most
of the functions are "fw_loading_*".  That seems inconsistent for a
structure that is (currently) only used by these functions.

I would personally do either:

a) "struct fw_load_status" and "fw_load_*()"

or

b) "struct firmware_load_stat" and "firmware_load_*()"

I'd also change the function names from "loading" -> "load", similar to
how userland has read(2), not reading(2).

Dan

> We use swait instead completion. complete() would only wake up one
> waiter, so complete_all() is used. complete_all() wakes max
> MAX_UINT/2
> waiters which is plenty in all cases. Though withh swait we just wait
> until the exptected status shows up and wake any waiter.
> 
> Signed-off-by: Daniel Wagner <daniel.wagner@...-carit.de>
> ---
>  drivers/base/firmware_class.c | 112 +++++++++++++++++++-------------
> ----------
>  include/linux/firmware.h      |  71 ++++++++++++++++++++++++++
>  2 files changed, 122 insertions(+), 61 deletions(-)
> 
> diff --git a/drivers/base/firmware_class.c
> b/drivers/base/firmware_class.c
> index 773fc30..bf1ca70 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>
>  
> @@ -85,11 +86,36 @@ static inline bool fw_is_builtin_firmware(const
> struct firmware *fw)
>  }
>  #endif
>  
> -enum {
> -	FW_STATUS_LOADING,
> -	FW_STATUS_DONE,
> -	FW_STATUS_ABORT,
> -};
> +
> +#if defined(CONFIG_FW_LOADER) || (defined(CONFIG_FW_LOADER_MODULE)
> && defined(MODULE))
> +
> +static inline bool is_fw_sync_done(unsigned long status)
> +{
> +	return status == FW_STATUS_LOADED || status ==
> FW_STATUS_ABORT;
> +}
> +
> +int __firmware_stat_wait(struct firmware_stat *fwst,
> +				long timeout)
> +{
> +	int err;
> +	err = swait_event_interruptible_timeout(fwst->wq,
> +				is_fw_sync_done(READ_ONCE(fwst-
> >status)),
> +				timeout);
> +	if (err == 0 && fwst->status == FW_STATUS_ABORT)
> +		return -ENOENT;
> +
> +	return err;
> +}
> +EXPORT_SYMBOL(__firmware_stat_wait);
> +
> +void __firmware_stat_set(struct firmware_stat *fwst, unsigned long
> status)
> +{
> +	WRITE_ONCE(fwst->status, status);
> +	swake_up(&fwst->wq);
> +}
> +EXPORT_SYMBOL(__firmware_stat_set);
> +
> +#endif
>  
>  static int loading_timeout = 60;	/* In seconds */
>  
> @@ -138,9 +164,8 @@ struct firmware_cache {
>  struct firmware_buf {
>  	struct kref ref;
>  	struct list_head list;
> -	struct completion completion;
> +	struct firmware_stat fwst;
>  	struct firmware_cache *fwc;
> -	unsigned long status;
>  	void *data;
>  	size_t size;
>  #ifdef CONFIG_FW_LOADER_USER_HELPER
> @@ -194,7 +219,7 @@ static struct firmware_buf
> *__allocate_fw_buf(const char *fw_name,
>  
>  	kref_init(&buf->ref);
>  	buf->fwc = fwc;
> -	init_completion(&buf->completion);
> +	firmware_stat_init(&buf->fwst);
>  #ifdef CONFIG_FW_LOADER_USER_HELPER
>  	INIT_LIST_HEAD(&buf->pending_list);
>  #endif
> @@ -292,15 +317,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)
>  {
> @@ -339,7 +355,7 @@ static int fw_get_filesystem_firmware(struct
> device *device,
>  		}
>  		dev_dbg(device, "direct-loading %s\n", buf->fw_id);
>  		buf->size = size;
> -		fw_finish_direct_load(device, buf);
> +		fw_loading_done(buf->fwst);
>  		break;
>  	}
>  	__putname(path);
> @@ -457,12 +473,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 (is_fw_loading_done(buf->fwst))
>  		return;
>  
>  	list_del_init(&buf->pending_list);
> -	set_bit(FW_STATUS_ABORT, &buf->status);
> -	complete_all(&buf->completion);
> +	fw_loading_abort(buf->fwst);
>  }
>  
>  static void fw_load_abort(struct firmware_priv *fw_priv)
> @@ -475,9 +490,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 */
> @@ -577,7 +589,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 = is_fw_loading(fw_priv->buf->fwst);
>  	mutex_unlock(&fw_lock);
>  
>  	return sprintf(buf, "%d\n", loading);
> @@ -632,23 +644,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 (!is_fw_loading_done(fw_buf->fwst)) {
>  			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_loading_start(fw_buf->fwst);
>  		}
>  		break;
>  	case 0:
> -		if (test_bit(FW_STATUS_LOADING, &fw_buf->status)) {
> +		if (is_fw_loading(fw_buf->fwst)) {
>  			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
> @@ -670,10 +679,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_loading_abort(fw_buf->fwst);
>  				written = rc;
> +			} else {
> +				fw_loading_done(fw_buf->fwst);
>  			}
> -			complete_all(&fw_buf->completion);
>  			break;
>  		}
>  		/* fallthrough */
> @@ -681,7 +691,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_loading_abort(fw_buf->fwst);
>  		break;
>  	}
>  out:
> @@ -702,7 +712,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 || is_fw_loading_done(buf->fwst)) {
>  		ret_count = -ENODEV;
>  		goto out;
>  	}
> @@ -799,7 +809,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 || is_fw_loading_done(buf->fwst)) {
>  		retval = -ENODEV;
>  		goto out;
>  	}
> @@ -917,8 +927,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_loading_wait_timeout(buf->fwst, timeout);
>  	if (retval == -ERESTARTSYS || !retval) {
>  		mutex_lock(&fw_lock);
>  		fw_load_abort(fw_priv);
> @@ -927,7 +936,7 @@ static int _request_firmware_load(struct
> firmware_priv *fw_priv,
>  		retval = 0;
>  	}
>  
> -	if (is_fw_load_aborted(buf))
> +	if (is_fw_loading_aborted(buf->fwst))
>  		retval = -EAGAIN;
>  	else if (!buf->data)
>  		retval = -ENOMEM;
> @@ -986,26 +995,6 @@ static inline void
> kill_requests_without_uevent(void) { }
>  
>  #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
> @@ -1039,7 +1028,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_loading_wait_timeout(buf->fwst,
> +					      firmware_loading_timeo
> ut());
>  		if (!ret) {
>  			fw_set_page_data(buf, firmware);
>  			return 0; /* assigned */
> @@ -1057,7 +1047,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 || is_fw_loading_aborted(buf->fwst)) {
>  		mutex_unlock(&fw_lock);
>  		return -ENOENT;
>  	}
> diff --git a/include/linux/firmware.h b/include/linux/firmware.h
> index 5c41c5e..f584160 100644
> --- a/include/linux/firmware.h
> +++ b/include/linux/firmware.h
> @@ -4,10 +4,17 @@
>  #include <linux/types.h>
>  #include <linux/compiler.h>
>  #include <linux/gfp.h>
> +#include <linux/swait.h>
>  
>  #define FW_ACTION_NOHOTPLUG 0
>  #define FW_ACTION_HOTPLUG 1
>  
> +enum {
> +	FW_STATUS_LOADING,
> +	FW_STATUS_LOADED,
> +	FW_STATUS_ABORT,
> +};
> +
>  struct firmware {
>  	size_t size;
>  	const u8 *data;
> @@ -17,6 +24,11 @@ struct firmware {
>  	void *priv;
>  };
>  
> +struct firmware_stat {
> +	unsigned long status;
> +	struct swait_queue_head wq;
> +};
> +
>  struct module;
>  struct device;
>  
> @@ -49,6 +61,36 @@ int request_firmware_direct(const struct firmware
> **fw, const char *name,
>  			    struct device *device);
>  
>  void release_firmware(const struct firmware *fw);
> +
> +static inline void firmware_stat_init(struct firmware_stat *fwst)
> +{
> +	init_swait_queue_head(&fwst->wq);
> +}
> +
> +static inline unsigned long __firmware_stat_get(struct firmware_stat
> *fwst)
> +{
> +	return READ_ONCE(fwst->status);
> +}
> +void __firmware_stat_set(struct firmware_stat *fwst, unsigned long
> status);
> +int __firmware_stat_wait(struct firmware_stat *fwst, long timeout);
> +
> +#define fw_loading_start(fwst)					
> \
> +	__firmware_stat_set(&fwst, FW_STATUS_LOADING)
> +#define fw_loading_done(fwst)					
> \
> +	__firmware_stat_set(&fwst, FW_STATUS_LOADED)
> +#define fw_loading_abort(fwst)					
> \
> +	__firmware_stat_set(&fwst, FW_STATUS_ABORT)
> +#define fw_loading_wait(fwst)					
> \
> +	__firmware_stat_wait(&fwst, 0)
> +#define fw_loading_wait_timeout(fwst, timeout)			
> \
> +	__firmware_stat_wait(&fwst, timeout)
> +#define is_fw_loading(fwst)					\
> +	(__firmware_stat_get(&fwst) == FW_STATUS_LOADING)
> +#define is_fw_loading_done(fwst)				\
> +	(__firmware_stat_get(&fwst) == FW_STATUS_LOADED)
> +#define is_fw_loading_aborted(fwst)				\
> +	(__firmware_stat_get(&fwst) == FW_STATUS_ABORT)
> +
>  #else
>  static inline int request_firmware(const struct firmware **fw,
>  				   const char *name,
> @@ -75,5 +117,34 @@ static inline int request_firmware_direct(const
> struct firmware **fw,
>  	return -EINVAL;
>  }
>  
> +static inline void firmware_stat_init(struct firmware_stat *fwst)
> +{
> +}
> +
> +static inline unsigned long __firmware_stat_get(struct firmware_stat
> *fwst)
> +{
> +	return -EINVAL;
> +}
> +
> +static inline void __firmware_stat_set(struct firmware_stat *fwst,
> +				       unsigned long status)
> +{
> +}
> +
> +static inline int __firmware_stat_wait(struct firmware_stat *fwst,
> +				       long timeout)
> +{
> +	return -EINVAL;
> +}
> +
> +#define fw_loading_start(fwst)
> +#define fw_loading_done(fwst)
> +#define fw_loading_abort(fwst)
> +#define fw_loading_wait(fwst)
> +#define fw_loading_wait_timeout(fwst, timeout)
> +#define is_fw_loading(fwst)		0
> +#define is_fw_loading_done(fwst)	0
> +#define is_fw_loading_aborted(fwst)	0
> +
>  #endif
>  #endif

Powered by blists - more mailing lists