Firmware loader - fix races and resource dealloocation problems

2.6 PATCH/RFC Dmitry Torokhov 4 participants 15 messages Dec 21, 2003 – Jan 1, 2004

Subsystems (2)

Library code Driver core

lore

Share & tools

lore

Dmitry Torokhov

author

2003-12-21 06:38 UTC #0 lore raw

Hi,

It seems that implementation of the firmware loader is racy as it relies on kobject hotplug handler. Unfortunately that handler runs too early, before firmware class attributes controlling the loading process, are created. This causes firmware loading fail at least half of the times on my laptop.

Another problem that I see is that the present implementation tries to free some of the allocated resources manually instead of relying on driver model. Particularly damaging is freeing fw_priv in request_firmware. Although the code calls fw_remove_class_device (which in turns calls class_device_unregister) the freeing of class device and all its attributes can be delayed as the attribute files may still be held open by the userspace handler or any other program. Subsequent access to these files could cause trouble.

Also, there is no protection from overwriting firmware image once it has been committed.

I tried to correct all 3 problems in the patch below. It creates a custom hotplug handler that is called from request_hardware. I tried to mimic the hotplug handler from kobject - it's nice to have DEVPATH pointing to the right place - so I exported kobject_get_path_length and kobject_fill_path (former get_kobj_path_length and fill_kobj_patch). I think these 2 should not be considered "implementation details" and exporting them is OK. Write access to firmware device class attributes is protected by a semaphore and the code refuses any updates once firmware loading has been committed or aborted. Firmware

Dmitry

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

[email protected], 2003-12-21 01:00:41-05:00, [email protected]
- Provide handcrafted hotplug method as generic one runs too early.
- Do not allow changes to firmware image once it has been committed.
- Correctly free resources.

 drivers/base/firmware_class.c |  530 +++++++++++++++++++++++++-----------------
 include/linux/kobject.h       |    2 
 lib/kobject.c                 |   29 +-
 3 files changed, 347 insertions(+), 214 deletions(-)

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

drivers/base/firmware_class.c +322 ~~-208~~

diff -Nru a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
@@ -7,12 +7,14 @@
  *
  */
 
+#include <linux/kobject.h>
 #include <linux/device.h>
 #include <linux/module.h>
 #include <linux/init.h>
 #include <linux/timer.h>
 #include <linux/vmalloc.h>
 #include <asm/hardirq.h>
+#include <asm/semaphore.h>
 
 #include <linux/firmware.h>
 #include "base.h"

@@ -21,124 +23,109 @@
 MODULE_DESCRIPTION("Multi purpose firmware loading support");
 MODULE_LICENSE("GPL");
 
+#define FW_STATE_NEW		0
+#define FW_STATE_LOADING	1
+#define FW_STATE_LOADED		2
+#define FW_STATE_ABORTED	3
+
 static int loading_timeout = 10;	/* In seconds */
 
 struct firmware_priv {
-	char fw_id[FIRMWARE_NAME_MAX];
+	struct semaphore semaphore;
 	struct completion completion;
 	struct bin_attribute attr_data;
 	struct firmware *fw;
+	int state;
 	int loading;
-	int abort;
 	int alloc_size;
 	struct timer_list timeout;
 };
 
-static ssize_t
-firmware_timeout_show(struct class *class, char *buf)
-{
-	return sprintf(buf, "%d\n", loading_timeout);
-}
-
 /**
- * firmware_timeout_store:
- * Description:
- *	Sets the number of seconds to wait for the firmware.  Once
- *	this expires an error will be return to the driver and no
- *	firmware will be provided.
+ * firmware loading attribute
  *
- *	Note: zero means 'wait for ever'
- *  
+ *	Controls process of loading firmware into kernel.
+ *	The relevant values are: 
+ *
+ *	 1: Start a load, discarding any previous partial load.
+ *	 0: Conclude the load and handle the data to the driver code.
+ *	-1: Conclude the load with an error and discard any written data.
+ *
+ *	Note: If no firmware has been loaded 0 is equivalent to -1
  **/
-static ssize_t
-firmware_timeout_store(struct class *class, const char *buf, size_t count)
+static void firmware_start_loading(struct firmware_priv *fw_priv)
 {
-	loading_timeout = simple_strtol(buf, NULL, 10);
-	return count;
+	fw_priv->state = FW_STATE_LOADING;
+	vfree(fw_priv->fw->data);
+	fw_priv->fw->data = NULL;
+	fw_priv->fw->size = 0;
+	fw_priv->alloc_size = 0;
 }
 
-static CLASS_ATTR(timeout, 0644, firmware_timeout_show, firmware_timeout_store);
-
-static void  fw_class_dev_release(struct class_device *class_dev);
-int firmware_class_hotplug(struct class_device *dev, char **envp,
-			   int num_envp, char *buffer, int buffer_size);
-
-static struct class firmware_class = {
-	.name		= "firmware",
-	.hotplug	= firmware_class_hotplug,
-	.release	= fw_class_dev_release,
-};
-
-int
-firmware_class_hotplug(struct class_device *class_dev, char **envp,
-		       int num_envp, char *buffer, int buffer_size)
+static void firmware_abort_loading(struct firmware_priv *fw_priv)
 {
-	struct firmware_priv *fw_priv = class_get_devdata(class_dev);
-	int i = 0;
-	char *scratch = buffer;
-
-	if (buffer_size < (FIRMWARE_NAME_MAX + 10))
-		return -ENOMEM;
-	if (num_envp < 1)
-		return -ENOMEM;
+	fw_priv->state = FW_STATE_ABORTED;
+	wmb();
+	complete(&fw_priv->completion);
+}
 
-	envp[i++] = scratch;
-	scratch += sprintf(scratch, "FIRMWARE=%s", fw_priv->fw_id) + 1;
-	return 0;
+static void firmware_finish_loading(struct firmware_priv *fw_priv)
+{
+	fw_priv->state = FW_STATE_LOADED;
+	wmb();
+	complete(&fw_priv->completion);
 }
 
-static ssize_t
-firmware_loading_show(struct class_device *class_dev, char *buf)
+static ssize_t 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);
 }
 
