| Message ID | 1311627344-8097-1-git-send-email-ccross@android.com (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
On Monday, July 25, 2011, Colin Cross wrote: > Some of the entry points to pm runtime are not safe to > call in atomic context unless pm_runtime_irq_safe() has > been called. Inspecting the code, it is not immediately > obvious that the functions sleep at all, as they run > inside a spin_lock_irqsave, but under some conditions > they can drop the lock and turn on irqs. > > If a driver incorrectly calls the pm_runtime apis, it can > cause sleeping and irq processing when it expects to stay > in atomic context. > > Add might_sleep_if to all the __pm_runtime_* entry points > to enforce correct usage. > > Add pm_runtime_put_sync_autosuspend to the list of > functions that can be called in atomic context. > > Signed-off-by: Colin Cross <ccross@android.com> > --- > Documentation/power/runtime_pm.txt | 1 + > drivers/base/power/runtime.c | 15 ++++++++++++--- > 2 files changed, 13 insertions(+), 3 deletions(-) > > diff --git a/Documentation/power/runtime_pm.txt b/Documentation/power/runtime_pm.txt > index c291233..1ad507c 100644 > --- a/Documentation/power/runtime_pm.txt > +++ b/Documentation/power/runtime_pm.txt > @@ -469,6 +469,7 @@ pm_runtime_resume() > pm_runtime_get_sync() > pm_runtime_put_sync() > pm_runtime_put_sync_suspend() > +pm_runtime_put_sync_autosuspend() > > 5. Run-time PM Initialization, Device Probing and Removal > > diff --git a/drivers/base/power/runtime.c b/drivers/base/power/runtime.c > index 2e746f8..f3d8583 100644 > --- a/drivers/base/power/runtime.c > +++ b/drivers/base/power/runtime.c > @@ -731,13 +731,16 @@ EXPORT_SYMBOL_GPL(pm_schedule_suspend); > * return immediately if it is larger than zero. Then carry out an idle > * notification, either synchronous or asynchronous. > * > - * This routine may be called in atomic context if the RPM_ASYNC flag is set. > + * This routine may be called in atomic context if the RPM_ASYNC flag is set, > + * or if pm_runtime_irq_safe() has been called. > */ > int __pm_runtime_idle(struct device *dev, int rpmflags) > { > unsigned long flags; > int retval; > > + might_sleep_if(!(rpmflags & RPM_ASYNC) && !dev->power.irq_safe); > + Now that I think of it, perhaps it's better to put the might_sleep() annotations into the actual code paths that should trigger them instead of checking the conditions upfront on every call? This way we'll avoid quite some overhead that's only necessary for debugging. Thanks, Rafael
On Tue, Jul 26, 2011 at 3:14 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote: > On Monday, July 25, 2011, Colin Cross wrote: >> Some of the entry points to pm runtime are not safe to >> call in atomic context unless pm_runtime_irq_safe() has >> been called. Inspecting the code, it is not immediately >> obvious that the functions sleep at all, as they run >> inside a spin_lock_irqsave, but under some conditions >> they can drop the lock and turn on irqs. >> >> If a driver incorrectly calls the pm_runtime apis, it can >> cause sleeping and irq processing when it expects to stay >> in atomic context. >> >> Add might_sleep_if to all the __pm_runtime_* entry points >> to enforce correct usage. >> >> Add pm_runtime_put_sync_autosuspend to the list of >> functions that can be called in atomic context. >> >> Signed-off-by: Colin Cross <ccross@android.com> >> --- >> Documentation/power/runtime_pm.txt | 1 + >> drivers/base/power/runtime.c | 15 ++++++++++++--- >> 2 files changed, 13 insertions(+), 3 deletions(-) >> >> diff --git a/Documentation/power/runtime_pm.txt b/Documentation/power/runtime_pm.txt >> index c291233..1ad507c 100644 >> --- a/Documentation/power/runtime_pm.txt >> +++ b/Documentation/power/runtime_pm.txt >> @@ -469,6 +469,7 @@ pm_runtime_resume() >> pm_runtime_get_sync() >> pm_runtime_put_sync() >> pm_runtime_put_sync_suspend() >> +pm_runtime_put_sync_autosuspend() >> >> 5. Run-time PM Initialization, Device Probing and Removal >> >> diff --git a/drivers/base/power/runtime.c b/drivers/base/power/runtime.c >> index 2e746f8..f3d8583 100644 >> --- a/drivers/base/power/runtime.c >> +++ b/drivers/base/power/runtime.c >> @@ -731,13 +731,16 @@ EXPORT_SYMBOL_GPL(pm_schedule_suspend); >> * return immediately if it is larger than zero. Then carry out an idle >> * notification, either synchronous or asynchronous. >> * >> - * This routine may be called in atomic context if the RPM_ASYNC flag is set. >> + * This routine may be called in atomic context if the RPM_ASYNC flag is set, >> + * or if pm_runtime_irq_safe() has been called. >> */ >> int __pm_runtime_idle(struct device *dev, int rpmflags) >> { >> unsigned long flags; >> int retval; >> >> + might_sleep_if(!(rpmflags & RPM_ASYNC) && !dev->power.irq_safe); >> + > > Now that I think of it, perhaps it's better to put the might_sleep() > annotations into the actual code paths that should trigger them instead of > checking the conditions upfront on every call? This way we'll avoid quite > some overhead that's only necessary for debugging. > You can't put the might_sleep after the spin_lock_irqsave(), because you are always in atomic context, and you can't put it after the spin_unlock_irq() that triggers the problem because you have already unconditionally left atomic context. Anyways, the sleeps happen in a farily rare case, so putting the might_sleep in a more specific location will hide the errors when developers perform simple tests. For example, every kmalloc ends up calling might_sleep_if(flags & __GFP_WAIT), so that putting kmalloc(..., GFP_KERNEL) will print a stack trace every time, instead of only the very rare case when kmalloc has to block in a low memory condition. The calls are very low overhead - the condition in the might_sleep_if(), and then in the common case: if ((preempt_count_equals(preempt_offset) && !irqs_disabled()) || ...) return;
Colin Cross <ccross@android.com> writes: > Some of the entry points to pm runtime are not safe to > call in atomic context unless pm_runtime_irq_safe() has > been called. Inspecting the code, it is not immediately > obvious that the functions sleep at all, as they run > inside a spin_lock_irqsave, but under some conditions > they can drop the lock and turn on irqs. > > If a driver incorrectly calls the pm_runtime apis, it can > cause sleeping and irq processing when it expects to stay > in atomic context. > > Add might_sleep_if to all the __pm_runtime_* entry points > to enforce correct usage. Minor: s/all/most of/, or something similar since in v2, it doesn't annotate all of the functions anymore, just the main ones likely to be (mis)used by drivers. Other than that, looks good... > Add pm_runtime_put_sync_autosuspend to the list of > functions that can be called in atomic context. > > Signed-off-by: Colin Cross <ccross@android.com> Reviewed-by: Kevin Hilman <khilman@ti.com> Kevin
On Wednesday, July 27, 2011, Colin Cross wrote: > On Tue, Jul 26, 2011 at 3:14 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote: > > On Monday, July 25, 2011, Colin Cross wrote: > >> Some of the entry points to pm runtime are not safe to > >> call in atomic context unless pm_runtime_irq_safe() has > >> been called. Inspecting the code, it is not immediately > >> obvious that the functions sleep at all, as they run > >> inside a spin_lock_irqsave, but under some conditions > >> they can drop the lock and turn on irqs. > >> > >> If a driver incorrectly calls the pm_runtime apis, it can > >> cause sleeping and irq processing when it expects to stay > >> in atomic context. > >> > >> Add might_sleep_if to all the __pm_runtime_* entry points > >> to enforce correct usage. > >> > >> Add pm_runtime_put_sync_autosuspend to the list of > >> functions that can be called in atomic context. > >> > >> Signed-off-by: Colin Cross <ccross@android.com> > >> --- > >> Documentation/power/runtime_pm.txt | 1 + > >> drivers/base/power/runtime.c | 15 ++++++++++++--- > >> 2 files changed, 13 insertions(+), 3 deletions(-) > >> > >> diff --git a/Documentation/power/runtime_pm.txt b/Documentation/power/runtime_pm.txt > >> index c291233..1ad507c 100644 > >> --- a/Documentation/power/runtime_pm.txt > >> +++ b/Documentation/power/runtime_pm.txt > >> @@ -469,6 +469,7 @@ pm_runtime_resume() > >> pm_runtime_get_sync() > >> pm_runtime_put_sync() > >> pm_runtime_put_sync_suspend() > >> +pm_runtime_put_sync_autosuspend() > >> > >> 5. Run-time PM Initialization, Device Probing and Removal > >> > >> diff --git a/drivers/base/power/runtime.c b/drivers/base/power/runtime.c > >> index 2e746f8..f3d8583 100644 > >> --- a/drivers/base/power/runtime.c > >> +++ b/drivers/base/power/runtime.c > >> @@ -731,13 +731,16 @@ EXPORT_SYMBOL_GPL(pm_schedule_suspend); > >> * return immediately if it is larger than zero. Then carry out an idle > >> * notification, either synchronous or asynchronous. > >> * > >> - * This routine may be called in atomic context if the RPM_ASYNC flag is set. > >> + * This routine may be called in atomic context if the RPM_ASYNC flag is set, > >> + * or if pm_runtime_irq_safe() has been called. > >> */ > >> int __pm_runtime_idle(struct device *dev, int rpmflags) > >> { > >> unsigned long flags; > >> int retval; > >> > >> + might_sleep_if(!(rpmflags & RPM_ASYNC) && !dev->power.irq_safe); > >> + > > > > Now that I think of it, perhaps it's better to put the might_sleep() > > annotations into the actual code paths that should trigger them instead of > > checking the conditions upfront on every call? This way we'll avoid quite > > some overhead that's only necessary for debugging. > > > > You can't put the might_sleep after the spin_lock_irqsave(), because > you are always in atomic context, and you can't put it after the > spin_unlock_irq() that triggers the problem because you have already > unconditionally left atomic context. > > Anyways, the sleeps happen in a farily rare case, so putting the > might_sleep in a more specific location will hide the errors when > developers perform simple tests. For example, every kmalloc ends up > calling might_sleep_if(flags & __GFP_WAIT), so that putting > kmalloc(..., GFP_KERNEL) will print a stack trace every time, instead > of only the very rare case when kmalloc has to block in a low memory > condition. > > The calls are very low overhead - the condition in the > might_sleep_if(), and then in the common case: > if ((preempt_count_equals(preempt_offset) && !irqs_disabled()) || ...) > return; OK, I'm going to take the $subject patch for 3.2. Thanks, Rafael
diff --git a/Documentation/power/runtime_pm.txt b/Documentation/power/runtime_pm.txt index c291233..1ad507c 100644 --- a/Documentation/power/runtime_pm.txt +++ b/Documentation/power/runtime_pm.txt @@ -469,6 +469,7 @@ pm_runtime_resume() pm_runtime_get_sync() pm_runtime_put_sync() pm_runtime_put_sync_suspend() +pm_runtime_put_sync_autosuspend() 5. Run-time PM Initialization, Device Probing and Removal diff --git a/drivers/base/power/runtime.c b/drivers/base/power/runtime.c index 2e746f8..f3d8583 100644 --- a/drivers/base/power/runtime.c +++ b/drivers/base/power/runtime.c @@ -731,13 +731,16 @@ EXPORT_SYMBOL_GPL(pm_schedule_suspend); * return immediately if it is larger than zero. Then carry out an idle * notification, either synchronous or asynchronous. * - * This routine may be called in atomic context if the RPM_ASYNC flag is set. + * This routine may be called in atomic context if the RPM_ASYNC flag is set, + * or if pm_runtime_irq_safe() has been called. */ int __pm_runtime_idle(struct device *dev, int rpmflags) { unsigned long flags; int retval; + might_sleep_if(!(rpmflags & RPM_ASYNC) && !dev->power.irq_safe); + if (rpmflags & RPM_GET_PUT) { if (!atomic_dec_and_test(&dev->power.usage_count)) return 0; @@ -760,13 +763,16 @@ EXPORT_SYMBOL_GPL(__pm_runtime_idle); * return immediately if it is larger than zero. Then carry out a suspend, * either synchronous or asynchronous. * - * This routine may be called in atomic context if the RPM_ASYNC flag is set. + * This routine may be called in atomic context if the RPM_ASYNC flag is set, + * or if pm_runtime_irq_safe() has been called. */ int __pm_runtime_suspend(struct device *dev, int rpmflags) { unsigned long flags; int retval; + might_sleep_if(!(rpmflags & RPM_ASYNC) && !dev->power.irq_safe); + if (rpmflags & RPM_GET_PUT) { if (!atomic_dec_and_test(&dev->power.usage_count)) return 0; @@ -788,13 +794,16 @@ EXPORT_SYMBOL_GPL(__pm_runtime_suspend); * If the RPM_GET_PUT flag is set, increment the device's usage count. Then * carry out a resume, either synchronous or asynchronous. * - * This routine may be called in atomic context if the RPM_ASYNC flag is set. + * This routine may be called in atomic context if the RPM_ASYNC flag is set, + * or if pm_runtime_irq_safe() has been called. */ int __pm_runtime_resume(struct device *dev, int rpmflags) { unsigned long flags; int retval; + might_sleep_if(!(rpmflags & RPM_ASYNC) && !dev->power.irq_safe); + if (rpmflags & RPM_GET_PUT) atomic_inc(&dev->power.usage_count);
Some of the entry points to pm runtime are not safe to call in atomic context unless pm_runtime_irq_safe() has been called. Inspecting the code, it is not immediately obvious that the functions sleep at all, as they run inside a spin_lock_irqsave, but under some conditions they can drop the lock and turn on irqs. If a driver incorrectly calls the pm_runtime apis, it can cause sleeping and irq processing when it expects to stay in atomic context. Add might_sleep_if to all the __pm_runtime_* entry points to enforce correct usage. Add pm_runtime_put_sync_autosuspend to the list of functions that can be called in atomic context. Signed-off-by: Colin Cross <ccross@android.com> --- Documentation/power/runtime_pm.txt | 1 + drivers/base/power/runtime.c | 15 ++++++++++++--- 2 files changed, 13 insertions(+), 3 deletions(-)