diff mbox series

[v1,02/10] cpufreq: Add a flag to auto-register a cooling device

Message ID c1e460c49ad2f16b15ace909514a770c9753d1b1.1547481320.git.amit.kucheria@linaro.org
State New
Headers show
Series [v1,01/10] cpufreq: Add thermal_cooling_device pointer to struct cpufreq_policy | expand

Commit Message

Amit Kucheria Jan. 14, 2019, 4:34 p.m. UTC 
All cpufreq drivers do similar things to register as a cooling device.
Provide a cpufreq driver flag so drivers can just ask the cpufreq core
to register the cooling device on their behalf. This allows us to get
rid of duplicated code in the drivers.

Suggested-by: Stephen Boyd <swboyd@chromium.org>
Suggested-by: Viresh Kumar <viresh.kumar@linaro.org>
Signed-off-by: Amit Kucheria <amit.kucheria@linaro.org>

---
 drivers/cpufreq/cpufreq.c | 17 +++++++++++++++++
 include/linux/cpufreq.h   |  6 ++++++
 2 files changed, 23 insertions(+)

-- 
2.17.1

Comments

Matthias Kaehlcke Jan. 14, 2019, 8:51 p.m. UTC | #1 
On Mon, Jan 14, 2019 at 10:04:54PM +0530, Amit Kucheria wrote:
> All cpufreq drivers do similar things to register as a cooling device.

> Provide a cpufreq driver flag so drivers can just ask the cpufreq core

> to register the cooling device on their behalf. This allows us to get

> rid of duplicated code in the drivers.

> 

> Suggested-by: Stephen Boyd <swboyd@chromium.org>

> Suggested-by: Viresh Kumar <viresh.kumar@linaro.org>

> Signed-off-by: Amit Kucheria <amit.kucheria@linaro.org>

> ---

>  drivers/cpufreq/cpufreq.c | 17 +++++++++++++++++

>  include/linux/cpufreq.h   |  6 ++++++

>  2 files changed, 23 insertions(+)

> 

> diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c

> index 6f23ebb395f1..cd6e750d3d82 100644

> --- a/drivers/cpufreq/cpufreq.c

> +++ b/drivers/cpufreq/cpufreq.c

> @@ -30,6 +30,7 @@

>  #include <linux/syscore_ops.h>

>  #include <linux/tick.h>

>  #include <trace/events/power.h>

> +#include <linux/cpu_cooling.h>

>  

>  static LIST_HEAD(cpufreq_policy_list);

>  

> @@ -1318,6 +1319,14 @@ static int cpufreq_online(unsigned int cpu)

>  	if (cpufreq_driver->ready)

>  		cpufreq_driver->ready(policy);

>  

> +#ifdef CONFIG_CPU_THERMAL

> +	if (cpufreq_driver->flags & CPUFREQ_AUTO_REGISTER_COOLING_DEV) {

> +		struct thermal_cooling_device **cdev = &policy->cooldev;

> +

> +		*cdev = of_cpufreq_cooling_register(policy);

> +	}

> +#endif

> +

>  	pr_debug("initialization complete\n");

>  

>  	return 0;

> @@ -1411,6 +1420,14 @@ static int cpufreq_offline(unsigned int cpu)

>  	if (has_target())

>  		cpufreq_exit_governor(policy);

>  

> +#ifdef CONFIG_CPU_THERMAL

> +	if (cpufreq_driver->flags & CPUFREQ_AUTO_REGISTER_COOLING_DEV) {

> +		struct thermal_cooling_device **cdev = &policy->cooldev;

> +

> +		cpufreq_cooling_unregister(*cdev);

> +	}

> +#endif

> +

>  	/*

>  	 * Perform the ->exit() even during light-weight tear-down,

>  	 * since this is a core component, and is essential for the

> diff --git a/include/linux/cpufreq.h b/include/linux/cpufreq.h

> index 7d0cf54125fa..70ad02088825 100644

> --- a/include/linux/cpufreq.h

> +++ b/include/linux/cpufreq.h

> @@ -390,6 +390,12 @@ struct cpufreq_driver {

>   */

