Messages in this thread | | | Date | Fri, 17 Jun 2022 09:35:19 +0800 | Subject | Re: [PATCH v2] ACPI: Split out processor thermal register from ACPI PSS | From | Riwen Lu <> |
| |
在 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; >> + } >> +} > > [...]
| |