[v1,1/2] ACPI: processor: perflib: Use the "no limit" frequency QoS

  From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

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
and avoid updating the QoS request when the _PPC return value has not
changed.

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.

While at it, move the _PPC return value check against the state count
earlier to avoid setting performance_platform_limit to an invalid value.

Reported-by: Pratyush Yadav <ptyadav@amazon.de>
Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/acpi/processor_perflib.c |   27 +++++++++++++++++++++------
 1 file changed, 21 insertions(+), 6 deletions(-)
  

On Tue, 2022-12-27 at 20:51 +0100, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> 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
> and avoid updating the QoS request when the _PPC return value has not
> changed.
> 
> 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.
> 
> While at it, move the _PPC return value check against the state count
> earlier to avoid setting performance_platform_limit to an invalid
> value.
> 
> Reported-by: Pratyush Yadav <ptyadav@amazon.de>
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---
>  drivers/acpi/processor_perflib.c |   27 +++++++++++++++++++++------
>  1 file changed, 21 insertions(+), 6 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,30 @@ static int acpi_processor_get_platform_l
>                 }
>         }
>  
> +       index = ppc;
> +
> +       if (pr->performance_platform_limit == index ||
> +           ppc >= pr->performance->state_count)
> +               return 0;

Do we need to re initialize pr->performance_platform_limit to 0 in
acpi_processor_unregister_performance()?

If PPC was 1 before the offline and after online the above check will
cause it to return as the pr->performance_platform_limit is not
changed. Not sure if the PM QOS state is preserved after offline and
online. This is stored in a per CPU variable, not in dynamically
allocated memory which will be reallocated during online again.


Thanks,
Srinivas

> +
>         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)))
> +       if (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;
> +       else
> +               qos_value = pr->performance-
> >states[index].core_frequency * 1000;
> +
> +       ret = freq_qos_update_request(&pr->perflib_req, qos_value);
>         if (ret < 0) {
>                 pr_warn("Failed to update perflib freq constraint:
> CPU%d (%d)\n",
>                         pr->id, ret);
> 
> 
>
  

On Tue, Dec 27 2022, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>
> 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
> and avoid updating the QoS request when the _PPC return value has not
> changed.
>
> 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.
>
> While at it, move the _PPC return value check against the state count
> earlier to avoid setting performance_platform_limit to an invalid value.
>
> Reported-by: Pratyush Yadav <ptyadav@amazon.de>
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

Tested-by: Pratyush Yadav <ptyadav@amazon.de>
  

On Tue, Dec 27, 2022 at 9:55 PM srinivas pandruvada
<srinivas.pandruvada@linux.intel.com> wrote:
>
> On Tue, 2022-12-27 at 20:51 +0100, Rafael J. Wysocki wrote:
> > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> >
> > 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
> > and avoid updating the QoS request when the _PPC return value has not
> > changed.
> >
> > 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.
> >
> > While at it, move the _PPC return value check against the state count
> > earlier to avoid setting performance_platform_limit to an invalid
> > value.
> >
> > Reported-by: Pratyush Yadav <ptyadav@amazon.de>
> > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > ---
> >  drivers/acpi/processor_perflib.c |   27 +++++++++++++++++++++------
> >  1 file changed, 21 insertions(+), 6 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,30 @@ static int acpi_processor_get_platform_l
> >                 }
> >         }
> >
> > +       index = ppc;
> > +
> > +       if (pr->performance_platform_limit == index ||
> > +           ppc >= pr->performance->state_count)
> > +               return 0;
>
> Do we need to re initialize pr->performance_platform_limit to 0 in
> acpi_processor_unregister_performance()?
>
> If PPC was 1 before the offline and after online the above check will
> cause it to return as the pr->performance_platform_limit is not
> changed. Not sure if the PM QOS state is preserved after offline and
> online. This is stored in a per CPU variable, not in dynamically
> allocated memory which will be reallocated during online again.

Good point in general, but the QoS request is tied to the cpufreq
policy, so it is not freed on offline.

However, if the policy goes away and is created again for the same CPU
(like when the intel_pstate mode is changed via its 'status' attribute
in sysfs), there may be a stale value in performance_platform_limit,
so it needs to be cleared in acpi_processor_ppc_init() when the QoS
request is first set to "no limit".

I'll update the patch accordingly.  I think I'll also split it in two
to avoid making too many changes in one go.