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

[Myeonghun Pak]

Hi Guenter,

The original issue is that read_work can remain pending after a
measurement timeout, while unregistering the regulator notifier does
not drain previously queued update_supply_work. Both may access
resources after they are released.

You're right that v2 is unnecessarily complex. I'll remove
IRQF_NO_AUTOEN and the IRQ caching changes, and keep only the explicit
IRQ teardown and work synchronization in v3.

Thanks for the review.

Best regards,
Myeonghun Pak

2026년 10월 19일 (월) 오후 8:37, Guenter Roeck <linux@xxxxxxxxxxxx>님이 작성:
>
> 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@xxxxxxxxx>
> > Signed-off-by: Ijae Kim <ae878000@xxxxxxxxx>
> > Signed-off-by: Myeonghun Pak <mhun512@xxxxxxxxx>
> > ---
> > 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);
> >       }
> >   }
>