Re: [RFC PATCH 1/1] acpi: processor_driver: register cooling device per package

[Srinivas Pandruvada]

On Thu, 2016-05-12 at 23:19 -0700, Eduardo Valentin wrote:
> The current throttling strategy is to apply cooling levels
> on all processors in a package. Therefore, if one cooling
> device is requested to cap the frequency, the same request
> is applied to all sibling processors in the same package.
> 
> For this reason, this patch removes the redundant cooling
> devices, registering only one cooling device per package.
> 
> Signed-off-by: Eduardo Valentin <edubezval@xxxxxxxxx>
> ---
> Srinivas,
> 
> This is the outcome of our discussion on the Processor cooling
> device.
> As per your suggestion, I am now attempting to register a single
> cooling device per package.
> 
> The only thing is about the sysfs links. Now the Processor cooling
> device would look like:
> $ ls /sys/class/thermal/cooling_device0/
> cur_stateÂÂdevice.0ÂÂdevice.1ÂÂdevice.2ÂÂdevice.3ÂÂmax_stateÂÂpowerÂÂ
> subsystem	typeÂÂuevent
> 
> Instead of:
> 
> $ ls /sys/class/thermal/cooling_device0/
> cur_stateÂÂdeviceÂÂmax_stateÂÂpowerÂÂsubsystem	typeÂÂuevent
> 
> This is obviously to keep a link between the cooling device and
> its affected devices. Would this break userspace?
> 
Conceptually it is fine and should work. I need to run some tests
before I confirm.
Hopefully I can do in next 2 weeks.

Thanks,
Srinivas
> BR,
> 
> Eduardo Valentin
> 
> Âdrivers/acpi/processor_driver.c | 40
> +++++++++++++++++++++++++++++++++++++---
> Â1 file changed, 37 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/acpi/processor_driver.c
> b/drivers/acpi/processor_driver.c
> index 11154a3..e1e17de 100644
> --- a/drivers/acpi/processor_driver.c
> +++ b/drivers/acpi/processor_driver.c
> @@ -160,9 +160,29 @@ static struct notifier_block acpi_cpu_notifier =
> {
> Â};
> Â
> Â#ifdef CONFIG_ACPI_CPU_FREQ_PSS
> +static struct thermal_cooling_device *
> +acpi_pss_register_cooling(struct acpi_processor *pr, struct
> acpi_device *device)
> +{
> +	int i, cpu = pr->id;
> +
> +	for_each_online_cpu(i) {
> +		if (topology_physical_package_id(i) ==
> +		ÂÂÂÂtopology_physical_package_id(cpu)) {
> +			struct acpi_processor *pre =
> per_cpu(processors, i);
> +
> +			if (pre->cdev && !IS_ERR(pre->cdev))
> +				return pre->cdev;
> +		}
> +	}
> +
> +	return thermal_cooling_device_register("Processor", device,
> +					ÂÂÂÂÂÂÂ&processor_cooling_op
> s);
> +}
> +
> Âstatic int acpi_pss_perf_init(struct acpi_processor *pr,
> Â		struct acpi_device *device)
> Â{
> +	char cpu_id[15];
> Â	int result = 0;
> Â
> Â	acpi_processor_ppc_has_changed(pr, 0);
> @@ -172,8 +192,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_coolin
> g_ops);
> +	pr->cdev = acpi_pss_register_cooling(pr, device);
> Â	if (IS_ERR(pr->cdev)) {
> Â		result = PTR_ERR(pr->cdev);
> Â		return result;
> @@ -191,9 +210,10 @@ static int acpi_pss_perf_init(struct
> acpi_processor *pr,
> Â		goto err_thermal_unregister;
> Â	}
> Â
> +	snprintf(cpu_id, sizeof(cpu_id), "device.%d", pr->id);
> Â	result = sysfs_create_link(&pr->cdev->device.kobj,
> Â				ÂÂÂ&device->dev.kobj,
> -				ÂÂÂ"device");
> +				ÂÂÂcpu_id);
> Â	if (result) {
> Â		dev_err(&pr->cdev->device,
> Â			"Failed to create sysfs link 'device'\n");
> @@ -213,11 +233,25 @@ static int acpi_pss_perf_init(struct
> acpi_processor *pr,
> Âstatic void acpi_pss_perf_exit(struct acpi_processor *pr,
> Â		struct acpi_device *device)
> Â{
> +	char cpu_id[15];
> +	int i;
> +
> Â	if (pr->cdev) {
> Â		sysfs_remove_link(&device->dev.kobj,
> "thermal_cooling");
> +		snprintf(cpu_id, sizeof(cpu_id), "device.%d", pr-
> >id);
> Â		sysfs_remove_link(&pr->cdev->device.kobj, "device");
> Â		thermal_cooling_device_unregister(pr->cdev);
> Â		pr->cdev = NULL;
> +
> +		for_each_online_cpu(i) {
> +			if (topology_physical_package_id(i) ==
> +			ÂÂÂÂtopology_physical_package_id(pr->id)) {
> +				struct acpi_processor *pre;
> +
> +				pre = per_cpu(processors, i);
> +				pre->cdev = NULL;
> +			}
> +		}
> Â	}
> Â}
> Â#else