Re: [PATCH v2 1/3] ACPI: processor: perflib: Use the "no limit" frequency QoS

[Rafael J. Wysocki]

On Thu, Dec 29, 2022 at 1:58 PM Pratyush Yadav <ptyadav@xxxxxxxxx> wrote:
>
> Hi Rafael,
>
> On Wed, Dec 28 2022, Rafael J. Wysocki wrote:
> > From: Rafael J. Wysocki <rafael.j.wysocki@xxxxxxxxx>
> >
> > When _PPC returns 0, it means that the CPU frequency is not limited by
> > the platform firmware, so make acpi_processor_get_platform_limit()
> > update the frequency QoS request used by it to "no limit" in that case.
> >
> > This addresses a problem with limiting CPU frequency artificially on
> > some systems after CPU offline/online to the frequency that corresponds
> > to the first entry in the _PSS return package.
> >
> > Reported-by: Pratyush Yadav <ptyadav@xxxxxxxxx>
> > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@xxxxxxxxx>
> > ---
> >
> > v1 -> v2:
> >    * Move some changes into a separate patch
> >    * Update the changelog accordingly
> >
> > ---
> >  drivers/acpi/processor_perflib.c |   20 ++++++++++++++++----
> >  1 file changed, 16 insertions(+), 4 deletions(-)
> >
> > Index: linux-pm/drivers/acpi/processor_perflib.c
> > ===================================================================
> > --- linux-pm.orig/drivers/acpi/processor_perflib.c
> > +++ linux-pm/drivers/acpi/processor_perflib.c
> > @@ -53,6 +53,8 @@ static int acpi_processor_get_platform_l
> >  {
> >         acpi_status status = 0;
> >         unsigned long long ppc = 0;
> > +       s32 qos_value;
> > +       int index;
> >         int ret;
> >
> >         if (!pr)
> > @@ -72,17 +74,27 @@ static int acpi_processor_get_platform_l
> >                 }
> >         }
> >
> > +       index = ppc;
> > +
> >         pr_debug("CPU %d: _PPC is %d - frequency %s limited\n", pr->id,
> > -                      (int)ppc, ppc ? "" : "not");
> > +                index, index ? "is" : "is not");
> >
> > -       pr->performance_platform_limit = (int)ppc;
> > +       pr->performance_platform_limit = index;
> >
> >         if (ppc >= pr->performance->state_count ||
> >             unlikely(!freq_qos_request_active(&pr->perflib_req)))
> >                 return 0;
> >
> > -       ret = freq_qos_update_request(&pr->perflib_req,
> > -                       pr->performance->states[ppc].core_frequency * 1000);
> > +       /*
> > +        * If _PPC returns 0, it means that all of the available states can be
> > +        * used ("no limit").
> > +        */
> > +       if (index == 0)
> > +               qos_value = FREQ_QOS_MAX_DEFAULT_VALUE;
>
> One small thing I noticed: in acpi_processor_ppc_init() "no limit" value
> is set to INT_MAX and here it is set to FREQ_QOS_MAX_DEFAULT_VALUE. Both
> should evaluate to the same value but I think it would be nice if the
> same thing is used in both places. Perhaps you can fix that up when
> applying?

Yes, I'll do that.

> Other than this,
>
> Reviewed-by: Pratyush Yadav <ptyadav@xxxxxxxxx>
> Tested-by: Pratyush Yadav <ptyadav@xxxxxxxxx>

Thanks!

> Thanks for working on this.

You're welcome.