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

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

[Jaswinder Singh]

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;
+};
+
+static struct firmware_list firmwarelist;
+
 #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) {
+		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",
@@ -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__);
+		retval = -ENOMEM;
+		goto error_kfree_fw;
+	}
+
+	tmp->name = kzalloc(strlen(name), GFP_KERNEL);
+	if (!tmp->name) {
+		printk(KERN_ERR "%s: kmalloc firmware_list->name failed\n",
+		       __func__);
+		retval = -ENOMEM;
+		goto error_kfree_fw_list;
+	}
+
+	tmp->fw = *firmware_p;
+	tmp->count = 1;
+	strcpy(tmp->name, name);
+	list_add(&(tmp->list), &(firmwarelist.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);
 
@@ -451,12 +491,20 @@ _request_firmware(const struct firmware **firmware_p, const char *name,
 		retval = -ENOENT;
 		release_firmware(fw_priv->fw);
 		*firmware_p = NULL;
+		list_del(&tmp->list);
+		kfree(tmp->name);
+		kfree(tmp);
 	}
 	fw_priv->fw = NULL;
 	mutex_unlock(&fw_lock);
 	device_unregister(f_dev);
 	goto out;
 
+error_kfree_fw_name:
+	list_del(&tmp->list);
+	kfree(tmp->name);
+error_kfree_fw_list:
+	kfree(tmp);
 error_kfree_fw:
 	kfree(firmware);
 	*firmware_p = NULL;
@@ -487,24 +535,60 @@ request_firmware(const struct firmware **firmware_p, const char *name,
         return _request_firmware(firmware_p, name, device, uevent);
 }
 
-/**
+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);
+	kfree(tmp->name);
+	kfree(tmp);
+}
+
+/*
  * release_firmware: - release the resource associated with a firmware image
  * @fw: firmware resource to release
- **/
-void
-release_firmware(const struct firmware *fw)
+ */
+void release_firmware(const struct firmware *fw)
 {
-	struct builtin_fw *builtin;
+	struct firmware_list *tmp;
 
 	if (fw) {
-		for (builtin = __start_builtin_fw; builtin != __end_builtin_fw;
-		     builtin++) {
-			if (fw->data == builtin->data)
-				goto free_fw;
+		list_for_each_entry(tmp, &firmwarelist.list, list) {
+			if (fw == tmp->fw) {
+				if (tmp->count > 1) {
+					tmp->count--;
+					return;
+				} else
+					break;
+			}
 		}
-		vfree(fw->data);
-	free_fw:
-		kfree(fw);
+		__release_firmware(fw, tmp);
+	}
+}
+
+/*
+ * release_firmware_all: - release the resource associated with a firmware
+ * image forcefully
+ * @fw: firmware resource to release
+ */
+void release_firmware_all(const struct firmware *fw)
+{
+	struct firmware_list *tmp;
+
+	if (fw) {
+		list_for_each_entry(tmp, &firmwarelist.list, list)
+			if (fw == tmp->fw)
+				break;
+		__release_firmware(fw, tmp);
 	}
 }
 
@@ -610,6 +694,8 @@ firmware_class_init(void)
 		       __func__);
 		class_unregister(&firmware_class);
 	}
+
+	INIT_LIST_HEAD(&firmwarelist.list);
 	return error;
 
 }
diff --git a/include/linux/firmware.h b/include/linux/firmware.h
index c8ecf5b..238f1e4 100644
--- a/include/linux/firmware.h
+++ b/include/linux/firmware.h
@@ -43,6 +43,7 @@ int request_firmware_nowait(
 	void (*cont)(const struct firmware *fw, void *context));
 
 void release_firmware(const struct firmware *fw);
