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
_______________________________________________
linaro-dev mailing list
linaro-dev@lists.linaro.org
http://lists.linaro.org/mailman/listinfo/linaro-dev
