linux-kernel - Re: [PATCH v3 4/5] platform/x86: acer-wmi: use an ACPI bitmap to set the platform profile choices

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] [day] [month] [year] [list]
Message-ID: <2hzdti3v4t65rxpaxofj35rdl6jbkymoc2txivjeddwr3a5nkz@ggltjtr7szty>
Date: Thu, 9 Jan 2025 10:34:38 -0500
From: Kurt Borja <kuurtb@...il.com>
To: Hridesh MG <hridesh699@...il.com>
Cc: Hans de Goede <hdegoede@...hat.com>, 
	Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>, Armin Wolf <W_Armin@....de>, platform-driver-x86@...r.kernel.org, 
	linux-kernel@...r.kernel.org, Shuah Khan <skhan@...uxfoundation.org>
Subject: Re: [PATCH v3 4/5] platform/x86: acer-wmi: use an ACPI bitmap to set
 the platform profile choices

On Thu, Jan 09, 2025 at 04:40:14PM +0530, Hridesh MG wrote:
> On Wed, Jan 8, 2025 at 7:21 PM Kurt Borja <kuurtb@...il.com> wrote:
> >
> > On Wed, Jan 08, 2025 at 02:15:26PM +0530, Hridesh MG wrote:
> > > Currently the choices for the platform profile are hardcoded. There is
> > > an ACPI bitmap accessible via WMI that specifies the supported profiles,
> > > use this bitmap to dynamically set the choices for the platform profile.
> > >
> > > Link: https://lore.kernel.org/platform-driver-x86/ecb60ee5-3df7-4d7e-8ebf-8c162b339ade@gmx.de/
> > > Signed-off-by: Hridesh MG <hridesh699@...il.com>
> > > ---
> > >  drivers/platform/x86/acer-wmi.c | 55 +++++++++++++++++++++++++++++------------
> > >  1 file changed, 39 insertions(+), 16 deletions(-)
> > >
> > > diff --git a/drivers/platform/x86/acer-wmi.c b/drivers/platform/x86/acer-wmi.c
> > > index 7968fe21507b1cf28fdc575139057c795e6a873b..6c98c1bb3bdce6a7c6559f6da4ff3c6ce56b60e3 100644
> > > --- a/drivers/platform/x86/acer-wmi.c
> > > +++ b/drivers/platform/x86/acer-wmi.c
> > > @@ -33,6 +33,7 @@
> > >  #include <linux/units.h>
> > >  #include <linux/unaligned.h>
> > >  #include <linux/bitfield.h>
> > > +#include <linux/bitmap.h>
> > >
> > >  MODULE_AUTHOR("Carlos Corbacho");
> > >  MODULE_DESCRIPTION("Acer Laptop WMI Extras Driver");
> > > @@ -127,6 +128,7 @@ enum acer_wmi_predator_v4_oc {
> > >  enum acer_wmi_gaming_misc_setting {
> > >       ACER_WMID_MISC_SETTING_OC_1                     = 0x0005,
> > >       ACER_WMID_MISC_SETTING_OC_2                     = 0x0007,
> > > +     ACER_WMID_MISC_SETTING_SUPPORTED_PROFILES       = 0x000A,
> > >       ACER_WMID_MISC_SETTING_PLATFORM_PROFILE         = 0x000B,
> > >  };
> > >
> > > @@ -1957,7 +1959,7 @@ static int
> > >  acer_predator_v4_platform_profile_set(struct platform_profile_handler *pprof,
> > >                                     enum platform_profile_option profile)
> > >  {
> > > -     int err, tp;
> > > +     int max_perf, err, tp;
> > >
> > >       switch (profile) {
> > >       case PLATFORM_PROFILE_PERFORMANCE:
> > > @@ -1983,7 +1985,10 @@ acer_predator_v4_platform_profile_set(struct platform_profile_handler *pprof,
> > >       if (err)
> > >               return err;
> > >
> > > -     if (tp != ACER_PREDATOR_V4_THERMAL_PROFILE_TURBO)
> > > +     max_perf = find_last_bit(platform_profile_handler.choices,
> > > +                              PLATFORM_PROFILE_LAST);
> > > +
> > > +     if (tp != max_perf)
> >
> > You can't directly compare `tp` and `max_perf`. ACER_PREDATOR_V4 values
> > may not match PLATFORM_PROFILE ones.
> >
> > It does in the case of PERFORMANCE and TURBO, but it does not in the
> > case of QUIET and BALANCED.
> >
> > I suggest you store the actual ACER_PREDATOR_V4 max_perf when setting up
> > the platform_profile.
> Ah this was quite a stupid mistake. I'm not sure why I even assumed
> both were equivalent. I have one doubt though, if i set it during
> profile setup, the code becomes quite verbose -
> 
>         /* Iterate through supported profiles in order of increasing
> performance */
>         if (test_bit(ACER_PREDATOR_V4_THERMAL_PROFILE_ECO,
> &supported_profiles)) {
>             set_bit(PLATFORM_PROFILE_LOW_POWER,
>                 platform_profile_handler.choices);
>             max_perf = ACER_PREDATOR_V4_THERMAL_PROFILE_ECO;
>         }
> 
>         if (test_bit(ACER_PREDATOR_V4_THERMAL_PROFILE_QUIET,
> &supported_profiles)) {
>             set_bit(PLATFORM_PROFILE_QUIET,
>                 platform_profile_handler.choices);
>             max_perf = ACER_PREDATOR_V4_THERMAL_PROFILE_QUIET;
>         }
> 
>         if (test_bit(ACER_PREDATOR_V4_THERMAL_PROFILE_BALANCED,
> &supported_profiles)) {
>             set_bit(PLATFORM_PROFILE_BALANCED,
>                 platform_profile_handler.choices);
>             max_perf = ACER_PREDATOR_V4_THERMAL_PROFILE_BALANCED;
>         }
> 
>         if (test_bit(ACER_PREDATOR_V4_THERMAL_PROFILE_PERFORMANCE,
> &supported_profiles)) {
>             set_bit(PLATFORM_PROFILE_BALANCED_PERFORMANCE,
>                 platform_profile_handler.choices);
>             max_perf = ACER_PREDATOR_V4_THERMAL_PROFILE_PERFORMANCE;
>         }
> 
>         if (test_bit(ACER_PREDATOR_V4_THERMAL_PROFILE_TURBO,
> &supported_profiles)) {
>             set_bit(PLATFORM_PROFILE_PERFORMANCE,
>                 platform_profile_handler.choices);
>             max_perf = ACER_PREDATOR_V4_THERMAL_PROFILE_TURBO;
>         }

Hi Hridesh,

It looks a bit verbose, but for me it's fine, it even caches the value. 
If max_perf is a global variable now, rename it to something like 
acer_predator_v4_max_perf.

> 
> Is this fine? Maybe for readability's sake, I could lift it up into a
> different function, like what you did in the RFC patch. Btw, thanks a lot
> for the detailed reviews so far—they’ve been very helpful!

Glad I can help :)

~ Kurt

> 
> --
> Thanks,
> Hridesh MG