On Mon, Mar 13, 2023 at 3:54 PM Zhang, Rui <rui.zhang@xxxxxxxxx> wrote: > > 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@xxxxxxxxx> > > wrote: > > > On Fri, 2023-03-03 at 20:19 +0100, Rafael J. Wysocki wrote: > > > > From: Rafael J. Wysocki <rafael.j.wysocki@xxxxxxxxx> > > > > > > > > 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@xxxxxxxxx> > > > > Link: > > > > https://lore.kernel.org/linux-pm/53ec1f06f61c984100868926f282647e57ecfb2d.camel@xxxxxxxxx/ > > > > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@xxxxxxxxx> > > > > > > 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) Yes. > 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. OK > > 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: > > > > > > > > > > > >