>  #define CPUFREQ_NO_AUTO_DYNAMIC_SWITCHING (1 << 6)

>  

> +/*

> + * Set by drivers that want the core to automatically register the cpufreq

> + * driver as a thermal cooling device

> + */

> +#define CPUFREQ_AUTO_REGISTER_COOLING_DEV (1 << 7)

> +

>  int cpufreq_register_driver(struct cpufreq_driver *driver_data);

>  int cpufreq_unregister_driver(struct cpufreq_driver *driver_data);


Reviewed-by: Matthias Kaehlcke <mka@chromium.org>

Tested-by: Matthias Kaehlcke <mka@chromium.org>
Viresh Kumar Jan. 17, 2019, 4:42 a.m. UTC | #2 
On 17-01-19, 00:03, Rafael J. Wysocki wrote:
> On Monday, January 14, 2019 5:34:54 PM CET Amit Kucheria wrote:

> > All cpufreq drivers do similar things to register as a cooling device.

> > Provide a cpufreq driver flag so drivers can just ask the cpufreq core

> > to register the cooling device on their behalf. This allows us to get

> > rid of duplicated code in the drivers.

> > 

> > Suggested-by: Stephen Boyd <swboyd@chromium.org>

> > Suggested-by: Viresh Kumar <viresh.kumar@linaro.org>

> > Signed-off-by: Amit Kucheria <amit.kucheria@linaro.org>

> > ---

> >  drivers/cpufreq/cpufreq.c | 17 +++++++++++++++++

> >  include/linux/cpufreq.h   |  6 ++++++

> >  2 files changed, 23 insertions(+)

> > 

> > diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c

> > index 6f23ebb395f1..cd6e750d3d82 100644

> > --- a/drivers/cpufreq/cpufreq.c

> > +++ b/drivers/cpufreq/cpufreq.c

> > @@ -30,6 +30,7 @@

> >  #include <linux/syscore_ops.h>

> >  #include <linux/tick.h>

> >  #include <trace/events/power.h>

> > +#include <linux/cpu_cooling.h>

> >  

> >  static LIST_HEAD(cpufreq_policy_list);

> >  

> > @@ -1318,6 +1319,14 @@ static int cpufreq_online(unsigned int cpu)

> >  	if (cpufreq_driver->ready)

> >  		cpufreq_driver->ready(policy);

> >  

> > +#ifdef CONFIG_CPU_THERMAL

> > +	if (cpufreq_driver->flags & CPUFREQ_AUTO_REGISTER_COOLING_DEV) {

> > +		struct thermal_cooling_device **cdev = &policy->cooldev;


We use cdev for the cooling device everywhere in the kernel, so please
do s/cooldev/cdev/ in your patches.

> > +

> > +		*cdev = of_cpufreq_cooling_register(policy);

> 

> What would be wrong with

> 

> 		policy->cooldev = of_cpufreq_cooling_register(policy);

> 

> > +	}

> > +#endif

> 

> Please remove the #ifdefs from cpufreq_online() and cpufreq_offline().

> 

> Use wrappers that would become empty stubs for CONFIG_CPU_THERMAL unset.

> 

> > +

> >  	pr_debug("initialization complete\n");

> >  

> >  	return 0;

> > @@ -1411,6 +1420,14 @@ static int cpufreq_offline(unsigned int cpu)

> >  	if (has_target())

> >  		cpufreq_exit_governor(policy);

> >  

> > +#ifdef CONFIG_CPU_THERMAL

> > +	if (cpufreq_driver->flags & CPUFREQ_AUTO_REGISTER_COOLING_DEV) {

> > +		struct thermal_cooling_device **cdev = &policy->cooldev;

> > +

> > +		cpufreq_cooling_unregister(*cdev);

> 

> Again, why don't you simply pass policy->cooldev here?


I also had the same comments when I looked at your patch :)

I also think we must do the unregistering before calling stop_cpu()
callback.

