* [PATCH V3 1/3] cpufreq: Move the IS_ENABLED(CPU_THERMAL) macro in a stub @ 2019-06-25 11:32 Daniel Lezcano 2019-06-25 11:32 ` [PATCH V3 2/3] thermal/drivers/cpu_cooling: Unregister with the policy Daniel Lezcano 2019-06-25 11:32 ` [PATCH V3 3/3] thermal/drivers/cpu_cooling: cpufreq_cooling_register returns an int Daniel Lezcano 0 siblings, 2 replies; 11+ messages in thread From: Daniel Lezcano @ 2019-06-25 11:32 UTC (permalink / raw) To: viresh.kumar Cc: rjw, edubezval, linux-kernel, open list:CPU FREQUENCY SCALING FRAMEWORK The cpufreq_online and the cpufreq_offline [un]register the driver as a cooling device. This is done if the driver is flagged as a cooling device in addition with a IS_ENABLED macro to compile out the branching code. Group this test in a stub function added in the cpufreq header instead of having the IS_ENABLED in the code path. Suggested-by: Rafael J. Wysocki <rafael@kernel.org> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org> Acked-by: Viresh Kumar <viresh.kumar@linaro.org> --- drivers/cpufreq/cpufreq.c | 6 ++---- include/linux/cpufreq.h | 6 ++++++ 2 files changed, 8 insertions(+), 4 deletions(-) diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c index 85ff958e01f1..aee024e42618 100644 --- a/drivers/cpufreq/cpufreq.c +++ b/drivers/cpufreq/cpufreq.c @@ -1378,8 +1378,7 @@ static int cpufreq_online(unsigned int cpu) if (cpufreq_driver->ready) cpufreq_driver->ready(policy); - if (IS_ENABLED(CONFIG_CPU_THERMAL) && - cpufreq_driver->flags & CPUFREQ_IS_COOLING_DEV) + if (cpufreq_thermal_control_enabled(cpufreq_driver)) policy->cdev = of_cpufreq_cooling_register(policy); pr_debug("initialization complete\n"); @@ -1469,8 +1468,7 @@ static int cpufreq_offline(unsigned int cpu) goto unlock; } - if (IS_ENABLED(CONFIG_CPU_THERMAL) && - cpufreq_driver->flags & CPUFREQ_IS_COOLING_DEV) { + if (cpufreq_thermal_control_enabled(cpufreq_driver)) { cpufreq_cooling_unregister(policy->cdev); policy->cdev = NULL; } diff --git a/include/linux/cpufreq.h b/include/linux/cpufreq.h index d01a74fbc4db..a1467aa7f58b 100644 --- a/include/linux/cpufreq.h +++ b/include/linux/cpufreq.h @@ -409,6 +409,12 @@ int cpufreq_unregister_driver(struct cpufreq_driver *driver_data); const char *cpufreq_get_current_driver(void); void *cpufreq_get_driver_data(void); +static inline int cpufreq_thermal_control_enabled(struct cpufreq_driver *drv) +{ + return IS_ENABLED(CONFIG_CPU_THERMAL) && + (drv->flags & CPUFREQ_IS_COOLING_DEV); +} + static inline void cpufreq_verify_within_limits(struct cpufreq_policy *policy, unsigned int min, unsigned int max) { -- 2.17.1 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH V3 2/3] thermal/drivers/cpu_cooling: Unregister with the policy 2019-06-25 11:32 [PATCH V3 1/3] cpufreq: Move the IS_ENABLED(CPU_THERMAL) macro in a stub Daniel Lezcano @ 2019-06-25 11:32 ` Daniel Lezcano 2019-06-26 2:58 ` Viresh Kumar 2019-06-25 11:32 ` [PATCH V3 3/3] thermal/drivers/cpu_cooling: cpufreq_cooling_register returns an int Daniel Lezcano 1 sibling, 1 reply; 11+ messages in thread From: Daniel Lezcano @ 2019-06-25 11:32 UTC (permalink / raw) To: viresh.kumar Cc: rjw, edubezval, linux-kernel, Sudeep Holla, Amit Daniel Kachhap, Javi Merino, Zhang Rui, Shawn Guo, Sascha Hauer, Pengutronix Kernel Team, Fabio Estevam, NXP Linux Team, Keerthy, open list:CPU FREQUENCY DRIVERS - ARM BIG LITTLE, moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE, open list:TI BANDGAP AND THERMAL DRIVER 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. Because the cpufreq_cooling_unregister() function uses the policy to unregister itself. The only purpose of the cooling device pointer is to unregister the cpu cooling device. As there is no more need of this pointer, remove it. Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org> Acked-by: Viresh Kumar <viresh.kumar@linaro.org> --- drivers/cpufreq/arm_big_little.c | 9 +++------ drivers/cpufreq/cpufreq.c | 10 ++++------ drivers/thermal/cpu_cooling.c | 18 ++++++++++-------- drivers/thermal/imx_thermal.c | 12 ++++++------ .../thermal/ti-soc-thermal/ti-thermal-common.c | 10 +++++----- include/linux/cpu_cooling.h | 6 +++--- include/linux/cpufreq.h | 3 --- 7 files changed, 31 insertions(+), 37 deletions(-) diff --git a/drivers/cpufreq/arm_big_little.c b/drivers/cpufreq/arm_big_little.c index 7fe52fcddcf1..718c63231e66 100644 --- a/drivers/cpufreq/arm_big_little.c +++ b/drivers/cpufreq/arm_big_little.c @@ -56,7 +56,6 @@ static bool bL_switching_enabled; #define ACTUAL_FREQ(cluster, freq) ((cluster == A7_CLUSTER) ? freq << 1 : freq) #define VIRT_FREQ(cluster, freq) ((cluster == A7_CLUSTER) ? freq >> 1 : freq) -static struct thermal_cooling_device *cdev[MAX_CLUSTERS]; static const struct cpufreq_arm_bL_ops *arm_bL_ops; static struct clk *clk[MAX_CLUSTERS]; static struct cpufreq_frequency_table *freq_table[MAX_CLUSTERS + 1]; @@ -501,10 +500,8 @@ static int bL_cpufreq_exit(struct cpufreq_policy *policy) struct device *cpu_dev; int cur_cluster = cpu_to_cluster(policy->cpu); - if (cur_cluster < MAX_CLUSTERS) { - cpufreq_cooling_unregister(cdev[cur_cluster]); - cdev[cur_cluster] = NULL; - } + if (cur_cluster < MAX_CLUSTERS) + cpufreq_cooling_unregister(policy); cpu_dev = get_cpu_device(policy->cpu); if (!cpu_dev) { @@ -527,7 +524,7 @@ static void bL_cpufreq_ready(struct cpufreq_policy *policy) if (cur_cluster >= MAX_CLUSTERS) return; - cdev[cur_cluster] = of_cpufreq_cooling_register(policy); + of_cpufreq_cooling_register(policy); } static struct cpufreq_driver bL_cpufreq_driver = { diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c index aee024e42618..f07454249fbc 100644 --- a/drivers/cpufreq/cpufreq.c +++ b/drivers/cpufreq/cpufreq.c @@ -1379,8 +1379,8 @@ static int cpufreq_online(unsigned int cpu) cpufreq_driver->ready(policy); if (cpufreq_thermal_control_enabled(cpufreq_driver)) - policy->cdev = of_cpufreq_cooling_register(policy); - + of_cpufreq_cooling_register(policy); + pr_debug("initialization complete\n"); return 0; @@ -1468,10 +1468,8 @@ static int cpufreq_offline(unsigned int cpu) goto unlock; } - if (cpufreq_thermal_control_enabled(cpufreq_driver)) { - cpufreq_cooling_unregister(policy->cdev); - policy->cdev = NULL; - } + if (cpufreq_thermal_control_enabled(cpufreq_driver)) + cpufreq_cooling_unregister(policy); if (cpufreq_driver->stop_cpu) cpufreq_driver->stop_cpu(policy); 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..021c0948b740 100644 --- a/drivers/thermal/imx_thermal.c +++ b/drivers/thermal/imx_thermal.c @@ -203,7 +203,6 @@ static struct thermal_soc_data thermal_imx7d_data = { struct imx_thermal_data { struct cpufreq_policy *policy; struct thermal_zone_device *tz; - struct thermal_cooling_device *cdev; enum thermal_device_mode mode; struct regmap *tempmon; u32 c1, c2; /* See formula in imx_init_calib() */ @@ -656,6 +655,7 @@ MODULE_DEVICE_TABLE(of, of_imx_thermal_match); static int imx_thermal_register_legacy_cooling(struct imx_thermal_data *data) { struct device_node *np; + struct thermal_cooling_device *cdev; int ret; data->policy = cpufreq_cpu_get(0); @@ -667,9 +667,9 @@ static int imx_thermal_register_legacy_cooling(struct imx_thermal_data *data) 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); + cdev = cpufreq_cooling_register(data->policy); + if (IS_ERR(cdev)) { + ret = PTR_ERR(cdev); cpufreq_cpu_put(data->policy); return ret; } @@ -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..170b70b6ec61 100644 --- a/drivers/thermal/ti-soc-thermal/ti-thermal-common.c +++ b/drivers/thermal/ti-soc-thermal/ti-thermal-common.c @@ -41,7 +41,6 @@ struct ti_thermal_data { struct cpufreq_policy *policy; struct thermal_zone_device *ti_thermal; struct thermal_zone_device *pcb_tz; - struct thermal_cooling_device *cool_dev; struct ti_bandgap *bgp; enum thermal_device_mode mode; struct work_struct thermal_wq; @@ -233,6 +232,7 @@ int ti_thermal_register_cpu_cooling(struct ti_bandgap *bgp, int id) { struct ti_thermal_data *data; struct device_node *np = bgp->dev->of_node; + struct thermal_cooling_device *cdev; /* * We are assuming here that if one deploys the zone @@ -256,9 +256,9 @@ int ti_thermal_register_cpu_cooling(struct ti_bandgap *bgp, int id) } /* Register cooling device */ - data->cool_dev = cpufreq_cooling_register(data->policy); - if (IS_ERR(data->cool_dev)) { - int ret = PTR_ERR(data->cool_dev); + cdev = cpufreq_cooling_register(data->policy); + if (IS_ERR(cdev)) { + int ret = PTR_ERR(cdev); dev_err(bgp->dev, "Failed to register cpu cooling device %d\n", ret); cpufreq_cpu_put(data->policy); @@ -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; } diff --git a/include/linux/cpufreq.h b/include/linux/cpufreq.h index a1467aa7f58b..ce13204df972 100644 --- a/include/linux/cpufreq.h +++ b/include/linux/cpufreq.h @@ -144,9 +144,6 @@ struct cpufreq_policy { /* For cpufreq driver's internal use */ void *driver_data; - - /* Pointer to the cooling device if used for thermal mitigation */ - struct thermal_cooling_device *cdev; }; struct cpufreq_freqs { -- 2.17.1 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH V3 2/3] thermal/drivers/cpu_cooling: Unregister with the policy 2019-06-25 11:32 ` [PATCH V3 2/3] thermal/drivers/cpu_cooling: Unregister with the policy Daniel Lezcano @ 2019-06-26 2:58 ` Viresh Kumar 2019-06-26 6:02 ` Daniel Lezcano 0 siblings, 1 reply; 11+ messages in thread From: Viresh Kumar @ 2019-06-26 2:58 UTC (permalink / raw) To: Daniel Lezcano Cc: rjw, edubezval, linux-kernel, Sudeep Holla, Amit Daniel Kachhap, Javi Merino, Zhang Rui, Shawn Guo, Sascha Hauer, Pengutronix Kernel Team, Fabio Estevam, NXP Linux Team, Keerthy, open list:CPU FREQUENCY DRIVERS - ARM BIG LITTLE, moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE, open list:TI BANDGAP AND THERMAL DRIVER On 25-06-19, 13:32, Daniel Lezcano wrote: > diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c > index aee024e42618..f07454249fbc 100644 > --- a/drivers/cpufreq/cpufreq.c > +++ b/drivers/cpufreq/cpufreq.c > @@ -1379,8 +1379,8 @@ static int cpufreq_online(unsigned int cpu) > cpufreq_driver->ready(policy); > > if (cpufreq_thermal_control_enabled(cpufreq_driver)) > - policy->cdev = of_cpufreq_cooling_register(policy); > - > + of_cpufreq_cooling_register(policy); > + We don't need any error checking here anymore ? > pr_debug("initialization complete\n"); > > return 0; > @@ -1468,10 +1468,8 @@ static int cpufreq_offline(unsigned int cpu) > goto unlock; > } > > - if (cpufreq_thermal_control_enabled(cpufreq_driver)) { > - cpufreq_cooling_unregister(policy->cdev); > - policy->cdev = NULL; > - } > + if (cpufreq_thermal_control_enabled(cpufreq_driver)) > + cpufreq_cooling_unregister(policy); And we unregister unconditionally, even if we failed ? What if this routine prints error messages for such an case ? > > if (cpufreq_driver->stop_cpu) > cpufreq_driver->stop_cpu(policy); > 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; > + } > + } What if we reach here without a match for the policy ? We shouldn't continue and error out, right ? Print an error message as well ? -- viresh ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH V3 2/3] thermal/drivers/cpu_cooling: Unregister with the policy 2019-06-26 2:58 ` Viresh Kumar @ 2019-06-26 6:02 ` Daniel Lezcano 2019-06-26 6:37 ` Viresh Kumar 0 siblings, 1 reply; 11+ messages in thread From: Daniel Lezcano @ 2019-06-26 6:02 UTC (permalink / raw) To: Viresh Kumar Cc: rjw, edubezval, linux-kernel, Sudeep Holla, Amit Daniel Kachhap, Javi Merino, Zhang Rui, Shawn Guo, Sascha Hauer, Pengutronix Kernel Team, Fabio Estevam, NXP Linux Team, Keerthy, open list:CPU FREQUENCY DRIVERS - ARM BIG LITTLE, moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE, open list:TI BANDGAP AND THERMAL DRIVER Hi Viresh, On 26/06/2019 04:58, Viresh Kumar wrote: > On 25-06-19, 13:32, Daniel Lezcano wrote: >> diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c >> index aee024e42618..f07454249fbc 100644 >> --- a/drivers/cpufreq/cpufreq.c >> +++ b/drivers/cpufreq/cpufreq.c >> @@ -1379,8 +1379,8 @@ static int cpufreq_online(unsigned int cpu) >> cpufreq_driver->ready(policy); >> >> if (cpufreq_thermal_control_enabled(cpufreq_driver)) >> - policy->cdev = of_cpufreq_cooling_register(policy); >> - >> + of_cpufreq_cooling_register(policy); >> + > > We don't need any error checking here anymore ? There was no error checking initially. This comment and the others below are for an additional patch IMO, not a change in this one. >> pr_debug("initialization complete\n"); >> >> return 0; >> @@ -1468,10 +1468,8 @@ static int cpufreq_offline(unsigned int cpu) >> goto unlock; >> } >> >> - if (cpufreq_thermal_control_enabled(cpufreq_driver)) { >> - cpufreq_cooling_unregister(policy->cdev); >> - policy->cdev = NULL; >> - } >> + if (cpufreq_thermal_control_enabled(cpufreq_driver)) >> + cpufreq_cooling_unregister(policy); > > And we unregister unconditionally, even if we failed ? What if this > routine prints error messages for such an case ? >> >> if (cpufreq_driver->stop_cpu) >> cpufreq_driver->stop_cpu(policy); >> 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; >> + } >> + } > > What if we reach here without a match for the policy ? We shouldn't > continue and error out, right ? Print an error message as well ? > -- <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs Follow Linaro: <http://www.facebook.com/pages/Linaro> Facebook | <http://twitter.com/#!/linaroorg> Twitter | <http://www.linaro.org/linaro-blog/> Blog ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH V3 2/3] thermal/drivers/cpu_cooling: Unregister with the policy 2019-06-26 6:02 ` Daniel Lezcano @ 2019-06-26 6:37 ` Viresh Kumar 2019-06-26 9:06 ` Rafael J. Wysocki 0 siblings, 1 reply; 11+ messages in thread From: Viresh Kumar @ 2019-06-26 6:37 UTC (permalink / raw) To: Daniel Lezcano Cc: rjw, edubezval, linux-kernel, Sudeep Holla, Amit Daniel Kachhap, Javi Merino, Zhang Rui, Shawn Guo, Sascha Hauer, Pengutronix Kernel Team, Fabio Estevam, NXP Linux Team, Keerthy, open list:CPU FREQUENCY DRIVERS - ARM BIG LITTLE, moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE, open list:TI BANDGAP AND THERMAL DRIVER On 26-06-19, 08:02, Daniel Lezcano wrote: > On 26/06/2019 04:58, Viresh Kumar wrote: > > On 25-06-19, 13:32, Daniel Lezcano wrote: > >> diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c > >> index aee024e42618..f07454249fbc 100644 > >> --- a/drivers/cpufreq/cpufreq.c > >> +++ b/drivers/cpufreq/cpufreq.c > >> @@ -1379,8 +1379,8 @@ static int cpufreq_online(unsigned int cpu) > >> cpufreq_driver->ready(policy); > >> > >> if (cpufreq_thermal_control_enabled(cpufreq_driver)) > >> - policy->cdev = of_cpufreq_cooling_register(policy); > >> - > >> + of_cpufreq_cooling_register(policy); > >> + > > > > We don't need any error checking here anymore ? > > There was no error checking initially. This comment and the others below > are for an additional patch IMO, not a change in this one. right, but ... > >> -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; we used to return without any errors from here. Now we will have problems if regsitering fails for some reason. -- viresh ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH V3 2/3] thermal/drivers/cpu_cooling: Unregister with the policy 2019-06-26 6:37 ` Viresh Kumar @ 2019-06-26 9:06 ` Rafael J. Wysocki 2019-06-26 10:19 ` Daniel Lezcano 0 siblings, 1 reply; 11+ messages in thread From: Rafael J. Wysocki @ 2019-06-26 9:06 UTC (permalink / raw) To: Viresh Kumar Cc: Daniel Lezcano, Rafael J. Wysocki, Eduardo Valentin, Linux Kernel Mailing List, Sudeep Holla, Amit Daniel Kachhap, Javi Merino, Zhang Rui, Shawn Guo, Sascha Hauer, Pengutronix Kernel Team, Fabio Estevam, NXP Linux Team, Keerthy, open list:CPU FREQUENCY DRIVERS - ARM BIG LITTLE, moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE, open list:TI BANDGAP AND THERMAL DRIVER On Wed, Jun 26, 2019 at 8:37 AM Viresh Kumar <viresh.kumar@linaro.org> wrote: > > On 26-06-19, 08:02, Daniel Lezcano wrote: > > On 26/06/2019 04:58, Viresh Kumar wrote: > > > On 25-06-19, 13:32, Daniel Lezcano wrote: > > >> diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c > > >> index aee024e42618..f07454249fbc 100644 > > >> --- a/drivers/cpufreq/cpufreq.c > > >> +++ b/drivers/cpufreq/cpufreq.c > > >> @@ -1379,8 +1379,8 @@ static int cpufreq_online(unsigned int cpu) > > >> cpufreq_driver->ready(policy); > > >> > > >> if (cpufreq_thermal_control_enabled(cpufreq_driver)) > > >> - policy->cdev = of_cpufreq_cooling_register(policy); > > >> - > > >> + of_cpufreq_cooling_register(policy); > > >> + > > > > > > We don't need any error checking here anymore ? > > > > There was no error checking initially. This comment and the others below > > are for an additional patch IMO, not a change in this one. > > right, but ... > > > >> -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; > > we used to return without any errors from here. Now we will have > problems if regsitering fails for some reason. Specifically, the last cpufreq_cdev in the list will be unregistered AFAICS, and without removing it from the list for that matter, which isn't what the caller wants. ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH V3 2/3] thermal/drivers/cpu_cooling: Unregister with the policy 2019-06-26 9:06 ` Rafael J. Wysocki @ 2019-06-26 10:19 ` Daniel Lezcano 2019-06-26 11:28 ` Rafael J. Wysocki 0 siblings, 1 reply; 11+ messages in thread From: Daniel Lezcano @ 2019-06-26 10:19 UTC (permalink / raw) To: Rafael J. Wysocki, Viresh Kumar Cc: Rafael J. Wysocki, Eduardo Valentin, Linux Kernel Mailing List, Sudeep Holla, Amit Daniel Kachhap, Javi Merino, Zhang Rui, Shawn Guo, Sascha Hauer, Pengutronix Kernel Team, Fabio Estevam, NXP Linux Team, Keerthy, open list:CPU FREQUENCY DRIVERS - ARM BIG LITTLE, moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE, open list:TI BANDGAP AND THERMAL DRIVER On 26/06/2019 11:06, Rafael J. Wysocki wrote: > On Wed, Jun 26, 2019 at 8:37 AM Viresh Kumar <viresh.kumar@linaro.org> wrote: >> >> On 26-06-19, 08:02, Daniel Lezcano wrote: >>> On 26/06/2019 04:58, Viresh Kumar wrote: >>>> On 25-06-19, 13:32, Daniel Lezcano wrote: >>>>> diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c >>>>> index aee024e42618..f07454249fbc 100644 >>>>> --- a/drivers/cpufreq/cpufreq.c >>>>> +++ b/drivers/cpufreq/cpufreq.c >>>>> @@ -1379,8 +1379,8 @@ static int cpufreq_online(unsigned int cpu) >>>>> cpufreq_driver->ready(policy); >>>>> >>>>> if (cpufreq_thermal_control_enabled(cpufreq_driver)) >>>>> - policy->cdev = of_cpufreq_cooling_register(policy); >>>>> - >>>>> + of_cpufreq_cooling_register(policy); >>>>> + >>>> >>>> We don't need any error checking here anymore ? >>> >>> There was no error checking initially. This comment and the others below >>> are for an additional patch IMO, not a change in this one. >> >> right, but ... >> >>>>> -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; >> >> we used to return without any errors from here. Now we will have >> problems if regsitering fails for some reason. > > Specifically, the last cpufreq_cdev in the list will be unregistered > AFAICS, and without removing it from the list for that matter, which > isn't what the caller wants. Indeed, What about the resulting code above: void __cpufreq_cooling_unregister(struct cpufreq_cooling_device *cpufreq_cdev, int last) { /* Unregister the notifier for the last cpufreq cooling device */ if (last) cpufreq_unregister_notifier(&thermal_cpufreq_notifier_block, CPUFREQ_POLICY_NOTIFIER); thermal_cooling_device_unregister(cpufreq_cdev->cdev); ida_simple_remove(&cpufreq_ida, cpufreq_cdev->id); kfree(cpufreq_cdev->idle_time); kfree(cpufreq_cdev); } /** * cpufreq_cooling_unregister - function to remove cpufreq cooling device. * @cdev: thermal cooling device pointer. * * This interface function unregisters the "thermal-cpufreq-%x" cooling device. */ void cpufreq_cooling_unregister(struct cpufreq_policy *policy) { struct cpufreq_cooling_device *cpufreq_cdev; bool last; mutex_lock(&cooling_list_lock); list_for_each_entry(cpufreq_cdev, &cpufreq_cdev_list, node) { if (cpufreq_cdev->policy == policy) { list_del(&cpufreq_cdev->node); last = list_empty(&cpufreq_cdev_list); break; } } mutex_unlock(&cooling_list_lock); if (cpufreq_cdev->policy == policy) __cpufreq_cooling_unregister(cpufreq_cdev, last); } EXPORT_SYMBOL_GPL(cpufreq_cooling_unregister); -- <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs Follow Linaro: <http://www.facebook.com/pages/Linaro> Facebook | <http://twitter.com/#!/linaroorg> Twitter | <http://www.linaro.org/linaro-blog/> Blog ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH V3 2/3] thermal/drivers/cpu_cooling: Unregister with the policy 2019-06-26 10:19 ` Daniel Lezcano @ 2019-06-26 11:28 ` Rafael J. Wysocki 2019-06-26 12:52 ` Daniel Lezcano 0 siblings, 1 reply; 11+ messages in thread From: Rafael J. Wysocki @ 2019-06-26 11:28 UTC (permalink / raw) To: Daniel Lezcano Cc: Rafael J. Wysocki, Viresh Kumar, Rafael J. Wysocki, Eduardo Valentin, Linux Kernel Mailing List, Sudeep Holla, Amit Daniel Kachhap, Javi Merino, Zhang Rui, Shawn Guo, Sascha Hauer, Pengutronix Kernel Team, Fabio Estevam, NXP Linux Team, Keerthy, open list:CPU FREQUENCY DRIVERS - ARM BIG LITTLE, moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE, open list:TI BANDGAP AND THERMAL DRIVER On Wed, Jun 26, 2019 at 12:19 PM Daniel Lezcano <daniel.lezcano@linaro.org> wrote: > > On 26/06/2019 11:06, Rafael J. Wysocki wrote: > > On Wed, Jun 26, 2019 at 8:37 AM Viresh Kumar <viresh.kumar@linaro.org> wrote: > >> > >> On 26-06-19, 08:02, Daniel Lezcano wrote: > >>> On 26/06/2019 04:58, Viresh Kumar wrote: > >>>> On 25-06-19, 13:32, Daniel Lezcano wrote: > >>>>> diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c > >>>>> index aee024e42618..f07454249fbc 100644 > >>>>> --- a/drivers/cpufreq/cpufreq.c > >>>>> +++ b/drivers/cpufreq/cpufreq.c > >>>>> @@ -1379,8 +1379,8 @@ static int cpufreq_online(unsigned int cpu) > >>>>> cpufreq_driver->ready(policy); > >>>>> > >>>>> if (cpufreq_thermal_control_enabled(cpufreq_driver)) > >>>>> - policy->cdev = of_cpufreq_cooling_register(policy); > >>>>> - > >>>>> + of_cpufreq_cooling_register(policy); > >>>>> + > >>>> > >>>> We don't need any error checking here anymore ? > >>> > >>> There was no error checking initially. This comment and the others below > >>> are for an additional patch IMO, not a change in this one. > >> > >> right, but ... > >> > >>>>> -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; > >> > >> we used to return without any errors from here. Now we will have > >> problems if regsitering fails for some reason. > > > > Specifically, the last cpufreq_cdev in the list will be unregistered > > AFAICS, and without removing it from the list for that matter, which > > isn't what the caller wants. > > Indeed, > > What about the resulting code above: > > void __cpufreq_cooling_unregister(struct cpufreq_cooling_device > *cpufreq_cdev, int last) > { > /* Unregister the notifier for the last cpufreq cooling device */ > if (last) > cpufreq_unregister_notifier(&thermal_cpufreq_notifier_block, > CPUFREQ_POLICY_NOTIFIER); > Doesn't the notifier need to be unregistered under cooling_list_lock ? > thermal_cooling_device_unregister(cpufreq_cdev->cdev); > ida_simple_remove(&cpufreq_ida, cpufreq_cdev->id); > kfree(cpufreq_cdev->idle_time); > kfree(cpufreq_cdev); > } > > /** > > * cpufreq_cooling_unregister - function to remove cpufreq cooling > device. > * @cdev: thermal cooling device pointer. > > * > > * This interface function unregisters the "thermal-cpufreq-%x" cooling > device. > */ > void cpufreq_cooling_unregister(struct cpufreq_policy *policy) > { > struct cpufreq_cooling_device *cpufreq_cdev; > bool last; > > mutex_lock(&cooling_list_lock); > list_for_each_entry(cpufreq_cdev, &cpufreq_cdev_list, node) { > if (cpufreq_cdev->policy == policy) { > list_del(&cpufreq_cdev->node); > last = list_empty(&cpufreq_cdev_list); > break; > } > } > mutex_unlock(&cooling_list_lock); > > if (cpufreq_cdev->policy == policy) > __cpufreq_cooling_unregister(cpufreq_cdev, last); > } > EXPORT_SYMBOL_GPL(cpufreq_cooling_unregister); > > > > > -- > <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs > > Follow Linaro: <http://www.facebook.com/pages/Linaro> Facebook | > <http://twitter.com/#!/linaroorg> Twitter | > <http://www.linaro.org/linaro-blog/> Blog > ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH V3 2/3] thermal/drivers/cpu_cooling: Unregister with the policy 2019-06-26 11:28 ` Rafael J. Wysocki @ 2019-06-26 12:52 ` Daniel Lezcano 0 siblings, 0 replies; 11+ messages in thread From: Daniel Lezcano @ 2019-06-26 12:52 UTC (permalink / raw) To: Rafael J. Wysocki Cc: Viresh Kumar, Rafael J. Wysocki, Eduardo Valentin, Linux Kernel Mailing List, Sudeep Holla, Amit Daniel Kachhap, Javi Merino, Zhang Rui, Shawn Guo, Sascha Hauer, Pengutronix Kernel Team, Fabio Estevam, NXP Linux Team, Keerthy, open list:CPU FREQUENCY DRIVERS - ARM BIG LITTLE, moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE, open list:TI BANDGAP AND THERMAL DRIVER On 26/06/2019 13:28, Rafael J. Wysocki wrote: > On Wed, Jun 26, 2019 at 12:19 PM Daniel Lezcano > <daniel.lezcano@linaro.org> wrote: >> >> On 26/06/2019 11:06, Rafael J. Wysocki wrote: >>> On Wed, Jun 26, 2019 at 8:37 AM Viresh Kumar <viresh.kumar@linaro.org> wrote: >>>> >>>> On 26-06-19, 08:02, Daniel Lezcano wrote: >>>>> On 26/06/2019 04:58, Viresh Kumar wrote: >>>>>> On 25-06-19, 13:32, Daniel Lezcano wrote: >>>>>>> diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c >>>>>>> index aee024e42618..f07454249fbc 100644 >>>>>>> --- a/drivers/cpufreq/cpufreq.c >>>>>>> +++ b/drivers/cpufreq/cpufreq.c >>>>>>> @@ -1379,8 +1379,8 @@ static int cpufreq_online(unsigned int cpu) >>>>>>> cpufreq_driver->ready(policy); >>>>>>> >>>>>>> if (cpufreq_thermal_control_enabled(cpufreq_driver)) >>>>>>> - policy->cdev = of_cpufreq_cooling_register(policy); >>>>>>> - >>>>>>> + of_cpufreq_cooling_register(policy); >>>>>>> + >>>>>> >>>>>> We don't need any error checking here anymore ? >>>>> >>>>> There was no error checking initially. This comment and the others below >>>>> are for an additional patch IMO, not a change in this one. >>>> >>>> right, but ... >>>> >>>>>>> -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; >>>> >>>> we used to return without any errors from here. Now we will have >>>> problems if regsitering fails for some reason. >>> >>> Specifically, the last cpufreq_cdev in the list will be unregistered >>> AFAICS, and without removing it from the list for that matter, which >>> isn't what the caller wants. >> >> Indeed, >> >> What about the resulting code above: >> >> void __cpufreq_cooling_unregister(struct cpufreq_cooling_device >> *cpufreq_cdev, int last) >> { >> /* Unregister the notifier for the last cpufreq cooling device */ >> if (last) >> cpufreq_unregister_notifier(&thermal_cpufreq_notifier_block, >> CPUFREQ_POLICY_NOTIFIER); >> > > Doesn't the notifier need to be unregistered under cooling_list_lock ? I don't think so because the element is no longer in the list and we don't touch the list anymore. Do you see another possible race? >> thermal_cooling_device_unregister(cpufreq_cdev->cdev); >> ida_simple_remove(&cpufreq_ida, cpufreq_cdev->id); >> kfree(cpufreq_cdev->idle_time); >> kfree(cpufreq_cdev); >> } >> >> /** >> >> * cpufreq_cooling_unregister - function to remove cpufreq cooling >> device. >> * @cdev: thermal cooling device pointer. >> >> * >> >> * This interface function unregisters the "thermal-cpufreq-%x" cooling >> device. >> */ >> void cpufreq_cooling_unregister(struct cpufreq_policy *policy) >> { >> struct cpufreq_cooling_device *cpufreq_cdev; >> bool last; >> >> mutex_lock(&cooling_list_lock); >> list_for_each_entry(cpufreq_cdev, &cpufreq_cdev_list, node) { >> if (cpufreq_cdev->policy == policy) { >> list_del(&cpufreq_cdev->node); >> last = list_empty(&cpufreq_cdev_list); >> break; >> } >> } >> mutex_unlock(&cooling_list_lock); >> >> if (cpufreq_cdev->policy == policy) >> __cpufreq_cooling_unregister(cpufreq_cdev, last); >> } >> EXPORT_SYMBOL_GPL(cpufreq_cooling_unregister); >> >> >> >> >> -- >> <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs >> >> Follow Linaro: <http://www.facebook.com/pages/Linaro> Facebook | >> <http://twitter.com/#!/linaroorg> Twitter | >> <http://www.linaro.org/linaro-blog/> Blog >> -- <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs Follow Linaro: <http://www.facebook.com/pages/Linaro> Facebook | <http://twitter.com/#!/linaroorg> Twitter | <http://www.linaro.org/linaro-blog/> Blog ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH V3 3/3] thermal/drivers/cpu_cooling: cpufreq_cooling_register returns an int 2019-06-25 11:32 [PATCH V3 1/3] cpufreq: Move the IS_ENABLED(CPU_THERMAL) macro in a stub Daniel Lezcano 2019-06-25 11:32 ` [PATCH V3 2/3] thermal/drivers/cpu_cooling: Unregister with the policy Daniel Lezcano @ 2019-06-25 11:32 ` Daniel Lezcano 2019-06-26 2:59 ` Viresh Kumar 1 sibling, 1 reply; 11+ messages in thread From: Daniel Lezcano @ 2019-06-25 11:32 UTC (permalink / raw) To: viresh.kumar Cc: rjw, edubezval, linux-kernel, Amit Daniel Kachhap, Javi Merino, Zhang Rui, Shawn Guo, Sascha Hauer, Pengutronix Kernel Team, Fabio Estevam, NXP Linux Team, Keerthy, open list:THERMAL/CPU_COOLING, moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE, open list:TI BANDGAP AND THERMAL DRIVER It looks like after the changes in the patch the only reason for returning (struct thermal_cooling_device *) from cpufreq_cooling_register() is error checking, but it would be much more straightforward to return int for this purpose. Moreover, that would prevent the callers of it from doing incorrect things with the returned pointers (like using it to unregister the cooling device). Replace the returned value an integer instead of a pointer to a thermal cooling device structure. Suggested-by: Rafael J. Wysocki <rafael@kernel.org> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org> --- drivers/thermal/cpu_cooling.c | 63 +++++++++---------- drivers/thermal/imx_thermal.c | 6 +- .../ti-soc-thermal/ti-thermal-common.c | 7 +-- include/linux/cpu_cooling.h | 16 ++--- 4 files changed, 40 insertions(+), 52 deletions(-) diff --git a/drivers/thermal/cpu_cooling.c b/drivers/thermal/cpu_cooling.c index 007c7c6bf845..f5fa31a57658 100644 --- a/drivers/thermal/cpu_cooling.c +++ b/drivers/thermal/cpu_cooling.c @@ -530,13 +530,12 @@ static struct notifier_block thermal_cpufreq_notifier_block = { * cooling devices. It also gives the opportunity to link the cooling device * with a device tree node, in order to bind it via the thermal DT code. * - * Return: a valid struct thermal_cooling_device pointer on success, - * on failure, it returns a corresponding ERR_PTR(). + * Return: zero on success, less than zero corresponding to the + * negative error code. */ -static struct thermal_cooling_device * -__cpufreq_cooling_register(struct device_node *np, - struct cpufreq_policy *policy, - struct em_perf_domain *em) +static int __cpufreq_cooling_register(struct device_node *np, + struct cpufreq_policy *policy, + struct em_perf_domain *em) { struct thermal_cooling_device *cdev; struct cpufreq_cooling_device *cpufreq_cdev; @@ -548,19 +547,19 @@ __cpufreq_cooling_register(struct device_node *np, if (IS_ERR_OR_NULL(policy)) { pr_err("%s: cpufreq policy isn't valid: %p\n", __func__, policy); - return ERR_PTR(-EINVAL); + return -EINVAL; } i = cpufreq_table_count_valid_entries(policy); if (!i) { pr_debug("%s: CPUFreq table not found or has no valid entries\n", __func__); - return ERR_PTR(-ENODEV); + return -ENODEV; } cpufreq_cdev = kzalloc(sizeof(*cpufreq_cdev), GFP_KERNEL); if (!cpufreq_cdev) - return ERR_PTR(-ENOMEM); + return -ENOMEM; cpufreq_cdev->policy = policy; num_cpus = cpumask_weight(policy->related_cpus); @@ -568,7 +567,7 @@ __cpufreq_cooling_register(struct device_node *np, sizeof(*cpufreq_cdev->idle_time), GFP_KERNEL); if (!cpufreq_cdev->idle_time) { - cdev = ERR_PTR(-ENOMEM); + ret = -ENOMEM; goto free_cdev; } @@ -576,10 +575,8 @@ __cpufreq_cooling_register(struct device_node *np, cpufreq_cdev->max_level = i - 1; ret = ida_simple_get(&cpufreq_ida, 0, 0, GFP_KERNEL); - if (ret < 0) { - cdev = ERR_PTR(ret); + if (ret < 0) goto free_idle_time; - } cpufreq_cdev->id = ret; snprintf(dev_name, sizeof(dev_name), "thermal-cpufreq-%d", @@ -597,14 +594,16 @@ __cpufreq_cooling_register(struct device_node *np, if (policy->freq_table_sorted == CPUFREQ_TABLE_UNSORTED) { pr_err("%s: unsorted frequency tables are not supported\n", __func__); - cdev = ERR_PTR(-EINVAL); + ret = -EINVAL; goto remove_ida; } cdev = thermal_of_cooling_device_register(np, dev_name, cpufreq_cdev, cooling_ops); - if (IS_ERR(cdev)) + if (IS_ERR(cdev)) { + ret = PTR_ERR(cdev); goto remove_ida; + } cpufreq_cdev->clipped_freq = get_state_freq(cpufreq_cdev, 0); cpufreq_cdev->cdev = cdev; @@ -619,7 +618,7 @@ __cpufreq_cooling_register(struct device_node *np, cpufreq_register_notifier(&thermal_cpufreq_notifier_block, CPUFREQ_POLICY_NOTIFIER); - return cdev; + return 0; remove_ida: ida_simple_remove(&cpufreq_ida, cpufreq_cdev->id); @@ -627,7 +626,7 @@ __cpufreq_cooling_register(struct device_node *np, kfree(cpufreq_cdev->idle_time); free_cdev: kfree(cpufreq_cdev); - return cdev; + return ret; } /** @@ -638,11 +637,10 @@ __cpufreq_cooling_register(struct device_node *np, * "thermal-cpufreq-%x". This api can support multiple instances of cpufreq * cooling devices. * - * Return: a valid struct thermal_cooling_device pointer on success, - * on failure, it returns a corresponding ERR_PTR(). + * Return: zero on success, less than zero corresponding to the + * negative error code. */ -struct thermal_cooling_device * -cpufreq_cooling_register(struct cpufreq_policy *policy) +int cpufreq_cooling_register(struct cpufreq_policy *policy) { return __cpufreq_cooling_register(NULL, policy, NULL); } @@ -664,34 +662,31 @@ EXPORT_SYMBOL_GPL(cpufreq_cooling_register); * It also takes into account, if property present in policy CPU node, the * static power consumed by the cpu. * - * Return: a valid struct thermal_cooling_device pointer on success, - * and NULL on failure. + * Return: zero on success, less than zero corresponding to the + * negative error code. */ -struct thermal_cooling_device * -of_cpufreq_cooling_register(struct cpufreq_policy *policy) +int of_cpufreq_cooling_register(struct cpufreq_policy *policy) { struct device_node *np = of_get_cpu_node(policy->cpu, NULL); - struct thermal_cooling_device *cdev = NULL; + int ret = -EINVAL; if (!np) { pr_err("cpu_cooling: OF node not available for cpu%d\n", policy->cpu); - return NULL; + return -EINVAL; } if (of_find_property(np, "#cooling-cells", NULL)) { struct em_perf_domain *em = em_cpu_get(policy->cpu); - cdev = __cpufreq_cooling_register(np, policy, em); - if (IS_ERR(cdev)) { - pr_err("cpu_cooling: cpu%d failed to register as cooling device: %ld\n", - policy->cpu, PTR_ERR(cdev)); - cdev = NULL; - } + ret = __cpufreq_cooling_register(np, policy, em); + if (ret) + pr_err("cpu_cooling: cpu%d failed to register as cooling device: %d\n", + policy->cpu, ret); } of_node_put(np); - return cdev; + return ret; } EXPORT_SYMBOL_GPL(of_cpufreq_cooling_register); diff --git a/drivers/thermal/imx_thermal.c b/drivers/thermal/imx_thermal.c index 021c0948b740..1c4b49b583bc 100644 --- a/drivers/thermal/imx_thermal.c +++ b/drivers/thermal/imx_thermal.c @@ -655,7 +655,6 @@ MODULE_DEVICE_TABLE(of, of_imx_thermal_match); static int imx_thermal_register_legacy_cooling(struct imx_thermal_data *data) { struct device_node *np; - struct thermal_cooling_device *cdev; int ret; data->policy = cpufreq_cpu_get(0); @@ -667,9 +666,8 @@ static int imx_thermal_register_legacy_cooling(struct imx_thermal_data *data) np = of_get_cpu_node(data->policy->cpu, NULL); if (!np || !of_find_property(np, "#cooling-cells", NULL)) { - cdev = cpufreq_cooling_register(data->policy); - if (IS_ERR(cdev)) { - ret = PTR_ERR(cdev); + ret = cpufreq_cooling_register(data->policy); + if (ret) { cpufreq_cpu_put(data->policy); return ret; } diff --git a/drivers/thermal/ti-soc-thermal/ti-thermal-common.c b/drivers/thermal/ti-soc-thermal/ti-thermal-common.c index 170b70b6ec61..eacc46d7bd1c 100644 --- a/drivers/thermal/ti-soc-thermal/ti-thermal-common.c +++ b/drivers/thermal/ti-soc-thermal/ti-thermal-common.c @@ -232,7 +232,7 @@ int ti_thermal_register_cpu_cooling(struct ti_bandgap *bgp, int id) { struct ti_thermal_data *data; struct device_node *np = bgp->dev->of_node; - struct thermal_cooling_device *cdev; + int ret; /* * We are assuming here that if one deploys the zone @@ -256,9 +256,8 @@ int ti_thermal_register_cpu_cooling(struct ti_bandgap *bgp, int id) } /* Register cooling device */ - cdev = cpufreq_cooling_register(data->policy); - if (IS_ERR(cdev)) { - int ret = PTR_ERR(cdev); + ret = cpufreq_cooling_register(data->policy); + if (ret) { dev_err(bgp->dev, "Failed to register cpu cooling device %d\n", ret); cpufreq_cpu_put(data->policy); diff --git a/include/linux/cpu_cooling.h b/include/linux/cpu_cooling.h index 89f469ee4be4..98f7c8a9cab6 100644 --- a/include/linux/cpu_cooling.h +++ b/include/linux/cpu_cooling.h @@ -24,8 +24,7 @@ struct cpufreq_policy; * cpufreq_cooling_register - function to create cpufreq cooling device. * @policy: cpufreq policy. */ -struct thermal_cooling_device * -cpufreq_cooling_register(struct cpufreq_policy *policy); +int cpufreq_cooling_register(struct cpufreq_policy *policy); /** * cpufreq_cooling_unregister - function to remove cpufreq cooling device. @@ -34,10 +33,9 @@ cpufreq_cooling_register(struct cpufreq_policy *policy); void cpufreq_cooling_unregister(struct cpufreq_policy *policy); #else /* !CONFIG_CPU_THERMAL */ -static inline struct thermal_cooling_device * -cpufreq_cooling_register(struct cpufreq_policy *policy) +static inline int cpufreq_cooling_register(struct cpufreq_policy *policy) { - return ERR_PTR(-ENOSYS); + return -ENOSYS; } static inline @@ -52,13 +50,11 @@ void cpufreq_cooling_unregister(struct cpufreq_policy *policy) * of_cpufreq_cooling_register - create cpufreq cooling device based on DT. * @policy: cpufreq policy. */ -struct thermal_cooling_device * -of_cpufreq_cooling_register(struct cpufreq_policy *policy); +int of_cpufreq_cooling_register(struct cpufreq_policy *policy); #else -static inline struct thermal_cooling_device * -of_cpufreq_cooling_register(struct cpufreq_policy *policy) +static inline int of_cpufreq_cooling_register(struct cpufreq_policy *policy) { - return NULL; + return -ENOSYS; } #endif /* defined(CONFIG_THERMAL_OF) && defined(CONFIG_CPU_THERMAL) */ -- 2.17.1 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH V3 3/3] thermal/drivers/cpu_cooling: cpufreq_cooling_register returns an int 2019-06-25 11:32 ` [PATCH V3 3/3] thermal/drivers/cpu_cooling: cpufreq_cooling_register returns an int Daniel Lezcano @ 2019-06-26 2:59 ` Viresh Kumar 0 siblings, 0 replies; 11+ messages in thread From: Viresh Kumar @ 2019-06-26 2:59 UTC (permalink / raw) To: Daniel Lezcano Cc: rjw, edubezval, linux-kernel, Amit Daniel Kachhap, Javi Merino, Zhang Rui, Shawn Guo, Sascha Hauer, Pengutronix Kernel Team, Fabio Estevam, NXP Linux Team, Keerthy, open list:THERMAL/CPU_COOLING, moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE, open list:TI BANDGAP AND THERMAL DRIVER On 25-06-19, 13:32, Daniel Lezcano wrote: > It looks like after the changes in the patch the only reason for which patch ? Instead of "patch" you should refer to the latest changes in code, like "now that cpufreq_cooling_unregister() doesn't accept cdev as an argument ....." > returning (struct thermal_cooling_device *) from > cpufreq_cooling_register() is error checking, but it would be much > more straightforward to return int for this purpose. > > Moreover, that would prevent the callers of it from doing incorrect > things with the returned pointers (like using it to unregister the > cooling device). > > Replace the returned value an integer instead of a pointer to a as ? > thermal cooling device structure. > > Suggested-by: Rafael J. Wysocki <rafael@kernel.org> > Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org> > --- > drivers/thermal/cpu_cooling.c | 63 +++++++++---------- > drivers/thermal/imx_thermal.c | 6 +- > .../ti-soc-thermal/ti-thermal-common.c | 7 +-- > include/linux/cpu_cooling.h | 16 ++--- > 4 files changed, 40 insertions(+), 52 deletions(-) -- viresh ^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2019-06-26 12:52 UTC | newest] Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-06-25 11:32 [PATCH V3 1/3] cpufreq: Move the IS_ENABLED(CPU_THERMAL) macro in a stub Daniel Lezcano 2019-06-25 11:32 ` [PATCH V3 2/3] thermal/drivers/cpu_cooling: Unregister with the policy Daniel Lezcano 2019-06-26 2:58 ` Viresh Kumar 2019-06-26 6:02 ` Daniel Lezcano 2019-06-26 6:37 ` Viresh Kumar 2019-06-26 9:06 ` Rafael J. Wysocki 2019-06-26 10:19 ` Daniel Lezcano 2019-06-26 11:28 ` Rafael J. Wysocki 2019-06-26 12:52 ` Daniel Lezcano 2019-06-25 11:32 ` [PATCH V3 3/3] thermal/drivers/cpu_cooling: cpufreq_cooling_register returns an int Daniel Lezcano 2019-06-26 2:59 ` Viresh Kumar
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).