+void release_firmware_all(const struct firmware *fw);
 #else
 static inline int request_firmware(const struct firmware **fw,
 				   const char *name,
@@ -61,6 +62,10 @@ static inline int request_firmware_nowait(
 static inline void release_firmware(const struct firmware *fw)
 {
 }
+
+static inline void release_firmware_all(const struct firmware *fw)
+{
+}
 #endif
 
 #endif
-- 
1.5.5.1

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

[David Woodhouse]

On Fri, 2008-08-01 at 11:30 +0530, Jaswinder Singh 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.

I like this. It means that when you have multiple similar devices, you
only need one copy of the firmware in memory for _all_ of them. Because
we've now made fw->data constant, we know it's safe to share them now.

I wonder if we should change the implementation of 'built-in firmware'
to use your new implementation -- just set the use count on the built-in
firmware to 1 when we start up, and then it should never go away.

Unless we're _really_ clever with linker tricks it would mean we'd have
an init function which takes the built-in stuff and builds it into the
linked list, but I think that might be worth the runtime simplification.

-- 
David Woodhouse                            Open Source Technology Centre
David.Woodhouse@intel.com                              Intel Corporation

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

[Andrew Morton]

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);
> +}
> +

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

[Jaswinder Singh]

Hello Andrew,

On Tue, 2008-08-05 at 14:03 -0700, Andrew Morton wrote:
> On Fri, 01 Aug 2008 11:30:59 +0530

> 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.
> 

Thanks for your feedbacks.

You can also check my other firmware patches from :
http://git.infradead.org/users/jaswinder/firm-jsr-2.6.git

Here is updated patch:

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

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 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 if count != 0.

release_firmware and release_firmware_all is safe
No need to check for fw for !NULL before calling them.

Signed-off-by: Jaswinder Singh <jaswinder@infradead.org>
---
 drivers/base/firmware_class.c |  154 +++++++++++++++++++++++++++++++++++------
 include/linux/firmware.h      |    5 ++
 2 files changed, 137 insertions(+), 22 deletions(-)

diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
index c9c92b0..0ba8857 100644
--- a/drivers/base/firmware_class.c
+++ b/drivers/base/firmware_class.c
@@ -49,6 +49,18 @@ struct firmware_priv {
 	struct timer_list timeout;
 };
 
+/* Created firmware link list to avoiding multiple replication
+ * for same firmware file */
+struct firmware_list {
+	const struct firmware *fw;
+	char *name;
+	int count;
+	struct list_head list;
+};
+
+/* Protected by fw_lock */
+static LIST_HEAD(firmwarelist);
+
 #ifdef CONFIG_FW_LOADER
 extern struct builtin_fw __start_builtin_fw[];
 extern struct builtin_fw __end_builtin_fw[];
@@ -394,36 +406,63 @@ _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)
+		if (strcmp(name, tmp->name) == 0) {
+			mutex_lock(&fw_lock);
+			tmp->count++;
+			*firmware_p = tmp->fw;
+			mutex_unlock(&fw_lock);
+			printk(KERN_INFO "firmware: requesting %s count %d\n",
+			       name, tmp->count);
+			return 0;
+		}
+
 	*firmware_p = firmware = kzalloc(sizeof(*firmware), GFP_KERNEL);
