Re: [PATCH] firmware: avoiding multiple replication for same firmware file

[Andrew Morton <akpm@linux-foundation.org>]

On Fri, 01 Aug 2008 11:30:59 +0530
Jaswinder Singh <jaswinder@infradead.org> wrote:

> Now when request_firmware will be called it will check whether firmware
> is already allocated or not. If already allocated then it provide handle
> of old firmware handle and increase count of firmware.
> 
> release_firmware will decrease count of firmware, if count is one only then
> firmware will be release.
> 
> Added release_firmware_all() can be called from driver cleanup or exit
> to release firmware forcefully.
> 
> Signed-off-by: Jaswinder Singh <jaswinder@infradead.org>
> ---
>  drivers/base/firmware_class.c |  112 ++++++++++++++++++++++++++++++++++++-----
>  include/linux/firmware.h      |    5 ++
>  2 files changed, 104 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
> index b0be1d1..9743a3d 100644
> --- a/drivers/base/firmware_class.c
> +++ b/drivers/base/firmware_class.c
> @@ -49,6 +49,15 @@ struct firmware_priv {
>  	struct timer_list timeout;
>  };
>  
> +struct firmware_list {
> +	const struct firmware *fw;
> +	char *name;
> +	int count;
> +	struct list_head list;
> +};

OK, that's a per-item data structure.

> +static struct firmware_list firmwarelist;

No, we shouldn't declare an entire static `struct firmware_list' just
for storage of all the dynamically allocated ones. Instead simply do:

static LIST_HEAD(firmwarelist);

and use that.

Please also add a comment indicating which lock protects that global
list.  If it is indeed protected.  If not, fix it.  If no fix is
needed, add a comment explaining why this list doesn't need locking.

>  #ifdef CONFIG_FW_LOADER
>  extern struct builtin_fw __start_builtin_fw[];
>  extern struct builtin_fw __end_builtin_fw[];
> @@ -400,11 +409,21 @@ _request_firmware(const struct firmware **firmware_p, const char *name,
>  	struct firmware_priv *fw_priv;
>  	struct firmware *firmware;
>  	struct builtin_fw *builtin;
> +	struct firmware_list *tmp;
>  	int retval;
>  
>  	if (!firmware_p)
>  		return -EINVAL;
>  
> +	/* Return firmware pointer from firmware list if already allocated */
> +	list_for_each_entry(tmp, &firmwarelist.list, list) {

And this becomes

	list_for_each_entry(tmp, &firmwarelist, list)

> +		if (strcmp(name, tmp->name) == 0) {
> +			tmp->count++;
> +			*firmware_p = tmp->fw;
> +			break;
> +		}
> +	}
> +
>  	*firmware_p = firmware = kzalloc(sizeof(*firmware), GFP_KERNEL);
>  	if (!firmware) {
>  		printk(KERN_ERR "%s: kmalloc(struct firmware) failed\n",

I don't think that existing printk was needed - the page allocator (or
the oom-killer) will dump information for us in the unlikely event that
this allocation failed.

> @@ -413,6 +432,27 @@ _request_firmware(const struct firmware **firmware_p, const char *name,
>  		goto out;
>  	}
>  
> +	tmp = kzalloc(sizeof(struct firmware_list), GFP_KERNEL);
> +	if (!tmp) {
> +		printk(KERN_ERR "%s: kmalloc(struct firmware_list) failed\n",
> +		       __func__);

So the newly-added ones don't have much value.

> +		retval = -ENOMEM;
> +		goto error_kfree_fw;
> +	}
> +
> +	tmp->name = kzalloc(strlen(name), GFP_KERNEL);

Could have been kmalloc(), but see below.

> +	if (!tmp->name) {
> +		printk(KERN_ERR "%s: kmalloc firmware_list->name failed\n",
> +		       __func__);

Unneeeded printk.

> +		retval = -ENOMEM;
> +		goto error_kfree_fw_list;
> +	}
> +
> +	tmp->fw = *firmware_p;
> +	tmp->count = 1;
> +	strcpy(tmp->name, name);

Please replace the kmalloc+strcpy with a call to kstrdup().

> +	list_add(&(tmp->list), &(firmwarelist.list));

That is excessively parenthesised.

There is no locking for that list.

>  	for (builtin = __start_builtin_fw; builtin != __end_builtin_fw;
>  	     builtin++) {
>  		if (strcmp(name, builtin->name))
> @@ -429,7 +469,7 @@ _request_firmware(const struct firmware **firmware_p, const char *name,
>  
>  	retval = fw_setup_device(firmware, &f_dev, name, device, uevent);
>  	if (retval)
> -		goto error_kfree_fw;
> +		goto error_kfree_fw_name;
>  
>  	fw_priv = dev_get_drvdata(f_dev);
>
> ...
>
> -/**
> +static void __release_firmware(const struct firmware *fw,
> +			       struct firmware_list *tmp)
> +{
> +	struct builtin_fw *builtin;
> +
> +	for (builtin = __start_builtin_fw; builtin != __end_builtin_fw;
> +	     builtin++) {
> +		if (fw->data == builtin->data)
> +			goto free_fw;
> +	}
> +	vfree(fw->data);
> +free_fw:
> +	kfree(fw);
> +	list_del(&tmp->list);

locking.

> +	kfree(tmp->name);
> +	kfree(tmp);
> +}
> +