Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <12138067.O9o76ZdvQC@kreacher> <24821308109ba20d845e11caf32bede92fec5d8e.camel@linux.intel.com>
In-Reply-To: <24821308109ba20d845e11caf32bede92fec5d8e.camel@linux.intel.com>
From:   "Rafael J. Wysocki" <rafael@kernel.org>
Date:   Wed, 28 Dec 2022 21:41:24 +0100
Message-ID: <CAJZ5v0j1km2y6RAZcfJ8ERnn8r73vm2mSd6MUq2mHj32hFXu-g@mail.gmail.com>
Subject: Re: [PATCH v1 1/2] ACPI: processor: perflib: Use the "no limit"
 frequency QoS
To:     srinivas pandruvada <srinivas.pandruvada@linux.intel.com>
Cc:     "Rafael J. Wysocki" <rjw@rjwysocki.net>,
        Linux PM <linux-pm@vger.kernel.org>,
        Pratyush Yadav <ptyadav@amazon.de>,
        LKML <linux-kernel@vger.kernel.org>,
        Linux ACPI <linux-acpi@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

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.