On Sat, 23 Apr 2016 15:40:21 +0200, Dmitry Vyukov wrote:
Hi Takashi,
I've incorporated your hrtimer fixes (but also updated to ddce192106e4f984123884f8e878f66ace94b573) and now I am seeing lots of the following deadlock messages:
[ INFO: possible circular locking dependency detected ] 4.6.0-rc4+ #351 Not tainted
swapper/0/0 is trying to acquire lock: (&(&timer->lock)->rlock){-.-...}, at: [<ffffffff8537a749>] snd_timer_interrupt+0xa9/0xd30 sound/core/timer.c:701
but task is already holding lock: (&(&stime->lock)->rlock){-.....}, at: [<ffffffff85383d3f>] snd_hrtimer_callback+0x4f/0x2b0 sound/core/hrtimer.c:54
which lock already depends on the new lock.
Oh crap, my second patch is buggy, it leads to ABBA lock, indeed. The first patch is still OK, as it just adds a new behavior mode.
Could you replace the second patch with the below one?
thanks,
Takashi
-- 8< -- From: Takashi Iwai tiwai@suse.de Subject: [PATCH] ALSA: hrtimer: Use manual start/stop in callback (v2)
With the new SNDRV_TIMER_HW_RET_CTRL flag, hrtimer can manage the callback behavior more correctly. Now it gets a return value from snd_timer_interrupt() whether to reprogram or stop the timer, and it can choose the right return value.
Thanks to this change, we can revert the long-standing hack to work around the hrtimer_cancel() call at stopping. Since the stop is called only explicitly outside the callback, hrtimer_cancel() can be called safely now.
This fix essentially reverts the old commit [fcfdebe70759: ALSA: hrtimer - Fix lock-up] while converting to use the new framework with SNDRV_TIMER_HW_RET_CTRL.
Signed-off-by: Takashi Iwai tiwai@suse.de --- sound/core/hrtimer.c | 21 +++++++-------------- 1 file changed, 7 insertions(+), 14 deletions(-)
diff --git a/sound/core/hrtimer.c b/sound/core/hrtimer.c index 656d9a9032dc..27e2493c94e4 100644 --- a/sound/core/hrtimer.c +++ b/sound/core/hrtimer.c @@ -38,7 +38,6 @@ static unsigned int resolution; struct snd_hrtimer { struct snd_timer *timer; struct hrtimer hrt; - atomic_t running; };
static enum hrtimer_restart snd_hrtimer_callback(struct hrtimer *hrt) @@ -46,14 +45,11 @@ static enum hrtimer_restart snd_hrtimer_callback(struct hrtimer *hrt) struct snd_hrtimer *stime = container_of(hrt, struct snd_hrtimer, hrt); struct snd_timer *t = stime->timer; unsigned long oruns; - - if (!atomic_read(&stime->running)) - return HRTIMER_NORESTART; + int ret;
oruns = hrtimer_forward_now(hrt, ns_to_ktime(t->sticks * resolution)); - snd_timer_interrupt(stime->timer, t->sticks * oruns); - - if (!atomic_read(&stime->running)) + ret = snd_timer_interrupt(stime->timer, t->sticks * oruns); + if (ret == SNDRV_TIMER_RET_STOP) return HRTIMER_NORESTART; return HRTIMER_RESTART; } @@ -68,7 +64,6 @@ static int snd_hrtimer_open(struct snd_timer *t) hrtimer_init(&stime->hrt, CLOCK_MONOTONIC, HRTIMER_MODE_REL); stime->timer = t; stime->hrt.function = snd_hrtimer_callback; - atomic_set(&stime->running, 0); t->private_data = stime; return 0; } @@ -89,24 +84,22 @@ static int snd_hrtimer_start(struct snd_timer *t) { struct snd_hrtimer *stime = t->private_data;
- atomic_set(&stime->running, 0); - hrtimer_try_to_cancel(&stime->hrt); hrtimer_start(&stime->hrt, ns_to_ktime(t->sticks * resolution), HRTIMER_MODE_REL); - atomic_set(&stime->running, 1); return 0; }
static int snd_hrtimer_stop(struct snd_timer *t) { struct snd_hrtimer *stime = t->private_data; - atomic_set(&stime->running, 0); - hrtimer_try_to_cancel(&stime->hrt); + + hrtimer_cancel(&stime->hrt); return 0; }
static struct snd_timer_hardware hrtimer_hw = { - .flags = SNDRV_TIMER_HW_AUTO | SNDRV_TIMER_HW_TASKLET, + .flags = SNDRV_TIMER_HW_AUTO | SNDRV_TIMER_HW_TASKLET | + SNDRV_TIMER_HW_RET_CTRL, .open = snd_hrtimer_open, .close = snd_hrtimer_close, .start = snd_hrtimer_start,