| Message ID | 1348526634-19029-3-git-send-email-daniel.lezcano@linaro.org (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
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
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);
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(-)