lkml.org 
[lkml]   [2023]   [Mar]   [13]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    SubjectRe: [PATCH v1 1/4] ACPI: processor: Reorder acpi_processor_driver_init()
    Date
    On Mon, 2023-03-13 at 14:48 +0100, Rafael J. Wysocki wrote:
    > On Sun, Mar 12, 2023 at 5:09 PM Zhang, Rui <rui.zhang@intel.com>
    > wrote:
    > > On Fri, 2023-03-03 at 20:19 +0100, Rafael J. Wysocki wrote:
    > > > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
    > > >
    > > > The cpufreq policy notifier in the ACPI processor driver may as
    > > > well be registered before the driver itself, which causes
    > > > acpi_processor_cpufreq_init to be true (unless the notifier
    > > > registration fails, which is unlikely at that point) when the
    > > > ACPI CPU thermal cooling devices are registered, so the
    > > > processor_get_max_state() result does not change while
    > > > acpi_processor_driver_init() is running.
    > > >
    > > > Change the ordering in acpi_processor_driver_init() accordingly
    > > > to prevent the max_state value from remaining 0 permanently for
    > > > all
    > > > ACPI CPU cooling devices.
    > > >
    > > > Fixes: a365105c685c("thermal: sysfs: Reuse cdev->max_state")
    > > > Reported-by: Wang, Quanxian <quanxian.wang@intel.com>
    > > > Link:
    > > > https://lore.kernel.org/linux-pm/53ec1f06f61c984100868926f282647e57ecfb2d.camel@intel.com/
    > > > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
    > >
    > > The full patch series fixes the problem but this one does not.
    >
    > That is a correct observation, but the $subject patch fixes part of
    > the problem (which is not addressed by the rest of the series AFAICS)
    > and so it deserves a Fixes tag of its own IMO.

    Am I understanding this correctly that this patch helps in below case?

    cpufreq driver like intel_pstate is registered before we register the
    notifier callback in processor_driver. In this case, we are not able to
    catch the CPUFREQ_CREATE_POLICY notification and cpufreq should be
    counted as part of cooling states when registering the ACPI CPU cooling
    device. (acpi_processor_cpufreq_init must be set at this time)

    Or else, in normal case, the ACPI CPU cdev->max_state always return 0
    (when t-state not available) until we receive the CPUFREQ_CREATE_POLICY
    notification and call thermal_cooling_device_update(), both with and
    without this patch.

    Patch 2,3,4 works on my test platform, without applying patch 1/4.

    thanks,
    rui

    >
    > I guess I should clarify that in the changelog.
    >
    > > This is because,
    > >
    > > static int cpu_has_cpufreq(unsigned int cpu)
    > > {
    > > struct
    > > cpufreq_policy *policy;
    > >
    > > if (!acpi_processor_cpufreq_init)
    > > return 0;
    > >
    > > policy = cpufreq_cpu_get(cpu);
    > > if (policy) {
    > > cpufreq_cpu_put(policy);
    > > return 1;
    > > }
    > > return 0;
    > > }
    > >
    > > Although acpi_processor_cpufreq_init is set to true with patch 1/4,
    > > but
    > > we don't have cpufreq driver registered, thus cpufreq_cpu_get()
    > > return
    > > NULL.
    > > so acpi_processor_cpufreq_init is not the only dependency here. :(
    >
    > Right. That's why the other patches in the series are needed too.
    >
    > > > ---
    > > > drivers/acpi/processor_driver.c | 12 ++++++------
    > > > 1 file changed, 6 insertions(+), 6 deletions(-)
    > > >
    > > > Index: linux-pm/drivers/acpi/processor_driver.c
    > > > =================================================================
    > > > ==
    > > > --- linux-pm.orig/drivers/acpi/processor_driver.c
    > > > +++ linux-pm/drivers/acpi/processor_driver.c
    > > > @@ -263,6 +263,12 @@ static int __init acpi_processor_driver_
    > > > if (acpi_disabled)
    > > > return 0;
    > > >
    > > > + if
    > > > (!cpufreq_register_notifier(&acpi_processor_notifier_block,
    > > > + CPUFREQ_POLICY_NOTIFIER)) {
    > > > + acpi_processor_cpufreq_init = true;
    > > > + acpi_processor_ignore_ppc_init();
    > > > + }
    > > > +
    > > > result = driver_register(&acpi_processor_driver);
    > > > if (result < 0)
    > > > return result;
    > > > @@ -276,12 +282,6 @@ static int __init acpi_processor_driver_
    > > > cpuhp_setup_state_nocalls(CPUHP_ACPI_CPUDRV_DEAD,
    > > > "acpi/cpu-
    > > > drv:dead",
    > > > NULL, acpi_soft_cpu_dead);
    > > >
    > > > - if
    > > > (!cpufreq_register_notifier(&acpi_processor_notifier_block,
    > > > - CPUFREQ_POLICY_NOTIFIER)) {
    > > > - acpi_processor_cpufreq_init = true;
    > > > - acpi_processor_ignore_ppc_init();
    > > > - }
    > > > -
    > > > acpi_processor_throttling_init();
    > > > return 0;
    > > > err:
    > > >
    > > >
    > > >
    \
     
     \ /
      Last update: 2023-03-27 00:56    [W:7.080 / U:0.528 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site