[PATCH 0/5] platform/x86: hp-wmi: fix bugs in Victus S fan control

[PATCH 0/5] platform/x86: hp-wmi: fix bugs in Victus S fan control

[Emre Cecanpunar]

This series fixes five bugs in the hwmon fan control code added for
HP Victus S laptops:

- Return values from WMI calls were silently discarded in two paths
  of hp_wmi_apply_fan_settings(), and the function's own return value
  was ignored by both of its callers.
- cancel_delayed_work_sync() was called from inside the work handler
  it was trying to cancel, causing a potential deadlock.
- schedule_delayed_work() does not reset a pending timer, so rapid
  back-to-back fan updates did not extend the keep-alive window.
- u8 arithmetic was used to compute a signed delta, producing silent
  unsigned wrap-around on malformed firmware fan tables.
- mode and pwm fields in hp_wmi_hwmon_priv were accessed from both
  sysfs and workqueue context without any synchronisation.

Patches are ordered so that each one applies cleanly on top of the
previous. Patches 1-4 are independent fixes; patch 5 (locking) builds
on patch 2 (cancel_delayed_work) to avoid a lock-ordering issue.

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

-- 
2.53.0

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

[Emre Cecanpunar]

hp_wmi_get_fan_count_userdefine_trigger() can fail, but its return
value was silently discarded in the PWM_MODE_MAX and PWM_MODE_AUTO
cases of hp_wmi_apply_fan_settings(). The same function's return value
is already checked in hp_wmi_fan_speed_set(). Propagate errors
consistently.

hp_wmi_apply_fan_settings() itself returned an error code in all
paths, but its callers hp_wmi_hwmon_keep_alive_handler() and
hp_wmi_hwmon_init() both discarded the return value. Log failures via
pr_warn_ratelimited() in the keep-alive handler (rate-limited because
it fires every 90 seconds) and dev_warn() in init (non-fatal: the
hwmon device is already registered).

Also remove the unreachable "return 0" after the switch statement in
hp_wmi_apply_fan_settings(): every case branch already returns
explicitly.

Signed-off-by: Emre Cecanpunar <emreleno@gmail.com>
---
 drivers/platform/x86/hp/hp-wmi.c | 23 ++++++++++++++++-------
 1 file changed, 16 insertions(+), 7 deletions(-)

diff --git a/drivers/platform/x86/hp/hp-wmi.c b/drivers/platform/x86/hp/hp-wmi.c
index 68ede7e5757a..0cb2a2b31998 100644
--- a/drivers/platform/x86/hp/hp-wmi.c
+++ b/drivers/platform/x86/hp/hp-wmi.c
@@ -2334,8 +2334,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;
@@ -2353,7 +2356,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);
@@ -2366,8 +2371,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,
@@ -2509,6 +2512,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);
@@ -2516,7 +2520,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)
@@ -2578,7 +2585,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;
 }
-- 
2.53.0

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

[Emre Cecanpunar]

hp_wmi_apply_fan_settings() calls cancel_delayed_work_sync() in the
PWM_MODE_AUTO case to stop the keep-alive timer. This function is also
called from hp_wmi_hwmon_keep_alive_handler(), which is itself a
delayed work handler.

If the keep-alive work is executing and the fan mode happens to be
AUTO (e.g. a concurrent sysfs write changed it just before the handler
read priv->mode), cancel_delayed_work_sync() tries to flush the work
item it is already running inside, causing a deadlock.

The workqueue documentation explicitly requires callers to ensure this
self-flush situation does not arise.

Replace cancel_delayed_work_sync() with cancel_delayed_work() in
hp_wmi_apply_fan_settings(). The non-synchronous variant cancels
pending work without waiting for a running instance to complete,
avoiding the deadlock. The synchronous cancel in hp_wmi_bios_remove()
is unaffected and still provides the final cleanup guarantee at driver
removal time.

Signed-off-by: Emre Cecanpunar <emreleno@gmail.com>
---
 drivers/platform/x86/hp/hp-wmi.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/platform/x86/hp/hp-wmi.c b/drivers/platform/x86/hp/hp-wmi.c
index 0cb2a2b31998..41769c0861e5 100644
--- a/drivers/platform/x86/hp/hp-wmi.c
+++ b/drivers/platform/x86/hp/hp-wmi.c
@@ -2365,7 +2365,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 */
-- 
2.53.0

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

[Emre Cecanpunar]

The keep-alive mechanism re-applies fan settings every 90 seconds to
prevent the firmware from reverting to automatic mode after its 120 s
timeout. Fan settings are also applied immediately when the user writes
to the hwmon sysfs interface.

