[PATCH v2] hwmon: (sht15) Stop deferred work before releasing its resources

[PATCH v2] hwmon: (sht15) Stop deferred work before releasing its resources

[Myeonghun Pak]

The IRQ handler can queue read_work, which accesses GPIOs and may re-enable
that same IRQ.  A measurement can return while read_work is still pending
or running.  The regulator notifier can also queue update_supply_work,
which accesses the regulator.  Neither work item was drained on removal
or on all probe failure paths before the managed resources were released.

Use request_irq() and free_irq() since the IRQ must be released at an
explicit point in the teardown sequence.  Disable and drain read_work
before freeing the IRQ.  Unregister the regulator notifier before draining
update_supply_work and disabling the regulator.

Use IRQF_NO_AUTOEN to ensure that the IRQ stays disabled until the first
measurement, rather than enabling it briefly between request_irq() and
disable_irq_nosync().  Cache the IRQ number and check errors from
gpiod_to_irq().

Fixes: 251eb40f5ccd ("hwmon: sht15 humidity sensor driver")
Assisted-by: LLM
Co-developed-by: Ijae Kim <ae878000@gmail.com>
Signed-off-by: Ijae Kim <ae878000@gmail.com>
Signed-off-by: Myeonghun Pak <mhun512@gmail.com>
---
Compile-tested (W=1). No runtime testing.

Changes in v2:
- Replace managed IRQ request/free with request_irq()/free_irq().
- Use IRQF_NO_AUTOEN and cache the data GPIO IRQ.
- Preserve asynchronous work drain ordering on probe failure and remove.

 drivers/hwmon/sht15.c | 47 +++++++++++++++++++++++++++++++++--------------
 1 file changed, 33 insertions(+), 14 deletions(-)

diff --git a/drivers/hwmon/sht15.c b/drivers/hwmon/sht15.c
index 7bb2105d1ae223e7131ba4cc0523e4e074e01302..c95acec7ae2f8f57a10bd72f65a2905e64c3c58c 100644
--- a/drivers/hwmon/sht15.c
+++ b/drivers/hwmon/sht15.c
@@ -31,6 +31,7 @@
 #include <linux/bitrev.h>
 #include <linux/gpio/consumer.h>
 #include <linux/of.h>
+#include <linux/workqueue.h>
 
 /* Commands */
 #define SHT15_MEASURE_TEMP		0x03
