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

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

[Mark M. Hoffman]

Hi all:

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.

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

I'll add lm75, lm78, and smsc47b397 to this patch as I have time.
Jean: don't apply yet.

---

This patch fixes up some hwmon drivers so that they no longer ignore
return status from device_create_file().

Signed-off-by: Mark M. Hoffman <mhoffman at lightlink.com>

---
 drivers/hwmon/asb100.c   |  122 +++++++++++++----------
 drivers/hwmon/w83627hf.c |  245 +++++++++++++++++++++++++++++------------------
 2 files changed, 223 insertions(+), 144 deletions(-)

--- linux-2.6.18-rc4-mm1.orig/drivers/hwmon/asb100.c
+++ linux-2.6.18-rc4-mm1/drivers/hwmon/asb100.c
@@ -298,12 +298,6 @@ sysfs_in(4);
 sysfs_in(5);
 sysfs_in(6);
 
-#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); \
-} while (0)
-
 /* 3 Fans */
 static ssize_t show_fan(struct device *dev, char *buf, int nr)
 {
@@ -421,12 +415,6 @@ sysfs_fan(1);
 sysfs_fan(2);
 sysfs_fan(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); \
-	device_create_file(&client->dev, &dev_attr_fan##offset##_div); \
-} while (0)
-
 /* 4 Temp. Sensors */
 static int sprintf_temp_from_reg(u16 reg, char *buf, int nr)
 {
@@ -515,12 +503,6 @@ sysfs_temp(3);
 sysfs_temp(4);
 
 /* VID */
-#define device_create_file_temp(client, num) do { \
-	device_create_file(&client->dev, &dev_attr_temp##num##_input); \
-	device_create_file(&client->dev, &dev_attr_temp##num##_max); \
-	device_create_file(&client->dev, &dev_attr_temp##num##_max_hyst); \
-} while (0)
-
 static ssize_t show_vid(struct device *dev, struct device_attribute *attr, char *buf)
 {
 	struct asb100_data *data = asb100_update_device(dev);
@@ -528,8 +510,6 @@ static ssize_t show_vid(struct device *d
 }
 
 static DEVICE_ATTR(cpu0_vid, S_IRUGO, show_vid, NULL);
-#define device_create_file_vid(client) \
-device_create_file(&client->dev, &dev_attr_cpu0_vid)
 
 /* VRM */
 static ssize_t show_vrm(struct device *dev, struct device_attribute *attr, char *buf)
@@ -549,8 +529,6 @@ static ssize_t set_vrm(struct device *de
 
 /* Alarms */
 static DEVICE_ATTR(vrm, S_IRUGO | S_IWUSR, show_vrm, set_vrm);
-#define device_create_file_vrm(client) \
-device_create_file(&client->dev, &dev_attr_vrm);
 
 static ssize_t show_alarms(struct device *dev, struct device_attribute *attr, char *buf)
 {
@@ -559,8 +537,6 @@ static ssize_t show_alarms(struct device
 }
 
 static DEVICE_ATTR(alarms, S_IRUGO, show_alarms, NULL);
-#define device_create_file_alarms(client) \
-device_create_file(&client->dev, &dev_attr_alarms)
 
 /* 1 PWM */
 static ssize_t show_pwm1(struct device *dev, struct device_attribute *attr, char *buf)
@@ -607,10 +583,65 @@ static ssize_t set_pwm_enable1(struct de
 static DEVICE_ATTR(pwm1, S_IRUGO | S_IWUSR, show_pwm1, set_pwm1);
 static DEVICE_ATTR(pwm1_enable, S_IRUGO | S_IWUSR,
 		show_pwm_enable1, set_pwm_enable1);
-#define device_create_file_pwm1(client) do { \
-	device_create_file(&new_client->dev, &dev_attr_pwm1); \
-	device_create_file(&new_client->dev, &dev_attr_pwm1_enable); \
-} while (0)
+
+static struct attribute *asb100_attributes[] = {
+	&dev_attr_in0_input.attr,
+	&dev_attr_in0_min.attr,
+	&dev_attr_in0_max.attr,
+	&dev_attr_in1_input.attr,
+	&dev_attr_in1_min.attr,
+	&dev_attr_in1_max.attr,
+	&dev_attr_in2_input.attr,
+	&dev_attr_in2_min.attr,
+	&dev_attr_in2_max.attr,
+	&dev_attr_in3_input.attr,
+	&dev_attr_in3_min.attr,
+	&dev_attr_in3_max.attr,
+	&dev_attr_in4_input.attr,
+	&dev_attr_in4_min.attr,
+	&dev_attr_in4_max.attr,
+	&dev_attr_in5_input.attr,
+	&dev_attr_in5_min.attr,
+	&dev_attr_in5_max.attr,
+	&dev_attr_in6_input.attr,
+	&dev_attr_in6_min.attr,
+	&dev_attr_in6_max.attr,
+
+	&dev_attr_fan1_input.attr,
+	&dev_attr_fan1_min.attr,
+	&dev_attr_fan1_div.attr,
+	&dev_attr_fan2_input.attr,
+	&dev_attr_fan2_min.attr,
+	&dev_attr_fan2_div.attr,
+	&dev_attr_fan3_input.attr,
+	&dev_attr_fan3_min.attr,
+	&dev_attr_fan3_div.attr,
+
+	&dev_attr_temp1_input.attr,
+	&dev_attr_temp1_max.attr,
+	&dev_attr_temp1_max_hyst.attr,
+	&dev_attr_temp2_input.attr,
+	&dev_attr_temp2_max.attr,
+	&dev_attr_temp2_max_hyst.attr,
+	&dev_attr_temp3_input.attr,
+	&dev_attr_temp3_max.attr,
+	&dev_attr_temp3_max_hyst.attr,
+	&dev_attr_temp4_input.attr,
+	&dev_attr_temp4_max.attr,
+	&dev_attr_temp4_max_hyst.attr,
+
+	&dev_attr_cpu0_vid.attr,
+	&dev_attr_vrm.attr,
+	&dev_attr_alarms.attr,
+	&dev_attr_pwm1.attr,
+	&dev_attr_pwm1_enable.attr,
+
+	NULL,
+};
+
+static struct attribute_group asb100_group = {
+	.attrs = asb100_attributes,
+};
 
 /* This function is called when:
 	asb100_driver is inserted (when this module is loaded), for each
@@ -810,38 +841,19 @@ static int asb100_detect(struct i2c_adap
 	data->fan_min[2] = asb100_read_value(new_client, ASB100_REG_FAN_MIN(2));
 
 	/* Register sysfs hooks */
+	if ((err = sysfs_create_group(&new_client->dev.kobj, &asb100_group)))
+		goto ERROR3;
+
 	data->class_dev = hwmon_device_register(&new_client->dev);
 	if (IS_ERR(data->class_dev)) {
 		err = PTR_ERR(data->class_dev);
-		goto ERROR3;
+		goto ERROR4;
 	}
 
-	device_create_file_in(new_client, 0);
-	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);
-	device_create_file_in(new_client, 5);
-	device_create_file_in(new_client, 6);
-
-	device_create_file_fan(new_client, 1);
-	device_create_file_fan(new_client, 2);
-	device_create_file_fan(new_client, 3);
-
-	device_create_file_temp(new_client, 1);
-	device_create_file_temp(new_client, 2);
-	device_create_file_temp(new_client, 3);
-	device_create_file_temp(new_client, 4);
-
-	device_create_file_vid(new_client);
-	device_create_file_vrm(new_client);
-
-	device_create_file_alarms(new_client);
-
-	device_create_file_pwm1(new_client);
-
 	return 0;
 
+ERROR4:
+	sysfs_remove_group(&new_client->dev.kobj, &asb100_group);
 ERROR3:
 	i2c_detach_client(data->lm75[1]);
 	i2c_detach_client(data->lm75[0]);
@@ -861,8 +873,10 @@ static int asb100_detach_client(struct i
 	int err;
 
 	/* main client */
-	if (data)
+	if (data) {
 		hwmon_device_unregister(data->class_dev);
+		sysfs_remove_group(&client->dev.kobj, &asb100_group);
+	}
 
 	if ((err = i2c_detach_client(client)))
 		return err;
--- linux-2.6.18-rc4-mm1.orig/drivers/hwmon/w83627hf.c
+++ linux-2.6.18-rc4-mm1/drivers/hwmon/w83627hf.c
@@ -511,13 +511,6 @@ static DEVICE_ATTR(in0_min, S_IRUGO | S_
 static DEVICE_ATTR(in0_max, S_IRUGO | S_IWUSR,
 	show_regs_in_max0, store_regs_in_max0);
 
-#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); \
-} while (0)
-
 #define show_fan_reg(reg) \
 static ssize_t show_##reg (struct device *dev, char *buf, int nr) \
 { \
@@ -575,12 +568,6 @@ sysfs_fan_min_offset(2);
 sysfs_fan_offset(3);
 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); \
-} while (0)
-
 #define show_temp_reg(reg) \
 static ssize_t show_##reg (struct device *dev, char *buf, int nr) \
 { \
@@ -655,13 +642,6 @@ sysfs_temp_offsets(1);
 sysfs_temp_offsets(2);
 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); \
-} while (0)
-
 static ssize_t
 show_vid_reg(struct device *dev, struct device_attribute *attr, char *buf)
 {
@@ -669,8 +649,6 @@ show_vid_reg(struct device *dev, struct 
 	return sprintf(buf, "%ld\n", (long) vid_from_reg(data->vid, data->vrm));
 }
 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)
 
 static ssize_t
 show_vrm_reg(struct device *dev, struct device_attribute *attr, char *buf)
@@ -691,8 +669,6 @@ store_vrm_reg(struct device *dev, struct
 	return count;
 }
 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)
 
 static ssize_t
 show_alarms_reg(struct device *dev, struct device_attribute *attr, char *buf)
@@ -701,8 +677,6 @@ show_alarms_reg(struct device *dev, stru
 	return sprintf(buf, "%ld\n", (long) data->alarms);
 }
 static DEVICE_ATTR(alarms, S_IRUGO, show_alarms_reg, NULL);
-#define device_create_file_alarms(client) \
-device_create_file(&client->dev, &dev_attr_alarms)
 
 #define show_beep_reg(REG, reg) \
 static ssize_t show_beep_##reg (struct device *dev, struct device_attribute *attr, char *buf) \
@@ -765,12 +739,6 @@ static DEVICE_ATTR(beep_##reg, S_IRUGO |
 sysfs_beep(ENABLE, enable);
 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); \
-} while (0)
-
 static ssize_t
 show_fan_div_reg(struct device *dev, char *buf, int nr)
 {
@@ -836,11 +804,6 @@ sysfs_fan_div(1);
 sysfs_fan_div(2);
 sysfs_fan_div(3);
 
-#define device_create_file_fan_div(client, offset) \
-do { \
-device_create_file(&client->dev, &dev_attr_fan##offset##_div); \
-} while (0)
-
 static ssize_t
 show_pwm_reg(struct device *dev, char *buf, int nr)
 {
@@ -895,11 +858,6 @@ sysfs_pwm(1);
 sysfs_pwm(2);
 sysfs_pwm(3);
 
-#define device_create_file_pwm(client, offset) \
-do { \
-device_create_file(&client->dev, &dev_attr_pwm##offset); \
-} while (0)
-
 static ssize_t
 show_sensor_reg(struct device *dev, char *buf, int nr)
 {
@@ -971,12 +929,6 @@ sysfs_sensor(1);
 sysfs_sensor(2);
 sysfs_sensor(3);
 
-#define device_create_file_sensor(client, offset) \
-do { \
-device_create_file(&client->dev, &dev_attr_temp##offset##_type); \
-} while (0)
-
-
 static int __init w83627hf_find(int sioaddr, unsigned short *addr)
 {
 	u16 val;
@@ -1008,6 +960,85 @@ static int __init w83627hf_find(int sioa
 	return 0;
 }
 
+static struct attribute *w83627hf_attributes[] = {
+	&dev_attr_in0_input.attr,
+	&dev_attr_in0_min.attr,
+	&dev_attr_in0_max.attr,
+	&dev_attr_in2_input.attr,
+	&dev_attr_in2_min.attr,
+	&dev_attr_in2_max.attr,
+	&dev_attr_in3_input.attr,
+	&dev_attr_in3_min.attr,
+	&dev_attr_in3_max.attr,
+	&dev_attr_in4_input.attr,
+	&dev_attr_in4_min.attr,
+	&dev_attr_in4_max.attr,
+	&dev_attr_in7_input.attr,
+	&dev_attr_in7_min.attr,
+	&dev_attr_in7_max.attr,
+	&dev_attr_in8_input.attr,
+	&dev_attr_in8_min.attr,
+	&dev_attr_in8_max.attr,
+
+	&dev_attr_fan1_input.attr,
+	&dev_attr_fan1_min.attr,
+	&dev_attr_fan1_div.attr,
+	&dev_attr_fan2_input.attr,
+	&dev_attr_fan2_min.attr,
+	&dev_attr_fan2_div.attr,
+
+	&dev_attr_temp1_input.attr,
+	&dev_attr_temp1_max.attr,
+	&dev_attr_temp1_max_hyst.attr,
+	&dev_attr_temp1_type.attr,
+	&dev_attr_temp2_input.attr,
+	&dev_attr_temp2_max.attr,
+	&dev_attr_temp2_max_hyst.attr,
+	&dev_attr_temp2_type.attr,
+
+	&dev_attr_alarms.attr,
+	&dev_attr_beep_enable.attr,
+	&dev_attr_beep_mask.attr,
+
+	&dev_attr_pwm1.attr,
+	&dev_attr_pwm2.attr,
+
+	NULL
+};
+
+static struct attribute_group w83627hf_group = {
+	.attrs = w83627hf_attributes,
+};
+
+static struct attribute *w83627hf_attributes_opt[] = {
+	&dev_attr_in1_input.attr,
+	&dev_attr_in1_min.attr,
+	&dev_attr_in1_max.attr,
+	&dev_attr_in5_input.attr,
+	&dev_attr_in5_min.attr,
+	&dev_attr_in5_max.attr,
+	&dev_attr_in6_input.attr,
+	&dev_attr_in6_min.attr,
+	&dev_attr_in6_max.attr,
+
+	&dev_attr_fan3_input.attr,
+	&dev_attr_fan3_min.attr,
+	&dev_attr_fan3_div.attr,
+
+	&dev_attr_temp3_input.attr,
+	&dev_attr_temp3_max.attr,
+	&dev_attr_temp3_max_hyst.attr,
+	&dev_attr_temp3_type.attr,
+
+	&dev_attr_pwm3.attr,
+
+	NULL
+};
+
+static struct attribute_group w83627hf_group_opt = {
+	.attrs = w83627hf_attributes_opt,
+};
+
 static int w83627hf_detect(struct i2c_adapter *adapter)
 {
 	int val, kind;
@@ -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;
 	}
 
-	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:
@@ -1180,6 +1242,9 @@ static int w83627hf_detach_client(struct
 
 	hwmon_device_unregister(data->class_dev);
 
+	sysfs_remove_group(&client->dev.kobj, &w83627hf_group);
+	sysfs_remove_group(&client->dev.kobj, &w83627hf_group_opt);
+
 	if ((err = i2c_detach_client(client)))
 		return err;
 
-- 
Mark M. Hoffman
mhoffman at lightlink.com

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

[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

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

[David Hubbard]

Hi Mark, Jean,

Jean, I have a request to make of the I2C mailing list. See below...

On 8/14/06, Jean Delvare <khali at linux-fr.org> wrote:
> 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.

The w83627ehf driver uses struct sensor_device_attribute instead of
struct device_attribute, and we've put them in arrays, so if I created
a struct attribute *w83627ehf_attributes, it would contain something
like &sda_sf3_arrays[0].device_attr, &sda_sf3_arrays[1].device_attr,
...

I think that would be ugly code. It would allow me to call
sysfs_create_group() just like Mark does, which I would do if I could
find a way. Is there a sysfs_create_* function that takes
sensor_device_attribute arrays instead? If I read correctly, no. :-(


@Jean:

I added some return value checks in w83627ehf_detect(). This is called
from i2c_isa_add_driver using driver->attach_adapter(&isa_adapter). So
I returned an error from w83627ehf_detect(), and was surprised to see
that the module silently succeeded, without fully creating the
w83627ehf sysfs interface. I looked deeper and found that
i2c_isa_add_driver() does not check the return value when calling its
driver->attach_adapter(). i2c_isa_add_driver() is called from
sensors_w83627ehf_init(), which is the module_init function.

I have attached a very short patch for drivers/i2c/busses/i2c-isa.c
which should fix the problem. Instead of returning 0, it returns the
result of driver->attach_adapter(). However, this may cause a great
deal of unexpected grief, considering how many drivers use this
function. So now I can either subscribe to the I2C mailing list for
this (I'd rather not), or I can hand you the patch and ask you to talk
to the I2C developers (easy for me, but...).

Which should it be? If you can pass it on to the I2C developers, I'll
continue with the w83627ehf driver and to check for errors from
device_create_file().

Thanks!
David
-------------- next part --------------
A non-text attachment was scrubbed...
Name: i2c-isa_return_attach_adapter.patch
Type: application/octet-stream
Size: 629 bytes
Desc: not available
Url : http://lists.lm-sensors.org/pipermail/lm-sensors/attachments/20060814/7bd3d8ec/attachment.obj 

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

[Mark M. Hoffman]

Hi David:

* David Hubbard <david.c.hubbard at gmail.com> [2006-08-14 17:09:47 -0700]:
> Hi Mark, Jean,
> 
> Jean, I have a request to make of the I2C mailing list. See below...
> 
> On 8/14/06, Jean Delvare <khali at linux-fr.org> wrote:
> >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.
> 
> The w83627ehf driver uses struct sensor_device_attribute instead of
> struct device_attribute, and we've put them in arrays, so if I created

The only reason to put them in arrays in the first place was to make
registration easier.  For w83627ehf, I would do this...

-static struct sensor_device_attribute sda_in_input[] = {
-	SENSOR_ATTR(in0_input, S_IRUGO, show_in, NULL, 0),
-	SENSOR_ATTR(in1_input, S_IRUGO, show_in, NULL, 1),
-	SENSOR_ATTR(in2_input, S_IRUGO, show_in, NULL, 2),
-	SENSOR_ATTR(in3_input, S_IRUGO, show_in, NULL, 3),
-	SENSOR_ATTR(in4_input, S_IRUGO, show_in, NULL, 4),
-	SENSOR_ATTR(in5_input, S_IRUGO, show_in, NULL, 5),
-	SENSOR_ATTR(in6_input, S_IRUGO, show_in, NULL, 6),
-	SENSOR_ATTR(in7_input, S_IRUGO, show_in, NULL, 7),
-	SENSOR_ATTR(in8_input, S_IRUGO, show_in, NULL, 8),
-	SENSOR_ATTR(in9_input, S_IRUGO, show_in, NULL, 9),
-};
+static SENSOR_DEVICE_ATTR(in0_input, S_IRUGO, show_in, NULL, 0);
+static SENSOR_DEVICE_ATTR(in1_input, S_IRUGO, show_in, NULL, 1);
+static SENSOR_DEVICE_ATTR(in2_input, S_IRUGO, show_in, NULL, 2);
+static SENSOR_DEVICE_ATTR(in3_input, S_IRUGO, show_in, NULL, 3);
+static SENSOR_DEVICE_ATTR(in4_input, S_IRUGO, show_in, NULL, 4);
+static SENSOR_DEVICE_ATTR(in5_input, S_IRUGO, show_in, NULL, 5);
+static SENSOR_DEVICE_ATTR(in6_input, S_IRUGO, show_in, NULL, 6);
+static SENSOR_DEVICE_ATTR(in7_input, S_IRUGO, show_in, NULL, 7);
+static SENSOR_DEVICE_ATTR(in8_input, S_IRUGO, show_in, NULL, 8);
+static SENSOR_DEVICE_ATTR(in9_input, S_IRUGO, show_in, NULL, 9);
 
> a struct attribute *w83627ehf_attributes, it would contain something
> like &sda_sf3_arrays[0].device_attr, &sda_sf3_arrays[1].device_attr,

Given the above, it would be &dev_attr_in0_input.dev_attr.attr.

> I think that would be ugly code. It would allow me to call
> sysfs_create_group() just like Mark does, which I would do if I could
> find a way. Is there a sysfs_create_* function that takes
> sensor_device_attribute arrays instead? If I read correctly, no. :-(

Of course we could create one and put it into the hwmon module.  Not sure
if I like that idea though.  It won't save any code, and I don't think it
would save any time either.

> @Jean:
> 
> I added some return value checks in w83627ehf_detect(). This is called
> from i2c_isa_add_driver using driver->attach_adapter(&isa_adapter). So
> I returned an error from w83627ehf_detect(), and was surprised to see
> that the module silently succeeded, without fully creating the
> w83627ehf sysfs interface. I looked deeper and found that
> i2c_isa_add_driver() does not check the return value when calling its
> driver->attach_adapter(). i2c_isa_add_driver() is called from
> sensors_w83627ehf_init(), which is the module_init function.
> 
> I have attached a very short patch for drivers/i2c/busses/i2c-isa.c
> which should fix the problem. Instead of returning 0, it returns the
> result of driver->attach_adapter(). However, this may cause a great
> deal of unexpected grief, considering how many drivers use this
> function. So now I can either subscribe to the I2C mailing list for
> this (I'd rather not), or I can hand you the patch and ask you to talk
> to the I2C developers (easy for me, but...).

I2C developers = mostly Jean, increasingly me too.  Anyway, i2c-isa is
not used by anyone except hwmon, so it's just as well to discuss it here.

Jean: I thought you already had such a patch in your queue?  Now I can't
seem to find it.

> Which should it be? If you can pass it on to the I2C developers, I'll
> continue with the w83627ehf driver and to check for errors from
> device_create_file().

Regards,

-- 
Mark M. Hoffman
mhoffman at lightlink.com

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

[Mark M. Hoffman]

Hi Jean:

* Jean Delvare <khali at linux-fr.org> [2006-08-14 15:02:21 +0200]:
> 	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.

Agreed.

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

I already combined one (compared to original code)... so, yeah.

Regards,

-- 
Mark M. Hoffman
mhoffman at lightlink.com

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

[Jean Delvare]

Hi David,

> I added some return value checks in w83627ehf_detect(). This is called
> from i2c_isa_add_driver using driver->attach_adapter(&isa_adapter). So
> I returned an error from w83627ehf_detect(), and was surprised to see
> that the module silently succeeded, without fully creating the
> w83627ehf sysfs interface. I looked deeper and found that
> i2c_isa_add_driver() does not check the return value when calling its
> driver->attach_adapter(). i2c_isa_add_driver() is called from
> sensors_w83627ehf_init(), which is the module_init function.

Good catch, we should indeed check the return value.

This code was copied from i2c-core, where the value returned by
driver->attach_adapter() is ignored as well. There's even a comment:
"We ignore the return code; if it fails, too bad". Sigh. The problem in
i2c-core is that attach_adapter() will be called in a loop, on each
available driver (when calling i2c_add_adapter) or each available
adapter (when calling i2c_add_driver). We can't stop if one call fail,
that wouldn't be fair for the others which may still succeed. The proper
fix is to switch to the device driver model...

In the i2c-isa case things are much more simple, we have a single call
so failing on error sounds good.

> I have attached a very short patch for drivers/i2c/busses/i2c-isa.c
> which should fix the problem. Instead of returning 0, it returns the
> result of driver->attach_adapter().

> diff -ur linux-2.6.18-rc4/drivers/i2c/busses/i2c-isa.c linux-2.6.18-rc4/drivers/i2c/busses/i2c-isa.c
> --- linux-2.6.18-rc4/drivers/i2c/busses/i2c-isa.c	2006-08-14 17:48:28.000000000 -0700
> +++ linux-2.6.18-rc4/drivers/i2c/busses/i2c-isa.c	2006-08-14 17:52:04.000000000 -0700
> @@ -89,9 +89,7 @@
>  	dev_dbg(&isa_adapter.dev, "Driver %s registered\n", driver->driver.name);
>  
>  	/* Now look for clients */
> -	driver->attach_adapter(&isa_adapter);
> -
> -	return 0;
> +	return driver->attach_adapter(&isa_adapter);
>  }

That's not enough, unfortunately. In case of error you must unregister
the driver before returning. I also think we should print some message
on error. Care to send an updated patch?

> However, this may cause a great deal of unexpected grief, considering
> how many drivers use this function.

Not that many (11), and they are all hardware monitoring drivers.

-- 
Jean Delvare

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

[Jean Delvare]

Mark, David,

> > The w83627ehf driver uses struct sensor_device_attribute instead of
> > struct device_attribute, and we've put them in arrays, so if I created
> 
> The only reason to put them in arrays in the first place was to make
> registration easier.  For w83627ehf, I would do this...
> 
> -static struct sensor_device_attribute sda_in_input[] = {
> -	SENSOR_ATTR(in0_input, S_IRUGO, show_in, NULL, 0),
> -	SENSOR_ATTR(in1_input, S_IRUGO, show_in, NULL, 1),
> -	SENSOR_ATTR(in2_input, S_IRUGO, show_in, NULL, 2),
> -	SENSOR_ATTR(in3_input, S_IRUGO, show_in, NULL, 3),
> -	SENSOR_ATTR(in4_input, S_IRUGO, show_in, NULL, 4),
> -	SENSOR_ATTR(in5_input, S_IRUGO, show_in, NULL, 5),
> -	SENSOR_ATTR(in6_input, S_IRUGO, show_in, NULL, 6),
> -	SENSOR_ATTR(in7_input, S_IRUGO, show_in, NULL, 7),
> -	SENSOR_ATTR(in8_input, S_IRUGO, show_in, NULL, 8),
> -	SENSOR_ATTR(in9_input, S_IRUGO, show_in, NULL, 9),
> -};
> +static SENSOR_DEVICE_ATTR(in0_input, S_IRUGO, show_in, NULL, 0);
> +static SENSOR_DEVICE_ATTR(in1_input, S_IRUGO, show_in, NULL, 1);
> +static SENSOR_DEVICE_ATTR(in2_input, S_IRUGO, show_in, NULL, 2);
> +static SENSOR_DEVICE_ATTR(in3_input, S_IRUGO, show_in, NULL, 3);
> +static SENSOR_DEVICE_ATTR(in4_input, S_IRUGO, show_in, NULL, 4);
> +static SENSOR_DEVICE_ATTR(in5_input, S_IRUGO, show_in, NULL, 5);
> +static SENSOR_DEVICE_ATTR(in6_input, S_IRUGO, show_in, NULL, 6);
> +static SENSOR_DEVICE_ATTR(in7_input, S_IRUGO, show_in, NULL, 7);
> +static SENSOR_DEVICE_ATTR(in8_input, S_IRUGO, show_in, NULL, 8);
> +static SENSOR_DEVICE_ATTR(in9_input, S_IRUGO, show_in, NULL, 9);

Yep. The driver core wants arrays of pointers, rather that arrays of
structures. We have to adapt.

> > a struct attribute *w83627ehf_attributes, it would contain something
> > like &sda_sf3_arrays[0].device_attr, &sda_sf3_arrays[1].device_attr,
> 
> Given the above, it would be &dev_attr_in0_input.dev_attr.attr.
> 
> > I think that would be ugly code. It would allow me to call
> > sysfs_create_group() just like Mark does, which I would do if I could
> > find a way. Is there a sysfs_create_* function that takes
> > sensor_device_attribute arrays instead? If I read correctly, no. :-(
> 
> Of course we could create one and put it into the hwmon module.  Not sure
> if I like that idea though.  It won't save any code, and I don't think it
> would save any time either.

There are currently 3 possible structures for device attributes in
hardware monitoring drivers, depending on whether they take 0, 1 or 2
parameters. Some drivers mix the three types. Having helpers for arrays
of structures would mean 3 different function sets in the hwmon driver,
and some drivers would have to call the three of them. This is going to
make things much more complex in some cases.

So I'd rather stick to what the driver core offers. Arrays of pointers
make it possible to handle all attributes in a single array, regardless
of their "original type".

And indeed, the few drivers which had been using arrays for attributes
will have to change. I'm sorry about that, but this seems to be the
best move in the long run.

Now, the usual rules apply. If someone comes up with real working code
and real world numbers which prove me wrong, the discussion reopens.

> I2C developers = mostly Jean, increasingly me too.  Anyway, i2c-isa is
> not used by anyone except hwmon, so it's just as well to discuss it here.

Yup, in fact i2c-isa should be in drivers/hwmon. I didn't bother moving
it only because it was planned for removal anyway.

> Jean: I thought you already had such a patch in your queue?  Now I can't
> seem to find it.

No, I don't. Maybe you are thinking of your own "i2c algorithm drivers
don't propagate bus creation error" patch [1] or my work-in-progress
patch addressing the __must_check warnings in i2c-isa [2]. But the
specific bug reported by David is new to me.

[1] http://www.kernel.org/git/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commit;h³9ad0cf7c19fc14e8f823b1b36245f7a3711655
[2] http://khali.linux-fr.org/devel/i2c/linux-2.6/i2c-core-__must_check-fixes.patch

Thanks,
-- 
Jean Delvare

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

[David Hubbard]

Hi Jean,

> That's not enough, unfortunately. In case of error you must unregister
> the driver before returning. I also think we should print some message
> on error. Care to send an updated patch?

Updated patch attached. I have tested it on my machine and everything
is working, but if other hwmon drivers use this, can they apply the
patch on their machine and see if the modules fail to load?

David
-------------- next part --------------
A non-text attachment was scrubbed...
Name: i2c-isa_return_attach_adapter.patch
Type: application/octet-stream
Size: 815 bytes
Desc: not available
Url : http://lists.lm-sensors.org/pipermail/lm-sensors/attachments/20060816/f6006eed/attachment.obj 

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

[Jean Delvare]

Hi David,

> > That's not enough, unfortunately. In case of error you must unregister
> > the driver before returning. I also think we should print some message
> > on error. Care to send an updated patch?
> 
> Updated patch attached. I have tested it on my machine and everything
> is working, but if other hwmon drivers use this, can they apply the
> patch on their machine and see if the modules fail to load?

Thanks for the update. I tested this with the it87 driver and it worked
as designed, preventing the driver to load if an error occurs.

Unfortunately you forgot to sign your patch. Can you please resend the
patch with the proper Signed-off-by line? While you're there please
also fix the indentation of the "return" line, which isn't correct.

-- 
Jean Delvare

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

[David Hubbard]

Hi Jean,

On 8/18/06, Jean Delvare <khali at linux-fr.org> wrote:
> Hi David,
>
> > > That's not enough, unfortunately. In case of error you must unregister
> > > the driver before returning. I also think we should print some message
> > > on error. Care to send an updated patch?
> >
> > Updated patch attached. I have tested it on my machine and everything
> > is working, but if other hwmon drivers use this, can they apply the
> > patch on their machine and see if the modules fail to load?
>
> Thanks for the update. I tested this with the it87 driver and it worked
> as designed, preventing the driver to load if an error occurs.
>
> Unfortunately you forgot to sign your patch. Can you please resend the
> patch with the proper Signed-off-by line? While you're there please
> also fix the indentation of the "return" line, which isn't correct.

Good catch!

Updated patch attached.

David
-------------- next part --------------
A non-text attachment was scrubbed...
Name: i2c-isa_return_attach_adapter.patch
Type: application/octet-stream
Size: 871 bytes
Desc: not available
Url : http://lists.lm-sensors.org/pipermail/lm-sensors/attachments/20060819/4cd3cce9/attachment.obj 

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

[Jean Delvare]

> Updated patch attached.

Applied, thanks.

-- 
Jean Delvare

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

[David Hubbard]

Hi Jean,

> > Updated patch attached.
>
> Applied, thanks.

I guess moving forward, I'll finish the patch for the w83627ehf
driver, but let's not include it until the i2c patch has had some time
to be tested.

David

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

[Jean Delvare]

David,

> > > Updated patch attached.
> >
> > Applied, thanks.
> 
> I guess moving forward, I'll finish the patch for the w83627ehf
> driver, but let's not include it until the i2c patch has had some time
> to be tested.

No need to be that cautious. If the test your added actually triggers,
it means something was broken in the first place so it won't constitute
a regression. And both patches will spend some time in -mm anyway, where
they will get some testing.

-- 
Jean Delvare

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

[David Hubbard]

Hi all,

Attached is a patch for 2.6.18-rc4 as well as the complete driver
(w83627ehf.c). It tests the return value when device_create_file() is
called. I would like to add sysfs_create_group() calls with another
patch. Anyone who would like to test the patch, please do.

Sylvain, if you are running a 2.4-series kernel, take a look at
i2c-isa_return_attach_adapter.patch as well (in this thread). I do not
know if this will backport to the 2.4 kernel successfully. I have also
attached a regression test script (w83627ehf_regression.sh) which
matches this driver. There is not a way to script a test for the
changes made to correctly handle device_create_file() errors, so this
regression test is the same as previous tests.

Thanks,
David
-------------- next part --------------
A non-text attachment was scrubbed...
Name: w83627ehf_unchecked_device_create_file.patch
Type: application/octet-stream
Size: 5584 bytes
Desc: not available
Url : http://lists.lm-sensors.org/pipermail/lm-sensors/attachments/20060821/9c4b9641/attachment-0002.obj 
-------------- next part --------------
A non-text attachment was scrubbed...
Name: w83627ehf.c
Type: application/octet-stream
Size: 43824 bytes
Desc: not available
Url : http://lists.lm-sensors.org/pipermail/lm-sensors/attachments/20060821/9c4b9641/attachment-0003.obj 
-------------- next part --------------
A non-text attachment was scrubbed...
Name: w83627ehf_regression.sh
Type: application/x-sh
Size: 16224 bytes
Desc: not available
Url : http://lists.lm-sensors.org/pipermail/lm-sensors/attachments/20060821/9c4b9641/attachment-0001.sh 

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

[Jean Delvare]

Hi David,

> Attached is a patch for 2.6.18-rc4 as well as the complete driver
> (w83627ehf.c). It tests the return value when device_create_file() is
> called. I would like to add sysfs_create_group() calls with another
> patch. Anyone who would like to test the patch, please do.

I see very little benefit if making two separate patches for error
checking and grouping. Both will touch the very same code. Let's get it
right at once.

Also, when sending patches to the list, please set the attachement type
to text/plain rather than application/octet-stream. It will make it way
easier for people to read it.

> Sylvain, if you are running a 2.4-series kernel, take a look at
> i2c-isa_return_attach_adapter.patch as well (in this thread). I do not
> know if this will backport to the 2.4 kernel successfully.

No, it will not. Back in 2.4 i2c-isa was considered almost as a regular
i2c adapter, and you can't prevent a chip driver from loading just
because it failed to register a client on one of the many i2c adapters
of a system. So the best we could do was to issue a warning message in
the logs.

> I have also
> attached a regression test script (w83627ehf_regression.sh) which
> matches this driver. There is not a way to script a test for the
> changes made to correctly handle device_create_file() errors, so this
> regression test is the same as previous tests.

As discussed with Jim Cromie earlier, the only thing to check is that
the list of created files is unchanged before and after the patch.

You can also simulate an error at the last file creation, to make sure
the error path is tested at least once. I did that when updating
i2c-core, i2c-dev and i2c-isa, and found a bug thanks to this. Ideally
you should even test every possible error case, if you have more time.

To the code:

> diff -ur linux-2.6.18-rc4/drivers/hwmon/w83627ehf.c linux-2.6.18-rc4/drivers/hwmon/w83627ehf.c
> --- linux-2.6.18-rc4/drivers/hwmon/w83627ehf.c	2006-08-16 15:56:34.000000000 -0700
> +++ linux-2.6.18-rc4/drivers/hwmon/w83627ehf.c	2006-08-21 23:02:59.000000000 -0700
> @@ -621,14 +621,6 @@
>        SENSOR_ATTR(in9_max, S_IWUSR | S_IRUGO, show_in_max, store_in_max, 9),
>  };
>  
> -static void device_create_file_in(struct device *dev, int i)
> -{
> -	device_create_file(dev, &sda_in_input[i].dev_attr);
> -	device_create_file(dev, &sda_in_alarm[i].dev_attr);
> -	device_create_file(dev, &sda_in_min[i].dev_attr);
> -	device_create_file(dev, &sda_in_max[i].dev_attr);
> -}

You don't actually need to get rid of these functions if you are happy
with them. You could do something like:

static int device_create_file_in(struct device *dev, int i)
{
	int err;

	if ((err = device_create_file(dev, &sda_in_input[i].dev_attr))
	 || (err = device_create_file(dev, &sda_in_alarm[i].dev_attr))
	 || (err = device_create_file(dev, &sda_in_min[i].dev_attr))
	 || (err = device_create_file(dev, &sda_in_max[i].dev_attr)))
		return err;

	return 0;
}

> @@ -1223,30 +1198,92 @@
>  		goto exit_detach;
>  	}
>  
> -  	for (i = 0; i < ARRAY_SIZE(sda_sf3_arrays); i++)
> -  		device_create_file(dev, &sda_sf3_arrays[i].dev_attr);
> +  	for (i = 0; i < ARRAY_SIZE(sda_sf3_arrays); i++) {
> +		err = device_create_file(dev, &sda_sf3_arrays[i].dev_attr);
> +		if (err) goto exit_remove;
> +	}

Coding style: "goto exit_remove" should be on its own line.

> -	for (i = 0; i < 10; i++)
> -		device_create_file_in(dev, i);
> +	for (i = 0; i < 10; i++) {
> +		err = device_create_file(dev, &sda_in_input[i].dev_attr);
> +		if (err) goto exit_remove;
> +		err = device_create_file(dev, &sda_in_alarm[i].dev_attr);
> +		if (err) goto exit_remove;
> +		err = device_create_file(dev, &sda_in_min[i].dev_attr);
> +		if (err) goto exit_remove;
> +		err = device_create_file(dev, &sda_in_max[i].dev_attr);
> +		if (err) goto exit_remove;
> +	}

You can group the tests with || so you have a single "goto exit_remove"
for the whole block (see my proposed device_create_file_in above.)

> +exit_remove:
> +	/* some entries in the following arrays may not have been used in
> +	 * device_create_file(), but device_remove_file() will ignore them */
> +	for (i = 0; i < ARRAY_SIZE(sda_temp); i++)
> +		device_remove_file(dev, &sda_temp[i].dev_attr);
> +	for (i = 5; --i; ) {

Please, don't try to be smart. It doesn't even work (you exit at i = 0
before removing the [0] files.) Use a regular for loop, thanks.

> +		if (i < 4) { /* w83627ehf only has 4 pwm */
> +			device_remove_file(dev, &sda_tolerance[i].dev_attr);
> +			device_remove_file(dev, &sda_target_temp[i].dev_attr);
> +			device_remove_file(dev, &sda_pwm_enable[i].dev_attr);
> +			device_remove_file(dev, &sda_pwm_mode[i].dev_attr);
> +			device_remove_file(dev, &sda_pwm[i].dev_attr);
> +		}
> +		device_remove_file(dev, &sda_fan_min[i].dev_attr);
> +		device_remove_file(dev, &sda_fan_div[i].dev_attr);
> +		device_remove_file(dev, &sda_fan_alarm[i].dev_attr);
> +		device_remove_file(dev, &sda_fan_input[i].dev_attr);
> +	}
> +	for (i = 10; --i; ) {
> +		device_remove_file(dev, &sda_in_max[i].dev_attr);
> +		device_remove_file(dev, &sda_in_min[i].dev_attr);
> +		device_remove_file(dev, &sda_in_alarm[i].dev_attr);
> +		device_remove_file(dev, &sda_in_input[i].dev_attr);
> +	}
> +	for (i = 0; i < ARRAY_SIZE(sda_sf3_arrays_fan4); i++)
> +		device_remove_file(dev, &sda_sf3_arrays_fan4[i].dev_attr);
> +	for (i = 0; i < ARRAY_SIZE(sda_sf3_arrays); i++)
> +		device_remove_file(dev, &sda_sf3_arrays[i].dev_attr);
> +	hwmon_device_unregister(data->class_dev);
>  exit_detach:
>  	i2c_detach_client(client);
>  exit_free:

Your patch does remove the files if creation failed, however it fails
to remove them if the device is removed later.

Please provide an updated patch.

Thanks,
-- 
Jean Delvare

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

[David Hubbard]

Hi Jean,

> I see very little benefit if making two separate patches for error
> checking and grouping. Both will touch the very same code. Let's get it
> right at once.

Agreed.

> Also, when sending patches to the list, please set the attachement type
> to text/plain rather than application/octet-stream. It will make it way
> easier for people to read it.

I'm looking into it. One possibility is to change the extension of the
files (e.g. add ".txt" to every file) -- however, then anyone who
reads emails from me will think, "what is this guy doing adding .txt
to everything?" Gmail allows POP access, which I don't currently use,
and I'm looking for SMTP access--if that is possible, then I'll fix it
that way. I also considered sending a patch in the body of the
message, but gmail will wrap the text and break the patch. What I'm
doing for now is attaching it twice, first with .txt and then without.
It's just an extra 7k in the email.

> You can also simulate an error at the last file creation, to make sure
> the error path is tested at least once. I did that when updating
> i2c-core, i2c-dev and i2c-isa, and found a bug thanks to this. Ideally
> you should even test every possible error case, if you have more time.

Okay, tested the return paths. They are working on my machine.

> You don't actually need to get rid of these functions if you are happy
> with them.
>
> You can group the tests with || so you have a single "goto exit_remove"
> for the whole block

Yes. I'll do that.

> Your patch does remove the files if creation failed, however it fails
> to remove them if the device is removed later.

Updated patch follows.

As a side note, I implemented the sysfs_create_group() and
sysfs_remove_group() method. Here are a few comparisons:

Using device_create_file() / device_remove_file() in for loops:
-rw-r--r-- 1 root root  6227 Aug 27 20:01
w83627ehf_unchecked_device_create_file1.patch
-rw-r--r-- 1 root root 39125 Aug 27 20:02 module1.ko

Using sysfs_create_group() and sysfs_remove_group():
-rw-r--r-- 1 root root 13109 Aug 27 19:40
w83627ehf_unchecked_device_create_file2.patch
-rw-r--r-- 1 root root 40617 Aug 27 19:42 module2.ko

I can send the sysfs_create_group patch if anyone is interested. But
since the device_create_file() is a smaller patch, I'll stay with that
one.

Thanks for the input,
David
-------------- next part --------------
Fix: check return value from device_create_file()
Signed-off-by: David Hubbard <david.c.hubbard at gmail.com>

diff -ur linux-2.6.18-rc4/drivers/hwmon/w83627ehf.c linux-2.6.18-rc4/drivers/hwmon/w83627ehf.c
--- linux-2.6.18-rc4/drivers/hwmon/w83627ehf.c	2006-08-16 15:56:34.000000000 -0700
+++ linux-2.6.18-rc4/drivers/hwmon/w83627ehf1.c	2006-08-27 19:59:48.000000000 -0700
@@ -4,6 +4,7 @@
     Copyright (C) 2005  Jean Delvare <khali at linux-fr.org>
     Copyright (C) 2006  Yuan Mu <Ymu at Winbond.com.tw>,
                         Rudolf Marek <r.marek at sh.cvut.cz>
+                        David Hubbard <david.c.hubbard at gmail.com>
 
     Shamelessly ripped from the w83627hf driver
     Copyright (C) 2003  Mark Studebaker
@@ -621,14 +622,6 @@
        SENSOR_ATTR(in9_max, S_IWUSR | S_IRUGO, show_in_max, store_in_max, 9),
 };
 
-static void device_create_file_in(struct device *dev, int i)
-{
-	device_create_file(dev, &sda_in_input[i].dev_attr);
-	device_create_file(dev, &sda_in_alarm[i].dev_attr);
-	device_create_file(dev, &sda_in_min[i].dev_attr);
-	device_create_file(dev, &sda_in_max[i].dev_attr);
-}
-
 #define show_fan_reg(reg) \
 static ssize_t \
 show_##reg(struct device *dev, struct device_attribute *attr, \
@@ -756,14 +749,6 @@
 	SENSOR_ATTR(fan5_div, S_IRUGO, show_fan_div, NULL, 4),
 };
 
-static void device_create_file_fan(struct device *dev, int i)
-{
-	device_create_file(dev, &sda_fan_input[i].dev_attr);
-	device_create_file(dev, &sda_fan_alarm[i].dev_attr);
-	device_create_file(dev, &sda_fan_div[i].dev_attr);
-	device_create_file(dev, &sda_fan_min[i].dev_attr);
-}
-
 #define show_temp1_reg(reg) \
 static ssize_t \
 show_##reg(struct device *dev, struct device_attribute *attr, \
@@ -1036,15 +1021,6 @@
 		    store_tolerance, 3),
 };
 
-static void device_create_file_pwm(struct device *dev, int i)
-{
-	device_create_file(dev, &sda_pwm[i].dev_attr);
-	device_create_file(dev, &sda_pwm_mode[i].dev_attr);
-	device_create_file(dev, &sda_pwm_enable[i].dev_attr);
-	device_create_file(dev, &sda_target_temp[i].dev_attr);
-	device_create_file(dev, &sda_tolerance[i].dev_attr);
-}
-
 /* Smart Fan registers */
 
 #define fan_functions(reg, REG) \
@@ -1131,6 +1107,39 @@
  * Driver and client management
  */
 
+static void remove_unregister(struct w83627ehf_data *data, struct device *dev)
+{
+	/* some entries in the following arrays may not have been used in
+	 * device_create_file(), but device_remove_file() will ignore them */
+	int i;
+	for (i = 0; i < ARRAY_SIZE(sda_temp); i++)
+		device_remove_file(dev, &sda_temp[i].dev_attr);
+	for (i = 0; i < 5; i++) {
+		if (i < 4) { /* w83627ehf only has 4 pwm */
+			device_remove_file(dev, &sda_tolerance[i].dev_attr);
+			device_remove_file(dev, &sda_target_temp[i].dev_attr);
+			device_remove_file(dev, &sda_pwm_enable[i].dev_attr);
+			device_remove_file(dev, &sda_pwm_mode[i].dev_attr);
+			device_remove_file(dev, &sda_pwm[i].dev_attr);
+		}
+		device_remove_file(dev, &sda_fan_min[i].dev_attr);
+		device_remove_file(dev, &sda_fan_div[i].dev_attr);
+		device_remove_file(dev, &sda_fan_alarm[i].dev_attr);
+		device_remove_file(dev, &sda_fan_input[i].dev_attr);
+	}
+	for (i = 0; i < 10; i++) {
+		device_remove_file(dev, &sda_in_max[i].dev_attr);
+		device_remove_file(dev, &sda_in_min[i].dev_attr);
+		device_remove_file(dev, &sda_in_alarm[i].dev_attr);
+		device_remove_file(dev, &sda_in_input[i].dev_attr);
+	}
+	for (i = 0; i < ARRAY_SIZE(sda_sf3_arrays_fan4); i++)
+		device_remove_file(dev, &sda_sf3_arrays_fan4[i].dev_attr);
+	for (i = 0; i < ARRAY_SIZE(sda_sf3_arrays); i++)
+		device_remove_file(dev, &sda_sf3_arrays[i].dev_attr);
+	hwmon_device_unregister(data->class_dev);
+}
+
 static struct i2c_driver w83627ehf_driver;
 
 static void w83627ehf_init_client(struct i2c_client *client)
@@ -1224,29 +1233,62 @@
 	}
 
   	for (i = 0; i < ARRAY_SIZE(sda_sf3_arrays); i++)
-  		device_create_file(dev, &sda_sf3_arrays[i].dev_attr);
+		if ((err = device_create_file(dev,
+			&sda_sf3_arrays[i].dev_attr)))
+			goto exit_remove;
 
 	/* if fan4 is enabled create the sf3 files for it */
 	if (data->has_fan & (1 << 3))
-		for (i = 0; i < ARRAY_SIZE(sda_sf3_arrays_fan4); i++)
-			device_create_file(dev, &sda_sf3_arrays_fan4[i].dev_attr);
+		for (i = 0; i < ARRAY_SIZE(sda_sf3_arrays_fan4); i++) {
+			if ((err = device_create_file(dev,
+				&sda_sf3_arrays_fan4[i].dev_attr)))
+				goto exit_remove;
+		}
 
 	for (i = 0; i < 10; i++)
-		device_create_file_in(dev, i);
+		if ((err = device_create_file(dev, &sda_in_input[i].dev_attr))
+			|| (err = device_create_file(dev,
+				&sda_in_alarm[i].dev_attr))
+			|| (err = device_create_file(dev,
+				&sda_in_min[i].dev_attr))
+			|| (err = device_create_file(dev,
+				&sda_in_max[i].dev_attr)))
+			goto exit_remove;
 
 	for (i = 0; i < 5; i++) {
 		if (data->has_fan & (1 << i)) {
-			device_create_file_fan(dev, i);
-			if (i != 4) /* we have only 4 pwm */
-				device_create_file_pwm(dev, i);
+			if ((err = device_create_file(dev,
+					&sda_fan_input[i].dev_attr))
+				|| (err = device_create_file(dev,
+					&sda_fan_alarm[i].dev_attr))
+				|| (err = device_create_file(dev,
+					&sda_fan_div[i].dev_attr))
+				|| (err = device_create_file(dev,
+					&sda_fan_min[i].dev_attr)))
+				goto exit_remove;
+			if (i < 4 && /* w83627ehf only has 4 pwm */
+				((err = device_create_file(dev,
+					&sda_pwm[i].dev_attr))
+				|| (err = device_create_file(dev,
+					&sda_pwm_mode[i].dev_attr))
+				|| (err = device_create_file(dev,
+					&sda_pwm_enable[i].dev_attr))
+				|| (err = device_create_file(dev,
+					&sda_target_temp[i].dev_attr))
+				|| (err = device_create_file(dev,
+					&sda_tolerance[i].dev_attr))))
+				goto exit_remove;
 		}
 	}
 
 	for (i = 0; i < ARRAY_SIZE(sda_temp); i++)
-		device_create_file(dev, &sda_temp[i].dev_attr);
+		if ((err = device_create_file(dev, &sda_temp[i].dev_attr)))
+			goto exit_remove;
 
 	return 0;
 
+exit_remove:
+	remove_unregister(data, dev);
 exit_detach:
 	i2c_detach_client(client);
 exit_free:
@@ -1262,7 +1304,7 @@
 	struct w83627ehf_data *data = i2c_get_clientdata(client);
 	int err;
 
-	hwmon_device_unregister(data->class_dev);
+	remove_unregister(data, &client->dev);
 
 	if ((err = i2c_detach_client(client)))
 		return err;
-------------- next part --------------
A non-text attachment was scrubbed...
Name: w83627ehf_unchecked_device_create_file1.patch
Type: application/octet-stream
Size: 6226 bytes
Desc: not available
Url : http://lists.lm-sensors.org/pipermail/lm-sensors/attachments/20060827/2b0d2f12/attachment-0001.obj 

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

[Jean Delvare]

Hi David,

> > Also, when sending patches to the list, please set the attachement type
> > to text/plain rather than application/octet-stream. It will make it way
> > easier for people to read it.
> 
> I'm looking into it. One possibility is to change the extension of the
> files (e.g. add ".txt" to every file) -- however, then anyone who
> reads emails from me will think, "what is this guy doing adding .txt
> to everything?"

I'm sure most readers of this list have heard about binary vs. text
attachement issues before, and will understand why you are doing that.

> Gmail allows POP access, which I don't currently use,
> and I'm looking for SMTP access--if that is possible, then I'll fix it
> that way.

Maybe you could contact the gmail admins and ask them to add .patch
and .diff to the list of known file extensions? It shouldn't be too
hard.

> I also considered sending a patch in the body of the
> message, but gmail will wrap the text and break the patch. What I'm
> doing for now is attaching it twice, first with .txt and then without.
> It's just an extra 7k in the email.

No, please don't do that. It's even more confusing, and "just an extra
7k in the email" is not exactly acceptable on a mailing list.

> As a side note, I implemented the sysfs_create_group() and
> sysfs_remove_group() method. Here are a few comparisons:
> 
> Using device_create_file() / device_remove_file() in for loops:
> -rw-r--r-- 1 root root  6227 Aug 27 20:01
> w83627ehf_unchecked_device_create_file1.patch
> -rw-r--r-- 1 root root 39125 Aug 27 20:02 module1.ko
> 
> Using sysfs_create_group() and sysfs_remove_group():
> -rw-r--r-- 1 root root 13109 Aug 27 19:40
> w83627ehf_unchecked_device_create_file2.patch
> -rw-r--r-- 1 root root 40617 Aug 27 19:42 module2.ko
> 
> I can send the sysfs_create_group patch if anyone is interested. But
> since the device_create_file() is a smaller patch, I'll stay with that
> one.

The difference is that most of the added size is data in the
sysfs_remove_group() case, and code in the device_remove_file()-in-loop
case. I think the chances to have a bug in data is lower in general, so
sometimes it's better to have a slightly larger binary if it means less
code.

Note that using sysfs_create_group() was never thought to be a good
idea in general for the hardware monitoring drivers. What Mark M.
Hoffman had been advocating for complex cases was device_create_file()
in loops for file creation and sysfs_remove_group() for file removal.
You maye have something even smaller and more simple code if you try
that.

That being said, I don't actually care. As long as your patch does the
job properly, I'm OK. And the patch you just sent does appear to be
mostly OK, I only have cosmetic comments:

> +static void remove_unregister(struct w83627ehf_data *data, struct device *dev)

It would make sense to prefix it with w83627ehf_ as most other "main"
functions of that driver.

> +{
> +	/* some entries in the following arrays may not have been used in
> +	 * device_create_file(), but device_remove_file() will ignore them */
> +	int i;

Add a blank line here?

> +	for (i = 0; i < ARRAY_SIZE(sda_temp); i++)
> +		device_remove_file(dev, &sda_temp[i].dev_attr);
> +	for (i = 0; i < 5; i++) {
> +		if (i < 4) { /* w83627ehf only has 4 pwm */
> +			device_remove_file(dev, &sda_tolerance[i].dev_attr);
> +			device_remove_file(dev, &sda_target_temp[i].dev_attr);
> +			device_remove_file(dev, &sda_pwm_enable[i].dev_attr);
> +			device_remove_file(dev, &sda_pwm_mode[i].dev_attr);
> +			device_remove_file(dev, &sda_pwm[i].dev_attr);
> +		}
> +		device_remove_file(dev, &sda_fan_min[i].dev_attr);
> +		device_remove_file(dev, &sda_fan_div[i].dev_attr);
> +		device_remove_file(dev, &sda_fan_alarm[i].dev_attr);
> +		device_remove_file(dev, &sda_fan_input[i].dev_attr);
> +	}

It makes little sense to include the pwm loop inside the fan loop IMHO.
On the file creation side, this strange construct was done to test for
fan presence only once, but here the removal is unconditional, so
having seperate loops would work just as well, saving you one test and
one level of indentation.

> +	for (i = 0; i < 10; i++) {
> +		device_remove_file(dev, &sda_in_max[i].dev_attr);
> +		device_remove_file(dev, &sda_in_min[i].dev_attr);
> +		device_remove_file(dev, &sda_in_alarm[i].dev_attr);
> +		device_remove_file(dev, &sda_in_input[i].dev_attr);
> +	}
> +	for (i = 0; i < ARRAY_SIZE(sda_sf3_arrays_fan4); i++)
> +		device_remove_file(dev, &sda_sf3_arrays_fan4[i].dev_attr);
> +	for (i = 0; i < ARRAY_SIZE(sda_sf3_arrays); i++)
> +		device_remove_file(dev, &sda_sf3_arrays[i].dev_attr);
> +	hwmon_device_unregister(data->class_dev);

Grouping hwmon_device_unregister with the file deletions is a bit
awkward. It is something different, using a different parameter. I
would leave it outside.

We also discussed earlier on this list about the file creation vs.
hwmon class registration order, and agreed that the hwmon class should
be registered _after_ all the files are created, to avoid a race
condition where libsensors would (in the future) discover the interface
files before they are all created. Maybe you could do that change in
your patch as well as it affects the same code area. This would address
my previous point as well.

> +}

You seem to be deleting the files in the reverse order of creation?
There's no need to do that, and I find it slightly confusing.

Rest is just fine.

Thanks,
-- 
Jean Delvare

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

[Jean Delvare]

Hi David,

Back to the mailing-list, as I doubt you replied to me privately on
purpose?

> Patch attached.

Applied, with two minor edits:

* Moved the "Register sysfs hooks" comment back to where it belongs.
* In w83627ehf_detach_client, call hwmon_device_unregister before
w83627ehf_device_remove_files. It probably doesn't matter much as
everything ends up being removed, but it looks less confusing for
userspace that way.

Thanks,
-- 
Jean Delvare

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

[David Hubbard]

Hi Jean,

> Back to the mailing-list, as I doubt you replied to me privately on
> purpose?

Oops, yes, I forgot.

> Applied, with two minor edits:
>
> * Moved the "Register sysfs hooks" comment back to where it belongs.
> * In w83627ehf_detach_client, call hwmon_device_unregister before
> w83627ehf_device_remove_files. It probably doesn't matter much as
> everything ends up being removed, but it looks less confusing for
> userspace that way.

Okay, that's good.

David