schedule_delayed_work() is a no-op when the work item is already
pending. If the user updates fan settings at T=85s, the keep-alive
still fires at T=90s (5 s after the update) instead of 90 s later.
This shortens the effective keep-alive window and may allow the
firmware timeout to expire between cycles.

Replace schedule_delayed_work() with mod_delayed_work() in both the
PWM_MODE_MAX and PWM_MODE_MANUAL cases. mod_delayed_work() resets the
delay to KEEP_ALIVE_DELAY_SECS from the time of each call, whether or
not the work is already pending, ensuring each user action and each
keep-alive expiry restarts the full 90 s window.

Signed-off-by: Emre Cecanpunar <emreleno@gmail.com>
---
 drivers/platform/x86/hp/hp-wmi.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/platform/x86/hp/hp-wmi.c b/drivers/platform/x86/hp/hp-wmi.c
index 41769c0861e5..a29f34588055 100644
--- a/drivers/platform/x86/hp/hp-wmi.c
+++ b/drivers/platform/x86/hp/hp-wmi.c
@@ -2342,8 +2342,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())
@@ -2351,8 +2351,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()) {
-- 
2.53.0

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

[Emre Cecanpunar]

gpu_delta was declared as u8 and computed as the difference of two u8
fan RPM values from the firmware fan table. If gpu_rpm < cpu_rpm, the
subtraction wraps around modulo 256, producing a large positive value
(e.g. 10 - 20 = 246 as u8). This value is then added to every
requested fan speed in hp_wmi_fan_speed_set(), causing the GPU fan to
be clamped to U8_MAX on almost every write.

The fan table originates from an undocumented WMI interface; a
firmware bug or unexpected layout could produce this condition.

Change gpu_delta to int and perform the subtraction in signed
arithmetic. On underflow, emit a warning and treat the delta as zero
so the GPU fan tracks the CPU fan directly rather than saturating.

Signed-off-by: Emre Cecanpunar <emreleno@gmail.com>
---
 drivers/platform/x86/hp/hp-wmi.c | 13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)

diff --git a/drivers/platform/x86/hp/hp-wmi.c b/drivers/platform/x86/hp/hp-wmi.c
index a29f34588055..b8f22d70e996 100644
--- a/drivers/platform/x86/hp/hp-wmi.c
+++ b/drivers/platform/x86/hp/hp-wmi.c
@@ -2530,8 +2530,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;
@@ -2553,10 +2553,15 @@ 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 = (int)fan_table->entries[0].gpu_rpm -
+		    (int)fan_table->entries[0].cpu_rpm;
+	if (gpu_delta < 0) {
+		pr_warn("fan table has gpu_rpm < cpu_rpm, ignoring gpu delta\n");
+		gpu_delta = 0;
+	}
 	priv->min_rpm = min_rpm;
 	priv->max_rpm = max_rpm;
-	priv->gpu_delta = gpu_delta;
+	priv->gpu_delta = (u8)gpu_delta;
 
 	return 0;
 }
-- 
2.53.0

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

[Krishna Chomal]

On Sat, Mar 21, 2026 at 02:55:56AM +0300, Emre Cecanpunar wrote:
>gpu_delta was declared as u8 and computed as the difference of two u8
>fan RPM values from the firmware fan table. If gpu_rpm < cpu_rpm, the
>subtraction wraps around modulo 256, producing a large positive value
>(e.g. 10 - 20 = 246 as u8). This value is then added to every
>requested fan speed in hp_wmi_fan_speed_set(), causing the GPU fan to
>be clamped to U8_MAX on almost every write.
>
>The fan table originates from an undocumented WMI interface; a
>firmware bug or unexpected layout could produce this condition.
>
>Change gpu_delta to int and perform the subtraction in signed
>arithmetic. On underflow, emit a warning and treat the delta as zero
>so the GPU fan tracks the CPU fan directly rather than saturating.
>
>Signed-off-by: Emre Cecanpunar <emreleno@gmail.com>
>---
> drivers/platform/x86/hp/hp-wmi.c | 13 +++++++++----
> 1 file changed, 9 insertions(+), 4 deletions(-)
>
>diff --git a/drivers/platform/x86/hp/hp-wmi.c b/drivers/platform/x86/hp/hp-wmi.c
>index a29f34588055..b8f22d70e996 100644
>--- a/drivers/platform/x86/hp/hp-wmi.c
>+++ b/drivers/platform/x86/hp/hp-wmi.c
>@@ -2530,8 +2530,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;
>@@ -2553,10 +2553,15 @@ 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 = (int)fan_table->entries[0].gpu_rpm -
>+		    (int)fan_table->entries[0].cpu_rpm;
>+	if (gpu_delta < 0) {
>+		pr_warn("fan table has gpu_rpm < cpu_rpm, ignoring gpu delta\n");

Since some boards have CPU_RPM > GPU_RPM in their firmware tables, this
subtraction can naturally result in a negative value. I don't think
we should pr_warn() what is essentially intended hardware behavior.

>+		gpu_delta = 0;
>+	}

