platform/x86: hp-wmi: Victus S fan control fixes
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
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.
--- drivers/platform/x86/hp/hp-wmi.c | 23 ++++++++++++++++------- 1 file changed, 16 insertions(+), 7 deletions(-)
drivers/platform/x86/hp/hp-wmi.c +16 -7
@@ -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
PATCH v4 2/5 platform/x86: hp-wmi: avoid cancel_delayed_work_sync from work handler
In reply to Emre Cecanpunarhp_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.
--- drivers/platform/x86/hp/hp-wmi.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/platform/x86/hp/hp-wmi.c +1 -1
@@ -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
PATCH v4 3/5 platform/x86: hp-wmi: use mod_delayed_work to reset keep-alive timer
In reply to Emre CecanpunarCurrently, 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.
---
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
@@ -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
PATCH v4 4/5 platform/x86: hp-wmi: fix u8 underflow in gpu_delta calculation
In reply to Emre Cecanpunargpu_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.
---
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
@@ -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
PATCH v4 5/5 platform/x86: hp-wmi: add locking for concurrent hwmon access
In reply to Emre Cecanpunarhp_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.
---
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
@@ -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
Ilpo Järvinen
maintainerQuoted 8 lines · Emre Cecanpunar
Bug fixes for the Victus S manual fan control:
- Propagatefan_count_userdefine_trigger()errors
- Fixcancel_delayed_work_sync()deadlock from work handler
- Usemod_delayed_work()for efficient keep-alive timer resets
- Fix u8 underflow ingpu_deltacalculation
- 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.