| Message ID | c1e460c49ad2f16b15ace909514a770c9753d1b1.1547481320.git.amit.kucheria@linaro.org (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Delegated to: | Andy Gross |
| Headers | show |
| Series | cpufreq: Add flag to auto-register as cooling device | expand |
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>
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; > + > + *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? Also, would it make sense to clear policy->cooldev at this point? It points to freed memory after cpufreq_cooling_unregister(). > + } > +#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); > >
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); > > > > >
On Thu, Jan 17, 2019 at 10:12 AM Viresh Kumar <viresh.kumar@linaro.org> wrote: > > 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. Fixed > > > + > > > + *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. Fixed. > > 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. Fixed Thanks for the review. > > > + */ > > > +#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); > > >
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);
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(+)