Skip to contentSkip to conversation

platform/x86: hp-wmi: Victus S fan control fixes

PATCH In review Emre Cecanpunar 2 participants 7 messages Apr 7 – Apr 9 x86 platform drivers

lore
Share & tools
lore
#0 lore raw

v4 cover platform/x86: hp-wmi: Victus S fan control fixes

Bug fixes for the Victus S manual fan control:
- Propagate fan_count_userdefine_trigger() errors
- Fix cancel_delayed_work_sync() deadlock from work handler
- Use mod_delayed_work() for efficient keep-alive timer resets
- Fix u8 underflow in gpu_delta calculation
- Add mutex for concurrent hwmon state access

Changes in v4:
- Patch 4: Prevent line wrap in gpu_delta assignment.
- Patch 5: Use devm_mutex_init() and handle init errors.

Changes in v3:
- Add Fixes: tags to all patches.
- Patch 4: Remove unnecessary casting.

Changes in v2:
- Patch 4: Store gpu_delta as 'int' to handle negative deltas correctly.

Emre Cecanpunar (5):
platform/x86: hp-wmi: fix ignored return values in fan settings platform/x86: hp-wmi: avoid cancel_delayed_work_sync from work handler platform/x86: hp-wmi: use mod_delayed_work to reset keep-alive timer platform/x86: hp-wmi: fix u8 underflow in gpu_delta calculation platform/x86: hp-wmi: add locking for concurrent hwmon access

 drivers/platform/x86/hp/hp-wmi.c | 54 ++++++++++++++++++++++----------
 1 file changed, 37 insertions(+), 17 deletions(-)
Signature · 1 line
2.53.0
#1 lore raw

PATCH v4 1/5 platform/x86: hp-wmi: fix ignored return values in fan settings

hp_wmi_get_fan_count_userdefine_trigger() can fail, but its return value was silently ignored in hp_wmi_apply_fan_settings() for PWM_MODE_MAX/AUTO. Propagate these errors consistently.

Additionally, handle the return value of hp_wmi_apply_fan_settings() in its callers by adding appropriate warnings on failure, and remove an unreachable "return 0" at the end of the function.

Fixes 46be1453e6e6 ("platform/x86: hp-wmi: add manual fan control for Victus S models")
---
 drivers/platform/x86/hp/hp-wmi.c | 23 ++++++++++++++++-------
 1 file changed, 16 insertions(+), 7 deletions(-)
drivers/platform/x86/hp/hp-wmi.c +16 -7
index 988a0acc9622..eac39f68d762 100644
@@ -2353,8 +2353,11 @@ static int hp_wmi_apply_fan_settings(struct hp_wmi_hwmon_priv *priv)
 
 	switch (priv->mode) {
 	case PWM_MODE_MAX:
-		if (is_victus_s_thermal_profile())
-			hp_wmi_get_fan_count_userdefine_trigger();
+		if (is_victus_s_thermal_profile()) {
+			ret = hp_wmi_get_fan_count_userdefine_trigger();
+			if (ret < 0)
+				return ret;
+		}
 		ret = hp_wmi_fan_speed_max_set(1);
 		if (ret < 0)
 			return ret;
@@ -2372,7 +2375,9 @@ static int hp_wmi_apply_fan_settings(struct hp_wmi_hwmon_priv *priv)
 		return 0;
 	case PWM_MODE_AUTO:
 		if (is_victus_s_thermal_profile()) {
-			hp_wmi_get_fan_count_userdefine_trigger();
+			ret = hp_wmi_get_fan_count_userdefine_trigger();
+			if (ret < 0)
+				return ret;
 			ret = hp_wmi_fan_speed_max_reset(priv);
 		} else {
 			ret = hp_wmi_fan_speed_max_set(0);
@@ -2385,8 +2390,6 @@ static int hp_wmi_apply_fan_settings(struct hp_wmi_hwmon_priv *priv)
 		/* shouldn't happen */
 		return -EINVAL;
 	}
-
-	return 0;
 }
 
 static umode_t hp_wmi_hwmon_is_visible(const void *data,
@@ -2528,6 +2531,7 @@ static void hp_wmi_hwmon_keep_alive_handler(struct work_struct *work)
 {
 	struct delayed_work *dwork;
 	struct hp_wmi_hwmon_priv *priv;
+	int ret;
 
 	dwork = to_delayed_work(work);
 	priv = container_of(dwork, struct hp_wmi_hwmon_priv, keep_alive_dwork);
@@ -2535,7 +2539,10 @@ static void hp_wmi_hwmon_keep_alive_handler(struct work_struct *work)
 	 * Re-apply the current hwmon context settings.
 	 * NOTE: hp_wmi_apply_fan_settings will handle the re-scheduling.
 	 */
-	hp_wmi_apply_fan_settings(priv);
+	ret = hp_wmi_apply_fan_settings(priv);
+	if (ret)
+		pr_warn_ratelimited("keep-alive failed to refresh fan settings: %d\n",
+				    ret);
 }
 
 static int hp_wmi_setup_fan_settings(struct hp_wmi_hwmon_priv *priv)
@@ -2597,7 +2604,9 @@ static int hp_wmi_hwmon_init(void)
 
 	INIT_DELAYED_WORK(&priv->keep_alive_dwork, hp_wmi_hwmon_keep_alive_handler);
 	platform_set_drvdata(hp_wmi_platform_dev, priv);
-	hp_wmi_apply_fan_settings(priv);
+	ret = hp_wmi_apply_fan_settings(priv);
+	if (ret)
+		dev_warn(dev, "Failed to apply initial fan settings: %d\n", ret);
 
 	return 0;
 }
