| Message ID | 2885079.e9J7NaK4W3@kreacher |
|---|---|
| State | Superseded |
| Headers | show |
| Series | thermal: core/ACPI: Fix processor cooling device regression | expand |
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> > --- > 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: > Just FYI. I need some time to ramp up on the ordering here to double confirm this does not break any dependency, too many things are involved here :p. I will test the whole patch series later this week. thanks, rui
On Tue, Mar 7, 2023 at 5:55 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> > > --- > > 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: > > > Just FYI. > I need some time to ramp up on the ordering here to double confirm this > does not break any dependency, too many things are involved here :p. Unless I've overlooked something tricky, it should be fine, but of course verifying this independently won't hurt. > I will test the whole patch series later this week. Thank you!
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. 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. :( thanks, rui > --- > 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: > > >
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. 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: > > > > > >
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: > > > > > > > > >
On Mon, Mar 13, 2023 at 3:54 PM Zhang, Rui <rui.zhang@intel.com> 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@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) 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: > > > > > > > > > > > >
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: