[lm-sensors] [patch] hwmon/w83791d - fix unchecked errs

Charles Spirakis bezaur at gmail.com
Mon Sep 25 22:13:40 CEST 2006


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


More information about the lm-sensors mailing list