hwmon:fix sparse warnings + error handling
RFC PATCH 1/1 hwmon:fix sparse warnings + error handling
Hi,
This patch fixes 56 sparse "ignoring return value of 'device_create_file'" warnings. It also adds error handling.
w83627hf.c | 96 ++++++++++++++++++++++++++++++++++++++++++++++++++-----------
1 file changed, 80 insertions(+), 16 deletions(-)
Regards,
Michal
Signature · 173 lines
Michal K. K. Piotrowski LTG - Linux Testers Group (http://www.stardust.webpages.pl/ltg/wiki/) Signed-off-by: Michal Piotrowski <[email protected]> diff -uprN -X linux-work/Documentation/dontdiff linux-work-clean/drivers/hwmon/w83627hf.c linux-work/drivers/hwmon/w83627hf.c --- linux-work-clean/drivers/hwmon/w83627hf.c 2006-08-20 22:02:40.000000000 +0200 +++ linux-work/drivers/hwmon/w83627hf.c 2006-08-20 22:27:14.000000000 +0200 @@ -513,9 +513,21 @@ static DEVICE_ATTR(in0_max, S_IRUGO | S_ #define device_create_file_in(client, offset) \ do { \ -device_create_file(&client->dev, &dev_attr_in##offset##_input); \ -device_create_file(&client->dev, &dev_attr_in##offset##_min); \ -device_create_file(&client->dev, &dev_attr_in##offset##_max); \ + err = device_create_file(&client->dev, &dev_attr_in##offset##_input); \ + if (err) {\ + hwmon_device_unregister(data->class_dev); \ + return err; \ + } \ + err = device_create_file(&client->dev, &dev_attr_in##offset##_min); \ + if (err) {\ + hwmon_device_unregister(data->class_dev); \ + return err; \ + } \ + err = device_create_file(&client->dev, &dev_attr_in##offset##_max); \ + if (err) {\ + hwmon_device_unregister(data->class_dev); \ + return err; \ + } \ } while (0) #define show_fan_reg(reg) \ @@ -577,8 +589,16 @@ sysfs_fan_min_offset(3); #define device_create_file_fan(client, offset) \ do { \ -device_create_file(&client->dev, &dev_attr_fan##offset##_input); \ -device_create_file(&client->dev, &dev_attr_fan##offset##_min); \ + err = device_create_file(&client->dev, &dev_attr_fan##offset##_input); \ + if (err) {\ + hwmon_device_unregister(data->class_dev); \ + return err; \ + } \ + err = device_create_file(&client->dev, &dev_attr_fan##offset##_min); \ + if (err) {\ + hwmon_device_unregister(data->class_dev); \ + return err; \ + } \ } while (0) #define show_temp_reg(reg) \ @@ -657,9 +677,21 @@ sysfs_temp_offsets(3); #define device_create_file_temp(client, offset) \ do { \ -device_create_file(&client->dev, &dev_attr_temp##offset##_input); \ -device_create_file(&client->dev, &dev_attr_temp##offset##_max); \ -device_create_file(&client->dev, &dev_attr_temp##offset##_max_hyst); \ + err = device_create_file(&client->dev, &dev_attr_temp##offset##_input); \ + if (err) {\ + hwmon_device_unregister(data->class_dev); \ + return err; \ + } \ + err = device_create_file(&client->dev, &dev_attr_temp##offset##_max); \ + if (err) {\ + hwmon_device_unregister(data->class_dev); \ + return err; \ + } \ + err = device_create_file(&client->dev, &dev_attr_temp##offset##_max_hyst); \ + if (err) {\ + hwmon_device_unregister(data->class_dev); \ + return err; \ + } \ } while (0) static ssize_t @@ -670,7 +702,11 @@ show_vid_reg(struct device *dev, struct } static DEVICE_ATTR(cpu0_vid, S_IRUGO, show_vid_reg, NULL); #define device_create_file_vid(client) \ -device_create_file(&client->dev, &dev_attr_cpu0_vid) +err = device_create_file(&client->dev, &dev_attr_cpu0_vid); \ +if (err) { \ + hwmon_device_unregister(data->class_dev); \ + return err; \ +} \ static ssize_t show_vrm_reg(struct device *dev, struct device_attribute *attr, char *buf) @@ -692,7 +728,11 @@ store_vrm_reg(struct device *dev, struct } static DEVICE_ATTR(vrm, S_IRUGO | S_IWUSR, show_vrm_reg, store_vrm_reg); #define device_create_file_vrm(client) \ -device_create_file(&client->dev, &dev_attr_vrm) +err = device_create_file(&client->dev, &dev_attr_vrm); \ +if (err) {\ + hwmon_device_unregister(data->class_dev); \ + return err; \ +} \ static ssize_t show_alarms_reg(struct device *dev, struct device_attribute *attr, char *buf) @@ -702,7 +742,11 @@ show_alarms_reg(struct device *dev, stru } static DEVICE_ATTR(alarms, S_IRUGO, show_alarms_reg, NULL); #define device_create_file_alarms(client) \ -device_create_file(&client->dev, &dev_attr_alarms) +err = device_create_file(&client->dev, &dev_attr_alarms); \ +if (err) {\ + hwmon_device_unregister(data->class_dev); \ + return err; \ +} \ #define show_beep_reg(REG, reg) \ static ssize_t show_beep_##reg (struct device *dev, struct device_attribute *attr, char *buf) \ @@ -767,8 +811,16 @@ sysfs_beep(MASK, mask); #define device_create_file_beep(client) \ do { \ -device_create_file(&client->dev, &dev_attr_beep_enable); \ -device_create_file(&client->dev, &dev_attr_beep_mask); \ + err = device_create_file(&client->dev, &dev_attr_beep_enable); \ + if (err) {\ + hwmon_device_unregister(data->class_dev); \ + return err; \ + } \ + err = device_create_file(&client->dev, &dev_attr_beep_mask); \ + if (err) {\ + hwmon_device_unregister(data->class_dev); \ + return err; \ + } \ } while (0) static ssize_t @@ -838,7 +890,11 @@ sysfs_fan_div(3); #define device_create_file_fan_div(client, offset) \ do { \ -device_create_file(&client->dev, &dev_attr_fan##offset##_div); \ + err = device_create_file(&client->dev, &dev_attr_fan##offset##_div); \ + if (err) {\ + hwmon_device_unregister(data->class_dev); \ + return err; \ + } \ } while (0) static ssize_t @@ -897,7 +953,11 @@ sysfs_pwm(3); #define device_create_file_pwm(client, offset) \ do { \ -device_create_file(&client->dev, &dev_attr_pwm##offset); \ + err = device_create_file(&client->dev, &dev_attr_pwm##offset); \ + if (err) {\ + hwmon_device_unregister(data->class_dev); \ + return err; \ + } \ } while (0) static ssize_t @@ -973,7 +1033,11 @@ sysfs_sensor(3); #define device_create_file_sensor(client, offset) \ do { \ -device_create_file(&client->dev, &dev_attr_temp##offset##_type); \ + err = device_create_file(&client->dev, &dev_attr_temp##offset##_type); \ + if (err) {\ + hwmon_device_unregister(data->class_dev); \ + return err; \ + } \ } while (0)
Quoted 11 lines · Michal Piotrowski
Hi,
This patch fixes 56 sparse "ignoring return value of '
device_create_file'" warnings. It also adds error handling.
w83627hf.c| 96 ++++++++++++++++++++++++++++++++++++++++++++++++++-----------1 file changed, 80 insertions(+), 16 deletions(-)Regards,
Michal
Ouch.
Ideally, you should have checked here (LM-Sensors ML) 1st. The driver you chose has already been reworked along those lines, and accepted by Jean Delvare, and thus slated for .19
an early copy (8/02)
http://lists.lm-sensors.org/pipermail/lm-sensors/2006-August/017043.html
If youre so inclined, there are ~38 others in need of similar attention ;)
<cp-pasted from ML thread>
Here is asb100.c... I'll add lm75, lm78, smsc47b397, and w83627hf to this patch as I have time. (Jean: don't apply yet.)
pc87360 (one of the larger offenders) is also now fixed.
Im sure if you ask nicely, Jean will provide you with a current/authoritative list ;-)
http://khali.linux-fr.org/devel/i2c/linux-2.6/?M=A
has a fairly current collection and series-file (8/14/06) Unfortunately, the series doesnt enumerate the patches as a set, and several accepted patches arent there yet. Jean, is this still the best/good place to keep current ?
Ar Sul, 2006-08-20 am 22:44 +0200, ysgrifennodd Michal Piotrowski:
Quoted 4 lines
Hi,
This patch fixes 56 sparse "ignoring return value of '
device_create_file'" warnings. It also adds error handling.
The core of these wants turning into called functions with the extra checks included, just because of the size it now is.
On 20/08/06, jim.cromie at gmail.com <jim.cromie at gmail.com> wrote:
[snip]
Quoted 2 lines
If youre so inclined, there are ~38 others in need of similar attention ;)
I'll try to fix them.
Regards,
Michal
Signature · 3 lines
Michal K. K. Piotrowski LTG - Linux Testers Group (http://www.stardust.webpages.pl/ltg/wiki/)
Andrew, Michal:
Quoted 4 lines · Michal Piotrowski
This patch fixes 56 sparse "ignoring return value of '
device_create_file'" warnings. It also adds error handling.
w83627hf.c| 96 ++++++++++++++++++++++++++++++++++++++++++++++++++-----------1 file changed, 80 insertions(+), 16 deletions(-)
Thanks for doing this... but Andrew please don't apply it. The sensors project people are working on these even now, and we already have a patch for the w83627hf driver...
http://lists.lm-sensors.org/pipermail/lm-sensors/2006-August/017204.html
Jean Delvare (hwmon maintainer) should be sending these up the chain soon.
Michal: if you're interested in fixing any of the rest of them, please take a look at the patch above to see the mechanism we intend to use. It actually makes the drivers *smaller* than they were.
Regards,
Signature · 2 lines
Mark M. Hoffman mhoffman at lightlink.com
Quoted 15 lines
drivers/hwmon/w83627hf.c +6
-3@@ -513,9 +513,21 @@ static DEVICE_ATTR(in0_max, S_IRUGO | S_#define device_create_file_in (client, offset) \ do { \ -device_create_file(&client->dev, &dev_attr_in##offset##_input); \ -device_create_file(&client->dev, &dev_attr_in##offset##_min); \ -device_create_file(&client->dev, &dev_attr_in##offset##_max); \ + err = device_create_file(&client->dev, &dev_attr_in##offset##_input); \ + if (err) {\ + hwmon_device_unregister(data->class_dev); \ + return err; \ + } \ + err = device_create_file(&client->dev, &dev_attr_in##offset##_min); \
drivers/hwmon/w83627hf.c static DEVICE_ATTR(in0_max, S_IRUGO | S_ · line 531
Quoted 10 lines
drivers/hwmon/w83627hf.c +9
-0+if (err) {\ +hwmon_device_unregister (data->class_dev); \ +return err; \ + } \ + err = device_create_file(&client->dev, &dev_attr_in##offset##_max); \ +if (err) {\ +hwmon_device_unregister (data->class_dev); \ +return err; \ + } \ }while (0 )
_Never_ use "return" in a macro. It's way too confusing for whoever will read the code later.
Signature · 1 line
Jean Delvare
Mark, Michal,
Quoted 11 lines
Thanks for doing this... but Andrew please don't apply it. The sensors project people are working on these even now, and we already have a patch for the w83627hf driver...
http://lists.lm-sensors.org/pipermail/lm-sensors/2006-August/017204.html
Jean Delvare (hwmon maintainer) should be sending these up the chain soon.
Michal: if you're interested in fixing any of the rest of them, please take a look at the patch above to see the mechanism we intend to use. It actually makes the drivers *smaller* than they were.
The size change really depends on the driver. For older drivers with individual file registration (sometimes hidden behind macros) the driver size will indeed shrink, but for newer drivers with loop-based file registration, this would be a slight increase in size. Not that it really matters anyway, what matters is that we handle errors and file deletion properly from now on.
Michal, if you go on working on this (and this is welcome), please follow what Mark did, as this is what we agreed was the best approach. Here is a quick status summary for drivers/hwmon:
Done by Mark M. Hoffman:
o asb100
o lm75
o lm78
o smsc47b397
o w83627hf
Done by Jim Cromie:
o pc87360
Will be done by David Hubbard:
o w83627ehf
Will be done by me:
o f71805f
o it87
o lm63
o lm83
o lm90
This leaves the following list:
o abituguru
o adm1021
o adm1025
o adm1026
o adm1031
o adm9240
o atxp1
o ds1621
o fscher
o fscpos
o gl518sm
o gl520sm
o lm77
o lm80
o lm85
o lm87
o lm92
o max1619
o sis5595
o smsc47m1
o smsc47m192
o via686a
o vt8231
o w83781d
o w83791d
o w83792d
o w83l785ts
Almost 1000 warnings for drivers/hwmon alone... OTOH I wonder how device_create_file and friends qualified for __must_check given that nothing wrong can happen if they fail, from the kernel's point of view. The files are not created and that's about it.
If you are going to fix some of the drivers listed above, please advertise on the lm-sensors list so that your work is not duplicated.
As a side note, I have patches ready for everything under drivers/i2c already, I sent them on the i2c list last week and will push them soon now.
Thanks,
Signature · 1 line
Jean Delvare
Quoted 25 lines · Jean Delvare
Mark, Michal,
Quoted 11 lines
Thanks for doing this... but Andrew please don't apply it. The sensors project people are working on these even now, and we already have a patch for the w83627hf driver...
http://lists.lm-sensors.org/pipermail/lm-sensors/2006-August/017204.html
Jean Delvare (hwmon maintainer) should be sending these up the chain soon.
Michal: if you're interested in fixing any of the rest of them, please take a look at the patch above to see the mechanism we intend to use. It actually makes the drivers *smaller* than they were.
The size change really depends on the driver. For older drivers with individual file registration (sometimes hidden behind macros) the driver size will indeed shrink, but for newer drivers with loop-based file registration, this would be a slight increase in size. Not that it really matters anyway, what matters is that we handle errors and file deletion properly from now on.
Michal, if you go on working on this (and this is welcome), please follow what Mark did, as this is what we agreed was the best approach. Here is a quick status summary for
drivers/hwmon:
...
Quoted 1 line
o adm9240
Somebody else is welcome to do this one, my last patch was dropped.
Grant.
Quoted 20 lines · Jean Delvare
Quoted 20 lines
drivers/hwmon/w83627hf.c +11
-3@@ -513,9 +513,21 @@ static DEVICE_ATTR(in0_max, S_IRUGO | S_#define device_create_file_in (client, offset) \ do { \ -device_create_file(&client->dev, &dev_attr_in##offset##_input); \ -device_create_file(&client->dev, &dev_attr_in##offset##_min); \ -device_create_file(&client->dev, &dev_attr_in##offset##_max); \ + err = device_create_file(&client->dev, &dev_attr_in##offset##_input); \ + if (err) {\ + hwmon_device_unregister(data->class_dev); \ + return err; \ + } \ + err = device_create_file(&client->dev, &dev_attr_in##offset##_min); \ + if (err) {\ + hwmon_device_unregister(data->class_dev); \ + return err; \ + } \ + err = device_create_file(&client->dev, &dev_attr_in##offset##_max); \
Quoted 9 lines
drivers/hwmon/w83627hf.c static DEVICE_ATTR(in0_max, S_IRUGO | S_ · line 531
Quoted 5 lines
drivers/hwmon/w83627hf.c +4
-0+if (err) {\ +hwmon_device_unregister (data->class_dev); \ +return err; \ + } \ }while (0 )_Never_ use "return" in a macro. It's way too confusing for whoever will read the code later.
Also I believe it is good practice to remove created attributes explicitely instead of relying on sysfs to do the cleanup - I beliee Greg was going to remove it from sysfs at some point of time...
Signature · 1 line
Dmitry
Hi Dmitry:
Quoted 31 lines · Dmitry Torokhov
Quoted 20 lines · Jean Delvare
Quoted 20 lines
drivers/hwmon/w83627hf.c +11
-3@@ -513,9 +513,21 @@ static DEVICE_ATTR(in0_max, S_IRUGO | S_#define device_create_file_in (client, offset) \ do { \ -device_create_file(&client->dev, &dev_attr_in##offset##_input); \ -device_create_file(&client->dev, &dev_attr_in##offset##_min); \ -device_create_file(&client->dev, &dev_attr_in##offset##_max); \ + err = device_create_file(&client->dev, &dev_attr_in##offset##_input); \ + if (err) {\ + hwmon_device_unregister(data->class_dev); \ + return err; \ + } \ + err = device_create_file(&client->dev, &dev_attr_in##offset##_min); \ + if (err) {\ + hwmon_device_unregister(data->class_dev); \ + return err; \ + } \ + err = device_create_file(&client->dev, &dev_attr_in##offset##_max); \Quoted 9 lines
drivers/hwmon/w83627hf.c static DEVICE_ATTR(in0_max, S_IRUGO | S_ · line 531
Quoted 5 lines
drivers/hwmon/w83627hf.c +4
-0+if (err) {\ +hwmon_device_unregister (data->class_dev); \ +return err; \ + } \ }while (0 )_Never_ use "return" in a macro. It's way too confusing for whoever will read the code later.
Quoted 3 lines
Also I believe it is good practice to remove created attributes explicitely instead of relying on sysfs to do the cleanup - I beliee Greg was going to remove it from sysfs at some point of time...
Yep: the patches that are floating on the lm-sensors mailing list do fix this also. Again, see here:
http://lists.lm-sensors.org/pipermail/lm-sensors/2006-August/017204.html
Regards,
Signature · 2 lines
Mark M. Hoffman mhoffman at lightlink.com