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

[Myeonghun Pak]

Hi Guenter,

You're right. I've switched to request_irq()/free_irq() in v2, with
read_work disabled and drained before freeing the IRQ.

Thanks for the review.

Best regards,
Myeonghun Pak

2026년 10월 18일 (일) 오전 11:57, Guenter Roeck <linux@xxxxxxxxxxxx>님이 작성:
>
> On Thu, Oct 15, 2026 at 11:07:49PM -0400, Myeonghun Pak wrote:
> > A measurement can return while read_work still accesses GPIOs or
> > re-enables the IRQ. Regulator notifications can likewise leave
> > update_supply_work using the regulator after it is released.
> >
> > On removal and the applicable probe error paths, disable and drain
> > read_work before freeing the IRQ. Unregister the regulator notifier
> > before canceling update_supply_work.
> >
> > This issue was identified as part of our ongoing research while
> > reviewing kernel code.
> >
> > 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 with GCC, W=1 (x86_64). No runtime testing.
> >
> >  drivers/hwmon/sht15.c | 16 +++++++++++++---
> >  1 file changed, 13 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/hwmon/sht15.c b/drivers/hwmon/sht15.c
> > index 7bb2105..6c5b2a1 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
> > @@ -986,15 +987,15 @@ static int sht15_probe(struct platform_device *pdev)
> >       disable_irq_nosync(gpiod_to_irq(data->data));
> >       ret = sht15_connection_reset(data);
> >       if (ret)
> > -             goto err_release_reg;
> > +             goto err_release_irq;
> >       ret = sht15_soft_reset(data);
> >       if (ret)
> > -             goto err_release_reg;
> > +             goto err_release_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_release_irq;
> >       }
> >
> >       data->hwmon_dev = hwmon_device_register(data->dev);
> > @@ -1007,9 +1008,13 @@ static int sht15_probe(struct platform_device *pdev)
> >
> >  err_release_sysfs_group:
> >       sysfs_remove_group(&pdev->dev.kobj, &sht15_attr_group);
> > +err_release_irq:
> > +     disable_work_sync(&data->read_work);
> > +     devm_free_irq(&pdev->dev, gpiod_to_irq(data->data), data);
>
> With this here and in the remove function, what is the point of using
> devm_request_irq() ?
>
> Guenter
>
> >  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 +1028,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 read work can re-enable the IRQ, so stop it before freeing IRQ. */
> > +     disable_work_sync(&data->read_work);
> > +     devm_free_irq(&pdev->dev, gpiod_to_irq(data->data), 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);
> >       }
> >  }