[2/4] cpuidle : move driver checking within the lock section

Message ID 1348526634-19029-3-git-send-email-daniel.lezcano@linaro.org
State New
Headers show

Commit Message

Daniel Lezcano Sept. 24, 2012, 10:43 p.m.
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.lezcano@linaro.org>
---
 drivers/cpuidle/driver.c |    8 ++++----
 1 files changed, 4 insertions(+), 4 deletions(-)

Comments

Rafael J. Wysocki Oct. 7, 2012, 8:17 p.m. | #1
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.lezcano@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

Patch

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);
-		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);