Skip to contentSkip to conversation

hwmon: unchecked return

RFC PATCH New Mark M. Hoffman 2 participants 3 messages Aug 2, 2006 – Aug 6, 2006 Hardware monitoring

lore
Share & tools
lore
#0 lore raw

RFC PATCH 1/1 hwmon: unchecked return

Hi all:

Here is asb100.c... I'll add lm75, lm78, smsc47b397, and w83627hf 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 +68 -54
@@ -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;
Signature · 2 lines
Mark M. Hoffman
mhoffman at lightlink.com
#1 lore raw

RFC PATCH 1/1 hwmon: unchecked return

Hi Mark,

Quoted 2 lines

Here is asb100.c... I'll add lm75, lm78, smsc47b397, and w83627hf to this patch as I have time. (Jean: don't apply yet.)

Very nice, I like it. The asb100 driver is probably an easy one, supporting only one chip with no conditionals in sysfs file creations, but it's still nice to see we can handle this case really easily.

drivers/hwmon/asb100.c static int asb100_detect(struct i2c_adap · line 856

Quoted 2 lines
drivers/hwmon/asb100.c +2 -0
+ERROR4:
+	sysfs_remove_group(&new_client->dev.kobj, &asb100_group);

The fact that we can (ab)use sysfs_remove_group to remove all files without having to care which ones were actually created, will help us much. Even where we can't use the group mecanism to create the files, we can use it to remove them, so the error handling should not be too awful.

I have a similar patch for drivers/i2c/chips/*.c, I'll post it on the i2c list shortly.

Signature · 1 line
Jean Delvare
#2 lore raw

RFC PATCH 1/1 hwmon: unchecked return

In reply to Mark M. Hoffman

Hi Mark,

A few more comments:

Quoted 2 lines

Here is asb100.c... I'll add lm75, lm78, smsc47b397, and w83627hf to this patch as I have time. (Jean: don't apply yet.)

I see little reason to not apply this patch. I don't know for sure what we will do for the other drivers, but what we have come up with for the asb100 is so nice that I just can't think of any better solution for this driver.

BTW, do you have any plan to convert the asb100 driver to use the "dynamic" sysfs callbacks? This would kill some more macros, and shrink the driver size even more.

One minor comment on the code:

Quoted 44 lines
drivers/hwmon/asb100.c +44 -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,

drivers/hwmon/asb100.c static ssize_t set_pwm_enable1(struct de · line 640

Quoted 10 lines
drivers/hwmon/asb100.c +10 -0
+	&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,
+};

The rationale for leaving a comma at the end of the last line of arrays is to minimize the diff when later adding a line at the end of that array. In the case of a NULL-terminated list, nothing will ever be added after that last line, so I'd say we don't want that last comma.

Thanks,

Signature · 1 line
Jean Delvare