* [lm-sensors] [patch] hwmon/w83791d - fix unchecked errs
@ 2006-09-25 2:27 Jim Cromie
2006-09-25 9:48 ` Charles Spirakis
` (3 more replies)
0 siblings, 4 replies; 5+ messages in thread
From: Jim Cromie @ 2006-09-25 2:27 UTC (permalink / raw)
To: lm-sensors
replace all unchecked calls to device_create_file with a single group
declaration,
and one call to sysfs_create_group, and check that one return status.
Signed-off-by: Jim Cromie <jim.cromie at gmail.com>
---
compile tested only. Charles, can you test it for me ? TIA ;-)
$ diffstat pc-set/hwmon-unchecked-return-status-fixes-w83791d.patch
w83791d.c | 84
++++++++++++++++++++++++++++++++++++++++----------------------
1 files changed, 55 insertions(+), 29 deletions(-)
diff -ruNp -X dontdiff -X exclude-diffs 6rlocks-0/drivers/hwmon/w83791d.c w83791d/drivers/hwmon/w83791d.c
--- 6rlocks-0/drivers/hwmon/w83791d.c 2006-09-19 23:58:37.000000000 -0600
+++ w83791d/drivers/hwmon/w83791d.c 2006-09-24 20:05:04.000000000 -0600
@@ -747,6 +747,52 @@ static ssize_t store_vrm_reg(struct devi
static DEVICE_ATTR(vrm, S_IRUGO | S_IWUSR, show_vrm_reg, store_vrm_reg);
+#define IN_UNIT_ATTRS(X) \
+ &sda_in_input[X].dev_attr.attr, \
+ &sda_in_min[X].dev_attr.attr, \
+ &sda_in_max[X].dev_attr.attr
+
+#define FAN_UNIT_ATTRS(X) \
+ &sda_fan_input[X].dev_attr.attr, \
+ &sda_fan_min[X].dev_attr.attr, \
+ &sda_fan_div[X].dev_attr.attr
+
+#define TEMP_UNIT_ATTRS(X) \
+ &sda_temp_input[X].dev_attr.attr, \
+ &sda_temp_max[X].dev_attr.attr, \
+ &sda_temp_max_hyst[X].dev_attr.attr
+
+static struct attribute *w83791d_attributes[] = {
+ IN_UNIT_ATTRS(0),
+ IN_UNIT_ATTRS(1),
+ IN_UNIT_ATTRS(2),
+ IN_UNIT_ATTRS(3),
+ IN_UNIT_ATTRS(4),
+ IN_UNIT_ATTRS(5),
+ IN_UNIT_ATTRS(6),
+ IN_UNIT_ATTRS(7),
+ IN_UNIT_ATTRS(8),
+ IN_UNIT_ATTRS(9),
+ FAN_UNIT_ATTRS(0),
+ FAN_UNIT_ATTRS(1),
+ FAN_UNIT_ATTRS(2),
+ FAN_UNIT_ATTRS(3),
+ FAN_UNIT_ATTRS(4),
+ TEMP_UNIT_ATTRS(0),
+ TEMP_UNIT_ATTRS(1),
+ TEMP_UNIT_ATTRS(2),
+ &dev_attr_alarms.attr,
+ &sda_beep_ctrl[0].dev_attr.attr,
+ &sda_beep_ctrl[1].dev_attr.attr,
+ &dev_attr_cpu0_vid.attr,
+ &dev_attr_vrm.attr,
+ NULL
+};
+
+static const struct attribute_group w83791d_group = {
+ .attrs = w83791d_attributes,
+};
+
/* This function is called when:
* w83791d_driver is inserted (when this module is loaded), for each
available adapter
@@ -968,42 +1014,19 @@ static int w83791d_detect(struct i2c_ada
}
/* Register sysfs hooks */
+ if ((err = sysfs_create_group(&client->dev.kobj, &w83791d_group)))
+ goto error3;
+
+ /* everythings ready, now register working device */
data->class_dev = hwmon_device_register(dev);
if (IS_ERR(data->class_dev)) {
err = PTR_ERR(data->class_dev);
- goto error3;
- }
-
- for (i = 0; i < NUMBER_OF_VIN; i++) {
- device_create_file(dev, &sda_in_input[i].dev_attr);
- device_create_file(dev, &sda_in_min[i].dev_attr);
- device_create_file(dev, &sda_in_max[i].dev_attr);
- }
-
- for (i = 0; i < NUMBER_OF_FANIN; i++) {
- device_create_file(dev, &sda_fan_input[i].dev_attr);
- device_create_file(dev, &sda_fan_div[i].dev_attr);
- device_create_file(dev, &sda_fan_min[i].dev_attr);
+ goto error4;
}
- for (i = 0; i < NUMBER_OF_TEMPIN; i++) {
- device_create_file(dev, &sda_temp_input[i].dev_attr);
- device_create_file(dev, &sda_temp_max[i].dev_attr);
- device_create_file(dev, &sda_temp_max_hyst[i].dev_attr);
- }
-
- device_create_file(dev, &dev_attr_alarms);
-
- for (i = 0; i < ARRAY_SIZE(sda_beep_ctrl); i++) {
- device_create_file(dev, &sda_beep_ctrl[i].dev_attr);
- }
-
- device_create_file(dev, &dev_attr_cpu0_vid);
- device_create_file(dev, &dev_attr_vrm);
-
return 0;
-error3:
+error4:
if (data->lm75[0] != NULL) {
i2c_detach_client(data->lm75[0]);
kfree(data->lm75[0]);
@@ -1012,6 +1035,9 @@ error3:
i2c_detach_client(data->lm75[1]);
kfree(data->lm75[1]);
}
+
+error3:
+ sysfs_remove_group(&client->dev.kobj, &w83791d_group);
error2:
i2c_detach_client(client);
error1:
^ permalink raw reply [flat|nested] 5+ messages in thread
* [lm-sensors] [patch] hwmon/w83791d - fix unchecked errs
2006-09-25 2:27 [lm-sensors] [patch] hwmon/w83791d - fix unchecked errs Jim Cromie
@ 2006-09-25 9:48 ` Charles Spirakis
2006-09-25 13:35 ` Jean Delvare
` (2 subsequent siblings)
3 siblings, 0 replies; 5+ messages in thread
From: Charles Spirakis @ 2006-09-25 9:48 UTC (permalink / raw)
To: lm-sensors
Appears to work.
Signed-off by: Charles Spirakis <bezaur at gmail.com>
On 9/24/06, Jim Cromie <jim.cromie at gmail.com> wrote:
>
> replace all unchecked calls to device_create_file with a single group
> declaration,
> and one call to sysfs_create_group, and check that one return status.
>
> Signed-off-by: Jim Cromie <jim.cromie at gmail.com>
>
> ---
>
> compile tested only. Charles, can you test it for me ? TIA ;-)
>
> $ diffstat pc-set/hwmon-unchecked-return-status-fixes-w83791d.patch
> w83791d.c | 84
> ++++++++++++++++++++++++++++++++++++++++----------------------
> 1 files changed, 55 insertions(+), 29 deletions(-)
>
> diff -ruNp -X dontdiff -X exclude-diffs 6rlocks-0/drivers/hwmon/w83791d.c w83791d/drivers/hwmon/w83791d.c
> --- 6rlocks-0/drivers/hwmon/w83791d.c 2006-09-19 23:58:37.000000000 -0600
> +++ w83791d/drivers/hwmon/w83791d.c 2006-09-24 20:05:04.000000000 -0600
> @@ -747,6 +747,52 @@ static ssize_t store_vrm_reg(struct devi
>
> static DEVICE_ATTR(vrm, S_IRUGO | S_IWUSR, show_vrm_reg, store_vrm_reg);
>
> +#define IN_UNIT_ATTRS(X) \
> + &sda_in_input[X].dev_attr.attr, \
> + &sda_in_min[X].dev_attr.attr, \
> + &sda_in_max[X].dev_attr.attr
> +
> +#define FAN_UNIT_ATTRS(X) \
> + &sda_fan_input[X].dev_attr.attr, \
> + &sda_fan_min[X].dev_attr.attr, \
> + &sda_fan_div[X].dev_attr.attr
> +
> +#define TEMP_UNIT_ATTRS(X) \
> + &sda_temp_input[X].dev_attr.attr, \
> + &sda_temp_max[X].dev_attr.attr, \
> + &sda_temp_max_hyst[X].dev_attr.attr
> +
> +static struct attribute *w83791d_attributes[] = {
> + IN_UNIT_ATTRS(0),
> + IN_UNIT_ATTRS(1),
> + IN_UNIT_ATTRS(2),
> + IN_UNIT_ATTRS(3),
> + IN_UNIT_ATTRS(4),
> + IN_UNIT_ATTRS(5),
> + IN_UNIT_ATTRS(6),
> + IN_UNIT_ATTRS(7),
> + IN_UNIT_ATTRS(8),
> + IN_UNIT_ATTRS(9),
> + FAN_UNIT_ATTRS(0),
> + FAN_UNIT_ATTRS(1),
> + FAN_UNIT_ATTRS(2),
> + FAN_UNIT_ATTRS(3),
> + FAN_UNIT_ATTRS(4),
> + TEMP_UNIT_ATTRS(0),
> + TEMP_UNIT_ATTRS(1),
> + TEMP_UNIT_ATTRS(2),
> + &dev_attr_alarms.attr,
> + &sda_beep_ctrl[0].dev_attr.attr,
> + &sda_beep_ctrl[1].dev_attr.attr,
> + &dev_attr_cpu0_vid.attr,
> + &dev_attr_vrm.attr,
> + NULL
> +};
> +
> +static const struct attribute_group w83791d_group = {
> + .attrs = w83791d_attributes,
> +};
> +
> /* This function is called when:
> * w83791d_driver is inserted (when this module is loaded), for each
> available adapter
> @@ -968,42 +1014,19 @@ static int w83791d_detect(struct i2c_ada
> }
>
> /* Register sysfs hooks */
> + if ((err = sysfs_create_group(&client->dev.kobj, &w83791d_group)))
> + goto error3;
> +
> + /* everythings ready, now register working device */
> data->class_dev = hwmon_device_register(dev);
> if (IS_ERR(data->class_dev)) {
> err = PTR_ERR(data->class_dev);
> - goto error3;
> - }
> -
> - for (i = 0; i < NUMBER_OF_VIN; i++) {
> - device_create_file(dev, &sda_in_input[i].dev_attr);
> - device_create_file(dev, &sda_in_min[i].dev_attr);
> - device_create_file(dev, &sda_in_max[i].dev_attr);
> - }
> -
> - for (i = 0; i < NUMBER_OF_FANIN; i++) {
> - device_create_file(dev, &sda_fan_input[i].dev_attr);
> - device_create_file(dev, &sda_fan_div[i].dev_attr);
> - device_create_file(dev, &sda_fan_min[i].dev_attr);
> + goto error4;
> }
>
> - for (i = 0; i < NUMBER_OF_TEMPIN; i++) {
> - device_create_file(dev, &sda_temp_input[i].dev_attr);
> - device_create_file(dev, &sda_temp_max[i].dev_attr);
> - device_create_file(dev, &sda_temp_max_hyst[i].dev_attr);
> - }
> -
> - device_create_file(dev, &dev_attr_alarms);
> -
> - for (i = 0; i < ARRAY_SIZE(sda_beep_ctrl); i++) {
> - device_create_file(dev, &sda_beep_ctrl[i].dev_attr);
> - }
> -
> - device_create_file(dev, &dev_attr_cpu0_vid);
> - device_create_file(dev, &dev_attr_vrm);
> -
> return 0;
>
> -error3:
> +error4:
> if (data->lm75[0] != NULL) {
> i2c_detach_client(data->lm75[0]);
> kfree(data->lm75[0]);
> @@ -1012,6 +1035,9 @@ error3:
> i2c_detach_client(data->lm75[1]);
> kfree(data->lm75[1]);
> }
> +
> +error3:
> + sysfs_remove_group(&client->dev.kobj, &w83791d_group);
> error2:
> i2c_detach_client(client);
> error1:
>
>
>
^ permalink raw reply [flat|nested] 5+ messages in thread
* [lm-sensors] [patch] hwmon/w83791d - fix unchecked errs
2006-09-25 2:27 [lm-sensors] [patch] hwmon/w83791d - fix unchecked errs Jim Cromie
2006-09-25 9:48 ` Charles Spirakis
@ 2006-09-25 13:35 ` Jean Delvare
2006-09-25 20:13 ` Charles Spirakis
2006-09-25 21:31 ` Jean Delvare
3 siblings, 0 replies; 5+ messages in thread
From: Jean Delvare @ 2006-09-25 13:35 UTC (permalink / raw)
To: lm-sensors
Hi Jim,
> replace all unchecked calls to device_create_file with a single group
> declaration,
> and one call to sysfs_create_group, and check that one return status.
>
> Signed-off-by: Jim Cromie <jim.cromie at gmail.com>
>
> ---
>
> compile tested only. Charles, can you test it for me ? TIA ;-)
>
> $ diffstat pc-set/hwmon-unchecked-return-status-fixes-w83791d.patch
> w83791d.c | 84
> ++++++++++++++++++++++++++++++++++++++++----------------------
> 1 files changed, 55 insertions(+), 29 deletions(-)
>
> diff -ruNp -X dontdiff -X exclude-diffs 6rlocks-0/drivers/hwmon/w83791d.c w83791d/drivers/hwmon/w83791d.c
> --- 6rlocks-0/drivers/hwmon/w83791d.c 2006-09-19 23:58:37.000000000 -0600
> +++ w83791d/drivers/hwmon/w83791d.c 2006-09-24 20:05:04.000000000 -0600
> @@ -747,6 +747,52 @@ static ssize_t store_vrm_reg(struct devi
>
> static DEVICE_ATTR(vrm, S_IRUGO | S_IWUSR, show_vrm_reg, store_vrm_reg);
>
> +#define IN_UNIT_ATTRS(X) \
> + &sda_in_input[X].dev_attr.attr, \
> + &sda_in_min[X].dev_attr.attr, \
> + &sda_in_max[X].dev_attr.attr
> +
> +#define FAN_UNIT_ATTRS(X) \
> + &sda_fan_input[X].dev_attr.attr, \
> + &sda_fan_min[X].dev_attr.attr, \
> + &sda_fan_div[X].dev_attr.attr
> +
> +#define TEMP_UNIT_ATTRS(X) \
> + &sda_temp_input[X].dev_attr.attr, \
> + &sda_temp_max[X].dev_attr.attr, \
> + &sda_temp_max_hyst[X].dev_attr.attr
> +
> +static struct attribute *w83791d_attributes[] = {
> + IN_UNIT_ATTRS(0),
> + IN_UNIT_ATTRS(1),
> + IN_UNIT_ATTRS(2),
> + IN_UNIT_ATTRS(3),
> + IN_UNIT_ATTRS(4),
> + IN_UNIT_ATTRS(5),
> + IN_UNIT_ATTRS(6),
> + IN_UNIT_ATTRS(7),
> + IN_UNIT_ATTRS(8),
> + IN_UNIT_ATTRS(9),
> + FAN_UNIT_ATTRS(0),
> + FAN_UNIT_ATTRS(1),
> + FAN_UNIT_ATTRS(2),
> + FAN_UNIT_ATTRS(3),
> + FAN_UNIT_ATTRS(4),
> + TEMP_UNIT_ATTRS(0),
> + TEMP_UNIT_ATTRS(1),
> + TEMP_UNIT_ATTRS(2),
> + &dev_attr_alarms.attr,
> + &sda_beep_ctrl[0].dev_attr.attr,
> + &sda_beep_ctrl[1].dev_attr.attr,
> + &dev_attr_cpu0_vid.attr,
> + &dev_attr_vrm.attr,
> + NULL
> +};
> +
> +static const struct attribute_group w83791d_group = {
> + .attrs = w83791d_attributes,
> +};
> +
> /* This function is called when:
> * w83791d_driver is inserted (when this module is loaded), for each
> available adapter
> @@ -968,42 +1014,19 @@ static int w83791d_detect(struct i2c_ada
> }
>
> /* Register sysfs hooks */
> + if ((err = sysfs_create_group(&client->dev.kobj, &w83791d_group)))
> + goto error3;
> +
> + /* everythings ready, now register working device */
> data->class_dev = hwmon_device_register(dev);
> if (IS_ERR(data->class_dev)) {
> err = PTR_ERR(data->class_dev);
> - goto error3;
> - }
> -
> - for (i = 0; i < NUMBER_OF_VIN; i++) {
> - device_create_file(dev, &sda_in_input[i].dev_attr);
> - device_create_file(dev, &sda_in_min[i].dev_attr);
> - device_create_file(dev, &sda_in_max[i].dev_attr);
> - }
> -
> - for (i = 0; i < NUMBER_OF_FANIN; i++) {
> - device_create_file(dev, &sda_fan_input[i].dev_attr);
> - device_create_file(dev, &sda_fan_div[i].dev_attr);
> - device_create_file(dev, &sda_fan_min[i].dev_attr);
> + goto error4;
> }
>
> - for (i = 0; i < NUMBER_OF_TEMPIN; i++) {
> - device_create_file(dev, &sda_temp_input[i].dev_attr);
> - device_create_file(dev, &sda_temp_max[i].dev_attr);
> - device_create_file(dev, &sda_temp_max_hyst[i].dev_attr);
> - }
> -
> - device_create_file(dev, &dev_attr_alarms);
> -
> - for (i = 0; i < ARRAY_SIZE(sda_beep_ctrl); i++) {
> - device_create_file(dev, &sda_beep_ctrl[i].dev_attr);
> - }
> -
> - device_create_file(dev, &dev_attr_cpu0_vid);
> - device_create_file(dev, &dev_attr_vrm);
> -
> return 0;
>
> -error3:
> +error4:
> if (data->lm75[0] != NULL) {
> i2c_detach_client(data->lm75[0]);
> kfree(data->lm75[0]);
> @@ -1012,6 +1035,9 @@ error3:
> i2c_detach_client(data->lm75[1]);
> kfree(data->lm75[1]);
> }
> +
> +error3:
> + sysfs_remove_group(&client->dev.kobj, &w83791d_group);
Looks broken to me. error3 should remove the subclients, and error4
should remove the sysfs files, rather than the over way around.
> error2:
> i2c_detach_client(client);
> error1:
Also, it misses the sysfs files removal at i2c client deletion time.
Care to submit a fixed patch? Thanks.
--
Jean Delvare
^ permalink raw reply [flat|nested] 5+ messages in thread
* [lm-sensors] [patch] hwmon/w83791d - fix unchecked errs
2006-09-25 2:27 [lm-sensors] [patch] hwmon/w83791d - fix unchecked errs Jim Cromie
2006-09-25 9:48 ` Charles Spirakis
2006-09-25 13:35 ` Jean Delvare
@ 2006-09-25 20:13 ` Charles Spirakis
2006-09-25 21:31 ` Jean Delvare
3 siblings, 0 replies; 5+ messages in thread
From: Charles Spirakis @ 2006-09-25 20:13 UTC (permalink / raw)
To: lm-sensors
In the original code, there was nothing that removed the sysfs files
in the detach. Was that a bug? Or is the removal something new for the
sysfs_create_group() call? Adding and removing the module before
didn't show any problems (would it show as a kernel memory leak
visible via something like /proc/meminfo?)
I've modified the patch per the discussion below (attached). I have
confirmed the sysfs files are created and the values are accessed
appropriately, but is there a good way to validate the error cases?
-- charles
On 9/25/06, Jean Delvare <khali at linux-fr.org> wrote:
> Hi Jim,
>
> > replace all unchecked calls to device_create_file with a single group
> > declaration,
> > and one call to sysfs_create_group, and check that one return status.
> >
> > Signed-off-by: Jim Cromie <jim.cromie at gmail.com>
> >
> > ---
> >
> > compile tested only. Charles, can you test it for me ? TIA ;-)
> >
> > $ diffstat pc-set/hwmon-unchecked-return-status-fixes-w83791d.patch
> > w83791d.c | 84
> > ++++++++++++++++++++++++++++++++++++++++----------------------
> > 1 files changed, 55 insertions(+), 29 deletions(-)
> >
> > diff -ruNp -X dontdiff -X exclude-diffs 6rlocks-0/drivers/hwmon/w83791d.c w83791d/drivers/hwmon/w83791d.c
> > --- 6rlocks-0/drivers/hwmon/w83791d.c 2006-09-19 23:58:37.000000000 -0600
> > +++ w83791d/drivers/hwmon/w83791d.c 2006-09-24 20:05:04.000000000 -0600
> > @@ -747,6 +747,52 @@ static ssize_t store_vrm_reg(struct devi
> >
> > static DEVICE_ATTR(vrm, S_IRUGO | S_IWUSR, show_vrm_reg, store_vrm_reg);
> >
> > +#define IN_UNIT_ATTRS(X) \
> > + &sda_in_input[X].dev_attr.attr, \
> > + &sda_in_min[X].dev_attr.attr, \
> > + &sda_in_max[X].dev_attr.attr
> > +
> > +#define FAN_UNIT_ATTRS(X) \
> > + &sda_fan_input[X].dev_attr.attr, \
> > + &sda_fan_min[X].dev_attr.attr, \
> > + &sda_fan_div[X].dev_attr.attr
> > +
> > +#define TEMP_UNIT_ATTRS(X) \
> > + &sda_temp_input[X].dev_attr.attr, \
> > + &sda_temp_max[X].dev_attr.attr, \
> > + &sda_temp_max_hyst[X].dev_attr.attr
> > +
> > +static struct attribute *w83791d_attributes[] = {
> > + IN_UNIT_ATTRS(0),
> > + IN_UNIT_ATTRS(1),
> > + IN_UNIT_ATTRS(2),
> > + IN_UNIT_ATTRS(3),
> > + IN_UNIT_ATTRS(4),
> > + IN_UNIT_ATTRS(5),
> > + IN_UNIT_ATTRS(6),
> > + IN_UNIT_ATTRS(7),
> > + IN_UNIT_ATTRS(8),
> > + IN_UNIT_ATTRS(9),
> > + FAN_UNIT_ATTRS(0),
> > + FAN_UNIT_ATTRS(1),
> > + FAN_UNIT_ATTRS(2),
> > + FAN_UNIT_ATTRS(3),
> > + FAN_UNIT_ATTRS(4),
> > + TEMP_UNIT_ATTRS(0),
> > + TEMP_UNIT_ATTRS(1),
> > + TEMP_UNIT_ATTRS(2),
> > + &dev_attr_alarms.attr,
> > + &sda_beep_ctrl[0].dev_attr.attr,
> > + &sda_beep_ctrl[1].dev_attr.attr,
> > + &dev_attr_cpu0_vid.attr,
> > + &dev_attr_vrm.attr,
> > + NULL
> > +};
> > +
> > +static const struct attribute_group w83791d_group = {
> > + .attrs = w83791d_attributes,
> > +};
> > +
> > /* This function is called when:
> > * w83791d_driver is inserted (when this module is loaded), for each
> > available adapter
> > @@ -968,42 +1014,19 @@ static int w83791d_detect(struct i2c_ada
> > }
> >
> > /* Register sysfs hooks */
> > + if ((err = sysfs_create_group(&client->dev.kobj, &w83791d_group)))
> > + goto error3;
> > +
> > + /* everythings ready, now register working device */
> > data->class_dev = hwmon_device_register(dev);
> > if (IS_ERR(data->class_dev)) {
> > err = PTR_ERR(data->class_dev);
> > - goto error3;
> > - }
> > -
> > - for (i = 0; i < NUMBER_OF_VIN; i++) {
> > - device_create_file(dev, &sda_in_input[i].dev_attr);
> > - device_create_file(dev, &sda_in_min[i].dev_attr);
> > - device_create_file(dev, &sda_in_max[i].dev_attr);
> > - }
> > -
> > - for (i = 0; i < NUMBER_OF_FANIN; i++) {
> > - device_create_file(dev, &sda_fan_input[i].dev_attr);
> > - device_create_file(dev, &sda_fan_div[i].dev_attr);
> > - device_create_file(dev, &sda_fan_min[i].dev_attr);
> > + goto error4;
> > }
> >
> > - for (i = 0; i < NUMBER_OF_TEMPIN; i++) {
> > - device_create_file(dev, &sda_temp_input[i].dev_attr);
> > - device_create_file(dev, &sda_temp_max[i].dev_attr);
> > - device_create_file(dev, &sda_temp_max_hyst[i].dev_attr);
> > - }
> > -
> > - device_create_file(dev, &dev_attr_alarms);
> > -
> > - for (i = 0; i < ARRAY_SIZE(sda_beep_ctrl); i++) {
> > - device_create_file(dev, &sda_beep_ctrl[i].dev_attr);
> > - }
> > -
> > - device_create_file(dev, &dev_attr_cpu0_vid);
> > - device_create_file(dev, &dev_attr_vrm);
> > -
> > return 0;
> >
> > -error3:
> > +error4:
> > if (data->lm75[0] != NULL) {
> > i2c_detach_client(data->lm75[0]);
> > kfree(data->lm75[0]);
> > @@ -1012,6 +1035,9 @@ error3:
> > i2c_detach_client(data->lm75[1]);
> > kfree(data->lm75[1]);
> > }
> > +
> > +error3:
> > + sysfs_remove_group(&client->dev.kobj, &w83791d_group);
>
> Looks broken to me. error3 should remove the subclients, and error4
> should remove the sysfs files, rather than the over way around.
>
> > error2:
> > i2c_detach_client(client);
> > error1:
>
> Also, it misses the sysfs files removal at i2c client deletion time.
>
> Care to submit a fixed patch? Thanks.
>
> --
> Jean Delvare
>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: w83791d_sysfs_error.patch
Type: text/x-patch
Size: 3885 bytes
Desc: not available
Url : http://lists.lm-sensors.org/pipermail/lm-sensors/attachments/20060925/e22d6f71/attachment-0001.bin
^ permalink raw reply [flat|nested] 5+ messages in thread
* [lm-sensors] [patch] hwmon/w83791d - fix unchecked errs
2006-09-25 2:27 [lm-sensors] [patch] hwmon/w83791d - fix unchecked errs Jim Cromie
` (2 preceding siblings ...)
2006-09-25 20:13 ` Charles Spirakis
@ 2006-09-25 21:31 ` Jean Delvare
3 siblings, 0 replies; 5+ messages in thread
From: Jean Delvare @ 2006-09-25 21:31 UTC (permalink / raw)
To: lm-sensors
Hi Charles,
> In the original code, there was nothing that removed the sysfs files
> in the detach. Was that a bug? Or is the removal something new for the
> sysfs_create_group() call? Adding and removing the module before
> didn't show any problems (would it show as a kernel memory leak
> visible via something like /proc/meminfo?)
No, it wasn't a real bug and there was no memory leak. Not removing the
files was simply considered not clean and bad, and given that it is now
really easy to remove them when using groups, we decided to fix both
problems (unchecked errors on creation and lack of deletion) at once.
We could have made separate patches, but given that we had to change
all drivers, doing it all in one pass sounded easier.
Another reason to now properly delete the files on driver detach is
that we plan to convert the i2c subsystem to the device driver model
someday. In the device driver model, devices exist independently of
drivers being loaded to handle them. This means that a devices will
survive its driver removal, and if we don't remove the files when the
driver is removed, we end up with dangling callbacks. Bad.
> I've modified the patch per the discussion below (attached). I have
> confirmed the sysfs files are created and the values are accessed
> appropriately, but is there a good way to validate the error cases?
The easiest way to test is to simulate an error at the end of the detect
function, i.e. do as if hwmon_device_register() had failed. That way
you train the whole error path. Of course it's even better to test each
possible failure individually, but that takes more time.
Anyway, I'm confident the patch is OK now - it was really the same fix
for all drivers. I'll apply the patch to my tree now.
Thanks,
--
Jean Delvare
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2006-09-25 21:31 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2006-09-25 2:27 [lm-sensors] [patch] hwmon/w83791d - fix unchecked errs Jim Cromie
2006-09-25 9:48 ` Charles Spirakis
2006-09-25 13:35 ` Jean Delvare
2006-09-25 20:13 ` Charles Spirakis
2006-09-25 21:31 ` Jean Delvare
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.