Re: [PATCH V2 4/6] Thermal: Remove the cooling_cpufreq_list

[Francesco Lavra <francescolavra.fl@gmail.com>]

Hi,
Hongbo Zhang wrote:
> Problem of using this list is that the cpufreq_get_max_state callback will be
> called when register cooling device by thermal_cooling_device_register, but
> this list isn't ready at this moment. What's more, there is no need to maintain
> such a list, we can get cpufreq_cooling_device instance by the private
> thermal_cooling_device.devdata.
> 
> Signed-off-by: hongbo.zhang <hongbo.zhang at linaro.com>
> ---
>  drivers/thermal/cpu_cooling.c | 81 +++++++++----------------------------------
>  1 file changed, 16 insertions(+), 65 deletions(-)
> 
> diff --git a/drivers/thermal/cpu_cooling.c b/drivers/thermal/cpu_cooling.c
> index 415b041..cc80d29 100644
> --- a/drivers/thermal/cpu_cooling.c
> +++ b/drivers/thermal/cpu_cooling.c
> @@ -58,8 +58,9 @@ struct cpufreq_cooling_device {
>  };
>  static LIST_HEAD(cooling_cpufreq_list);
>  static DEFINE_IDR(cpufreq_idr);
> +static DEFINE_MUTEX(cooling_cpufreq_lock);
>  
> -static struct mutex cooling_cpufreq_lock;
> +static unsigned int cpufreq_dev_count;
>  
>  /* notify_table passes value to the CPUFREQ_ADJUST callback function. */
>  #define NOTIFY_INVALID NULL
> @@ -241,20 +242,12 @@ static int cpufreq_get_max_state(struct thermal_cooling_device *cdev,
>  				 unsigned long *state)
>  {
>  	int ret = -EINVAL, i = 0;
> -	struct cpufreq_cooling_device *cpufreq_device;
> +	struct cpufreq_cooling_device *cpufreq_device = cdev->devdata;
>  	struct cpumask *maskPtr;
>  	unsigned int cpu;
>  	struct cpufreq_frequency_table *table;
>  	unsigned long count = 0;
>  
> -	mutex_lock(&cooling_cpufreq_lock);
> -	list_for_each_entry(cpufreq_device, &cooling_cpufreq_list, node) {
> -		if (cpufreq_device && cpufreq_device->cool_dev == cdev)
> -			break;
> -	}
> -	if (cpufreq_device == NULL)
> -		goto return_get_max_state;
> -
>  	maskPtr = &cpufreq_device->allowed_cpus;
>  	cpu = cpumask_any(maskPtr);
>  	table = cpufreq_frequency_get_table(cpu);
> @@ -276,7 +269,6 @@ static int cpufreq_get_max_state(struct thermal_cooling_device *cdev,
>  	}
>  
>  return_get_max_state:
> -	mutex_unlock(&cooling_cpufreq_lock);
>  	return ret;

Since there is no mutex locking/unlocking anymore, I'd say the goto
label should be removed.

[...]
>  void cpufreq_cooling_unregister(struct thermal_cooling_device *cdev)
>  {
> -	struct cpufreq_cooling_device *cpufreq_dev = NULL;
> -	unsigned int cpufreq_dev_count = 0;
> +	struct cpufreq_cooling_device *cpufreq_dev = cdev->devdata;
>  
> -	mutex_lock(&cooling_cpufreq_lock);
> -	list_for_each_entry(cpufreq_dev, &cooling_cpufreq_list, node) {
> -		if (cpufreq_dev && cpufreq_dev->cool_dev == cdev)
> -			break;
> -		cpufreq_dev_count++;
> -	}
> -
> -	if (!cpufreq_dev || cpufreq_dev->cool_dev != cdev) {
> -		mutex_unlock(&cooling_cpufreq_lock);
> -		return;
> -	}
> +	thermal_cooling_device_unregister(cpufreq_dev->cool_dev);
>  
> -	list_del(&cpufreq_dev->node);
> +	mutex_lock(&cooling_cpufreq_lock);
> +	cpufreq_dev_count--;
>  
>  	/* Unregister the notifier for the last cpufreq cooling device */
> -	if (cpufreq_dev_count == 1) {
> +	if (cpufreq_dev_count == 0) {
>  		cpufreq_unregister_notifier(&thermal_cpufreq_notifier_block,
>  					CPUFREQ_POLICY_NOTIFIER);
>  	}
>  	mutex_unlock(&cooling_cpufreq_lock);
> -	thermal_cooling_device_unregister(cpufreq_dev->cool_dev);

Why did you move the call to thermal_cooling_device_unregister() from
here? I don't see any reason for moving it.

> +
>  	release_idr(&cpufreq_idr, cpufreq_dev->id);
> -	if (cpufreq_dev_count == 1)
> -		mutex_destroy(&cooling_cpufreq_lock);
>  	kfree(cpufreq_dev);
>  }
>  EXPORT_SYMBOL(cpufreq_cooling_unregister);
> -- 
> 1.7.11.3

--
Francesco