[PATCH] request_firmware(): fixes and polishing.

[PATCH] request_firmware(): fixes and polishing.

[Manuel Estrada Sainz]

 Hi,

 Please apply.

 Dmitry Torokhov has been criticizing my code for some days (Thanks Dmitry),
 and here is the result. It should be ready for -mm tree.
 
 Simon Kelly tested the patch series and reported improvement with some
 problems he was having.

On Fri, 06 Feb 2004 07:52:31 +0000, Simon Kelley wrote:
> I'm seeing problems when request_firmware() is called from the open()
> routine of the atmel driver if that is called as a result of  process
> which was started from /sbin/hotplug.
 
 Each patch starts with a changelog, but I'll write a full changelog
 here for the casual reader:

 - 01_request_firmware-misc-fix.diff
	- use vfree to free vmalloc memory.
	- Make sure fw_setup_class_device sets *class_dev_p to NULL in
	  all case of error.
	- Fix error handling in firmware_class_init.

 - 02_request_firmware-misc-polish.diff
	- Take advantage of strlcpy.
	- Extra error logging.
	- Use struct coping instead of memcpy.
	- Put all aborting code in a single place, and fully abort if
	  fw_realloc_buffer fails.
	- Abort on unexpected 'loading' values.

 - 03_request_firmware-state-tracking.diff
	- Make an status bitmap instead of using independent boolean
	  variables.  It will make things nicer later when new issues
	  need to be tracked.

 - 04_request_firmware-propper-release-1.diff
	- release 'struct firmware_priv' from class_dev->release.

 - 05_request_firmware-propper-release-2.diff
	- Remove races related to the handling and release of 'struct
	  firmware'

 - 06_request_firmware-rearrange.diff
	- Refactor fw_setup_class_device for readability and
	  maintainability.

 - 07_request_firmware-dont-remove-attr.diff
	- Don't remove attributes, they should be gone automatically.
 

 Have a nice day

 	Manuel

Re: [PATCH] request_firmware(): fixes and polishing.

[Manuel Estrada Sainz]

Based on patch and suggestions from Dmitry Torokhov

Changelog:
	- use vfree to free vmalloc memory.
	- Make sure fw_setup_class_device sets *class_dev_p to NULL in all case
	  of error.
	- Fix error handling in firmware_class_init.

Index: linux-2.5/drivers/base/firmware_class.c
===================================================================
--- linux-2.5.orig/drivers/base/firmware_class.c	2004-01-02 20:42:03.000000000 +0100
+++ linux-2.5/drivers/base/firmware_class.c	2004-01-02 21:43:17.000000000 +0100
@@ -119,7 +119,7 @@
 		complete(&fw_priv->completion);
 		break;
 	case 1:
-		kfree(fw_priv->fw->data);
+		vfree(fw_priv->fw->data);
 		fw_priv->fw->data = NULL;
 		fw_priv->fw->size = 0;
 		fw_priv->alloc_size = 0;
@@ -297,6 +297,7 @@
 	}
 	memset(fw_priv->fw, 0, sizeof (*fw_priv->fw));
 
+	*class_dev_p = class_dev;
 	goto out;
 
 error_remove_loading:
@@ -310,7 +311,6 @@
 	kfree(class_dev);
 	*class_dev_p = NULL;
 out:
-	*class_dev_p = class_dev;
 	return retval;
 }
 static void
@@ -489,6 +489,7 @@
 	error = class_register(&firmware_class);
 	if (error) {
 		printk(KERN_ERR "%s: class_register failed\n", __FUNCTION__);
+		return error;
 	}
 	error = class_create_file(&firmware_class, &class_attr_timeout);
 	if (error) {

Re: [PATCH] request_firmware(): fixes and polishing.

[Manuel Estrada Sainz]

Based on patch and suggestions from Dmitry Torokhov

Changelog:
	- Take advantage of strlcpy.
	- Extra error logging.
	- Use struct coping instead of memcpy.
	- Put all aborting code in a single place, and fully abort if
	  fw_realloc_buffer fails.
	- Abort on unexpected 'loading' values.

Index: linux-2.5/drivers/base/firmware_class.c
===================================================================
--- linux-2.5.orig/drivers/base/firmware_class.c	2004-01-06 03:23:14.000000000 +0100
+++ linux-2.5/drivers/base/firmware_class.c	2004-01-06 03:53:30.000000000 +0100
@@ -34,6 +34,14 @@
 	struct timer_list timeout;
 };
 
