Re: [PATCH v2 6/7] hwmon: (sht15) Remove redundant dev_err()

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

Pre-existing issues:
- [High] Race window during probe leads to permanently disabled IRQ and broken device measurements.
- [High] Missing cancellation of pending workqueues on probe failure and device removal leads to Use-After-Free.
--

--- Patch [6]: [PATCH v2 6/7] hwmon: (sht15) Remove redundant dev_err() ---
commit 47aa96341071db3373308927505b18391833a75b
Author: Pan Chuang <panchuang@vivo.com>

hwmon: (sht15) Remove redundant dev_err()

Since commit 55b48e23f5c4 ("genirq/devres: Add error handling in
devm_request_*_irq()"), devm_request_irq() automatically logs detailed
error messages on failure. Remove the now-redundant driver-specific
dev_err() call.

> diff --git a/drivers/hwmon/sht15.c b/drivers/hwmon/sht15.c
> index 101cebbe68e4d..7bb2105d1ae22 100644
> --- a/drivers/hwmon/sht15.c
> +++ b/drivers/hwmon/sht15.c
> @@ -981,10 +981,8 @@ static int sht15_probe(struct platform_device *pdev)
>  			       IRQF_TRIGGER_FALLING,
>  			       "sht15 data",
>  			       data);
> -	if (ret) {
> -		dev_err(&pdev->dev, "failed to get irq for data line\n");
> +	if (ret)
>  		goto err_release_reg;

[Severity: High]
This is a pre-existing issue, but does the err_release_reg error path fail
to cancel pending workqueues, potentially leading to a use-after-free?

If initialization fails here and the code jumps to err_release_reg:

sht15_probe() {
    ...
err_release_reg:
    if (!IS_ERR(data->reg)) {
        regulator_unregister_notifier(data->reg, &data->nb);
        regulator_disable(data->reg);
    }
    return ret;
}

The update_supply_work workqueue is not cancelled via cancel_work_sync().
Because the data struct is allocated with devm_kzalloc(), devres will free
it immediately when the probe fails. If the regulator notifier scheduled
update_supply_work just before the probe failed, will the pending work
execute and dereference the freed data pointer?

A similar issue appears to exist in sht15_remove(), where both read_work
and update_supply_work are left uncancelled before returning.

> -	}
>  	disable_irq_nosync(gpiod_to_irq(data->data));

[Severity: High]
This isn't a bug introduced by this patch, but is there a race window here
that could leave the IRQ permanently disabled?

Since devm_request_irq() is called without IRQF_NO_AUTOEN, the interrupt is
enabled immediately. If a falling edge occurs on the data line before the
disable_irq_nosync() call, sht15_interrupt_fired() will execute and call
disable_irq_nosync(), setting the IRQ disable depth to 1.

When sht15_probe() resumes, it will call disable_irq_nosync() again,
incrementing the disable depth to 2. Later, when a measurement starts,
enable_irq() only decrements the depth to 1. Could this cause the interrupt
to remain masked in hardware forever, breaking the driver?

>  	ret = sht15_connection_reset(data);
>  	if (ret)

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260720063745.309331-1-panchuang@vivo.com?part=6