Signature · 1 line
2.53.0
#2 lore raw

PATCH v4 2/5 platform/x86: hp-wmi: avoid cancel_delayed_work_sync from work handler

In reply to Emre Cecanpunar

hp_wmi_apply_fan_settings() uses cancel_delayed_work_sync() to stop the keep-alive timer in AUTO mode. However, since hp_wmi_apply_fan_settings() is also called from the keep-alive handler, a race condition with a sysfs write can cause the handler to wait on itself, leading to a deadlock.

Replace cancel_delayed_work_sync() with cancel_delayed_work() in hp_wmi_apply_fan_settings() to avoid the self-flush deadlock.

Fixes c203c59fb5de ("platform/x86: hp-wmi: implement fan keep-alive")
---
 drivers/platform/x86/hp/hp-wmi.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/platform/x86/hp/hp-wmi.c +1 -1
index eac39f68d762..79d6bc3cd223 100644
@@ -2384,7 +2384,7 @@ static int hp_wmi_apply_fan_settings(struct hp_wmi_hwmon_priv *priv)
 		}
 		if (ret < 0)
 			return ret;
-		cancel_delayed_work_sync(&priv->keep_alive_dwork);
+		cancel_delayed_work(&priv->keep_alive_dwork);
 		return 0;
 	default:
 		/* shouldn't happen */
Signature · 1 line
2.53.0
#3 lore raw

PATCH v4 3/5 platform/x86: hp-wmi: use mod_delayed_work to reset keep-alive timer

In reply to Emre Cecanpunar

Currently, schedule_delayed_work() is used to queue the 90s keep-alive timer. If a user manually changes the fan speed at T=85s, schedule_delayed_work() leaves the existing timer in place as it is a no-op if the work is already pending. This results in the keep-alive timer firing unnecessarily at T=90s, just 5 seconds after the user action.

Replace schedule_delayed_work() with mod_delayed_work() to reset the 90s timer whenever fan settings are applied. This guarantees a full 90s delay after every user interaction, preventing redundant keep-alive executions and improving efficiency.

Fixes c203c59fb5de ("platform/x86: hp-wmi: implement fan keep-alive")

---
Changes in v3:
- Reworded commit message to clarify this is an inefficiency fix, not a functional bug.

 drivers/platform/x86/hp/hp-wmi.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)
drivers/platform/x86/hp/hp-wmi.c +4 -4
index 79d6bc3cd223..2932cab9aa78 100644
@@ -2361,8 +2361,8 @@ static int hp_wmi_apply_fan_settings(struct hp_wmi_hwmon_priv *priv)
 		ret = hp_wmi_fan_speed_max_set(1);
 		if (ret < 0)
 			return ret;
