[lm-sensors] [RFC PATCH 2.6.18-rc4-mm1] hwmon: unchecked return

[khali@linux-fr.org (Jean Delvare)]

Hi Mark,

> Added w83627hf, which should demonstrate how to do all the remaining
> hwmon drivers.  There is more (ab)use of sysfs_remove_group() here to
> make life easier.

Yeah, I like it. Your approach deals nicely with chip-dependent files
and it can be applied to all drivers as far as I can see.

> This patch was lightly tested against w83627thf.  Testing on the other
> variants supported by that driver would be nice.

I don't have any supported chip here (yet), sorry.

> --- linux-2.6.18-rc4-mm1.orig/drivers/hwmon/w83627hf.c
> +++ linux-2.6.18-rc4-mm1/drivers/hwmon/w83627hf.c

> @@ -1107,62 +1138,93 @@ static int w83627hf_detect(struct i2c_ad
>  	data->fan_min[1] = w83627hf_read_value(new_client, W83781D_REG_FAN_MIN(2));
>  	data->fan_min[2] = w83627hf_read_value(new_client, W83781D_REG_FAN_MIN(3));
>  
> -	/* Register sysfs hooks */
> -	data->class_dev = hwmon_device_register(&new_client->dev);
> -	if (IS_ERR(data->class_dev)) {
> -		err = PTR_ERR(data->class_dev);
> +	/* Register common device attributes */
> +	if ((err = sysfs_create_group(&new_client->dev.kobj, &w83627hf_group)))
>  		goto ERROR3;
> +
> +	/* Register chip-specific device attributes */
> +	if (kind != w83697hf) {
> +		if ((err = device_create_file(&new_client->dev,
> +					&dev_attr_in1_input)))
> +			goto ERROR4;
> +		if ((err = device_create_file(&new_client->dev,
> +					&dev_attr_in1_min)))
> +			goto ERROR4;
> +		if ((err = device_create_file(&new_client->dev,
> +					&dev_attr_in1_max)))
> +			goto ERROR4;
>  	}

What about:

	if (kind != w83697hf) {
		if ((err = device_create_file(&new_client->dev,
					&dev_attr_in1_input))
		 || (err = device_create_file(&new_client->dev,
					&dev_attr_in1_min))
		 || (err = device_create_file(&new_client->dev,
					&dev_attr_in1_max)))
			goto ERROR4;
	}

More concise and it works just as fine, doesn't it? Same applies below.

Maybe you can also group the two (kind != w83697hf) blocks?

>  
> -	device_create_file_in(new_client, 0);
> -	if (kind != w83697hf)
> -		device_create_file_in(new_client, 1);
> -	device_create_file_in(new_client, 2);
> -	device_create_file_in(new_client, 3);
> -	device_create_file_in(new_client, 4);
>  	if (kind = w83627hf || kind = w83697hf) {
> -		device_create_file_in(new_client, 5);
> -		device_create_file_in(new_client, 6);
> +		if ((err = device_create_file(&new_client->dev,
> +					&dev_attr_in5_input)))
> +			goto ERROR4;
> +		if ((err = device_create_file(&new_client->dev,
> +					&dev_attr_in5_min)))
> +			goto ERROR4;
> +		if ((err = device_create_file(&new_client->dev,
> +					&dev_attr_in5_max)))
> +			goto ERROR4;
> +		if ((err = device_create_file(&new_client->dev,
> +					&dev_attr_in6_input)))
> +			goto ERROR4;
> +		if ((err = device_create_file(&new_client->dev,
> +					&dev_attr_in6_min)))
> +			goto ERROR4;
> +		if ((err = device_create_file(&new_client->dev,
> +					&dev_attr_in6_max)))
> +			goto ERROR4;
> +	}
> +
> +	if (kind != w83697hf) {
> +		if ((err = device_create_file(&new_client->dev,
> +					&dev_attr_fan3_input)))
> +			goto ERROR4;
> +		if ((err = device_create_file(&new_client->dev,
> +					&dev_attr_fan3_min)))
> +			goto ERROR4;
> +		if ((err = device_create_file(&new_client->dev,
> +					&dev_attr_fan3_div)))
> +			goto ERROR4;
> +		if ((err = device_create_file(&new_client->dev,
> +					&dev_attr_temp3_input)))
> +			goto ERROR4;
> +		if ((err = device_create_file(&new_client->dev,
> +					&dev_attr_temp3_max)))
> +			goto ERROR4;
> +		if ((err = device_create_file(&new_client->dev,
> +					&dev_attr_temp3_max_hyst)))
> +			goto ERROR4;
> +		if ((err = device_create_file(&new_client->dev,
> +					&dev_attr_temp3_type)))
> +			goto ERROR4;
>  	}
> -	device_create_file_in(new_client, 7);
> -	device_create_file_in(new_client, 8);
> -
> -	device_create_file_fan(new_client, 1);
> -	device_create_file_fan(new_client, 2);
> -	if (kind != w83697hf)
> -		device_create_file_fan(new_client, 3);
> -
> -	device_create_file_temp(new_client, 1);
> -	device_create_file_temp(new_client, 2);
> -	if (kind != w83697hf)
> -		device_create_file_temp(new_client, 3);
>  
>  	if (kind != w83697hf && data->vid != 0xff) {
> -		device_create_file_vid(new_client);
> -		device_create_file_vrm(new_client);
> +		if ((err = device_create_file(&new_client->dev,
> +					&dev_attr_cpu0_vid)))
> +			goto ERROR4;
> +		if ((err = device_create_file(&new_client->dev,
> +					&dev_attr_vrm)))
> +			goto ERROR4;
>  	}
>  
> -	device_create_file_fan_div(new_client, 1);
> -	device_create_file_fan_div(new_client, 2);
> -	if (kind != w83697hf)
> -		device_create_file_fan_div(new_client, 3);
> -
> -	device_create_file_alarms(new_client);
> -
> -	device_create_file_beep(new_client);
> -
> -	device_create_file_pwm(new_client, 1);
> -	device_create_file_pwm(new_client, 2);
>  	if (kind = w83627thf || kind = w83637hf || kind = w83687thf)
> -		device_create_file_pwm(new_client, 3);
> +		if ((err = device_create_file(&new_client->dev,
> +					&dev_attr_pwm3)))
> +			goto ERROR4;
>  
> -	device_create_file_sensor(new_client, 1);
> -	device_create_file_sensor(new_client, 2);
> -	if (kind != w83697hf)
> -		device_create_file_sensor(new_client, 3);
> +	data->class_dev = hwmon_device_register(&new_client->dev);
> +	if (IS_ERR(data->class_dev)) {
> +		err = PTR_ERR(data->class_dev);
> +		goto ERROR4;
> +	}
>  
>  	return 0;
>  
> +      ERROR4:
> +	sysfs_remove_group(&new_client->dev.kobj, &w83627hf_group);
> +	sysfs_remove_group(&new_client->dev.kobj, &w83627hf_group_opt);
>        ERROR3:
>  	i2c_detach_client(new_client);
>        ERROR2:

Thanks,
-- 
Jean Delvare