diff mbox series

[v2,1/4] thermal: cpufreq_cooling: Use private callback ops for each cooling device

Message ID 20220613124327.30766-2-lukasz.luba@arm.com
State Accepted
Commit 3cbf6a8ab70b21de75bc389d384911392cf7f616
Headers show
Series Thermal cpufreq & devfreq cooling minor clean-ups | expand

Commit Message

Lukasz Luba June 13, 2022, 12:43 p.m. UTC
It is very unlikely that one CPU cluster would have the EM and some other
won't have it (because EM registration failed or DT lacks needed entry).
Although, we should avoid modifying global variable with callbacks anyway.
Redesign this and add safety for such situation.

Signed-off-by: Lukasz Luba <lukasz.luba@arm.com>
---
 drivers/thermal/cpufreq_cooling.c | 15 ++++++---------
 1 file changed, 6 insertions(+), 9 deletions(-)

Comments

Viresh Kumar June 14, 2022, 2:24 a.m. UTC | #1
On 13-06-22, 13:43, Lukasz Luba wrote:
> It is very unlikely that one CPU cluster would have the EM and some other
> won't have it (because EM registration failed or DT lacks needed entry).
> Although, we should avoid modifying global variable with callbacks anyway.
> Redesign this and add safety for such situation.
> 
> Signed-off-by: Lukasz Luba <lukasz.luba@arm.com>
> ---
>  drivers/thermal/cpufreq_cooling.c | 15 ++++++---------
>  1 file changed, 6 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/thermal/cpufreq_cooling.c b/drivers/thermal/cpufreq_cooling.c
> index b8151d95a806..ad8b86f5281b 100644
> --- a/drivers/thermal/cpufreq_cooling.c
> +++ b/drivers/thermal/cpufreq_cooling.c
> @@ -59,6 +59,7 @@ struct time_in_idle {
>   * @cdev: thermal_cooling_device pointer to keep track of the
>   *	registered cooling device.
>   * @policy: cpufreq policy.
> + * @cooling_ops: cpufreq callbacks to thermal cooling device ops
>   * @idle_time: idle time stats
>   * @qos_req: PM QoS contraint to apply
>   *
> @@ -71,6 +72,7 @@ struct cpufreq_cooling_device {
>  	unsigned int max_level;
>  	struct em_perf_domain *em;
>  	struct cpufreq_policy *policy;
> +	struct thermal_cooling_device_ops cooling_ops;
>  #ifndef CONFIG_SMP
>  	struct time_in_idle *idle_time;
>  #endif
> @@ -485,14 +487,6 @@ static int cpufreq_set_cur_state(struct thermal_cooling_device *cdev,
>  	return ret;
>  }
>  
> -/* Bind cpufreq callbacks to thermal cooling device ops */
> -
> -static struct thermal_cooling_device_ops cpufreq_cooling_ops = {
> -	.get_max_state		= cpufreq_get_max_state,
> -	.get_cur_state		= cpufreq_get_cur_state,
> -	.set_cur_state		= cpufreq_set_cur_state,
> -};
> -
>  /**
>   * __cpufreq_cooling_register - helper function to create cpufreq cooling device
>   * @np: a valid struct device_node to the cooling device device tree node
> @@ -554,7 +548,10 @@ __cpufreq_cooling_register(struct device_node *np,
>  	/* max_level is an index, not a counter */
>  	cpufreq_cdev->max_level = i - 1;
>  
> -	cooling_ops = &cpufreq_cooling_ops;
> +	cooling_ops = &cpufreq_cdev->cooling_ops;
> +	cooling_ops->get_max_state = cpufreq_get_max_state;
> +	cooling_ops->get_cur_state = cpufreq_get_cur_state;
> +	cooling_ops->set_cur_state = cpufreq_set_cur_state;
>  
>  #ifdef CONFIG_THERMAL_GOV_POWER_ALLOCATOR
>  	if (em_is_sane(cpufreq_cdev, em)) {

Acked-by: Viresh Kumar <viresh.kumar@linaro.org>
Lukasz Luba June 14, 2022, 9:50 a.m. UTC | #2
On 6/14/22 03:24, Viresh Kumar wrote:
> On 13-06-22, 13:43, Lukasz Luba wrote:
>> It is very unlikely that one CPU cluster would have the EM and some other
>> won't have it (because EM registration failed or DT lacks needed entry).
>> Although, we should avoid modifying global variable with callbacks anyway.
>> Redesign this and add safety for such situation.
>>
>> Signed-off-by: Lukasz Luba <lukasz.luba@arm.com>
>> ---
>>   drivers/thermal/cpufreq_cooling.c | 15 ++++++---------
>>   1 file changed, 6 insertions(+), 9 deletions(-)
>>
>> diff --git a/drivers/thermal/cpufreq_cooling.c b/drivers/thermal/cpufreq_cooling.c
>> index b8151d95a806..ad8b86f5281b 100644
>> --- a/drivers/thermal/cpufreq_cooling.c
>> +++ b/drivers/thermal/cpufreq_cooling.c
>> @@ -59,6 +59,7 @@ struct time_in_idle {
>>    * @cdev: thermal_cooling_device pointer to keep track of the
>>    *	registered cooling device.
>>    * @policy: cpufreq policy.
>> + * @cooling_ops: cpufreq callbacks to thermal cooling device ops
>>    * @idle_time: idle time stats
>>    * @qos_req: PM QoS contraint to apply
>>    *
>> @@ -71,6 +72,7 @@ struct cpufreq_cooling_device {
>>   	unsigned int max_level;
>>   	struct em_perf_domain *em;
>>   	struct cpufreq_policy *policy;
>> +	struct thermal_cooling_device_ops cooling_ops;
>>   #ifndef CONFIG_SMP
>>   	struct time_in_idle *idle_time;
>>   #endif
>> @@ -485,14 +487,6 @@ static int cpufreq_set_cur_state(struct thermal_cooling_device *cdev,
>>   	return ret;
>>   }
>>   
>> -/* Bind cpufreq callbacks to thermal cooling device ops */
>> -
>> -static struct thermal_cooling_device_ops cpufreq_cooling_ops = {
>> -	.get_max_state		= cpufreq_get_max_state,
>> -	.get_cur_state		= cpufreq_get_cur_state,
>> -	.set_cur_state		= cpufreq_set_cur_state,
>> -};
>> -
>>   /**
>>    * __cpufreq_cooling_register - helper function to create cpufreq cooling device
>>    * @np: a valid struct device_node to the cooling device device tree node
>> @@ -554,7 +548,10 @@ __cpufreq_cooling_register(struct device_node *np,
>>   	/* max_level is an index, not a counter */
>>   	cpufreq_cdev->max_level = i - 1;
>>   
>> -	cooling_ops = &cpufreq_cooling_ops;
>> +	cooling_ops = &cpufreq_cdev->cooling_ops;
>> +	cooling_ops->get_max_state = cpufreq_get_max_state;
>> +	cooling_ops->get_cur_state = cpufreq_get_cur_state;
>> +	cooling_ops->set_cur_state = cpufreq_set_cur_state;
>>   
>>   #ifdef CONFIG_THERMAL_GOV_POWER_ALLOCATOR
>>   	if (em_is_sane(cpufreq_cdev, em)) {
> 
> Acked-by: Viresh Kumar <viresh.kumar@linaro.org>
> 


Thank you Viresh for the ACK!
diff mbox series

Patch

diff --git a/drivers/thermal/cpufreq_cooling.c b/drivers/thermal/cpufreq_cooling.c
index b8151d95a806..ad8b86f5281b 100644
--- a/drivers/thermal/cpufreq_cooling.c
+++ b/drivers/thermal/cpufreq_cooling.c
@@ -59,6 +59,7 @@  struct time_in_idle {
  * @cdev: thermal_cooling_device pointer to keep track of the
  *	registered cooling device.
  * @policy: cpufreq policy.
+ * @cooling_ops: cpufreq callbacks to thermal cooling device ops
  * @idle_time: idle time stats
  * @qos_req: PM QoS contraint to apply
  *
@@ -71,6 +72,7 @@  struct cpufreq_cooling_device {
 	unsigned int max_level;
 	struct em_perf_domain *em;
 	struct cpufreq_policy *policy;
+	struct thermal_cooling_device_ops cooling_ops;
 #ifndef CONFIG_SMP
 	struct time_in_idle *idle_time;
 #endif
@@ -485,14 +487,6 @@  static int cpufreq_set_cur_state(struct thermal_cooling_device *cdev,
 	return ret;
 }
 
-/* Bind cpufreq callbacks to thermal cooling device ops */
-
-static struct thermal_cooling_device_ops cpufreq_cooling_ops = {
-	.get_max_state		= cpufreq_get_max_state,
-	.get_cur_state		= cpufreq_get_cur_state,
-	.set_cur_state		= cpufreq_set_cur_state,
-};
-
 /**
  * __cpufreq_cooling_register - helper function to create cpufreq cooling device
  * @np: a valid struct device_node to the cooling device device tree node
@@ -554,7 +548,10 @@  __cpufreq_cooling_register(struct device_node *np,
 	/* max_level is an index, not a counter */
 	cpufreq_cdev->max_level = i - 1;
 
-	cooling_ops = &cpufreq_cooling_ops;
+	cooling_ops = &cpufreq_cdev->cooling_ops;
+	cooling_ops->get_max_state = cpufreq_get_max_state;
+	cooling_ops->get_cur_state = cpufreq_get_cur_state;
+	cooling_ops->set_cur_state = cpufreq_set_cur_state;
 
 #ifdef CONFIG_THERMAL_GOV_POWER_ALLOCATOR
 	if (em_is_sane(cpufreq_cdev, em)) {