+static inline void
+fw_load_abort(struct firmware_priv *fw_priv)
+{
+	fw_priv->abort = 1;
+	wmb();
+	complete(&fw_priv->completion);
+}
+
 static ssize_t
 firmware_timeout_show(struct class *class, char *buf)
 {
@@ -113,11 +121,6 @@
 	fw_priv->loading = simple_strtol(buf, NULL, 10);
 
 	switch (fw_priv->loading) {
-	case -1:
-		fw_priv->abort = 1;
-		wmb();
-		complete(&fw_priv->completion);
-		break;
 	case 1:
 		vfree(fw_priv->fw->data);
 		fw_priv->fw->data = NULL;
@@ -125,8 +128,17 @@
 		fw_priv->alloc_size = 0;
 		break;
 	case 0:
-		if (prev_loading == 1)
+		if (prev_loading == 1) {
 			complete(&fw_priv->completion);
+			break;
+		}
+		/* fallthrough */
+	default:
+		printk(KERN_ERR "%s: unexpected value (%d)\n", __FUNCTION__,
+		       fw_priv->loading);
+		/* fallthrough */
+	case -1:
+		fw_load_abort(fw_priv);
 		break;
 	}
 
@@ -164,7 +176,7 @@
 	if (!new_data) {
 		printk(KERN_ERR "%s: unable to alloc buffer\n", __FUNCTION__);
 		/* Make sure that we don't keep incomplete data */
-		fw_priv->abort = 1;
+		fw_load_abort(fw_priv);
 		return -ENOMEM;
 	}
 	fw_priv->alloc_size += PAGE_SIZE;
@@ -221,17 +233,14 @@
 firmware_class_timeout(u_long data)
 {
 	struct firmware_priv *fw_priv = (struct firmware_priv *) data;
-	fw_priv->abort = 1;
-	wmb();
-	complete(&fw_priv->completion);
+	fw_load_abort(fw_priv);
 }
 
 static inline void
 fw_setup_class_device_id(struct class_device *class_dev, struct device *dev)
 {
 	/* XXX warning we should watch out for name collisions */
-	strncpy(class_dev->class_id, dev->bus_id, BUS_ID_SIZE);
-	class_dev->class_id[BUS_ID_SIZE - 1] = '\0';
+	strlcpy(class_dev->class_id, dev->bus_id, BUS_ID_SIZE);
 }
 static int
 fw_setup_class_device(struct class_device **class_dev_p,
@@ -244,6 +253,7 @@
 						 GFP_KERNEL);
 
 	if (!fw_priv || !class_dev) {
+		printk(KERN_ERR "%s: kmalloc failed\n", __FUNCTION__);
 		retval = -ENOMEM;
 		goto error_kfree;
 	}
@@ -251,12 +261,8 @@
 	memset(class_dev, 0, sizeof (*class_dev));
 
 	init_completion(&fw_priv->completion);
-	memcpy(&fw_priv->attr_data, &firmware_attr_data_tmpl,
-	       sizeof (firmware_attr_data_tmpl));
-
-	strncpy(&fw_priv->fw_id[0], fw_name, FIRMWARE_NAME_MAX);
-	fw_priv->fw_id[FIRMWARE_NAME_MAX - 1] = '\0';
-
+	fw_priv->attr_data = firmware_attr_data_tmpl;
+	strlcpy(fw_priv->fw_id, fw_name, FIRMWARE_NAME_MAX);
 	fw_setup_class_device_id(class_dev, device);
 	class_dev->dev = device;
 

Re: [PATCH] request_firmware(): fixes and polishing.

[Manuel Estrada Sainz]

Based on patch and suggestions from Dmitry Torokhov

Changelog:
	- Make an status bitmap instead of using independent boolean variables.
	  It will make things nicer later when new issues need to be tracked.
	
Index: linux-2.5/drivers/base/firmware_class.c
===================================================================
--- linux-2.5.orig/drivers/base/firmware_class.c	2004-01-06 14:29:01.000000000 +0100
+++ linux-2.5/drivers/base/firmware_class.c	2004-01-06 21:29:09.000000000 +0100
@@ -13,6 +13,7 @@
 #include <linux/timer.h>
 #include <linux/vmalloc.h>
 #include <asm/hardirq.h>
+#include <linux/bitops.h>
 
 #include <linux/firmware.h>
 #include "base.h"
@@ -21,6 +22,11 @@
 MODULE_DESCRIPTION("Multi purpose firmware loading support");
 MODULE_LICENSE("GPL");
 
+enum {
+	FW_STATUS_LOADING,
+	FW_STATUS_ABORT,
+};
+
 static int loading_timeout = 10;	/* In seconds */
 
 struct firmware_priv {
@@ -28,8 +34,7 @@
 	struct completion completion;
 	struct bin_attribute attr_data;
 	struct firmware *fw;
-	int loading;
-	int abort;
+	unsigned long status;
 	int alloc_size;
 	struct timer_list timeout;
 };
@@ -37,7 +42,7 @@
 static inline void
 fw_load_abort(struct firmware_priv *fw_priv)
 {
-	fw_priv->abort = 1;
+	set_bit(FW_STATUS_ABORT, &fw_priv->status);
 	wmb();
 	complete(&fw_priv->completion);
 }
@@ -99,7 +104,8 @@
 firmware_loading_show(struct class_device *class_dev, char *buf)
 {
 	struct firmware_priv *fw_priv = class_get_devdata(class_dev);
-	return sprintf(buf, "%d\n", fw_priv->loading);
+	int loading = test_bit(FW_STATUS_LOADING, &fw_priv->status);
+	return sprintf(buf, "%d\n", loading);
 }
 
 /**
@@ -116,26 +122,26 @@
 		       const char *buf, size_t count)
 {
 	struct firmware_priv *fw_priv = class_get_devdata(class_dev);
-	int prev_loading = fw_priv->loading;
-
-	fw_priv->loading = simple_strtol(buf, NULL, 10);
+	int loading = simple_strtol(buf, NULL, 10);
 
-	switch (fw_priv->loading) {
+	switch (loading) {
 	case 1:
 		vfree(fw_priv->fw->data);
 		fw_priv->fw->data = NULL;
 		fw_priv->fw->size = 0;
 		fw_priv->alloc_size = 0;
+		set_bit(FW_STATUS_LOADING, &fw_priv->status);
 		break;
 	case 0:
-		if (prev_loading == 1) {
+		if (test_bit(FW_STATUS_LOADING, &fw_priv->status)) {
 			complete(&fw_priv->completion);
+			clear_bit(FW_STATUS_LOADING, &fw_priv->status);
 			break;
 		}
 		/* fallthrough */
 	default:
 		printk(KERN_ERR "%s: unexpected value (%d)\n", __FUNCTION__,
-		       fw_priv->loading);
+		       loading);
 		/* fallthrough */
 	case -1:
 		fw_load_abort(fw_priv);
