[PATCH 0/5] Assorted patches for firmware loader

[PATCH 0/5] Assorted patches for firmware loader

[Dmitry Torokhov]

Hi Greg,

The latest lockdep warning coming form firmware loader code prompted me
to take a look at it and I thought that it could use a bit of cleaning up,
especially the part where old code was allocating 3 data structures
having the same life time separately whereas they could be combined into
one and allocated in one shot.

Please consider applying. Note that I already sent you first patch a couple
of days ago...

Thanks.

-- 
Dmitry

[PATCH 1/5] firmware loader: use statically initialized data attribute

[Dmitry Torokhov]

There is no reason why we are using a template for binary attribute
and copying it into per-firmware data before registering. Using the
original works as well and gets rid of the following lockdep
compaint:

 platform microcode: firmware: requesting intel-ucode/06-0f-0b
 BUG: key ffff88011371be70 not in .data!
 ------------[ cut here ]------------
 WARNING: at kernel/lockdep.c:2706 lockdep_init_map+0x125/0x140()
 Hardware name: Latitude D630
 Modules linked in: ...
 Pid: 738, comm: modprobe Tainted: P           2.6.34-rc1 #237
 Call Trace:
  [<ffffffff81048fb6>] warn_slowpath_common+0x76/0xb0
  [<ffffffff81048fff>] warn_slowpath_null+0xf/0x20
  [<ffffffff8107c915>] lockdep_init_map+0x125/0x140
  [<ffffffff8118485a>] sysfs_add_file_mode+0x6a/0xc0
  [<ffffffff811848bc>] sysfs_add_file+0xc/0x10
  [<ffffffff811870e1>] sysfs_create_bin_file+0x21/0x30
  [<ffffffff8130dff1>] fw_setup_device+0x81/0x120
  [<ffffffff8130e14f>] _request_firmware+0xbf/0x270
  [<ffffffff8130e38e>] request_firmware+0xe/0x10
  [<ffffffffa0064be1>] request_microcode_fw+0x61/0xa0 [microcode]
  [<ffffffffa00643b8>] microcode_init_cpu+0xb8/0xd0 [microcode]
  [<ffffffffa0064426>] mc_sysdev_add+0x56/0x70 [microcode]
  [<ffffffff813032ae>] sysdev_driver_register+0x9e/0x130
  [<ffffffffa006a000>] ? microcode_init+0x0/0x12a [microcode]
  [<ffffffffa006a0bd>] microcode_init+0xbd/0x12a [microcode]
  [<ffffffff810001d7>] do_one_initcall+0x37/0x190
  [<ffffffff8108d178>] sys_init_module+0xd8/0x250
  [<ffffffff81002fab>] system_call_fastpath+0x16/0x1b
 ---[ end trace 93c9f72439beee7f ]---


Signed-off-by: Dmitry Torokhov <dtor@mail.ru>
---

 drivers/base/firmware_class.c |    8 +++-----
 1 files changed, 3 insertions(+), 5 deletions(-)

diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
index d0dc26a..3f50b2e 100644
--- a/drivers/base/firmware_class.c
+++ b/drivers/base/firmware_class.c
@@ -41,7 +41,6 @@ static DEFINE_MUTEX(fw_lock);
 struct firmware_priv {
 	char *fw_id;
 	struct completion completion;
-	struct bin_attribute attr_data;
 	struct firmware *fw;
 	unsigned long status;
 	struct page **pages;
@@ -344,8 +343,8 @@ out:
 	return retval;
 }
 
