Message-ID: 
 <CAGwozwGyntPekopke0iKZdb-rJEm8MRkhrfvfvhn4sJKRxQb9A@mail.gmail.com>
Date: Tue, 4 Mar 2025 15:02:32 +0100
From: Antheas Kapenekakis <lkml@...heas.dev>
To: Kurt Borja <kuurtb@...il.com>
Cc: Mario Limonciello <superm1@...nel.org>,
 Shyam Sundar S K <Shyam-sundar.S-k@....com>,
	"Rafael J . Wysocki" <rafael@...nel.org>,
 Hans de Goede <hdegoede@...hat.com>,
	Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>,
	"Luke D . Jones" <luke@...nes.dev>, Mark Pearson <mpearson-lenovo@...ebb.ca>,
	"open list:AMD PMF DRIVER" <platform-driver-x86@...r.kernel.org>,
	open list <linux-kernel@...r.kernel.org>,
	"open list:ACPI" <linux-acpi@...r.kernel.org>,
 "Derek J . Clark" <derekjohn.clark@...il.com>,
	me@...egospodneti.ch, Denis Benato <benato.denis96@...il.com>,
	Mario Limonciello <mario.limonciello@....com>
Subject: Re: [PATCH v2 1/1] ACPI: platform_profile: Treat quiet and low power
 the same

On Tue, 4 Mar 2025 at 14:55, Kurt Borja <kuurtb@...il.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@...il.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@...nel.org> wrote:
> >> >>>
> >> >>> From: Mario Limonciello <mario.limonciello@....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@...heas.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@....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
> >> >>>
> >>
>

Powered by blists - more mailing lists