| Message ID | 20250304064745.1073770-2-superm1@kernel.org (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Add quiet/low power compat code | expand |
On Tue, 4 Mar 2025 at 07:48, Mario Limonciello <superm1@kernel.org> wrote: > > From: Mario Limonciello <mario.limonciello@amd.com> > > When two drivers don't support all the same profiles the legacy interface > only exports the common profiles. > > This causes problems for cases where one driver uses low-power but another > uses quiet because the result is that neither is exported to sysfs. > > If one platform profile handler supports quiet and the other > supports low power treat them as the same for the purpose of > the sysfs interface. > > Fixes: 688834743d67 ("ACPI: platform_profile: Allow multiple handlers") > Reported-by: Antheas Kapenekakis <lkml@antheas.dev> > Closes: https://lore.kernel.org/platform-driver-x86/e64b771e-3255-42ad-9257-5b8fc6c24ac9@gmx.de/T/#mc068042dd29df36c16c8af92664860fc4763974b > Signed-off-by: Mario Limonciello <mario.limonciello@amd.com> > --- > drivers/acpi/platform_profile.c | 38 ++++++++++++++++++++++++++++++--- > 1 file changed, 35 insertions(+), 3 deletions(-) > > diff --git a/drivers/acpi/platform_profile.c b/drivers/acpi/platform_profile.c > index 2ad53cc6aae53..d9a7cc5891734 100644 > --- a/drivers/acpi/platform_profile.c > +++ b/drivers/acpi/platform_profile.c > @@ -73,8 +73,20 @@ static int _store_class_profile(struct device *dev, void *data) > > lockdep_assert_held(&profile_lock); > handler = to_pprof_handler(dev); > - if (!test_bit(*bit, handler->choices)) > - return -EOPNOTSUPP; > + if (!test_bit(*bit, handler->choices)) { > + switch (*bit) { > + case PLATFORM_PROFILE_QUIET: > + *bit = PLATFORM_PROFILE_LOW_POWER; > + break; > + case PLATFORM_PROFILE_LOW_POWER: > + *bit = PLATFORM_PROFILE_QUIET; > + break; > + default: > + return -EOPNOTSUPP; > + } > + if (!test_bit(*bit, handler->choices)) > + return -EOPNOTSUPP; > + } > > return handler->ops->profile_set(dev, *bit); > } > @@ -252,8 +264,16 @@ static int _aggregate_choices(struct device *dev, void *data) > handler = to_pprof_handler(dev); > if (test_bit(PLATFORM_PROFILE_LAST, aggregate)) > bitmap_copy(aggregate, handler->choices, PLATFORM_PROFILE_LAST); > - else > + else { > + /* treat quiet and low power the same for aggregation purposes */ > + if (test_bit(PLATFORM_PROFILE_QUIET, handler->choices) && > + test_bit(PLATFORM_PROFILE_LOW_POWER, aggregate)) > + set_bit(PLATFORM_PROFILE_QUIET, aggregate); > + else if (test_bit(PLATFORM_PROFILE_LOW_POWER, handler->choices) && > + test_bit(PLATFORM_PROFILE_QUIET, aggregate)) > + set_bit(PLATFORM_PROFILE_LOW_POWER, aggregate); > bitmap_and(aggregate, handler->choices, aggregate, PLATFORM_PROFILE_LAST); > + } So you end up showing both? If that's the case, isn't it equivalent to just make amd-pmf show both quiet and low-power? I guess it is not ideal for framework devices. But if asus devices end up showing both, then it should be ok for framework devices to show both. I like the behavior of the V1 personally. > return 0; > } > @@ -305,6 +325,13 @@ static int _aggregate_profiles(struct device *dev, void *data) > if (err) > return err; > > + /* treat low-power and quiet as the same */ > + if ((*profile == PLATFORM_PROFILE_LOW_POWER && > + val == PLATFORM_PROFILE_QUIET) || > + (*profile == PLATFORM_PROFILE_QUIET && > + val == PLATFORM_PROFILE_LOW_POWER)) > + *profile = val; > + > if (*profile != PLATFORM_PROFILE_LAST && *profile != val) > *profile = PLATFORM_PROFILE_CUSTOM; > else > @@ -531,6 +558,11 @@ struct device *platform_profile_register(struct device *dev, const char *name, > dev_err(dev, "Failed to register platform_profile class device with empty choices\n"); > return ERR_PTR(-EINVAL); > } > + if (test_bit(PLATFORM_PROFILE_QUIET, pprof->choices) && > + test_bit(PLATFORM_PROFILE_LOW_POWER, pprof->choices)) { > + dev_err(dev, "Failed to register platform_profile class device with both quiet and low-power\n"); > + return ERR_PTR(-EINVAL); > + } Can you avoid failing here? It caused a lot of issues in the past (the WMI driver bails). a dev_err should be enough. Since you do not fail maybe it can be increased to dev_crit. There is at least one driver that implements both currently, and a fix would have to precede this patch. > > guard(mutex)(&profile_lock); > > -- > 2.43.0 >
On 3/4/25 02:38, Antheas Kapenekakis wrote: > On Tue, 4 Mar 2025 at 07:48, Mario Limonciello <superm1@kernel.org> wrote: >> >> From: Mario Limonciello <mario.limonciello@amd.com> >> >> When two drivers don't support all the same profiles the legacy interface >> only exports the common profiles. >> >> This causes problems for cases where one driver uses low-power but another >> uses quiet because the result is that neither is exported to sysfs. >> >> If one platform profile handler supports quiet and the other >> supports low power treat them as the same for the purpose of >> the sysfs interface. >> >> Fixes: 688834743d67 ("ACPI: platform_profile: Allow multiple handlers") >> Reported-by: Antheas Kapenekakis <lkml@antheas.dev> >> Closes: https://lore.kernel.org/platform-driver-x86/e64b771e-3255-42ad-9257-5b8fc6c24ac9@gmx.de/T/#mc068042dd29df36c16c8af92664860fc4763974b >> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com> >> --- >> drivers/acpi/platform_profile.c | 38 ++++++++++++++++++++++++++++++--- >> 1 file changed, 35 insertions(+), 3 deletions(-) >> >> diff --git a/drivers/acpi/platform_profile.c b/drivers/acpi/platform_profile.c >> index 2ad53cc6aae53..d9a7cc5891734 100644 >> --- a/drivers/acpi/platform_profile.c >> +++ b/drivers/acpi/platform_profile.c >> @@ -73,8 +73,20 @@ static int _store_class_profile(struct device *dev, void *data) >> >> lockdep_assert_held(&profile_lock); >> handler = to_pprof_handler(dev); >> - if (!test_bit(*bit, handler->choices)) >> - return -EOPNOTSUPP; >> + if (!test_bit(*bit, handler->choices)) { >> + switch (*bit) { >> + case PLATFORM_PROFILE_QUIET: >> + *bit = PLATFORM_PROFILE_LOW_POWER; >> + break; >> + case PLATFORM_PROFILE_LOW_POWER: >> + *bit = PLATFORM_PROFILE_QUIET; >> + break; >> + default: >> + return -EOPNOTSUPP; >> + } >> + if (!test_bit(*bit, handler->choices)) >> + return -EOPNOTSUPP; >> + } >> >> return handler->ops->profile_set(dev, *bit); >> } >> @@ -252,8 +264,16 @@ static int _aggregate_choices(struct device *dev, void *data) >> handler = to_pprof_handler(dev); >> if (test_bit(PLATFORM_PROFILE_LAST, aggregate)) >> bitmap_copy(aggregate, handler->choices, PLATFORM_PROFILE_LAST); >> - else >> + else { >> + /* treat quiet and low power the same for aggregation purposes */ >> + if (test_bit(PLATFORM_PROFILE_QUIET, handler->choices) && >> + test_bit(PLATFORM_PROFILE_LOW_POWER, aggregate)) >> + set_bit(PLATFORM_PROFILE_QUIET, aggregate); >> + else if (test_bit(PLATFORM_PROFILE_LOW_POWER, handler->choices) && >> + test_bit(PLATFORM_PROFILE_QUIET, aggregate)) >> + set_bit(PLATFORM_PROFILE_LOW_POWER, aggregate); >> bitmap_and(aggregate, handler->choices, aggregate, PLATFORM_PROFILE_LAST); >> + } > > So you end up showing both? If that's the case, isn't it equivalent to > just make amd-pmf show both quiet and low-power? > > I guess it is not ideal for framework devices. But if asus devices end > up showing both, then it should be ok for framework devices to show > both. > > I like the behavior of the V1 personally. No; this doesn't cause it to show both. It only causes one to show up. I confirmed it with a contrived situation on my laptop that forced multiple profile handlers that supported a mix. # cat /sys/firmware/acpi/platform_profile* low-power low-power balanced performance # cat /sys/class/platform-profile/platform-profile-*/profile quiet quiet quiet quiet quiet quiet quiet quiet quiet quiet quiet quiet quiet quiet quiet quiet quiet quiet quiet quiet quiet quiet quiet quiet low-power > >> return 0; >> } >> @@ -305,6 +325,13 @@ static int _aggregate_profiles(struct device *dev, void *data) >> if (err) >> return err; >> >> + /* treat low-power and quiet as the same */ >> + if ((*profile == PLATFORM_PROFILE_LOW_POWER && >> + val == PLATFORM_PROFILE_QUIET) || >> + (*profile == PLATFORM_PROFILE_QUIET && >> + val == PLATFORM_PROFILE_LOW_POWER)) >> + *profile = val; >> + >> if (*profile != PLATFORM_PROFILE_LAST && *profile != val) >> *profile = PLATFORM_PROFILE_CUSTOM; >> else >> @@ -531,6 +558,11 @@ struct device *platform_profile_register(struct device *dev, const char *name, >> dev_err(dev, "Failed to register platform_profile class device with empty choices\n"); >> return ERR_PTR(-EINVAL); >> } >> + if (test_bit(PLATFORM_PROFILE_QUIET, pprof->choices) && >> + test_bit(PLATFORM_PROFILE_LOW_POWER, pprof->choices)) { >> + dev_err(dev, "Failed to register platform_profile class device with both quiet and low-power\n"); >> + return ERR_PTR(-EINVAL); >> + } > > Can you avoid failing here? It caused a lot of issues in the past (the > WMI driver bails). a dev_err should be enough. Since you do not fail > maybe it can be increased to dev_crit. > > There is at least one driver that implements both currently, and a fix > would have to precede this patch. Oh, acer-wmi? Kurt; can you please comment? Are both simultaneous? > >> >> guard(mutex)(&profile_lock); >> >> -- >> 2.43.0 >>
On Tue, 4 Mar 2025 at 13:49, Mario Limonciello <superm1@kernel.org> wrote: > > > > On 3/4/25 02:38, Antheas Kapenekakis wrote: > > On Tue, 4 Mar 2025 at 07:48, Mario Limonciello <superm1@kernel.org> wrote: > >> > >> From: Mario Limonciello <mario.limonciello@amd.com> > >> > >> When two drivers don't support all the same profiles the legacy interface > >> only exports the common profiles. > >> > >> This causes problems for cases where one driver uses low-power but another > >> uses quiet because the result is that neither is exported to sysfs. > >> > >> If one platform profile handler supports quiet and the other > >> supports low power treat them as the same for the purpose of > >> the sysfs interface. > >> > >> Fixes: 688834743d67 ("ACPI: platform_profile: Allow multiple handlers") > >> Reported-by: Antheas Kapenekakis <lkml@antheas.dev> > >> Closes: https://lore.kernel.org/platform-driver-x86/e64b771e-3255-42ad-9257-5b8fc6c24ac9@gmx.de/T/#mc068042dd29df36c16c8af92664860fc4763974b > >> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com> > >> --- > >> drivers/acpi/platform_profile.c | 38 ++++++++++++++++++++++++++++++--- > >> 1 file changed, 35 insertions(+), 3 deletions(-) > >> > >> diff --git a/drivers/acpi/platform_profile.c b/drivers/acpi/platform_profile.c > >> index 2ad53cc6aae53..d9a7cc5891734 100644 > >> --- a/drivers/acpi/platform_profile.c > >> +++ b/drivers/acpi/platform_profile.c > >> @@ -73,8 +73,20 @@ static int _store_class_profile(struct device *dev, void *data) > >> > >> lockdep_assert_held(&profile_lock); > >> handler = to_pprof_handler(dev); > >> - if (!test_bit(*bit, handler->choices)) > >> - return -EOPNOTSUPP; > >> + if (!test_bit(*bit, handler->choices)) { > >> + switch (*bit) { > >> + case PLATFORM_PROFILE_QUIET: > >> + *bit = PLATFORM_PROFILE_LOW_POWER; > >> + break; > >> + case PLATFORM_PROFILE_LOW_POWER: > >> + *bit = PLATFORM_PROFILE_QUIET; > >> + break; > >> + default: > >> + return -EOPNOTSUPP; > >> + } > >> + if (!test_bit(*bit, handler->choices)) > >> + return -EOPNOTSUPP; > >> + } > >> > >> return handler->ops->profile_set(dev, *bit); > >> } > >> @@ -252,8 +264,16 @@ static int _aggregate_choices(struct device *dev, void *data) > >> handler = to_pprof_handler(dev); > >> if (test_bit(PLATFORM_PROFILE_LAST, aggregate)) > >> bitmap_copy(aggregate, handler->choices, PLATFORM_PROFILE_LAST); > >> - else > >> + else { > >> + /* treat quiet and low power the same for aggregation purposes */ > >> + if (test_bit(PLATFORM_PROFILE_QUIET, handler->choices) && > >> + test_bit(PLATFORM_PROFILE_LOW_POWER, aggregate)) > >> + set_bit(PLATFORM_PROFILE_QUIET, aggregate); > >> + else if (test_bit(PLATFORM_PROFILE_LOW_POWER, handler->choices) && > >> + test_bit(PLATFORM_PROFILE_QUIET, aggregate)) > >> + set_bit(PLATFORM_PROFILE_LOW_POWER, aggregate); > >> bitmap_and(aggregate, handler->choices, aggregate, PLATFORM_PROFILE_LAST); > >> + } > > > > So you end up showing both? If that's the case, isn't it equivalent to > > just make amd-pmf show both quiet and low-power? > > > > I guess it is not ideal for framework devices. But if asus devices end > > up showing both, then it should be ok for framework devices to show > > both. > > > > I like the behavior of the V1 personally. > > No; this doesn't cause it to show both. It only causes one to show up. > I confirmed it with a contrived situation on my laptop that forced > multiple profile handlers that supported a mix. If you can somehow force it to show the same option every time with a tie breaker against amd-pmf it should be good enough. Still does not solve balanced-power so unlike V1 it is not a permanent fix. Hidden options was a nice tiebreaker imo. > > # cat /sys/firmware/acpi/platform_profile* > low-power > low-power balanced performance > > # cat /sys/class/platform-profile/platform-profile-*/profile > quiet > quiet > quiet > quiet > quiet > quiet > quiet > quiet > quiet > quiet > quiet > quiet > quiet > quiet > quiet > quiet > quiet > quiet > quiet > quiet > quiet > quiet > quiet > quiet > low-power > > > > >> return 0; > >> } > >> @@ -305,6 +325,13 @@ static int _aggregate_profiles(struct device *dev, void *data) > >> if (err) > >> return err; > >> > >> + /* treat low-power and quiet as the same */ > >> + if ((*profile == PLATFORM_PROFILE_LOW_POWER && > >> + val == PLATFORM_PROFILE_QUIET) || > >> + (*profile == PLATFORM_PROFILE_QUIET && > >> + val == PLATFORM_PROFILE_LOW_POWER)) > >> + *profile = val; > >> + > >> if (*profile != PLATFORM_PROFILE_LAST && *profile != val) > >> *profile = PLATFORM_PROFILE_CUSTOM; > >> else > >> @@ -531,6 +558,11 @@ struct device *platform_profile_register(struct device *dev, const char *name, > >> dev_err(dev, "Failed to register platform_profile class device with empty choices\n"); > >> return ERR_PTR(-EINVAL); > >> } > >> + if (test_bit(PLATFORM_PROFILE_QUIET, pprof->choices) && > >> + test_bit(PLATFORM_PROFILE_LOW_POWER, pprof->choices)) { > >> + dev_err(dev, "Failed to register platform_profile class device with both quiet and low-power\n"); > >> + return ERR_PTR(-EINVAL); > >> + } > > > > Can you avoid failing here? It caused a lot of issues in the past (the > > WMI driver bails). a dev_err should be enough. Since you do not fail > > maybe it can be increased to dev_crit. > > > > There is at least one driver that implements both currently, and a fix > > would have to precede this patch. > > Oh, acer-wmi? Kurt; can you please comment? Are both simultaneous? I do not have access to my kernel tree but when looking at it I remember an if block that did a set_bit on both for certain laptops in one of the drivers. Unsure if it was acer. But it was not ambiguous. > > > >> > >> guard(mutex)(&profile_lock); > >> > >> -- > >> 2.43.0 > >> >
Hi all, On Tue Mar 4, 2025 at 7:49 AM -05, Mario Limonciello wrote: > > > On 3/4/25 02:38, Antheas Kapenekakis wrote: >> On Tue, 4 Mar 2025 at 07:48, Mario Limonciello <superm1@kernel.org> wrote: >>> >>> From: Mario Limonciello <mario.limonciello@amd.com> >>> >>> When two drivers don't support all the same profiles the legacy interface >>> only exports the common profiles. >>> >>> This causes problems for cases where one driver uses low-power but another >>> uses quiet because the result is that neither is exported to sysfs. >>> >>> If one platform profile handler supports quiet and the other >>> supports low power treat them as the same for the purpose of >>> the sysfs interface. >>> >>> Fixes: 688834743d67 ("ACPI: platform_profile: Allow multiple handlers") >>> Reported-by: Antheas Kapenekakis <lkml@antheas.dev> >>> Closes: https://lore.kernel.org/platform-driver-x86/e64b771e-3255-42ad-9257-5b8fc6c24ac9@gmx.de/T/#mc068042dd29df36c16c8af92664860fc4763974b >>> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com> >>> --- >>> drivers/acpi/platform_profile.c | 38 ++++++++++++++++++++++++++++++--- >>> 1 file changed, 35 insertions(+), 3 deletions(-) >>> >>> diff --git a/drivers/acpi/platform_profile.c b/drivers/acpi/platform_profile.c >>> index 2ad53cc6aae53..d9a7cc5891734 100644 >>> --- a/drivers/acpi/platform_profile.c >>> +++ b/drivers/acpi/platform_profile.c >>> @@ -73,8 +73,20 @@ static int _store_class_profile(struct device *dev, void *data) >>> >>> lockdep_assert_held(&profile_lock); >>> handler = to_pprof_handler(dev); >>> - if (!test_bit(*bit, handler->choices)) >>> - return -EOPNOTSUPP; >>> + if (!test_bit(*bit, handler->choices)) { >>> + switch (*bit) { >>> + case PLATFORM_PROFILE_QUIET: >>> + *bit = PLATFORM_PROFILE_LOW_POWER; >>> + break; >>> + case PLATFORM_PROFILE_LOW_POWER: >>> + *bit = PLATFORM_PROFILE_QUIET; >>> + break; >>> + default: >>> + return -EOPNOTSUPP; >>> + } >>> + if (!test_bit(*bit, handler->choices)) >>> + return -EOPNOTSUPP; >>> + } >>> >>> return handler->ops->profile_set(dev, *bit); >>> } >>> @@ -252,8 +264,16 @@ static int _aggregate_choices(struct device *dev, void *data) >>> handler = to_pprof_handler(dev); >>> if (test_bit(PLATFORM_PROFILE_LAST, aggregate)) >>> bitmap_copy(aggregate, handler->choices, PLATFORM_PROFILE_LAST); >>> - else >>> + else { >>> + /* treat quiet and low power the same for aggregation purposes */ >>> + if (test_bit(PLATFORM_PROFILE_QUIET, handler->choices) && >>> + test_bit(PLATFORM_PROFILE_LOW_POWER, aggregate)) >>> + set_bit(PLATFORM_PROFILE_QUIET, aggregate); >>> + else if (test_bit(PLATFORM_PROFILE_LOW_POWER, handler->choices) && >>> + test_bit(PLATFORM_PROFILE_QUIET, aggregate)) >>> + set_bit(PLATFORM_PROFILE_LOW_POWER, aggregate); >>> bitmap_and(aggregate, handler->choices, aggregate, PLATFORM_PROFILE_LAST); >>> + } >> >> So you end up showing both? If that's the case, isn't it equivalent to >> just make amd-pmf show both quiet and low-power? >> >> I guess it is not ideal for framework devices. But if asus devices end >> up showing both, then it should be ok for framework devices to show >> both. >> >> I like the behavior of the V1 personally. > > No; this doesn't cause it to show both. It only causes one to show up. > I confirmed it with a contrived situation on my laptop that forced > multiple profile handlers that supported a mix. > > > # cat /sys/firmware/acpi/platform_profile* > low-power > low-power balanced performance > > # cat /sys/class/platform-profile/platform-profile-*/profile > quiet > quiet > quiet > quiet > quiet > quiet > quiet > quiet > quiet > quiet > quiet > quiet > quiet > quiet > quiet > quiet > quiet > quiet > quiet > quiet > quiet > quiet > quiet > quiet > low-power > >> >>> return 0; >>> } >>> @@ -305,6 +325,13 @@ static int _aggregate_profiles(struct device *dev, void *data) >>> if (err) >>> return err; >>> >>> + /* treat low-power and quiet as the same */ >>> + if ((*profile == PLATFORM_PROFILE_LOW_POWER && >>> + val == PLATFORM_PROFILE_QUIET) || >>> + (*profile == PLATFORM_PROFILE_QUIET && >>> + val == PLATFORM_PROFILE_LOW_POWER)) >>> + *profile = val; >>> + >>> if (*profile != PLATFORM_PROFILE_LAST && *profile != val) >>> *profile = PLATFORM_PROFILE_CUSTOM; >>> else >>> @@ -531,6 +558,11 @@ struct device *platform_profile_register(struct device *dev, const char *name, >>> dev_err(dev, "Failed to register platform_profile class device with empty choices\n"); >>> return ERR_PTR(-EINVAL); >>> } >>> + if (test_bit(PLATFORM_PROFILE_QUIET, pprof->choices) && >>> + test_bit(PLATFORM_PROFILE_LOW_POWER, pprof->choices)) { >>> + dev_err(dev, "Failed to register platform_profile class device with both quiet and low-power\n"); >>> + return ERR_PTR(-EINVAL); >>> + } >> >> Can you avoid failing here? It caused a lot of issues in the past (the >> WMI driver bails). a dev_err should be enough. Since you do not fail >> maybe it can be increased to dev_crit. >> >> There is at least one driver that implements both currently, and a fix >> would have to precede this patch. > > Oh, acer-wmi? Kurt; can you please comment? Are both simultaneous? There are a few laptops supported by alienware-wmi that definitely have both (including mine). The acer-wmi and the samsung-galaxybook drivers also probe for available choices dynamically, so some of those devices may be affected by this too. So yes, we shouldn't fail registration here. Anyway, I like this approach more than v1. What do you think about constraining this fix to the legacy interface?
On Tue, 4 Mar 2025 at 14:28, Kurt Borja <kuurtb@gmail.com> wrote: > > Hi all, > > On Tue Mar 4, 2025 at 7:49 AM -05, Mario Limonciello wrote: > > > > > > On 3/4/25 02:38, Antheas Kapenekakis wrote: > >> On Tue, 4 Mar 2025 at 07:48, Mario Limonciello <superm1@kernel.org> wrote: > >>> > >>> From: Mario Limonciello <mario.limonciello@amd.com> > >>> > >>> When two drivers don't support all the same profiles the legacy interface > >>> only exports the common profiles. > >>> > >>> This causes problems for cases where one driver uses low-power but another > >>> uses quiet because the result is that neither is exported to sysfs. > >>> > >>> If one platform profile handler supports quiet and the other > >>> supports low power treat them as the same for the purpose of > >>> the sysfs interface. > >>> > >>> Fixes: 688834743d67 ("ACPI: platform_profile: Allow multiple handlers") > >>> Reported-by: Antheas Kapenekakis <lkml@antheas.dev> > >>> Closes: https://lore.kernel.org/platform-driver-x86/e64b771e-3255-42ad-9257-5b8fc6c24ac9@gmx.de/T/#mc068042dd29df36c16c8af92664860fc4763974b > >>> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com> > >>> --- > >>> drivers/acpi/platform_profile.c | 38 ++++++++++++++++++++++++++++++--- > >>> 1 file changed, 35 insertions(+), 3 deletions(-) > >>> > >>> diff --git a/drivers/acpi/platform_profile.c b/drivers/acpi/platform_profile.c > >>> index 2ad53cc6aae53..d9a7cc5891734 100644 > >>> --- a/drivers/acpi/platform_profile.c > >>> +++ b/drivers/acpi/platform_profile.c > >>> @@ -73,8 +73,20 @@ static int _store_class_profile(struct device *dev, void *data) > >>> > >>> lockdep_assert_held(&profile_lock); > >>> handler = to_pprof_handler(dev); > >>> - if (!test_bit(*bit, handler->choices)) > >>> - return -EOPNOTSUPP; > >>> + if (!test_bit(*bit, handler->choices)) { > >>> + switch (*bit) { > >>> + case PLATFORM_PROFILE_QUIET: > >>> + *bit = PLATFORM_PROFILE_LOW_POWER; > >>> + break; > >>> + case PLATFORM_PROFILE_LOW_POWER: > >>> + *bit = PLATFORM_PROFILE_QUIET; > >>> + break; > >>> + default: > >>> + return -EOPNOTSUPP; > >>> + } > >>> + if (!test_bit(*bit, handler->choices)) > >>> + return -EOPNOTSUPP; > >>> + } > >>> > >>> return handler->ops->profile_set(dev, *bit); > >>> } > >>> @@ -252,8 +264,16 @@ static int _aggregate_choices(struct device *dev, void *data) > >>> handler = to_pprof_handler(dev); > >>> if (test_bit(PLATFORM_PROFILE_LAST, aggregate)) > >>> bitmap_copy(aggregate, handler->choices, PLATFORM_PROFILE_LAST); > >>> - else > >>> + else { > >>> + /* treat quiet and low power the same for aggregation purposes */ > >>> + if (test_bit(PLATFORM_PROFILE_QUIET, handler->choices) && > >>> + test_bit(PLATFORM_PROFILE_LOW_POWER, aggregate)) > >>> + set_bit(PLATFORM_PROFILE_QUIET, aggregate); > >>> + else if (test_bit(PLATFORM_PROFILE_LOW_POWER, handler->choices) && > >>> + test_bit(PLATFORM_PROFILE_QUIET, aggregate)) > >>> + set_bit(PLATFORM_PROFILE_LOW_POWER, aggregate); > >>> bitmap_and(aggregate, handler->choices, aggregate, PLATFORM_PROFILE_LAST); > >>> + } > >> > >> So you end up showing both? If that's the case, isn't it equivalent to > >> just make amd-pmf show both quiet and low-power? > >> > >> I guess it is not ideal for framework devices. But if asus devices end > >> up showing both, then it should be ok for framework devices to show > >> both. > >> > >> I like the behavior of the V1 personally. > > > > No; this doesn't cause it to show both. It only causes one to show up. > > I confirmed it with a contrived situation on my laptop that forced > > multiple profile handlers that supported a mix. > > > > > > # cat /sys/firmware/acpi/platform_profile* > > low-power > > low-power balanced performance > > > > # cat /sys/class/platform-profile/platform-profile-*/profile > > quiet > > quiet > > quiet > > quiet > > quiet > > quiet > > quiet > > quiet > > quiet > > quiet > > quiet > > quiet > > quiet > > quiet > > quiet > > quiet > > quiet > > quiet > > quiet > > quiet > > quiet > > quiet > > quiet > > quiet > > low-power > > > >> > >>> return 0; > >>> } > >>> @@ -305,6 +325,13 @@ static int _aggregate_profiles(struct device *dev, void *data) > >>> if (err) > >>> return err; > >>> > >>> + /* treat low-power and quiet as the same */ > >>> + if ((*profile == PLATFORM_PROFILE_LOW_POWER && > >>> + val == PLATFORM_PROFILE_QUIET) || > >>> + (*profile == PLATFORM_PROFILE_QUIET && > >>> + val == PLATFORM_PROFILE_LOW_POWER)) > >>> + *profile = val; > >>> + > >>> if (*profile != PLATFORM_PROFILE_LAST && *profile != val) > >>> *profile = PLATFORM_PROFILE_CUSTOM; > >>> else > >>> @@ -531,6 +558,11 @@ struct device *platform_profile_register(struct device *dev, const char *name, > >>> dev_err(dev, "Failed to register platform_profile class device with empty choices\n"); > >>> return ERR_PTR(-EINVAL); > >>> } > >>> + if (test_bit(PLATFORM_PROFILE_QUIET, pprof->choices) && > >>> + test_bit(PLATFORM_PROFILE_LOW_POWER, pprof->choices)) { > >>> + dev_err(dev, "Failed to register platform_profile class device with both quiet and low-power\n"); > >>> + return ERR_PTR(-EINVAL); > >>> + } > >> > >> Can you avoid failing here? It caused a lot of issues in the past (the > >> WMI driver bails). a dev_err should be enough. Since you do not fail > >> maybe it can be increased to dev_crit. > >> > >> There is at least one driver that implements both currently, and a fix > >> would have to precede this patch. > > > > Oh, acer-wmi? Kurt; can you please comment? Are both simultaneous? > > There are a few laptops supported by alienware-wmi that definitely have > both (including mine). The acer-wmi and the samsung-galaxybook drivers > also probe for available choices dynamically, so some of those devices > may be affected by this too. > > So yes, we shouldn't fail registration here. > > Anyway, I like this approach more than v1. What do you think about > constraining this fix to the legacy interface? AFAIK new interface is ok and should not be modified. None of the previous solutions touched it (well, changing quiet to low-power did). But I still expect the legacy interface to work the same way on 6.14. What happens if there is one handler that does low-power and one that does quiet? Is one choice preferred? And then are writes accepted in both? I cannot have the same device requiring low-power and quiet depending on kernel version or boot. I do tdp controls per manufacturer. > -- > ~ Kurt > > > > >> > >>> > >>> guard(mutex)(&profile_lock); > >>> > >>> -- > >>> 2.43.0 > >>> >
On March 4, 2025 5:32:50 AM PST, Antheas Kapenekakis <lkml@antheas.dev> wrote: >On Tue, 4 Mar 2025 at 14:28, Kurt Borja <kuurtb@gmail.com> wrote: >> >> Hi all, >> >> On Tue Mar 4, 2025 at 7:49 AM -05, Mario Limonciello wrote: >> > >> > >> > On 3/4/25 02:38, Antheas Kapenekakis wrote: >> >> On Tue, 4 Mar 2025 at 07:48, Mario Limonciello <superm1@kernel.org> wrote: >> >>> >> >>> From: Mario Limonciello <mario.limonciello@amd.com> >> >>> >> >>> When two drivers don't support all the same profiles the legacy interface >> >>> only exports the common profiles. >> >>> >> >>> This causes problems for cases where one driver uses low-power but another >> >>> uses quiet because the result is that neither is exported to sysfs. >> >>> >> >>> If one platform profile handler supports quiet and the other >> >>> supports low power treat them as the same for the purpose of >> >>> the sysfs interface. >> >>> >> >>> Fixes: 688834743d67 ("ACPI: platform_profile: Allow multiple handlers") >> >>> Reported-by: Antheas Kapenekakis <lkml@antheas.dev> >> >>> Closes: https://lore.kernel.org/platform-driver-x86/e64b771e-3255-42ad-9257-5b8fc6c24ac9@gmx.de/T/#mc068042dd29df36c16c8af92664860fc4763974b >> >>> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com> >> >>> --- >> >>> drivers/acpi/platform_profile.c | 38 ++++++++++++++++++++++++++++++--- >> >>> 1 file changed, 35 insertions(+), 3 deletions(-) >> >>> >> >>> diff --git a/drivers/acpi/platform_profile.c b/drivers/acpi/platform_profile.c >> >>> index 2ad53cc6aae53..d9a7cc5891734 100644 >> >>> --- a/drivers/acpi/platform_profile.c >> >>> +++ b/drivers/acpi/platform_profile.c >> >>> @@ -73,8 +73,20 @@ static int _store_class_profile(struct device *dev, void *data) >> >>> >> >>> lockdep_assert_held(&profile_lock); >> >>> handler = to_pprof_handler(dev); >> >>> - if (!test_bit(*bit, handler->choices)) >> >>> - return -EOPNOTSUPP; >> >>> + if (!test_bit(*bit, handler->choices)) { >> >>> + switch (*bit) { >> >>> + case PLATFORM_PROFILE_QUIET: >> >>> + *bit = PLATFORM_PROFILE_LOW_POWER; >> >>> + break; >> >>> + case PLATFORM_PROFILE_LOW_POWER: >> >>> + *bit = PLATFORM_PROFILE_QUIET; >> >>> + break; >> >>> + default: >> >>> + return -EOPNOTSUPP; >> >>> + } >> >>> + if (!test_bit(*bit, handler->choices)) >> >>> + return -EOPNOTSUPP; >> >>> + } >> >>> >> >>> return handler->ops->profile_set(dev, *bit); >> >>> } >> >>> @@ -252,8 +264,16 @@ static int _aggregate_choices(struct device *dev, void *data) >> >>> handler = to_pprof_handler(dev); >> >>> if (test_bit(PLATFORM_PROFILE_LAST, aggregate)) >> >>> bitmap_copy(aggregate, handler->choices, PLATFORM_PROFILE_LAST); >> >>> - else >> >>> + else { >> >>> + /* treat quiet and low power the same for aggregation purposes */ >> >>> + if (test_bit(PLATFORM_PROFILE_QUIET, handler->choices) && >> >>> + test_bit(PLATFORM_PROFILE_LOW_POWER, aggregate)) >> >>> + set_bit(PLATFORM_PROFILE_QUIET, aggregate); >> >>> + else if (test_bit(PLATFORM_PROFILE_LOW_POWER, handler->choices) && >> >>> + test_bit(PLATFORM_PROFILE_QUIET, aggregate)) >> >>> + set_bit(PLATFORM_PROFILE_LOW_POWER, aggregate); >> >>> bitmap_and(aggregate, handler->choices, aggregate, PLATFORM_PROFILE_LAST); >> >>> + } >> >> >> >> So you end up showing both? If that's the case, isn't it equivalent to >> >> just make amd-pmf show both quiet and low-power? >> >> >> >> I guess it is not ideal for framework devices. But if asus devices end >> >> up showing both, then it should be ok for framework devices to show >> >> both. >> >> >> >> I like the behavior of the V1 personally. >> > >> > No; this doesn't cause it to show both. It only causes one to show up. >> > I confirmed it with a contrived situation on my laptop that forced >> > multiple profile handlers that supported a mix. >> > >> > >> > # cat /sys/firmware/acpi/platform_profile* >> > low-power >> > low-power balanced performance >> > >> > # cat /sys/class/platform-profile/platform-profile-*/profile >> > quiet >> > quiet >> > quiet >> > quiet >> > quiet >> > quiet >> > quiet >> > quiet >> > quiet >> > quiet >> > quiet >> > quiet >> > quiet >> > quiet >> > quiet >> > quiet >> > quiet >> > quiet >> > quiet >> > quiet >> > quiet >> > quiet >> > quiet >> > quiet >> > low-power >> > >> >> >> >>> return 0; >> >>> } >> >>> @@ -305,6 +325,13 @@ static int _aggregate_profiles(struct device *dev, void *data) >> >>> if (err) >> >>> return err; >> >>> >> >>> + /* treat low-power and quiet as the same */ >> >>> + if ((*profile == PLATFORM_PROFILE_LOW_POWER && >> >>> + val == PLATFORM_PROFILE_QUIET) || >> >>> + (*profile == PLATFORM_PROFILE_QUIET && >> >>> + val == PLATFORM_PROFILE_LOW_POWER)) >> >>> + *profile = val; >> >>> + >> >>> if (*profile != PLATFORM_PROFILE_LAST && *profile != val) >> >>> *profile = PLATFORM_PROFILE_CUSTOM; >> >>> else >> >>> @@ -531,6 +558,11 @@ struct device *platform_profile_register(struct device *dev, const char *name, >> >>> dev_err(dev, "Failed to register platform_profile class device with empty choices\n"); >> >>> return ERR_PTR(-EINVAL); >> >>> } >> >>> + if (test_bit(PLATFORM_PROFILE_QUIET, pprof->choices) && >> >>> + test_bit(PLATFORM_PROFILE_LOW_POWER, pprof->choices)) { >> >>> + dev_err(dev, "Failed to register platform_profile class device with both quiet and low-power\n"); >> >>> + return ERR_PTR(-EINVAL); >> >>> + } >> >> >> >> Can you avoid failing here? It caused a lot of issues in the past (the >> >> WMI driver bails). a dev_err should be enough. Since you do not fail >> >> maybe it can be increased to dev_crit. >> >> >> >> There is at least one driver that implements both currently, and a fix >> >> would have to precede this patch. >> > >> > Oh, acer-wmi? Kurt; can you please comment? Are both simultaneous? >> >> There are a few laptops supported by alienware-wmi that definitely have >> both (including mine). The acer-wmi and the samsung-galaxybook drivers >> also probe for available choices dynamically, so some of those devices >> may be affected by this too. >> >> So yes, we shouldn't fail registration here. >> >> Anyway, I like this approach more than v1. What do you think about >> constraining this fix to the legacy interface? > >AFAIK new interface is ok and should not be modified. None of the >previous solutions touched it (well, changing quiet to low-power did). >But I still expect the legacy interface to work the same way on 6.14. > >What happens if there is one handler that does low-power and one that >does quiet? Is one choice preferred? And then are writes accepted in >both? > >I cannot have the same device requiring low-power and quiet depending >on kernel version or boot. I do tdp controls per manufacturer. > I agree that isn't ideal, but I see no reason why you can't index the _choices at startup to cover that in a generic way for all manufacturers. They present in performance order as text, specifically ensuring dynamic loading isn't difficult. >> -- >> ~ Kurt >> >> > >> >> >> >>> >> >>> guard(mutex)(&profile_lock); >> >>> >> >>> -- >> >>> 2.43.0 >> >>> >> - Derek
On Tue Mar 4, 2025 at 8:32 AM -05, Antheas Kapenekakis wrote: > On Tue, 4 Mar 2025 at 14:28, Kurt Borja <kuurtb@gmail.com> wrote: >> >> Hi all, >> >> On Tue Mar 4, 2025 at 7:49 AM -05, Mario Limonciello wrote: >> > >> > >> > On 3/4/25 02:38, Antheas Kapenekakis wrote: >> >> On Tue, 4 Mar 2025 at 07:48, Mario Limonciello <superm1@kernel.org> wrote: >> >>> >> >>> From: Mario Limonciello <mario.limonciello@amd.com> >> >>> >> >>> When two drivers don't support all the same profiles the legacy interface >> >>> only exports the common profiles. >> >>> >> >>> This causes problems for cases where one driver uses low-power but another >> >>> uses quiet because the result is that neither is exported to sysfs. >> >>> >> >>> If one platform profile handler supports quiet and the other >> >>> supports low power treat them as the same for the purpose of >> >>> the sysfs interface. >> >>> >> >>> Fixes: 688834743d67 ("ACPI: platform_profile: Allow multiple handlers") >> >>> Reported-by: Antheas Kapenekakis <lkml@antheas.dev> >> >>> Closes: https://lore.kernel.org/platform-driver-x86/e64b771e-3255-42ad-9257-5b8fc6c24ac9@gmx.de/T/#mc068042dd29df36c16c8af92664860fc4763974b >> >>> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com> >> >>> --- >> >>> drivers/acpi/platform_profile.c | 38 ++++++++++++++++++++++++++++++--- >> >>> 1 file changed, 35 insertions(+), 3 deletions(-) >> >>> >> >>> diff --git a/drivers/acpi/platform_profile.c b/drivers/acpi/platform_profile.c >> >>> index 2ad53cc6aae53..d9a7cc5891734 100644 >> >>> --- a/drivers/acpi/platform_profile.c >> >>> +++ b/drivers/acpi/platform_profile.c >> >>> @@ -73,8 +73,20 @@ static int _store_class_profile(struct device *dev, void *data) >> >>> >> >>> lockdep_assert_held(&profile_lock); >> >>> handler = to_pprof_handler(dev); >> >>> - if (!test_bit(*bit, handler->choices)) >> >>> - return -EOPNOTSUPP; >> >>> + if (!test_bit(*bit, handler->choices)) { >> >>> + switch (*bit) { >> >>> + case PLATFORM_PROFILE_QUIET: >> >>> + *bit = PLATFORM_PROFILE_LOW_POWER; >> >>> + break; >> >>> + case PLATFORM_PROFILE_LOW_POWER: >> >>> + *bit = PLATFORM_PROFILE_QUIET; >> >>> + break; >> >>> + default: >> >>> + return -EOPNOTSUPP; >> >>> + } >> >>> + if (!test_bit(*bit, handler->choices)) >> >>> + return -EOPNOTSUPP; >> >>> + } >> >>> >> >>> return handler->ops->profile_set(dev, *bit); >> >>> } >> >>> @@ -252,8 +264,16 @@ static int _aggregate_choices(struct device *dev, void *data) >> >>> handler = to_pprof_handler(dev); >> >>> if (test_bit(PLATFORM_PROFILE_LAST, aggregate)) >> >>> bitmap_copy(aggregate, handler->choices, PLATFORM_PROFILE_LAST); >> >>> - else >> >>> + else { >> >>> + /* treat quiet and low power the same for aggregation purposes */ >> >>> + if (test_bit(PLATFORM_PROFILE_QUIET, handler->choices) && >> >>> + test_bit(PLATFORM_PROFILE_LOW_POWER, aggregate)) >> >>> + set_bit(PLATFORM_PROFILE_QUIET, aggregate); >> >>> + else if (test_bit(PLATFORM_PROFILE_LOW_POWER, handler->choices) && >> >>> + test_bit(PLATFORM_PROFILE_QUIET, aggregate)) >> >>> + set_bit(PLATFORM_PROFILE_LOW_POWER, aggregate); >> >>> bitmap_and(aggregate, handler->choices, aggregate, PLATFORM_PROFILE_LAST); >> >>> + } >> >> >> >> So you end up showing both? If that's the case, isn't it equivalent to >> >> just make amd-pmf show both quiet and low-power? >> >> >> >> I guess it is not ideal for framework devices. But if asus devices end >> >> up showing both, then it should be ok for framework devices to show >> >> both. >> >> >> >> I like the behavior of the V1 personally. >> > >> > No; this doesn't cause it to show both. It only causes one to show up. >> > I confirmed it with a contrived situation on my laptop that forced >> > multiple profile handlers that supported a mix. >> > >> > >> > # cat /sys/firmware/acpi/platform_profile* >> > low-power >> > low-power balanced performance >> > >> > # cat /sys/class/platform-profile/platform-profile-*/profile >> > quiet >> > quiet >> > quiet >> > quiet >> > quiet >> > quiet >> > quiet >> > quiet >> > quiet >> > quiet >> > quiet >> > quiet >> > quiet >> > quiet >> > quiet >> > quiet >> > quiet >> > quiet >> > quiet >> > quiet >> > quiet >> > quiet >> > quiet >> > quiet >> > low-power >> > >> >> >> >>> return 0; >> >>> } >> >>> @@ -305,6 +325,13 @@ static int _aggregate_profiles(struct device *dev, void *data) >> >>> if (err) >> >>> return err; >> >>> >> >>> + /* treat low-power and quiet as the same */ >> >>> + if ((*profile == PLATFORM_PROFILE_LOW_POWER && >> >>> + val == PLATFORM_PROFILE_QUIET) || >> >>> + (*profile == PLATFORM_PROFILE_QUIET && >> >>> + val == PLATFORM_PROFILE_LOW_POWER)) >> >>> + *profile = val; >> >>> + >> >>> if (*profile != PLATFORM_PROFILE_LAST && *profile != val) >> >>> *profile = PLATFORM_PROFILE_CUSTOM; >> >>> else >> >>> @@ -531,6 +558,11 @@ struct device *platform_profile_register(struct device *dev, const char *name, >> >>> dev_err(dev, "Failed to register platform_profile class device with empty choices\n"); >> >>> return ERR_PTR(-EINVAL); >> >>> } >> >>> + if (test_bit(PLATFORM_PROFILE_QUIET, pprof->choices) && >> >>> + test_bit(PLATFORM_PROFILE_LOW_POWER, pprof->choices)) { >> >>> + dev_err(dev, "Failed to register platform_profile class device with both quiet and low-power\n"); >> >>> + return ERR_PTR(-EINVAL); >> >>> + } >> >> >> >> Can you avoid failing here? It caused a lot of issues in the past (the >> >> WMI driver bails). a dev_err should be enough. Since you do not fail >> >> maybe it can be increased to dev_crit. >> >> >> >> There is at least one driver that implements both currently, and a fix >> >> would have to precede this patch. >> > >> > Oh, acer-wmi? Kurt; can you please comment? Are both simultaneous? >> >> There are a few laptops supported by alienware-wmi that definitely have >> both (including mine). The acer-wmi and the samsung-galaxybook drivers >> also probe for available choices dynamically, so some of those devices >> may be affected by this too. >> >> So yes, we shouldn't fail registration here. >> >> Anyway, I like this approach more than v1. What do you think about >> constraining this fix to the legacy interface? > > AFAIK new interface is ok and should not be modified. None of the > previous solutions touched it (well, changing quiet to low-power did). > But I still expect the legacy interface to work the same way on 6.14. This patch also permanently alias quiet and low-power for the new interface, if either one is not available. > > What happens if there is one handler that does low-power and one that > does quiet? Is one choice preferred? And then are writes accepted in > both? > > I cannot have the same device requiring low-power and quiet depending > on kernel version or boot. I do tdp controls per manufacturer. I'm not sure what you mean here.
On Tue, 4 Mar 2025 at 14:55, Kurt Borja <kuurtb@gmail.com> wrote: > > On Tue Mar 4, 2025 at 8:32 AM -05, Antheas Kapenekakis wrote: > > On Tue, 4 Mar 2025 at 14:28, Kurt Borja <kuurtb@gmail.com> wrote: > >> > >> Hi all, > >> > >> On Tue Mar 4, 2025 at 7:49 AM -05, Mario Limonciello wrote: > >> > > >> > > >> > On 3/4/25 02:38, Antheas Kapenekakis wrote: > >> >> On Tue, 4 Mar 2025 at 07:48, Mario Limonciello <superm1@kernel.org> wrote: > >> >>> > >> >>> From: Mario Limonciello <mario.limonciello@amd.com> > >> >>> > >> >>> When two drivers don't support all the same profiles the legacy interface > >> >>> only exports the common profiles. > >> >>> > >> >>> This causes problems for cases where one driver uses low-power but another > >> >>> uses quiet because the result is that neither is exported to sysfs. > >> >>> > >> >>> If one platform profile handler supports quiet and the other > >> >>> supports low power treat them as the same for the purpose of > >> >>> the sysfs interface. > >> >>> > >> >>> Fixes: 688834743d67 ("ACPI: platform_profile: Allow multiple handlers") > >> >>> Reported-by: Antheas Kapenekakis <lkml@antheas.dev> > >> >>> Closes: https://lore.kernel.org/platform-driver-x86/e64b771e-3255-42ad-9257-5b8fc6c24ac9@gmx.de/T/#mc068042dd29df36c16c8af92664860fc4763974b > >> >>> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com> > >> >>> --- > >> >>> drivers/acpi/platform_profile.c | 38 ++++++++++++++++++++++++++++++--- > >> >>> 1 file changed, 35 insertions(+), 3 deletions(-) > >> >>> > >> >>> diff --git a/drivers/acpi/platform_profile.c b/drivers/acpi/platform_profile.c > >> >>> index 2ad53cc6aae53..d9a7cc5891734 100644 > >> >>> --- a/drivers/acpi/platform_profile.c > >> >>> +++ b/drivers/acpi/platform_profile.c > >> >>> @@ -73,8 +73,20 @@ static int _store_class_profile(struct device *dev, void *data) > >> >>> > >> >>> lockdep_assert_held(&profile_lock); > >> >>> handler = to_pprof_handler(dev); > >> >>> - if (!test_bit(*bit, handler->choices)) > >> >>> - return -EOPNOTSUPP; > >> >>> + if (!test_bit(*bit, handler->choices)) { > >> >>> + switch (*bit) { > >> >>> + case PLATFORM_PROFILE_QUIET: > >> >>> + *bit = PLATFORM_PROFILE_LOW_POWER; > >> >>> + break; > >> >>> + case PLATFORM_PROFILE_LOW_POWER: > >> >>> + *bit = PLATFORM_PROFILE_QUIET; > >> >>> + break; > >> >>> + default: > >> >>> + return -EOPNOTSUPP; > >> >>> + } > >> >>> + if (!test_bit(*bit, handler->choices)) > >> >>> + return -EOPNOTSUPP; > >> >>> + } > >> >>> > >> >>> return handler->ops->profile_set(dev, *bit); > >> >>> } > >> >>> @@ -252,8 +264,16 @@ static int _aggregate_choices(struct device *dev, void *data) > >> >>> handler = to_pprof_handler(dev); > >> >>> if (test_bit(PLATFORM_PROFILE_LAST, aggregate)) > >> >>> bitmap_copy(aggregate, handler->choices, PLATFORM_PROFILE_LAST); > >> >>> - else > >> >>> + else { > >> >>> + /* treat quiet and low power the same for aggregation purposes */ > >> >>> + if (test_bit(PLATFORM_PROFILE_QUIET, handler->choices) && > >> >>> + test_bit(PLATFORM_PROFILE_LOW_POWER, aggregate)) > >> >>> + set_bit(PLATFORM_PROFILE_QUIET, aggregate); > >> >>> + else if (test_bit(PLATFORM_PROFILE_LOW_POWER, handler->choices) && > >> >>> + test_bit(PLATFORM_PROFILE_QUIET, aggregate)) > >> >>> + set_bit(PLATFORM_PROFILE_LOW_POWER, aggregate); > >> >>> bitmap_and(aggregate, handler->choices, aggregate, PLATFORM_PROFILE_LAST); > >> >>> + } > >> >> > >> >> So you end up showing both? If that's the case, isn't it equivalent to > >> >> just make amd-pmf show both quiet and low-power? > >> >> > >> >> I guess it is not ideal for framework devices. But if asus devices end > >> >> up showing both, then it should be ok for framework devices to show > >> >> both. > >> >> > >> >> I like the behavior of the V1 personally. > >> > > >> > No; this doesn't cause it to show both. It only causes one to show up. > >> > I confirmed it with a contrived situation on my laptop that forced > >> > multiple profile handlers that supported a mix. > >> > > >> > > >> > # cat /sys/firmware/acpi/platform_profile* > >> > low-power > >> > low-power balanced performance > >> > > >> > # cat /sys/class/platform-profile/platform-profile-*/profile > >> > quiet > >> > quiet > >> > quiet > >> > quiet > >> > quiet > >> > quiet > >> > quiet > >> > quiet > >> > quiet > >> > quiet > >> > quiet > >> > quiet > >> > quiet > >> > quiet > >> > quiet > >> > quiet > >> > quiet > >> > quiet > >> > quiet > >> > quiet > >> > quiet > >> > quiet > >> > quiet > >> > quiet > >> > low-power > >> > > >> >> > >> >>> return 0; > >> >>> } > >> >>> @@ -305,6 +325,13 @@ static int _aggregate_profiles(struct device *dev, void *data) > >> >>> if (err) > >> >>> return err; > >> >>> > >> >>> + /* treat low-power and quiet as the same */ > >> >>> + if ((*profile == PLATFORM_PROFILE_LOW_POWER && > >> >>> + val == PLATFORM_PROFILE_QUIET) || > >> >>> + (*profile == PLATFORM_PROFILE_QUIET && > >> >>> + val == PLATFORM_PROFILE_LOW_POWER)) > >> >>> + *profile = val; > >> >>> + > >> >>> if (*profile != PLATFORM_PROFILE_LAST && *profile != val) > >> >>> *profile = PLATFORM_PROFILE_CUSTOM; > >> >>> else > >> >>> @@ -531,6 +558,11 @@ struct device *platform_profile_register(struct device *dev, const char *name, > >> >>> dev_err(dev, "Failed to register platform_profile class device with empty choices\n"); > >> >>> return ERR_PTR(-EINVAL); > >> >>> } > >> >>> + if (test_bit(PLATFORM_PROFILE_QUIET, pprof->choices) && > >> >>> + test_bit(PLATFORM_PROFILE_LOW_POWER, pprof->choices)) { > >> >>> + dev_err(dev, "Failed to register platform_profile class device with both quiet and low-power\n"); > >> >>> + return ERR_PTR(-EINVAL); > >> >>> + } > >> >> > >> >> Can you avoid failing here? It caused a lot of issues in the past (the > >> >> WMI driver bails). a dev_err should be enough. Since you do not fail > >> >> maybe it can be increased to dev_crit. > >> >> > >> >> There is at least one driver that implements both currently, and a fix > >> >> would have to precede this patch. > >> > > >> > Oh, acer-wmi? Kurt; can you please comment? Are both simultaneous? > >> > >> There are a few laptops supported by alienware-wmi that definitely have > >> both (including mine). The acer-wmi and the samsung-galaxybook drivers > >> also probe for available choices dynamically, so some of those devices > >> may be affected by this too. > >> > >> So yes, we shouldn't fail registration here. > >> > >> Anyway, I like this approach more than v1. What do you think about > >> constraining this fix to the legacy interface? > > > > AFAIK new interface is ok and should not be modified. None of the > > previous solutions touched it (well, changing quiet to low-power did). > > But I still expect the legacy interface to work the same way on 6.14. > > This patch also permanently alias quiet and low-power for the new > interface, if either one is not available. Mmm, aliasing it as a hidden option is more of a side effect. I guess if people start relying on that it might become problematic to revert though. > > > > What happens if there is one handler that does low-power and one that > > does quiet? Is one choice preferred? And then are writes accepted in > > both? > > > > I cannot have the same device requiring low-power and quiet depending > > on kernel version or boot. I do tdp controls per manufacturer. > > I'm not sure what you mean here. You have an Asus Z13, in 6.13 it reports low-power, in 6.14 it reports quiet. This patch series fixes writing blindly to it I would say, not so much reading from it. Although it is unclear who that would affect. I think reading will become a bigger problem in the future, as Legion/Thinkpad devices can change their platform profile via user action, and I would expect ppd/tuned to respond to that. They do not currently. By the point they do, they can use the modern ABI though, and bind bidirectionality to the /name attribute of platform profiles. > -- > ~ Kurt > > > > >> -- > >> ~ Kurt > >> > >> > > >> >> > >> >>> > >> >>> guard(mutex)(&profile_lock); > >> >>> > >> >>> -- > >> >>> 2.43.0 > >> >>> > >> >
On Tue, Mar 4, 2025 at 1:49 PM Mario Limonciello <superm1@kernel.org> wrote: > > On 3/4/25 02:38, Antheas Kapenekakis wrote: > > On Tue, 4 Mar 2025 at 07:48, Mario Limonciello <superm1@kernel.org> wrote: > >> > >> From: Mario Limonciello <mario.limonciello@amd.com> > >> > >> When two drivers don't support all the same profiles the legacy interface > >> only exports the common profiles. > >> > >> This causes problems for cases where one driver uses low-power but another > >> uses quiet because the result is that neither is exported to sysfs. > >> > >> If one platform profile handler supports quiet and the other > >> supports low power treat them as the same for the purpose of > >> the sysfs interface. > >> > >> Fixes: 688834743d67 ("ACPI: platform_profile: Allow multiple handlers") > >> Reported-by: Antheas Kapenekakis <lkml@antheas.dev> > >> Closes: https://lore.kernel.org/platform-driver-x86/e64b771e-3255-42ad-9257-5b8fc6c24ac9@gmx.de/T/#mc068042dd29df36c16c8af92664860fc4763974b > >> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com> > >> --- > >> drivers/acpi/platform_profile.c | 38 ++++++++++++++++++++++++++++++--- > >> 1 file changed, 35 insertions(+), 3 deletions(-) > >> > >> diff --git a/drivers/acpi/platform_profile.c b/drivers/acpi/platform_profile.c > >> index 2ad53cc6aae53..d9a7cc5891734 100644 > >> --- a/drivers/acpi/platform_profile.c > >> +++ b/drivers/acpi/platform_profile.c > >> @@ -73,8 +73,20 @@ static int _store_class_profile(struct device *dev, void *data) > >> > >> lockdep_assert_held(&profile_lock); > >> handler = to_pprof_handler(dev); > >> - if (!test_bit(*bit, handler->choices)) > >> - return -EOPNOTSUPP; > >> + if (!test_bit(*bit, handler->choices)) { > >> + switch (*bit) { > >> + case PLATFORM_PROFILE_QUIET: > >> + *bit = PLATFORM_PROFILE_LOW_POWER; > >> + break; > >> + case PLATFORM_PROFILE_LOW_POWER: > >> + *bit = PLATFORM_PROFILE_QUIET; > >> + break; > >> + default: > >> + return -EOPNOTSUPP; > >> + } > >> + if (!test_bit(*bit, handler->choices)) > >> + return -EOPNOTSUPP; > >> + } > >> > >> return handler->ops->profile_set(dev, *bit); > >> } > >> @@ -252,8 +264,16 @@ static int _aggregate_choices(struct device *dev, void *data) > >> handler = to_pprof_handler(dev); > >> if (test_bit(PLATFORM_PROFILE_LAST, aggregate)) > >> bitmap_copy(aggregate, handler->choices, PLATFORM_PROFILE_LAST); > >> - else > >> + else { > >> + /* treat quiet and low power the same for aggregation purposes */ > >> + if (test_bit(PLATFORM_PROFILE_QUIET, handler->choices) && > >> + test_bit(PLATFORM_PROFILE_LOW_POWER, aggregate)) > >> + set_bit(PLATFORM_PROFILE_QUIET, aggregate); > >> + else if (test_bit(PLATFORM_PROFILE_LOW_POWER, handler->choices) && > >> + test_bit(PLATFORM_PROFILE_QUIET, aggregate)) > >> + set_bit(PLATFORM_PROFILE_LOW_POWER, aggregate); > >> bitmap_and(aggregate, handler->choices, aggregate, PLATFORM_PROFILE_LAST); > >> + } > > > > So you end up showing both? If that's the case, isn't it equivalent to > > just make amd-pmf show both quiet and low-power? > > > > I guess it is not ideal for framework devices. But if asus devices end > > up showing both, then it should be ok for framework devices to show > > both. > > > > I like the behavior of the V1 personally. > > No; this doesn't cause it to show both. It only causes one to show up. Which may not be the one that was shown before IIUC and that's not good. What actually is the problem with the previous version?
On 3/4/2025 08:08, Rafael J. Wysocki wrote: > On Tue, Mar 4, 2025 at 1:49 PM Mario Limonciello <superm1@kernel.org> wrote: >> >> On 3/4/25 02:38, Antheas Kapenekakis wrote: >>> On Tue, 4 Mar 2025 at 07:48, Mario Limonciello <superm1@kernel.org> wrote: >>>> >>>> From: Mario Limonciello <mario.limonciello@amd.com> >>>> >>>> When two drivers don't support all the same profiles the legacy interface >>>> only exports the common profiles. >>>> >>>> This causes problems for cases where one driver uses low-power but another >>>> uses quiet because the result is that neither is exported to sysfs. >>>> >>>> If one platform profile handler supports quiet and the other >>>> supports low power treat them as the same for the purpose of >>>> the sysfs interface. >>>> >>>> Fixes: 688834743d67 ("ACPI: platform_profile: Allow multiple handlers") >>>> Reported-by: Antheas Kapenekakis <lkml@antheas.dev> >>>> Closes: https://lore.kernel.org/platform-driver-x86/e64b771e-3255-42ad-9257-5b8fc6c24ac9@gmx.de/T/#mc068042dd29df36c16c8af92664860fc4763974b >>>> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com> >>>> --- >>>> drivers/acpi/platform_profile.c | 38 ++++++++++++++++++++++++++++++--- >>>> 1 file changed, 35 insertions(+), 3 deletions(-) >>>> >>>> diff --git a/drivers/acpi/platform_profile.c b/drivers/acpi/platform_profile.c >>>> index 2ad53cc6aae53..d9a7cc5891734 100644 >>>> --- a/drivers/acpi/platform_profile.c >>>> +++ b/drivers/acpi/platform_profile.c >>>> @@ -73,8 +73,20 @@ static int _store_class_profile(struct device *dev, void *data) >>>> >>>> lockdep_assert_held(&profile_lock); >>>> handler = to_pprof_handler(dev); >>>> - if (!test_bit(*bit, handler->choices)) >>>> - return -EOPNOTSUPP; >>>> + if (!test_bit(*bit, handler->choices)) { >>>> + switch (*bit) { >>>> + case PLATFORM_PROFILE_QUIET: >>>> + *bit = PLATFORM_PROFILE_LOW_POWER; >>>> + break; >>>> + case PLATFORM_PROFILE_LOW_POWER: >>>> + *bit = PLATFORM_PROFILE_QUIET; >>>> + break; >>>> + default: >>>> + return -EOPNOTSUPP; >>>> + } >>>> + if (!test_bit(*bit, handler->choices)) >>>> + return -EOPNOTSUPP; >>>> + } >>>> >>>> return handler->ops->profile_set(dev, *bit); >>>> } >>>> @@ -252,8 +264,16 @@ static int _aggregate_choices(struct device *dev, void *data) >>>> handler = to_pprof_handler(dev); >>>> if (test_bit(PLATFORM_PROFILE_LAST, aggregate)) >>>> bitmap_copy(aggregate, handler->choices, PLATFORM_PROFILE_LAST); >>>> - else >>>> + else { >>>> + /* treat quiet and low power the same for aggregation purposes */ >>>> + if (test_bit(PLATFORM_PROFILE_QUIET, handler->choices) && >>>> + test_bit(PLATFORM_PROFILE_LOW_POWER, aggregate)) >>>> + set_bit(PLATFORM_PROFILE_QUIET, aggregate); >>>> + else if (test_bit(PLATFORM_PROFILE_LOW_POWER, handler->choices) && >>>> + test_bit(PLATFORM_PROFILE_QUIET, aggregate)) >>>> + set_bit(PLATFORM_PROFILE_LOW_POWER, aggregate); >>>> bitmap_and(aggregate, handler->choices, aggregate, PLATFORM_PROFILE_LAST); >>>> + } >>> >>> So you end up showing both? If that's the case, isn't it equivalent to >>> just make amd-pmf show both quiet and low-power? >>> >>> I guess it is not ideal for framework devices. But if asus devices end >>> up showing both, then it should be ok for framework devices to show >>> both. >>> >>> I like the behavior of the V1 personally. >> >> No; this doesn't cause it to show both. It only causes one to show up. > > Which may not be the one that was shown before IIUC and that's not good. > > What actually is the problem with the previous version? Functionally? Nothing. This was to demonstrate the other way to do it that I preferred and get feedback on it as an alternative. If you and Ilpo are happy with v1 that's totally fine and we can go with that.
On Tue, Mar 4, 2025 at 3:52 PM Mario Limonciello <superm1@kernel.org> wrote: > > On 3/4/2025 08:08, Rafael J. Wysocki wrote: > > On Tue, Mar 4, 2025 at 1:49 PM Mario Limonciello <superm1@kernel.org> wrote: > >> > >> On 3/4/25 02:38, Antheas Kapenekakis wrote: > >>> On Tue, 4 Mar 2025 at 07:48, Mario Limonciello <superm1@kernel.org> wrote: > >>>> > >>>> From: Mario Limonciello <mario.limonciello@amd.com> > >>>> > >>>> When two drivers don't support all the same profiles the legacy interface > >>>> only exports the common profiles. > >>>> > >>>> This causes problems for cases where one driver uses low-power but another > >>>> uses quiet because the result is that neither is exported to sysfs. > >>>> > >>>> If one platform profile handler supports quiet and the other > >>>> supports low power treat them as the same for the purpose of > >>>> the sysfs interface. > >>>> > >>>> Fixes: 688834743d67 ("ACPI: platform_profile: Allow multiple handlers") > >>>> Reported-by: Antheas Kapenekakis <lkml@antheas.dev> > >>>> Closes: https://lore.kernel.org/platform-driver-x86/e64b771e-3255-42ad-9257-5b8fc6c24ac9@gmx.de/T/#mc068042dd29df36c16c8af92664860fc4763974b > >>>> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com> > >>>> --- > >>>> drivers/acpi/platform_profile.c | 38 ++++++++++++++++++++++++++++++--- > >>>> 1 file changed, 35 insertions(+), 3 deletions(-) > >>>> > >>>> diff --git a/drivers/acpi/platform_profile.c b/drivers/acpi/platform_profile.c > >>>> index 2ad53cc6aae53..d9a7cc5891734 100644 > >>>> --- a/drivers/acpi/platform_profile.c > >>>> +++ b/drivers/acpi/platform_profile.c > >>>> @@ -73,8 +73,20 @@ static int _store_class_profile(struct device *dev, void *data) > >>>> > >>>> lockdep_assert_held(&profile_lock); > >>>> handler = to_pprof_handler(dev); > >>>> - if (!test_bit(*bit, handler->choices)) > >>>> - return -EOPNOTSUPP; > >>>> + if (!test_bit(*bit, handler->choices)) { > >>>> + switch (*bit) { > >>>> + case PLATFORM_PROFILE_QUIET: > >>>> + *bit = PLATFORM_PROFILE_LOW_POWER; > >>>> + break; > >>>> + case PLATFORM_PROFILE_LOW_POWER: > >>>> + *bit = PLATFORM_PROFILE_QUIET; > >>>> + break; > >>>> + default: > >>>> + return -EOPNOTSUPP; > >>>> + } > >>>> + if (!test_bit(*bit, handler->choices)) > >>>> + return -EOPNOTSUPP; > >>>> + } > >>>> > >>>> return handler->ops->profile_set(dev, *bit); > >>>> } > >>>> @@ -252,8 +264,16 @@ static int _aggregate_choices(struct device *dev, void *data) > >>>> handler = to_pprof_handler(dev); > >>>> if (test_bit(PLATFORM_PROFILE_LAST, aggregate)) > >>>> bitmap_copy(aggregate, handler->choices, PLATFORM_PROFILE_LAST); > >>>> - else > >>>> + else { > >>>> + /* treat quiet and low power the same for aggregation purposes */ > >>>> + if (test_bit(PLATFORM_PROFILE_QUIET, handler->choices) && > >>>> + test_bit(PLATFORM_PROFILE_LOW_POWER, aggregate)) > >>>> + set_bit(PLATFORM_PROFILE_QUIET, aggregate); > >>>> + else if (test_bit(PLATFORM_PROFILE_LOW_POWER, handler->choices) && > >>>> + test_bit(PLATFORM_PROFILE_QUIET, aggregate)) > >>>> + set_bit(PLATFORM_PROFILE_LOW_POWER, aggregate); > >>>> bitmap_and(aggregate, handler->choices, aggregate, PLATFORM_PROFILE_LAST); > >>>> + } > >>> > >>> So you end up showing both? If that's the case, isn't it equivalent to > >>> just make amd-pmf show both quiet and low-power? > >>> > >>> I guess it is not ideal for framework devices. But if asus devices end > >>> up showing both, then it should be ok for framework devices to show > >>> both. > >>> > >>> I like the behavior of the V1 personally. > >> > >> No; this doesn't cause it to show both. It only causes one to show up. > > > > Which may not be the one that was shown before IIUC and that's not good. > > > > What actually is the problem with the previous version? > > Functionally? Nothing. This was to demonstrate the other way to do it > that I preferred and get feedback on it as an alternative. > > If you and Ilpo are happy with v1 that's totally fine and we can go with > that. I'd prefer to go for the v1 at this point because it fixes a regression affecting user space that needs to be addressed before the 6.14 release (and there is not too much time left) and it has been checked on the affected systems. Ilpo, do you agree?
diff --git a/drivers/acpi/platform_profile.c b/drivers/acpi/platform_profile.c index 2ad53cc6aae53..d9a7cc5891734 100644 --- a/drivers/acpi/platform_profile.c +++ b/drivers/acpi/platform_profile.c @@ -73,8 +73,20 @@ static int _store_class_profile(struct device *dev, void *data) lockdep_assert_held(&profile_lock); handler = to_pprof_handler(dev); - if (!test_bit(*bit, handler->choices)) - return -EOPNOTSUPP; + if (!test_bit(*bit, handler->choices)) { + switch (*bit) { + case PLATFORM_PROFILE_QUIET: + *bit = PLATFORM_PROFILE_LOW_POWER; + break; + case PLATFORM_PROFILE_LOW_POWER: + *bit = PLATFORM_PROFILE_QUIET; + break; + default: + return -EOPNOTSUPP; + } + if (!test_bit(*bit, handler->choices)) + return -EOPNOTSUPP; + } return handler->ops->profile_set(dev, *bit); } @@ -252,8 +264,16 @@ static int _aggregate_choices(struct device *dev, void *data) handler = to_pprof_handler(dev); if (test_bit(PLATFORM_PROFILE_LAST, aggregate)) bitmap_copy(aggregate, handler->choices, PLATFORM_PROFILE_LAST); - else + else { + /* treat quiet and low power the same for aggregation purposes */ + if (test_bit(PLATFORM_PROFILE_QUIET, handler->choices) && + test_bit(PLATFORM_PROFILE_LOW_POWER, aggregate)) + set_bit(PLATFORM_PROFILE_QUIET, aggregate); + else if (test_bit(PLATFORM_PROFILE_LOW_POWER, handler->choices) && + test_bit(PLATFORM_PROFILE_QUIET, aggregate)) + set_bit(PLATFORM_PROFILE_LOW_POWER, aggregate); bitmap_and(aggregate, handler->choices, aggregate, PLATFORM_PROFILE_LAST); + } return 0; } @@ -305,6 +325,13 @@ static int _aggregate_profiles(struct device *dev, void *data) if (err) return err; + /* treat low-power and quiet as the same */ + if ((*profile == PLATFORM_PROFILE_LOW_POWER && + val == PLATFORM_PROFILE_QUIET) || + (*profile == PLATFORM_PROFILE_QUIET && + val == PLATFORM_PROFILE_LOW_POWER)) + *profile = val; + if (*profile != PLATFORM_PROFILE_LAST && *profile != val) *profile = PLATFORM_PROFILE_CUSTOM; else @@ -531,6 +558,11 @@ struct device *platform_profile_register(struct device *dev, const char *name, dev_err(dev, "Failed to register platform_profile class device with empty choices\n"); return ERR_PTR(-EINVAL); } + if (test_bit(PLATFORM_PROFILE_QUIET, pprof->choices) && + test_bit(PLATFORM_PROFILE_LOW_POWER, pprof->choices)) { + dev_err(dev, "Failed to register platform_profile class device with both quiet and low-power\n"); + return ERR_PTR(-EINVAL); + } guard(mutex)(&profile_lock);