hwmon: unchecked return

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.

drivers/hwmon/w83627hf.c +5 -4

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

drivers/hwmon/w83627hf.c +55 -38

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

Hi Mark,

Quoted 3 lines

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.

Hi Mark, Jean,

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

Quoted 6 lines · Jean Delvare

Hi Mark,

Quoted 3 lines

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.

Quoted 2 lines

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

	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?

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

diff -ur linux-2.6.18-rc4/drivers/i2c/busses/i2c-isa.c linux-2.6.18-rc4/drivers/i2c/busses/i2c-isa.c

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

Quoted 2 lines

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

(quoted diff) +0 -12

-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),
-};

Quoted 2 lines

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.

Quoted 4 lines

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.

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.

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?

Quoted 3 lines

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?

Hi David,

Quoted 7 lines

Quoted 3 lines

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.

Updated patch attached.

Quoted 1 line

Updated patch attached.

Applied, thanks.

Quoted 3 lines

Quoted 1 line

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.

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.

diff -ur linux-2.6.18-rc4/drivers/hwmon/w83627ehf.c linux-2.6.18-rc4/drivers/hwmon/w83627ehf.c

drivers/hwmon/w83627ehf.c +16 -0

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

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.

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.

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

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

Quoted 3 lines

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.

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.

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

(quoted diff) +5 -0

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

+}

Patch attached.

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

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.