[lm-sensors] [patch 3/3] pc87360 - fix unchecked

[lm-sensors] [patch 3/3] pc87360 - fix unchecked

[Jim Cromie]

3- rework sys-device-interface startup
    use sysfs_create_group() for 2 sensor-types which are chip-model 
invariant.
       ie all-or-nothing attribute groups
    other 2 groups vary too much due to configuration, etc,
       so we keep the loops of device_create_file(), but now check their 
returns.

Signed-off-by:   Jim Cromie  <jim.cromie at gmail.com>

[jimc at harpo hwmon-stuff]$ diffstat diff.ab-3.20060818.185837
 pc87360.c |   90 
+++++++++++++++++++++++++++++++-------------------------------
 1 files changed, 46 insertions(+), 44 deletions(-)


diff -ruNp -X dontdiff -X exclude-diffs ab-2/drivers/hwmon/pc87360.c ab-3/drivers/hwmon/pc87360.c
--- ab-2/drivers/hwmon/pc87360.c	2006-08-18 17:40:35.000000000 -0600
+++ ab-3/drivers/hwmon/pc87360.c	2006-08-18 18:54:24.000000000 -0600
@@ -1017,67 +1017,69 @@ static int pc87360_detect(struct i2c_ada
 		pc87360_init_client(client, use_thermistors);
 	}
 
-	/* Register sysfs hooks */
-	data->class_dev = hwmon_device_register(&client->dev);
-	if (IS_ERR(data->class_dev)) {
-		err = PTR_ERR(data->class_dev);
+	/* Register all-or-nothing sysfs groups */
+
+	if (data->innr && 
+	    sysfs_create_group(&client->dev.kobj,
+			       &pc8736x_vin_group))
 		goto ERROR3;
-	}
 
-	if (data->innr) {
-		for (i = 0; i < 11; i++) {
-			device_create_file(dev, &in_input[i].dev_attr);
-			device_create_file(dev, &in_min[i].dev_attr);
-			device_create_file(dev, &in_max[i].dev_attr);
-			device_create_file(dev, &in_status[i].dev_attr);
-		}
-		device_create_file(dev, &dev_attr_cpu0_vid);
-		device_create_file(dev, &dev_attr_vrm);
-		device_create_file(dev, &dev_attr_alarms_in);
-	}
+	if (data->innr = 14 &&
+	    sysfs_create_group(&client->dev.kobj, 
+			       &pc8736x_therm_group))
+		goto ERROR3;
+
+	/* create device attr-files for varying sysfs groups */
 
 	if (data->tempnr) {
 		for (i = 0; i < data->tempnr; i++) {
-			device_create_file(dev, &temp_input[i].dev_attr);
-			device_create_file(dev, &temp_min[i].dev_attr);
-			device_create_file(dev, &temp_max[i].dev_attr);
-			device_create_file(dev, &temp_crit[i].dev_attr);
-			device_create_file(dev, &temp_status[i].dev_attr);
-		}
-		device_create_file(dev, &dev_attr_alarms_temp);
-	}
-
-	if (data->innr = 14) {
-		for (i = 0; i < 3; i++) {
-			device_create_file(dev, &therm_input[i].dev_attr);
-			device_create_file(dev, &therm_min[i].dev_attr);
-			device_create_file(dev, &therm_max[i].dev_attr);
-			device_create_file(dev, &therm_crit[i].dev_attr);
-			device_create_file(dev, &therm_status[i].dev_attr);
+			if (device_create_file(dev, &temp_input[i].dev_attr)
+			    || device_create_file(dev, &temp_min[i].dev_attr)
+			    || device_create_file(dev, &temp_max[i].dev_attr)
+			    || device_create_file(dev, &temp_crit[i].dev_attr)
+			    || device_create_file(dev, &temp_status[i].dev_attr))
+
+				goto ERROR3;
 		}
+		if (device_create_file(dev, &dev_attr_alarms_temp))
+			goto ERROR3;
 	}
 
 	for (i = 0; i < data->fannr; i++) {
-		if (FAN_CONFIG_MONITOR(data->fan_conf, i)) {
-			device_create_file(dev, &fan_input[i].dev_attr);
-			device_create_file(dev, &fan_min[i].dev_attr);
-			device_create_file(dev, &fan_div[i].dev_attr);
-			device_create_file(dev, &fan_status[i].dev_attr);
-		}
-		if (FAN_CONFIG_CONTROL(data->fan_conf, i))
-			device_create_file(dev, &pwm[i].dev_attr);
+
+		if (FAN_CONFIG_CONTROL(data->fan_conf, i)
+		    && device_create_file(dev, &pwm[i].dev_attr))
+
+			goto ERROR3;
+
+		if (FAN_CONFIG_MONITOR(data->fan_conf, i)
+		    && (device_create_file(dev, &fan_input[i].dev_attr)
+			|| device_create_file(dev, &fan_min[i].dev_attr)
+			|| device_create_file(dev, &fan_div[i].dev_attr)
+			|| device_create_file(dev, &fan_status[i].dev_attr)))
+
+			goto ERROR3;
 	}
 
