On Tue, 4 Mar 2025, Rafael J. Wysocki wrote:
> On Tue, Mar 4, 2025 at 3:52 PM Mario Limonciello <superm1@xxxxxxxxxx> wrote:
> >
> > On 3/4/2025 08:08, Rafael J. Wysocki wrote:
> > > On Tue, Mar 4, 2025 at 1:49 PM 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.
> > >
> > > 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?
>
Yes, I'm fine with that.
I would have acked those patches earlier but noticed they'd managed to in
the meantime come up yet another version of the fix so I waited some more.
I've added my ack there now.
--
i.
