Re: Need help understanding the logic of __cpuidle_set_driver

[Daniel Lezcano <daniel.lezcano@linaro.org>]

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