On Thursday 11 of October 2012 12:04:37 Daniel Lezcano wrote: > On 10/07/2012 11:26 PM, Rafael J. Wysocki wrote: > > On Tuesday 25 of September 2012 00:43:54 Daniel Lezcano wrote: > >> With the tegra3 and the big.LITTLE [1] new architectures, several cpus > >> with different characteristics (latencies and states) can co-exists on the > >> system. > >> > >> The cpuidle framework has the limitation of handling only identical cpus. > >> > >> This patch removes this limitation by introducing the multiple driver support > >> for cpuidle. > >> > >> This option is configurable at compile time and should be enabled for the > >> architectures mentioned above. So there is no impact for the other platforms > >> if the option is disabled. The option defaults to 'n'. Note the multiple drivers > >> support is also compatible with the existing drivers, even if just one driver is > >> needed, all the cpu will be tied to this driver using an extra small chunk of > >> processor memory. > >> > >> The multiple driver support use a per-cpu driver pointer instead of a global > >> variable and the accessor to this variable are done from a cpu context. > > Thanks Rafael for the review. > > I took into account all your remarks for the V2. > > [ cut ] > > >> +static int __cpuidle_register_all_cpu_driver(struct cpuidle_driver *drv) > >> +{ > >> + int ret = 0; > >> + int i, cpu; > >> + > >> + for_each_present_cpu(cpu) { > >> + ret = __cpuidle_register_driver(drv, cpu); > >> + if (!ret) > >> + continue; > >> + for (i = cpu - 1; i >= 0; i--) > > I wonder if this is going to work in all cases. For example, is there any > > guarantee that the CPU numbers start from 0 and that there are no gaps? > > AFAICS, the cpumask.h is not assuming the cpu numbers start from zero > and they are contiguous. > > I will fix this reverse loop, thanks for spotting this. > > [ cut ] > > >> void cpuidle_unregister_driver(struct cpuidle_driver *drv) > >> { > >> spin_lock(&cpuidle_driver_lock); > >> - __cpuidle_unregister_driver(drv); > >> +#ifdef CONFIG_CPU_IDLE_MULTIPLE_DRIVERS > >> + __cpuidle_unregister_all_cpu_driver(drv); > >> +#else > >> + __cpuidle_unregister_driver(drv, smp_processor_id()); > >> +#endif > > I'm slightly cautious about using smp_processor_id() above. > > get_cpu()/put_cpu() is the preferred way of doing this nowadays (although > > this particular code is under the spinlock, so it should be OK). > yes, get_cpu does preempt_disable() and smp_processor_id() > As spin_lock does also preempt_disable() that should be ok. > But I changed the code to use the preferred way. Cool, thanks! I've seen your new version and I'm going to look deeper into it in the next few days, but I will be travelling from tomorrow through the end of the next week, so I may be even slower to respond than usually. Sorry about that. Thanks, Rafael -- I speak only for myself. Rafael J. Wysocki, Intel Open Source Technology Center. -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html