-	if (!firmware) {
-		printk(KERN_ERR "%s: kmalloc(struct firmware) failed\n",
-		       __func__);
+	if (!firmware)
+		return -ENOMEM;
+
+	mutex_lock(&fw_lock);
+	tmp = kzalloc(sizeof(struct firmware_list), GFP_KERNEL);
+	if (!tmp) {
 		retval = -ENOMEM;
-		goto out;
+		goto error_kfree_fw;
+	}
+	tmp->name = kstrdup(name, GFP_KERNEL);
+	if (!tmp->name) {
+		retval = -ENOMEM;
+		goto error_kfree_fw_list;
 	}
+	tmp->fw = *firmware_p;
+	tmp->count = 1;
+	list_add(&tmp->list, &firmwarelist);
+	mutex_unlock(&fw_lock);
 
 	for (builtin = __start_builtin_fw; builtin != __end_builtin_fw;
 	     builtin++) {
 		if (strcmp(name, builtin->name))
 			continue;
-		printk(KERN_INFO "firmware: using built-in firmware %s\n",
-		       name);
+		printk(KERN_INFO
+		       "firmware: using built-in firmware %s count %d\n",
+		       name, tmp->count);
 		firmware->size = builtin->size;
 		firmware->data = builtin->data;
 		return 0;
 	}
 
 	if (uevent)
-		printk(KERN_INFO "firmware: requesting %s\n", name);
+		printk(KERN_INFO "firmware: requesting %s count %d\n",
+		       name, tmp->count);
 
 	retval = fw_setup_device(firmware, &f_dev, name, device, uevent);
 	if (retval)
-		goto error_kfree_fw;
+		goto error_kfree_fw_llist;
 
 	fw_priv = dev_get_drvdata(f_dev);
 
@@ -445,12 +484,22 @@ _request_firmware(const struct firmware **firmware_p, const char *name,
 		retval = -ENOENT;
 		release_firmware(fw_priv->fw);
 		*firmware_p = NULL;
+		list_del(&tmp->list);
+		kfree(tmp->name);
+		kfree(tmp);
 	}
 	fw_priv->fw = NULL;
 	mutex_unlock(&fw_lock);
 	device_unregister(f_dev);
 	goto out;
 
+error_kfree_fw_llist:
+	mutex_lock(&fw_lock);
+	list_del(&tmp->list);
+	kfree(tmp->name);
+error_kfree_fw_list:
+	kfree(tmp);
+	mutex_unlock(&fw_lock);
 error_kfree_fw:
 	kfree(firmware);
 	*firmware_p = NULL;
@@ -459,7 +508,10 @@ out:
 }
 
 /**
- * request_firmware: - send firmware request and wait for it
+ * request_firmware: - Checks whether firmware is already allocated or not.
+ * If already allocated then it provide handle of old firmware and
+ * increase count of firmware.
+ * If not allocated then send firmware request and wait for it.
  * @firmware_p: pointer to firmware image
  * @name: name of firmware file
  * @device: device for which firmware is being loaded
@@ -481,24 +533,80 @@ request_firmware(const struct firmware **firmware_p, const char *name,
         return _request_firmware(firmware_p, name, device, uevent);
 }
 
-/**
- * release_firmware: - release the resource associated with a firmware image
- * @fw: firmware resource to release
- **/
-void
-release_firmware(const struct firmware *fw)
+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);
+	mutex_lock(&fw_lock);
+	list_del(&tmp->list);
+	kfree(tmp->name);
+	kfree(tmp);
+	mutex_unlock(&fw_lock);
+}
+
+/*
+ * release_firmware: - decrease count of firmware, if count is one only then
+ * release the resource associated with a firmware image
+ * @fw: firmware resource to release
+ *
+ * release_firmware is safe you can call release_firmware(NULL) so
+ * no need to check for fw for !NULL before calling release_firmware
+ */
+void release_firmware(const struct firmware *fw)
+{
+	struct firmware_list *tmp;
+
+	if (fw)
+		list_for_each_entry(tmp, &firmwarelist, list)
+			if (fw == tmp->fw) {
+				printk(KERN_INFO
+				       "firmware: releasing %s count %d\n",
+				       tmp->name, tmp->count);
+				mutex_lock(&fw_lock);
+				tmp->count--;
+				mutex_unlock(&fw_lock);
+				if (tmp->count == 0)
+					__release_firmware(fw, tmp);
+				return;
+			}
+}
+
+/*
+ * release_firmware_all: - release the resource associated with a firmware
+ * image forcefully if count != 0
+ * @fw: firmware resource to release
+ *
+ * release_firmware_all is safe you can call release_firmware_all(NULL) so
+ * no need to check for fw for !NULL before calling release_firmware_all
+ *
+ *  release_firmware_all() can be called from driver cleanup or exit
+ *  to release firmware forcefully
+ */
+void release_firmware_all(const struct firmware *fw)
+{
+	struct firmware_list *tmp;
+
 	if (fw) {
-		for (builtin = __start_builtin_fw; builtin != __end_builtin_fw;
-		     builtin++) {
-			if (fw->data == builtin->data)
-				goto free_fw;
+		list_for_each_entry(tmp, &firmwarelist, list)
+			if (fw == tmp->fw)
+				break;
+		printk(KERN_INFO "firmware: releasing all %s count %d\n",
+		       tmp->name, tmp->count);
+		if (tmp->count) {
+			mutex_lock(&fw_lock);
+			tmp->count = 0;
+			mutex_unlock(&fw_lock);
+			__release_firmware(fw, tmp);
 		}
-		vfree(fw->data);
-	free_fw:
-		kfree(fw);
 	}
 }
 
@@ -604,6 +712,8 @@ firmware_class_init(void)
 		       __func__);
 		class_unregister(&firmware_class);
 	}
