| Message ID | 1343213162-8064-6-git-send-email-daniel.lezcano@linaro.org (mailing list archive) |
|---|---|
| State | RFC, archived |
| Headers | show |
On 07/25/2012 04:16 PM, Daniel Lezcano wrote: > The tegra3 and big.LITTLE architecture have different cpu latencies. > This API allows to specify a different cpu latency for a specific cpu. > > With the previous patches, we use the per cpuidle device states pointer, > this function overrides this pointer. > May be add some more explanation and pointers to previous discussions, as stated on the cover in the patch series. > Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org> Acked-by: Deepthi Dharwar <deepthi@linux.vnet.ibm.com> > --- > drivers/cpuidle/cpuidle.c | 17 +++++++++++++++++ > include/linux/cpuidle.h | 10 +++++++--- > 2 files changed, 24 insertions(+), 3 deletions(-) > > diff --git a/drivers/cpuidle/cpuidle.c b/drivers/cpuidle/cpuidle.c > index 199878a..3b21b68 100644 > --- a/drivers/cpuidle/cpuidle.c > +++ b/drivers/cpuidle/cpuidle.c > @@ -456,6 +456,23 @@ void cpuidle_unregister_device(struct cpuidle_device *dev) > > EXPORT_SYMBOL_GPL(cpuidle_unregister_device); > > +int cpuidle_register_states(struct cpuidle_device *dev, > + struct cpuidle_state *states, > + int state_count) > +{ > + if (!dev || !states) > + return -EINVAL; > + > + if (state_count <= 0) > + return -EINVAL; > + > + dev->states = states; > + dev->state_count = state_count; > + > + return 0; > +} > +EXPORT_SYMBOL_GPL(cpuidle_register_state); > + > #ifdef CONFIG_SMP > > static void smp_callback(void *v) > diff --git a/include/linux/cpuidle.h b/include/linux/cpuidle.h > index 40a04a1..425200d 100644 > --- a/include/linux/cpuidle.h > +++ b/include/linux/cpuidle.h > @@ -144,7 +144,9 @@ extern void cpuidle_driver_unref(void); > extern void cpuidle_unregister_driver(struct cpuidle_driver *drv); > extern int cpuidle_register_device(struct cpuidle_device *dev); > extern void cpuidle_unregister_device(struct cpuidle_device *dev); > - > +extern int cpuidle_register_states(struct cpuidle_device *dev, > + struct cpuidle_state *states, > + int state_count); > extern void cpuidle_pause_and_lock(void); > extern void cpuidle_resume_and_unlock(void); > extern void cpuidle_pause(void); > @@ -156,7 +158,6 @@ extern int cpuidle_wrap_enter(struct cpuidle_device *dev, > int (*enter)(struct cpuidle_device *dev, > struct cpuidle_driver *drv, int index)); > extern int cpuidle_play_dead(void); > - > #else > static inline void disable_cpuidle(void) { } > static inline int cpuidle_idle_call(void) { return -ENODEV; } > @@ -169,7 +170,10 @@ static inline void cpuidle_unregister_driver(struct cpuidle_driver *drv) { } > static inline int cpuidle_register_device(struct cpuidle_device *dev) > {return -ENODEV; } > static inline void cpuidle_unregister_device(struct cpuidle_device *dev) { } > - > +static inline int cpuidle_register_states(struct cpuidle_device *dev, > + struct cpuidle_state *states, > + int state_count) > +{ return -ENODEV; } > static inline void cpuidle_pause_and_lock(void) { } > static inline void cpuidle_resume_and_unlock(void) { } > static inline void cpuidle_pause(void) { } -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Daniel, thanks for this patchset. On Wed, Jul 25, 2012 at 11:46:02AM +0100, Daniel Lezcano wrote: > The tegra3 and big.LITTLE architecture have different cpu latencies. > This API allows to specify a different cpu latency for a specific cpu. > > With the previous patches, we use the per cpuidle device states pointer, > this function overrides this pointer. > > Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org> > --- > drivers/cpuidle/cpuidle.c | 17 +++++++++++++++++ > include/linux/cpuidle.h | 10 +++++++--- > 2 files changed, 24 insertions(+), 3 deletions(-) > > diff --git a/drivers/cpuidle/cpuidle.c b/drivers/cpuidle/cpuidle.c > index 199878a..3b21b68 100644 > --- a/drivers/cpuidle/cpuidle.c > +++ b/drivers/cpuidle/cpuidle.c > @@ -456,6 +456,23 @@ void cpuidle_unregister_device(struct cpuidle_device *dev) > > EXPORT_SYMBOL_GPL(cpuidle_unregister_device); > > +int cpuidle_register_states(struct cpuidle_device *dev, > + struct cpuidle_state *states, > + int state_count) > +{ > + if (!dev || !states) > + return -EINVAL; > + > + if (state_count <= 0) > + return -EINVAL; > + > + dev->states = states; > + dev->state_count = state_count; Is this function supposed to be called after cpuidle_device registration ? I think so since at registration time the dev->states pointers are all initialized to point to the driver state array, which is global and not really what we want. Unless this function is called on the cpu that requires swapping the state pointer, I think it is unsafe to register a different state pointer without a minimal level of locking (or disabling idle and renabling idle) since the update of dev->states and dev->state_count is not atomic. Maybe it is implicit but it should be documented somehow to define cpuidle_register_states(...) proper usage. Thanks, Lorenzo -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 08/10/2012 07:17 PM, Lorenzo Pieralisi wrote: > Hi Daniel, > > thanks for this patchset. > > On Wed, Jul 25, 2012 at 11:46:02AM +0100, Daniel Lezcano wrote: >> The tegra3 and big.LITTLE architecture have different cpu latencies. >> This API allows to specify a different cpu latency for a specific cpu. >> >> With the previous patches, we use the per cpuidle device states pointer, >> this function overrides this pointer. >> >> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org> >> --- >> drivers/cpuidle/cpuidle.c | 17 +++++++++++++++++ >> include/linux/cpuidle.h | 10 +++++++--- >> 2 files changed, 24 insertions(+), 3 deletions(-) >> >> diff --git a/drivers/cpuidle/cpuidle.c b/drivers/cpuidle/cpuidle.c >> index 199878a..3b21b68 100644 >> --- a/drivers/cpuidle/cpuidle.c >> +++ b/drivers/cpuidle/cpuidle.c >> @@ -456,6 +456,23 @@ void cpuidle_unregister_device(struct cpuidle_device *dev) >> >> EXPORT_SYMBOL_GPL(cpuidle_unregister_device); >> >> +int cpuidle_register_states(struct cpuidle_device *dev, >> + struct cpuidle_state *states, >> + int state_count) >> +{ >> + if (!dev || !states) >> + return -EINVAL; >> + >> + if (state_count <= 0) >> + return -EINVAL; >> + >> + dev->states = states; >> + dev->state_count = state_count; > > Is this function supposed to be called after cpuidle_device registration ? > I think so since at registration time the dev->states pointers are all > initialized to point to the driver state array, which is global and not > really what we want. > > Unless this function is called on the cpu that requires swapping the state > pointer, I think it is unsafe to register a different state pointer > without a minimal level of locking (or disabling idle and renabling idle) > since the update of dev->states and dev->state_count is not atomic. > Maybe it is implicit but it should be documented somehow to define > cpuidle_register_states(...) proper usage. Hi Lorenzo, Yes, you are right. I will add the cpuidle lock. Thanks ! -- Daniel
On Wed, Jul 25, 2012 at 12:46:02PM +0200, Daniel Lezcano wrote: > The tegra3 and big.LITTLE architecture have different cpu latencies. > This API allows to specify a different cpu latency for a specific cpu. > > With the previous patches, we use the per cpuidle device states pointer, > this function overrides this pointer. > > Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org> > --- > drivers/cpuidle/cpuidle.c | 17 +++++++++++++++++ > include/linux/cpuidle.h | 10 +++++++--- > 2 files changed, 24 insertions(+), 3 deletions(-) > > diff --git a/drivers/cpuidle/cpuidle.c b/drivers/cpuidle/cpuidle.c > index 199878a..3b21b68 100644 > --- a/drivers/cpuidle/cpuidle.c > +++ b/drivers/cpuidle/cpuidle.c > @@ -456,6 +456,23 @@ void cpuidle_unregister_device(struct cpuidle_device *dev) > > EXPORT_SYMBOL_GPL(cpuidle_unregister_device); > > +int cpuidle_register_states(struct cpuidle_device *dev, > + struct cpuidle_state *states, > + int state_count) > +{ > + if (!dev || !states) > + return -EINVAL; > + > + if (state_count <= 0) > + return -EINVAL; > + > + dev->states = states; > + dev->state_count = state_count; > + > + return 0; > +} > +EXPORT_SYMBOL_GPL(cpuidle_register_state); > + > #ifdef CONFIG_SMP > Looks good... apart from the fact that the function definition says cpuidle_register_stateS and the exported symbol is cpuidle_register_state... Cheers, Peter. -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 09/03/2012 03:22 PM, Peter De Schrijver wrote: > On Wed, Jul 25, 2012 at 12:46:02PM +0200, Daniel Lezcano wrote: >> The tegra3 and big.LITTLE architecture have different cpu latencies. >> This API allows to specify a different cpu latency for a specific cpu. >> >> With the previous patches, we use the per cpuidle device states pointer, >> this function overrides this pointer. >> >> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org> >> --- >> drivers/cpuidle/cpuidle.c | 17 +++++++++++++++++ >> include/linux/cpuidle.h | 10 +++++++--- >> 2 files changed, 24 insertions(+), 3 deletions(-) >> >> diff --git a/drivers/cpuidle/cpuidle.c b/drivers/cpuidle/cpuidle.c >> index 199878a..3b21b68 100644 >> --- a/drivers/cpuidle/cpuidle.c >> +++ b/drivers/cpuidle/cpuidle.c >> @@ -456,6 +456,23 @@ void cpuidle_unregister_device(struct cpuidle_device *dev) >> >> EXPORT_SYMBOL_GPL(cpuidle_unregister_device); >> >> +int cpuidle_register_states(struct cpuidle_device *dev, >> + struct cpuidle_state *states, >> + int state_count) >> +{ >> + if (!dev || !states) >> + return -EINVAL; >> + >> + if (state_count <= 0) >> + return -EINVAL; >> + >> + dev->states = states; >> + dev->state_count = state_count; >> + >> + return 0; >> +} >> +EXPORT_SYMBOL_GPL(cpuidle_register_state); >> + >> #ifdef CONFIG_SMP >> > > Looks good... apart from the fact that the function definition says > cpuidle_register_stateS and the exported symbol is cpuidle_register_state... Ok, fixed. Thanks ! -- Daniel
diff --git a/drivers/cpuidle/cpuidle.c b/drivers/cpuidle/cpuidle.c index 199878a..3b21b68 100644 --- a/drivers/cpuidle/cpuidle.c +++ b/drivers/cpuidle/cpuidle.c @@ -456,6 +456,23 @@ void cpuidle_unregister_device(struct cpuidle_device *dev) EXPORT_SYMBOL_GPL(cpuidle_unregister_device); +int cpuidle_register_states(struct cpuidle_device *dev, + struct cpuidle_state *states, + int state_count) +{ + if (!dev || !states) + return -EINVAL; + + if (state_count <= 0) + return -EINVAL; + + dev->states = states; + dev->state_count = state_count; + + return 0; +} +EXPORT_SYMBOL_GPL(cpuidle_register_state); + #ifdef CONFIG_SMP static void smp_callback(void *v) diff --git a/include/linux/cpuidle.h b/include/linux/cpuidle.h index 40a04a1..425200d 100644 --- a/include/linux/cpuidle.h +++ b/include/linux/cpuidle.h @@ -144,7 +144,9 @@ extern void cpuidle_driver_unref(void); extern void cpuidle_unregister_driver(struct cpuidle_driver *drv); extern int cpuidle_register_device(struct cpuidle_device *dev); extern void cpuidle_unregister_device(struct cpuidle_device *dev); - +extern int cpuidle_register_states(struct cpuidle_device *dev, + struct cpuidle_state *states, + int state_count); extern void cpuidle_pause_and_lock(void); extern void cpuidle_resume_and_unlock(void); extern void cpuidle_pause(void); @@ -156,7 +158,6 @@ extern int cpuidle_wrap_enter(struct cpuidle_device *dev, int (*enter)(struct cpuidle_device *dev, struct cpuidle_driver *drv, int index)); extern int cpuidle_play_dead(void); - #else static inline void disable_cpuidle(void) { } static inline int cpuidle_idle_call(void) { return -ENODEV; } @@ -169,7 +170,10 @@ static inline void cpuidle_unregister_driver(struct cpuidle_driver *drv) { } static inline int cpuidle_register_device(struct cpuidle_device *dev) {return -ENODEV; } static inline void cpuidle_unregister_device(struct cpuidle_device *dev) { } - +static inline int cpuidle_register_states(struct cpuidle_device *dev, + struct cpuidle_state *states, + int state_count) +{ return -ENODEV; } static inline void cpuidle_pause_and_lock(void) { } static inline void cpuidle_resume_and_unlock(void) { } static inline void cpuidle_pause(void) { }
The tegra3 and big.LITTLE architecture have different cpu latencies. This API allows to specify a different cpu latency for a specific cpu. With the previous patches, we use the per cpuidle device states pointer, this function overrides this pointer. Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org> --- drivers/cpuidle/cpuidle.c | 17 +++++++++++++++++ include/linux/cpuidle.h | 10 +++++++--- 2 files changed, 24 insertions(+), 3 deletions(-)