On 26 October 2012 03:14, Francesco Lavra [off-list ref] wrote:

Hi,
Hongbo Zhang wrote:

quoted

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.

[...]

quoted

 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.

quoted

+
      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