On Tue, 4 Mar 2025 at 13:49, Mario Limonciello <superm1@xxxxxxxxxx> wrote:
>
>
>
> On 3/4/25 02:38, Antheas Kapenekakis wrote:
> > On Tue, 4 Mar 2025 at 07:48, Mario Limonciello <superm1@xxxxxxxxxx> wrote:
> >>
> >> From: Mario Limonciello <mario.limonciello@xxxxxxx>
> >>
> >> 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@xxxxxxxxxxx>
> >> Closes: https://lore.kernel.org/platform-driver-x86/e64b771e-3255-42ad-9257-5b8fc6c24ac9@xxxxxx/T/#mc068042dd29df36c16c8af92664860fc4763974b
> >> Signed-off-by: Mario Limonciello <mario.limonciello@xxxxxxx>
> >> ---
> >> 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
> >>
>
