[RFC][PATCH] hwmon:fix sparse warnings + error handling

[RFC][PATCH] hwmon:fix sparse warnings + error handling

[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

-- 
Michal K. K. Piotrowski
LTG - Linux Testers Group
(http://www.stardust.webpages.pl/ltg/wiki/)

Signed-off-by: Michal Piotrowski <michal.k.k.piotrowski@gmail.com>

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)

Re: [lm-sensors] [RFC][PATCH] hwmon:fix sparse warnings + error handling

[jim.cromie]

Michal Piotrowski wrote:
> 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 ?

Re: [lm-sensors] [RFC][PATCH] hwmon:fix sparse warnings + error handling

[Michal Piotrowski]

On 20/08/06, jim.cromie@gmail.com <jim.cromie@gmail.com> wrote:
[snip]
> If youre so inclined, there are ~38 others in need
> of similar attention ;)

I'll try to fix them.

Regards,
Michal

-- 
Michal K. K. Piotrowski
LTG - Linux Testers Group
(http://www.stardust.webpages.pl/ltg/wiki/)

Re: [RFC][PATCH] hwmon:fix sparse warnings + error handling

[Alan Cox]

Ar Sul, 2006-08-20 am 22:44 +0200, ysgrifennodd Michal Piotrowski:
> 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.

Re: [lm-sensors] [RFC][PATCH] hwmon:fix sparse warnings + error handling

[Mark M. Hoffman]

Andrew, Michal:

* Michal Piotrowski <michal.k.k.piotrowski@gmail.com> [2006-08-20 22:44:30 +0200]:
> 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,

-- 
Mark M. Hoffman
mhoffman@lightlink.com

Re: [lm-sensors] [RFC][PATCH] hwmon:fix sparse warnings + error handling

[Jean Delvare]

Mark, Michal,

> 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,
-- 
Jean Delvare

Re: [lm-sensors] [RFC][PATCH] hwmon:fix sparse warnings + error handling

[Grant Coady]

On Mon, 21 Aug 2006 11:11:27 +0200, Jean Delvare <khali@linux-fr.org> wrote:

>Mark, Michal,
>
>> 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:
>
...
> o adm9240

Somebody else is welcome to do this one, my last patch was dropped.

Grant.

Re: [lm-sensors] [RFC][PATCH] hwmon:fix sparse warnings + error handling

[Jean Delvare]

> --- 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)

_Never_ use "return" in a macro. It's way too confusing for whoever will
read the code later.

-- 
Jean Delvare

Re: [lm-sensors] [RFC][PATCH] hwmon:fix sparse warnings + error handling

[Dmitry Torokhov]

On Monday 21 August 2006 04:04, Jean Delvare wrote:
> > --- 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)
> 
> _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... 

-- 
Dmitry

Re: [lm-sensors] [RFC][PATCH] hwmon:fix sparse warnings + error handling

[Mark M. Hoffman]

Hi Dmitry:

* Dmitry Torokhov <dtor@insightbb.com> [2006-08-21 20:43:05 -0400]:
> On Monday 21 August 2006 04:04, Jean Delvare wrote:
> > > --- 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)
> > 
> > _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... 

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,

-- 
Mark M. Hoffman
mhoffman@lightlink.com