On Tuesday 25 of September 2012 00:43:52 Daniel Lezcano wrote:
> The code checks if the driver is already set without taking the lock,
> but, right after, it takes the lock to assign the variable.
>
> If it is safe to check the variable without lock, then it is safe to
> assign it without lock. If it is unsafe to assign without a lock, then
> it is unsafe to check it without a lock.
>
> I don't find a path in the different drivers where that could happen
> because the arch specific drivers are written in such way it is not
> possible to register a driver while it is unregistered, except maybe
> in a very improbable case when "intel_idle" and "processor_idle" are
> competing. One could unregister a driver, while the other one is
> registering.
>
> Signed-off-by: Daniel Lezcano <daniel.lezc...@linaro.org>
The code looks generally OK (a minor nit below), but I'm not really sure what
you wanted to say in the changelog. It would suffice to say that the existing
code is racy and either we don't need to take the spinlock in
cpuidle_unregister_driver() at all, or we should put the test under the
spinlock as well as the modification.
> ---
> drivers/cpuidle/driver.c | 8 ++++----
> 1 files changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/cpuidle/driver.c b/drivers/cpuidle/driver.c
> index 39ba8e1..4a0c4ab 100644
> --- a/drivers/cpuidle/driver.c
> +++ b/drivers/cpuidle/driver.c
> @@ -85,17 +85,17 @@ EXPORT_SYMBOL_GPL(cpuidle_get_driver);
> */
> void cpuidle_unregister_driver(struct cpuidle_driver *drv)
> {
> + spin_lock(&cpuidle_driver_lock);
> +
> if (drv != cpuidle_curr_driver) {
> WARN(1, "invalid cpuidle_unregister_driver(%s)\n",
> drv->name);
It is not necessary to execute the WARN() under the spinlock.
> - return;
> + goto out;
> }
>
> - spin_lock(&cpuidle_driver_lock);
> -
> if (!WARN_ON(drv->refcnt > 0))
> cpuidle_curr_driver = NULL;
> -
> +out:
> spin_unlock(&cpuidle_driver_lock);
> }
> EXPORT_SYMBOL_GPL(cpuidle_unregister_driver);
Thanks,
Rafael
--
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.
_______________________________________________
linaro-dev mailing list
linaro-dev@lists.linaro.org
http://lists.linaro.org/mailman/listinfo/linaro-dev
