[PATCH] request_firmware(): fixes and polishing.
From: Manuel Estrada Sainz
Date: Tue Jan 06 2004 - 20:26:18 EST
Hi,
Dmitry Torokhov has been criticising my code for some days (Thanks
Dmitry), and here is the result. Please test and keep criticising :-)
Each patch starts with a changelog, but I'll write a full changelog
here for the casuall reader:
- 01_request_firmware-misc-fix.diff
- use vfree to free vmalloc memory.
- Make sure fw_setup_class_device sets *class_dev_p to NULL in
all case of error.
- Fix error handling in firmware_class_init.
- 02_request_firmware-misc-polish.diff
- Take advantage of strlcpy.
- Extra error logging.
- Use struct coping instead of memcpy.
- Put all aborting code in a single place, and fully abort if
fw_realloc_buffer fails.
- Abort on unexpected 'loading' values.
- 03_request_firmware-state-tracking.diff
- Make an status bitmap instead of using independent boolean
variables. It will make things nicer later when new issues
need to be tracked.
- 04_request_firmware-propper-release-1.diff
- release 'struct firmware_priv' from class_dev->release.
- 05_request_firmware-propper-release-2.diff
- Remove races related to the handling and release of 'struct
firmware'
- 06_request_firmware-rearrange.diff
- Refactor fw_setup_class_device for readability and
maintainavility.
- 07_request_firmware-dont-remove-attr.diff
- Don't remove attributes, they should be gone automatically.
Have a nice day
Manuel
--
--- Manuel Estrada Sainz <ranty@xxxxxxxxxx>
<ranty@xxxxxxxxxxx>
<ranty@xxxxxxxxxxxxxxxxxxxxx>
------------------------ <manuel.estrada@xxxxxxxxxxxxx> -------------------
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.
Based on patch and suggestions from Dmitry Torokhov
Changelog:
- use vfree to free vmalloc memory.
- Make sure fw_setup_class_device sets *class_dev_p to NULL in all case
of error.
- Fix error handling in firmware_class_init.
Index: linux-2.5/drivers/base/firmware_class.c
===================================================================
--- linux-2.5.orig/drivers/base/firmware_class.c 2004-01-02 20:42:03.000000000 +0100
+++ linux-2.5/drivers/base/firmware_class.c 2004-01-02 21:43:17.000000000 +0100
@@ -119,7 +119,7 @@
complete(&fw_priv->completion);
break;
case 1:
- kfree(fw_priv->fw->data);
+ vfree(fw_priv->fw->data);
fw_priv->fw->data = NULL;
fw_priv->fw->size = 0;
fw_priv->alloc_size = 0;
@@ -297,6 +297,7 @@
}
memset(fw_priv->fw, 0, sizeof (*fw_priv->fw));
+ *class_dev_p = class_dev;
goto out;
error_remove_loading:
@@ -310,7 +311,6 @@
kfree(class_dev);
*class_dev_p = NULL;
out:
- *class_dev_p = class_dev;
return retval;
}
static void
@@ -489,6 +489,7 @@
error = class_register(&firmware_class);
if (error) {
printk(KERN_ERR "%s: class_register failed\n", __FUNCTION__);
+ return error;
}
error = class_create_file(&firmware_class, &class_attr_timeout);
if (error) {
Based on patch and suggestions from Dmitry Torokhov
Changelog:
- Take advantage of strlcpy.
- Extra error logging.
- Use struct coping instead of memcpy.
- Put all aborting code in a single place, and fully abort if
fw_realloc_buffer fails.
- Abort on unexpected 'loading' values.
Index: linux-2.5/drivers/base/firmware_class.c
===================================================================
--- linux-2.5.orig/drivers/base/firmware_class.c 2004-01-06 03:23:14.000000000 +0100
+++ linux-2.5/drivers/base/firmware_class.c 2004-01-06 03:53:30.000000000 +0100
@@ -34,6 +34,14 @@
struct timer_list timeout;
};
+static inline void
+fw_load_abort(struct firmware_priv *fw_priv)
+{
+ fw_priv->abort = 1;
+ wmb();
+ complete(&fw_priv->completion);
+}
+
static ssize_t
firmware_timeout_show(struct class *class, char *buf)
{
@@ -113,11 +121,6 @@
fw_priv->loading = simple_strtol(buf, NULL, 10);
switch (fw_priv->loading) {
- case -1:
- fw_priv->abort = 1;
- wmb();
- complete(&fw_priv->completion);
- break;
case 1:
vfree(fw_priv->fw->data);
fw_priv->fw->data = NULL;
@@ -125,8 +128,17 @@
fw_priv->alloc_size = 0;
break;
case 0:
- if (prev_loading == 1)
+ if (prev_loading == 1) {
complete(&fw_priv->completion);
+ break;
+ }
+ /* fallthrough */
+ default:
+ printk(KERN_ERR "%s: unexpected value (%d)\n", __FUNCTION__,
+ fw_priv->loading);
+ /* fallthrough */
+ case -1:
+ fw_load_abort(fw_priv);
break;
}
@@ -164,7 +176,7 @@
if (!new_data) {
printk(KERN_ERR "%s: unable to alloc buffer\n", __FUNCTION__);
/* Make sure that we don't keep incomplete data */
- fw_priv->abort = 1;
+ fw_load_abort(fw_priv);
return -ENOMEM;
}
fw_priv->alloc_size += PAGE_SIZE;
@@ -221,17 +233,14 @@
firmware_class_timeout(u_long data)
{
struct firmware_priv *fw_priv = (struct firmware_priv *) data;
- fw_priv->abort = 1;
- wmb();
- complete(&fw_priv->completion);
+ fw_load_abort(fw_priv);
}
static inline void
fw_setup_class_device_id(struct class_device *class_dev, struct device *dev)
{
/* XXX warning we should watch out for name collisions */
- strncpy(class_dev->class_id, dev->bus_id, BUS_ID_SIZE);
- class_dev->class_id[BUS_ID_SIZE - 1] = '\0';
+ strlcpy(class_dev->class_id, dev->bus_id, BUS_ID_SIZE);
}
static int
fw_setup_class_device(struct class_device **class_dev_p,
@@ -244,6 +253,7 @@
GFP_KERNEL);
if (!fw_priv || !class_dev) {
+ printk(KERN_ERR "%s: kmalloc failed\n", __FUNCTION__);
retval = -ENOMEM;
goto error_kfree;
}
@@ -251,12 +261,8 @@
memset(class_dev, 0, sizeof (*class_dev));
init_completion(&fw_priv->completion);
- memcpy(&fw_priv->attr_data, &firmware_attr_data_tmpl,
- sizeof (firmware_attr_data_tmpl));
-
- strncpy(&fw_priv->fw_id[0], fw_name, FIRMWARE_NAME_MAX);
- fw_priv->fw_id[FIRMWARE_NAME_MAX - 1] = '\0';
-
+ fw_priv->attr_data = firmware_attr_data_tmpl;
+ strlcpy(fw_priv->fw_id, fw_name, FIRMWARE_NAME_MAX);
fw_setup_class_device_id(class_dev, device);
class_dev->dev = device;
Based on patch and suggestions from Dmitry Torokhov
Changelog:
- Make an status bitmap instead of using independent boolean variables.
It will make things nicer later when new issues need to be tracked.
Index: linux-2.5/drivers/base/firmware_class.c
===================================================================
--- linux-2.5.orig/drivers/base/firmware_class.c 2004-01-06 14:29:01.000000000 +0100
+++ linux-2.5/drivers/base/firmware_class.c 2004-01-06 21:29:09.000000000 +0100
@@ -13,6 +13,7 @@
#include <linux/timer.h>
#include <linux/vmalloc.h>
#include <asm/hardirq.h>
+#include <linux/bitops.h>
#include <linux/firmware.h>
#include "base.h"
@@ -21,6 +22,11 @@
MODULE_DESCRIPTION("Multi purpose firmware loading support");
MODULE_LICENSE("GPL");
+enum {
+ FW_STATUS_LOADING,
+ FW_STATUS_ABORT,
+};
+
static int loading_timeout = 10; /* In seconds */
struct firmware_priv {
@@ -28,8 +34,7 @@
struct completion completion;
struct bin_attribute attr_data;
struct firmware *fw;
- int loading;
- int abort;
+ unsigned long status;
int alloc_size;
struct timer_list timeout;
};
@@ -37,7 +42,7 @@
static inline void
fw_load_abort(struct firmware_priv *fw_priv)
{
- fw_priv->abort = 1;
+ set_bit(FW_STATUS_ABORT, &fw_priv->status);
wmb();
complete(&fw_priv->completion);
}
@@ -99,7 +104,8 @@
firmware_loading_show(struct class_device *class_dev, char *buf)
{
struct firmware_priv *fw_priv = class_get_devdata(class_dev);
- return sprintf(buf, "%d\n", fw_priv->loading);
+ int loading = test_bit(FW_STATUS_LOADING, &fw_priv->status);
+ return sprintf(buf, "%d\n", loading);
}
/**
@@ -116,26 +122,26 @@
const char *buf, size_t count)
{
struct firmware_priv *fw_priv = class_get_devdata(class_dev);
- int prev_loading = fw_priv->loading;
-
- fw_priv->loading = simple_strtol(buf, NULL, 10);
+ int loading = simple_strtol(buf, NULL, 10);
- switch (fw_priv->loading) {
+ switch (loading) {
case 1:
vfree(fw_priv->fw->data);
fw_priv->fw->data = NULL;
fw_priv->fw->size = 0;
fw_priv->alloc_size = 0;
+ set_bit(FW_STATUS_LOADING, &fw_priv->status);
break;
case 0:
- if (prev_loading == 1) {
+ if (test_bit(FW_STATUS_LOADING, &fw_priv->status)) {
complete(&fw_priv->completion);
+ clear_bit(FW_STATUS_LOADING, &fw_priv->status);
break;
}
/* fallthrough */
default:
printk(KERN_ERR "%s: unexpected value (%d)\n", __FUNCTION__,
- fw_priv->loading);
+ loading);
/* fallthrough */
case -1:
fw_load_abort(fw_priv);
@@ -370,7 +376,7 @@
del_timer(&fw_priv->timeout);
fw_remove_class_device(class_dev);
- if (fw_priv->fw->size && !fw_priv->abort) {
+ if (fw_priv->fw->size && !test_bit(FW_STATUS_ABORT, &fw_priv->status)) {
*firmware = fw_priv->fw;
} else {
retval = -ENOENT;
Based on patch and suggestions from Dmitry Torokhov
Changelog:
- release 'struct firmware_priv' from class_dev->release.
Index: linux-2.5/drivers/base/firmware_class.c
===================================================================
--- linux-2.5.orig/drivers/base/firmware_class.c 2004-01-06 13:30:48.000000000 +0100
+++ linux-2.5/drivers/base/firmware_class.c 2004-01-06 13:31:38.000000000 +0100
@@ -232,6 +232,9 @@
static void
fw_class_dev_release(struct class_device *class_dev)
{
+ struct firmware_priv *fw_priv = class_get_devdata(class_dev);
+
+ kfree(fw_priv);
kfree(class_dev);
}
@@ -258,6 +261,8 @@
struct class_device *class_dev = kmalloc(sizeof (struct class_device),
GFP_KERNEL);
+ *class_dev_p = NULL;
+
if (!fw_priv || !class_dev) {
printk(KERN_ERR "%s: kmalloc failed\n", __FUNCTION__);
retval = -ENOMEM;
@@ -318,10 +323,11 @@
sysfs_remove_bin_file(&class_dev->kobj, &fw_priv->attr_data);
error_unreg_class_dev:
class_device_unregister(class_dev);
+ goto out;
+
error_kfree:
kfree(fw_priv);
kfree(class_dev);
- *class_dev_p = NULL;
out:
return retval;
}
@@ -374,7 +380,6 @@
wait_for_completion(&fw_priv->completion);
del_timer(&fw_priv->timeout);
- fw_remove_class_device(class_dev);
if (fw_priv->fw->size && !test_bit(FW_STATUS_ABORT, &fw_priv->status)) {
*firmware = fw_priv->fw;
@@ -383,7 +388,7 @@
vfree(fw_priv->fw->data);
kfree(fw_priv->fw);
}
- kfree(fw_priv);
+ fw_remove_class_device(class_dev);
out:
return retval;
}
Changelog:
- Remove races related to the handling and release of 'struct firmware'
Index: linux-2.5/drivers/base/firmware_class.c
===================================================================
--- linux-2.5.orig/drivers/base/firmware_class.c 2004-01-06 13:41:22.000000000 +0100
+++ linux-2.5/drivers/base/firmware_class.c 2004-01-06 13:43:58.000000000 +0100
@@ -14,6 +14,7 @@
#include <linux/vmalloc.h>
#include <asm/hardirq.h>
#include <linux/bitops.h>
+#include <asm/semaphore.h>
#include <linux/firmware.h>
#include "base.h"
@@ -24,11 +25,16 @@
enum {
FW_STATUS_LOADING,
+ FW_STATUS_DONE,
FW_STATUS_ABORT,
};
static int loading_timeout = 10; /* In seconds */
+/* fw_lock could be moved to 'struct firmware_priv' but since it is just
+ * guarding for corner cases a global lock should be OK */
+static DECLARE_MUTEX(fw_lock);
+
struct firmware_priv {
char fw_id[FIRMWARE_NAME_MAX];
struct completion completion;
@@ -126,11 +132,13 @@
switch (loading) {
case 1:
+ down(&fw_lock);
vfree(fw_priv->fw->data);
fw_priv->fw->data = NULL;
fw_priv->fw->size = 0;
fw_priv->alloc_size = 0;
set_bit(FW_STATUS_LOADING, &fw_priv->status);
+ up(&fw_lock);
break;
case 0:
if (test_bit(FW_STATUS_LOADING, &fw_priv->status)) {
@@ -160,15 +168,26 @@
{
struct class_device *class_dev = to_class_dev(kobj);
struct firmware_priv *fw_priv = class_get_devdata(class_dev);
- struct firmware *fw = fw_priv->fw;
+ struct firmware *fw;
+ ssize_t ret_count = count;
- if (offset > fw->size)
- return 0;
- if (offset + count > fw->size)
- count = fw->size - offset;
+ down(&fw_lock);
+ fw = fw_priv->fw;
+ if (test_bit(FW_STATUS_DONE, &fw_priv->status)) {
+ ret_count = -ENODEV;
+ goto out;
+ }
+ if (offset > fw->size) {
+ ret_count = 0;
+ goto out;
+ }
+ if (offset + ret_count > fw->size)
+ ret_count = fw->size - offset;
- memcpy(buffer, fw->data + offset, count);
- return count;
+ memcpy(buffer, fw->data + offset, ret_count);
+out:
+ up(&fw_lock);
+ return ret_count;
}
static int
fw_realloc_buffer(struct firmware_priv *fw_priv, int min_size)
@@ -209,18 +228,26 @@
{
struct class_device *class_dev = to_class_dev(kobj);
struct firmware_priv *fw_priv = class_get_devdata(class_dev);
- struct firmware *fw = fw_priv->fw;
- int retval;
+ struct firmware *fw;
+ ssize_t retval;
+ down(&fw_lock);
+ fw = fw_priv->fw;
+ if (test_bit(FW_STATUS_DONE, &fw_priv->status)) {
+ retval = -ENODEV;
+ goto out;
+ }
retval = fw_realloc_buffer(fw_priv, offset + count);
if (retval)
- return retval;
+ goto out;
memcpy(fw->data + offset, buffer, count);
fw->size = max_t(size_t, offset + count, fw->size);
-
- return count;
+ retval = count;
+out:
+ up(&fw_lock);
+ return retval;
}
static struct bin_attribute firmware_attr_data_tmpl = {
.attr = {.name = "data", .mode = 0644},
@@ -252,7 +279,7 @@
strlcpy(class_dev->class_id, dev->bus_id, BUS_ID_SIZE);
}
static int
-fw_setup_class_device(struct class_device **class_dev_p,
+fw_setup_class_device(struct firmware *fw, struct class_device **class_dev_p,
const char *fw_name, struct device *device)
{
int retval = 0;
@@ -290,6 +317,8 @@
goto error_kfree;
}
+ fw_priv->fw = fw;
+
retval = sysfs_create_bin_file(&class_dev->kobj, &fw_priv->attr_data);
if (retval) {
printk(KERN_ERR "%s: sysfs_create_bin_file failed\n",
@@ -305,20 +334,9 @@
goto error_remove_data;
}
- fw_priv->fw = kmalloc(sizeof (struct firmware), GFP_KERNEL);
- if (!fw_priv->fw) {
- printk(KERN_ERR "%s: kmalloc(struct firmware) failed\n",
- __FUNCTION__);
- retval = -ENOMEM;
- goto error_remove_loading;
- }
- memset(fw_priv->fw, 0, sizeof (*fw_priv->fw));
-
*class_dev_p = class_dev;
goto out;
-error_remove_loading:
- class_device_remove_file(class_dev, &class_device_attr_loading);
error_remove_data:
sysfs_remove_bin_file(&class_dev->kobj, &fw_priv->attr_data);
error_unreg_class_dev:
@@ -354,21 +372,29 @@
* firmware image for this or any other device.
**/
int
-request_firmware(const struct firmware **firmware, const char *name,
+request_firmware(const struct firmware **firmware_p, const char *name,
struct device *device)
{
struct class_device *class_dev;
struct firmware_priv *fw_priv;
+ struct firmware *firmware;
int retval;
- if (!firmware)
+ if (!firmware_p)
return -EINVAL;
- *firmware = NULL;
+ *firmware_p = firmware = kmalloc(sizeof (struct firmware), GFP_KERNEL);
+ if (!firmware) {
+ printk(KERN_ERR "%s: kmalloc(struct firmware) failed\n",
+ __FUNCTION__);
+ retval = -ENOMEM;
+ goto out;
+ }
+ memset(firmware, 0, sizeof (*firmware));
- retval = fw_setup_class_device(&class_dev, name, device);
+ retval = fw_setup_class_device(firmware, &class_dev, name, device);
if (retval)
- goto out;
+ goto error_kfree_fw;
fw_priv = class_get_devdata(class_dev);
@@ -378,17 +404,23 @@
}
wait_for_completion(&fw_priv->completion);
+ set_bit(FW_STATUS_DONE, &fw_priv->status);
del_timer(&fw_priv->timeout);
- if (fw_priv->fw->size && !test_bit(FW_STATUS_ABORT, &fw_priv->status)) {
- *firmware = fw_priv->fw;
- } else {
+ down(&fw_lock);
+ if (!fw_priv->fw->size || test_bit(FW_STATUS_ABORT, &fw_priv->status)) {
retval = -ENOENT;
- vfree(fw_priv->fw->data);
- kfree(fw_priv->fw);
+ release_firmware(fw_priv->fw);
+ *firmware_p = NULL;
}
+ fw_priv->fw = NULL;
+ up(&fw_lock);
fw_remove_class_device(class_dev);
+ goto out;
+
+error_kfree_fw:
+ kfree(firmware);
out:
return retval;
}
Based on patch and suggestions from Dmitry Torokhov
Changelog:
- Refactor fw_setup_class_device for readability and maintainavility.
Index: linux-2.5/drivers/base/firmware_class.c
===================================================================
--- linux-2.5.orig/drivers/base/firmware_class.c 2004-01-06 13:40:49.000000000 +0100
+++ linux-2.5/drivers/base/firmware_class.c 2004-01-06 13:40:58.000000000 +0100
@@ -278,11 +278,12 @@
/* XXX warning we should watch out for name collisions */
strlcpy(class_dev->class_id, dev->bus_id, BUS_ID_SIZE);
}
+
static int
-fw_setup_class_device(struct firmware *fw, struct class_device **class_dev_p,
- const char *fw_name, struct device *device)
+fw_register_class_device(struct class_device **class_dev_p,
+ const char *fw_name, struct device *device)
{
- int retval = 0;
+ int retval;
struct firmware_priv *fw_priv = kmalloc(sizeof (struct firmware_priv),
GFP_KERNEL);
struct class_device *class_dev = kmalloc(sizeof (struct class_device),
@@ -301,13 +302,13 @@
init_completion(&fw_priv->completion);
fw_priv->attr_data = firmware_attr_data_tmpl;
strlcpy(fw_priv->fw_id, fw_name, FIRMWARE_NAME_MAX);
- fw_setup_class_device_id(class_dev, device);
- class_dev->dev = device;
fw_priv->timeout.function = firmware_class_timeout;
fw_priv->timeout.data = (u_long) fw_priv;
init_timer(&fw_priv->timeout);
+ fw_setup_class_device_id(class_dev, device);
+ class_dev->dev = device;
class_dev->class = &firmware_class;
class_set_devdata(class_dev, fw_priv);
retval = class_device_register(class_dev);
@@ -316,7 +317,28 @@
__FUNCTION__);
goto error_kfree;
}
+ *class_dev_p = class_dev;
+ return 0;
+error_kfree:
+ kfree(fw_priv);
+ kfree(class_dev);
+ return retval;
+}
+static int
+fw_setup_class_device(struct firmware *fw, struct class_device **class_dev_p,
+ const char *fw_name, struct device *device)
+{
+ struct class_device *class_dev;
+ struct firmware_priv *fw_priv;
+ int retval;
+
+ *class_dev_p = NULL;
+ retval = fw_register_class_device(&class_dev, fw_name, device);
+ if (retval)
+ goto out;
+
+ fw_priv = class_get_devdata(class_dev);
fw_priv->fw = fw;
retval = sysfs_create_bin_file(&class_dev->kobj, &fw_priv->attr_data);
@@ -341,11 +363,6 @@
sysfs_remove_bin_file(&class_dev->kobj, &fw_priv->attr_data);
error_unreg_class_dev:
class_device_unregister(class_dev);
- goto out;
-
-error_kfree:
- kfree(fw_priv);
- kfree(class_dev);
out:
return retval;
}
Based on patch and suggestions from Dmitry Torokhov
Changelog:
- Don't remove attributes, they should be gone automatically.
Index: linux-2.5/drivers/base/firmware_class.c
===================================================================
--- linux-2.5.orig/drivers/base/firmware_class.c 2004-01-06 13:40:58.000000000 +0100
+++ linux-2.5/drivers/base/firmware_class.c 2004-01-06 13:41:07.000000000 +0100
@@ -339,13 +339,13 @@
goto out;
fw_priv = class_get_devdata(class_dev);
- fw_priv->fw = fw;
+ fw_priv->fw = fw;
retval = sysfs_create_bin_file(&class_dev->kobj, &fw_priv->attr_data);
if (retval) {
printk(KERN_ERR "%s: sysfs_create_bin_file failed\n",
__FUNCTION__);
- goto error_unreg_class_dev;
+ goto error_unreg;
}
retval = class_device_create_file(class_dev,
@@ -353,28 +353,17 @@
if (retval) {
printk(KERN_ERR "%s: class_device_create_file failed\n",
__FUNCTION__);
- goto error_remove_data;
+ goto error_unreg;
}
*class_dev_p = class_dev;
goto out;
-error_remove_data:
- sysfs_remove_bin_file(&class_dev->kobj, &fw_priv->attr_data);
-error_unreg_class_dev:
+error_unreg:
class_device_unregister(class_dev);
out:
return retval;
}
-static void
-fw_remove_class_device(struct class_device *class_dev)
-{
- struct firmware_priv *fw_priv = class_get_devdata(class_dev);
-
- class_device_remove_file(class_dev, &class_device_attr_loading);
- sysfs_remove_bin_file(&class_dev->kobj, &fw_priv->attr_data);
- class_device_unregister(class_dev);
-}
/**
* request_firmware: - request firmware to hotplug and wait for it
@@ -433,7 +422,7 @@
}
fw_priv->fw = NULL;
up(&fw_lock);
- fw_remove_class_device(class_dev);
+ class_device_unregister(class_dev);
goto out;
error_kfree_fw:
@@ -569,7 +558,6 @@
static void __exit
firmware_class_exit(void)
{
- class_remove_file(&firmware_class, &class_attr_timeout);
class_unregister(&firmware_class);
}