lkml.org 
[lkml]   [2022]   [Jun]   [16]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
SubjectRe: [PATCH v2] ACPI: Split out processor thermal register from ACPI PSS
From
在 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;
>> + }
>> +}
>
> [...]

\
 
 \ /
  Last update: 2022-06-17 03:37    [W:0.066 / U:0.408 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site