+
+	INIT_LIST_HEAD(&firmwarelist);
 	return error;
 
 }
diff --git a/include/linux/firmware.h b/include/linux/firmware.h
index c8ecf5b..238f1e4 100644
--- a/include/linux/firmware.h
+++ b/include/linux/firmware.h
@@ -43,6 +43,7 @@ int request_firmware_nowait(
 	void (*cont)(const struct firmware *fw, void *context));
 
 void release_firmware(const struct firmware *fw);
+void release_firmware_all(const struct firmware *fw);
 #else
 static inline int request_firmware(const struct firmware **fw,
 				   const char *name,
@@ -61,6 +62,10 @@ static inline int request_firmware_nowait(
 static inline void release_firmware(const struct firmware *fw)
 {
 }
+
+static inline void release_firmware_all(const struct firmware *fw)
+{
+}
 #endif
 
 #endif
-- 
1.5.5.1

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

[Jaswinder Singh]

Sorry, minor correction.

On Wed, 2008-08-06 at 15:05 +0530, Jaswinder Singh wrote:
> +
> +	mutex_lock(&fw_lock);
> +	tmp = kzalloc(sizeof(struct firmware_list), GFP_KERNEL);
> +	if (!tmp) {
>  		retval = -ENOMEM;
> -		goto out;
> +		goto error_kfree_fw;
> +	}
> +	tmp->name = kstrdup(name, GFP_KERNEL);
> +	if (!tmp->name) {
> +		retval = -ENOMEM;
> +		goto error_kfree_fw_list;
>  	}

diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
index 0ba8857..c886113 100644
--- a/drivers/base/firmware_class.c
+++ b/drivers/base/firmware_class.c
@@ -428,12 +428,12 @@ _request_firmware(const struct firmware **firmware_p, const char *name,
 	if (!firmware)
 		return -ENOMEM;
 
-	mutex_lock(&fw_lock);
 	tmp = kzalloc(sizeof(struct firmware_list), GFP_KERNEL);
 	if (!tmp) {
 		retval = -ENOMEM;
 		goto error_kfree_fw;
 	}
+	mutex_lock(&fw_lock);
 	tmp->name = kstrdup(name, GFP_KERNEL);
 	if (!tmp->name) {
 		retval = -ENOMEM;

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

[Jaswinder Singh]

On Wed, 2008-08-06 at 15:05 +0530, Jaswinder Singh wrote:
> @@ -445,12 +484,22 @@ _request_firmware(const struct firmware **firmware_p, const char *name,
>  		retval = -ENOENT;
>  		release_firmware(fw_priv->fw);
>  		*firmware_p = NULL;
> +		list_del(&tmp->list);
> +		kfree(tmp->name);
> +		kfree(tmp);
>  	}
>  	fw_priv->fw = NULL;
>  	mutex_unlock(&fw_lock);

This is also not required as we are doing this in release_firmware().
So :

diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
index c886113..7b268d1 100644
--- a/drivers/base/firmware_class.c
+++ b/drivers/base/firmware_class.c
@@ -484,9 +484,6 @@ _request_firmware(const struct firmware **firmware_p, const char *name,
 		retval = -ENOENT;
 		release_firmware(fw_priv->fw);
 		*firmware_p = NULL;
-		list_del(&tmp->list);
-		kfree(tmp->name);
-		kfree(tmp);
 	}
 	fw_priv->fw = NULL;
 	mutex_unlock(&fw_lock);