@@ -120,6 +121,7 @@ static const u8 sht15_crc8_table[] = {
  * struct sht15_data - device instance specific data
  * @sck:		clock GPIO line
  * @data:		data GPIO line
+ * @irq:		interrupt number for the data GPIO line.
  * @read_work:		bh of interrupt handler.
  * @wait_queue:		wait queue for getting values from device.
  * @val_temp:		last temperature value read from device.
@@ -149,6 +151,7 @@ static const u8 sht15_crc8_table[] = {
 struct sht15_data {
 	struct gpio_desc		*sck;
 	struct gpio_desc		*data;
+	int				irq;
 	struct work_struct		read_work;
 	wait_queue_head_t		wait_queue;
 	uint16_t			val_temp;
@@ -532,9 +535,9 @@ static int sht15_measurement(struct sht15_data *data,
 		return ret;
 	atomic_set(&data->interrupt_handled, 0);
 
-	enable_irq(gpiod_to_irq(data->data));
+	enable_irq(data->irq);
 	if (gpiod_get_value(data->data) == 0) {
-		disable_irq_nosync(gpiod_to_irq(data->data));
+		disable_irq_nosync(data->irq);
 		/* Only relevant if the interrupt hasn't occurred. */
 		if (!atomic_read(&data->interrupt_handled))
 			schedule_work(&data->read_work);
@@ -546,7 +549,7 @@ static int sht15_measurement(struct sht15_data *data,
 		data->state = SHT15_READING_NOTHING;
 		return -EIO;
 	} else if (ret == 0) { /* timeout occurred */
-		disable_irq_nosync(gpiod_to_irq(data->data));
+		disable_irq_nosync(data->irq);
 		ret = sht15_connection_reset(data);
 		if (ret)
 			return ret;
@@ -825,7 +828,7 @@ static void sht15_bh_read_data(struct work_struct *work_s)
 		 * have gone low in meantime so verify it hasn't!
 		 */
 		atomic_set(&data->interrupt_handled, 0);
-		enable_irq(gpiod_to_irq(data->data));
+		enable_irq(data->irq);
 		/* If still not occurred or another handler was scheduled */
 		if (gpiod_get_value(data->data)
 		    || atomic_read(&data->interrupt_handled))
@@ -976,25 +979,31 @@ static int sht15_probe(struct platform_device *pdev)
 		goto err_release_reg;
 	}
 
-	ret = devm_request_irq(&pdev->dev, gpiod_to_irq(data->data),
-			       sht15_interrupt_fired,
-			       IRQF_TRIGGER_FALLING,
-			       "sht15 data",
-			       data);
-	if (ret)
+	data->irq = gpiod_to_irq(data->data);
+	if (data->irq < 0) {
+		ret = data->irq;
+		goto err_release_reg;
+	}
+
+	ret = request_irq(data->irq, sht15_interrupt_fired,
+			  IRQF_TRIGGER_FALLING | IRQF_NO_AUTOEN,
+			  "sht15 data", data);
+	if (ret) {
+		dev_err(&pdev->dev, "failed to get irq for data line\n");
 		goto err_release_reg;
-	disable_irq_nosync(gpiod_to_irq(data->data));
+	}
+	/* IRQ remains disabled until a measurement starts. */
 	ret = sht15_connection_reset(data);
 	if (ret)
-		goto err_release_reg;
+		goto err_free_irq;
 	ret = sht15_soft_reset(data);
 	if (ret)
-		goto err_release_reg;
+		goto err_free_irq;
 
 	ret = sysfs_create_group(&pdev->dev.kobj, &sht15_attr_group);
 	if (ret) {
 		dev_err(&pdev->dev, "sysfs create failed\n");
-		goto err_release_reg;
+		goto err_free_irq;
 	}
 
 	data->hwmon_dev = hwmon_device_register(data->dev);
@@ -1007,9 +1016,14 @@ static int sht15_probe(struct platform_device *pdev)
 
 err_release_sysfs_group:
 	sysfs_remove_group(&pdev->dev.kobj, &sht15_attr_group);
+err_free_irq:
+	/* The read worker can re-enable the IRQ, so drain it first. */
+	disable_work_sync(&data->read_work);
+	free_irq(data->irq, data);
 err_release_reg:
 	if (!IS_ERR(data->reg)) {
 		regulator_unregister_notifier(data->reg, &data->nb);
+		cancel_work_sync(&data->update_supply_work);
 		regulator_disable(data->reg);
 	}
 	return ret;
@@ -1023,12 +1037,17 @@ static void sht15_remove(struct platform_device *pdev)
 	hwmon_device_unregister(data->hwmon_dev);
 	sysfs_remove_group(&pdev->dev.kobj, &sht15_attr_group);
 
+	/* The worker can re-enable the IRQ, so stop it before free_irq(). */
+	disable_work_sync(&data->read_work);
+	free_irq(data->irq, data);
+
 	ret = sht15_soft_reset(data);
 	if (ret)
 		dev_err(&pdev->dev, "Failed to reset device (%pe)\n", ERR_PTR(ret));
 
 	if (!IS_ERR(data->reg)) {
 		regulator_unregister_notifier(data->reg, &data->nb);
+		cancel_work_sync(&data->update_supply_work);
 		regulator_disable(data->reg);
 	}
 }
-- 
2.47.3

Re: [PATCH v2] hwmon: (sht15) Stop deferred work before releasing its resources

[Guenter Roeck]

On 10/19/26 16:44, Myeonghun Pak wrote:
> The IRQ handler can queue read_work, which accesses GPIOs and may re-enable
> that same IRQ.  A measurement can return while read_work is still pending
> or running.  The regulator notifier can also queue update_supply_work,
> which accesses the regulator.  Neither work item was drained on removal
> or on all probe failure paths before the managed resources were released.
> 
> Use request_irq() and free_irq() since the IRQ must be released at an
> explicit point in the teardown sequence.  Disable and drain read_work
> before freeing the IRQ.  Unregister the regulator notifier before draining
> update_supply_work and disabling the regulator.
> 
> Use IRQF_NO_AUTOEN to ensure that the IRQ stays disabled until the first
> measurement, rather than enabling it briefly between request_irq() and
> disable_irq_nosync().  Cache the IRQ number and check errors from
> gpiod_to_irq().
> 
> Fixes: 251eb40f5ccd ("hwmon: sht15 humidity sensor driver")
> Assisted-by: LLM
> Co-developed-by: Ijae Kim <ae878000@gmail.com>
> Signed-off-by: Ijae Kim <ae878000@gmail.com>
> Signed-off-by: Myeonghun Pak <mhun512@gmail.com>
> ---
> Compile-tested (W=1). No runtime testing.
> 
> Changes in v2:
> - Replace managed IRQ request/free with request_irq()/free_irq().
> - Use IRQF_NO_AUTOEN and cache the data GPIO IRQ.

That replaces a relatively simple patch with a much more complex
one, and Sashiko takes offense with it. What is the problem with
the original code ?

Guenter

> - Preserve asynchronous work drain ordering on probe failure and remove.
> 
>   drivers/hwmon/sht15.c | 47 +++++++++++++++++++++++++++++++++--------------
>   1 file changed, 33 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/hwmon/sht15.c b/drivers/hwmon/sht15.c
> index 7bb2105d1ae223e7131ba4cc0523e4e074e01302..c95acec7ae2f8f57a10bd72f65a2905e64c3c58c 100644
> --- a/drivers/hwmon/sht15.c
> +++ b/drivers/hwmon/sht15.c
> @@ -31,6 +31,7 @@
>   #include <linux/bitrev.h>
>   #include <linux/gpio/consumer.h>
>   #include <linux/of.h>
> +#include <linux/workqueue.h>
>   
>   /* Commands */
>   #define SHT15_MEASURE_TEMP		0x03
> @@ -120,6 +121,7 @@ static const u8 sht15_crc8_table[] = {
>    * struct sht15_data - device instance specific data
>    * @sck:		clock GPIO line
>    * @data:		data GPIO line
> + * @irq:		interrupt number for the data GPIO line.
>    * @read_work:		bh of interrupt handler.
>    * @wait_queue:		wait queue for getting values from device.
>    * @val_temp:		last temperature value read from device.
> @@ -149,6 +151,7 @@ static const u8 sht15_crc8_table[] = {
>   struct sht15_data {
>   	struct gpio_desc		*sck;
>   	struct gpio_desc		*data;
> +	int				irq;
>   	struct work_struct		read_work;
>   	wait_queue_head_t		wait_queue;
>   	uint16_t			val_temp;
> @@ -532,9 +535,9 @@ static int sht15_measurement(struct sht15_data *data,
>   		return ret;
>   	atomic_set(&data->interrupt_handled, 0);
>   
> -	enable_irq(gpiod_to_irq(data->data));
> +	enable_irq(data->irq);
>   	if (gpiod_get_value(data->data) == 0) {
> -		disable_irq_nosync(gpiod_to_irq(data->data));
> +		disable_irq_nosync(data->irq);
>   		/* Only relevant if the interrupt hasn't occurred. */
>   		if (!atomic_read(&data->interrupt_handled))
>   			schedule_work(&data->read_work);
> @@ -546,7 +549,7 @@ static int sht15_measurement(struct sht15_data *data,
>   		data->state = SHT15_READING_NOTHING;
>   		return -EIO;
>   	} else if (ret == 0) { /* timeout occurred */
> -		disable_irq_nosync(gpiod_to_irq(data->data));
> +		disable_irq_nosync(data->irq);
>   		ret = sht15_connection_reset(data);
>   		if (ret)
>   			return ret;
> @@ -825,7 +828,7 @@ static void sht15_bh_read_data(struct work_struct *work_s)
>   		 * have gone low in meantime so verify it hasn't!
>   		 */
>   		atomic_set(&data->interrupt_handled, 0);
> -		enable_irq(gpiod_to_irq(data->data));
> +		enable_irq(data->irq);
>   		/* If still not occurred or another handler was scheduled */
>   		if (gpiod_get_value(data->data)
>   		    || atomic_read(&data->interrupt_handled))
> @@ -976,25 +979,31 @@ static int sht15_probe(struct platform_device *pdev)
>   		goto err_release_reg;
>   	}
>   
> -	ret = devm_request_irq(&pdev->dev, gpiod_to_irq(data->data),
> -			       sht15_interrupt_fired,
> -			       IRQF_TRIGGER_FALLING,
> -			       "sht15 data",
> -			       data);
> -	if (ret)
> +	data->irq = gpiod_to_irq(data->data);
> +	if (data->irq < 0) {
> +		ret = data->irq;
> +		goto err_release_reg;
> +	}
> +
> +	ret = request_irq(data->irq, sht15_interrupt_fired,
> +			  IRQF_TRIGGER_FALLING | IRQF_NO_AUTOEN,
> +			  "sht15 data", data);
> +	if (ret) {
> +		dev_err(&pdev->dev, "failed to get irq for data line\n");
>   		goto err_release_reg;
> -	disable_irq_nosync(gpiod_to_irq(data->data));
> +	}
> +	/* IRQ remains disabled until a measurement starts. */
>   	ret = sht15_connection_reset(data);
>   	if (ret)
> -		goto err_release_reg;
> +		goto err_free_irq;
>   	ret = sht15_soft_reset(data);
>   	if (ret)
> -		goto err_release_reg;
> +		goto err_free_irq;
>   
>   	ret = sysfs_create_group(&pdev->dev.kobj, &sht15_attr_group);
>   	if (ret) {
>   		dev_err(&pdev->dev, "sysfs create failed\n");
> -		goto err_release_reg;
> +		goto err_free_irq;
>   	}
>   
>   	data->hwmon_dev = hwmon_device_register(data->dev);
> @@ -1007,9 +1016,14 @@ static int sht15_probe(struct platform_device *pdev)
>   
>   err_release_sysfs_group:
>   	sysfs_remove_group(&pdev->dev.kobj, &sht15_attr_group);
> +err_free_irq:
> +	/* The read worker can re-enable the IRQ, so drain it first. */
> +	disable_work_sync(&data->read_work);
> +	free_irq(data->irq, data);
>   err_release_reg:
>   	if (!IS_ERR(data->reg)) {
>   		regulator_unregister_notifier(data->reg, &data->nb);
> +		cancel_work_sync(&data->update_supply_work);
>   		regulator_disable(data->reg);
>   	}
>   	return ret;
> @@ -1023,12 +1037,17 @@ static void sht15_remove(struct platform_device *pdev)
>   	hwmon_device_unregister(data->hwmon_dev);
>   	sysfs_remove_group(&pdev->dev.kobj, &sht15_attr_group);
>   
> +	/* The worker can re-enable the IRQ, so stop it before free_irq(). */
> +	disable_work_sync(&data->read_work);
> +	free_irq(data->irq, data);
> +
>   	ret = sht15_soft_reset(data);
>   	if (ret)
>   		dev_err(&pdev->dev, "Failed to reset device (%pe)\n", ERR_PTR(ret));
>   
>   	if (!IS_ERR(data->reg)) {
>   		regulator_unregister_notifier(data->reg, &data->nb);
> +		cancel_work_sync(&data->update_supply_work);
>   		regulator_disable(data->reg);
>   	}
>   }