[2.6 PATCH/RFC] Firmware loader - fix races and resource dealloocation problems

[2.6 PATCH/RFC] Firmware loader - fix races and resource dealloocation problems

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

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

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


ChangeSet@1.1521, 2003-12-21 01:00:41-05:00, dtor_core@ameritech.net
  - 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(-)


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



diff -Nru a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
--- a/drivers/base/firmware_class.c	Sun Dec 21 01:03:27 2003
+++ b/drivers/base/firmware_class.c	Sun Dec 21 01:03:27 2003
@@ -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);
 }
 
diff -Nru a/include/linux/kobject.h b/include/linux/kobject.h
--- a/include/linux/kobject.h	Sun Dec 21 01:03:27 2003
+++ b/include/linux/kobject.h	Sun Dec 21 01:03:27 2003
@@ -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 *);
diff -Nru a/lib/kobject.c b/lib/kobject.c
--- a/lib/kobject.c	Sun Dec 21 01:03:27 2003
+++ b/lib/kobject.c	Sun Dec 21 01:03:27 2003
@@ -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;

Re: [2.6 PATCH/RFC] Firmware loader - fix races and resource dealloocation problems

[Greg KH]

On Sun, Dec 21, 2003 at 01:37:39AM -0500, Dmitry Torokhov wrote:
> 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.

> 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

Re: [2.6 PATCH/RFC] Firmware loader - fix races and resource dealloocation problems

[Manuel Estrada Sainz]

On Sun, Dec 21, 2003 at 01:37:39AM -0500, Dmitry Torokhov wrote:
> 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.
 
> 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.

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

-- 
--- 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: [2.6 PATCH/RFC] Firmware loader - fix races and resource dealloocation problems

[Greg KH]

On Tue, Dec 23, 2003 at 12:05:59AM +0100, Manuel Estrada Sainz wrote:
> 
>  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

Re: [2.6 PATCH/RFC] Firmware loader - fix races and resource dealloocation problems

[Marcel Holtmann]

Hi Greg,

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

Re: [2.6 PATCH/RFC] Firmware loader - fix races and resource dealloocation problems

[Dmitry Torokhov]

On Monday 22 December 2003 04:37 am, Greg KH wrote:
> On Sun, Dec 21, 2003 at 01:37:39AM -0500, Dmitry Torokhov wrote:
> > 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

Re: [2.6 PATCH/RFC] Firmware loader - fix races and resource dealloocation problems

[Marcel Holtmann]

Hi Dmitry,

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

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.

> 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

Re: [2.6 PATCH/RFC] Firmware loader - fix races and resource dealloocation problems

[Manuel Estrada Sainz]

On Mon, Dec 22, 2003 at 03:22:50PM -0800, Greg KH wrote:
> On Tue, Dec 23, 2003 at 12:05:59AM +0100, Manuel Estrada Sainz wrote:
> > 
> >  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. 

> 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


-- 
--- 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: [2.6 PATCH/RFC] Firmware loader fixes - take 2

[Dmitry Torokhov]

Hi Marcel,

On Tuesday 23 December 2003 03:48 am, Marcel Holtmann wrote:
> Hi Dmitry,
[..SKIP..]
> > 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

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

[Dmitry Torokhov]

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


ChangeSet@1.1527, 2003-12-25 22:44:59-05:00, dtor_core@ameritech.net
  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(-)


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



diff -Nru a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
--- a/drivers/base/firmware_class.c	Sat Dec 27 00:27:19 2003
+++ b/drivers/base/firmware_class.c	Sat Dec 27 00:27:19 2003
@@ -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);
 }
 

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

[Dmitry Torokhov]

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


ChangeSet@1.1528, 2003-12-25 23:08:12-05:00, dtor_core@ameritech.net
  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(-)


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



diff -Nru a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
--- a/drivers/base/firmware_class.c	Sat Dec 27 00:27:38 2003
+++ b/drivers/base/firmware_class.c	Sat Dec 27 00:27:38 2003
@@ -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);
diff -Nru a/include/linux/kobject.h b/include/linux/kobject.h
--- a/include/linux/kobject.h	Sat Dec 27 00:27:38 2003
+++ b/include/linux/kobject.h	Sat Dec 27 00:27:38 2003
@@ -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 *);
diff -Nru a/lib/kobject.c b/lib/kobject.c
--- a/lib/kobject.c	Sat Dec 27 00:27:38 2003
+++ b/lib/kobject.c	Sat Dec 27 00:27:38 2003
@@ -198,9 +198,24 @@
 	kfree(envp);
 	return;
 }
+
+void kobject_hotplug(const char *action, struct kobject *kobj)
+{
+	struct kobject * top_kobj = kobj;
+
+	/* If this kobj does not belong to a kset,
+	   try to find a parent that does. */
+	if (!top_kobj->kset && top_kobj->parent) {
+		do {
+			top_kobj = top_kobj->parent;
+		} while (!top_kobj->kset && top_kobj->parent);
+	}
+
+	if (top_kobj->kset && top_kobj->kset->hotplug_ops)
+		kset_hotplug(action, top_kobj->kset, kobj);
+}
 #else
-static void kset_hotplug(const char *action, struct kset *kset,
-			 struct kobject *kobj)
+void kobject_hotplug(const char *action, struct kobject *kobj)
 {
 	return;
 }
@@ -248,7 +263,6 @@
 {
 	int error = 0;
 	struct kobject * parent;
-	struct kobject * top_kobj;
 
 	if (!(kobj = kobject_get(kobj)))
 		return -ENOENT;
@@ -277,18 +291,9 @@
 		if (parent)
 			kobject_put(parent);
 	} else {
-		/* If this kobj does not belong to a kset,
-		   try to find a parent that does. */
-		top_kobj = kobj;
-		if (!top_kobj->kset && top_kobj->parent) {
-			do {
-				top_kobj = top_kobj->parent;
-			} while (!top_kobj->kset && top_kobj->parent);
-		}
-	
-		if (top_kobj->kset && top_kobj->kset->hotplug_ops)
-			kset_hotplug("add", top_kobj->kset, kobj);
+		kobject_hotplug("add", kobj);
 	}
+
 	return error;
 }
 
@@ -396,20 +401,7 @@
 
 void kobject_del(struct kobject * kobj)
 {
-	struct kobject * top_kobj;
-
-	/* If this kobj does not belong to a kset,
-	   try to find a parent that does. */
-	top_kobj = kobj;
-	if (!top_kobj->kset && top_kobj->parent) {
-		do {
-			top_kobj = top_kobj->parent;
-		} while (!top_kobj->kset && top_kobj->parent);
-	}
-
-	if (top_kobj->kset && top_kobj->kset->hotplug_ops)
-		kset_hotplug("remove", top_kobj->kset, kobj);
-
+	kobject_hotplug("remove", kobj);
 	sysfs_remove_dir(kobj);
 	unlink(kobj);
 }
@@ -638,6 +630,7 @@
 EXPORT_SYMBOL(kobject_unregister);
 EXPORT_SYMBOL(kobject_get);
 EXPORT_SYMBOL(kobject_put);
+EXPORT_SYMBOL(kobject_hotplug);
 
 EXPORT_SYMBOL(kset_register);
 EXPORT_SYMBOL(kset_unregister);

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

[Greg KH]

On Sat, Dec 27, 2003 at 12:30:18AM -0500, Dmitry Torokhov wrote:
> ===================================================================
> 
> 
> ChangeSet@1.1528, 2003-12-25 23:08:12-05:00, dtor_core@ameritech.net
>   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

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

[Manuel Estrada Sainz]

On Sat, Dec 27, 2003 at 12:29:56AM -0500, Dmitry Torokhov wrote:
> ===================================================================
> 
> 
> ChangeSet@1.1527, 2003-12-25 22:44:59-05:00, dtor_core@ameritech.net
>   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
-- 
--- 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: [2.6 PATCH/RFC] Firmware loader - fix races and resource dealloocation problems

[Manuel Estrada Sainz]

On Tue, Dec 23, 2003 at 09:48:09AM +0100, Marcel Holtmann wrote:
> Hi Dmitry,
> 
> > > > 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?
> 
> 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.
> 
> > 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
 
	

-- 
--- 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: [2.6 PATCH/RFC] Firmware loader - fix races and resource dealloocation problems

[Greg KH]

On Wed, Dec 31, 2003 at 11:32:44PM +0100, Manuel Estrada Sainz wrote:
> 
>  Maybe some generic mechanism could be implemented to make the hotplug
>  event wait for the files.

No.

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

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