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

[Punit Agrawal <punit.agrawal@bytedance.com>]

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

[...]