| Message ID | 20190621132302.30414-2-daniel.lezcano@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | None | expand |
On 21-06-19, 15:22, Daniel Lezcano wrote: > Currently the function cpufreq_cooling_register() returns a cooling > device pointer which is used back as a pointer to call the function > cpufreq_cooling_unregister(). Even if it is correct, it would make > sense to not leak the structure inside a cpufreq driver and keep the > code thermal code self-encapsulate. Moreover, that forces to add an > extra variable in each driver using this function. > > Instead of passing the cooling device to unregister, pass the policy. > > Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org> > --- > drivers/cpufreq/arm_big_little.c | 2 +- > drivers/cpufreq/cpufreq.c | 2 +- > drivers/thermal/cpu_cooling.c | 18 ++++++++++-------- > drivers/thermal/imx_thermal.c | 4 ++-- > .../thermal/ti-soc-thermal/ti-thermal-common.c | 2 +- > include/linux/cpu_cooling.h | 6 +++--- > 6 files changed, 18 insertions(+), 16 deletions(-) Acked-by: Viresh Kumar <viresh.kumar@linaro.org>
On 24/06/2019 08:03, Viresh Kumar wrote: > On 21-06-19, 15:22, Daniel Lezcano wrote: >> Currently the function cpufreq_cooling_register() returns a cooling >> device pointer which is used back as a pointer to call the function >> cpufreq_cooling_unregister(). Even if it is correct, it would make >> sense to not leak the structure inside a cpufreq driver and keep the >> code thermal code self-encapsulate. Moreover, that forces to add an >> extra variable in each driver using this function. >> >> Instead of passing the cooling device to unregister, pass the policy. >> >> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org> >> --- >> drivers/cpufreq/arm_big_little.c | 2 +- >> drivers/cpufreq/cpufreq.c | 2 +- >> drivers/thermal/cpu_cooling.c | 18 ++++++++++-------- >> drivers/thermal/imx_thermal.c | 4 ++-- >> .../thermal/ti-soc-thermal/ti-thermal-common.c | 2 +- >> include/linux/cpu_cooling.h | 6 +++--- >> 6 files changed, 18 insertions(+), 16 deletions(-) > > Acked-by: Viresh Kumar <viresh.kumar@linaro.org> Just a side note, does it make sense to have the function called from imx_thermal.c and ti-thermal-common.c? Sounds like also a leakage from cpufreq to thermal drivers, no?
On 24-06-19, 09:30, Daniel Lezcano wrote: > On 24/06/2019 08:03, Viresh Kumar wrote: > > On 21-06-19, 15:22, Daniel Lezcano wrote: > >> Currently the function cpufreq_cooling_register() returns a cooling > >> device pointer which is used back as a pointer to call the function > >> cpufreq_cooling_unregister(). Even if it is correct, it would make > >> sense to not leak the structure inside a cpufreq driver and keep the > >> code thermal code self-encapsulate. Moreover, that forces to add an > >> extra variable in each driver using this function. > >> > >> Instead of passing the cooling device to unregister, pass the policy. > >> > >> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org> > >> --- > >> drivers/cpufreq/arm_big_little.c | 2 +- > >> drivers/cpufreq/cpufreq.c | 2 +- > >> drivers/thermal/cpu_cooling.c | 18 ++++++++++-------- > >> drivers/thermal/imx_thermal.c | 4 ++-- > >> .../thermal/ti-soc-thermal/ti-thermal-common.c | 2 +- > >> include/linux/cpu_cooling.h | 6 +++--- > >> 6 files changed, 18 insertions(+), 16 deletions(-) > > > > Acked-by: Viresh Kumar <viresh.kumar@linaro.org> > > Just a side note, does it make sense to have the function called from > imx_thermal.c and ti-thermal-common.c? Sounds like also a leakage from > cpufreq to thermal drivers, no? I am not sure what you are proposing here :)
On 24/06/2019 09:37, Viresh Kumar wrote: > On 24-06-19, 09:30, Daniel Lezcano wrote: >> On 24/06/2019 08:03, Viresh Kumar wrote: >>> On 21-06-19, 15:22, Daniel Lezcano wrote: >>>> Currently the function cpufreq_cooling_register() returns a cooling >>>> device pointer which is used back as a pointer to call the function >>>> cpufreq_cooling_unregister(). Even if it is correct, it would make >>>> sense to not leak the structure inside a cpufreq driver and keep the >>>> code thermal code self-encapsulate. Moreover, that forces to add an >>>> extra variable in each driver using this function. >>>> >>>> Instead of passing the cooling device to unregister, pass the policy. >>>> >>>> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org> >>>> --- >>>> drivers/cpufreq/arm_big_little.c | 2 +- >>>> drivers/cpufreq/cpufreq.c | 2 +- >>>> drivers/thermal/cpu_cooling.c | 18 ++++++++++-------- >>>> drivers/thermal/imx_thermal.c | 4 ++-- >>>> .../thermal/ti-soc-thermal/ti-thermal-common.c | 2 +- >>>> include/linux/cpu_cooling.h | 6 +++--- >>>> 6 files changed, 18 insertions(+), 16 deletions(-) >>> >>> Acked-by: Viresh Kumar <viresh.kumar@linaro.org> >> >> Just a side note, does it make sense to have the function called from >> imx_thermal.c and ti-thermal-common.c? Sounds like also a leakage from >> cpufreq to thermal drivers, no? > > I am not sure what you are proposing here :) Actually I'm asking your opinion :) The structure in drivers/thermal/imx_thermal.c struct imx_thermal_data { struct cpufreq_policy *policy; <<<< in the thermal data ?! [ ... ] }; And then: #ifdef CONFIG_CPU_FREQ /* * Create cooling device in case no #cooling-cells property is available in * CPU node */ static int imx_thermal_register_legacy_cooling(struct imx_thermal_data *data) { struct device_node *np; int ret; data->policy = cpufreq_cpu_get(0); if (!data->policy) { pr_debug("%s: CPUFreq policy not found\n", __func__); return -EPROBE_DEFER; } np = of_get_cpu_node(data->policy->cpu, NULL); if (!np || !of_find_property(np, "#cooling-cells", NULL)) { data->cdev = cpufreq_cooling_register(data->policy); if (IS_ERR(data->cdev)) { ret = PTR_ERR(data->cdev); cpufreq_cpu_put(data->policy); return ret; } } return 0; } [ ... ] Shouldn't this be move in the drivers/cpufreq/<whatever driver> ?
On 24-06-19, 09:45, Daniel Lezcano wrote: > Actually I'm asking your opinion :) > > The structure in drivers/thermal/imx_thermal.c > > struct imx_thermal_data { > struct cpufreq_policy *policy; <<<< in the thermal data ?! > [ ... ] > }; > > And then: > > #ifdef CONFIG_CPU_FREQ > /* > * Create cooling device in case no #cooling-cells property is available in > * CPU node > */ > static int imx_thermal_register_legacy_cooling(struct imx_thermal_data > *data) > { > struct device_node *np; > int ret; > > data->policy = cpufreq_cpu_get(0); > if (!data->policy) { > pr_debug("%s: CPUFreq policy not found\n", __func__); > return -EPROBE_DEFER; > } > > np = of_get_cpu_node(data->policy->cpu, NULL); > > if (!np || !of_find_property(np, "#cooling-cells", NULL)) { > data->cdev = cpufreq_cooling_register(data->policy); > if (IS_ERR(data->cdev)) { > ret = PTR_ERR(data->cdev); > cpufreq_cpu_put(data->policy); > return ret; > } > } > > return 0; > } > > [ ... ] > > Shouldn't this be move in the drivers/cpufreq/<whatever driver> ? Sure, we have platform specific drivers where this can be moved :)
diff --git a/drivers/cpufreq/arm_big_little.c b/drivers/cpufreq/arm_big_little.c index 7fe52fcddcf1..6b243202caa9 100644 --- a/drivers/cpufreq/arm_big_little.c +++ b/drivers/cpufreq/arm_big_little.c @@ -502,7 +502,7 @@ static int bL_cpufreq_exit(struct cpufreq_policy *policy) int cur_cluster = cpu_to_cluster(policy->cpu); if (cur_cluster < MAX_CLUSTERS) { - cpufreq_cooling_unregister(cdev[cur_cluster]); + cpufreq_cooling_unregister(policy); cdev[cur_cluster] = NULL; } diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c index 7c72f7d3509c..dfbc9bea606c 100644 --- a/drivers/cpufreq/cpufreq.c +++ b/drivers/cpufreq/cpufreq.c @@ -1469,7 +1469,7 @@ static int cpufreq_offline(unsigned int cpu) } if (cpufreq_driver->flags & CPUFREQ_IS_COOLING_DEV) { - cpufreq_cooling_unregister(policy->cdev); + cpufreq_cooling_unregister(policy); policy->cdev = NULL; } diff --git a/drivers/thermal/cpu_cooling.c b/drivers/thermal/cpu_cooling.c index 83486775e593..007c7c6bf845 100644 --- a/drivers/thermal/cpu_cooling.c +++ b/drivers/thermal/cpu_cooling.c @@ -78,6 +78,7 @@ struct cpufreq_cooling_device { struct cpufreq_policy *policy; struct list_head node; struct time_in_idle *idle_time; + struct thermal_cooling_device *cdev; }; static DEFINE_IDA(cpufreq_ida); @@ -606,6 +607,7 @@ __cpufreq_cooling_register(struct device_node *np, goto remove_ida; cpufreq_cdev->clipped_freq = get_state_freq(cpufreq_cdev, 0); + cpufreq_cdev->cdev = cdev; mutex_lock(&cooling_list_lock); /* Register the notifier for first cpufreq cooling device */ @@ -699,18 +701,18 @@ EXPORT_SYMBOL_GPL(of_cpufreq_cooling_register); * * This interface function unregisters the "thermal-cpufreq-%x" cooling device. */ -void cpufreq_cooling_unregister(struct thermal_cooling_device *cdev) +void cpufreq_cooling_unregister(struct cpufreq_policy *policy) { struct cpufreq_cooling_device *cpufreq_cdev; bool last; - if (!cdev) - return; - - cpufreq_cdev = cdev->devdata; - mutex_lock(&cooling_list_lock); - list_del(&cpufreq_cdev->node); + list_for_each_entry(cpufreq_cdev, &cpufreq_cdev_list, node) { + if (cpufreq_cdev->policy == policy) { + list_del(&cpufreq_cdev->node); + break; + } + } /* Unregister the notifier for the last cpufreq cooling device */ last = list_empty(&cpufreq_cdev_list); mutex_unlock(&cooling_list_lock); @@ -719,7 +721,7 @@ void cpufreq_cooling_unregister(struct thermal_cooling_device *cdev) cpufreq_unregister_notifier(&thermal_cpufreq_notifier_block, CPUFREQ_POLICY_NOTIFIER); - thermal_cooling_device_unregister(cdev); + thermal_cooling_device_unregister(cpufreq_cdev->cdev); ida_simple_remove(&cpufreq_ida, cpufreq_cdev->id); kfree(cpufreq_cdev->idle_time); kfree(cpufreq_cdev); diff --git a/drivers/thermal/imx_thermal.c b/drivers/thermal/imx_thermal.c index bb6754a5342c..6746f1b73eb7 100644 --- a/drivers/thermal/imx_thermal.c +++ b/drivers/thermal/imx_thermal.c @@ -680,7 +680,7 @@ static int imx_thermal_register_legacy_cooling(struct imx_thermal_data *data) static void imx_thermal_unregister_legacy_cooling(struct imx_thermal_data *data) { - cpufreq_cooling_unregister(data->cdev); + cpufreq_cooling_unregister(data->policy); cpufreq_cpu_put(data->policy); } @@ -872,7 +872,7 @@ static int imx_thermal_remove(struct platform_device *pdev) clk_disable_unprepare(data->thermal_clk); thermal_zone_device_unregister(data->tz); - cpufreq_cooling_unregister(data->cdev); + cpufreq_cooling_unregister(data->policy); cpufreq_cpu_put(data->policy); return 0; diff --git a/drivers/thermal/ti-soc-thermal/ti-thermal-common.c b/drivers/thermal/ti-soc-thermal/ti-thermal-common.c index b4f981daeaf2..217b1aae8b4f 100644 --- a/drivers/thermal/ti-soc-thermal/ti-thermal-common.c +++ b/drivers/thermal/ti-soc-thermal/ti-thermal-common.c @@ -277,7 +277,7 @@ int ti_thermal_unregister_cpu_cooling(struct ti_bandgap *bgp, int id) data = ti_bandgap_get_sensor_data(bgp, id); if (data) { - cpufreq_cooling_unregister(data->cool_dev); + cpufreq_cooling_unregister(data->policy); if (data->policy) cpufreq_cpu_put(data->policy); } diff --git a/include/linux/cpu_cooling.h b/include/linux/cpu_cooling.h index bae54bb7c048..89f469ee4be4 100644 --- a/include/linux/cpu_cooling.h +++ b/include/linux/cpu_cooling.h @@ -29,9 +29,9 @@ cpufreq_cooling_register(struct cpufreq_policy *policy); /** * cpufreq_cooling_unregister - function to remove cpufreq cooling device. - * @cdev: thermal cooling device pointer. + * @policy: cpufreq policy */ -void cpufreq_cooling_unregister(struct thermal_cooling_device *cdev); +void cpufreq_cooling_unregister(struct cpufreq_policy *policy); #else /* !CONFIG_CPU_THERMAL */ static inline struct thermal_cooling_device * @@ -41,7 +41,7 @@ cpufreq_cooling_register(struct cpufreq_policy *policy) } static inline -void cpufreq_cooling_unregister(struct thermal_cooling_device *cdev) +void cpufreq_cooling_unregister(struct cpufreq_policy *policy) { return; }
Currently the function cpufreq_cooling_register() returns a cooling device pointer which is used back as a pointer to call the function cpufreq_cooling_unregister(). Even if it is correct, it would make sense to not leak the structure inside a cpufreq driver and keep the code thermal code self-encapsulate. Moreover, that forces to add an extra variable in each driver using this function. Instead of passing the cooling device to unregister, pass the policy. Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org> --- drivers/cpufreq/arm_big_little.c | 2 +- drivers/cpufreq/cpufreq.c | 2 +- drivers/thermal/cpu_cooling.c | 18 ++++++++++-------- drivers/thermal/imx_thermal.c | 4 ++-- .../thermal/ti-soc-thermal/ti-thermal-common.c | 2 +- include/linux/cpu_cooling.h | 6 +++--- 6 files changed, 18 insertions(+), 16 deletions(-)