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

[sashiko-bot@kernel.org]

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

New issues:
- [High] If pm_runtime_resume_and_get() fails during probe, the devres cleanup path drops a PM reference that was never acquired, causing a runtime PM usage counter underflow. The patch attempts to fix early probe underflows by adding a `pm_enabled` guard, but prematurely sets it to `true` before successfully acquiring the reference.

Pre-existing issues:
- [High] Sysfs attributes are exposed to userspace before runtime PM is initialized, allowing concurrent writes to interact with uninitialized PM state and corrupt the PM usage counter.
- [High] Treating non-fatal errors from `pm_runtime_put_sync()` (like `-EAGAIN`) as fatal errors leaves the driver's software state permanently desynchronized from the hardware state, leading to subsequent PM usage counter underflows.
--

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

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

This commit registers the PM runtime disable cleanup before
pm_runtime_enable() is called to fix an imbalance for alarm-only fans.
It also adds a pm_enabled guard and a mutex to protect the enable block.

Link: https://lore.kernel.org/r/20260901111903.660681-1-congnt264@gmail.com

> diff --git a/drivers/hwmon/gpio-fan.c b/drivers/hwmon/gpio-fan.c
> --- a/drivers/hwmon/gpio-fan.c
> +++ b/drivers/hwmon/gpio-fan.c
> @@ -528,10 +529,21 @@ static void gpio_fan_stop(void *data)
>  	struct gpio_fan_data *fan_data = data;
>  
>  	mutex_lock(&fan_data->lock);
> -	set_fan_speed(data, 0);
> +	/* set_fan_speed(0) drops a PM ref; only valid once PM is enabled. */
> +	if (fan_data->pm_enabled)
> +		set_fan_speed(data, 0);

[Severity: High]
This isn't a bug introduced by this patch, but non-fatal errors from
pm_runtime_put_sync() appear to be treated as fatal in set_fan_speed().

When set_fan_speed() turns off the fan, it physically stops the fan and then
calls pm_runtime_put_sync():

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

If the PM core returns -EAGAIN, set_fan_speed() returns immediately without
updating fan_data->speed_index = 0.

The hardware is now OFF, but the software state remains ON.

A subsequent sysfs write to turn the fan off will cause set_fan_speed() to
call pm_runtime_put_sync() a second time, dropping a PM reference it no
longer holds.

Does set_fan_speed() need to update the software state even if
pm_runtime_put_sync() returns a non-fatal error like -EAGAIN?

> +	else
> +		__set_fan_ctrl(fan_data, fan_data->speed[0].ctrl_val);
>  	mutex_unlock(&fan_data->lock);
> +}

[ ... ]

> @@ -586,16 +603,22 @@ static int gpio_fan_probe(struct platform_device *pdev)
>  			return err;
>  	}
>  
> +	/* Lock so a racing sysfs write can't double-take a PM ref here. */
> +	mutex_lock(&fan_data->lock);

[Severity: High]
This isn't a bug introduced by this patch, but this lock doesn't fully close
the race window.

Sysfs attributes are exposed to userspace before runtime PM is initialized:

gpio_fan_probe()
   ...
   fan_data->hwmon_dev = devm_hwmon_device_register_with_groups(...);

A userspace process can immediately write to the pwm1 attribute, triggering
pwm1_store() and set_fan_speed().

If the user writes 0 during this window, set_fan_speed() calls
pm_runtime_put_sync() while PM is disabled.

This unconditionally decrements the usage counter and causes an underflow.

Does this lock need to be taken earlier, or should PM be initialized before
hwmon registration?

>  	pm_runtime_set_suspended(&pdev->dev);
>  	pm_runtime_enable(&pdev->dev);
> +	fan_data->pm_enabled = true;
>  	/* 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)
> +		if (ret) {
> +			mutex_unlock(&fan_data->lock);
>  			return ret;
> +		}

[Severity: High]
If pm_runtime_resume_and_get() fails here, does it lead to a runtime PM
usage counter underflow?

Because fan_data->pm_enabled is set to true before the reference is
successfully acquired, a failure from pm_runtime_resume_and_get()
(e.g. regulator failure) will return an error, triggering the devres
LIFO teardown.

The teardown calls gpio_fan_stop(), which checks pm_enabled and then calls
set_fan_speed(data, 0).

Since the initial state was active, this calls pm_runtime_put_sync(),
dropping a PM reference the driver never successfully acquired.

Should pm_enabled only be set to true after the initial PM reference is
successfully acquired?

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