@@ -370,7 +376,7 @@
 	del_timer_sync(&fw_priv->timeout);
 	fw_remove_class_device(class_dev);
 
-	if (fw_priv->fw->size && !fw_priv->abort) {
+	if (fw_priv->fw->size && !test_bit(FW_STATUS_ABORT, &fw_priv->status)) {
 		*firmware = fw_priv->fw;
 	} else {
 		retval = -ENOENT;

Re: [PATCH] request_firmware(): fixes and polishing.

[Manuel Estrada Sainz]

Based on patch and suggestions from Dmitry Torokhov

Changelog:
	- release 'struct firmware_priv' from class_dev->release.

Index: linux-2.5/drivers/base/firmware_class.c
===================================================================
--- linux-2.5.orig/drivers/base/firmware_class.c	2004-01-06 13:30:48.000000000 +0100
+++ linux-2.5/drivers/base/firmware_class.c	2004-01-06 13:31:38.000000000 +0100
@@ -232,6 +232,9 @@
 static void
 fw_class_dev_release(struct class_device *class_dev)
 {
+	struct firmware_priv *fw_priv = class_get_devdata(class_dev);
+
+	kfree(fw_priv);
 	kfree(class_dev);
 }
 
@@ -258,6 +261,8 @@
 	struct class_device *class_dev = kmalloc(sizeof (struct class_device),
 						 GFP_KERNEL);
 
+	*class_dev_p = NULL;
+
 	if (!fw_priv || !class_dev) {
 		printk(KERN_ERR "%s: kmalloc failed\n", __FUNCTION__);
 		retval = -ENOMEM;
@@ -318,10 +323,11 @@
 	sysfs_remove_bin_file(&class_dev->kobj, &fw_priv->attr_data);
 error_unreg_class_dev:
 	class_device_unregister(class_dev);
+	goto out;
+
 error_kfree:
 	kfree(fw_priv);
 	kfree(class_dev);
-	*class_dev_p = NULL;
 out:
 	return retval;
 }
@@ -374,7 +380,6 @@
 	wait_for_completion(&fw_priv->completion);
 
 	del_timer_sync(&fw_priv->timeout);
-	fw_remove_class_device(class_dev);
 
 	if (fw_priv->fw->size && !test_bit(FW_STATUS_ABORT, &fw_priv->status)) {
 		*firmware = fw_priv->fw;
@@ -383,7 +388,7 @@
 		vfree(fw_priv->fw->data);
 		kfree(fw_priv->fw);
 	}
-	kfree(fw_priv);
+	fw_remove_class_device(class_dev);
 out:
 	return retval;
 }

Re: [PATCH] request_firmware(): fixes and polishing.

[Manuel Estrada Sainz]

Changelog:
	- Remove races related to the handling and release of 'struct firmware'

Index: linux-2.5/drivers/base/firmware_class.c
===================================================================
--- linux-2.5.orig/drivers/base/firmware_class.c	2004-01-06 13:41:22.000000000 +0100
+++ linux-2.5/drivers/base/firmware_class.c	2004-01-06 13:43:58.000000000 +0100
@@ -14,6 +14,7 @@
 #include <linux/vmalloc.h>
 #include <asm/hardirq.h>
 #include <linux/bitops.h>
+#include <asm/semaphore.h>
 
 #include <linux/firmware.h>
 #include "base.h"
@@ -24,11 +25,16 @@
 
 enum {
 	FW_STATUS_LOADING,
+	FW_STATUS_DONE,
 	FW_STATUS_ABORT,
 };
 
 static int loading_timeout = 10;	/* In seconds */
 
+/* fw_lock could be moved to 'struct firmware_priv' but since it is just
+ * guarding for corner cases a global lock should be OK */
+static DECLARE_MUTEX(fw_lock);
+
 struct firmware_priv {
 	char fw_id[FIRMWARE_NAME_MAX];
 	struct completion completion;
@@ -126,11 +132,13 @@
 
 	switch (loading) {
 	case 1:
+		down(&fw_lock);
 		vfree(fw_priv->fw->data);
 		fw_priv->fw->data = NULL;
 		fw_priv->fw->size = 0;
 		fw_priv->alloc_size = 0;
 		set_bit(FW_STATUS_LOADING, &fw_priv->status);
+		up(&fw_lock);
 		break;
 	case 0:
 		if (test_bit(FW_STATUS_LOADING, &fw_priv->status)) {
@@ -160,15 +168,26 @@
 {
 	struct class_device *class_dev = to_class_dev(kobj);
 	struct firmware_priv *fw_priv = class_get_devdata(class_dev);
-	struct firmware *fw = fw_priv->fw;
+	struct firmware *fw;
+	ssize_t ret_count = count;
 
-	if (offset > fw->size)
-		return 0;
-	if (offset + count > fw->size)
-		count = fw->size - offset;
+	down(&fw_lock);
+	fw = fw_priv->fw;
+	if (test_bit(FW_STATUS_DONE, &fw_priv->status)) {
+		ret_count = -ENODEV;
+		goto out;
+	}
+	if (offset > fw->size) {
+		ret_count = 0;
+		goto out;
+	}
+	if (offset + ret_count > fw->size)
+		ret_count = fw->size - offset;
 
-	memcpy(buffer, fw->data + offset, count);
-	return count;
+	memcpy(buffer, fw->data + offset, ret_count);
+out:
+	up(&fw_lock);
+	return ret_count;
 }
 static int
 fw_realloc_buffer(struct firmware_priv *fw_priv, int min_size)
@@ -209,18 +228,26 @@
 {
 	struct class_device *class_dev = to_class_dev(kobj);
 	struct firmware_priv *fw_priv = class_get_devdata(class_dev);
-	struct firmware *fw = fw_priv->fw;
-	int retval;
+	struct firmware *fw;
+	ssize_t retval;
 
+	down(&fw_lock);
+	fw = fw_priv->fw;
+	if (test_bit(FW_STATUS_DONE, &fw_priv->status)) {
+		retval = -ENODEV;
+		goto out;
+	}
 	retval = fw_realloc_buffer(fw_priv, offset + count);
 	if (retval)
-		return retval;
+		goto out;
 
 	memcpy(fw->data + offset, buffer, count);
 
 	fw->size = max_t(size_t, offset + count, fw->size);
-
-	return count;
+	retval = count;
+out:
+	up(&fw_lock);
+	return retval;
 }
 static struct bin_attribute firmware_attr_data_tmpl = {
 	.attr = {.name = "data", .mode = 0644},
@@ -252,7 +279,7 @@
 	strlcpy(class_dev->class_id, dev->bus_id, BUS_ID_SIZE);
 }
 static int
-fw_setup_class_device(struct class_device **class_dev_p,
+fw_setup_class_device(struct firmware *fw, struct class_device **class_dev_p,
 		      const char *fw_name, struct device *device)
 {
 	int retval = 0;
@@ -290,6 +317,8 @@
 		goto error_kfree;
 	}
 
+	fw_priv->fw = fw;
+
 	retval = sysfs_create_bin_file(&class_dev->kobj, &fw_priv->attr_data);
 	if (retval) {
 		printk(KERN_ERR "%s: sysfs_create_bin_file failed\n",
@@ -305,20 +334,9 @@
 		goto error_remove_data;
 	}
 
-	fw_priv->fw = kmalloc(sizeof (struct firmware), GFP_KERNEL);
-	if (!fw_priv->fw) {
-		printk(KERN_ERR "%s: kmalloc(struct firmware) failed\n",
-		       __FUNCTION__);
-		retval = -ENOMEM;
-		goto error_remove_loading;
-	}
-	memset(fw_priv->fw, 0, sizeof (*fw_priv->fw));
-
 	*class_dev_p = class_dev;
 	goto out;
 
-error_remove_loading:
-	class_device_remove_file(class_dev, &class_device_attr_loading);
 error_remove_data:
 	sysfs_remove_bin_file(&class_dev->kobj, &fw_priv->attr_data);
 error_unreg_class_dev:
@@ -354,21 +372,29 @@
  *	firmware image for this or any other device.
  **/
 int
-request_firmware(const struct firmware **firmware, const char *name,
+request_firmware(const struct firmware **firmware_p, const char *name,
 		 struct device *device)
 {
 	struct class_device *class_dev;
 	struct firmware_priv *fw_priv;
+	struct firmware *firmware;
 	int retval;
 
-	if (!firmware)
+	if (!firmware_p)
 		return -EINVAL;
 
-	*firmware = NULL;
+	*firmware_p = firmware = kmalloc(sizeof (struct firmware), GFP_KERNEL);
+	if (!firmware) {
+		printk(KERN_ERR "%s: kmalloc(struct firmware) failed\n",
+		       __FUNCTION__);
+		retval = -ENOMEM;
+		goto out;
+	}
+	memset(firmware, 0, sizeof (*firmware));
 
-	retval = fw_setup_class_device(&class_dev, name, device);
+	retval = fw_setup_class_device(firmware, &class_dev, name, device);
 	if (retval)
-		goto out;
+		goto error_kfree_fw;
 
 	fw_priv = class_get_devdata(class_dev);
 
@@ -378,17 +404,23 @@
 	}
 
 	wait_for_completion(&fw_priv->completion);
+	set_bit(FW_STATUS_DONE, &fw_priv->status);
 
 	del_timer_sync(&fw_priv->timeout);
 
-	if (fw_priv->fw->size && !test_bit(FW_STATUS_ABORT, &fw_priv->status)) {
-		*firmware = fw_priv->fw;
-	} else {
+	down(&fw_lock);
+	if (!fw_priv->fw->size || test_bit(FW_STATUS_ABORT, &fw_priv->status)) {
 		retval = -ENOENT;
-		vfree(fw_priv->fw->data);
-		kfree(fw_priv->fw);
+		release_firmware(fw_priv->fw);
+		*firmware_p = NULL;
 	}
+	fw_priv->fw = NULL;
+	up(&fw_lock);
 	fw_remove_class_device(class_dev);
+	goto out;
+
+error_kfree_fw:
+	kfree(firmware);
 out:
 	return retval;
 }

Re: [PATCH] request_firmware(): fixes and polishing.

[Manuel Estrada Sainz]

Based on patch and suggestions from Dmitry Torokhov

Changelog:
	- Refactor fw_setup_class_device for readability and maintainability.

Index: linux-2.5/drivers/base/firmware_class.c
===================================================================
--- linux-2.5.orig/drivers/base/firmware_class.c	2004-01-06 13:40:49.000000000 +0100
+++ linux-2.5/drivers/base/firmware_class.c	2004-01-06 13:40:58.000000000 +0100
@@ -278,11 +278,12 @@
 	/* XXX warning we should watch out for name collisions */
 	strlcpy(class_dev->class_id, dev->bus_id, BUS_ID_SIZE);
 }
+
 static int
-fw_setup_class_device(struct firmware *fw, struct class_device **class_dev_p,
-		      const char *fw_name, struct device *device)
+fw_register_class_device(struct class_device **class_dev_p,
+			 const char *fw_name, struct device *device)
 {
-	int retval = 0;
+	int retval;
 	struct firmware_priv *fw_priv = kmalloc(sizeof (struct firmware_priv),
 						GFP_KERNEL);
 	struct class_device *class_dev = kmalloc(sizeof (struct class_device),
@@ -301,13 +302,13 @@
 	init_completion(&fw_priv->completion);
 	fw_priv->attr_data = firmware_attr_data_tmpl;
 	strlcpy(fw_priv->fw_id, fw_name, FIRMWARE_NAME_MAX);
-	fw_setup_class_device_id(class_dev, device);
-	class_dev->dev = device;
 
 	fw_priv->timeout.function = firmware_class_timeout;
 	fw_priv->timeout.data = (u_long) fw_priv;
 	init_timer(&fw_priv->timeout);
 
+	fw_setup_class_device_id(class_dev, device);
+	class_dev->dev = device;
 	class_dev->class = &firmware_class;
 	class_set_devdata(class_dev, fw_priv);
 	retval = class_device_register(class_dev);
@@ -316,7 +317,28 @@
 		       __FUNCTION__);
 		goto error_kfree;
 	}
+	*class_dev_p = class_dev;
+	return 0;
 
+error_kfree:
+	kfree(fw_priv);
+	kfree(class_dev);
+	return retval;
+}
+static int
+fw_setup_class_device(struct firmware *fw, struct class_device **class_dev_p,
+		      const char *fw_name, struct device *device)
+{
+	struct class_device *class_dev;
+	struct firmware_priv *fw_priv;
+	int retval;
+
+	*class_dev_p = NULL;
+	retval = fw_register_class_device(&class_dev, fw_name, device);
+	if (retval)
+		goto out;
+
+	fw_priv = class_get_devdata(class_dev);
 	fw_priv->fw = fw;
 
 	retval = sysfs_create_bin_file(&class_dev->kobj, &fw_priv->attr_data);
@@ -341,11 +363,6 @@
 	sysfs_remove_bin_file(&class_dev->kobj, &fw_priv->attr_data);
 error_unreg_class_dev:
 	class_device_unregister(class_dev);
-	goto out;
-
-error_kfree:
-	kfree(fw_priv);
-	kfree(class_dev);
 out:
 	return retval;
 }

Re: [PATCH] request_firmware(): fixes and polishing.

[Manuel Estrada Sainz]

Based on patch and suggestions from Dmitry Torokhov

Changelog:
	- Don't remove attributes, they should be gone automatically.

Index: linux-2.5/drivers/base/firmware_class.c
===================================================================
--- linux-2.5.orig/drivers/base/firmware_class.c	2004-01-06 13:40:58.000000000 +0100
+++ linux-2.5/drivers/base/firmware_class.c	2004-01-06 13:41:07.000000000 +0100
@@ -339,13 +339,13 @@
 		goto out;
 
 	fw_priv = class_get_devdata(class_dev);
-	fw_priv->fw = fw;
 
+	fw_priv->fw = fw;
 	retval = sysfs_create_bin_file(&class_dev->kobj, &fw_priv->attr_data);
 	if (retval) {
 		printk(KERN_ERR "%s: sysfs_create_bin_file failed\n",
 		       __FUNCTION__);
-		goto error_unreg_class_dev;
+		goto error_unreg;
 	}
 
 	retval = class_device_create_file(class_dev,
@@ -353,28 +353,17 @@
 	if (retval) {
 		printk(KERN_ERR "%s: class_device_create_file failed\n",
 		       __FUNCTION__);
-		goto error_remove_data;
+		goto error_unreg;
 	}
 
 	*class_dev_p = class_dev;
 	goto out;
 
-error_remove_data:
-	sysfs_remove_bin_file(&class_dev->kobj, &fw_priv->attr_data);
-error_unreg_class_dev:
+error_unreg:
 	class_device_unregister(class_dev);
 out:
 	return retval;
 }
-static void
-fw_remove_class_device(struct class_device *class_dev)
-{
-	struct firmware_priv *fw_priv = class_get_devdata(class_dev);
-
-	class_device_remove_file(class_dev, &class_device_attr_loading);
-	sysfs_remove_bin_file(&class_dev->kobj, &fw_priv->attr_data);
-	class_device_unregister(class_dev);
-}
 
 /** 
  * request_firmware: - request firmware to hotplug and wait for it
@@ -433,7 +422,7 @@
 	}
 	fw_priv->fw = NULL;
 	up(&fw_lock);
-	fw_remove_class_device(class_dev);
+	class_device_unregister(class_dev);
 	goto out;
 
 error_kfree_fw:
@@ -569,7 +558,6 @@
 static void __exit
 firmware_class_exit(void)
 {
-	class_remove_file(&firmware_class, &class_attr_timeout);
 	class_unregister(&firmware_class);
 }
 

Re: [PATCH] request_firmware(): fixes and polishing.

[Jean Tourrilhes]

On Wed, Feb 25, 2004 at 02:34:48AM +0100, Manuel Estrada Sainz wrote:
> 
>  Hi,
> 
>  Please apply.
> 
>  Dmitry Torokhov has been criticizing my code for some days (Thanks Dmitry),
>  and here is the result. It should be ready for -mm tree.
>  
>  Simon Kelly tested the patch series and reported improvement with some
>  problems he was having.

	By the way, this has fixed the problem I was seeing. My only
nit is that it seems you e-mailer is translating some of the chars in
the patch (' ' -> '=20' ; '=' -> '=3D').

	Jean

Re: [PATCH] request_firmware(): fixes and polishing.

[Manuel Estrada Sainz]

On Wed, Feb 25, 2004 at 11:47:44AM -0800, Jean Tourrilhes wrote:
> On Wed, Feb 25, 2004 at 02:34:48AM +0100, Manuel Estrada Sainz wrote:
> > 
> >  Hi,
> > 
> >  Please apply.
> > 
> >  Dmitry Torokhov has been criticizing my code for some days (Thanks Dmitry),
> >  and here is the result. It should be ready for -mm tree.
> >  
> >  Simon Kelly tested the patch series and reported improvement with some
> >  problems he was having.
> 
> 	By the way, this has fixed the problem I was seeing.

 Great.

> My only nit is that it seems you e-mailer is translating some of the
> chars in the patch (' ' -> '=20' ; '=' -> '=3D').

 Strange, I checked both the original patches and the emails as I got
 them I got from lkml and couldn't find any '=20' or '=3D' strings.
 
 I used Greg's patch sending perl script, does that make mime tricks on
 patches?.

 Have a nice day

 	Manuel
-- 
--- Manuel Estrada Sainz <ranty@debian.org>
                         <ranty@bigfoot.com>
			 <ranty@users.sourceforge.net>
------------------------ <manuel.estrada@hispalinux.es> -------------------
Let us have the serenity to accept the things we cannot change, courage to
change the things we can, and wisdom to know the difference.

Re: [PATCH] request_firmware(): fixes and polishing.

[Dmitry Torokhov]

On Tuesday 24 February 2004 08:34 pm, Manuel Estrada Sainz wrote:
> 
>  Hi,
> 
>  Please apply.
> 
>  Dmitry Torokhov has been criticizing my code for some days (Thanks Dmitry),
>  and here is the result. It should be ready for -mm tree.
>  
>  Simon Kelly tested the patch series and reported improvement with some
>  problems he was having.
> 

I have couple more fixes to the firmware loader class:

firmware-pin_module.patch:
  - we need to pin the firmware module if we successfully registered firmware
    class device and "put" it in device re4lease function otherwise the
    module could be unloaded too early. Consider:
    - some module requests firmware
    - firmware loader registers class device and calls hotplug
    - userspace hangs keeping 
    - firmware loader times out
    - the calls module is unloaded. Now firmware loader has 0 refcount and
      cazn be unloaded as well leaving device class behind.
    Am I seeing things?

firmware-hotplug.patch
  - I stll think that we sould not call userspace until we registered all
    necessary attributes. Now that Greg put my changes to kobject in mainline
    ist very easy -  just inhibit hotplug handler until we are ready and call
    kobject_hitplug by ourselves.

Please comment.

-- 
Dmitry

[PATCH 1/2] Pin firmware module (was Re: [PATCH] request_firmware(): fixes and polishing.)

[Dmitry Torokhov]

===================================================================


ChangeSet@1.1694, 2004-02-29 00:06:09-05:00, dtor_core@ameritech.net
  Firmware loader:
    We need to pin the firmware loader module until the last reference
    to the firmware class device is dropped and the class device is
    destroyed.


 firmware_class.c |    6 ++++++
 1 files changed, 6 insertions(+)


===================================================================



diff -Nru a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
--- a/drivers/base/firmware_class.c	Sun Feb 29 01:20:57 2004
+++ b/drivers/base/firmware_class.c	Sun Feb 29 01:20:57 2004
@@ -263,6 +263,8 @@
 
 	kfree(fw_priv);
 	kfree(class_dev);
+
+	module_put(THIS_MODULE);
 }
 
 static void
@@ -325,6 +327,7 @@
 	kfree(class_dev);
 	return retval;
 }
+
 static int
 fw_setup_class_device(struct firmware *fw, struct class_device **class_dev_p,
 		      const char *fw_name, struct device *device)
@@ -337,6 +340,9 @@
 	retval = fw_register_class_device(&class_dev, fw_name, device);
 	if (retval)
 		goto out;
+
+	/* Need to pin this module until class device is destroyed */
+	__module_get(THIS_MODULE);
 
 	fw_priv = class_get_devdata(class_dev);
 

[PATCH 2/2] Delay firmware hotplug event (was Re: [PATCH] request_firmware(): fixes and polishing.)

[Dmitry Torokhov]

===================================================================


ChangeSet@1.1695, 2004-02-29 00:27:53-05:00, dtor_core@ameritech.net
  Firmware loader:
    Do not call hotplug until firmware class device is completely
    instantiated.


 firmware_class.c |    6 ++++++
 1 files changed, 6 insertions(+)


===================================================================



diff -Nru a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
--- a/drivers/base/firmware_class.c	Sun Feb 29 01:21:18 2004
+++ b/drivers/base/firmware_class.c	Sun Feb 29 01:21:18 2004
@@ -27,6 +27,7 @@
 	FW_STATUS_LOADING,
 	FW_STATUS_DONE,
 	FW_STATUS_ABORT,
+	FW_STATUS_READY,
 };
 
 static int loading_timeout = 10;	/* In seconds */
@@ -96,6 +97,9 @@
 	int i = 0;
 	char *scratch = buffer;
 
+        if (!test_bit(FW_STATUS_READY, &fw_priv->status))
+                return -ENODEV;
+
 	if (buffer_size < (FIRMWARE_NAME_MAX + 10))
 		return -ENOMEM;
 	if (num_envp < 1)
@@ -362,6 +366,7 @@
 		goto error_unreg;
 	}
 
+	set_bit(FW_STATUS_READY, &fw_priv->status);
 	*class_dev_p = class_dev;
 	goto out;
 
@@ -415,6 +420,7 @@
 		add_timer(&fw_priv->timeout);
 	}
 
+	kobject_hotplug("add", &class_dev->kobj);
 	wait_for_completion(&fw_priv->completion);
 	set_bit(FW_STATUS_DONE, &fw_priv->status);