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

On 26 October 2012 03:14, Francesco Lavra francescolavra.fl@gmail.com wrote:

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.

Good.

[...]

...

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.

In common sense, usually unregister first and then count--; But here it should be opposite sequence of cpufreq_cooling_register, will update 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