Need help understanding the logic of __cpuidle_set_driver

[Mohammad Merajul Islam Molla]

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