Instead of capping gpu_delta to 0, I suggest we update the definition
of 'gpu_delta' in struct hp_wmi_hwmon_priv to an 'int'.

Since hp_wmi_fan_speed_set() already performs the calculation using
signed integer arithmetic:

     gpu_speed = speed + priv->gpu_delta;
     fan_speed[GPU_FAN] = clamp_val(gpu_speed, 0, U8_MAX);

Using a signed 'gpu_delta' will automatically result in the correct 
GPU_RPM without the risk of it clamping at U8_MAX.

> 	priv->min_rpm = min_rpm;
> 	priv->max_rpm = max_rpm;
>-	priv->gpu_delta = gpu_delta;
>+	priv->gpu_delta = (u8)gpu_delta;
>
> 	return 0;
> }
>-- 
>2.53.0
>

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

[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() running
in a workqueue. There is no synchronisation between them, so a
concurrent sysfs write and keep-alive expiry can observe an
inconsistent combination of mode and pwm (e.g. mode=MANUAL with an
stale pwm value from the previous mode).

Add a mutex to hp_wmi_hwmon_priv that protects mode and pwm. Acquire
it in hp_wmi_hwmon_write() for the full write operation (field update
plus hp_wmi_apply_fan_settings() call), and in
hp_wmi_hwmon_keep_alive_handler() before calling
hp_wmi_apply_fan_settings().

In hp_wmi_hwmon_read(), only the pwm_enable attribute reads priv->mode
and requires the lock. Use scoped_guard() to hold the mutex only for
the mode read, avoiding holding it across unrelated WMI hardware calls.

Signed-off-by: Emre Cecanpunar <emreleno@gmail.com>
---
 drivers/platform/x86/hp/hp-wmi.c | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/drivers/platform/x86/hp/hp-wmi.c b/drivers/platform/x86/hp/hp-wmi.c
index b8f22d70e996..cfb207f09e7b 100644
--- a/drivers/platform/x86/hp/hp-wmi.c
+++ b/drivers/platform/x86/hp/hp-wmi.c
@@ -434,6 +434,7 @@ enum pwm_modes {
 };
 
 struct hp_wmi_hwmon_priv {
+	struct mutex lock;	/* protects mode, pwm */
 	u8 min_rpm;
 	u8 max_rpm;
 	u8 gpu_delta;
@@ -2403,6 +2404,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) {
@@ -2426,11 +2428,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 */
@@ -2448,6 +2452,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) {
@@ -2516,6 +2521,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.
@@ -2577,6 +2584,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;
-- 
2.53.0

Re: [PATCH 0/5] platform/x86: hp-wmi: fix bugs in Victus S fan control

[Krishna Chomal]

On Sat, Mar 21, 2026 at 02:55:52AM +0300, Emre Cecanpunar wrote:
>This series fixes five bugs in the hwmon fan control code added for
>HP Victus S laptops:
>
>- Return values from WMI calls were silently discarded in two paths
>  of hp_wmi_apply_fan_settings(), and the function's own return value
>  was ignored by both of its callers.
>- cancel_delayed_work_sync() was called from inside the work handler
>  it was trying to cancel, causing a potential deadlock.
>- schedule_delayed_work() does not reset a pending timer, so rapid
>  back-to-back fan updates did not extend the keep-alive window.
>- u8 arithmetic was used to compute a signed delta, producing silent
>  unsigned wrap-around on malformed firmware fan tables.
>- mode and pwm fields in hp_wmi_hwmon_priv were accessed from both
>  sysfs and workqueue context without any synchronisation.
>
>Patches are ordered so that each one applies cleanly on top of the
>previous. Patches 1-4 are independent fixes; patch 5 (locking) builds
>on patch 2 (cancel_delayed_work) to avoid a lock-ordering issue.
>
>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 | 59 ++++++++++++++++++++++----------
> 1 file changed, 41 insertions(+), 18 deletions(-)
>
>-- 
>2.53.0
>

Hi Emre,

Thank you for reviewing and catching these bugs. As this was my first
attempt with delayed workqueues, I appreciate the bug fixes, specially
regarding the deadlock.

I have tested this series on my hardware (board ID: 8C78) and can
confirm that the fans still behave correctly with the improvements.