-/**
- * firmware_loading_store: - loading control file
- * Description:
- *	The relevant values are: 
- *
- *	 1: Start a load, discarding any previous partial load.
- *	 0: Conclude the load and handle the data to the driver code.
- *	-1: Conclude the load with an error and discard any written data.
- **/
-static ssize_t
-firmware_loading_store(struct class_device *class_dev,
-		       const char *buf, size_t count)
+static ssize_t firmware_loading_store(struct class_device *class_dev,
+				      const char *buf, size_t count)
 {
 	struct firmware_priv *fw_priv = class_get_devdata(class_dev);
-	int prev_loading = fw_priv->loading;
+	int retval = count;
 
-	fw_priv->loading = simple_strtol(buf, NULL, 10);
+	down(&fw_priv->semaphore);
 
-	switch (fw_priv->loading) {
-	case -1:
-		fw_priv->abort = 1;
-		wmb();
-		complete(&fw_priv->completion);
-		break;
-	case 1:
-		kfree(fw_priv->fw->data);
-		fw_priv->fw->data = NULL;
-		fw_priv->fw->size = 0;
-		fw_priv->alloc_size = 0;
-		break;
-	case 0:
-		if (prev_loading == 1)
-			complete(&fw_priv->completion);
-		break;
-	}
+	if (fw_priv->state <= FW_STATE_LOADING) {
+		fw_priv->loading = simple_strtol(buf, NULL, 10);
 
-	return count;
+		switch (fw_priv->loading) {
+		case -1:
+			firmware_abort_loading(fw_priv);
+			break;
+		case 1:
+			firmware_start_loading(fw_priv);
+			break;
+		case 0:
+			if (fw_priv->state == FW_STATE_LOADING && fw_priv->fw->size)
+				firmware_finish_loading(fw_priv); 
+			else
+				firmware_abort_loading(fw_priv);
+			break;
+		}
+	} else
+		retval = -EACCES;
+
+	up(&fw_priv->semaphore);
+
+	return retval;
 }
 
 static CLASS_DEVICE_ATTR(loading, 0644,
-			firmware_loading_show, firmware_loading_store);
+			 firmware_loading_show, firmware_loading_store);
 
-static ssize_t
-firmware_data_read(struct kobject *kobj,
-		   char *buffer, loff_t offset, size_t count)
+/**
+ * firmware data attribute
+ *
+ *	Data written to the 'data' attribute will be later handled to
+ *	the driver as a firmware image.
+ **/
+static ssize_t firmware_data_read(struct kobject *kobj,
+		   		  char *buffer, loff_t offset, size_t count)
 {
 	struct class_device *class_dev = to_class_dev(kobj);
 	struct firmware_priv *fw_priv = class_get_devdata(class_dev);

@@ -152,8 +139,8 @@
 	memcpy(buffer, fw->data + offset, count);
 	return count;
 }
-static int
-fw_realloc_buffer(struct firmware_priv *fw_priv, int min_size)
+
+static int fw_realloc_buffer(struct firmware_priv *fw_priv, int min_size)
 {
 	u8 *new_data;

@@ -164,7 +151,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_priv->state = FW_STATE_ABORTED;
 		return -ENOMEM;
 	}
 	fw_priv->alloc_size += PAGE_SIZE;

@@ -177,33 +164,33 @@
 	return 0;
 }
 
-/**
- * firmware_data_write:
- *
- * Description:
- *
- *	Data written to the 'data' attribute will be later handled to
- *	the driver as a firmware image.
- **/
-static ssize_t
-firmware_data_write(struct kobject *kobj,
-		    char *buffer, loff_t offset, size_t count)
+static ssize_t firmware_data_write(struct kobject *kobj,
+				   char *buffer, loff_t offset, size_t count)
 {
 	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;
 
-	retval = fw_realloc_buffer(fw_priv, offset + count);
-	if (retval)
-		return retval;
+	down(&fw_priv->semaphore);
 
-	memcpy(fw->data + offset, buffer, count);
+	if (fw_priv->state != FW_STATE_LOADING) {
+		retval = -EACCES;
+	} else {
+		retval = fw_realloc_buffer(fw_priv, offset + count);
 
-	fw->size = max_t(size_t, offset + count, fw->size);
+		if (!retval) {
+			memcpy(fw->data + offset, buffer, count);
+			fw->size = max_t(size_t, offset + count, fw->size);
+			retval = count;
+		}
+	}
 
-	return count;
+	up(&fw_priv->semaphore);
+
+	return retval;
 }
+
 static struct bin_attribute firmware_attr_data_tmpl = {
 	.attr = {.name = "data", .mode = 0644},
 	.size = 0,

@@ -211,73 +198,228 @@
 	.write = firmware_data_write,
 };
 
-static void
-fw_class_dev_release(struct class_device *class_dev)
+/**
+ * firmware timeout attribute
+ *
+ *	Applies to all instances of firmware loaders (it is a class
+ *	attribute.
+ *
+ *	Sets the number of seconds to wait for the firmware.  Once
+ *	this expires an error will be return to the driver and no
+ *	firmware will be provided.
+ *
+ *	Note: zero means 'wait for ever'
+ *  
+ **/
+static void firmware_class_timeout(u_long data)
 {
-	kfree(class_dev);
+	struct firmware_priv *fw_priv = (struct firmware_priv *) data;
+	
+	down(&fw_priv->semaphore);
+
+	firmware_abort_loading(fw_priv);
+
+	up(&fw_priv->semaphore);
 }
 
-static void
-firmware_class_timeout(u_long data)
+static ssize_t firmware_timeout_show(struct class *class, char *buf)
 {
-	struct firmware_priv *fw_priv = (struct firmware_priv *) data;
-	fw_priv->abort = 1;
-	wmb();
-	complete(&fw_priv->completion);
+	return sprintf(buf, "%d\n", loading_timeout);
 }
 
