Skip to contentSkip to conversation

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

PATCH Superseded Emre Cecanpunar 2 participants 8 messages Apr 5 – Apr 7 x86 platform drivers

v3 of 4 Superseded Latest is v4 →

lore
Share & tools
lore

Emre Cecanpunar

author
#0 lore raw

v3 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 v2:
- Patch 4: Store gpu_delta as 'int' to handle negative deltas correctly.

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

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 | 55 +++++++++++++++++++++-----------
 1 file changed, 37 insertions(+), 18 deletions(-)

--
2.53.0

author
#1 lore raw

PATCH v3 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")
Signed-off-by Emre Cecanpunar <[email protected]>
---
 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
author
#2 lore raw

PATCH v3 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")
Signed-off-by Emre Cecanpunar <[email protected]>
---
 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
author
#3 lore raw

PATCH v3 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")
Signed-off-by Emre Cecanpunar <[email protected]>

---
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
author
#4 lore raw

PATCH v3 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")
Suggested-by Ilpo Järvinen <[email protected]>
Signed-off-by Emre Cecanpunar <[email protected]>

---
Changes in v2:
- Drop the if (gpu_delta < 0) guard and pr_warn. A negative delta is valid firmware behavior on boards where CPU_RPM > GPU_RPM. Store gpu_delta as int in struct hp_wmi_hwmon_priv so the existing signed arithmetic and clamp_val() in hp_wmi_fan_speed_set() handle the negative case correctly without saturating at U8_MAX.

Changes in v3:
- Removed unnecessary explicit (int) casts. C's integer promotion rules already convert u8 operands to int for arithmetic; the fix only requires storing the result in an int variable.

 drivers/platform/x86/hp/hp-wmi.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)
drivers/platform/x86/hp/hp-wmi.c +5 -4
index 2932cab9aa78..db72ad9da0a5 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;

@@ -2572,7 +2572,8 @@ static int hp_wmi_setup_fan_settings(struct hp_wmi_hwmon_priv *priv)
 
 	min_rpm = fan_table->entries[0].cpu_rpm;
 	max_rpm = fan_table->entries[fan_table->header.num_entries - 1].cpu_rpm;
-	gpu_delta = fan_table->entries[0].gpu_rpm - fan_table->entries[0].cpu_rpm;
+	gpu_delta = fan_table->entries[0].gpu_rpm -
+		    fan_table->entries[0].cpu_rpm;
 	priv->min_rpm = min_rpm;
 	priv->max_rpm = max_rpm;
 	priv->gpu_delta = gpu_delta;
Signature · 1 line
2.53.0
maintainer
#5 lore raw

Re: [PATCH v3 4/5] platform/x86: hp-wmi: fix u8 underflow in gpu_delta calculation

Quoted 52 lines · 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")
Suggested-by Ilpo Järvinen <[email protected]>
Signed-off-by Emre Cecanpunar <[email protected]>

---
Changes in v2:
- Drop the if (gpu_delta < 0) guard and pr_warn. A negative delta is valid firmware behavior on boards where CPU_RPM > GPU_RPM. Store gpu_delta as int in struct hp_wmi_hwmon_priv so the existing signed arithmetic and clamp_val() in hp_wmi_fan_speed_set() handle the negative case correctly without saturating at U8_MAX.

Changes in v3:
- Removed unnecessary explicit (int) casts. C's integer promotion rules already convert u8 operands to int for arithmetic; the fix only requires storing the result in an int variable.

 drivers/platform/x86/hp/hp-wmi.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)
drivers/platform/x86/hp/hp-wmi.c +3 -3
index 2932cab9aa78..db72ad9da0a5 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;

drivers/platform/x86/hp/hp-wmi.c static int hp_wmi_setup_fan_settings(struct hp_wmi_hwmon_priv *priv) · line 2576

Quoted 6 lines
drivers/platform/x86/hp/hp-wmi.c +2 -1
@@ -2572,7 +2572,8 @@ static int hp_wmi_setup_fan_settings(struct hp_wmi_hwmon_priv *priv)
 
 	min_rpm = fan_table->entries[0].cpu_rpm;
 	max_rpm = fan_table->entries[fan_table->header.num_entries - 1].cpu_rpm;
-	gpu_delta = fan_table->entries[0].gpu_rpm - fan_table->entries[0].cpu_rpm;
+	gpu_delta = fan_table->entries[0].gpu_rpm -
+		    fan_table->entries[0].cpu_rpm;

So this part has now become pure newline change (which is unrelated to the intent of this patch)?

Quoted 4 lines
drivers/platform/x86/hp/hp-wmi.c +0 -0
 	priv->min_rpm = min_rpm;
 	priv->max_rpm = max_rpm;
 	priv->gpu_delta = gpu_delta;
Signature · 1 line
 i.
author
#6 lore raw

PATCH v3 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")
Signed-off-by Emre Cecanpunar <[email protected]>
---
 drivers/platform/x86/hp/hp-wmi.c | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)
drivers/platform/x86/hp/hp-wmi.c +11 -2
index db72ad9da0a5..1096fb46cbcd 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.

@@ -2592,6 +2599,8 @@ static int hp_wmi_hwmon_init(void)
 	if (!priv)
 		return -ENOMEM;
 
+	mutex_init(&priv->lock);
+
 	ret = hp_wmi_setup_fan_settings(priv);
 	if (ret)
 		return ret;
Signature · 1 line
2.53.0
maintainer
#7 lore raw

Re: [PATCH v3 5/5] platform/x86: hp-wmi: add locking for concurrent hwmon access

Quoted 73 lines · 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")
Signed-off-by Emre Cecanpunar <[email protected]>
---
 drivers/platform/x86/hp/hp-wmi.c | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)
drivers/platform/x86/hp/hp-wmi.c +9 -2
index db72ad9da0a5..1096fb46cbcd 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.

drivers/platform/x86/hp/hp-wmi.c static int hp_wmi_hwmon_init(void) · line 2602

Quoted 4 lines
drivers/platform/x86/hp/hp-wmi.c +1 -0
@@ -2592,6 +2599,8 @@ static int hp_wmi_hwmon_init(void)
 	if (!priv)
 		return -ENOMEM;
 
+	mutex_init(&priv->lock);

Please use devm_mutex_init() so you don't need to deinit it manually with mutex_destroy(). While changing that, don't forget that devm_*() can fail so remember to handle the error.

Signature · 1 line
 i.