On Tue, 4 Mar 2025 at 14:28, Kurt Borja <kuurtb@xxxxxxxxx> 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@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.
> >
> >
> > # 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
> >>>
>
