Re: [PATCH] PM-runtime: fix deadlock when canceling hrtimer
On Thu, 21 Feb 2019 at 10:35, Rafael J. Wysocki wrote: > > On Thu, Feb 21, 2019 at 8:59 AM Vincent Guittot > wrote: > > > > When rpm_resume() desactivates the autosuspend timer, it should only try > > to cancel hrtimer but not wait for the handler to finish because both > > rpm_resume() and pm_suspend_timer_fn() are taking the power.lock. > > We can have the deadlock sequence: > > CPU0 CPU1 > > rpm_resume() > > spin_lock_irqsave > > pm_suspend_timer_fn() > > spin_lock_irqsave > > pm_runtime_deactivate_timer() > > hrtimer_cancel() > > Everybody has missed this, not good. yes. default behavior if different between timer and hrtimer del_timer() is equivalent to hrtimer_try_to_cancel() and hrtimer_cancel() to del_timer_sync() > > > hrtimer_try_to_cancel() is enough because dev->power.timer_expires is also > > set to 0. > > > > Fixes: 8234f6734c5d ("PM-runtime: Switch autosuspend over to using > > hrtimers") > > Reported-by: Sunzhaosheng Sun(Zhaosheng) > > Signed-off-by: Vincent Guittot > > --- > > drivers/base/power/runtime.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/drivers/base/power/runtime.c b/drivers/base/power/runtime.c > > index 04407d9..78937c4 100644 > > --- a/drivers/base/power/runtime.c > > +++ b/drivers/base/power/runtime.c > > @@ -121,7 +121,7 @@ EXPORT_SYMBOL_GPL(pm_runtime_suspended_time); > > static void pm_runtime_deactivate_timer(struct device *dev) > > { > > if (dev->power.timer_expires > 0) { > > - hrtimer_cancel(>power.suspend_timer); > > + hrtimer_try_to_cancel(>power.suspend_timer); > > dev->power.timer_expires = 0; > > } > > } > > -- > > Applied with some changelog modifications, thanks! Thanks
Re: [PATCH] PM-runtime: fix deadlock when canceling hrtimer
On Thu, Feb 21, 2019 at 8:59 AM Vincent Guittot wrote: > > When rpm_resume() desactivates the autosuspend timer, it should only try > to cancel hrtimer but not wait for the handler to finish because both > rpm_resume() and pm_suspend_timer_fn() are taking the power.lock. > We can have the deadlock sequence: > CPU0 CPU1 > rpm_resume() > spin_lock_irqsave > pm_suspend_timer_fn() > spin_lock_irqsave > pm_runtime_deactivate_timer() > hrtimer_cancel() Everybody has missed this, not good. > hrtimer_try_to_cancel() is enough because dev->power.timer_expires is also > set to 0. > > Fixes: 8234f6734c5d ("PM-runtime: Switch autosuspend over to using hrtimers") > Reported-by: Sunzhaosheng Sun(Zhaosheng) > Signed-off-by: Vincent Guittot > --- > drivers/base/power/runtime.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/base/power/runtime.c b/drivers/base/power/runtime.c > index 04407d9..78937c4 100644 > --- a/drivers/base/power/runtime.c > +++ b/drivers/base/power/runtime.c > @@ -121,7 +121,7 @@ EXPORT_SYMBOL_GPL(pm_runtime_suspended_time); > static void pm_runtime_deactivate_timer(struct device *dev) > { > if (dev->power.timer_expires > 0) { > - hrtimer_cancel(>power.suspend_timer); > + hrtimer_try_to_cancel(>power.suspend_timer); > dev->power.timer_expires = 0; > } > } > -- Applied with some changelog modifications, thanks!
[PATCH] PM-runtime: fix deadlock when canceling hrtimer
When rpm_resume() desactivates the autosuspend timer, it should only try to cancel hrtimer but not wait for the handler to finish because both rpm_resume() and pm_suspend_timer_fn() are taking the power.lock. We can have the deadlock sequence: CPU0 CPU1 rpm_resume() spin_lock_irqsave pm_suspend_timer_fn() spin_lock_irqsave pm_runtime_deactivate_timer() hrtimer_cancel() hrtimer_try_to_cancel() is enough because dev->power.timer_expires is also set to 0. Fixes: 8234f6734c5d ("PM-runtime: Switch autosuspend over to using hrtimers") Reported-by: Sunzhaosheng Sun(Zhaosheng) Signed-off-by: Vincent Guittot --- drivers/base/power/runtime.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/base/power/runtime.c b/drivers/base/power/runtime.c index 04407d9..78937c4 100644 --- a/drivers/base/power/runtime.c +++ b/drivers/base/power/runtime.c @@ -121,7 +121,7 @@ EXPORT_SYMBOL_GPL(pm_runtime_suspended_time); static void pm_runtime_deactivate_timer(struct device *dev) { if (dev->power.timer_expires > 0) { - hrtimer_cancel(>power.suspend_timer); + hrtimer_try_to_cancel(>power.suspend_timer); dev->power.timer_expires = 0; } } -- 2.7.4