Re: [RFC v0 3/8] firmware: Factor out firmware load helpers

[Dan Williams]

On Thu, 2016-07-28 at 09:55 +0200, Daniel Wagner wrote:
> From: Daniel Wagner <daniel.wagner@xxxxxxxxxxxx>
> 
> 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@xxxxxxxxxxxx>
> ---
> Â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