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

[Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>]

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@gmail.com>
> ---
> 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