| Message ID | 1365603743-5618-6-git-send-email-daniel.lezcano@linaro.org |
|---|---|
| State | New |
| Headers | show |
> +/** > + * cpuidle_register: registers the driver and the cpu devices with the > + * coupled_cpus passed as parameter. This function is used for all common > + * initialization pattern there are in the arch specific drivers. The > + * devices is globally defined in this file. > + * > + * @drv : a valid pointer to a struct cpuidle_driver > + * @coupled_cpus: a cpumask for the coupled states > + * > + * Returns 0 on success, < 0 otherwise > + */ > +int cpuidle_register(struct cpuidle_driver *drv, > + const struct cpumask *const coupled_cpus) > +{ > + int ret, cpu; > + struct cpuidle_device *device; > + > + ret = cpuidle_register_driver(drv); > + if (ret) { > + printk(KERN_ERR "failed to register cpuidle driver\n"); pr_err() > + return ret; > + } > + > + for_each_possible_cpu(cpu) { > + device = &per_cpu(cpuidle_dev, cpu); > + device->cpu = cpu; > +#ifdef CONFIG_ARCH_NEEDS_CPU_IDLE_COUPLED > + device->coupled_cpus = *coupled_cpus; > +#endif > + ret = cpuidle_register_device(device); > + if (!ret) > + continue; > + > + printk(KERN_ERR "Failed to register cpuidle " > + "device for cpu%d\n", cpu); pr_err() and don't split the message over two lines, it makes it harder for somebody to find with grep -r "Failed to register cpuidle device for cpu" * > + cpuidle_unregister(drv); > + break; > + } > + > + return 0; You should return an error code, so that the caller can also return an error code. If you look at cpuidle-kirkwood and cpuidle-calxeda, and maybe others, if the registration fails, the probe function returns an error code, as it should. With your change, its always going to return 0, even if it fails. Andrew
> +int cpuidle_register(struct cpuidle_driver *drv, > + const struct cpumask *const coupled_cpus) > +{ > + int ret, cpu; > + struct cpuidle_device *device; > + > + ret = cpuidle_register_driver(drv); > + if (ret) { > + printk(KERN_ERR "failed to register cpuidle driver\n"); > + return ret; > + } > + > + for_each_possible_cpu(cpu) { > + device = &per_cpu(cpuidle_dev, cpu); > + device->cpu = cpu; > +#ifdef CONFIG_ARCH_NEEDS_CPU_IDLE_COUPLED > + device->coupled_cpus = *coupled_cpus; > +#endif At least the kirkwood and the calxeda driver set device->state_count which you don't appear to do. cpuidle_add_state_sysfs() and cpuidle_remove_state_sysfs() use this. Is it now being set somewhere else? Thanks Andrew
On 04/10/2013 07:04 PM, Andrew Lunn wrote: >> +int cpuidle_register(struct cpuidle_driver *drv, >> + const struct cpumask *const coupled_cpus) >> +{ >> + int ret, cpu; >> + struct cpuidle_device *device; >> + >> + ret = cpuidle_register_driver(drv); >> + if (ret) { >> + printk(KERN_ERR "failed to register cpuidle driver\n"); >> + return ret; >> + } >> + >> + for_each_possible_cpu(cpu) { >> + device = &per_cpu(cpuidle_dev, cpu); >> + device->cpu = cpu; >> +#ifdef CONFIG_ARCH_NEEDS_CPU_IDLE_COUPLED >> + device->coupled_cpus = *coupled_cpus; >> +#endif > > At least the kirkwood and the calxeda driver set > > device->state_count > > which you don't appear to do. cpuidle_add_state_sysfs() and > cpuidle_remove_state_sysfs() use this. Is it now being set somewhere > else? Yes, in cpuidle_enable_device called from cpuidle_register_device: int cpuidle_enable_device(struct cpuidle_device *dev) { ... if (!dev->state_count) dev->state_count = drv->state_count; ... }
On 04/10/2013 06:55 PM, Andrew Lunn wrote: >> +/** >> + * cpuidle_register: registers the driver and the cpu devices with the >> + * coupled_cpus passed as parameter. This function is used for all common >> + * initialization pattern there are in the arch specific drivers. The >> + * devices is globally defined in this file. >> + * >> + * @drv : a valid pointer to a struct cpuidle_driver >> + * @coupled_cpus: a cpumask for the coupled states >> + * >> + * Returns 0 on success, < 0 otherwise >> + */ >> +int cpuidle_register(struct cpuidle_driver *drv, >> + const struct cpumask *const coupled_cpus) >> +{ >> + int ret, cpu; >> + struct cpuidle_device *device; >> + >> + ret = cpuidle_register_driver(drv); >> + if (ret) { >> + printk(KERN_ERR "failed to register cpuidle driver\n"); > > pr_err() Ok. >> + return ret; >> + } >> + >> + for_each_possible_cpu(cpu) { >> + device = &per_cpu(cpuidle_dev, cpu); >> + device->cpu = cpu; >> +#ifdef CONFIG_ARCH_NEEDS_CPU_IDLE_COUPLED >> + device->coupled_cpus = *coupled_cpus; >> +#endif >> + ret = cpuidle_register_device(device); >> + if (!ret) >> + continue; >> + >> + printk(KERN_ERR "Failed to register cpuidle " >> + "device for cpu%d\n", cpu); > > pr_err() and don't split the message over two lines, it makes it > harder for somebody to find with > > grep -r "Failed to register cpuidle device for cpu" * Ok if the line length is under 80 chars. >> + cpuidle_unregister(drv); >> + break; >> + } >> + >> + return 0; > > You should return an error code, so that the caller can also return an > error code. If you look at cpuidle-kirkwood and cpuidle-calxeda, and > maybe others, if the registration fails, the probe function returns an > error code, as it should. With your change, its always going to return > 0, even if it fails. Right, right :) it should be 'return ret;' Thanks for pointing this. -- Daniel
On Wed, Apr 10, 2013 at 08:04:22PM +0200, Daniel Lezcano wrote: > On 04/10/2013 06:55 PM, Andrew Lunn wrote: > >> +/** > >> + * cpuidle_register: registers the driver and the cpu devices with the > >> + * coupled_cpus passed as parameter. This function is used for all common > >> + * initialization pattern there are in the arch specific drivers. The > >> + * devices is globally defined in this file. > >> + * > >> + * @drv : a valid pointer to a struct cpuidle_driver > >> + * @coupled_cpus: a cpumask for the coupled states > >> + * > >> + * Returns 0 on success, < 0 otherwise > >> + */ > >> +int cpuidle_register(struct cpuidle_driver *drv, > >> + const struct cpumask *const coupled_cpus) > >> +{ > >> + int ret, cpu; > >> + struct cpuidle_device *device; > >> + > >> + ret = cpuidle_register_driver(drv); > >> + if (ret) { > >> + printk(KERN_ERR "failed to register cpuidle driver\n"); > > > > pr_err() > > Ok. > > >> + return ret; > >> + } > >> + > >> + for_each_possible_cpu(cpu) { > >> + device = &per_cpu(cpuidle_dev, cpu); > >> + device->cpu = cpu; > >> +#ifdef CONFIG_ARCH_NEEDS_CPU_IDLE_COUPLED > >> + device->coupled_cpus = *coupled_cpus; > >> +#endif > >> + ret = cpuidle_register_device(device); > >> + if (!ret) > >> + continue; > >> + > >> + printk(KERN_ERR "Failed to register cpuidle " > >> + "device for cpu%d\n", cpu); > > > > pr_err() and don't split the message over two lines, it makes it > > harder for somebody to find with > > > > grep -r "Failed to register cpuidle device for cpu" * > > Ok if the line length is under 80 chars. Take a closer look at the CodingStyle documents. This is one except to the rule. Such lines can be longer than 80 characters. Andrew
On Wed, Apr 10, 2013 at 7:22 AM, Daniel Lezcano <daniel.lezcano@linaro.org> wrote: > The usual scheme to initialize a cpuidle driver on a SMP is: > > cpuidle_register_driver(drv); > for_each_possible_cpu(cpu) { > device = &per_cpu(cpuidle_dev, cpu); > cpuidle_register_device(device); > } > > This code is duplicated in each cpuidle driver. > > On UP systems, it is done this way: > > cpuidle_register_driver(drv); > device = &per_cpu(cpuidle_dev, cpu); > cpuidle_register_device(device); > > On UP, the macro 'for_each_cpu' does one iteration: > > #define for_each_cpu(cpu, mask) \ > for ((cpu) = 0; (cpu) < 1; (cpu)++, (void)mask) > > Hence, the initialization loop is the same for UP than SMP. > > Beside, we saw different bugs / mis-initialization / return code unchecked in > the different drivers, the code is duplicated including bugs. After fixing all > these ones, it appears the initialization pattern is the same for everyone. > > Let's add a wrapper function doing this initialization with a cpumask parameter > for the coupled idle states and use it for all the drivers. Isn't this going to cause problems for big.LITTLE, which has two independent sets of coupled cpus, and may even have different drivers for the two types of cpu?
On Wed, Apr 10, 2013 at 08:02:31PM +0200, Daniel Lezcano wrote: > On 04/10/2013 07:04 PM, Andrew Lunn wrote: > >> +int cpuidle_register(struct cpuidle_driver *drv, > >> + const struct cpumask *const coupled_cpus) > >> +{ > >> + int ret, cpu; > >> + struct cpuidle_device *device; > >> + > >> + ret = cpuidle_register_driver(drv); > >> + if (ret) { > >> + printk(KERN_ERR "failed to register cpuidle driver\n"); > >> + return ret; > >> + } > >> + > >> + for_each_possible_cpu(cpu) { > >> + device = &per_cpu(cpuidle_dev, cpu); > >> + device->cpu = cpu; > >> +#ifdef CONFIG_ARCH_NEEDS_CPU_IDLE_COUPLED > >> + device->coupled_cpus = *coupled_cpus; > >> +#endif > > > > At least the kirkwood and the calxeda driver set > > > > device->state_count > > > > which you don't appear to do. cpuidle_add_state_sysfs() and > > cpuidle_remove_state_sysfs() use this. Is it now being set somewhere > > else? > > Yes, in cpuidle_enable_device called from cpuidle_register_device: O.K. It would be nice to add a comment in the change log message about this. Ah, also, it would be good to update Documentation/cpuidle/drivers.txt with these new functions and update the text. Thanks Andrew
On 04/10/2013 08:22 PM, Colin Cross wrote: > On Wed, Apr 10, 2013 at 7:22 AM, Daniel Lezcano > <daniel.lezcano@linaro.org> wrote: >> The usual scheme to initialize a cpuidle driver on a SMP is: >> >> cpuidle_register_driver(drv); >> for_each_possible_cpu(cpu) { >> device = &per_cpu(cpuidle_dev, cpu); >> cpuidle_register_device(device); >> } >> >> This code is duplicated in each cpuidle driver. >> >> On UP systems, it is done this way: >> >> cpuidle_register_driver(drv); >> device = &per_cpu(cpuidle_dev, cpu); >> cpuidle_register_device(device); >> >> On UP, the macro 'for_each_cpu' does one iteration: >> >> #define for_each_cpu(cpu, mask) \ >> for ((cpu) = 0; (cpu) < 1; (cpu)++, (void)mask) >> >> Hence, the initialization loop is the same for UP than SMP. >> >> Beside, we saw different bugs / mis-initialization / return code unchecked in >> the different drivers, the code is duplicated including bugs. After fixing all >> these ones, it appears the initialization pattern is the same for everyone. >> >> Let's add a wrapper function doing this initialization with a cpumask parameter >> for the coupled idle states and use it for all the drivers. > > Isn't this going to cause problems for big.LITTLE, which has two > independent sets of coupled cpus, and may even have different drivers > for the two types of cpu? AFAICT no, because the "low levels" API are still there, I mean cpuidle_register_driver and cpuidle_register_device. That will be up to the b.L driver to use the API which fits its need.
On 04/10/2013 08:23 PM, Andrew Lunn wrote: > On Wed, Apr 10, 2013 at 08:02:31PM +0200, Daniel Lezcano wrote: >> On 04/10/2013 07:04 PM, Andrew Lunn wrote: >>>> +int cpuidle_register(struct cpuidle_driver *drv, >>>> + const struct cpumask *const coupled_cpus) >>>> +{ >>>> + int ret, cpu; >>>> + struct cpuidle_device *device; >>>> + >>>> + ret = cpuidle_register_driver(drv); >>>> + if (ret) { >>>> + printk(KERN_ERR "failed to register cpuidle driver\n"); >>>> + return ret; >>>> + } >>>> + >>>> + for_each_possible_cpu(cpu) { >>>> + device = &per_cpu(cpuidle_dev, cpu); >>>> + device->cpu = cpu; >>>> +#ifdef CONFIG_ARCH_NEEDS_CPU_IDLE_COUPLED >>>> + device->coupled_cpus = *coupled_cpus; >>>> +#endif >>> >>> At least the kirkwood and the calxeda driver set >>> >>> device->state_count >>> >>> which you don't appear to do. cpuidle_add_state_sysfs() and >>> cpuidle_remove_state_sysfs() use this. Is it now being set somewhere >>> else? >> >> Yes, in cpuidle_enable_device called from cpuidle_register_device: > > O.K. It would be nice to add a comment in the change log message about > this. > > Ah, also, it would be good to update Documentation/cpuidle/drivers.txt > with these new functions and update the text. Ok, I will take care of that. Thanks. -- Daniel
On 04/10/2013 08:18 PM, Andrew Lunn wrote: > On Wed, Apr 10, 2013 at 08:04:22PM +0200, Daniel Lezcano wrote: >> On 04/10/2013 06:55 PM, Andrew Lunn wrote: >>>> +/** >>>> + * cpuidle_register: registers the driver and the cpu devices with the >>>> + * coupled_cpus passed as parameter. This function is used for all common >>>> + * initialization pattern there are in the arch specific drivers. The >>>> + * devices is globally defined in this file. >>>> + * >>>> + * @drv : a valid pointer to a struct cpuidle_driver >>>> + * @coupled_cpus: a cpumask for the coupled states >>>> + * >>>> + * Returns 0 on success, < 0 otherwise >>>> + */ >>>> +int cpuidle_register(struct cpuidle_driver *drv, >>>> + const struct cpumask *const coupled_cpus) >>>> +{ >>>> + int ret, cpu; >>>> + struct cpuidle_device *device; >>>> + >>>> + ret = cpuidle_register_driver(drv); >>>> + if (ret) { >>>> + printk(KERN_ERR "failed to register cpuidle driver\n"); >>> >>> pr_err() >> >> Ok. >> >>>> + return ret; >>>> + } >>>> + >>>> + for_each_possible_cpu(cpu) { >>>> + device = &per_cpu(cpuidle_dev, cpu); >>>> + device->cpu = cpu; >>>> +#ifdef CONFIG_ARCH_NEEDS_CPU_IDLE_COUPLED >>>> + device->coupled_cpus = *coupled_cpus; >>>> +#endif >>>> + ret = cpuidle_register_device(device); >>>> + if (!ret) >>>> + continue; >>>> + >>>> + printk(KERN_ERR "Failed to register cpuidle " >>>> + "device for cpu%d\n", cpu); >>> >>> pr_err() and don't split the message over two lines, it makes it >>> harder for somebody to find with >>> >>> grep -r "Failed to register cpuidle device for cpu" * >> >> Ok if the line length is under 80 chars. > > Take a closer look at the CodingStyle documents. This is one except to > the rule. Such lines can be longer than 80 characters. Oh, right ! "However, never break user-visible strings such as printk messages, because that breaks the ability to grep for them." Will fix it, thanks. -- Daniel
On 04/10/2013 04:59 PM, Rob Herring wrote: > On 04/10/2013 09:22 AM, Daniel Lezcano wrote: >> The usual scheme to initialize a cpuidle driver on a SMP is: >> >> cpuidle_register_driver(drv); >> for_each_possible_cpu(cpu) { >> device = &per_cpu(cpuidle_dev, cpu); >> cpuidle_register_device(device); >> } >> >> This code is duplicated in each cpuidle driver. >> >> On UP systems, it is done this way: >> >> cpuidle_register_driver(drv); >> device = &per_cpu(cpuidle_dev, cpu); >> cpuidle_register_device(device); >> >> On UP, the macro 'for_each_cpu' does one iteration: >> >> #define for_each_cpu(cpu, mask) \ >> for ((cpu) = 0; (cpu) < 1; (cpu)++, (void)mask) >> >> Hence, the initialization loop is the same for UP than SMP. >> >> Beside, we saw different bugs / mis-initialization / return code unchecked in >> the different drivers, the code is duplicated including bugs. After fixing all >> these ones, it appears the initialization pattern is the same for everyone. >> >> Let's add a wrapper function doing this initialization with a cpumask parameter >> for the coupled idle states and use it for all the drivers. >> >> That will save a lot of LOC, consolidate the code, and the modifications in the >> future could be done in a single place. Another benefit is the consolidation of >> the cpuidle_device variable which is now in the cpuidle framework and no longer >> spread accross the different arch specific drivers. >> >> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org> >> --- > Perhaps my ack was too quick... Hi Rob, now that I have fixed the routine in V3, shall I consider the patch is acked ? Thanks -- Daniel
diff --git a/drivers/cpuidle/cpuidle.c b/drivers/cpuidle/cpuidle.c index 0da795b..3a5454b 100644 --- a/drivers/cpuidle/cpuidle.c +++ b/drivers/cpuidle/cpuidle.c @@ -24,6 +24,7 @@ #include "cpuidle.h" DEFINE_PER_CPU(struct cpuidle_device *, cpuidle_devices); +DEFINE_PER_CPU(struct cpuidle_device, cpuidle_dev); DEFINE_MUTEX(cpuidle_lock); LIST_HEAD(cpuidle_detected_devices); @@ -453,6 +454,70 @@ void cpuidle_unregister_device(struct cpuidle_device *dev) EXPORT_SYMBOL_GPL(cpuidle_unregister_device); +/* + * cpuidle_unregister: unregister a driver and the devices. This function + * can be used only if the driver has been previously registered through + * the cpuidle_register function. + * + * @drv: a valid pointer to a struct cpuidle_driver + */ +void cpuidle_unregister(struct cpuidle_driver *drv) +{ + int cpu; + struct cpuidle_device *device; + + for_each_possible_cpu(cpu) { + device = &per_cpu(cpuidle_dev, cpu); + cpuidle_unregister_device(device); + } + + cpuidle_unregister_driver(drv); +} +EXPORT_SYMBOL_GPL(cpuidle_unregister); + +/** + * cpuidle_register: registers the driver and the cpu devices with the + * coupled_cpus passed as parameter. This function is used for all common + * initialization pattern there are in the arch specific drivers. The + * devices is globally defined in this file. + * + * @drv : a valid pointer to a struct cpuidle_driver + * @coupled_cpus: a cpumask for the coupled states + * + * Returns 0 on success, < 0 otherwise + */ +int cpuidle_register(struct cpuidle_driver *drv, + const struct cpumask *const coupled_cpus) +{ + int ret, cpu; + struct cpuidle_device *device; + + ret = cpuidle_register_driver(drv); + if (ret) { + printk(KERN_ERR "failed to register cpuidle driver\n"); + return ret; + } + + for_each_possible_cpu(cpu) { + device = &per_cpu(cpuidle_dev, cpu); + device->cpu = cpu; +#ifdef CONFIG_ARCH_NEEDS_CPU_IDLE_COUPLED + device->coupled_cpus = *coupled_cpus; +#endif + ret = cpuidle_register_device(device); + if (!ret) + continue; + + printk(KERN_ERR "Failed to register cpuidle " + "device for cpu%d\n", cpu); + cpuidle_unregister(drv); + break; + } + + return 0; +} +EXPORT_SYMBOL_GPL(cpuidle_register); + #ifdef CONFIG_SMP static void smp_callback(void *v) diff --git a/include/linux/cpuidle.h b/include/linux/cpuidle.h index 79e3811..3c86faa 100644 --- a/include/linux/cpuidle.h +++ b/include/linux/cpuidle.h @@ -123,7 +123,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(struct cpuidle_driver *drv, + const struct cpumask *const coupled_cpus); +extern void cpuidle_unregister(struct cpuidle_driver *drv); extern void cpuidle_pause_and_lock(void); extern void cpuidle_resume_and_unlock(void); extern void cpuidle_pause(void); @@ -148,7 +150,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(struct cpuidle_driver *drv, + const struct cpumask *const coupled_cpus) +{return -ENODEV; } +static inline void cpuidle_unregister(struct cpuidle_driver *drv) { } static inline void cpuidle_pause_and_lock(void) { } static inline void cpuidle_resume_and_unlock(void) { } static inline void cpuidle_pause(void) { }
The usual scheme to initialize a cpuidle driver on a SMP is: cpuidle_register_driver(drv); for_each_possible_cpu(cpu) { device = &per_cpu(cpuidle_dev, cpu); cpuidle_register_device(device); } This code is duplicated in each cpuidle driver. On UP systems, it is done this way: cpuidle_register_driver(drv); device = &per_cpu(cpuidle_dev, cpu); cpuidle_register_device(device); On UP, the macro 'for_each_cpu' does one iteration: #define for_each_cpu(cpu, mask) \ for ((cpu) = 0; (cpu) < 1; (cpu)++, (void)mask) Hence, the initialization loop is the same for UP than SMP. Beside, we saw different bugs / mis-initialization / return code unchecked in the different drivers, the code is duplicated including bugs. After fixing all these ones, it appears the initialization pattern is the same for everyone. Let's add a wrapper function doing this initialization with a cpumask parameter for the coupled idle states and use it for all the drivers. That will save a lot of LOC, consolidate the code, and the modifications in the future could be done in a single place. Another benefit is the consolidation of the cpuidle_device variable which is now in the cpuidle framework and no longer spread accross the different arch specific drivers. Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org> --- drivers/cpuidle/cpuidle.c | 65 +++++++++++++++++++++++++++++++++++++++++++++ include/linux/cpuidle.h | 9 +++++-- 2 files changed, 72 insertions(+), 2 deletions(-)