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

[Hongbo Zhang]

On 26 October 2012 03:14, Francesco Lavra <francescolavra.fl@xxxxxxxxx> 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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/