Re: [PATCH v2] ACPI: Split out processor thermal register from ACPI PSS

From: Riwen Lu
Date: Thu Jun 16 2022 - 21:35:35 EST


在 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@xxxxxxxxxxx> writes:

From: Riwen Lu <luriwen@xxxxxxxxxx>

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@xxxxxxxxxx>
---
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;
+ }
+}

[...]