Re: [RFC PATCH 5/5] ACPI: thermal: processor: Use the generic cpufreq infrastructure

[Eduardo Valentin]

Hello Amit,

On Thu, May 08, 2014 at 08:08:00PM +0530, Amit Daniel Kachhap wrote:
> This patch upgrades the ACPI cpufreq cooling portions to use the generic
> cpufreq cooling infrastructure. There should not be any functionality
> related changes as the same behaviour is provided by the generic
> cpufreq APIs with the notifier mechanism.
> 

This is a very good move as we are converging towards a more and more
common infrastructure. How did you test this code?

> Signed-off-by: Amit Daniel Kachhap <amit.daniel@xxxxxxxxxxx>
> ---
>  drivers/acpi/processor_driver.c  |    6 +-
>  drivers/acpi/processor_thermal.c |  210 +++++++++++++++++---------------------
>  2 files changed, 99 insertions(+), 117 deletions(-)
> 
> diff --git a/drivers/acpi/processor_driver.c b/drivers/acpi/processor_driver.c
> index 7f70f31..10aba4a 100644
> --- a/drivers/acpi/processor_driver.c
> +++ b/drivers/acpi/processor_driver.c
> @@ -36,6 +36,7 @@
>  #include <linux/cpuidle.h>
>  #include <linux/slab.h>
>  #include <linux/acpi.h>
> +#include <linux/cpu_cooling.h>
>  
>  #include <acpi/processor.h>
>  
> @@ -178,8 +179,7 @@ static int __acpi_processor_start(struct acpi_device *device)
>  	if (!cpuidle_get_driver() || cpuidle_get_driver() == &acpi_idle_driver)
>  		acpi_processor_power_init(pr);
>  
> -	pr->cdev = thermal_cooling_device_register("Processor", device,
> -						   &processor_cooling_ops);
> +	pr->cdev = acpi_processor_cooling_register(device);
>  	if (IS_ERR(pr->cdev)) {
>  		result = PTR_ERR(pr->cdev);
>  		goto err_power_exit;
> @@ -250,7 +250,7 @@ static int acpi_processor_stop(struct device *dev)
>  	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);
> +		cpufreq_cooling_unregister(pr->cdev);
>  		pr->cdev = NULL;
>  	}
>  	return 0;
> diff --git a/drivers/acpi/processor_thermal.c b/drivers/acpi/processor_thermal.c
> index e003663..c442a58 100644
> --- a/drivers/acpi/processor_thermal.c
> +++ b/drivers/acpi/processor_thermal.c
> @@ -31,6 +31,7 @@
>  #include <linux/init.h>
>  #include <linux/cpufreq.h>
>  #include <linux/acpi.h>
> +#include <linux/cpu_cooling.h>
>  #include <acpi/processor.h>
>  #include <asm/uaccess.h>
>  
> @@ -53,28 +54,11 @@ ACPI_MODULE_NAME("processor_thermal");
>  
>  static DEFINE_PER_CPU(unsigned int, cpufreq_thermal_reduction_pctg);
>  static unsigned int acpi_thermal_cpufreq_is_init = 0;
> +static struct notifier_block cpufreq_cooling_notifier_block;
>  
>  #define reduction_pctg(cpu) \
>  	per_cpu(cpufreq_thermal_reduction_pctg, phys_package_first_cpu(cpu))
>  
> -/*
> - * Emulate "per package data" using per cpu data (which should really be
> - * provided elsewhere)
> - *
> - * Note we can lose a CPU on cpu hotunplug, in this case we forget the state
> - * temporarily. Fortunately that's not a big issue here (I hope)
> - */
> -static int phys_package_first_cpu(int cpu)
> -{
> -	int i;
> -	int id = topology_physical_package_id(cpu);
> -
> -	for_each_online_cpu(i)
> -		if (topology_physical_package_id(i) == id)
> -			return i;
> -	return 0;
> -}
> -
>  static int cpu_has_cpufreq(unsigned int cpu)
>  {
>  	struct cpufreq_policy policy;
> @@ -83,30 +67,6 @@ static int cpu_has_cpufreq(unsigned int cpu)
>  	return 1;
>  }
>  
> -static int acpi_thermal_cpufreq_notifier(struct notifier_block *nb,
> -					 unsigned long event, void *data)
> -{
> -	struct cpufreq_policy *policy = data;
> -	unsigned long max_freq = 0;
> -
> -	if (event != CPUFREQ_ADJUST)
> -		goto out;
> -
> -	max_freq = (
> -	    policy->cpuinfo.max_freq *
> -	    (100 - reduction_pctg(policy->cpu) * 20)
> -	) / 100;
> -
> -	cpufreq_verify_within_limits(policy, 0, max_freq);
> -
> -      out:
> -	return 0;
> -}
> -
> -static struct notifier_block acpi_thermal_cpufreq_notifier_block = {
> -	.notifier_call = acpi_thermal_cpufreq_notifier,
> -};
> -
>  static int cpufreq_get_max_state(unsigned int cpu)
>  {
>  	if (!cpu_has_cpufreq(cpu))
> @@ -123,34 +83,32 @@ static int cpufreq_get_cur_state(unsigned int cpu)
>  	return reduction_pctg(cpu);
>  }
>  
> -static int cpufreq_set_cur_state(unsigned int cpu, int state)
> +static int acpi_processor_freq_level(unsigned int cpu, int state)
>  {
> -	int i;
> +	struct cpufreq_policy policy;
> +	unsigned long max_freq = 0;
> +	int level = 0;
>  
> -	if (!cpu_has_cpufreq(cpu))
> +	if (!acpi_thermal_cpufreq_is_init || cpufreq_get_policy(&policy, cpu))
>  		return 0;
>  
>  	reduction_pctg(cpu) = state;
> +	max_freq = (
> +	    policy.cpuinfo.max_freq *
> +	    (100 - reduction_pctg(cpu) * 20)
> +	) / 100;
>  
> -	/*
> -	 * Update all the CPUs in the same package because they all
> -	 * contribute to the temperature and often share the same
> -	 * frequency.
> -	 */
> -	for_each_online_cpu(i) {
> -		if (topology_physical_package_id(i) ==
> -		    topology_physical_package_id(cpu))
> -			cpufreq_update_policy(i);
> -	}
> -	return 0;
> +	level =  cpufreq_cooling_get_level(phys_package_first_cpu(cpu),
> +						max_freq, GET_LEVEL_FLOOR);
> +	return level;
>  }
>  
>  void acpi_thermal_cpufreq_init(void)
>  {
>  	int i;
>  
> -	i = cpufreq_register_notifier(&acpi_thermal_cpufreq_notifier_block,
> -				      CPUFREQ_POLICY_NOTIFIER);
> +	i = cpufreq_cooling_register_notifier(&cpufreq_cooling_notifier_block,
> +					CPU_COOLING_STATE_NOTIFIER);
>  	if (!i)
>  		acpi_thermal_cpufreq_is_init = 1;
>  }
> @@ -158,9 +116,9 @@ void acpi_thermal_cpufreq_init(void)
>  void acpi_thermal_cpufreq_exit(void)
>  {
>  	if (acpi_thermal_cpufreq_is_init)
> -		cpufreq_unregister_notifier
> -		    (&acpi_thermal_cpufreq_notifier_block,
> -		     CPUFREQ_POLICY_NOTIFIER);
> +		cpufreq_cooling_unregister_notifier(
> +						&cpufreq_cooling_notifier_block,
> +						CPU_COOLING_STATE_NOTIFIER);
>  
>  	acpi_thermal_cpufreq_is_init = 0;
>  }
> @@ -176,13 +134,31 @@ static int cpufreq_get_cur_state(unsigned int cpu)
>  	return 0;
>  }
>  
> -static int cpufreq_set_cur_state(unsigned int cpu, int state)
> +static int acpi_processor_freq_level(unsigned int cpu, int state)
>  {
>  	return 0;
>  }
>  
>  #endif
>  
> +/*
> + * Emulate "per package data" using per cpu data (which should really be
> + * provided elsewhere)
> + *
> + * Note we can lose a CPU on cpu hotunplug, in this case we forget the state
> + * temporarily. Fortunately that's not a big issue here (I hope)
> + */
> +static int phys_package_first_cpu(int cpu)
> +{
> +	int i;
> +	int id = topology_physical_package_id(cpu);
> +
> +	for_each_online_cpu(i)
> +		if (topology_physical_package_id(i) == id)
> +			return i;
> +	return 0;
> +}
> +
>  /* thermal cooling device callbacks */
>  static int acpi_processor_max_state(struct acpi_processor *pr)
>  {
> @@ -198,57 +174,22 @@ static int acpi_processor_max_state(struct acpi_processor *pr)
>  
>  	return max_state;
>  }
> -static int
> -processor_get_max_state(struct thermal_cooling_device *cdev,
> -			unsigned long *state)
> -{
> -	struct acpi_device *device = cdev->devdata;
> -	struct acpi_processor *pr;
> -
> -	if (!device)
> -		return -EINVAL;
> -
> -	pr = acpi_driver_data(device);
> -	if (!pr)
> -		return -EINVAL;
>  
> -	*state = acpi_processor_max_state(pr);
> -	return 0;
> -}
> -
> -static int
> -processor_get_cur_state(struct thermal_cooling_device *cdev,
> -			unsigned long *cur_state)
> +static int acpi_processor_cur_state(struct acpi_processor *pr)
>  {
> -	struct acpi_device *device = cdev->devdata;
> -	struct acpi_processor *pr;
> -
> -	if (!device)
> -		return -EINVAL;
> -
> -	pr = acpi_driver_data(device);
> -	if (!pr)
> -		return -EINVAL;
> -
> -	*cur_state = cpufreq_get_cur_state(pr->id);
> +	int cur_state = 0;
> +	cur_state = cpufreq_get_cur_state(pr->id);
>  	if (pr->flags.throttling)
> -		*cur_state += pr->throttling.state;
> -	return 0;
> +		cur_state += pr->throttling.state;
> +	return cur_state;
>  }
>  
> -static int
> -processor_set_cur_state(struct thermal_cooling_device *cdev,
> -			unsigned long state)
> +static int acpi_processor_set_cur_state(struct acpi_processor *pr,
> +		struct cpufreq_cooling_status *cooling,	unsigned long event)
>  {
> -	struct acpi_device *device = cdev->devdata;
> -	struct acpi_processor *pr;
> -	int result = 0;
> -	int max_pstate;
> -
> -	if (!device)
> -		return -EINVAL;
> +	int result = 0, level = 0;
> +	int max_pstate, state = cooling->new_state;
>  
> -	pr = acpi_driver_data(device);
>  	if (!pr)
>  		return -EINVAL;
>  
> @@ -257,20 +198,61 @@ processor_set_cur_state(struct thermal_cooling_device *cdev,
>  	if (state > acpi_processor_max_state(pr))
>  		return -EINVAL;
>  
> -	if (state <= max_pstate) {
> +	if (state <= max_pstate && event == CPU_COOLING_SET_STATE_PRE) {
>  		if (pr->flags.throttling && pr->throttling.state)
>  			result = acpi_processor_set_throttling(pr, 0, false);
> -		cpufreq_set_cur_state(pr->id, state);
> -	} else {
> -		cpufreq_set_cur_state(pr->id, max_pstate);
> +	} else if (state > max_pstate && event == CPU_COOLING_SET_STATE_POST) {
>  		result = acpi_processor_set_throttling(pr,
>  				state - max_pstate, false);
>  	}
> +
> +	level = acpi_processor_freq_level(pr->id, state);
> +	cooling->new_state = level;
> +
>  	return result;
>  }
>  
> -const struct thermal_cooling_device_ops processor_cooling_ops = {
> -	.get_max_state = processor_get_max_state,
> -	.get_cur_state = processor_get_cur_state,
> -	.set_cur_state = processor_set_cur_state,
> +static int acpi_cpufreq_cooling_notifier(struct notifier_block *nb,
> +					 unsigned long event, void *data)
> +{
> +	struct cpufreq_cooling_status *cooling = data;
> +	struct acpi_device *device = cooling->devdata;
> +	struct acpi_processor *pr = acpi_driver_data(device);
> +	switch (event) {
> +	case CPU_COOLING_SET_STATE_PRE:
> +	case CPU_COOLING_SET_STATE_POST:
> +		acpi_processor_set_cur_state(pr, cooling, event);
> +		break;
> +	case CPU_COOLING_GET_MAX_STATE:
> +		cooling->max_state = acpi_processor_max_state(pr);
> +		break;
> +	case CPU_COOLING_GET_CUR_STATE:
> +		cooling->cur_state = acpi_processor_cur_state(pr);
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +	return 0;
> +}
> +
> +static struct notifier_block cpufreq_cooling_notifier_block = {
> +	.notifier_call = acpi_cpufreq_cooling_notifier,
>  };
> +
> +struct thermal_cooling_device *
> +acpi_processor_cooling_register(struct acpi_device *device)
> +{
> +	struct thermal_cooling_device *cdev;
> +	struct acpi_processor *pr = acpi_driver_data(device);
> +	int cpu = phys_package_first_cpu(pr->id);
> +	int i;
> +	int id = topology_physical_package_id(cpu);
> +	struct cpumask cpus;
> +
> +	for_each_online_cpu(i)
> +		if (topology_physical_package_id(i) == id)
> +			cpumask_set_cpu(i, &cpus);
> +
> +	cdev = cpufreq_cooling_register(&cpus, (void *)device);
> +	return cdev;
> +}
> -- 
> 1.7.1
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@xxxxxxxxxxxxxxx
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/