hwmon: (ads7871) Fix ads7871_probe error path
PATCH 1/1 hwmon: (ads7871) Fix ads7871_probe error path
We need to call hwmon_device_unregister() if an error is detected after sucessfully register hwmon device.
--- drivers/hwmon/ads7871.c | 4 +++- 1 files changed, 3 insertions(+), 1 deletions(-)
drivers/hwmon/ads7871.c +3 -1
@@ -201,11 +201,13 @@ static int __devinit ads7871_probe(struct spi_device *spi)we need to make sure we really have a chip*/ if (val != ret) { err = -ENODEV ; - goto error_remove; + goto error_unregister; }return 0 ; +error_unregister: +hwmon_device_unregister (pdata->hwmon_dev); error_remove:sysfs_remove_group (&spi->dev.kobj, &ads7871_group); error_free:
Signature · 1 line
1.7.2
Hi Alex,
Quoted 17 lines · Axel Lin
We need to call
hwmon_device_unregister()if an error is detected after sucessfully register hwmon device.Signed-off-by Axel Lin <[email protected]>--- drivers/hwmon/ads7871.c | 4 +++- 1 files changed, 3 insertions(+), 1 deletions(-)drivers/hwmon/ads7871.c +0
-1@@ -201,11 +201,13 @@ static int __devinit ads7871_probe(struct spi_device *spi)we need to make sure we really have a chip*/ if (val != ret) { err = -ENODEV ; - goto error_remove;
drivers/hwmon/ads7871.c static int __devinit ads7871_probe(struct spi_device *spi) · line 213
Quoted 10 lines
drivers/hwmon/ads7871.c +3
-0+ goto error_unregister; }return 0 ; +error_unregister: +hwmon_device_unregister (pdata->hwmon_dev); error_remove:sysfs_remove_group (&spi->dev.kobj, &ads7871_group); error_free:
The bug is real, but I don't think the fix is correct. You should never have to unregister the hwmon device in the error path, because you should ensure the hardware is there and working _before_ you register the hwmon device.
User-space could watch for hwmon devices being added or removed, and you don't want to trigger such events for a device which isn't going to work.
So I would suggest reworking the order in which things are done, leaving hwmon_device_register() at the end of the function. This won't only fix the error path, this will also close a race window, as for the moment, the device is exposed to user-space _before_ it it properly initialized, which is wrong.
Please send an updated patch and I'll be happy to apply it.
Signature · 6 lines
Jean Delvare _______________________________________________ lm-sensors mailing list [email protected] http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
Quoted 46 lines · Jean Delvare
Hi Alex,
Quoted 20 lines · Axel Lin
We need to call
hwmon_device_unregister()if an error is detected after sucessfully register hwmon device.Signed-off-by Axel Lin <[email protected]>---
drivers/hwmon/ads7871.c | 4 +++-1 files changed, 3 insertions(+), 1 deletions(-)drivers/hwmon/ads7871.c +0
-0@@ -201,11 +201,13 @@ static int __devinit ads7871_probe(struct spi_device *spi)we need to make sure we really have a chip*/ if (val != ret) {
err =-ENODEV;
- gotoerror_remove;
+ gotoerror_unregister;
}Quoted 7 lines
return 0;
+error_unregister:
+hwmon_device_unregister(pdata->hwmon_dev);error_remove:
sysfs_remove_group(&spi->dev.kobj, &ads7871_group);error_free:The bug is real, but I don't think the fix is correct. You should never have to unregister the hwmon device in the error path, because you should ensure the hardware is there and working _before_ you register the hwmon device.
User-space could watch for hwmon devices being added or removed, and you don't want to trigger such events for a device which isn't going to work.
So I would suggest reworking the order in which things are done, leaving
hwmon_device_register()at the end of the function. This won't only fix the error path, this will also close a race window, as for the moment, the device is exposed to user-space _before_ it it properly initialized, which is wrong.
Quoted 5 lines
Please send an updated patch and I'll be happy to apply it.
--
Jean Delvare
Should the kzalloc, sysfs_create_group and hwmon_device_register all stay in the same order just following the register check? I can send a patch for that.
thanks,
Paul
_______________________________________________
lm-sensors mailing list
[email protected]
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
Quoted 49 lines · Paul Thomas
Quoted 48 lines · Jean Delvare
Hi Alex,
Quoted 20 lines · Axel Lin
We need to call
hwmon_device_unregister()if an error is detected after sucessfully register hwmon device.Signed-off-by Axel Lin <[email protected]>---
drivers/hwmon/ads7871.c | 4 +++-1 files changed, 3 insertions(+), 1 deletions(-)drivers/hwmon/ads7871.c +0
-0@@ -201,11 +201,13 @@ static int __devinit ads7871_probe(struct spi_device *spi)we need to make sure we really have a chip*/ if (val != ret) {
err =-ENODEV;
- gotoerror_remove;
+ gotoerror_unregister;
}Quoted 7 lines
return 0;
+error_unregister:
+hwmon_device_unregister(pdata->hwmon_dev);error_remove:
sysfs_remove_group(&spi->dev.kobj, &ads7871_group);error_free:The bug is real, but I don't think the fix is correct. You should never have to unregister the hwmon device in the error path, because you should ensure the hardware is there and working _before_ you register the hwmon device.
User-space could watch for hwmon devices being added or removed, and you don't want to trigger such events for a device which isn't going to work.
So I would suggest reworking the order in which things are done, leaving
hwmon_device_register()at the end of the function. This won't only fix the error path, this will also close a race window, as for the moment, the device is exposed to user-space _before_ it it properly initialized, which is wrong.Please send an updated patch and I'll be happy to apply it.
Quoted 7 lines
Quoted 3 lines
--
Jean DelvareShould the kzalloc,
sysfs_create_groupandhwmon_device_registerall stay in the same order just following the register check? I can send a patch for that.
Axel already sent an updated patch, so it's all OK.
Signature · 6 lines
Jean Delvare _______________________________________________ lm-sensors mailing list [email protected] http://lists.lm-sensors.org/mailman/listinfo/lm-sensors