* [PATCH v2 1/2] platform/x86: asus-wmi: Fix inconsistent use of thermal policies
2024-11-07 0:38 [PATCH v2 0/2] Fix thermal profile handling Armin Wolf
@ 2024-11-07 0:38 ` Armin Wolf
2024-11-07 9:51 ` Hans de Goede
2024-11-07 0:38 ` [PATCH v2 2/2] platform/x86: asus-wmi: Use platform_profile_cycle() Armin Wolf
` (2 subsequent siblings)
3 siblings, 1 reply; 7+ messages in thread
From: Armin Wolf @ 2024-11-07 0:38 UTC (permalink / raw)
To: corentin.chary, luke, mohamed.ghanmi
Cc: srinivas.pandruvada, hdegoede, ilpo.jarvinen, Michael,
casey.g.bowman, platform-driver-x86, linux-kernel
When changing the thermal policy using the platform profile API,
a Vivobook thermal policy is stored in throttle_thermal_policy_mode.
However everywhere else a normal thermal policy is stored inside this
variable, potentially confusing the platform profile.
Fix this by always storing normal thermal policy values inside
throttle_thermal_policy_mode and only do the conversion when writing
the thermal policy to hardware. This also fixes the order in which
throttle_thermal_policy_switch_next() steps through the thermal modes
on Vivobook machines.
Tested-by: Casey G Bowman <casey.g.bowman@intel.com>
Fixes: bcbfcebda2cb ("platform/x86: asus-wmi: add support for vivobook fan profiles")
Signed-off-by: Armin Wolf <W_Armin@gmx.de>
---
drivers/platform/x86/asus-wmi.c | 64 +++++++++++----------------------
1 file changed, 21 insertions(+), 43 deletions(-)
diff --git a/drivers/platform/x86/asus-wmi.c b/drivers/platform/x86/asus-wmi.c
index ab9342a01a48..ce60835d0303 100644
--- a/drivers/platform/x86/asus-wmi.c
+++ b/drivers/platform/x86/asus-wmi.c
@@ -3696,10 +3696,28 @@ static int asus_wmi_custom_fan_curve_init(struct asus_wmi *asus)
/* Throttle thermal policy ****************************************************/
static int throttle_thermal_policy_write(struct asus_wmi *asus)
{
- u8 value = asus->throttle_thermal_policy_mode;
u32 retval;
+ u8 value;
int err;
+ if (asus->throttle_thermal_policy_dev == ASUS_WMI_DEVID_THROTTLE_THERMAL_POLICY_VIVO) {
+ switch (asus->throttle_thermal_policy_mode) {
+ case ASUS_THROTTLE_THERMAL_POLICY_DEFAULT:
+ value = ASUS_THROTTLE_THERMAL_POLICY_DEFAULT_VIVO;
+ break;
+ case ASUS_THROTTLE_THERMAL_POLICY_OVERBOOST:
+ value = ASUS_THROTTLE_THERMAL_POLICY_OVERBOOST_VIVO;
+ break;
+ case ASUS_THROTTLE_THERMAL_POLICY_SILENT:
+ value = ASUS_THROTTLE_THERMAL_POLICY_SILENT_VIVO;
+ break;
+ default:
+ return -EINVAL;
+ }
+ } else {
+ value = asus->throttle_thermal_policy_mode;
+ }
+
err = asus_wmi_set_devstate(asus->throttle_thermal_policy_dev,
value, &retval);
@@ -3804,46 +3822,6 @@ static ssize_t throttle_thermal_policy_store(struct device *dev,
static DEVICE_ATTR_RW(throttle_thermal_policy);
/* Platform profile ***********************************************************/
-static int asus_wmi_platform_profile_to_vivo(struct asus_wmi *asus, int mode)
-{
- bool vivo;
-
- vivo = asus->throttle_thermal_policy_dev == ASUS_WMI_DEVID_THROTTLE_THERMAL_POLICY_VIVO;
-
- if (vivo) {
- switch (mode) {
- case ASUS_THROTTLE_THERMAL_POLICY_DEFAULT:
- return ASUS_THROTTLE_THERMAL_POLICY_DEFAULT_VIVO;
- case ASUS_THROTTLE_THERMAL_POLICY_OVERBOOST:
- return ASUS_THROTTLE_THERMAL_POLICY_OVERBOOST_VIVO;
- case ASUS_THROTTLE_THERMAL_POLICY_SILENT:
- return ASUS_THROTTLE_THERMAL_POLICY_SILENT_VIVO;
- }
- }
-
- return mode;
-}
-
-static int asus_wmi_platform_profile_mode_from_vivo(struct asus_wmi *asus, int mode)
-{
- bool vivo;
-
- vivo = asus->throttle_thermal_policy_dev == ASUS_WMI_DEVID_THROTTLE_THERMAL_POLICY_VIVO;
-
- if (vivo) {
- switch (mode) {
- case ASUS_THROTTLE_THERMAL_POLICY_DEFAULT_VIVO:
- return ASUS_THROTTLE_THERMAL_POLICY_DEFAULT;
- case ASUS_THROTTLE_THERMAL_POLICY_OVERBOOST_VIVO:
- return ASUS_THROTTLE_THERMAL_POLICY_OVERBOOST;
- case ASUS_THROTTLE_THERMAL_POLICY_SILENT_VIVO:
- return ASUS_THROTTLE_THERMAL_POLICY_SILENT;
- }
- }
-
- return mode;
-}
-
static int asus_wmi_platform_profile_get(struct platform_profile_handler *pprof,
enum platform_profile_option *profile)
{
@@ -3853,7 +3831,7 @@ static int asus_wmi_platform_profile_get(struct platform_profile_handler *pprof,
asus = container_of(pprof, struct asus_wmi, platform_profile_handler);
tp = asus->throttle_thermal_policy_mode;
- switch (asus_wmi_platform_profile_mode_from_vivo(asus, tp)) {
+ switch (tp) {
case ASUS_THROTTLE_THERMAL_POLICY_DEFAULT:
*profile = PLATFORM_PROFILE_BALANCED;
break;
@@ -3892,7 +3870,7 @@ static int asus_wmi_platform_profile_set(struct platform_profile_handler *pprof,
return -EOPNOTSUPP;
}
- asus->throttle_thermal_policy_mode = asus_wmi_platform_profile_to_vivo(asus, tp);
+ asus->throttle_thermal_policy_mode = tp;
return throttle_thermal_policy_write(asus);
}
--
2.39.5
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH v2 1/2] platform/x86: asus-wmi: Fix inconsistent use of thermal policies
2024-11-07 0:38 ` [PATCH v2 1/2] platform/x86: asus-wmi: Fix inconsistent use of thermal policies Armin Wolf
@ 2024-11-07 9:51 ` Hans de Goede
0 siblings, 0 replies; 7+ messages in thread
From: Hans de Goede @ 2024-11-07 9:51 UTC (permalink / raw)
To: Armin Wolf, corentin.chary, luke, mohamed.ghanmi
Cc: srinivas.pandruvada, ilpo.jarvinen, Michael, casey.g.bowman,
platform-driver-x86, linux-kernel
Hi,
On 7-Nov-24 1:38 AM, Armin Wolf wrote:
> When changing the thermal policy using the platform profile API,
> a Vivobook thermal policy is stored in throttle_thermal_policy_mode.
>
> However everywhere else a normal thermal policy is stored inside this
> variable, potentially confusing the platform profile.
>
> Fix this by always storing normal thermal policy values inside
> throttle_thermal_policy_mode and only do the conversion when writing
> the thermal policy to hardware. This also fixes the order in which
> throttle_thermal_policy_switch_next() steps through the thermal modes
> on Vivobook machines.
>
> Tested-by: Casey G Bowman <casey.g.bowman@intel.com>
> Fixes: bcbfcebda2cb ("platform/x86: asus-wmi: add support for vivobook fan profiles")
> Signed-off-by: Armin Wolf <W_Armin@gmx.de>
Thanks, patch looks good to me:
Reviewed-by: Hans de Goede <hdegoede@redhat.com>
Regards,
Hans
> ---
> drivers/platform/x86/asus-wmi.c | 64 +++++++++++----------------------
> 1 file changed, 21 insertions(+), 43 deletions(-)
>
> diff --git a/drivers/platform/x86/asus-wmi.c b/drivers/platform/x86/asus-wmi.c
> index ab9342a01a48..ce60835d0303 100644
> --- a/drivers/platform/x86/asus-wmi.c
> +++ b/drivers/platform/x86/asus-wmi.c
> @@ -3696,10 +3696,28 @@ static int asus_wmi_custom_fan_curve_init(struct asus_wmi *asus)
> /* Throttle thermal policy ****************************************************/
> static int throttle_thermal_policy_write(struct asus_wmi *asus)
> {
> - u8 value = asus->throttle_thermal_policy_mode;
> u32 retval;
> + u8 value;
> int err;
>
> + if (asus->throttle_thermal_policy_dev == ASUS_WMI_DEVID_THROTTLE_THERMAL_POLICY_VIVO) {
> + switch (asus->throttle_thermal_policy_mode) {
> + case ASUS_THROTTLE_THERMAL_POLICY_DEFAULT:
> + value = ASUS_THROTTLE_THERMAL_POLICY_DEFAULT_VIVO;
> + break;
> + case ASUS_THROTTLE_THERMAL_POLICY_OVERBOOST:
> + value = ASUS_THROTTLE_THERMAL_POLICY_OVERBOOST_VIVO;
> + break;
> + case ASUS_THROTTLE_THERMAL_POLICY_SILENT:
> + value = ASUS_THROTTLE_THERMAL_POLICY_SILENT_VIVO;
> + break;
> + default:
> + return -EINVAL;
> + }
> + } else {
> + value = asus->throttle_thermal_policy_mode;
> + }
> +
> err = asus_wmi_set_devstate(asus->throttle_thermal_policy_dev,
> value, &retval);
>
> @@ -3804,46 +3822,6 @@ static ssize_t throttle_thermal_policy_store(struct device *dev,
> static DEVICE_ATTR_RW(throttle_thermal_policy);
>
> /* Platform profile ***********************************************************/
> -static int asus_wmi_platform_profile_to_vivo(struct asus_wmi *asus, int mode)
> -{
> - bool vivo;
> -
> - vivo = asus->throttle_thermal_policy_dev == ASUS_WMI_DEVID_THROTTLE_THERMAL_POLICY_VIVO;
> -
> - if (vivo) {
> - switch (mode) {
> - case ASUS_THROTTLE_THERMAL_POLICY_DEFAULT:
> - return ASUS_THROTTLE_THERMAL_POLICY_DEFAULT_VIVO;
> - case ASUS_THROTTLE_THERMAL_POLICY_OVERBOOST:
> - return ASUS_THROTTLE_THERMAL_POLICY_OVERBOOST_VIVO;
> - case ASUS_THROTTLE_THERMAL_POLICY_SILENT:
> - return ASUS_THROTTLE_THERMAL_POLICY_SILENT_VIVO;
> - }
> - }
> -
> - return mode;
> -}
> -
> -static int asus_wmi_platform_profile_mode_from_vivo(struct asus_wmi *asus, int mode)
> -{
> - bool vivo;
> -
> - vivo = asus->throttle_thermal_policy_dev == ASUS_WMI_DEVID_THROTTLE_THERMAL_POLICY_VIVO;
> -
> - if (vivo) {
> - switch (mode) {
> - case ASUS_THROTTLE_THERMAL_POLICY_DEFAULT_VIVO:
> - return ASUS_THROTTLE_THERMAL_POLICY_DEFAULT;
> - case ASUS_THROTTLE_THERMAL_POLICY_OVERBOOST_VIVO:
> - return ASUS_THROTTLE_THERMAL_POLICY_OVERBOOST;
> - case ASUS_THROTTLE_THERMAL_POLICY_SILENT_VIVO:
> - return ASUS_THROTTLE_THERMAL_POLICY_SILENT;
> - }
> - }
> -
> - return mode;
> -}
> -
> static int asus_wmi_platform_profile_get(struct platform_profile_handler *pprof,
> enum platform_profile_option *profile)
> {
> @@ -3853,7 +3831,7 @@ static int asus_wmi_platform_profile_get(struct platform_profile_handler *pprof,
> asus = container_of(pprof, struct asus_wmi, platform_profile_handler);
> tp = asus->throttle_thermal_policy_mode;
>
> - switch (asus_wmi_platform_profile_mode_from_vivo(asus, tp)) {
> + switch (tp) {
> case ASUS_THROTTLE_THERMAL_POLICY_DEFAULT:
> *profile = PLATFORM_PROFILE_BALANCED;
> break;
> @@ -3892,7 +3870,7 @@ static int asus_wmi_platform_profile_set(struct platform_profile_handler *pprof,
> return -EOPNOTSUPP;
> }
>
> - asus->throttle_thermal_policy_mode = asus_wmi_platform_profile_to_vivo(asus, tp);
> + asus->throttle_thermal_policy_mode = tp;
> return throttle_thermal_policy_write(asus);
> }
>
> --
> 2.39.5
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH v2 2/2] platform/x86: asus-wmi: Use platform_profile_cycle()
2024-11-07 0:38 [PATCH v2 0/2] Fix thermal profile handling Armin Wolf
2024-11-07 0:38 ` [PATCH v2 1/2] platform/x86: asus-wmi: Fix inconsistent use of thermal policies Armin Wolf
@ 2024-11-07 0:38 ` Armin Wolf
2024-11-07 9:52 ` Hans de Goede
2024-11-11 7:00 ` [PATCH v2 0/2] Fix thermal profile handling Armin Wolf
2024-11-12 17:17 ` Ilpo Järvinen
3 siblings, 1 reply; 7+ messages in thread
From: Armin Wolf @ 2024-11-07 0:38 UTC (permalink / raw)
To: corentin.chary, luke, mohamed.ghanmi
Cc: srinivas.pandruvada, hdegoede, ilpo.jarvinen, Michael,
casey.g.bowman, platform-driver-x86, linux-kernel
Replace throttle_thermal_policy_switch_next() with
platform_profile_cycle() to reduce code duplication and avoid a
potential race condition with the platform profile handler.
Suggested-by: Hans de Goede <hdegoede@redhat.com>
Signed-off-by: Armin Wolf <W_Armin@gmx.de>
---
drivers/platform/x86/asus-wmi.c | 24 +-----------------------
1 file changed, 1 insertion(+), 23 deletions(-)
diff --git a/drivers/platform/x86/asus-wmi.c b/drivers/platform/x86/asus-wmi.c
index ce60835d0303..ba8b6d028f9f 100644
--- a/drivers/platform/x86/asus-wmi.c
+++ b/drivers/platform/x86/asus-wmi.c
@@ -3755,28 +3755,6 @@ static int throttle_thermal_policy_set_default(struct asus_wmi *asus)
return throttle_thermal_policy_write(asus);
}
-static int throttle_thermal_policy_switch_next(struct asus_wmi *asus)
-{
- u8 new_mode = asus->throttle_thermal_policy_mode + 1;
- int err;
-
- if (new_mode > PLATFORM_PROFILE_MAX)
- new_mode = ASUS_THROTTLE_THERMAL_POLICY_DEFAULT;
-
- asus->throttle_thermal_policy_mode = new_mode;
- err = throttle_thermal_policy_write(asus);
- if (err)
- return err;
-
- /*
- * Ensure that platform_profile updates userspace with the change to ensure
- * that platform_profile and throttle_thermal_policy_mode are in sync.
- */
- platform_profile_notify();
-
- return 0;
-}
-
static ssize_t throttle_thermal_policy_show(struct device *dev,
struct device_attribute *attr, char *buf)
{
@@ -4301,7 +4279,7 @@ static void asus_wmi_handle_event_code(int code, struct asus_wmi *asus)
if (asus->fan_boost_mode_available)
fan_boost_mode_switch_next(asus);
if (asus->throttle_thermal_policy_dev)
- throttle_thermal_policy_switch_next(asus);
+ platform_profile_cycle();
return;
}
--
2.39.5
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH v2 2/2] platform/x86: asus-wmi: Use platform_profile_cycle()
2024-11-07 0:38 ` [PATCH v2 2/2] platform/x86: asus-wmi: Use platform_profile_cycle() Armin Wolf
@ 2024-11-07 9:52 ` Hans de Goede
0 siblings, 0 replies; 7+ messages in thread
From: Hans de Goede @ 2024-11-07 9:52 UTC (permalink / raw)
To: Armin Wolf, corentin.chary, luke, mohamed.ghanmi
Cc: srinivas.pandruvada, ilpo.jarvinen, Michael, casey.g.bowman,
platform-driver-x86, linux-kernel
Hi,
On 7-Nov-24 1:38 AM, Armin Wolf wrote:
> Replace throttle_thermal_policy_switch_next() with
> platform_profile_cycle() to reduce code duplication and avoid a
> potential race condition with the platform profile handler.
>
> Suggested-by: Hans de Goede <hdegoede@redhat.com>
> Signed-off-by: Armin Wolf <W_Armin@gmx.de>
Thanks, patch looks good to me:
Reviewed-by: Hans de Goede <hdegoede@redhat.com>
Regards,
Hans
> ---
> drivers/platform/x86/asus-wmi.c | 24 +-----------------------
> 1 file changed, 1 insertion(+), 23 deletions(-)
>
> diff --git a/drivers/platform/x86/asus-wmi.c b/drivers/platform/x86/asus-wmi.c
> index ce60835d0303..ba8b6d028f9f 100644
> --- a/drivers/platform/x86/asus-wmi.c
> +++ b/drivers/platform/x86/asus-wmi.c
> @@ -3755,28 +3755,6 @@ static int throttle_thermal_policy_set_default(struct asus_wmi *asus)
> return throttle_thermal_policy_write(asus);
> }
>
> -static int throttle_thermal_policy_switch_next(struct asus_wmi *asus)
> -{
> - u8 new_mode = asus->throttle_thermal_policy_mode + 1;
> - int err;
> -
> - if (new_mode > PLATFORM_PROFILE_MAX)
> - new_mode = ASUS_THROTTLE_THERMAL_POLICY_DEFAULT;
> -
> - asus->throttle_thermal_policy_mode = new_mode;
> - err = throttle_thermal_policy_write(asus);
> - if (err)
> - return err;
> -
> - /*
> - * Ensure that platform_profile updates userspace with the change to ensure
> - * that platform_profile and throttle_thermal_policy_mode are in sync.
> - */
> - platform_profile_notify();
> -
> - return 0;
> -}
> -
> static ssize_t throttle_thermal_policy_show(struct device *dev,
> struct device_attribute *attr, char *buf)
> {
> @@ -4301,7 +4279,7 @@ static void asus_wmi_handle_event_code(int code, struct asus_wmi *asus)
> if (asus->fan_boost_mode_available)
> fan_boost_mode_switch_next(asus);
> if (asus->throttle_thermal_policy_dev)
> - throttle_thermal_policy_switch_next(asus);
> + platform_profile_cycle();
> return;
>
> }
> --
> 2.39.5
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2 0/2] Fix thermal profile handling
2024-11-07 0:38 [PATCH v2 0/2] Fix thermal profile handling Armin Wolf
2024-11-07 0:38 ` [PATCH v2 1/2] platform/x86: asus-wmi: Fix inconsistent use of thermal policies Armin Wolf
2024-11-07 0:38 ` [PATCH v2 2/2] platform/x86: asus-wmi: Use platform_profile_cycle() Armin Wolf
@ 2024-11-11 7:00 ` Armin Wolf
2024-11-12 17:17 ` Ilpo Järvinen
3 siblings, 0 replies; 7+ messages in thread
From: Armin Wolf @ 2024-11-11 7:00 UTC (permalink / raw)
To: corentin.chary, luke, mohamed.ghanmi
Cc: srinivas.pandruvada, hdegoede, ilpo.jarvinen, Michael,
casey.g.bowman, platform-driver-x86, linux-kernel
Am 07.11.24 um 01:38 schrieb Armin Wolf:
> When support for Vivobook fan profiles was added, the new thermal
> profiles where used inconsistently.
>
> This patch series aims to fix this issue. Compile-tested only.
Whats your opinion on this, Ilpo? The first patch was already tested by Casey, and
the second patch should be low risk despite being only compile-tested by me.
Thanks,
Armin Wolf
> Changes since v1:
> - drop already applied patch
> - change commit description of first patch
> - add second patch based on a suggestion of Hans
>
> Armin Wolf (2):
> platform/x86: asus-wmi: Fix inconsistent use of thermal policies
> platform/x86: asus-wmi: Use platform_profile_cycle()
>
> drivers/platform/x86/asus-wmi.c | 88 +++++++++------------------------
> 1 file changed, 22 insertions(+), 66 deletions(-)
>
> --
> 2.39.5
>
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2 0/2] Fix thermal profile handling
2024-11-07 0:38 [PATCH v2 0/2] Fix thermal profile handling Armin Wolf
` (2 preceding siblings ...)
2024-11-11 7:00 ` [PATCH v2 0/2] Fix thermal profile handling Armin Wolf
@ 2024-11-12 17:17 ` Ilpo Järvinen
3 siblings, 0 replies; 7+ messages in thread
From: Ilpo Järvinen @ 2024-11-12 17:17 UTC (permalink / raw)
To: corentin.chary, luke, mohamed.ghanmi, Armin Wolf
Cc: srinivas.pandruvada, hdegoede, Michael, casey.g.bowman,
platform-driver-x86, linux-kernel
On Thu, 07 Nov 2024 01:38:09 +0100, Armin Wolf wrote:
> When support for Vivobook fan profiles was added, the new thermal
> profiles where used inconsistently.
>
> This patch series aims to fix this issue. Compile-tested only.
>
> Changes since v1:
> - drop already applied patch
> - change commit description of first patch
> - add second patch based on a suggestion of Hans
>
> [...]
Thank you for your contribution, it has been applied to my local
review-ilpo branch. Note it will show up in the public
platform-drivers-x86/review-ilpo branch only once I've pushed my
local branch there, which might take a while.
The list of commits applied:
[1/2] platform/x86: asus-wmi: Fix inconsistent use of thermal policies
commit: 895085ec3f2ed7a26389943729e2904df1f88dc0
[2/2] platform/x86: asus-wmi: Use platform_profile_cycle()
commit: b0955ce555478e24ed34c7d14f62fdf9c07b15cb
--
i.
^ permalink raw reply [flat|nested] 7+ messages in thread