Re: [RFC PATCH 11/13] driver core: firmware: introducedevices_cache/uncache_firmwares

[Borislav Petkov]

On Wed, Jul 25, 2012 at 01:00:11AM +0800, Ming Lei wrote:
> This patches introduces the three helpers below:
> 
> 	void device_cache_firmwares(void)
> 	void device_uncache_firmwares(void)
> 	void device_uncache_firmwares_delay(unsigned long)

I kinda don't like the plural of firmware: "firmwares". Can we call
those
	device_cache_fw_images
	device_uncache_fw_images

or
	device_cache_fw_blobs

or whatever?

> so we can call device_cache_firmwares() to cache firmwares for
> all devices which need firmwares to work, and the device driver
> can get the firmware easily from kernel memory when system isn't
> readly for completing their requests of loading firmwares.
> 
> When system is ready for completing firmware loading, driver core
> can call device_uncache_firmwares() or its delay version to free
> the cached firmwares.
> 
> The above helpers should be used to cache device firmwares during
> system suspend/resume cycle in the following patches.
> 
> Signed-off-by: Ming Lei <ming.lei@xxxxxxxxxxxxx>
> ---
>  drivers/base/firmware_class.c |  236 +++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 230 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
> index c181e6d..7a96e75 100644
> --- a/drivers/base/firmware_class.c
> +++ b/drivers/base/firmware_class.c
> @@ -22,6 +22,10 @@
>  #include <linux/slab.h>
>  #include <linux/sched.h>
>  #include <linux/list.h>
> +#include <linux/async.h>
> +#include <linux/pm.h>
> +
> +#include "base.h"
>  
>  MODULE_AUTHOR("Manuel Estrada Sainz");
>  MODULE_DESCRIPTION("Multi purpose firmware loading support");
> @@ -91,6 +95,17 @@ struct firmware_cache {
>  	/* firmware_buf instance will be added into the below list */
>  	spinlock_t lock;
>  	struct list_head head;
> +
> +	/*
> +	 * Name of firmware which has been cached successfully will be
> +	 * added into the below list so that device uncache helper can
> +	 * trace which firmware has been cached before.
> +	 */

Comment above the list_head and maybe call the list_head fw_names or so?

> +	spinlock_t name_lock;
> +	struct list_head name_head;
> +	wait_queue_head_t      wait_queue;

Stray \t

> +	int cnt;
> +	struct delayed_work work;
>  };
>  
>  struct firmware_buf {
> @@ -107,6 +122,11 @@ struct firmware_buf {
>  	char fw_id[];
>  };
>  
> +struct fw_name_for_cache {
> +	struct list_head list;
> +	char name[];
> +};

Maybe fw_cache_entry?

> +
>  struct firmware_priv {
>  	struct timer_list timeout;
>  	bool nowait;
> @@ -214,12 +234,6 @@ static void fw_free_buf(struct firmware_buf *buf)
>  	kref_put(&buf->ref, __fw_free_buf);
>  }
>  
> -static void fw_cache_init(void)
> -{
> -	spin_lock_init(&fw_cache.lock);
> -	INIT_LIST_HEAD(&fw_cache.head);
> -}
> -
>  static struct firmware_priv *to_firmware_priv(struct device *dev)
>  {
>  	return container_of(dev, struct firmware_priv, dev);
> @@ -981,6 +995,216 @@ int uncache_firmware(const char *fw_name)
>  	return -EINVAL;
>  }
>  
> +static struct fw_name_for_cache *alloc_fw_name_cache(const char *name)
> +{
> +	struct fw_name_for_cache *nc;
> +
> +	nc = kzalloc(sizeof(nc) + strlen(name) + 1, GFP_KERNEL);
> +	if (!nc)
> +		goto exit;
> +
> +	strcpy(nc->name, name);
> +exit:
> +	return nc;
> +}
> +
> +static void free_fw_name_cache(struct fw_name_for_cache *nc)
> +{
> +	kfree(nc);
> +}
> +
> +static void __async_dev_cache_firmware(void *fw_name,
> +		async_cookie_t cookie)

Arg alignment.

> +{
> +	struct fw_name_for_cache *nc;
> +	struct firmware_cache *fwc = &fw_cache;
> +	char *curr_name;
> +	int ret;
> +
> +	/* 'fw_name' is stored in devres, and it may be released,
> +	 * so allocate buffer to store the firmware name
> +	 */
> +	curr_name = kstrdup((const char *)fw_name, GFP_KERNEL);
> +	if (!curr_name) {
> +		ret = -ENOMEM;
> +		goto drop_ref;
> +	}
> +
> +	strcpy(curr_name, fw_name);

AFAICT kstrdup already copies the existing string, why do you need to
strcpy it again?

> +
> +	ret = cache_firmware(curr_name);
> +
> +	if (!ret) {

Invert check logic to save an indentation level:

	if (ret)
		goto free;

	nc = alloc...

	...

 free:
 	kfree(curr_name);

> +		/*
> +		 * allocate/all the instance of alloc_fw_name_cache
> +		 * for uncaching later if cache_firmware returns
> +		 * successfully
> +		 */
> +		nc = alloc_fw_name_cache(curr_name);
> +
> +		/*
> +		 * have to uncache firmware in case of allocation
> +		 * failure since we can't trace the firmware cache
> +		 * any more without the firmware name.
> +		 */
> +		if (!nc) {
> +			uncache_firmware(curr_name);
> +		} else {
> +			spin_lock(&fwc->name_lock);
> +			list_add(&nc->list, &fwc->name_head);
> +			spin_unlock(&fwc->name_lock);
> +		}
> +	}
> +	kfree(curr_name);
> +
> +drop_ref:
> +	spin_lock(&fwc->name_lock);
> +	fwc->cnt--;
> +	spin_unlock(&fwc->name_lock);
> +	wake_up(&fwc->wait_queue);
> +}
> +
> +static void __dev_cache_firmware(struct device *dev, void *res)
> +{
> +	struct fw_name_devm *fwn = res;
> +	const char *fw_name = fwn->name;
> +	struct firmware_cache *fwc = &fw_cache;
> +
> +	dev_dbg(dev, "fw-%s %d\n", fw_name, fwc->cnt);
> +
> +	spin_lock(&fwc->name_lock);
> +	fwc->cnt++;
> +	spin_unlock(&fwc->name_lock);
> +
> +	async_schedule(__async_dev_cache_firmware, (void *)fw_name);
> +}
> +
> +static int devm_name_match(struct device *dev, void *res,
> +		void *match_data)

arg alignment

> +{
> +	struct fw_name_devm *fwn = res;
> +	return (fwn->magic == (unsigned long)match_data);
> +}
> +
> +static void dev_cache_firmware(struct device *dev)
> +{
> +	devres_for_each_res(dev, fw_name_devm_release,
> +			devm_name_match, &fw_cache,
> +			__dev_cache_firmware);

arg alignment

> +}
> +
> +static void __device_uncache_firmwares(void)
> +{
> +	struct firmware_cache *fwc = &fw_cache;
> +	struct fw_name_for_cache *nc;
> +
> +	spin_lock(&fwc->name_lock);
> +	while (!list_empty(&fwc->name_head)) {
> +		nc = list_entry(fwc->name_head.next,
> +				struct fw_name_for_cache, list);
> +		list_del(&nc->list);
> +		spin_unlock(&fwc->name_lock);
> +
> +		uncache_firmware(nc->name);
> +		free_fw_name_cache(nc);
> +
> +		spin_lock(&fwc->name_lock);
> +	}
> +	spin_unlock(&fwc->name_lock);

Same thing here: maybe check if on the last element on the list and
don't grab the lock then?

> +}
> +
> +extern struct list_head dpm_list;

checkpatch says here:

WARNING: externs should be avoided in .c files
#181: FILE: drivers/base/firmware_class.c:1118:
+extern struct list_head dpm_list;

and I think it is correct. AFAICT, it should be included from
<drivers/base/power/power.h>...

> +/**
> + * device_cache_firmwares - cache devices' firmwares
> + *
> + * For each devices, if they called request_firmware or
> + * request_firmware_nowait successfully before, their firmware
> + * name will be recored into these devices' devres link list, so
> + * device_cache_firmwares can call cache_firmware() to cache these
> + * firmwares for these devices, then these device drivers can load
> + * their firmwares easily at any time even when system is not ready
> + * to complete loading firmwares.

This is a one looong sentence. Please simplify.

> + *
> + */
> +static void device_cache_firmwares(void)
> +{
> +	struct firmware_cache *fwc = &fw_cache;
> +	struct device *dev;
> +	DEFINE_WAIT(wait);
> +
> +	pr_debug("%s\n", __func__);

No real need for that debug printk since you have one below.

> +
> +	device_pm_lock();
> +	list_for_each_entry(dev, &dpm_list, power.entry)
> +		dev_cache_firmware(dev);
> +	device_pm_unlock();
> +
> +	pr_debug("%s firmwares %d\n", __func__, fwc->cnt);
> +
> +	/* wait for completion of caching firmware for all devices */
> +	spin_lock(&fwc->name_lock);
> +	for (;;) {
> +		prepare_to_wait(&fwc->wait_queue, &wait,
> +				TASK_UNINTERRUPTIBLE);
> +		if (!fwc->cnt)
> +			break;
> +
> +		spin_unlock(&fwc->name_lock);
> +
> +		schedule();
> +
> +		spin_lock(&fwc->name_lock);
> +	}
> +	spin_unlock(&fwc->name_lock);
> +	finish_wait(&fwc->wait_queue, &wait);
> +}
> +
> +/**
> + * device_uncache_firmwares - uncache devices' firmwares
> + *
> + * uncache all firmwares which have been cached successfully
> + * by device_uncache_firmwares

    "by device_cache_firmwares earlier."

> + *
> + */
> +static void device_uncache_firmwares(void)
> +{
> +	pr_debug("%s\n", __func__);
> +	__device_uncache_firmwares();
> +}
> +

Ok, the rest of the patches tomorrow.

-- 
Regards/Gruss,
Boris.

Advanced Micro Devices GmbH
Einsteinring 24, 85609 Dornach
GM: Alberto Bozzo
Reg: Dornach, Landkreis Muenchen
HRB Nr. 43632 WEEE Registernr: 129 19551
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/