On Tuesday 23 April 2013 06:21 PM, Daniel Lezcano wrote:
On 04/23/2013 02:32 PM, Santosh Shilimkar wrote:
On Tuesday 23 April 2013 02:24 PM, 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.
Please note, some drivers are doing dev->state_count = drv->state_count. This is not necessary because it is done by the cpuidle_enable_device function in the cpuidle framework. This is true, until you have the same states for all your devices. Otherwise, the 'low level' API should be used instead with the specific initialization for the driver.
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
I don't see you have addressed the comment on V3 [1] i gave for the subject patch Any reason ?
Yes, sorry for not answering.
This modification should be handled in the __cpuidle_register_device function.
diff --git a/drivers/cpuidle/cpuidle.c b/drivers/cpuidle/cpuidle.c index 49e8d30..936d862 100644 --- a/drivers/cpuidle/cpuidle.c +++ b/drivers/cpuidle/cpuidle.c @@ -372,7 +372,7 @@ static int __cpuidle_register_device(struct cpuidle_device *dev) int ret; struct cpuidle_driver *drv = cpuidle_get_cpu_driver(dev);
if (!try_module_get(drv->owner))
if (!drv || !try_module_get(drv->owner)) return -EINVAL; per_cpu(cpuidle_devices, dev->cpu) = dev;Thus, the cpuidle_register function is not impacted by this as it will always do cpuidle_register_driver, followed by cpuidle_register_device.
I still don't follow you. I know the fix will be needed but in the subject patch sequence as well, the device registration should be done first and then the driver registration. Below hunk I mean.
---- +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) { + pr_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 + /* + * On multiplatform for ARM, the coupled idle states could + * enabled in the kernel even if the cpuidle driver does not + * use it. Note, coupled_cpus is a struct copy. + */ + if (coupled_cpus) + device->coupled_cpus = *coupled_cpus; +#endif + ret = cpuidle_register_device(device); + if (!ret) + continue; + + pr_err("Failed to register cpuidle device for cpu%d\n", cpu); + + cpuidle_unregister(drv); + break; + } ------
I also had a comment on kernel doc for both of these new functions. Out of curiosity I have seen this patch again o.w I would have assumed that you did address the comments.
Regards, Santosh