| Message ID | TYWP286MB2601DDBB0F472C876D36FBCCB1A69@TYWP286MB2601.JPNP286.PROD.OUTLOOK.COM |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v2] ACPI: Split out processor thermal register from ACPI PSS | expand |
Hi Riwen, Usually it's a good practice to Cc anybody who has commented on previous versions. It makes it easier to follow your updates. A couple of comments below. Riwen Lu <luriwen@hotmail.com> writes: > From: Riwen Lu <luriwen@kylinos.cn> > > Commit 239708a3af44 ("ACPI: Split out ACPI PSS from ACPI Processor > driver"), moves processor thermal registration to acpi_pss_perf_init(), > which doesn't get executed if ACPI_CPU_FREQ_PSS is not enabled. > > As ARM64 supports P-states using CPPC, it should be possible to also > support processor passive cooling even if PSS is not enabled. Split > out the processor thermal cooling register from ACPI PSS to support > this, and move it into a separate function in processor_thermal.c. > > Signed-off-by: Riwen Lu <luriwen@kylinos.cn> > --- > drivers/acpi/Kconfig | 2 +- > drivers/acpi/Makefile | 5 +-- > drivers/acpi/processor_driver.c | 72 ++++---------------------------- > drivers/acpi/processor_thermal.c | 69 ++++++++++++++++++++++++++++++ > include/acpi/processor.h | 6 ++- > 5 files changed, 84 insertions(+), 70 deletions(-) > [...] > --- a/drivers/acpi/processor_driver.c > +++ b/drivers/acpi/processor_driver.c [...] > @@ -239,7 +183,7 @@ static int __acpi_processor_start(struct acpi_device *device) > return 0; > > result = -ENODEV; > - acpi_pss_perf_exit(pr, device); > + acpi_processor_thermal_exit(pr); > > err_power_exit: > acpi_processor_power_exit(pr); > @@ -277,10 +221,10 @@ static int acpi_processor_stop(struct device *dev) > return 0; > acpi_processor_power_exit(pr); > > - acpi_pss_perf_exit(pr, device); > - > acpi_cppc_processor_exit(pr); > > + acpi_processor_thermal_exit(pr); > + > return 0; > } > > diff --git a/drivers/acpi/processor_thermal.c b/drivers/acpi/processor_thermal.c > index d8b2dfcd59b5..93928db2ae5f 100644 > --- a/drivers/acpi/processor_thermal.c > +++ b/drivers/acpi/processor_thermal.c > @@ -266,3 +266,72 @@ const struct thermal_cooling_device_ops processor_cooling_ops = { > .get_cur_state = processor_get_cur_state, > .set_cur_state = processor_set_cur_state, > }; > + > +int acpi_processor_thermal_init(struct acpi_processor *pr) > +{ > + struct acpi_device *device; > + int result = 0; > + > + if (!pr) > + return -ENODEV; What's the reason for this check? When will "pr" be NULL in this code path? > + > + device = acpi_fetch_acpi_dev(pr->handle); > + if (!device) > + return -ENODEV; Wouldn't it be better to pass the acpi_device into the function as well? The device is already available in the caller and it'll avoid having to convert it back. > + > + pr->cdev = thermal_cooling_device_register("Processor", device, > + &processor_cooling_ops); > + if (IS_ERR(pr->cdev)) { > + result = PTR_ERR(pr->cdev); > + return result; > + } > + > + dev_dbg(&device->dev, "registered as cooling_device%d\n", > + pr->cdev->id); > + > + result = sysfs_create_link(&device->dev.kobj, > + &pr->cdev->device.kobj, > + "thermal_cooling"); > + if (result) { > + dev_err(&device->dev, > + "Failed to create sysfs link 'thermal_cooling'\n"); > + goto err_thermal_unregister; > + } > + > + result = sysfs_create_link(&pr->cdev->device.kobj, > + &device->dev.kobj, > + "device"); > + if (result) { > + dev_err(&pr->cdev->device, > + "Failed to create sysfs link 'device'\n"); > + goto err_remove_sysfs_thermal; > + } > + > + return 0; > + > +err_remove_sysfs_thermal: > + sysfs_remove_link(&device->dev.kobj, "thermal_cooling"); > +err_thermal_unregister: > + thermal_cooling_device_unregister(pr->cdev); > + > + return result; > +} > + > +void acpi_processor_thermal_exit(struct acpi_processor *pr) > +{ > + struct acpi_device *device; > + > + if (!pr) > + return; > + > + device = acpi_fetch_acpi_dev(pr->handle); > + if (!device) > + return; The same comment about passing the acpi_device structure applies here as well. > + > + if (pr->cdev) { > + sysfs_remove_link(&device->dev.kobj, "thermal_cooling"); > + sysfs_remove_link(&pr->cdev->device.kobj, "device"); > + thermal_cooling_device_unregister(pr->cdev); > + pr->cdev = NULL; > + } > +} [...]
在 2022/6/16 22:56, Punit Agrawal 写道: > Hi Riwen, > > Usually it's a good practice to Cc anybody who has commented on previous > versions. It makes it easier to follow your updates. Hi Punit, Sorry. I wanted to Cc to you, but I forgot it. I'll make the patch a v3 version and Cc you. Thanks! > > A couple of comments below. > > Riwen Lu <luriwen@hotmail.com> writes: > >> From: Riwen Lu <luriwen@kylinos.cn> >> >> Commit 239708a3af44 ("ACPI: Split out ACPI PSS from ACPI Processor >> driver"), moves processor thermal registration to acpi_pss_perf_init(), >> which doesn't get executed if ACPI_CPU_FREQ_PSS is not enabled. >> >> As ARM64 supports P-states using CPPC, it should be possible to also >> support processor passive cooling even if PSS is not enabled. Split >> out the processor thermal cooling register from ACPI PSS to support >> this, and move it into a separate function in processor_thermal.c. >> >> Signed-off-by: Riwen Lu <luriwen@kylinos.cn> >> --- >> drivers/acpi/Kconfig | 2 +- >> drivers/acpi/Makefile | 5 +-- >> drivers/acpi/processor_driver.c | 72 ++++---------------------------- >> drivers/acpi/processor_thermal.c | 69 ++++++++++++++++++++++++++++++ >> include/acpi/processor.h | 6 ++- >> 5 files changed, 84 insertions(+), 70 deletions(-) >> > > [...] > >> --- a/drivers/acpi/processor_driver.c >> +++ b/drivers/acpi/processor_driver.c > > [...] > >> @@ -239,7 +183,7 @@ static int __acpi_processor_start(struct acpi_device *device) >> return 0; >> >> result = -ENODEV; >> - acpi_pss_perf_exit(pr, device); >> + acpi_processor_thermal_exit(pr); >> >> err_power_exit: >> acpi_processor_power_exit(pr); >> @@ -277,10 +221,10 @@ static int acpi_processor_stop(struct device *dev) >> return 0; >> acpi_processor_power_exit(pr); >> >> - acpi_pss_perf_exit(pr, device); >> - >> acpi_cppc_processor_exit(pr); >> >> + acpi_processor_thermal_exit(pr); >> + >> return 0; >> } >> >> diff --git a/drivers/acpi/processor_thermal.c b/drivers/acpi/processor_thermal.c >> index d8b2dfcd59b5..93928db2ae5f 100644 >> --- a/drivers/acpi/processor_thermal.c >> +++ b/drivers/acpi/processor_thermal.c >> @@ -266,3 +266,72 @@ const struct thermal_cooling_device_ops processor_cooling_ops = { >> .get_cur_state = processor_get_cur_state, >> .set_cur_state = processor_set_cur_state, >> }; >> + >> +int acpi_processor_thermal_init(struct acpi_processor *pr) >> +{ >> + struct acpi_device *device; >> + int result = 0; >> + >> + if (!pr) >> + return -ENODEV; > > What's the reason for this check? When will "pr" be NULL in this code > path? > I was thinking the function might be called somewhere else. It seems to be meaningless. >> + >> + device = acpi_fetch_acpi_dev(pr->handle); >> + if (!device) >> + return -ENODEV; > > Wouldn't it be better to pass the acpi_device into the function as well? > The device is already available in the caller and it'll avoid having to > convert it back. > The same reason as above, and I'll modify it. >> + >> + pr->cdev = thermal_cooling_device_register("Processor", device, >> + &processor_cooling_ops); >> + if (IS_ERR(pr->cdev)) { >> + result = PTR_ERR(pr->cdev); >> + return result; >> + } >> + >> + dev_dbg(&device->dev, "registered as cooling_device%d\n", >> + pr->cdev->id); >> + >> + result = sysfs_create_link(&device->dev.kobj, >> + &pr->cdev->device.kobj, >> + "thermal_cooling"); >> + if (result) { >> + dev_err(&device->dev, >> + "Failed to create sysfs link 'thermal_cooling'\n"); >> + goto err_thermal_unregister; >> + } >> + >> + result = sysfs_create_link(&pr->cdev->device.kobj, >> + &device->dev.kobj, >> + "device"); >> + if (result) { >> + dev_err(&pr->cdev->device, >> + "Failed to create sysfs link 'device'\n"); >> + goto err_remove_sysfs_thermal; >> + } >> + >> + return 0; >> + >> +err_remove_sysfs_thermal: >> + sysfs_remove_link(&device->dev.kobj, "thermal_cooling"); >> +err_thermal_unregister: >> + thermal_cooling_device_unregister(pr->cdev); >> + >> + return result; >> +} >> + >> +void acpi_processor_thermal_exit(struct acpi_processor *pr) >> +{ >> + struct acpi_device *device; >> + >> + if (!pr) >> + return; >> + >> + device = acpi_fetch_acpi_dev(pr->handle); >> + if (!device) >> + return; > > The same comment about passing the acpi_device structure applies here as > well. > >> + >> + if (pr->cdev) { >> + sysfs_remove_link(&device->dev.kobj, "thermal_cooling"); >> + sysfs_remove_link(&pr->cdev->device.kobj, "device"); >> + thermal_cooling_device_unregister(pr->cdev); >> + pr->cdev = NULL; >> + } >> +} > > [...]
diff --git a/drivers/acpi/Kconfig b/drivers/acpi/Kconfig index 1e34f846508f..2457ade3f82d 100644 --- a/drivers/acpi/Kconfig +++ b/drivers/acpi/Kconfig @@ -255,7 +255,6 @@ config ACPI_DOCK config ACPI_CPU_FREQ_PSS bool - select THERMAL config ACPI_PROCESSOR_CSTATE def_bool y @@ -287,6 +286,7 @@ config ACPI_PROCESSOR depends on X86 || IA64 || ARM64 || LOONGARCH select ACPI_PROCESSOR_IDLE select ACPI_CPU_FREQ_PSS if X86 || IA64 || LOONGARCH + select THERMAL default y help This driver adds support for the ACPI Processor package. It is required diff --git a/drivers/acpi/Makefile b/drivers/acpi/Makefile index b5a8d3e00a52..0002eecbf870 100644 --- a/drivers/acpi/Makefile +++ b/drivers/acpi/Makefile @@ -109,10 +109,9 @@ obj-$(CONFIG_ACPI_PPTT) += pptt.o obj-$(CONFIG_ACPI_PFRUT) += pfr_update.o pfr_telemetry.o # processor has its own "processor." module_param namespace -processor-y := processor_driver.o +processor-y := processor_driver.o processor_thermal.o processor-$(CONFIG_ACPI_PROCESSOR_IDLE) += processor_idle.o -processor-$(CONFIG_ACPI_CPU_FREQ_PSS) += processor_throttling.o \ - processor_thermal.o +processor-$(CONFIG_ACPI_CPU_FREQ_PSS) += processor_throttling.o processor-$(CONFIG_CPU_FREQ) += processor_perflib.o obj-$(CONFIG_ACPI_PROCESSOR_AGGREGATOR) += acpi_pad.o diff --git a/drivers/acpi/processor_driver.c b/drivers/acpi/processor_driver.c index 368a9edefd0c..a99ff1634665 100644 --- a/drivers/acpi/processor_driver.c +++ b/drivers/acpi/processor_driver.c @@ -139,75 +139,17 @@ static int acpi_soft_cpu_dead(unsigned int cpu) } #ifdef CONFIG_ACPI_CPU_FREQ_PSS -static int acpi_pss_perf_init(struct acpi_processor *pr, - struct acpi_device *device) +static void acpi_pss_perf_init(struct acpi_processor *pr) { - int result = 0; - acpi_processor_ppc_has_changed(pr, 0); acpi_processor_get_throttling_info(pr); if (pr->flags.throttling) pr->flags.limit = 1; - - pr->cdev = thermal_cooling_device_register("Processor", device, - &processor_cooling_ops); - if (IS_ERR(pr->cdev)) { - result = PTR_ERR(pr->cdev); - return result; - } - - dev_dbg(&device->dev, "registered as cooling_device%d\n", - pr->cdev->id); - - result = sysfs_create_link(&device->dev.kobj, - &pr->cdev->device.kobj, - "thermal_cooling"); - if (result) { - dev_err(&device->dev, - "Failed to create sysfs link 'thermal_cooling'\n"); - goto err_thermal_unregister; - } - - result = sysfs_create_link(&pr->cdev->device.kobj, - &device->dev.kobj, - "device"); - if (result) { - dev_err(&pr->cdev->device, - "Failed to create sysfs link 'device'\n"); - goto err_remove_sysfs_thermal; - } - - return 0; - - err_remove_sysfs_thermal: - sysfs_remove_link(&device->dev.kobj, "thermal_cooling"); - err_thermal_unregister: - thermal_cooling_device_unregister(pr->cdev); - - return result; -} - -static void acpi_pss_perf_exit(struct acpi_processor *pr, - struct acpi_device *device) -{ - if (pr->cdev) { - sysfs_remove_link(&device->dev.kobj, "thermal_cooling"); - sysfs_remove_link(&pr->cdev->device.kobj, "device"); - thermal_cooling_device_unregister(pr->cdev); - pr->cdev = NULL; - } } #else -static inline int acpi_pss_perf_init(struct acpi_processor *pr, - struct acpi_device *device) -{ - return 0; -} - -static inline void acpi_pss_perf_exit(struct acpi_processor *pr, - struct acpi_device *device) {} +static inline void acpi_pss_perf_init(struct acpi_processor *pr) {} #endif /* CONFIG_ACPI_CPU_FREQ_PSS */ static int __acpi_processor_start(struct acpi_device *device) @@ -229,7 +171,9 @@ static int __acpi_processor_start(struct acpi_device *device) if (!cpuidle_get_driver() || cpuidle_get_driver() == &acpi_idle_driver) acpi_processor_power_init(pr); - result = acpi_pss_perf_init(pr, device); + acpi_pss_perf_init(pr); + + result = acpi_processor_thermal_init(pr); if (result) goto err_power_exit; @@ -239,7 +183,7 @@ static int __acpi_processor_start(struct acpi_device *device) return 0; result = -ENODEV; - acpi_pss_perf_exit(pr, device); + acpi_processor_thermal_exit(pr); err_power_exit: acpi_processor_power_exit(pr); @@ -277,10 +221,10 @@ static int acpi_processor_stop(struct device *dev) return 0; acpi_processor_power_exit(pr); - acpi_pss_perf_exit(pr, device); - acpi_cppc_processor_exit(pr); + acpi_processor_thermal_exit(pr); + return 0; } diff --git a/drivers/acpi/processor_thermal.c b/drivers/acpi/processor_thermal.c index d8b2dfcd59b5..93928db2ae5f 100644 --- a/drivers/acpi/processor_thermal.c +++ b/drivers/acpi/processor_thermal.c @@ -266,3 +266,72 @@ const struct thermal_cooling_device_ops processor_cooling_ops = { .get_cur_state = processor_get_cur_state, .set_cur_state = processor_set_cur_state, }; + +int acpi_processor_thermal_init(struct acpi_processor *pr) +{ + struct acpi_device *device; + int result = 0; + + if (!pr) + return -ENODEV; + + device = acpi_fetch_acpi_dev(pr->handle); + if (!device) + return -ENODEV; + + pr->cdev = thermal_cooling_device_register("Processor", device, + &processor_cooling_ops); + if (IS_ERR(pr->cdev)) { + result = PTR_ERR(pr->cdev); + return result; + } + + dev_dbg(&device->dev, "registered as cooling_device%d\n", + pr->cdev->id); + + result = sysfs_create_link(&device->dev.kobj, + &pr->cdev->device.kobj, + "thermal_cooling"); + if (result) { + dev_err(&device->dev, + "Failed to create sysfs link 'thermal_cooling'\n"); + goto err_thermal_unregister; + } + + result = sysfs_create_link(&pr->cdev->device.kobj, + &device->dev.kobj, + "device"); + if (result) { + dev_err(&pr->cdev->device, + "Failed to create sysfs link 'device'\n"); + goto err_remove_sysfs_thermal; + } + + return 0; + +err_remove_sysfs_thermal: + sysfs_remove_link(&device->dev.kobj, "thermal_cooling"); +err_thermal_unregister: + thermal_cooling_device_unregister(pr->cdev); + + return result; +} + +void acpi_processor_thermal_exit(struct acpi_processor *pr) +{ + struct acpi_device *device; + + if (!pr) + return; + + device = acpi_fetch_acpi_dev(pr->handle); + if (!device) + return; + + if (pr->cdev) { + sysfs_remove_link(&device->dev.kobj, "thermal_cooling"); + sysfs_remove_link(&pr->cdev->device.kobj, "device"); + thermal_cooling_device_unregister(pr->cdev); + pr->cdev = NULL; + } +} diff --git a/include/acpi/processor.h b/include/acpi/processor.h index 194027371928..5746ac206219 100644 --- a/include/acpi/processor.h +++ b/include/acpi/processor.h @@ -442,8 +442,10 @@ static inline int acpi_processor_hotplug(struct acpi_processor *pr) /* in processor_thermal.c */ int acpi_processor_get_limit_info(struct acpi_processor *pr); +int acpi_processor_thermal_init(struct acpi_processor *pr); +void acpi_processor_thermal_exit(struct acpi_processor *pr); extern const struct thermal_cooling_device_ops processor_cooling_ops; -#if defined(CONFIG_ACPI_CPU_FREQ_PSS) & defined(CONFIG_CPU_FREQ) +#ifdef CONFIG_CPU_FREQ void acpi_thermal_cpufreq_init(struct cpufreq_policy *policy); void acpi_thermal_cpufreq_exit(struct cpufreq_policy *policy); #else @@ -455,6 +457,6 @@ static inline void acpi_thermal_cpufreq_exit(struct cpufreq_policy *policy) { return; } -#endif /* CONFIG_ACPI_CPU_FREQ_PSS */ +#endif /* CONFIG_CPU_FREQ */ #endif