-static struct bin_attribute firmware_attr_data_tmpl = {
-	.attr = {.name = "data", .mode = 0644},
+static struct bin_attribute firmware_attr_data = {
+	.attr = { .name = "data", .mode = 0644 },
 	.size = 0,
 	.read = firmware_data_read,
 	.write = firmware_data_write,
@@ -390,7 +389,6 @@ static int fw_register_device(struct device **dev_p, const char *fw_name,
 	}
 
 	init_completion(&fw_priv->completion);
-	fw_priv->attr_data = firmware_attr_data_tmpl;
 	fw_priv->fw_id = kstrdup(fw_name, GFP_KERNEL);
 	if (!fw_priv->fw_id) {
 		dev_err(device, "%s: Firmware name allocation failed\n",
@@ -442,7 +440,7 @@ static int fw_setup_device(struct firmware *fw, struct device **dev_p,
 	fw_priv = dev_get_drvdata(f_dev);
 
 	fw_priv->fw = fw;
-	retval = sysfs_create_bin_file(&f_dev->kobj, &fw_priv->attr_data);
+	retval = sysfs_create_bin_file(&f_dev->kobj, &firmware_attr_data);
 	if (retval) {
 		dev_err(device, "%s: sysfs_create_bin_file failed\n", __func__);
 		goto error_unreg;

Re: [PATCH 1/5] firmware loader: use statically initialized data attribute

[Greg KH]

On Sat, Mar 13, 2010 at 11:49:07PM -0800, Dmitry Torokhov wrote:
> There is no reason why we are using a template for binary attribute
> and copying it into per-firmware data before registering. Using the
> original works as well and gets rid of the following lockdep
> compaint:

This patch isn't needed anymore, right?

thanks,

greg k-h

[PATCH 2/5] firmware loader: rely on driver core to create class attribute

[Dmitry Torokhov]

Do not create 'timeout' attribute manually, let driver core do it for us.
This also ensures that attribute is cleaned up properly.

Signed-off-by: Dmitry Torokhov <dtor@mail.ru>
---

 drivers/base/firmware_class.c |   59 +++++++++++++++++------------------------
 1 files changed, 24 insertions(+), 35 deletions(-)

diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
index 3f50b2e..5f9d906 100644
--- a/drivers/base/firmware_class.c
+++ b/drivers/base/firmware_class.c
@@ -97,9 +97,26 @@ firmware_timeout_store(struct class *class,
 	return count;
 }
 
-static CLASS_ATTR(timeout, 0644, firmware_timeout_show, firmware_timeout_store);
+static struct class_attribute firmware_class_attrs[] = {
+	__ATTR(timeout, S_IWUSR | S_IRUGO,
+		firmware_timeout_show, firmware_timeout_store),
+	__ATTR_NULL
+};
 
-static void fw_dev_release(struct device *dev);
+static void fw_dev_release(struct device *dev)
+{
+	struct firmware_priv *fw_priv = dev_get_drvdata(dev);
+	int i;
+
+	for (i = 0; i < fw_priv->nr_pages; i++)
+		__free_page(fw_priv->pages[i]);
+	kfree(fw_priv->pages);
+	kfree(fw_priv->fw_id);
+	kfree(fw_priv);
+	kfree(dev);
+
+	module_put(THIS_MODULE);
+}
 
 static int firmware_uevent(struct device *dev, struct kobj_uevent_env *env)
 {
@@ -115,6 +132,7 @@ static int firmware_uevent(struct device *dev, struct kobj_uevent_env *env)
 
 static struct class firmware_class = {
 	.name		= "firmware",
+	.class_attrs	= firmware_class_attrs,
 	.dev_uevent	= firmware_uevent,
 	.dev_release	= fw_dev_release,
 };
@@ -350,21 +368,6 @@ static struct bin_attribute firmware_attr_data = {
 	.write = firmware_data_write,
 };
 
-static void fw_dev_release(struct device *dev)
-{
-	struct firmware_priv *fw_priv = dev_get_drvdata(dev);
-	int i;
-
-	for (i = 0; i < fw_priv->nr_pages; i++)
-		__free_page(fw_priv->pages[i]);
-	kfree(fw_priv->pages);
-	kfree(fw_priv->fw_id);
-	kfree(fw_priv);
-	kfree(dev);
-
-	module_put(THIS_MODULE);
-}
-
 static void
 firmware_class_timeout(u_long data)
 {
@@ -665,26 +668,12 @@ request_firmware_nowait(
 	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", __func__);
-		return error;
-	}
-	error = class_create_file(&firmware_class, &class_attr_timeout);
-	if (error) {
-		printk(KERN_ERR "%s: class_create_file failed\n",
-		       __func__);
-		class_unregister(&firmware_class);
-	}
-	return error;
-
+	return class_register(&firmware_class);
 }
-static void __exit
-firmware_class_exit(void)
+
+static void __exit firmware_class_exit(void)
 {
 	class_unregister(&firmware_class);
 }

[PATCH 3/5] firmware loader: split out builtin firmware handling

[Dmitry Torokhov]

Split builtin firmware handling into separate functions to clean up the
main body of code.

Signed-off-by: Dmitry Torokhov <dtor@mail.ru>
---

 drivers/base/firmware_class.c |   81 +++++++++++++++++++++++++++--------------
 1 files changed, 53 insertions(+), 28 deletions(-)

diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
index 5f9d906..9b4bca0 100644
--- a/drivers/base/firmware_class.c
+++ b/drivers/base/firmware_class.c
@@ -26,6 +26,52 @@ MODULE_AUTHOR("Manuel Estrada Sainz");
 MODULE_DESCRIPTION("Multi purpose firmware loading support");
 MODULE_LICENSE("GPL");
 
+/* Builtin firmware support */
+
+#ifdef CONFIG_FW_LOADER
+
+extern struct builtin_fw __start_builtin_fw[];
+extern struct builtin_fw __end_builtin_fw[];
+
+static bool fw_get_builtin_firmware(struct firmware *fw, const char *name)
+{
+	struct builtin_fw *b_fw;
+
+	for (b_fw = __start_builtin_fw; b_fw != __end_builtin_fw; b_fw++) {
+		if (strcmp(name, b_fw->name) == 0) {
+			fw->size = b_fw->size;
+			fw->data = b_fw->data;
+			return true;
+		}
+	}
+
+	return false;
+}
+
+static bool fw_is_builtin_firmware(const struct firmware *fw)
+{
+	struct builtin_fw *b_fw;
+
+	for (b_fw = __start_builtin_fw; b_fw != __end_builtin_fw; b_fw++)
+		if (fw->data == b_fw->data)
+			return true;
+
+	return false;
+}
+
+#else /* Module case - no builtin firmware support */
+
+static inline bool fw_get_builtin_firmware(struct firmware *fw, const char *name)
+{
+	return false;
+}
+
+static inline bool fw_is_builtin_firmware(const struct firmware *fw)
+{
+	return false;
+}
+#endif
+
 enum {
 	FW_STATUS_LOADING,
 	FW_STATUS_DONE,
@@ -50,14 +96,6 @@ struct firmware_priv {
 	struct timer_list timeout;
 };
 
-#ifdef CONFIG_FW_LOADER
-extern struct builtin_fw __start_builtin_fw[];
-extern struct builtin_fw __end_builtin_fw[];
-#else /* Module case. Avoid ifdefs later; it'll all optimise out */
-static struct builtin_fw *__start_builtin_fw;
-static struct builtin_fw *__end_builtin_fw;
-#endif
-
 static void
 fw_load_abort(struct firmware_priv *fw_priv)
 {
@@ -473,7 +511,6 @@ _request_firmware(const struct firmware **firmware_p, const char *name,
 	struct device *f_dev;
 	struct firmware_priv *fw_priv;
 	struct firmware *firmware;
-	struct builtin_fw *builtin;
 	int retval;
 
 	if (!firmware_p)
@@ -487,14 +524,9 @@ _request_firmware(const struct firmware **firmware_p, const char *name,
 		goto out;
 	}
 
-	for (builtin = __start_builtin_fw; builtin != __end_builtin_fw;
-	     builtin++) {
-		if (strcmp(name, builtin->name))
-			continue;
-		dev_info(device, "firmware: using built-in firmware %s\n",
-			 name);
-		firmware->size = builtin->size;
-		firmware->data = builtin->data;
+	if (fw_get_builtin_firmware(firmware, name)) {
+		dev_info(device,
+			 "firmware: using built-in firmware %s\n", name);
 		return 0;
 	}
 
@@ -565,19 +597,12 @@ request_firmware(const struct firmware **firmware_p, const char *name,
  * release_firmware: - release the resource associated with a firmware image
  * @fw: firmware resource to release
  **/
-void
-release_firmware(const struct firmware *fw)
+void release_firmware(const struct firmware *fw)
 {
-	struct builtin_fw *builtin;
-
 	if (fw) {
-		for (builtin = __start_builtin_fw; builtin != __end_builtin_fw;
-		     builtin++) {
-			if (fw->data == builtin->data)
-				goto free_fw;
-		}
-		vfree(fw->data);
-	free_fw:
+		if (!fw_is_builtin_firmware(fw))
+			vfree(fw->data);
+
 		kfree(fw);
 	}
 }

[PATCH 4/5] firmware loader: do not allocate firmare id separately

[Dmitry Torokhov]

fw_id has the same life time as firmware_priv so it makes sense to move
it into firmware_priv structure instead of allocating separately.

Signed-off-by: Dmitry Torokhov <dtor@mail.ru>
---

 drivers/base/firmware_class.c |   16 ++++------------
 1 files changed, 4 insertions(+), 12 deletions(-)

diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
index 9b4bca0..06ff016 100644
--- a/drivers/base/firmware_class.c
+++ b/drivers/base/firmware_class.c
@@ -85,15 +85,14 @@ static int loading_timeout = 60;	/* In seconds */
 static DEFINE_MUTEX(fw_lock);
 
 struct firmware_priv {
-	char *fw_id;
 	struct completion completion;
 	struct firmware *fw;
 	unsigned long status;
 	struct page **pages;
 	int nr_pages;
 	int page_array_size;
-	const char *vdata;
 	struct timer_list timeout;
+	char fw_id[];
 };
 
 static void
@@ -149,7 +148,6 @@ static void fw_dev_release(struct device *dev)
 	for (i = 0; i < fw_priv->nr_pages; i++)
 		__free_page(fw_priv->pages[i]);
 	kfree(fw_priv->pages);
-	kfree(fw_priv->fw_id);
 	kfree(fw_priv);
 	kfree(dev);
 
@@ -417,8 +415,8 @@ static int fw_register_device(struct device **dev_p, const char *fw_name,
 			      struct device *device)
 {
 	int retval;
-	struct firmware_priv *fw_priv = kzalloc(sizeof(*fw_priv),
-						GFP_KERNEL);
+	struct firmware_priv *fw_priv =
+		kzalloc(sizeof(*fw_priv) + strlen(fw_name) + 1 , GFP_KERNEL);
 	struct device *f_dev = kzalloc(sizeof(*f_dev), GFP_KERNEL);
 
 	*dev_p = NULL;
@@ -429,14 +427,8 @@ static int fw_register_device(struct device **dev_p, const char *fw_name,
 		goto error_kfree;
 	}
 
+	strcpy(fw_priv->fw_id, fw_name);
 	init_completion(&fw_priv->completion);
-	fw_priv->fw_id = kstrdup(fw_name, GFP_KERNEL);
-	if (!fw_priv->fw_id) {
-		dev_err(device, "%s: Firmware name allocation failed\n",
-			__func__);
-		retval = -ENOMEM;
-		goto error_kfree;
-	}
 
 	fw_priv->timeout.function = firmware_class_timeout;
 	fw_priv->timeout.data = (u_long) fw_priv;

[PATCH 5/5] firmware loader: embed device into firmware_priv structure

[Dmitry Torokhov]

Both these structures have the same lifetime rules so instead of allocating
and managing them separately embed struct device into struct firmware_priv.
Also make sure to delete sysfs attributes ourselves instead of expecting
sysfs to clean up our mess.

Signed-off-by: Dmitry Torokhov <dtor@mail.ru>
---

 drivers/base/firmware_class.c |  251 ++++++++++++++++++++---------------------
 1 files changed, 121 insertions(+), 130 deletions(-)

diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
index 06ff016..0fbf554 100644
--- a/drivers/base/firmware_class.c
+++ b/drivers/base/firmware_class.c
@@ -92,21 +92,25 @@ struct firmware_priv {
 	int nr_pages;
 	int page_array_size;
 	struct timer_list timeout;
+	struct device dev;
 	char fw_id[];
 };
 
-static void
-fw_load_abort(struct firmware_priv *fw_priv)
+static struct firmware_priv *to_firmware_priv(struct device *dev)
+{
+	return container_of(dev, struct firmware_priv, dev);
+}
+
+static void fw_load_abort(struct firmware_priv *fw_priv)
 {
 	set_bit(FW_STATUS_ABORT, &fw_priv->status);
 	wmb();
 	complete(&fw_priv->completion);
 }
 
-static ssize_t
-firmware_timeout_show(struct class *class,
-		      struct class_attribute *attr,
-		      char *buf)
+static ssize_t firmware_timeout_show(struct class *class,
+				     struct class_attribute *attr,
+				     char *buf)
 {
 	return sprintf(buf, "%d\n", loading_timeout);
 }
@@ -123,14 +127,14 @@ firmware_timeout_show(struct class *class,
  *
  *	Note: zero means 'wait forever'.
  **/
-static ssize_t
-firmware_timeout_store(struct class *class,
-			struct class_attribute *attr,
-			const char *buf, size_t count)
+static ssize_t firmware_timeout_store(struct class *class,
+				      struct class_attribute *attr,
+				      const char *buf, size_t count)
 {
 	loading_timeout = simple_strtol(buf, NULL, 10);
 	if (loading_timeout < 0)
 		loading_timeout = 0;
+
 	return count;
 }
 
@@ -142,21 +146,20 @@ static struct class_attribute firmware_class_attrs[] = {
 
 static void fw_dev_release(struct device *dev)
 {
-	struct firmware_priv *fw_priv = dev_get_drvdata(dev);
+	struct firmware_priv *fw_priv = to_firmware_priv(dev);
 	int i;
 
 	for (i = 0; i < fw_priv->nr_pages; i++)
 		__free_page(fw_priv->pages[i]);
 	kfree(fw_priv->pages);
 	kfree(fw_priv);
-	kfree(dev);
 
 	module_put(THIS_MODULE);
 }
 
 static int firmware_uevent(struct device *dev, struct kobj_uevent_env *env)
 {
-	struct firmware_priv *fw_priv = dev_get_drvdata(dev);
+	struct firmware_priv *fw_priv = to_firmware_priv(dev);
 
 	if (add_uevent_var(env, "FIRMWARE=%s", fw_priv->fw_id))
 		return -ENOMEM;
@@ -176,8 +179,9 @@ static struct class firmware_class = {
 static ssize_t firmware_loading_show(struct device *dev,
 				     struct device_attribute *attr, char *buf)
 {
-	struct firmware_priv *fw_priv = dev_get_drvdata(dev);
+	struct firmware_priv *fw_priv = to_firmware_priv(dev);
 	int loading = test_bit(FW_STATUS_LOADING, &fw_priv->status);
+
 	return sprintf(buf, "%d\n", loading);
 }
 
@@ -202,7 +206,7 @@ static ssize_t firmware_loading_store(struct device *dev,
 				      struct device_attribute *attr,
 				      const char *buf, size_t count)
 {
-	struct firmware_priv *fw_priv = dev_get_drvdata(dev);
+	struct firmware_priv *fw_priv = to_firmware_priv(dev);
 	int loading = simple_strtol(buf, NULL, 10);
 	int i;
 
@@ -257,12 +261,12 @@ static ssize_t firmware_loading_store(struct device *dev,
 
 static DEVICE_ATTR(loading, 0644, firmware_loading_show, firmware_loading_store);
 
-static ssize_t
-firmware_data_read(struct kobject *kobj, struct bin_attribute *bin_attr,
-		   char *buffer, loff_t offset, size_t count)
+static ssize_t firmware_data_read(struct kobject *kobj,
+				  struct bin_attribute *bin_attr,
+				  char *buffer, loff_t offset, size_t count)
 {
 	struct device *dev = to_dev(kobj);
-	struct firmware_priv *fw_priv = dev_get_drvdata(dev);
+	struct firmware_priv *fw_priv = to_firmware_priv(dev);
 	struct firmware *fw;
 	ssize_t ret_count;
 
@@ -301,8 +305,7 @@ out:
 	return ret_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)
 {
 	int pages_needed = ALIGN(min_size, PAGE_SIZE) >> PAGE_SHIFT;
 
@@ -351,12 +354,12 @@ fw_realloc_buffer(struct firmware_priv *fw_priv, int min_size)
  *	Data written to the 'data' attribute will be later handed to
  *	the driver as a firmware image.
  **/
-static ssize_t
-firmware_data_write(struct kobject *kobj, struct bin_attribute *bin_attr,
-		    char *buffer, loff_t offset, size_t count)
+static ssize_t firmware_data_write(struct kobject *kobj,
+				   struct bin_attribute *bin_attr,
+				   char *buffer, loff_t offset, size_t count)
 {
 	struct device *dev = to_dev(kobj);
-	struct firmware_priv *fw_priv = dev_get_drvdata(dev);
+	struct firmware_priv *fw_priv = to_firmware_priv(dev);
 	struct firmware *fw;
 	ssize_t retval;
 
@@ -404,106 +407,94 @@ static struct bin_attribute firmware_attr_data = {
 	.write = firmware_data_write,
 };
 
-static void
-firmware_class_timeout(u_long data)
+static void firmware_class_timeout(u_long data)
 {
 	struct firmware_priv *fw_priv = (struct firmware_priv *) data;
+
 	fw_load_abort(fw_priv);
 }
 
-static int fw_register_device(struct device **dev_p, const char *fw_name,
-			      struct device *device)
+static struct firmware_priv *
+fw_create_instance(struct firmware *firmware, const char *fw_name,
+		   struct device *device, bool uevent)
 {
-	int retval;
-	struct firmware_priv *fw_priv =
-		kzalloc(sizeof(*fw_priv) + strlen(fw_name) + 1 , GFP_KERNEL);
-	struct device *f_dev = kzalloc(sizeof(*f_dev), GFP_KERNEL);
-
-	*dev_p = NULL;
+	struct firmware_priv *fw_priv;
+	struct device *f_dev;
+	int error;
 
-	if (!fw_priv || !f_dev) {
+	fw_priv = kzalloc(sizeof(*fw_priv) + strlen(fw_name) + 1 , GFP_KERNEL);
+	if (!fw_priv) {
 		dev_err(device, "%s: kmalloc failed\n", __func__);
-		retval = -ENOMEM;
-		goto error_kfree;
+		error = -ENOMEM;
+		goto err_out;
 	}
 
+	fw_priv->fw = firmware;
 	strcpy(fw_priv->fw_id, fw_name);
 	init_completion(&fw_priv->completion);
+	setup_timer(&fw_priv->timeout,
+		    firmware_class_timeout, (u_long) fw_priv);
 
-	fw_priv->timeout.function = firmware_class_timeout;
-	fw_priv->timeout.data = (u_long) fw_priv;
-	init_timer(&fw_priv->timeout);
+	f_dev = &fw_priv->dev;
 
+	device_initialize(f_dev);
 	dev_set_name(f_dev, "%s", dev_name(device));
 	f_dev->parent = device;
 	f_dev->class = &firmware_class;
-	dev_set_drvdata(f_dev, fw_priv);
-	dev_set_uevent_suppress(f_dev, 1);
-	retval = device_register(f_dev);
-	if (retval) {
-		dev_err(device, "%s: device_register failed\n", __func__);
-		put_device(f_dev);
-		return retval;
-	}
-	*dev_p = f_dev;
-	return 0;
-
-error_kfree:
-	kfree(f_dev);
-	kfree(fw_priv);
-	return retval;
-}
-
-static int fw_setup_device(struct firmware *fw, struct device **dev_p,
-			   const char *fw_name, struct device *device,
-			   int uevent)
-{
-	struct device *f_dev;
-	struct firmware_priv *fw_priv;
-	int retval;
-
-	*dev_p = NULL;
-	retval = fw_register_device(&f_dev, fw_name, device);
-	if (retval)
-		goto out;
+	dev_set_uevent_suppress(f_dev, true);
 
 	/* Need to pin this module until class device is destroyed */
 	__module_get(THIS_MODULE);
 
-	fw_priv = dev_get_drvdata(f_dev);
+	error = device_add(f_dev);
+	if (error) {
+		dev_err(device, "%s: device_register failed\n", __func__);
+		goto err_put_dev;
+	}
 
-	fw_priv->fw = fw;
-	retval = sysfs_create_bin_file(&f_dev->kobj, &firmware_attr_data);
-	if (retval) {
+	error = device_create_bin_file(f_dev, &firmware_attr_data);
+	if (error) {
 		dev_err(device, "%s: sysfs_create_bin_file failed\n", __func__);
-		goto error_unreg;
+		goto err_del_dev;
 	}
 
-	retval = device_create_file(f_dev, &dev_attr_loading);
-	if (retval) {
+	error = device_create_file(f_dev, &dev_attr_loading);
+	if (error) {
 		dev_err(device, "%s: device_create_file failed\n", __func__);
-		goto error_unreg;
+		goto err_del_bin_attr;
 	}
 
 	if (uevent)
-		dev_set_uevent_suppress(f_dev, 0);
-	*dev_p = f_dev;
-	goto out;
+		dev_set_uevent_suppress(f_dev, false);
+
+	return fw_priv;
+
+err_del_bin_attr:
+	device_remove_bin_file(f_dev, &firmware_attr_data);
+err_del_dev:
+	device_del(f_dev);
+err_put_dev:
+	put_device(f_dev);
+err_out:
+	return ERR_PTR(error);
+}
 
-error_unreg:
+static void fw_destroy_instance(struct firmware_priv *fw_priv)
+{
+	struct device *f_dev = &fw_priv->dev;
+
+	device_remove_file(f_dev, &dev_attr_loading);
+	device_remove_bin_file(f_dev, &firmware_attr_data);
 	device_unregister(f_dev);
-out:
-	return retval;
 }
 
-static int
-_request_firmware(const struct firmware **firmware_p, const char *name,
-		 struct device *device, int uevent)
+static int _request_firmware(const struct firmware **firmware_p,
+			     const char *name, struct device *device,
+			     bool uevent)
 {
-	struct device *f_dev;
 	struct firmware_priv *fw_priv;
 	struct firmware *firmware;
-	int retval;
+	int retval = 0;
 
 	if (!firmware_p)
 		return -EINVAL;
@@ -519,46 +510,46 @@ _request_firmware(const struct firmware **firmware_p, const char *name,
 	if (fw_get_builtin_firmware(firmware, name)) {
 		dev_info(device,
 			 "firmware: using built-in firmware %s\n", name);
-		return 0;
+		goto out;
 	}
 
 	if (uevent)
 		dev_info(device, "firmware: requesting %s\n", name);
 
-	retval = fw_setup_device(firmware, &f_dev, name, device, uevent);
-	if (retval)
-		goto error_kfree_fw;
-
-	fw_priv = dev_get_drvdata(f_dev);
+	fw_priv = fw_create_instance(firmware, name, device, uevent);
+	if (IS_ERR(fw_priv)) {
+		retval = PTR_ERR(fw_priv);
+		goto out;
+	}
 
 	if (uevent) {
-		if (loading_timeout > 0) {
-			fw_priv->timeout.expires = jiffies + loading_timeout * HZ;
-			add_timer(&fw_priv->timeout);
-		}
+		if (loading_timeout > 0)
+			mod_timer(&fw_priv->timeout,
+				  round_jiffies_up(jiffies +
+						   loading_timeout * HZ));
+
+		kobject_uevent(&fw_priv->dev.kobj, KOBJ_ADD);
+	}
+
+	wait_for_completion(&fw_priv->completion);
 
-		kobject_uevent(&f_dev->kobj, KOBJ_ADD);
-		wait_for_completion(&fw_priv->completion);
-		set_bit(FW_STATUS_DONE, &fw_priv->status);
-		del_timer_sync(&fw_priv->timeout);
-	} else
-		wait_for_completion(&fw_priv->completion);
+	set_bit(FW_STATUS_DONE, &fw_priv->status);
+	del_timer_sync(&fw_priv->timeout);
 
 	mutex_lock(&fw_lock);
-	if (!fw_priv->fw->size || test_bit(FW_STATUS_ABORT, &fw_priv->status)) {
+	if (!fw_priv->fw->size || test_bit(FW_STATUS_ABORT, &fw_priv->status))
 		retval = -ENOENT;
-		release_firmware(fw_priv->fw);
-		*firmware_p = NULL;
-	}
 	fw_priv->fw = NULL;
 	mutex_unlock(&fw_lock);
-	device_unregister(f_dev);
-	goto out;
 
-error_kfree_fw:
-	kfree(firmware);
-	*firmware_p = NULL;
+	fw_destroy_instance(fw_priv);
+
 out:
+	if (retval) {
+		release_firmware(firmware);
+		firmware_p = NULL;
+	}
+
 	return retval;
 }
 
@@ -610,23 +601,24 @@ struct firmware_work {
 	int uevent;
 };
 
-static int
-request_firmware_work_func(void *arg)
+static int request_firmware_work_func(void *arg)
 {
 	struct firmware_work *fw_work = arg;
 	const struct firmware *fw;
 	int ret;
+
 	if (!arg) {
 		WARN_ON(1);
 		return 0;
 	}
-	ret = _request_firmware(&fw, fw_work->name, fw_work->device,
-		fw_work->uevent);
 
+	ret = _request_firmware(&fw, fw_work->name, fw_work->device,
+				fw_work->uevent);
 	fw_work->cont(fw, fw_work->context);
 
 	module_put(fw_work->module);
 	kfree(fw_work);
+
 	return ret;
 }
 
@@ -654,34 +646,33 @@ request_firmware_nowait(
 	void (*cont)(const struct firmware *fw, void *context))
 {
 	struct task_struct *task;
-	struct firmware_work *fw_work = kmalloc(sizeof (struct firmware_work),
-						gfp);
+	struct firmware_work *fw_work;
 
+	fw_work = kzalloc(sizeof (struct firmware_work), gfp);
 	if (!fw_work)
 		return -ENOMEM;
+
+	fw_work->module = module;
+	fw_work->name = name;
+	fw_work->device = device;
+	fw_work->context = context;
+	fw_work->cont = cont;
+	fw_work->uevent = uevent;
+
 	if (!try_module_get(module)) {
 		kfree(fw_work);
 		return -EFAULT;
 	}
 
-	*fw_work = (struct firmware_work) {
-		.module = module,
-		.name = name,
-		.device = device,
-		.context = context,
-		.cont = cont,
-		.uevent = uevent,
-	};
-
 	task = kthread_run(request_firmware_work_func, fw_work,
 			    "firmware/%s", name);
-
 	if (IS_ERR(task)) {
 		fw_work->cont(NULL, fw_work->context);
 		module_put(fw_work->module);
 		kfree(fw_work);
 		return PTR_ERR(task);
 	}
+
 	return 0;
 }
 

Re: [PATCH 5/5] firmware loader: embed device into firmware_priv structure

[Greg KH]

On Sat, Mar 13, 2010 at 11:49:29PM -0800, Dmitry Torokhov wrote:
> Both these structures have the same lifetime rules so instead of allocating
> and managing them separately embed struct device into struct firmware_priv.
> Also make sure to delete sysfs attributes ourselves instead of expecting
> sysfs to clean up our mess.

This one didn't apply due to other changes that have happened to the
file since you sent this out.  Care to respin it?

thanks,

greg k-h

Re: [PATCH 5/5] firmware loader: embed device into firmware_priv structure

[Dmitry Torokhov]

On Thu, Apr 22, 2010 at 05:01:06PM -0700, Greg KH wrote:
> On Sat, Mar 13, 2010 at 11:49:29PM -0800, Dmitry Torokhov wrote:
> > Both these structures have the same lifetime rules so instead of allocating
> > and managing them separately embed struct device into struct firmware_priv.
> > Also make sure to delete sysfs attributes ourselves instead of expecting
> > sysfs to clean up our mess.
> 
> This one didn't apply due to other changes that have happened to the
> file since you sent this out.  Care to respin it?
> 

I'll wait till next next release, see what's there and resping what's
missing.

Thanks.

-- 
Dmitry

Re: [PATCH 5/5] firmware loader: embed device into firmware_priv structure

[Greg KH]

On Thu, Apr 22, 2010 at 05:19:03PM -0700, Dmitry Torokhov wrote:
> On Thu, Apr 22, 2010 at 05:01:06PM -0700, Greg KH wrote:
> > On Sat, Mar 13, 2010 at 11:49:29PM -0800, Dmitry Torokhov wrote:
> > > Both these structures have the same lifetime rules so instead of allocating
> > > and managing them separately embed struct device into struct firmware_priv.
> > > Also make sure to delete sysfs attributes ourselves instead of expecting
> > > sysfs to clean up our mess.
> > 
> > This one didn't apply due to other changes that have happened to the
> > file since you sent this out.  Care to respin it?
> > 
> 
> I'll wait till next next release, see what's there and resping what's
> missing.

Sounds great.

thanks,

greg k-h