[alsa-devel] [PATCH 2/2] ALSA: hrtimer: Use manual start/stop in callback

[Takashi Iwai]

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.

The biggest bonus by this change is that we can protect the whole
interrupt with a spinlock against start/stop calls from another
thread.  The patch adds a spinlock and converts the former atomic flag
into a normal bool flag.

Hopefully this fixes the still remaining races spotted (but fairly
rarely) by syzkaller fuzzer.

Signed-off-by: Takashi Iwai <tiwai at suse.de>
---
 sound/core/hrtimer.c | 51 ++++++++++++++++++++++++++++++++++++++-------------
 1 file changed, 38 insertions(+), 13 deletions(-)

diff --git a/sound/core/hrtimer.c b/sound/core/hrtimer.c
index 656d9a9032dc..8a1b51473fd5 100644
--- a/sound/core/hrtimer.c
+++ b/sound/core/hrtimer.c
@@ -38,7 +38,8 @@ static unsigned int resolution;
 struct snd_hrtimer {
 	struct snd_timer *timer;
 	struct hrtimer hrt;
-	atomic_t running;
+	spinlock_t lock;
+	bool running;
 };
 
 static enum hrtimer_restart snd_hrtimer_callback(struct hrtimer *hrt)
@@ -46,16 +47,33 @@ 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;
+	int irq_ret;
+	enum hrtimer_restart ret = HRTIMER_NORESTART;
+	unsigned long flags;
 
-	if (!atomic_read(&stime->running))
-		return HRTIMER_NORESTART;
+	spin_lock_irqsave(&stime->lock, flags);
+	if (!stime->running)
+		goto unlock;
 
 	oruns = hrtimer_forward_now(hrt, ns_to_ktime(t->sticks * resolution));
-	snd_timer_interrupt(stime->timer, t->sticks * oruns);
+	irq_ret = snd_timer_interrupt(stime->timer, t->sticks * oruns);
+
+	switch (irq_ret) {
+	case SNDRV_TIMER_RET_START:
+		hrtimer_start(&stime->hrt, ns_to_ktime(t->sticks * resolution),
+			      HRTIMER_MODE_REL);
+		/* fallthru */
+	case SNDRV_TIMER_RET_NONE:
+		ret = HRTIMER_RESTART;
+		break;
+	default:
+		stime->running = false;
+		break; /* HRTIMER_NORESTART */
+	}
 
-	if (!atomic_read(&stime->running))
-		return HRTIMER_NORESTART;
-	return HRTIMER_RESTART;
+ unlock:
+	spin_unlock_irqrestore(&stime->lock, flags);
+	return  ret;
 }
 
 static int snd_hrtimer_open(struct snd_timer *t)
@@ -68,7 +86,8 @@ 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);
+	spin_lock_init(&stime->lock);
+	stime->running = false;
 	t->private_data = stime;
 	return 0;
 }
@@ -88,25 +107,31 @@ static int snd_hrtimer_close(struct snd_timer *t)
 static int snd_hrtimer_start(struct snd_timer *t)
 {
 	struct snd_hrtimer *stime = t->private_data;
+	unsigned long flags;
 
-	atomic_set(&stime->running, 0);
-	hrtimer_try_to_cancel(&stime->hrt);
+	spin_lock_irqsave(&stime->lock, flags);
 	hrtimer_start(&stime->hrt, ns_to_ktime(t->sticks * resolution),
 		      HRTIMER_MODE_REL);
-	atomic_set(&stime->running, 1);
+	stime->running = true;
+	spin_unlock_irqrestore(&stime->lock, flags);
 	return 0;
 }
 
 static int snd_hrtimer_stop(struct snd_timer *t)
 {
 	struct snd_hrtimer *stime = t->private_data;
-	atomic_set(&stime->running, 0);
+	unsigned long flags;
+
+	spin_lock_irqsave(&stime->lock, flags);
 	hrtimer_try_to_cancel(&stime->hrt);
+	stime->running = false;
+	spin_unlock_irqrestore(&stime->lock, flags);
 	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,
-- 
2.8.1