diff mbox series

[v1,03/10] hwmon: adt7x10: use devm_request_threaded_irq

Message ID 20211221123944.2683245-3-demonsingur@gmail.com (mailing list archive)
State Changes Requested
Headers show
Series [v1,01/10] hwmon: adt7x10: store bus_dev in private data | expand

Commit Message

Cosmin Tanislav Dec. 21, 2021, 12:39 p.m. UTC 
From: Cosmin Tanislav <cosmin.tanislav@analog.com>

To simplify the core driver remove function.

Signed-off-by: Cosmin Tanislav <cosmin.tanislav@analog.com>
---
 drivers/hwmon/adt7x10.c | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

Comments

Guenter Roeck Dec. 21, 2021, 4:08 p.m. UTC | #1 
On 12/21/21 4:39 AM, Cosmin Tanislav wrote:
> From: Cosmin Tanislav <cosmin.tanislav@analog.com>
> 
> To simplify the core driver remove function.
> 
> Signed-off-by: Cosmin Tanislav <cosmin.tanislav@analog.com>
> ---
>   drivers/hwmon/adt7x10.c | 11 +++++------
>   1 file changed, 5 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/hwmon/adt7x10.c b/drivers/hwmon/adt7x10.c
> index dbe9f1ad7db0..48adc0344e88 100644
> --- a/drivers/hwmon/adt7x10.c
> +++ b/drivers/hwmon/adt7x10.c
> @@ -402,9 +402,11 @@ int adt7x10_probe(struct device *dev, const char *name, int irq,
>   	}
>   
>   	if (irq > 0) {
> -		ret = request_threaded_irq(irq, NULL, adt7x10_irq_handler,
> -				IRQF_TRIGGER_FALLING | IRQF_ONESHOT,
> -				dev_name(dev), dev);
> +		ret = devm_request_threaded_irq(dev, irq, NULL,
> +						adt7x10_irq_handler,
> +						IRQF_TRIGGER_FALLING |
> +						IRQF_ONESHOT,
> +						dev_name(dev), dev);
>   		if (ret)
>   			goto exit_hwmon_device_unregister;
>   	}
> @@ -425,9 +427,6 @@ void adt7x10_remove(struct device *dev, int irq)
>   {
>   	struct adt7x10_data *data = dev_get_drvdata(dev);
>   
> -	if (irq > 0)
> -		free_irq(irq, dev);
> -
>   	hwmon_device_unregister(data->hwmon_dev);
>   	sysfs_remove_group(&dev->kobj, &adt7x10_group);
>   	if (data->oldconfig != data->config)
> 

This will keep the interrupt running after the hwmon device was removed,
and after the configuration was restored. If an interrupt occurs at that time,
the handler may potentially access released data. I don't mind making those
changes, but the patches will have to be well sequenced to ensure that each
patch on its own doesn't leave a crippled driver behind. Again, "oh, this will
be ok after the entire series was applied" is not acceptable.

Thanks,
Guenter
diff mbox series

Patch

diff --git a/drivers/hwmon/adt7x10.c b/drivers/hwmon/adt7x10.c
index dbe9f1ad7db0..48adc0344e88 100644
--- a/drivers/hwmon/adt7x10.c
+++ b/drivers/hwmon/adt7x10.c
@@ -402,9 +402,11 @@  int adt7x10_probe(struct device *dev, const char *name, int irq,
 	}
 
 	if (irq > 0) {
-		ret = request_threaded_irq(irq, NULL, adt7x10_irq_handler,
-				IRQF_TRIGGER_FALLING | IRQF_ONESHOT,
-				dev_name(dev), dev);
+		ret = devm_request_threaded_irq(dev, irq, NULL,
+						adt7x10_irq_handler,
+						IRQF_TRIGGER_FALLING |
+						IRQF_ONESHOT,
+						dev_name(dev), dev);
 		if (ret)
 			goto exit_hwmon_device_unregister;
 	}
@@ -425,9 +427,6 @@  void adt7x10_remove(struct device *dev, int irq)
 {
 	struct adt7x10_data *data = dev_get_drvdata(dev);
 
-	if (irq > 0)
-		free_irq(irq, dev);
-
 	hwmon_device_unregister(data->hwmon_dev);
 	sysfs_remove_group(&dev->kobj, &adt7x10_group);
 	if (data->oldconfig != data->config)