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

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

[Riwen Lu]

From: Riwen Lu <luriwen@kylinos.com>

In prior commit 239708a3af44 ("ACPI: Split out ACPI PSS from ACPI Processor
driver"), move processor thermal register to acpi_pss_perf_init(), and it
won't excute if ACPI_CPU_FREQ_PSS not enabled.

Since ARM64 support P states by CPPC, it should also support processor
passive cooling. So split out the processor thermal cooling register from
ACPI PSS.

Signed-off-by: Riwen Lu <luriwen@kylinos.com>
---
 drivers/acpi/Kconfig            |  2 +-
 drivers/acpi/Makefile           |  5 +-
 drivers/acpi/processor_driver.c | 97 ++++++++++++++-------------------
 include/acpi/processor.h        |  4 +-
 4 files changed, 45 insertions(+), 63 deletions(-)

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..c84738a24eca 100644
--- a/drivers/acpi/processor_driver.c
+++ b/drivers/acpi/processor_driver.c
@@ -142,8 +142,6 @@ static int acpi_soft_cpu_dead(unsigned int cpu)
 static int acpi_pss_perf_init(struct acpi_processor *pr,
 		struct acpi_device *device)
 {
-	int result = 0;
-
 	acpi_processor_ppc_has_changed(pr, 0);
 
 	acpi_processor_get_throttling_info(pr);
@@ -151,53 +149,7 @@ static int acpi_pss_perf_init(struct acpi_processor *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,
@@ -205,9 +157,6 @@ static inline int acpi_pss_perf_init(struct acpi_processor *pr,
 {
 	return 0;
 }
-
-static inline void acpi_pss_perf_exit(struct acpi_processor *pr,
-		struct acpi_device *device) {}
 #endif /* CONFIG_ACPI_CPU_FREQ_PSS */
 
 static int __acpi_processor_start(struct acpi_device *device)
@@ -229,9 +178,35 @@ 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);
-	if (result)
+	acpi_pss_perf_init(pr, device);
+
+	pr->cdev = thermal_cooling_device_register("Processor", device,
+						   &processor_cooling_ops);
+	if (IS_ERR(pr->cdev)) {
+		result = PTR_ERR(pr->cdev);
 		goto err_power_exit;
+	}
+
+	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;
+	}
 
 	status = acpi_install_notify_handler(device->handle, ACPI_DEVICE_NOTIFY,
 					     acpi_processor_notify, device);
@@ -239,8 +214,11 @@ static int __acpi_processor_start(struct acpi_device *device)
 		return 0;
 
 	result = -ENODEV;
-	acpi_pss_perf_exit(pr, device);
-
+	sysfs_remove_link(&pr->cdev->device.kobj, "device");
+err_remove_sysfs_thermal:
+	sysfs_remove_link(&device->dev.kobj, "thermal_cooling");
+err_thermal_unregister:
+	thermal_cooling_device_unregister(pr->cdev);
 err_power_exit:
 	acpi_processor_power_exit(pr);
 	return result;
@@ -277,10 +255,15 @@ 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);
 
+	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;
+	}
+
 	return 0;
 }
 
diff --git a/include/acpi/processor.h b/include/acpi/processor.h
index 194027371928..f2d8f0cd1736 100644
--- a/include/acpi/processor.h
+++ b/include/acpi/processor.h
@@ -443,7 +443,7 @@ static inline int acpi_processor_hotplug(struct acpi_processor *pr)
 /* in processor_thermal.c */
 int acpi_processor_get_limit_info(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 +455,6 @@ static inline void acpi_thermal_cpufreq_exit(struct cpufreq_policy *policy)
 {
 	return;
 }
-#endif	/* CONFIG_ACPI_CPU_FREQ_PSS */
+#endif /* CONFIG_CPU_FREQ */
 
 #endif
-- 
2.25.1

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

[Punit Agrawal]

Hi Riwen,

Riwen Lu <luriwen@hotmail.com> writes:

> From: Riwen Lu <luriwen@kylinos.com>
>
> In prior commit 239708a3af44 ("ACPI: Split out ACPI PSS from ACPI Processor
> driver"), move processor thermal register to acpi_pss_perf_init(), and it
> won't excute if ACPI_CPU_FREQ_PSS not enabled.
>
> Since ARM64 support P states by CPPC, it should also support processor
> passive cooling. So split out the processor thermal cooling register from
> ACPI PSS.

The commit log is a bit difficult to understand - maybe reword as -

    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.

> Signed-off-by: Riwen Lu <luriwen@kylinos.com>
> ---

[...]

> diff --git a/drivers/acpi/processor_driver.c b/drivers/acpi/processor_driver.c
> index 368a9edefd0c..c84738a24eca 100644
> --- a/drivers/acpi/processor_driver.c
> +++ b/drivers/acpi/processor_driver.c
> @@ -142,8 +142,6 @@ static int acpi_soft_cpu_dead(unsigned int cpu)
>  static int acpi_pss_perf_init(struct acpi_processor *pr,
>  		struct acpi_device *device)
>  {
> -	int result = 0;
> -
>  	acpi_processor_ppc_has_changed(pr, 0);
>  
>  	acpi_processor_get_throttling_info(pr);
> @@ -151,53 +149,7 @@ static int acpi_pss_perf_init(struct acpi_processor *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,
> @@ -205,9 +157,6 @@ static inline int acpi_pss_perf_init(struct acpi_processor *pr,
>  {
>  	return 0;
>  }
> -
> -static inline void acpi_pss_perf_exit(struct acpi_processor *pr,
> -		struct acpi_device *device) {}
>  #endif /* CONFIG_ACPI_CPU_FREQ_PSS */
>  
>  static int __acpi_processor_start(struct acpi_device *device)
> @@ -229,9 +178,35 @@ 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);
> -	if (result)
> +	acpi_pss_perf_init(pr, device);

The return value of acpi_pss_perf_init() isn't used anymore. Please also
update the function signature to void and drop the redundant return.

> +
> +	pr->cdev = thermal_cooling_device_register("Processor", device,
> +						   &processor_cooling_ops);
> +	if (IS_ERR(pr->cdev)) {
> +		result = PTR_ERR(pr->cdev);
>  		goto err_power_exit;
> +	}
> +
> +	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;
> +	}

Instead of copying the block back here, it would be better to move it
into a separate function in processor_thermal.c that can be called
here.

>  
>  	status = acpi_install_notify_handler(device->handle, ACPI_DEVICE_NOTIFY,
>  					     acpi_processor_notify, device);
> @@ -239,8 +214,11 @@ static int __acpi_processor_start(struct acpi_device *device)
>  		return 0;
>  
>  	result = -ENODEV;
> -	acpi_pss_perf_exit(pr, device);
> -
> +	sysfs_remove_link(&pr->cdev->device.kobj, "device");
> +err_remove_sysfs_thermal:
> +	sysfs_remove_link(&device->dev.kobj, "thermal_cooling");
> +err_thermal_unregister:
> +	thermal_cooling_device_unregister(pr->cdev);
>  err_power_exit:
>  	acpi_processor_power_exit(pr);
>  	return result;
> @@ -277,10 +255,15 @@ 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);
>  
> +	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;
> +	}
> +

Similarly, the removal can also be moved to a function in processor_thermal.c.

Thoughts?

[...]