Re: [PATCH v1 03/10] hwmon: adt7x10: use devm_request_threaded_irq

From: Guenter Roeck <linux@roeck-us.net>
To: Cosmin Tanislav <demonsingur@gmail.com>
Cc: cosmin.tanislav@analog.com, Jean Delvare <jdelvare@suse.com>,
	linux-hwmon@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v1 03/10] hwmon: adt7x10: use devm_request_threaded_irq
Date: Tue, 21 Dec 2021 08:08:58 -0800	[thread overview]
Message-ID: <82012996-b266-873a-699a-26d77bcb7e5a@roeck-us.net> (raw)
In-Reply-To: <20211221123944.2683245-3-demonsingur@gmail.com>

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