Messages in this thread | | | Date | Tue, 5 Aug 2008 14:03:27 -0700 | From | Andrew Morton <> | Subject | Re: [PATCH] firmware: avoiding multiple replication for same firmware file |
| |
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); > +} > +
| |