| Message ID | 20250224223551.16918-1-luke@ljones.dev (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | platform/x86: asus-wmi: change quiet to low-power | expand |
On 2/24/2025 16:35, Luke Jones wrote: > From: "Luke D. Jones" <luke@ljones.dev> > > Change the profile name "quiet" to "low-power" to match the AMD name. The > primary reason for this is to match AMD naming for platform_profiles and > allow both to match. It does not affect Intel machines. > > The quiet profile is essentially a low-power profile which tweaks > both TDP and fans - this applies to 80+ ASUS laptops. > > Signed-off-by: Luke D. Jones <luke@ljones.dev> Reviewed-by: Mario Limonciello <mario.limonciello@amd.com> IMO - this should have a fixes tag since this should probably go in the 6.14 cycle too. Fixes: 688834743d67 ("ACPI: platform_profile: Allow multiple handlers") > --- > drivers/platform/x86/asus-wmi.c | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/drivers/platform/x86/asus-wmi.c b/drivers/platform/x86/asus-wmi.c > index d22748f1e154..de19c3b3d8fb 100644 > --- a/drivers/platform/x86/asus-wmi.c > +++ b/drivers/platform/x86/asus-wmi.c > @@ -3945,7 +3945,7 @@ static int asus_wmi_platform_profile_get(struct device *dev, > *profile = PLATFORM_PROFILE_PERFORMANCE; > break; > case ASUS_THROTTLE_THERMAL_POLICY_SILENT: > - *profile = PLATFORM_PROFILE_QUIET; > + *profile = PLATFORM_PROFILE_LOW_POWER; > break; > default: > return -EINVAL; > @@ -3969,7 +3969,7 @@ static int asus_wmi_platform_profile_set(struct device *dev, > case PLATFORM_PROFILE_BALANCED: > tp = ASUS_THROTTLE_THERMAL_POLICY_DEFAULT; > break; > - case PLATFORM_PROFILE_QUIET: > + case PLATFORM_PROFILE_LOW_POWER: > tp = ASUS_THROTTLE_THERMAL_POLICY_SILENT; > break; > default: > @@ -3982,7 +3982,7 @@ static int asus_wmi_platform_profile_set(struct device *dev, > > static int asus_wmi_platform_profile_probe(void *drvdata, unsigned long *choices) > { > - set_bit(PLATFORM_PROFILE_QUIET, choices); > + set_bit(PLATFORM_PROFILE_LOW_POWER, choices); > set_bit(PLATFORM_PROFILE_BALANCED, choices); > set_bit(PLATFORM_PROFILE_PERFORMANCE, choices); >
On Mon, 2025-02-24 at 18:39 -0800, Mario Limonciello wrote: > On 2/24/2025 16:35, Luke Jones wrote: > > From: "Luke D. Jones" <luke@ljones.dev> > > > > Change the profile name "quiet" to "low-power" to match the AMD > > name. The > > primary reason for this is to match AMD naming for > > platform_profiles and > > allow both to match. It does not affect Intel machines. > > > > The quiet profile is essentially a low-power profile which tweaks > > both TDP and fans - this applies to 80+ ASUS laptops. > > > > Signed-off-by: Luke D. Jones <luke@ljones.dev> > > Reviewed-by: Mario Limonciello <mario.limonciello@amd.com> > > IMO - this should have a fixes tag since this should probably go in > the > 6.14 cycle too. > > Fixes: 688834743d67 ("ACPI: platform_profile: Allow multiple > handlers") > Good point, thanks. I assume when pulled in this can be added? > > --- > > drivers/platform/x86/asus-wmi.c | 6 +++--- > > 1 file changed, 3 insertions(+), 3 deletions(-) > > > > diff --git a/drivers/platform/x86/asus-wmi.c > > b/drivers/platform/x86/asus-wmi.c > > index d22748f1e154..de19c3b3d8fb 100644 > > --- a/drivers/platform/x86/asus-wmi.c > > +++ b/drivers/platform/x86/asus-wmi.c > > @@ -3945,7 +3945,7 @@ static int > > asus_wmi_platform_profile_get(struct device *dev, > > *profile = PLATFORM_PROFILE_PERFORMANCE; > > break; > > case ASUS_THROTTLE_THERMAL_POLICY_SILENT: > > - *profile = PLATFORM_PROFILE_QUIET; > > + *profile = PLATFORM_PROFILE_LOW_POWER; > > break; > > default: > > return -EINVAL; > > @@ -3969,7 +3969,7 @@ static int > > asus_wmi_platform_profile_set(struct device *dev, > > case PLATFORM_PROFILE_BALANCED: > > tp = ASUS_THROTTLE_THERMAL_POLICY_DEFAULT; > > break; > > - case PLATFORM_PROFILE_QUIET: > > + case PLATFORM_PROFILE_LOW_POWER: > > tp = ASUS_THROTTLE_THERMAL_POLICY_SILENT; > > break; > > default: > > @@ -3982,7 +3982,7 @@ static int > > asus_wmi_platform_profile_set(struct device *dev, > > > > static int asus_wmi_platform_profile_probe(void *drvdata, > > unsigned long *choices) > > { > > - set_bit(PLATFORM_PROFILE_QUIET, choices); > > + set_bit(PLATFORM_PROFILE_LOW_POWER, choices); > > set_bit(PLATFORM_PROFILE_BALANCED, choices); > > set_bit(PLATFORM_PROFILE_PERFORMANCE, choices); > > >
Hi Luke, please add appropriate attribution. Closes: https://lore.kernel.org/all/20250224195059.10185-1-lkml@antheas.dev/ Reported-by: Antheas Kapenekakis <lkml@antheas.dev> For me, this patch series plus the multi-platform profile one constitute a double ABI break. Not only does the legacy sysfs for platform profile regress when there is a second profile handler for a device, but all hardcoded scripts for Asus devices will have to be updated. While I would personally like to avoid this, I am ok with it, given appropriate attribution, since I did go through the effort of reporting it and providing a mitigation. @Mario: you added Reviewed-by to a patch without proper attribution. Let's not rehash our discussion from few days ago. Please try to do better when it comes to attributions in the future. Antheas
On 2/25/2025 06:25, Antheas Kapenekakis wrote: > Hi Luke, > please add appropriate attribution. > > Closes: https://lore.kernel.org/all/20250224195059.10185-1-lkml@antheas.dev/ > Reported-by: Antheas Kapenekakis <lkml@antheas.dev> Good call on adding these tags. > > For me, this patch series plus the multi-platform profile one constitute > a double ABI break. Not only does the legacy sysfs for platform profile > regress when there is a second profile handler for a device, but all > hardcoded scripts for Asus devices will have to be updated. The documentation says to look at platform_profile_choices. To determine what is supported. FWIW this is exactly what power-profiles-daemon does. > > While I would personally like to avoid this, I am ok with it, given > appropriate attribution, since I did go through the effort of reporting it > and providing a mitigation. > > @Mario: you added Reviewed-by to a patch without proper attribution. Let's > not rehash our discussion from few days ago. Please try to do better when > it comes to attributions in the future. > > Antheas It's an oversight, no malice intended. b4 (which most maintainers use) scans the whole thread for tags. Adding them inline as a response is totally fine. If Luke needs to spin a v2 for some reason before this is committed then he can add them as well to the v2.
> The documentation says to look at platform_profile_choices. To > determine what is supported. FWIW this is exactly what > power-profiles-daemon does. Yeah, this one is minor all things considered. Since there is a justification. Even if user error is not the best of justifications. But as I maintain a userspace TDP tool, I can tell you that if the other issue is not fixed, /sys/firmware/acpi/platform_profile is no longer trustworthy due to being able to occlude platform profiles. So a mitigation will be needed for all userspace power utilities. But there is actually very little use currently for /sys/class/platform-profile as AMD pmf is mostly a NOOP, so I think this would be premature and should be avoided. While I do not have a large portfolio of ACPI collections for Thinkpads, HP, Alienware, and Dell that have WMI drivers, I can see amd-pmf popping up on a lot of the late 2024 models of GPD, Ayaneo, Lenovo, and Asus, and going into 2025. So it is worthwhile fixing this now once and for all. Antheas
Am 25.02.25 um 07:13 schrieb Luke Jones: > On Mon, 2025-02-24 at 18:39 -0800, Mario Limonciello wrote: >> On 2/24/2025 16:35, Luke Jones wrote: >>> From: "Luke D. Jones" <luke@ljones.dev> >>> >>> Change the profile name "quiet" to "low-power" to match the AMD >>> name. The >>> primary reason for this is to match AMD naming for >>> platform_profiles and >>> allow both to match. It does not affect Intel machines. >>> >>> The quiet profile is essentially a low-power profile which tweaks >>> both TDP and fans - this applies to 80+ ASUS laptops. >>> >>> Signed-off-by: Luke D. Jones <luke@ljones.dev> >> Reviewed-by: Mario Limonciello <mario.limonciello@amd.com> >> >> IMO - this should have a fixes tag since this should probably go in >> the >> 6.14 cycle too. >> >> Fixes: 688834743d67 ("ACPI: platform_profile: Allow multiple >> handlers") >> > Good point, thanks. I assume when pulled in this can be added? Antheas is concerned that this patch might break brittle userspace scripts like "echo quiet | sudo tee /sys/firmware/acpi/platform_profile". Maybe we should instead change the strategy used by the legacy platform-profile handler when selecting supported profiles? Thanks, Armin Wolff >>> --- >>> drivers/platform/x86/asus-wmi.c | 6 +++--- >>> 1 file changed, 3 insertions(+), 3 deletions(-) >>> >>> diff --git a/drivers/platform/x86/asus-wmi.c >>> b/drivers/platform/x86/asus-wmi.c >>> index d22748f1e154..de19c3b3d8fb 100644 >>> --- a/drivers/platform/x86/asus-wmi.c >>> +++ b/drivers/platform/x86/asus-wmi.c >>> @@ -3945,7 +3945,7 @@ static int >>> asus_wmi_platform_profile_get(struct device *dev, >>> *profile = PLATFORM_PROFILE_PERFORMANCE; >>> break; >>> case ASUS_THROTTLE_THERMAL_POLICY_SILENT: >>> - *profile = PLATFORM_PROFILE_QUIET; >>> + *profile = PLATFORM_PROFILE_LOW_POWER; >>> break; >>> default: >>> return -EINVAL; >>> @@ -3969,7 +3969,7 @@ static int >>> asus_wmi_platform_profile_set(struct device *dev, >>> case PLATFORM_PROFILE_BALANCED: >>> tp = ASUS_THROTTLE_THERMAL_POLICY_DEFAULT; >>> break; >>> - case PLATFORM_PROFILE_QUIET: >>> + case PLATFORM_PROFILE_LOW_POWER: >>> tp = ASUS_THROTTLE_THERMAL_POLICY_SILENT; >>> break; >>> default: >>> @@ -3982,7 +3982,7 @@ static int >>> asus_wmi_platform_profile_set(struct device *dev, >>> >>> static int asus_wmi_platform_profile_probe(void *drvdata, >>> unsigned long *choices) >>> { >>> - set_bit(PLATFORM_PROFILE_QUIET, choices); >>> + set_bit(PLATFORM_PROFILE_LOW_POWER, choices); >>> set_bit(PLATFORM_PROFILE_BALANCED, choices); >>> set_bit(PLATFORM_PROFILE_PERFORMANCE, choices); >>> >
diff --git a/drivers/platform/x86/asus-wmi.c b/drivers/platform/x86/asus-wmi.c index d22748f1e154..de19c3b3d8fb 100644 --- a/drivers/platform/x86/asus-wmi.c +++ b/drivers/platform/x86/asus-wmi.c @@ -3945,7 +3945,7 @@ static int asus_wmi_platform_profile_get(struct device *dev, *profile = PLATFORM_PROFILE_PERFORMANCE; break; case ASUS_THROTTLE_THERMAL_POLICY_SILENT: - *profile = PLATFORM_PROFILE_QUIET; + *profile = PLATFORM_PROFILE_LOW_POWER; break; default: return -EINVAL; @@ -3969,7 +3969,7 @@ static int asus_wmi_platform_profile_set(struct device *dev, case PLATFORM_PROFILE_BALANCED: tp = ASUS_THROTTLE_THERMAL_POLICY_DEFAULT; break; - case PLATFORM_PROFILE_QUIET: + case PLATFORM_PROFILE_LOW_POWER: tp = ASUS_THROTTLE_THERMAL_POLICY_SILENT; break; default: @@ -3982,7 +3982,7 @@ static int asus_wmi_platform_profile_set(struct device *dev, static int asus_wmi_platform_profile_probe(void *drvdata, unsigned long *choices) { - set_bit(PLATFORM_PROFILE_QUIET, choices); + set_bit(PLATFORM_PROFILE_LOW_POWER, choices); set_bit(PLATFORM_PROFILE_BALANCED, choices); set_bit(PLATFORM_PROFILE_PERFORMANCE, choices);