-static inline void
-fw_setup_class_device_id(struct class_device *class_dev, struct device *dev)
+static ssize_t firmware_timeout_store(struct class *class, 
+				      const char *buf, size_t count)
 {
-	/* 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';
+	loading_timeout = simple_strtol(buf, NULL, 10);
+	return count;
 }
-static int
-fw_setup_class_device(struct class_device **class_dev_p,
-		      const char *fw_name, struct device *device)
+
+static CLASS_ATTR(timeout, 0644, firmware_timeout_show, firmware_timeout_store);
+
+/**
+ * fw_class_hotplug
+ *
+ *	Dummy hotplug handler that inhibits driver model core generating
+ *	hotplug events when firmware class device is created or destroyed.
+ *	The code hotplug events can not be used because when firmware class
+ *	device is registered its attributes are not awailable yet and they
+ *	are needed by userspace to communicate with the kernel.
+ **/
+static int fw_class_hotplug(struct class_device *class_dev, char **envp,
+			    int num_envp, char *buffer, int buffer_size)
 {
-	int retval = 0;
-	struct firmware_priv *fw_priv = kmalloc(sizeof (struct firmware_priv),
-						GFP_KERNEL);
-	struct class_device *class_dev = kmalloc(sizeof (struct class_device),
-						 GFP_KERNEL);
+	return -ENODEV;
+}
+
+/**
+ * firmware_hotplug
+ *
+ *	Generates handcrafted hotplug event mimicing events from the
+ *	driver model core. It is called after the class device is
+ *	registered. 
+ **/
+static int firmware_hotplug(struct class_device *class_dev, const char *fw_name)
+{
+	char *argv[3], *envp[5];
+	char *buf, *scratch, *path;
+	int path_len;
+	int i = 0;
+	int retval;
+
+	if (!hotplug_path[0])
+		return -ENODEV;
+	
+	if (!(buf = scratch = kmalloc(1024, GFP_KERNEL))) {
+		printk(KERN_ERR "%s: not enough memory allocating environment\n",
+		       __FUNCTION__);
+		return -ENOMEM;
+	}
 
-	if (!fw_priv || !class_dev) {
+	argv[0] = hotplug_path;
+	argv[1] = "firmware";
+	argv[2] = 0;
+
+	envp[i++] = "HOME=/";
+	envp[i++] = "PATH=/sbin:/bin:/usr/sbin:/usr/bin";
+	envp[i++] = "ACTION=add";
+
+	path_len = kobject_get_path_length(&class_dev->kobj);
+        path = kmalloc(path_len, GFP_KERNEL);
+        if (!path) {
 		retval = -ENOMEM;
-		goto error_kfree;
+                goto out;
 	}
-	memset(fw_priv, 0, sizeof (*fw_priv));
-	memset(class_dev, 0, sizeof (*class_dev));
+        kobject_fill_path(&class_dev->kobj, path, path_len);
 
-	init_completion(&fw_priv->completion);
-	memcpy(&fw_priv->attr_data, &firmware_attr_data_tmpl,
-	       sizeof (firmware_attr_data_tmpl));
+        envp [i++] = scratch;
+        scratch += sprintf(scratch, "DEVPATH=%s", path) + 1;
 
-	strncpy(&fw_priv->fw_id[0], fw_name, FIRMWARE_NAME_MAX);
-	fw_priv->fw_id[FIRMWARE_NAME_MAX - 1] = '\0';
+	envp[i++] = scratch;
+	scratch += sprintf(scratch, "FIRMWARE=%s", fw_name) + 1;
 
-	fw_setup_class_device_id(class_dev, device);
-	class_dev->dev = device;
+	envp[i++] = 0;
+
+	retval = call_usermodehelper(argv[0], argv, envp, 0);
+
+out:
+	kfree(buf);
+
+	return retval;
+}
+
+static void fw_class_dev_release(struct class_device *class_dev)
+{
+	struct firmware_priv *fw_priv = class_get_devdata(class_dev);
+
+	if (fw_priv->state != FW_STATE_LOADED)
+		release_firmware(fw_priv->fw);
+
+	kfree(fw_priv);
+	kfree(class_dev);
+}
 
+static struct class firmware_class = {
+	.name		= "firmware",
+	.hotplug	= fw_class_hotplug,
+	.release	= fw_class_dev_release,
+};
+
+static void fw_setup_class_device_id(struct class_device *class_dev,
+				     struct device *dev)
+{
+	/* XXX warning we should watch out for name collisions */
+	strlcpy(class_dev->class_id, dev->bus_id, BUS_ID_SIZE);
+}
+
+static struct class_device *fw_alloc_class_device(struct device *device)
+{
+	struct firmware *firmware;
+	struct firmware_priv *fw_priv;
+	struct class_device *class_dev;
+	int retval;
+ 
+	firmware = kmalloc(sizeof (struct firmware), GFP_KERNEL);
+	if (!firmware) {
+		printk(KERN_ERR "%s: kmalloc(struct firmware) failed\n",
+		       __FUNCTION__);
+		retval = -ENOMEM;
+		goto err_out; 
+	}
+
+	memset(firmware, 0, sizeof (*firmware));
+       
+	fw_priv = kmalloc(sizeof(struct firmware_priv), GFP_KERNEL);
+	if (!fw_priv) {
+		printk(KERN_ERR "%s: kmalloc(struct firmware_priv) failed\n",
+		       __FUNCTION__);
+		goto err_out_free_firmware;
+        }
+
+	memset(fw_priv, 0, sizeof (*fw_priv));
+	init_MUTEX(&fw_priv->semaphore);
+	init_completion(&fw_priv->completion);
+	fw_priv->attr_data = firmware_attr_data_tmpl;
+	fw_priv->fw = firmware;
+	init_timer(&fw_priv->timeout);
 	fw_priv->timeout.function = firmware_class_timeout;
 	fw_priv->timeout.data = (u_long) fw_priv;
-	init_timer(&fw_priv->timeout);
 
+	class_dev = kmalloc(sizeof(struct class_device), GFP_KERNEL);
+	if (!class_dev) {
+		printk(KERN_ERR "%s: kmalloc(struct class_device) failed\n",
+		       __FUNCTION__);
+		goto err_out_free_fw_priv;
+	}
+
+	memset(class_dev, 0, sizeof(*class_dev));
+	fw_setup_class_device_id(class_dev, device);
+	class_dev->dev = device;
 	class_dev->class = &firmware_class;
 	class_set_devdata(class_dev, fw_priv);
+
+	return class_dev;
+
+err_out_free_fw_priv:
+	kfree(fw_priv);
+err_out_free_firmware:
+	kfree(firmware);
+err_out:
+	return NULL;
+}
+
+static int fw_setup_class_device(struct class_device **class_dev_p,
+		      		 struct device *device)
+{
+	struct class_device *class_dev = fw_alloc_class_device(device);
+	struct firmware_priv *fw_priv;
+	int retval;
+
+	if (!class_dev) {
+		retval = -ENOMEM;
+		goto err_out;
+	}
+
+	fw_priv = class_get_devdata(class_dev);
+
 	retval = class_device_register(class_dev);
 	if (retval) {
 		printk(KERN_ERR "%s: class_device_register failed\n",
 		       __FUNCTION__);
-		goto error_kfree;
+		fw_class_dev_release(class_dev);
+		retval = -ENOMEM;
+		goto err_out;
 	}
 
+	/*
+	 * We successfully registered class_dev, now we should not
+	 * manually free resources as they will be freed automatically.
+	 */
+
 	retval = sysfs_create_bin_file(&class_dev->kobj, &fw_priv->attr_data);
-	if (retval) {
+	if (retval) { 
 		printk(KERN_ERR "%s: sysfs_create_bin_file failed\n",
 		       __FUNCTION__);
-		goto error_unreg_class_dev;
+		goto err_out_unregister;
 	}
 
 	retval = class_device_create_file(class_dev,

@@ -285,43 +427,18 @@
 	if (retval) {
 		printk(KERN_ERR "%s: class_device_create_file failed\n",
 		       __FUNCTION__);
-		goto error_remove_data;
+		goto err_out_unregister;
 	}
 
-	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));
-
-	goto out;
+	*class_dev_p = class_dev;
+	return 0;
 
-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:
+err_out_unregister:
 	class_device_unregister(class_dev);
-error_kfree:
-	kfree(fw_priv);
-	kfree(class_dev);
+err_out:
 	*class_dev_p = NULL;
-out:
-	*class_dev_p = class_dev;
 	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

@@ -335,20 +452,21 @@
  *	should be distinctive enough not to be confused with any other
  *	firmware image for this or any other device.
  **/
-int
-request_firmware(const struct firmware **firmware, const char *name,
-		 struct device *device)
+int request_firmware(const struct firmware **firmware,
+		     const char *name, struct device *device)
 {
 	struct class_device *class_dev;
 	struct firmware_priv *fw_priv;
-	int retval;
+	int retval = 0;
 
-	if (!firmware)
-		return -EINVAL;
+	if (!firmware) {
+		retval = -EINVAL;
+		goto out; 
+	}
 
 	*firmware = NULL;
 
-	retval = fw_setup_class_device(&class_dev, name, device);
+	retval = fw_setup_class_device(&class_dev, device);
 	if (retval)
 		goto out;

@@ -359,19 +477,20 @@
 		add_timer(&fw_priv->timeout);
 	}
 
-	wait_for_completion(&fw_priv->completion);
+	retval = firmware_hotplug(class_dev, name);
+	if (retval)
+		goto out_device_unregister;
 
+	wait_for_completion(&fw_priv->completion);
 	del_timer(&fw_priv->timeout);
-	fw_remove_class_device(class_dev);
 
-	if (fw_priv->fw->size && !fw_priv->abort) {
+	if (fw_priv->state == FW_STATE_LOADED)
 		*firmware = fw_priv->fw;
-	} else {
+	else
 		retval = -ENOENT;
-		vfree(fw_priv->fw->data);
-		kfree(fw_priv->fw);
-	}
-	kfree(fw_priv);
+
+out_device_unregister:
+	class_device_unregister(class_dev);	
 out:
 	return retval;
 }

@@ -379,11 +498,11 @@
 /**
  * release_firmware: - release the resource associated with a firmware image
  **/
-void
-release_firmware(const struct firmware *fw)
+void release_firmware(const struct firmware *fw)
 {
 	if (fw) {
-		vfree(fw->data);
+		if (fw->data)
+			vfree(fw->data);
 		kfree(fw);
 	}
 }

@@ -397,8 +516,7 @@
  *	Note: This will not be possible until some kind of persistence
  *	is available.
  **/
-void
-register_firmware(const char *name, const u8 *data, size_t size)
+void register_firmware(const char *name, const u8 *data, size_t size)
 {
 	/* This is meaningless without firmware caching, so until we
 	 * decide if firmware caching is reasonable just leave it as a

@@ -415,8 +533,7 @@
 	void (*cont)(const struct firmware *fw, void *context);
 };
 
-static void
-request_firmware_work_func(void *arg)
+static void request_firmware_work_func(void *arg)
 {
 	struct firmware_work *fw_work = arg;
 	const struct firmware *fw;

@@ -443,11 +560,9 @@
  *	@fw may be %NULL if firmware request fails.
  *
  **/
-int
-request_firmware_nowait(
-	struct module *module,
-	const char *name, struct device *device, void *context,
-	void (*cont)(const struct firmware *fw, void *context))
+int request_firmware_nowait(struct module *module,
+			    const char *name, struct device *device, void *context,
+			    void (*cont)(const struct firmware *fw, void *context))
 {
 	struct firmware_work *fw_work = kmalloc(sizeof (struct firmware_work),
 						GFP_ATOMIC);

@@ -471,27 +586,26 @@
 	return 0;
 }
 
-static int __init
-firmware_class_init(void)
+static int __init firmware_class_init(void)
 {
 	int error;
+
 	error = class_register(&firmware_class);
 	if (error) {
 		printk(KERN_ERR "%s: class_register failed\n", __FUNCTION__);
-	}
-	error = class_create_file(&firmware_class, &class_attr_timeout);
-	if (error) {
-		printk(KERN_ERR "%s: class_create_file failed\n",
-		       __FUNCTION__);
-		class_unregister(&firmware_class);
+	} else {
+		error = class_create_file(&firmware_class, &class_attr_timeout);
+		if (error) {
+			printk(KERN_ERR "%s: class_create_file failed\n",
+			       __FUNCTION__);
+			class_unregister(&firmware_class);
+		}
 	}
 	return error;
 
 }
-static void __exit
-firmware_class_exit(void)
+static void __exit firmware_class_exit(void)
 {
-	class_remove_file(&firmware_class, &class_attr_timeout);
 	class_unregister(&firmware_class);
 }

include/linux/kobject.h +2 -0

diff -Nru a/include/linux/kobject.h b/include/linux/kobject.h
@@ -56,6 +56,8 @@
 extern struct kobject * kobject_get(struct kobject *);
 extern void kobject_put(struct kobject *);
 
+extern int kobject_get_path_length(struct kobject *);
+extern void kobject_fill_path(struct kobject *, char *, int);
 
 struct kobj_type {
 	void (*release)(struct kobject *);

lib/kobject.c +23 -6

diff -Nru a/lib/kobject.c b/lib/kobject.c
@@ -64,9 +64,12 @@
 	return container_of(entry,struct kobject,entry);
 }
 
+/**
+ *	kobject_get_path_length - get length of path to the object.
+ *	@kobj:	object in question.
+ */
 
-#ifdef CONFIG_HOTPLUG
-static int get_kobj_path_length(struct kset *kset, struct kobject *kobj)
+int kobject_get_path_length(struct kobject *kobj)
 {
 	int length = 1;
 	struct kobject * parent = kobj;

@@ -82,10 +85,21 @@
 	return length;
 }
 
-static void fill_kobj_path(struct kset *kset, struct kobject *kobj, char *path, int length)
+EXPORT_SYMBOL(kobject_get_path_length);
+
+/**
+ *	kobject_get_path_length - get length of path to the object.
+ *	@kobj:		object in question.
+ *	@path:		buffer where path should go
+ *	@length:	length of the buffer obtained by calling
+ *			kobject_get_path_length 
+ */
+
+void kobject_fill_path(struct kobject *kobj, char *path, int length)
 {
 	struct kobject * parent;
 
+	memset(path, 0x00, length);
 	--length;
 	for (parent = kobj; parent; parent = parent->parent) {
 		int cur = strlen(kobject_name(parent));

@@ -98,6 +112,10 @@
 	pr_debug("%s: path = '%s'\n",__FUNCTION__,path);
 }
 
+EXPORT_SYMBOL(kobject_fill_path);
+
+#ifdef CONFIG_HOTPLUG
+
 #define BUFFER_SIZE	1024	/* should be enough memory for the env */
 #define NUM_ENVP	32	/* number of env pointers */
 static unsigned long sequence_num;

@@ -163,12 +181,11 @@
 	envp [i++] = scratch;
 	scratch += sprintf(scratch, "SEQNUM=%ld", seq) + 1;
 
-	kobj_path_length = get_kobj_path_length (kset, kobj);
+	kobj_path_length = kobject_get_path_length (kobj);
 	kobj_path = kmalloc (kobj_path_length, GFP_KERNEL);
 	if (!kobj_path)
 		goto exit;
-	memset (kobj_path, 0x00, kobj_path_length);
-	fill_kobj_path (kset, kobj, kobj_path, kobj_path_length);
+	kobject_fill_path (kobj, kobj_path, kobj_path_length);
 
 	envp [i++] = scratch;
 	scratch += sprintf (scratch, "DEVPATH=%s", kobj_path) + 1;

Greg KH

2003-12-22 09:38 UTC #1 lore raw

Quoted 7 lines · Dmitry Torokhov

Hi,
It seems that implementation of the firmware loader is racy as it relies on kobject hotplug handler. Unfortunately that handler runs too early, before firmware class attributes controlling the loading process, are created. This causes firmware loading fail at least half of the times on my laptop.

Um, why not have your script wait until the files are present? That will remove any race conditions you will have.

Quoted 8 lines

Another problem that I see is that the present implementation tries to free some of the allocated resources manually instead of relying on driver model. Particularly damaging is freeing fw_priv in request_firmware. Although the code calls fw_remove_class_device (which in turns calls class_device_unregister) the freeing of class device and all its attributes can be delayed as the attribute files may still be held open by the userspace handler or any other program. Subsequent access to these files could cause trouble.

Cleanups should happen in the release function. If the firmware code doesn't do this, it's wrong.

thanks,

greg k-h

Manuel Estrada Sainz

2003-12-22 23:06 UTC #12 lore raw

In reply to Dmitry Torokhov

Quoted 7 lines · Dmitry Torokhov

Hi,
It seems that implementation of the firmware loader is racy as it relies on kobject hotplug handler. Unfortunately that handler runs too early, before firmware class attributes controlling the loading process, are created. This causes firmware loading fail at least half of the times on my laptop.

As Greg suggested, make your hotplug handler wait for the files to appear and you are set.

Actually "sleep 1" will generally do the trick.

What can actually be a problem is that hotplug delays event handling while booting, and if firmware needing drivers load at boot time they usually timeout before the event gets handled, and when hotplug tryies to handle the event the files are already gone.

The only solution I see to the later is making the default timeout much bigger and maybe having hotplug reset it to a shorter value once it starts handling events.

Quoted 8 lines

Another problem that I see is that the present implementation tries to free some of the allocated resources manually instead of relying on driver model. Particularly damaging is freeing fw_priv in request_firmware. Although the code calls fw_remove_class_device (which in turns calls class_device_unregister) the freeing of class device and all its attributes can be delayed as the attribute files may still be held open by the userspace handler or any other program. Subsequent access to these files could cause trouble.

There are actually some oopses, which I believe come from incorrect refcounting in sysfs, when I get around to looking at it I'll also audit "use after free" issues with firmware_priv struct.

Quoted 2 lines

Also, there is no protection from overwriting firmware image once it has been committed.

The hotplug handler runs as root, and if buggy enough it could write a broken firmware image anyway, or write to /dev/mem or whatever, I don't see a point in this level of paranoia.

Quoted 9 lines

I tried to correct all 3 problems in the patch below. It creates a custom hotplug handler that is called from request_hardware. I tried to mimic the hotplug handler from kobject - it's nice to have DEVPATH pointing to the right place - so I exported kobject_get_path_length and kobject_fill_path (former get_kobj_path_length and fill_kobj_patch). I think these 2 should not be considered "implementation details" and exporting them is OK. Write access to firmware device class attributes is protected by a semaphore and the code refuses any updates once firmware loading has been committed or aborted. Firmware

I am currently on Xmas vacation, until January I won't be very responsive.

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.

Greg KH

2003-12-22 23:23 UTC #13 lore raw

Quoted 5 lines · Manuel Estrada Sainz

What can actually be a problem is that hotplug delays event handling while booting, and if firmware needing drivers load at boot time they usually timeout before the event gets handled, and when hotplug tryies to handle the event the files are already gone.

What would remove the files?

And if you need firmware at boot time, stick it, and the firmware loader in initramfs.

thanks,

greg k-h

Marcel Holtmann

2003-12-22 23:43 UTC #2 lore raw

In reply to Greg KH

Hi Greg,

Quoted 8 lines

Quoted 5 lines
It seems that implementation of the firmware loader is racy as it relies on kobject hotplug handler. Unfortunately that handler runs too early, before firmware class attributes controlling the loading process, are created. This causes firmware loading fail at least half of the times on my laptop.
Um, why not have your script wait until the files are present? That will remove any race conditions you will have.

the firmware.agent script that is now in linux-hotplug already do a "sleep 1" if the the loading file is not present.

Regards

Marcel

Dmitry Torokhov

author

2003-12-23 03:29 UTC #3 lore raw

In reply to Greg KH

Quoted 12 lines · Greg KH

Quoted 7 lines · Dmitry Torokhov
Hi,
It seems that implementation of the firmware loader is racy as it relies on kobject hotplug handler. Unfortunately that handler runs too early, before firmware class attributes controlling the loading process, are created. This causes firmware loading fail at least half of the times on my laptop.
Um, why not have your script wait until the files are present? That will remove any race conditions you will have.

How long should the userspace wait? One second as Manuel suggested? Indefinitely? Or should the firmware agent have some timeout? If userspace uses a timeout how should it correlate with the timeout on the kernel side?

I am sorry but I have to disagree with you. Kernel should not call user space until it has all infrastructure in place and is ready. Anything else is just a sloppy practice.

Dmitry

Marcel Holtmann

2003-12-23 08:49 UTC #4 lore raw

Hi Dmitry,

Quoted 6 lines

Quoted 6 lines
Quoted 5 lines
It seems that implementation of the firmware loader is racy as it relies on kobject hotplug handler. Unfortunately that handler runs too early, before firmware class attributes controlling the loading process, are created. This causes firmware loading fail at least half of the times on my laptop.

Quoted 6 lines

Quoted 2 lines
Um, why not have your script wait until the files are present? That will remove any race conditions you will have.
How long should the userspace wait? One second as Manuel suggested? Indefinitely? Or should the firmware agent have some timeout? If userspace uses a timeout how should it correlate with the timeout on the kernel side?

the timeout of the kernel (which can be set from userspace) is for the whole firmware loading process. What we talk about is waiting a little bit before the files become visible for the firmware.agent.

Quoted 3 lines

I am sorry but I have to disagree with you. Kernel should not call user space until it has all infrastructure in place and is ready. Anything else is just a sloppy practice.

The firmware.agent script has 3 extra lines to check for the visibility of the "loading" file and if it is not present it will sleep one second. This is a actual good practice compared to adding much more code to the kernel and have an own way of running hotplug.

Regards

Marcel

Manuel Estrada Sainz

2003-12-26 17:30 UTC #14 lore raw

In reply to Greg KH

Quoted 8 lines · Greg KH

Quoted 5 lines · Manuel Estrada Sainz
What can actually be a problem is that hotplug delays event handling while booting, and if firmware needing drivers load at boot time they usually timeout before the event gets handled, and when hotplug tryies to handle the event the files are already gone.
What would remove the files?

Once the firmware load is over, whether normally or because of any error request_firmware() cleans up, and that includes removing sysfs files.

Quoted 2 lines

And if you need firmware at boot time, stick it, and the firmware loader in initramfs.

Once klibc and hotplug go into the standard kernel I'll look into request_firmware support there, even ACPI could benefit from it, but until then ...

Although I don't have it clear, what happens between mounting the real root device on top of rootfs and hotplug starting to handle events? Maybe hotplug could treat firmware events specially and not delay them even while booting.

I guess that we will get the real taste of it once initramfs gets it's klibc and starts being used broadly.

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.

Dmitry Torokhov

author

2003-12-27 05:30 UTC #5 lore raw

Re: [2.6 PATCH/RFC] Firmware loader fixes - take 2

In reply to Marcel Holtmann

Hi Marcel,

Quoted 1 line · Marcel Holtmann

Hi Dmitry,

[..SKIP..]

Quoted 9 lines

Quoted 3 lines
I am sorry but I have to disagree with you. Kernel should not call user space until it has all infrastructure in place and is ready. Anything else is just a sloppy practice.
The firmware.agent script has 3 extra lines to check for the visibility of the "loading" file and if it is not present it will sleep one second. This is a actual good practice compared to adding much more code to the kernel and have an own way of running hotplug.

OK, so if I understand you correctly you are against bloating the kernel by duplicating hotplug handler, not against the basic idea that kernel should not call userspace unless its ready. Your wish is my command!

I changed the code a bit and split it into 2 patches for easier consumption. The first patch takes care of resource deallocation issues, the second one deals with hotplug. Firmware hotplug now uses the standard hotplug handler (I had to make it public) and as you can see it costs 5 extra lines of C code (not counting one blank line) in firmware_class.c, hardly a bloat.

Dmitry

Dmitry Torokhov

author

2003-12-27 05:30 UTC #7 lore raw

Re: [2.6 PATCH/RFC] Firmware loader fixes - take 2 (patch 2/2)

In reply to Dmitry Torokhov

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

[email protected], 2003-12-25 23:08:12-05:00, [email protected]
Firmware loader:
- make kobject hotplug mechanism public
- do not call hotplug until firmware class device is completely instantiated

 drivers/base/firmware_class.c |    6 +++++
 include/linux/kobject.h       |    1 
 lib/kobject.c                 |   49 ++++++++++++++++++------------------------
 3 files changed, 28 insertions(+), 28 deletions(-)

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

drivers/base/firmware_class.c +6 -0

diff -Nru a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
@@ -25,6 +25,7 @@
 #define FW_STATE_LOADED		0
 #define FW_STATE_LOADING	1
 #define FW_STATE_NEW		2
+#define FW_STATE_READY		3
 
 static int loading_timeout = 10;	/* In seconds */

@@ -81,6 +82,9 @@
 	int i = 0;
 	char *scratch = buffer;
 
+	if (fw_priv->state != FW_STATE_READY)
+		return -ENODEV;
+
 	if (buffer_size < (FIRMWARE_NAME_MAX + 10))
 		return -ENOMEM;
 	if (num_envp < 1)

@@ -356,6 +360,7 @@
 		goto err_out_unregister;
 	}
 
+	fw_priv->state = FW_STATE_READY;
 	*class_dev_p = class_dev;
 	return 0;

@@ -402,6 +407,7 @@
 		add_timer(&fw_priv->timeout);
 	}
 
+	kobject_hotplug("add", &class_dev->kobj);
 	wait_for_completion(&fw_priv->completion);
 
 	del_timer(&fw_priv->timeout);

include/linux/kobject.h +1 -0

diff -Nru a/include/linux/kobject.h b/include/linux/kobject.h
@@ -56,6 +56,7 @@
 extern struct kobject * kobject_get(struct kobject *);
 extern void kobject_put(struct kobject *);
 
+extern void kobject_hotplug(const char *action, struct kobject *);
 
 struct kobj_type {
 	void (*release)(struct kobject *);

lib/kobject.c +21 ~~-28~~

81 lines not on this page · raw message

Dmitry Torokhov

author

2003-12-27 05:32 UTC #6 lore raw

Re: [2.6 PATCH/RFC] Firmware loader fixes - take 2 (patch 1/2)

In reply to Dmitry Torokhov

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

[email protected], 2003-12-25 22:44:59-05:00, [email protected]
Firmware loader: correctly free allocated resources
- free allocated memory in class_device release method
- rely on sysfs to remove all attributes when device class gets unregistered

 firmware_class.c |  229 ++++++++++++++++++++++++++++++++-----------------------
 1 files changed, 135 insertions(+), 94 deletions(-)

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

drivers/base/firmware_class.c +135 ~~-94~~

diff -Nru a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
@@ -21,6 +21,11 @@
 MODULE_DESCRIPTION("Multi purpose firmware loading support");
 MODULE_LICENSE("GPL");
 
+#define FW_STATE_ABORTED	-1
+#define FW_STATE_LOADED		0
+#define FW_STATE_LOADING	1
+#define FW_STATE_NEW		2
+
 static int loading_timeout = 10;	/* In seconds */
 
 struct firmware_priv {

@@ -28,8 +33,7 @@
 	struct completion completion;
 	struct bin_attribute attr_data;
 	struct firmware *fw;
-	int loading;
-	int abort;
+	int state;
 	int alloc_size;
 	struct timer_list timeout;
 };

@@ -91,7 +95,23 @@
 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);
+	return sprintf(buf, "%d\n", fw_priv->state);
+}
+
+static void firmware_start_loading(struct firmware_priv *fw_priv)
+{
+	fw_priv->state = FW_STATE_LOADING;
+	vfree(fw_priv->fw->data);
+	fw_priv->fw->data = NULL;
+	fw_priv->fw->size = 0;
+	fw_priv->alloc_size = 0;
+}
+
+static void firmware_finish_loading(struct firmware_priv *fw_priv, int commit)
+{
+	fw_priv->state = commit ? FW_STATE_LOADED : FW_STATE_ABORTED;
+	wmb();
+	complete(&fw_priv->completion);
 }
 
 /**

@@ -108,25 +128,21 @@
 		       const char *buf, size_t count)
 {
 	struct firmware_priv *fw_priv = class_get_devdata(class_dev);
-	int prev_loading = fw_priv->loading;
+	int new_state = simple_strtol(buf, NULL, 10);
+	int complete;
 
-	fw_priv->loading = simple_strtol(buf, NULL, 10);
-
-	switch (fw_priv->loading) {
-	case -1:
-		fw_priv->abort = 1;
-		wmb();
-		complete(&fw_priv->completion);
+	switch (new_state) {
+	case FW_STATE_LOADING:
+		firmware_start_loading(fw_priv);
 		break;
-	case 1:
-		kfree(fw_priv->fw->data);
-		fw_priv->fw->data = NULL;
-		fw_priv->fw->size = 0;
-		fw_priv->alloc_size = 0;
+	case FW_STATE_LOADED:
+		complete = fw_priv->state == FW_STATE_LOADING &&
+			   fw_priv->fw->size != 0;
+		firmware_finish_loading(fw_priv, complete); 
 		break;
-	case 0:
-		if (prev_loading == 1)
-			complete(&fw_priv->completion);
+	case FW_STATE_ABORTED:
+	default:
+		firmware_finish_loading(fw_priv, 0);
 		break;
 	}

@@ -164,7 +180,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;
+		firmware_finish_loading(fw_priv, 0);
 		return -ENOMEM;
 	}
 	fw_priv->alloc_size += PAGE_SIZE;

@@ -214,6 +230,12 @@
 static void
 fw_class_dev_release(struct class_device *class_dev)
 {
+	struct firmware_priv *fw_priv = class_get_devdata(class_dev);
+
+	if (fw_priv->state != FW_STATE_LOADED)
+		release_firmware(fw_priv->fw);
+
+	kfree(fw_priv);
 	kfree(class_dev);
 }

@@ -221,63 +243,109 @@
 firmware_class_timeout(u_long data)
 {
 	struct firmware_priv *fw_priv = (struct firmware_priv *) data;
-	fw_priv->abort = 1;
-	wmb();
-	complete(&fw_priv->completion);
+
+	firmware_finish_loading(fw_priv, 0);
 }
 
 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,
-		      const char *fw_name, struct device *device)
-{
-	int retval = 0;
-	struct firmware_priv *fw_priv = kmalloc(sizeof (struct firmware_priv),
-						GFP_KERNEL);
-	struct class_device *class_dev = kmalloc(sizeof (struct class_device),
-						 GFP_KERNEL);
 
-	if (!fw_priv || !class_dev) {
+static struct class_device *fw_alloc_class_device(struct device *device,
+						  const char *fw_name)
+{
+	struct firmware *firmware;
+	struct firmware_priv *fw_priv;
+	struct class_device *class_dev;
+	int retval;
+ 
+	firmware = kmalloc(sizeof (struct firmware), GFP_KERNEL);
+	if (!firmware) {
+		printk(KERN_ERR "%s: kmalloc(struct firmware) failed\n",
+		       __FUNCTION__);
 		retval = -ENOMEM;
-		goto error_kfree;
+		goto err_out; 
 	}
-	memset(fw_priv, 0, sizeof (*fw_priv));
-	memset(class_dev, 0, sizeof (*class_dev));
 
+	memset(firmware, 0, sizeof (*firmware));
+       
+	fw_priv = kmalloc(sizeof(struct firmware_priv), GFP_KERNEL);
+	if (!fw_priv) {
+		printk(KERN_ERR "%s: kmalloc(struct firmware_priv) failed\n",
+		       __FUNCTION__);
+		goto err_out_free_firmware;
+        }
+
+	memset(fw_priv, 0, sizeof (*fw_priv));
+	strlcpy(fw_priv->fw_id, fw_name, FIRMWARE_NAME_MAX);
 	init_completion(&fw_priv->completion);
-	memcpy(&fw_priv->attr_data, &firmware_attr_data_tmpl,
-	       sizeof (firmware_attr_data_tmpl));
+	fw_priv->attr_data = firmware_attr_data_tmpl;
+	fw_priv->fw = firmware;
+	fw_priv->state = FW_STATE_NEW;
+	init_timer(&fw_priv->timeout);
+	fw_priv->timeout.function = firmware_class_timeout;
+	fw_priv->timeout.data = (u_long) fw_priv;
 
-	strncpy(&fw_priv->fw_id[0], fw_name, FIRMWARE_NAME_MAX);
-	fw_priv->fw_id[FIRMWARE_NAME_MAX - 1] = '\0';
+	class_dev = kmalloc(sizeof(struct class_device), GFP_KERNEL);
+	if (!class_dev) {
+		printk(KERN_ERR "%s: kmalloc(struct class_device) failed\n",
+		       __FUNCTION__);
+		goto err_out_free_fw_priv;
+	}
 
+	memset(class_dev, 0, sizeof(*class_dev));
 	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);
-
 	class_dev->class = &firmware_class;
 	class_set_devdata(class_dev, fw_priv);
+
+	return class_dev;
+
+err_out_free_fw_priv:
+	kfree(fw_priv);
+err_out_free_firmware:
+	kfree(firmware);
+err_out:
+	return NULL;
+}
+
+static int
+fw_setup_class_device(struct class_device **class_dev_p,
+		      const char *fw_name, struct device *device)
+{
+	struct class_device *class_dev = fw_alloc_class_device(device, fw_name);
+	struct firmware_priv *fw_priv;
+	int retval;
+
+	if (!class_dev) {
+		retval = -ENOMEM;
+		goto err_out;
+	}
+
+	fw_priv = class_get_devdata(class_dev);
+
 	retval = class_device_register(class_dev);
 	if (retval) {
 		printk(KERN_ERR "%s: class_device_register failed\n",
 		       __FUNCTION__);
-		goto error_kfree;
+		fw_class_dev_release(class_dev);
+		retval = -ENOMEM;
+		goto err_out;
 	}
 
+	/*
+	 * We successfully registered class_dev, now we should not
+	 * manually free resources as they will be freed automatically.
+	 */
+
 	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 err_out_unregister;
 	}
 
 	retval = class_device_create_file(class_dev,

@@ -285,43 +353,18 @@
 	if (retval) {
 		printk(KERN_ERR "%s: class_device_create_file failed\n",
 		       __FUNCTION__);
-		goto error_remove_data;
+		goto err_out_unregister;
 	}
 
-	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));
-
-	goto out;
+	*class_dev_p = class_dev;
+	return 0;
 
