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

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

[Cong Nguyen]

pm_runtime_enable() runs unconditionally in probe, but its devm cleanup
is only registered when control GPIOs are present, so alarm-only fans
never get pm_runtime_disable() on unbind.

v2's fix moved pm_runtime_enable() earlier and opened a sysfs race; v3
fixed that. Guenter asked this version also close the other two PM
issues Sashiko found: a pm_enabled guard so an early probe failure
can't drop a PM ref that was never taken, and a lock around the
enable-and-check block so a racing sysfs write can't double-take one.

Fixes: 0d01110e6356 ("hwmon: (gpio-fan) Add regulator support")
Reported-by: Sashiko AI review <sashiko-bot@kernel.org>
Link: https://lore.kernel.org/r/20260901111903.660681-1-congnt264@gmail.com
Cc: stable@vger.kernel.org
Assisted-by: Claude:claude-opus-4
Signed-off-by: Cong Nguyen <congnt264@gmail.com>
---
Changes in v4:
- Covers the other 2 PM findings Guenter asked for: pm_enabled guard
  on gpio_fan_stop(), and a lock around the enable-and-check block.

 drivers/hwmon/gpio-fan.c | 29 ++++++++++++++++++++++++++---
 1 file changed, 26 insertions(+), 3 deletions(-)

diff --git a/drivers/hwmon/gpio-fan.c b/drivers/hwmon/gpio-fan.c
index 084828e1e281..93dd685c4fa5 100644
--- a/drivers/hwmon/gpio-fan.c
+++ b/drivers/hwmon/gpio-fan.c
@@ -46,6 +46,7 @@ struct gpio_fan_data {
 	struct gpio_desc	*alarm_gpio;
 	struct work_struct	alarm_work;
 	struct regulator	*supply;
+	bool			pm_enabled;
 };
 
 /*
@@ -522,10 +523,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);
+	else
+		__set_fan_ctrl(fan_data, fan_data->speed[0].ctrl_val);
 	mutex_unlock(&fan_data->lock);
+}
 
-	pm_runtime_disable(fan_data->dev);
+static void gpio_fan_pm_runtime_disable(void *data)
+{
+	struct gpio_fan_data *fan_data = data;
+
+	/* Registered before pm_runtime_enable() runs; skip if it never did. */
+	if (fan_data->pm_enabled)
+		pm_runtime_disable(fan_data->dev);
 }
 
 static int gpio_fan_probe(struct platform_device *pdev)
@@ -553,6 +565,11 @@ static int gpio_fan_probe(struct platform_device *pdev)
 		return dev_err_probe(dev, PTR_ERR(fan_data->supply),
 				     "Failed to get fan-supply");
 
+	/* Register before gpio_fan_stop() so LIFO teardown runs that first. */
+	err = devm_add_action_or_reset(dev, gpio_fan_pm_runtime_disable, fan_data);
+	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)
@@ -580,16 +597,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);
 	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;
+		}
 	}
+	mutex_unlock(&fan_data->lock);
 
 	/* Optional cooling device register for Device tree platforms */
 	fan_data->cdev = devm_thermal_of_child_cooling_device_register(dev, np,
-- 
2.25.1

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

[sashiko-bot]

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