+	data->class_dev = hwmon_device_register(&client->dev);
+	if (IS_ERR(data->class_dev)) {
+		err = PTR_ERR(data->class_dev);
+		goto ERROR3;
+	}
 	return 0;
 
 ERROR3:
+	/* can still remove groups whose members were added individually */
+	sysfs_remove_group(&client->dev.kobj, &pc8736x_temp_group);
+	sysfs_remove_group(&client->dev.kobj, &pc8736x_fan_group);
+	sysfs_remove_group(&client->dev.kobj, &pc8736x_therm_group);
+	sysfs_remove_group(&client->dev.kobj, &pc8736x_vin_group);
+
 	i2c_detach_client(client);
 ERROR2:
-	for (i = 0; i < 3; i++) {
-		if (data->address[i]) {
+	for (i = 0; i < 3; i++)
+		if (data->address[i])
 			release_region(data->address[i], PC87360_EXTENT);
-		}
-	}
 ERROR1:
 	kfree(data);
 	return err;

[lm-sensors] [patch 3/3] pc87360 - fix unchecked

[Jean Delvare]

Hi Jim,

> 
> 3- rework sys-device-interface startup
>     use sysfs_create_group() for 2 sensor-types which are chip-model 
> invariant.
>        ie all-or-nothing attribute groups
>     other 2 groups vary too much due to configuration, etc,
>        so we keep the loops of device_create_file(), but now check their 
> returns.
> 
> Signed-off-by:   Jim Cromie  <jim.cromie at gmail.com>
> 
> [jimc at harpo hwmon-stuff]$ diffstat diff.ab-3.20060818.185837
>  pc87360.c |   90 
> +++++++++++++++++++++++++++++++-------------------------------
>  1 files changed, 46 insertions(+), 44 deletions(-)
> 
> 
> diff -ruNp -X dontdiff -X exclude-diffs ab-2/drivers/hwmon/pc87360.c ab-3/drivers/hwmon/pc87360.c
> --- ab-2/drivers/hwmon/pc87360.c	2006-08-18 17:40:35.000000000 -0600
> +++ ab-3/drivers/hwmon/pc87360.c	2006-08-18 18:54:24.000000000 -0600
> @@ -1017,67 +1017,69 @@ static int pc87360_detect(struct i2c_ada
>  		pc87360_init_client(client, use_thermistors);
>  	}
>  
> -	/* Register sysfs hooks */
> -	data->class_dev = hwmon_device_register(&client->dev);
> -	if (IS_ERR(data->class_dev)) {
> -		err = PTR_ERR(data->class_dev);
> +	/* Register all-or-nothing sysfs groups */
> +
> +	if (data->innr && 
> +	    sysfs_create_group(&client->dev.kobj,
> +			       &pc8736x_vin_group))
>  		goto ERROR3;
> -	}
>  
> -	if (data->innr) {
> -		for (i = 0; i < 11; i++) {
> -			device_create_file(dev, &in_input[i].dev_attr);
> -			device_create_file(dev, &in_min[i].dev_attr);
> -			device_create_file(dev, &in_max[i].dev_attr);
> -			device_create_file(dev, &in_status[i].dev_attr);
> -		}
> -		device_create_file(dev, &dev_attr_cpu0_vid);
> -		device_create_file(dev, &dev_attr_vrm);
> -		device_create_file(dev, &dev_attr_alarms_in);
> -	}
> +	if (data->innr = 14 &&
> +	    sysfs_create_group(&client->dev.kobj, 
> +			       &pc8736x_therm_group))
> +		goto ERROR3;
> +
> +	/* create device attr-files for varying sysfs groups */
>  
>  	if (data->tempnr) {
>  		for (i = 0; i < data->tempnr; i++) {
> -			device_create_file(dev, &temp_input[i].dev_attr);
> -			device_create_file(dev, &temp_min[i].dev_attr);
> -			device_create_file(dev, &temp_max[i].dev_attr);
> -			device_create_file(dev, &temp_crit[i].dev_attr);
> -			device_create_file(dev, &temp_status[i].dev_attr);
> -		}
> -		device_create_file(dev, &dev_attr_alarms_temp);
> -	}
> -
> -	if (data->innr = 14) {
> -		for (i = 0; i < 3; i++) {
> -			device_create_file(dev, &therm_input[i].dev_attr);
> -			device_create_file(dev, &therm_min[i].dev_attr);
> -			device_create_file(dev, &therm_max[i].dev_attr);
> -			device_create_file(dev, &therm_crit[i].dev_attr);
> -			device_create_file(dev, &therm_status[i].dev_attr);
> +			if (device_create_file(dev, &temp_input[i].dev_attr)
> +			    || device_create_file(dev, &temp_min[i].dev_attr)
> +			    || device_create_file(dev, &temp_max[i].dev_attr)
> +			    || device_create_file(dev, &temp_crit[i].dev_attr)
> +			    || device_create_file(dev, &temp_status[i].dev_attr))
> +
> +				goto ERROR3;

No space between condition and instruction, please.

>  		}
> +		if (device_create_file(dev, &dev_attr_alarms_temp))
> +			goto ERROR3;
>  	}
>  
>  	for (i = 0; i < data->fannr; i++) {
> -		if (FAN_CONFIG_MONITOR(data->fan_conf, i)) {
> -			device_create_file(dev, &fan_input[i].dev_attr);
> -			device_create_file(dev, &fan_min[i].dev_attr);
> -			device_create_file(dev, &fan_div[i].dev_attr);
> -			device_create_file(dev, &fan_status[i].dev_attr);
> -		}
> -		if (FAN_CONFIG_CONTROL(data->fan_conf, i))
> -			device_create_file(dev, &pwm[i].dev_attr);
> +
> +		if (FAN_CONFIG_CONTROL(data->fan_conf, i)
> +		    && device_create_file(dev, &pwm[i].dev_attr))
> +
> +			goto ERROR3;
> +
> +		if (FAN_CONFIG_MONITOR(data->fan_conf, i)
> +		    && (device_create_file(dev, &fan_input[i].dev_attr)
> +			|| device_create_file(dev, &fan_min[i].dev_attr)
> +			|| device_create_file(dev, &fan_div[i].dev_attr)
> +			|| device_create_file(dev, &fan_status[i].dev_attr)))
> +
> +			goto ERROR3;
>  	}
>  
> +	data->class_dev = hwmon_device_register(&client->dev);
> +	if (IS_ERR(data->class_dev)) {
> +		err = PTR_ERR(data->class_dev);
> +		goto ERROR3;
> +	}
>  	return 0;
>  
>  ERROR3:
> +	/* can still remove groups whose members were added individually */
> +	sysfs_remove_group(&client->dev.kobj, &pc8736x_temp_group);
> +	sysfs_remove_group(&client->dev.kobj, &pc8736x_fan_group);
> +	sysfs_remove_group(&client->dev.kobj, &pc8736x_therm_group);
> +	sysfs_remove_group(&client->dev.kobj, &pc8736x_vin_group);
> +
>  	i2c_detach_client(client);
>  ERROR2:
> -	for (i = 0; i < 3; i++) {
> -		if (data->address[i]) {
> +	for (i = 0; i < 3; i++)
> +		if (data->address[i])
>  			release_region(data->address[i], PC87360_EXTENT);
> -		}
> -	}