-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:
+err_out_unregister:
 	class_device_unregister(class_dev);
-error_kfree:
-	kfree(fw_priv);
-	kfree(class_dev);
+err_out:
 	*class_dev_p = NULL;
-out:
-	*class_dev_p = class_dev;
 	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

@@ -362,16 +405,13 @@
 	wait_for_completion(&fw_priv->completion);
 
 	del_timer(&fw_priv->timeout);
-	fw_remove_class_device(class_dev);
 
-	if (fw_priv->fw->size && !fw_priv->abort) {
+	if (fw_priv->state == FW_STATE_LOADED)
 		*firmware = fw_priv->fw;
-	} else {
+	else
 		retval = -ENOENT;
-		vfree(fw_priv->fw->data);
-		kfree(fw_priv->fw);
-	}
-	kfree(fw_priv);
+
+	class_device_unregister(class_dev);
 out:
 	return retval;
 }

@@ -475,23 +515,24 @@
 firmware_class_init(void)
 {
 	int error;
+
 	error = class_register(&firmware_class);
 	if (error) {
 		printk(KERN_ERR "%s: class_register failed\n", __FUNCTION__);
-	}
-	error = class_create_file(&firmware_class, &class_attr_timeout);
-	if (error) {
-		printk(KERN_ERR "%s: class_create_file failed\n",
-		       __FUNCTION__);
-		class_unregister(&firmware_class);
+	} else {
+		error = class_create_file(&firmware_class, &class_attr_timeout);
+		if (error) {
+			printk(KERN_ERR "%s: class_create_file failed\n",
+			       __FUNCTION__);
+			class_unregister(&firmware_class);
+		}
 	}
 	return error;
-
 }
