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

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

[Cong Nguyen]

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.

v2 fixed this by switching to devm_pm_runtime_enable() and moving it
before gpio_fan_stop()'s devm registration, so LIFO teardown runs
gpio_fan_stop() first. Sashiko/Guenter correctly flagged that moving
the actual pm_runtime_enable() call earlier opens a new race: hwmon
sysfs (pwm1, fan1_target) is now exposed while PM is already enabled,
so a concurrent sysfs write transitioning speed 0->nonzero can call
pm_runtime_resume_and_get() successfully, and probe's own final
speed_index check does it again -- a double-increment that permanently
blocks runtime suspend. In the original code this same race just fails
cleanly with -EACCES, since PM isn't enabled yet at that point.

Fix this properly: reserve the devm cleanup slot early (a small
gpio_fan_pm_runtime_disable() wrapper, registered right after the
regulator is acquired, before gpio_fan_stop()'s registration) without
touching when pm_runtime_enable() itself actually runs. The enable call
stays at its original position, after hwmon registration and alarm
init, so the race window is identical to the pristine driver -- only
the devm teardown order changes, restoring gpio_fan_stop() running
before the PM disable on unbind, exactly as v2 intended.

Fixes: 0d01110e6356 ("hwmon: (gpio-fan) Add regulator support")
Reported-by: Guenter Roeck <linux@roeck-us.net>
Link: https://lore.kernel.org/r/20260830152150.27F5F1F000E9@smtp.kernel.org
Cc: stable@vger.kernel.org
Assisted-by: Claude:claude-opus-4
Signed-off-by: Cong Nguyen <congnt264@gmail.com>
---
Changes in v3:
- v2 moved the actual pm_runtime_enable() call earlier, which fixed the
  LIFO ordering but opened a new race (Sashiko/Guenter): sysfs exposed
  while PM already enabled. v3 only moves the devm cleanup registration
  early, leaving pm_runtime_enable() at its original position -- no
  behavior change to the race window, only teardown order changes.

 drivers/hwmon/gpio-fan.c | 16 +++++++++++++++-
 1 file changed, 15 insertions(+), 1 deletion(-)

diff --git a/drivers/hwmon/gpio-fan.c b/drivers/hwmon/gpio-fan.c
index 084828e1e281..8b28f0d58c63 100644
--- a/drivers/hwmon/gpio-fan.c
+++ b/drivers/hwmon/gpio-fan.c
@@ -524,8 +524,11 @@ static void gpio_fan_stop(void *data)
 	mutex_lock(&fan_data->lock);
 	set_fan_speed(data, 0);
 	mutex_unlock(&fan_data->lock);
+}
 
-	pm_runtime_disable(fan_data->dev);
+static void gpio_fan_pm_runtime_disable(void *data)
+{
+	pm_runtime_disable(data);
 }
 
 static int gpio_fan_probe(struct platform_device *pdev)
@@ -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;
+
 	/* Configure control GPIOs if available. */
 	if (fan_data->gpios && fan_data->num_gpios > 0) {
 		if (!fan_data->speed || fan_data->num_speed <= 1)
-- 
2.25.1

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

[sashiko-bot]

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

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

[Guenter Roeck]

On 9/1/26 04:32, sashiko-bot@kernel.org wrote:
> 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.

PM seems to be almost impossible to get right :-(.
I don't expect the other issues to be fixed as part of this patch,
but it should fix all the PM issues.

Thanks,
Guenter

> --
> 
> 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)
>