-		schedule_delayed_work(&priv->keep_alive_dwork,
-				      secs_to_jiffies(KEEP_ALIVE_DELAY_SECS));
+		mod_delayed_work(system_wq, &priv->keep_alive_dwork,
+				 secs_to_jiffies(KEEP_ALIVE_DELAY_SECS));
 		return 0;
 	case PWM_MODE_MANUAL:
 		if (!is_victus_s_thermal_profile())
@@ -2370,8 +2370,8 @@ static int hp_wmi_apply_fan_settings(struct hp_wmi_hwmon_priv *priv)
 		ret = hp_wmi_fan_speed_set(priv, pwm_to_rpm(priv->pwm, priv));
 		if (ret < 0)
 			return ret;
-		schedule_delayed_work(&priv->keep_alive_dwork,
-				      secs_to_jiffies(KEEP_ALIVE_DELAY_SECS));
+		mod_delayed_work(system_wq, &priv->keep_alive_dwork,
+				 secs_to_jiffies(KEEP_ALIVE_DELAY_SECS));
 		return 0;
 	case PWM_MODE_AUTO:
 		if (is_victus_s_thermal_profile()) {
Signature · 1 line
2.53.0
#4 lore raw

PATCH v4 4/5 platform/x86: hp-wmi: fix u8 underflow in gpu_delta calculation

In reply to Emre Cecanpunar

gpu_delta was declared as u8. If the firmware specifies a GPU RPM lower than the CPU RPM, subtracting them causes an underflow (e.g. 10 - 20 = 246), which forces the GPU fan to remain clamped at U8_MAX (100% speed) during operation.

Change gpu_delta to int and use signed arithmetic. Existing signed logic in hp_wmi_fan_speed_set() correctly handles negative deltas.

Fixes 46be1453e6e6 ("platform/x86: hp-wmi: add manual fan control for Victus S models")

---
Changes in v4:
- Restore gpu_delta assignment to a single line to avoid unrelated style noise.
- This results in the assignment being identical to the base branch, effectively removing that hunk from the patch while keeping the type change.

Changes in v3:
- Remove unnecessary casting.
- Add Fixes: tag.

Changes in v2:
- Change gpu_delta type to int to handle potential negative values.

 drivers/platform/x86/hp/hp-wmi.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)