+
 static void __exit
 firmware_class_exit(void)
 {
-	class_remove_file(&firmware_class, &class_attr_timeout);
 	class_unregister(&firmware_class);
 }

Greg KH

2003-12-31 21:33 UTC #8 lore raw

Re: [2.6 PATCH/RFC] Firmware loader fixes - take 2 (patch 2/2)

In reply to Dmitry Torokhov

Quoted 6 lines · Dmitry Torokhov

===================================================================
[email protected], 2003-12-25 23:08:12-05:00, [email protected]
Firmware loader:
- make kobject hotplug mechanism public

I've applied the kobject portion of this patch to my local tree, as it's good stuff.

I'll let Pat decide on if it should be sent onward or not.

thanks,

greg k-h

Manuel Estrada Sainz

2003-12-31 22:16 UTC #9 lore raw

Re: [2.6 PATCH/RFC] Firmware loader fixes - take 2 (patch 1/2)

In reply to Dmitry Torokhov

Quoted 8 lines · Dmitry Torokhov

===================================================================
[email protected], 2003-12-25 22:44:59-05:00, [email protected]
Firmware loader: correctly free allocated resources
- free allocated memory in class_device release method
- rely on sysfs to remove all attributes when device class gets unregistered

The problem is real, but I find the patch too intrusive.

