Skip to contentSkip to conversation

request_firmware(): fixes and polishing.

PATCH In review Manuel Estrada Sainz 4 participants 13 messages Feb 25, 2004 – Feb 29, 2004 1 message not in the archive Driver core

lore
Share & tools
lore

Manuel Estrada Sainz

author
#1 lore raw

PATCH 1/1 request_firmware(): fixes and polishing.

In reply to a message not in the archive

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.

Quoted 3 lines · Simon Kelley

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

author
#2 lore raw

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.

drivers/base/firmware_class.c +3 -2
Index: linux-2.5/drivers/base/firmware_class.c
===================================================================
@@ -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) {
author
#8 lore raw
In reply to Manuel Estrada Sainz

Based on patch and suggestions from Dmitry Torokhov

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

drivers/base/firmware_class.c +5 -17
Index: linux-2.5/drivers/base/firmware_class.c
===================================================================
@@ -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);
 }
author
#7 lore raw
In reply to Manuel Estrada Sainz

Based on patch and suggestions from Dmitry Torokhov

Changelog:
- Refactor fw_setup_class_device for readability and maintainability.

drivers/base/firmware_class.c +27 -10
Index: linux-2.5/drivers/base/firmware_class.c
===================================================================
@@ -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;
 }
author
#6 lore raw
In reply to Manuel Estrada Sainz

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

drivers/base/firmware_class.c +66 -34
Index: linux-2.5/drivers/base/firmware_class.c
===================================================================
@@ -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;
 }
author
#3 lore raw
In reply to 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.

drivers/base/firmware_class.c +24 -18
Index: linux-2.5/drivers/base/firmware_class.c
===================================================================
@@ -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;
author
#4 lore raw

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.

drivers/base/firmware_class.c +17 -11
Index: linux-2.5/drivers/base/firmware_class.c
===================================================================
@@ -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;
author
#5 lore raw

Based on patch and suggestions from Dmitry Torokhov

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

drivers/base/firmware_class.c +8 -3
Index: linux-2.5/drivers/base/firmware_class.c
===================================================================
@@ -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;
 }
#9 lore raw
In reply to Manuel Estrada Sainz
Quoted 10 lines · 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.

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

#10 lore raw
Quoted 6 lines · Jean Tourrilhes
Quoted 5 lines · Manuel Estrada Sainz

Hi,

Please apply.

Quoted 7 lines
Quoted 5 lines

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.

Quoted 2 lines

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

Signature · 6 lines
--- Manuel Estrada Sainz <[email protected]>
                         <[email protected]>
			 <[email protected]>
------------------------ <[email protected]> -------------------
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.
#12 lore raw

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

In reply to Dmitry Torokhov

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

[email protected], 2004-02-29 00:06:09-05:00, [email protected]
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(+)

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

drivers/base/firmware_class.c +6 -0
diff -Nru a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
@@ -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);
#13 lore raw

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

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

[email protected], 2004-02-29 00:27:53-05:00, [email protected]
Firmware loader:
Do not call hotplug until firmware class device is completely instantiated.

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

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

drivers/base/firmware_class.c +6 -0
diff -Nru a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
@@ -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);
#11 lore raw
In reply to Manuel Estrada Sainz
Quoted 11 lines · 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.

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.

Signature · 1 line
Dmitry