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.
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?
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
_______________________________________________
Kernelnewbies mailing list
Kernelnewbies@xxxxxxxxxxxxxxxxx
http://lists.kernelnewbies.org/mailman/listinfo/kernelnewbies
