On 08/15/2014 12:20 PM, Mohammad Merajul Islam Molla wrote:
Hello,
I was looking into the code of drivers/cpuidle/driver.c. I have some doubts regarding the implementation of __cpuidle_set_driver function when CONFIG_CPU_IDLE_MULTIPLE_DRIVERS is defined.
If CONFIG_CPU_IDLE_MULTIPLE_DRIVERS is defined, the code for __cpuidle_set_driver/__cpuidle_unset_driver looks as -
39 * __cpuidle_unset_driver - unset per CPU driver variables. 40 * @drv: a valid pointer to a struct cpuidle_driver 41 * 42 * For each CPU in the driver's CPU mask, unset the registered driver per CPU 43 * variable. If @drv is different from the registered driver, the corresponding 44 * variable is not cleared. 45 */ 46 static inline void __cpuidle_unset_driver(struct cpuidle_driver *drv) 47 { 48 int cpu; 49 50 for_each_cpu(cpu, drv->cpumask) { 51 52 if (drv != __cpuidle_get_cpu_driver(cpu)) 53 continue; 54 55 per_cpu(cpuidle_drivers, cpu) = NULL; 56 } 57 } 58 59 /** 60 * __cpuidle_set_driver - set per CPU driver variables for the given driver. 61 * @drv: a valid pointer to a struct cpuidle_driver 62 * 63 * For each CPU in the driver's cpumask, unset the registered driver per CPU 64 * to @drv. 65 * 66 * Returns 0 on success, -EBUSY if the CPUs have driver(s) already. 67 */ 68 static inline int __cpuidle_set_driver(struct cpuidle_driver *drv) 69 { 70 int cpu; 71 72 for_each_cpu(cpu, drv->cpumask) { 73 74 if (__cpuidle_get_cpu_driver(cpu)) { 75 __cpuidle_unset_driver(drv); 76 return -EBUSY; 77 } 78 79 per_cpu(cpuidle_drivers, cpu) = drv; 80 } 81 82 return 0; 83 }
Apparently, the comment should be - "set/register the driver per CPU to @drv" instead of "unset the registered driver per CPU to @drv" in case of __cpuidle_set_driver.
Right, it is a typo.
However, regarding the logic, I have a few doubts -
1. for each cpu in drv->cpumask, if there is already a driver registered, its calling __cpuidle_unset_driver which loops over for each cpu in drv->cpumask again. Isn't it unnecessary to do this nested calls?
It is to rollback the previous changes done in the loop.
2. after calling __cpuidle_unset_driver, if drv equals already registered driver, it sets per_cpu driver to null? Isn't it wrong when we are trying to set to a new driver? Why do we need to unset and make the driver null when we are returning EBUSY from __cpuidle_set_driver?
Would it be correct and cleaner if the code is written as below -
static inline int __cpuidle_set_driver(struct cpuidle_driver *drv) { int ret = -EBUSY; int cpu;
for_each_cpu(cpu, drv->cpumask) { if (drv == __cpuidle_get_cpu_driver(cpu)) [if drv is already the registered driver, do nothing] continue;
per_cpu(cpuidle_drivers, cpu) = drv; [if only drv != already registered driver, set per_cpu driver to drv and set ret 0] ret = 0; }
return ret; [only if all cpus already had drv as registered driver, return -EBUSY. Otherwise return 0] }
Please let me know your views.
-- Thanks, -Meraj -- 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
-- <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs Follow Linaro: <http://www.facebook.com/pages/Linaro> Facebook | <http://twitter.com/#!/linaroorg> Twitter | <http://www.linaro.org/linaro-blog/> Blog