This change isn't really needed, and at any rate doesn't belong to this
patchset.

No need to resend, I fixed these minor issues and applied all three
patches, thanks.

-- 
Jean Delvare

[lm-sensors] [patch 3/3] pc87360 - fix unchecked

[Jean Delvare]

Hi Jim,

> > 3- rework sys-device-interface startup
> >     use sysfs_create_group() for 2 sensor-types which are chip-model invariant.
> >        ie all-or-nothing attribute groups
> >     other 2 groups vary too much due to configuration, etc,
> >        so we keep the loops of device_create_file(), but now check their returns.
> > 
> > Signed-off-by:   Jim Cromie  <jim.cromie at gmail.com>

I just noticed that you do not properly propagate the error value if a
device file creation actually fails:

> > diff -ruNp -X dontdiff -X exclude-diffs ab-2/drivers/hwmon/pc87360.c ab-3/drivers/hwmon/pc87360.c
> > --- ab-2/drivers/hwmon/pc87360.c	2006-08-18 17:40:35.000000000 -0600
> > +++ ab-3/drivers/hwmon/pc87360.c	2006-08-18 18:54:24.000000000 -0600

> > +	/* Register all-or-nothing sysfs groups */
> > +
> > +	if (data->innr && 
> > +	    sysfs_create_group(&client->dev.kobj,
> > +			       &pc8736x_vin_group))
> >  		goto ERROR3;