drivers/platform/x86/hp/hp-wmi.c +3 -3
index 2932cab9aa78..4f49861d3fbe 100644
@@ -455,7 +455,7 @@ enum pwm_modes {
 struct hp_wmi_hwmon_priv {
 	u8 min_rpm;
 	u8 max_rpm;
-	u8 gpu_delta;
+	int gpu_delta;
 	u8 mode;
 	u8 pwm;
 	struct delayed_work keep_alive_dwork;
@@ -2549,8 +2549,8 @@ static int hp_wmi_setup_fan_settings(struct hp_wmi_hwmon_priv *priv)
 {
 	u8 fan_data[128] = { 0 };
 	struct victus_s_fan_table *fan_table;
-	u8 min_rpm, max_rpm, gpu_delta;
-	int ret;
+	u8 min_rpm, max_rpm;
+	int gpu_delta, ret;
 
 	/* Default behaviour on hwmon init is automatic mode */
 	priv->mode = PWM_MODE_AUTO;

--
2.53.0

#5 lore raw

PATCH v4 5/5 platform/x86: hp-wmi: add locking for concurrent hwmon access

In reply to Emre Cecanpunar

hp_wmi_hwmon_priv.mode and .pwm are written by hp_wmi_hwmon_write() in sysfs context and read by hp_wmi_hwmon_keep_alive_handler() in a workqueue. A concurrent write and keep-alive expiry can observe an inconsistent mode/pwm pair (e.g. mode=MANUAL with a stale pwm).

Add a mutex to hp_wmi_hwmon_priv protecting mode and pwm. Hold it in hp_wmi_hwmon_write() across the field update and apply call, and in hp_wmi_hwmon_keep_alive_handler() before calling apply.

In hp_wmi_hwmon_read(), only the pwm_enable path reads priv->mode; use scoped_guard() there to avoid holding the lock across unrelated WMI calls.

Fixes c203c59fb5de ("platform/x86: hp-wmi: implement fan keep-alive")

---
Changes in v4:
- Use devm_mutex_init() and handle init errors.

 drivers/platform/x86/hp/hp-wmi.c | 15 +++++++++++++--
 1 file changed, 13 insertions(+), 2 deletions(-)
drivers/platform/x86/hp/hp-wmi.c +13 -2
index 4f49861d3fbe..470c6e48f8e9 100644
@@ -453,6 +453,7 @@ enum pwm_modes {
 };
 
 struct hp_wmi_hwmon_priv {
+	struct mutex lock;	/* protects mode, pwm */
 	u8 min_rpm;
 	u8 max_rpm;
 	int gpu_delta;
@@ -2422,6 +2423,7 @@ static int hp_wmi_hwmon_read(struct device *dev, enum hwmon_sensor_types type,
 {
 	struct hp_wmi_hwmon_priv *priv;
 	int rpm, ret;
+	u8 mode;
 
 	priv = dev_get_drvdata(dev);
 	switch (type) {
@@ -2445,11 +2447,13 @@ static int hp_wmi_hwmon_read(struct device *dev, enum hwmon_sensor_types type,
 			*val = rpm_to_pwm(rpm / 100, priv);
 			return 0;
 		}
-		switch (priv->mode) {
+		scoped_guard(mutex, &priv->lock)
+			mode = priv->mode;
+		switch (mode) {
 		case PWM_MODE_MAX:
 		case PWM_MODE_MANUAL:
 		case PWM_MODE_AUTO:
-			*val = priv->mode;
+			*val = mode;
 			return 0;
 		default:
 			/* shouldn't happen */
@@ -2467,6 +2471,7 @@ static int hp_wmi_hwmon_write(struct device *dev, enum hwmon_sensor_types type,
 	int rpm;
 
 	priv = dev_get_drvdata(dev);
+	guard(mutex)(&priv->lock);
 	switch (type) {
 	case hwmon_pwm:
 		if (attr == hwmon_pwm_input) {
@@ -2535,6 +2540,8 @@ static void hp_wmi_hwmon_keep_alive_handler(struct work_struct *work)
 
 	dwork = to_delayed_work(work);
 	priv = container_of(dwork, struct hp_wmi_hwmon_priv, keep_alive_dwork);
+
+	guard(mutex)(&priv->lock);
 	/*
 	 * Re-apply the current hwmon context settings.
 	 * NOTE: hp_wmi_apply_fan_settings will handle the re-scheduling.
@@ -2591,6 +2598,10 @@ static int hp_wmi_hwmon_init(void)
 	if (!priv)
 		return -ENOMEM;
 
+	ret = devm_mutex_init(dev, &priv->lock);
+	if (ret)
+		return ret;
+
 	ret = hp_wmi_setup_fan_settings(priv);
 	if (ret)
 		return ret;

--
2.53.0

maintainer
#6 lore raw
In reply to Emre Cecanpunar
Quoted 8 lines · Emre Cecanpunar

Bug fixes for the Victus S manual fan control:
- Propagate fan_count_userdefine_trigger() errors
- Fix cancel_delayed_work_sync() deadlock from work handler
- Use mod_delayed_work() for efficient keep-alive timer resets
- Fix u8 underflow in gpu_delta calculation
- Add mutex for concurrent hwmon state access

[...]

Thank you for your contribution, it has been applied to my local review-ilpo-next branch. Note it will show up in the public platform-drivers-x86/review-ilpo-next branch only once I've pushed my local branch there, which might take a while.

The list of commits applied:
[1/5] platform/x86: hp-wmi: fix ignored return values in fan settings

      commit: 7265b57fbc32782d02bdb8d865ba0d8efa209c8c
[2/5] platform/x86: hp-wmi: avoid cancel_delayed_work_sync from work handler
      commit: 249ddba9c0ba4453c0a6bc0e3626e7864751d940
[3/5] platform/x86: hp-wmi: use mod_delayed_work to reset keep-alive timer
      commit: 6297443beb0c5606399ec7d4f4b335e2e7379147
[4/5] platform/x86: hp-wmi: fix u8 underflow in gpu_delta calculation
      commit: cb4daa450f05447c1f914eaef75b2577c25a0fcd
[5/5] platform/x86: hp-wmi: add locking for concurrent hwmon access
      commit: 5969c55e2145368254194edbe0e64880314be69f

--
i.