Re: [PATCH v3] hwmon: (gpio-fan) fix pm_runtime imbalance for alarm-only fans

[sashiko-bot@kernel.org]

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

New issues:
- [High] Unbalanced pm_runtime_disable() on probe error paths permanently disables runtime PM.

Pre-existing issues:
- [High] Failing to resume the device at the end of probe causes a usage count underflow during devm cleanup.
- [High] A race condition between probe and sysfs writes can cause a double-increment of the runtime PM usage count, permanently blocking runtime suspend.
- [High] Missing synchronize_irq() in runtime suspend callback for shared IRQ.
- [High] Missing cancel_work_sync() for alarm_work leads to a Use-After-Free on driver unbind or probe failure.
- [High] Unconditional IRQ_NONE return in a shared interrupt handler.
--

commit c1b61438a4a40096fe54d7b86037b9d5eebc1cae
Author: Cong Nguyen <congnt264@gmail.com>

hwmon: (gpio-fan) fix pm_runtime imbalance for alarm-only fans

pm_runtime_enable() runs unconditionally in probe, but the devm cleanup
that calls pm_runtime_disable() is only registered when control GPIOs
are present. Alarm-only fans never get it, so unbind warns about the
missing disable.

Link: https://lore.kernel.org/r/20260830152150.27F5F1F000E9@smtp.kernel.org

> diff --git a/drivers/hwmon/gpio-fan.c b/drivers/hwmon/gpio-fan.c
> index 084828e1e2817..8b28f0d58c638 100644
> --- a/drivers/hwmon/gpio-fan.c
> +++ b/drivers/hwmon/gpio-fan.c
[ ... ]
> @@ -553,6 +556,17 @@ static int gpio_fan_probe(struct platform_device *pdev)
>  		return dev_err_probe(dev, PTR_ERR(fan_data->supply),
>  				     "Failed to get fan-supply");
>  
> +	/*
> +	 * Reserve this devm slot before gpio_fan_stop()'s so LIFO teardown
> +	 * runs gpio_fan_stop() (needs PM enabled to disable the regulator)
> +	 * first. pm_runtime_enable() itself still happens at its original
> +	 * position below, so this doesn't change when PM actually becomes
> +	 * enabled -- only where its eventual disable is queued.
> +	 */
> +	err = devm_add_action_or_reset(dev, gpio_fan_pm_runtime_disable, dev);
> +	if (err)
> +		return err;

[Severity: High]
Does unconditionally registering devm_add_action_or_reset() early in the
probe path cause an unbalanced pm_runtime_disable() on probe error paths?

If probe fails at any intermediate point (e.g., in fan_ctrl_init(),
hwmon_device_register_with_groups(), or fan_alarm_init()), devm teardown will
execute gpio_fan_pm_runtime_disable(). Since pm_runtime_enable() hasn't been
reached yet, this incorrectly increments dev->power.disable_depth. On a
subsequent probe attempt, pm_runtime_enable() will only decrement it to 1,
leaving runtime PM permanently disabled.

[Severity: High]
This isn't a bug introduced by this patch, but does failing to resume the device
at the end of probe cause a usage count underflow during devm cleanup?

fan_ctrl_init() initializes fan_data->speed_index based on the hardware state.
If the fan is active, speed_index > 0. gpio_fan_stop() is then queued as a devm
action. If probe subsequently fails before the final pm_runtime_resume_and_get()
succeeds, the devm cleanup runs gpio_fan_stop(), which calls set_fan_speed():

drivers/hwmon/gpio-fan.c:set_fan_speed() {
	...
	if (fan_data->speed_index > 0 && speed_index == 0) {
		int ret;

		ret = pm_runtime_put_sync(fan_data->dev);
		if (ret < 0 && ret != -ENOSYS)
			return ret;
	}
	...
}

Because speed_index > 0, set_fan_speed() drops a PM reference that was never
actually acquired, pushing the usage count below zero.

[Severity: High]
This is a pre-existing issue, but does a race condition between probe and
sysfs writes cause a double-increment of the runtime PM usage count?

Sysfs attributes are exposed early by devm_hwmon_device_register_with_groups().
Userspace can write to pwm1 right after probe calls pm_runtime_enable(). The
sysfs handler acquires a PM reference and sets speed_index > 0.

The probe thread then checks the state:

drivers/hwmon/gpio-fan.c:gpio_fan_probe() {
	...
	/* If current GPIO state is active, mark RPM as active as well */
	if (fan_data->speed_index > 0) {
		int ret;

		ret = pm_runtime_resume_and_get(&pdev->dev);
		if (ret)
			return ret;
	}
	...
}

Since this check doesn't hold fan_data->lock, probe sees the new value and takes
a second PM reference. When userspace later stops the fan, only one reference
is dropped, permanently blocking runtime suspend.

[Severity: High]
This is a pre-existing issue, but is synchronize_irq() missing in the runtime
suspend callback for a shared IRQ?

The driver requests a shared interrupt for the alarm GPIO:

drivers/hwmon/gpio-fan.c:fan_alarm_init() {
	...
	return devm_request_irq(dev, alarm_irq, fan_alarm_irq_handler,
				IRQF_SHARED, "GPIO fan alarm", fan_data);
}

The PM subsystem guide mandates that drivers using IRQF_SHARED MUST call
synchronize_irq() in their runtime suspend callback before powering down
hardware.

Since this is missing, the IRQ handler could execute mid-flight while the
regulator is being disabled in gpio_fan_runtime_suspend():

drivers/hwmon/gpio-fan.c:gpio_fan_runtime_suspend() {
	...
	if (fan_data->supply)
		ret = regulator_disable(fan_data->supply);
	...
}

[Severity: High]
This is a pre-existing issue, but does missing cancel_work_sync() for alarm_work
lead to a use-after-free on driver unbind or probe failure?

The driver initializes fan_data->alarm_work and requests an interrupt to
schedule it:

drivers/hwmon/gpio-fan.c:fan_alarm_init() {
	...
	INIT_WORK(&fan_data->alarm_work, fan_alarm_notify);
	...
	return devm_request_irq(dev, alarm_irq, ...);
}

It fails to register a cleanup action to call cancel_work_sync(). During driver
unbind or probe failure, devm_request_irq is unwound, which synchronizes with
running handlers but does not cancel pending work. fan_data is then freed by
devm. If an interrupt fired right before cleanup, the work executes
asynchronously and dereferences the freed fan_data pointer.

[Severity: High]
This is a pre-existing issue, but does the shared interrupt handler return an
unconditional IRQ_NONE?

drivers/hwmon/gpio-fan.c:fan_alarm_irq_handler() {
	...
	schedule_work(&fan_data->alarm_work);

	return IRQ_NONE;
}

The driver requests an IRQF_SHARED interrupt. In its handler, it processes the
event but unconditionally returns IRQ_NONE. When the kernel sees IRQ_NONE from
all handlers on a shared line, it registers an unhandled interrupt. After enough
unhandled interrupts, the kernel disables the IRQ line permanently, breaking all
devices sharing it.

> +
>  	/* Configure control GPIOs if available. */
>  	if (fan_data->gpios && fan_data->num_gpios > 0) {
>  		if (!fan_data->speed || fan_data->num_speed <= 1)

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260901111903.660681-1-congnt264@gmail.com?part=1