> >  	if (data->tempnr) {
> >  		for (i = 0; i < data->tempnr; i++) {
> > +			if (device_create_file(dev, &temp_input[i].dev_attr)
> > +			    || device_create_file(dev, &temp_min[i].dev_attr)
> > +			    || device_create_file(dev, &temp_max[i].dev_attr)
> > +			    || device_create_file(dev, &temp_crit[i].dev_attr)
> > +			    || device_create_file(dev, &temp_status[i].dev_attr))
> > +				goto ERROR3;

I've fixed that, modified patch is attached, I hope you like it.

-- 
Jean Delvare
-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: hwmon-unchecked-return-status-fixes-pc87360-3.patch
Url: http://lists.lm-sensors.org/pipermail/lm-sensors/attachments/20060923/de6416cf/attachment-0001.pl 

[lm-sensors] [patch 3/3] pc87360 - fix unchecked

[Jim Cromie]

Jean Delvare wrote:
> Hi Jim,
>
>   
>>> 3- rework sys-device-interface startup
>>>     use sysfs_create_group() for 2 sensor-types which are chip-model invariant.
>>>        ie all-or-nothing attribute groups
>>>     other 2 groups vary too much due to configuration, etc,
>>>        so we keep the loops of device_create_file(), but now check their returns.
>>>
>>> Signed-off-by:   Jim Cromie  <jim.cromie at gmail.com>
>>>       
>
> I just noticed that you do not properly propagate the error value if a
> device file creation actually fails:
>
>   
My timing was off.  I noted this too, and did a patch last nite. 
Was gonna stack them all up and test result this morning.
Ive now done so, using your patch, everything looks good :)

BTW, everything includes :
3 patches for pc87360 return-status-checks (already on your list)
1 vrm patch,
2 patches giving  pc87360 2D-callbacks,
3 superio-locks patches (add module, use in pc87360 & pc8736x_gpio)

Id like to get these into 18-mmX, for some value of X.
Do you have any thoughts on how and when this should happen ?
Im presuming it is sometime after the 19-rc0 submit window closes.


FWIW, my previously posted sensorck.pl shows this 'discrepancy', due to 
new vrm-vid patch..
(updated version attached)

$ sensorck.pl sensor-out
[10310.921653] hwmon-vid: requested unknown VRM version
check VPWR
check VCORE
check VCC
check Vbat
check Critical
check -12V
check Vsb
check Vdd
check +12V
check Temp
val out of range by 5.88%, 102 should be +108
check AVdd
check avi0
check GND
unknown sensors:
$VAR1 = {
  '          +0.000 V  (VRM Version 0.0)' => {
    'raw' => undef
  },
  'Voltage ID:' => {
    'raw' => undef
  }
};

$ sensorck.pl -h
Unknown option: h
usage: sensorck.pl [opts] <filename>
    -v  : verbose
    -r <5> : percentage difference (5% is default)
    -e  : verbose for errors
    -H  : more help

-------------- next part --------------
A non-text attachment was scrubbed...
Name: sensorck.pl
Type: application/x-perl
Size: 3743 bytes
Desc: not available
Url : http://lists.lm-sensors.org/pipermail/lm-sensors/attachments/20060923/1bd12843/attachment.bin 

[lm-sensors] [patch 3/3] pc87360 - fix unchecked

[Jean Delvare]

Hi Jim,

> BTW, everything includes :
> 3 patches for pc87360 return-status-checks (already on your list)
> 1 vrm patch,
> 2 patches giving  pc87360 2D-callbacks,
> 3 superio-locks patches (add module, use in pc87360 & pc8736x_gpio)
> 
> Id like to get these into 18-mmX, for some value of X.
> Do you have any thoughts on how and when this should happen ?
> Im presuming it is sometime after the 19-rc0 submit window closes.

Yeah, for now I am concentrating on pushing all "important" hwmon
patches to Greg for 2.6.19-rc1. This should happen tomorrow. This will
be a large patchset, which isn't ideal as the merge window is already
open, so I had to postpone some less important patches. But we really
want the unchecked return values to be fixed in 2.6.19.

For your own patches, this means the following:
* 3 patches for pc87360 return-status-checks
  -> 2.6.19-rc1
* 1 vrm patch
  -> 2.6.19-rc1-mm1
* 2 patches giving pc87360 2D-callbacks
  -> I need to review them first, I seem to remember that a quick look
  had me conclude "first OK, second not quite so". But I'm not so sure
  so I need to spend some more time on it.
* 3 superio-locks patches (add module, use in pc87360 & pc8736x_gpio)
  -> I already know I won't have time to handle this. I am busy enough
  with hwmon and i2c, I just can't work on yet another subsystem or I'm
  going to blow up into pieces.

-- 
Jean Delvare