I'll try to fix the problem based on it.

Thanks

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.

Manuel Estrada Sainz

2003-12-31 22:33 UTC #10 lore raw

In reply to Marcel Holtmann

Quoted 19 lines · Marcel Holtmann

Hi Dmitry,
Quoted 6 lines
Quoted 6 lines
Quoted 5 lines
It seems that implementation of the firmware loader is racy as it relies on kobject hotplug handler. Unfortunately that handler runs too early, before firmware class attributes controlling the loading process, are created. This causes firmware loading fail at least half of the times on my laptop.
Quoted 6 lines
Quoted 2 lines
Um, why not have your script wait until the files are present? That will remove any race conditions you will have.
How long should the userspace wait? One second as Manuel suggested? Indefinitely? Or should the firmware agent have some timeout? If userspace uses a timeout how should it correlate with the timeout on the kernel side?
the timeout of the kernel (which can be set from userspace) is for the whole firmware loading process. What we talk about is waiting a little bit before the files become visible for the firmware.agent.

Quoted 8 lines

Quoted 3 lines
I am sorry but I have to disagree with you. Kernel should not call user space until it has all infrastructure in place and is ready. Anything else is just a sloppy practice.
The firmware.agent script has 3 extra lines to check for the visibility of the "loading" file and if it is not present it will sleep one second. This is a actual good practice compared to adding much more code to the kernel and have an own way of running hotplug.

