linux-kernel - Re: [PATCH v2 1/1] ACPI: platform_profile: Treat quiet and low power the same

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: 
 <CAGwozwHniWGQ7qK6FYD_WK5zNjkro7-Q1nTcFPAuWDt9UQ+noA@mail.gmail.com>
Date: Tue, 4 Mar 2025 09:38:46 +0100
From: Antheas Kapenekakis <lkml@...heas.dev>
To: Mario Limonciello <superm1@...nel.org>
Cc: 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 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.

>         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
>