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

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