I would say that we will get into this with other issues.

Maybe some generic mechanism could be implemented to make the hotplug event wait for the files.

The least intrusive solution, although it doesn't sound quite clean could be:

- Kernel side:
- sysfs_hotplug_frezze()
- Creates a dummy file /sys/.hotplug_frozen
- sysfs_hotplug_thaw()
- Removes /sys/.hotplug_frozen
- Userspace:
- Main hotplug script waits "while [ -f /sys/.hotplug_frozen ]".

sysfs_hotplug_[frezze|thaw]() should be treated almost like spinlocks, so the system's behabiour doesn't get afected.

Dmitry's patch is probably cleaner, but since I already had written the email when I noticed ...

Happy new year everyone

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.

Greg KH

2004-01-01 20:00 UTC #11 lore raw

Quoted 3 lines · Manuel Estrada Sainz

Maybe some generic mechanism could be implemented to make the hotplug event wait for the files.

No.

Quoted 9 lines

The least intrusive solution, although it doesn't sound quite clean could be:
- Kernel side:
(quoted diff) +0 -0
 	- sysfs_hotplug_frezze()
 		- Creates a dummy file /sys/.hotplug_frozen
 	- sysfs_hotplug_thaw()
 		- Removes /sys/.hotplug_frozen
 - Userspace:
- Main hotplug script waits "while [ -f /sys/.hotplug_frozen ]".

Ick. You realize how many hotplug events we spit out already today? This would be a huge bottleneck.

It's not that tough to just see if the file you are looking is there, and if not sleep. If after an ammount of time the file still isn't there, just give up.

Also, not all hotplug scripts care about sysfs files being present :)

thanks,

greg k-h