> Also, would it make sense to clear policy->cooldev at this point?  It points

> to freed memory after cpufreq_cooling_unregister().


Since the core doesn't refer to this field at all and uses it only
while registering/unregistering as a cooling device, there is no
technical issue that we will have today. If someone uses the dangling
pointer later on in future, it will be a bug. So I wouldn't care much
about resetting it here.

> > +	}

> > +#endif

> > +

> >  	/*

> >  	 * Perform the ->exit() even during light-weight tear-down,

> >  	 * since this is a core component, and is essential for the

> > diff --git a/include/linux/cpufreq.h b/include/linux/cpufreq.h

> > index 7d0cf54125fa..70ad02088825 100644

> > --- a/include/linux/cpufreq.h

> > +++ b/include/linux/cpufreq.h

> > @@ -390,6 +390,12 @@ struct cpufreq_driver {

> >   */

> >  #define CPUFREQ_NO_AUTO_DYNAMIC_SWITCHING (1 << 6)

> >  

> > +/*

> > + * Set by drivers that want the core to automatically register the cpufreq

> > + * driver as a thermal cooling device


Add a full-stop here please.

> > + */

> > +#define CPUFREQ_AUTO_REGISTER_COOLING_DEV (1 << 7)

> > +

> >  int cpufreq_register_driver(struct cpufreq_driver *driver_data);

> >  int cpufreq_unregister_driver(struct cpufreq_driver *driver_data);

> >  

> > 

> 


-- 
viresh
diff mbox series

Patch

diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
index 6f23ebb395f1..cd6e750d3d82 100644
--- a/drivers/cpufreq/cpufreq.c
+++ b/drivers/cpufreq/cpufreq.c
@@ -30,6 +30,7 @@ 
 #include <linux/syscore_ops.h>
 #include <linux/tick.h>
 #include <trace/events/power.h>
+#include <linux/cpu_cooling.h>
 
 static LIST_HEAD(cpufreq_policy_list);
 
@@ -1318,6 +1319,14 @@  static int cpufreq_online(unsigned int cpu)
 	if (cpufreq_driver->ready)
 		cpufreq_driver->ready(policy);
 
+#ifdef CONFIG_CPU_THERMAL
+	if (cpufreq_driver->flags & CPUFREQ_AUTO_REGISTER_COOLING_DEV) {
+		struct thermal_cooling_device **cdev = &policy->cooldev;
+
+		*cdev = of_cpufreq_cooling_register(policy);
+	}
+#endif
+
 	pr_debug("initialization complete\n");
 
 	return 0;
@@ -1411,6 +1420,14 @@  static int cpufreq_offline(unsigned int cpu)
 	if (has_target())
 		cpufreq_exit_governor(policy);
 
+#ifdef CONFIG_CPU_THERMAL
+	if (cpufreq_driver->flags & CPUFREQ_AUTO_REGISTER_COOLING_DEV) {
+		struct thermal_cooling_device **cdev = &policy->cooldev;
+
+		cpufreq_cooling_unregister(*cdev);
+	}
+#endif
+
 	/*
 	 * Perform the ->exit() even during light-weight tear-down,
 	 * since this is a core component, and is essential for the
diff --git a/include/linux/cpufreq.h b/include/linux/cpufreq.h
index 7d0cf54125fa..70ad02088825 100644
--- a/include/linux/cpufreq.h
+++ b/include/linux/cpufreq.h
@@ -390,6 +390,12 @@  struct cpufreq_driver {
  */
 #define CPUFREQ_NO_AUTO_DYNAMIC_SWITCHING (1 << 6)
 
+/*
+ * Set by drivers that want the core to automatically register the cpufreq
+ * driver as a thermal cooling device
+ */
+#define CPUFREQ_AUTO_REGISTER_COOLING_DEV (1 << 7)
+
 int cpufreq_register_driver(struct cpufreq_driver *driver_data);
 int cpufreq_unregister_driver(struct cpufreq_driver *driver_data);