Am 26.10.24 um 11:56 schrieb Hans de Goede:
Hi Armin,
On 25-Oct-24 9:15 PM, 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.
You say "potentially confusing the platform profile", but did you
spot any actual issues when reviewing the code ?
Yes, for example:
1. User sets thermal policy to "1" (overboost) through the throttle_thermal_policy sysfs attr.
2. "1" gets stored inside throttle_thermal_policy_mode.
3. Platform profile will now think that ASUS_THROTTLE_THERMAL_POLICY_SILENT_VIVO (1) is set.
4. Platform profile reports current mode as PLATFORM_PROFILE_QUIET.
=> error!
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.
Fixes: bcbfcebda2cb ("platform/x86: asus-wmi: add support for vivobook fan profiles")
Signed-off-by: Armin Wolf <W_Armin@xxxxxx>
The problem with this approach is that it changes the order in which
we step through the modes in throttle_thermal_policy_switch_next()
after this change we have:
Normal Asus: balanced -> performance -> silent -> balanced -> etc.
Vivobook: balanced -> silent -> performance -> balanced -> etc.
where if we see "silent" as lower performance then the other 2,
the vivobook order is a bit weird.
I wonder if this is a big enough issue to really worry about it
though; and I do like the cleanup / simpler code.
I think users expect the normal Asus switching behavior, but you are right
that this change should be explained inside the commit message.
Note that this also causes a behavior change in
fan_curve_get_factory_default() as I mentioned in my cover-letter.
Since fan_curve_get_factory_default() was introduced before commit bcbfcebda2cb,
i think this patches actually fixes the behavior of this function.
I think that that behavior change might be a good thing to do actually,
but at a minimum it needs to be documented in the commit msg.
Regards,
Hans
I will wait till the maintainer of asus-wmi can clarify whether or not
fan_curve_get_factory_default() expects normal Asus thermal policy values
or not.
Once we resolved this i will send an updated series.
Thanks,
Armin Wolf
---
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
