ALSA: aloop - Close races at restarting the stream

There are small races opened in the check of running bit and the timer
lock.  Instead of adding yet more flag, just protect the whole racy
codes with the existing cable->lock.  As a bonus, we can get rid of
timer_lock now.

Reported-and-tested-by: Omair Mohammed Abdullah <omair.m.abdullah@linux.intel.com>
Signed-off-by: Takashi Iwai <tiwai@suse.de>

1 files changed, 19 insertions, 17 deletions

diff --git a/sound/drivers/aloop.c b/sound/drivers/aloop.c
index 0fe6d64..1904046 100644
--- a/sound/drivers/aloop.c
+++ b/sound/drivers/aloop.c
@@ -120,7 +120,6 @@ struct loopback_pcm {
 	unsigned int last_drift;
 	unsigned long last_jiffies;
 	struct timer_list timer;
-	spinlock_t timer_lock;
 };
 
 static struct platform_device *devices[SNDRV_CARDS];
@@ -166,12 +165,12 @@ static inline unsigned int get_rate_shift(struct loopback_pcm *dpcm)
 	return get_setup(dpcm)->rate_shift;
 }
 
+/* call in cable->lock */
 static void loopback_timer_start(struct loopback_pcm *dpcm)
 {
 	unsigned long tick;
 	unsigned int rate_shift = get_rate_shift(dpcm);
 
-	spin_lock(&dpcm->timer_lock);
 	if (rate_shift != dpcm->pcm_rate_shift) {
 		dpcm->pcm_rate_shift = rate_shift;
 		dpcm->period_size_frac = frac_pos(dpcm, dpcm->pcm_period_size);
@@ -184,15 +183,13 @@ static void loopback_timer_start(struct loopback_pcm *dpcm)
 	tick = (tick + dpcm->pcm_bps - 1) / dpcm->pcm_bps;
 	dpcm->timer.expires = jiffies + tick;
 	add_timer(&dpcm->timer);
-	spin_unlock(&dpcm->timer_lock);
 }
 
+/* call in cable->lock */
 static inline void loopback_timer_stop(struct loopback_pcm *dpcm)
 {
-	spin_lock(&dpcm->timer_lock);
 	del_timer(&dpcm->timer);
 	dpcm->timer.expires = 0;
-	spin_unlock(&dpcm->timer_lock);
 }
 
 #define CABLE_VALID_PLAYBACK	(1 << SNDRV_PCM_STREAM_PLAYBACK)
@@ -274,8 +271,8 @@ static int loopback_trigger(struct snd_pcm_substream *substream, int cmd)
 		spin_lock(&cable->lock);	
 		cable->running |= stream;
 		cable->pause &= ~stream;
-		spin_unlock(&cable->lock);
 		loopback_timer_start(dpcm);
+		spin_unlock(&cable->lock);
 		if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK)
 			loopback_active_notify(dpcm);
 		break;
@@ -283,23 +280,23 @@ static int loopback_trigger(struct snd_pcm_substream *substream, int cmd)
 		spin_lock(&cable->lock);	
 		cable->running &= ~stream;
 		cable->pause &= ~stream;
-		spin_unlock(&cable->lock);
 		loopback_timer_stop(dpcm);
+		spin_unlock(&cable->lock);
 		if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK)
 			loopback_active_notify(dpcm);
 		break;
 	case SNDRV_PCM_TRIGGER_PAUSE_PUSH:
 		spin_lock(&cable->lock);	
 		cable->pause |= stream;
-		spin_unlock(&cable->lock);
 		loopback_timer_stop(dpcm);
+		spin_unlock(&cable->lock);
 		break;
 	case SNDRV_PCM_TRIGGER_PAUSE_RELEASE:
 		spin_lock(&cable->lock);
 		dpcm->last_jiffies = jiffies;
 		cable->pause &= ~stream;
-		spin_unlock(&cable->lock);
 		loopback_timer_start(dpcm);
+		spin_unlock(&cable->lock);
 		break;
 	default:
 		return -EINVAL;
@@ -477,6 +474,7 @@ static inline void bytepos_finish(struct loopback_pcm *dpcm,
 	dpcm->buf_pos %= dpcm->pcm_buffer_size;
 }
 
+/* call in cable->lock */
 static unsigned int loopback_pos_update(struct loopback_cable *cable)
 {
 	struct loopback_pcm *dpcm_play =
@@ -485,9 +483,7 @@ static unsigned int loopback_pos_update(struct loopback_cable *cable)
 			cable->streams[SNDRV_PCM_STREAM_CAPTURE];
 	unsigned long delta_play = 0, delta_capt = 0;
 	unsigned int running, count1, count2;
-	unsigned long flags;
 
-	spin_lock_irqsave(&cable->lock, flags);
 	running = cable->running ^ cable->pause;
 	if (running & (1 << SNDRV_PCM_STREAM_PLAYBACK)) {
 		delta_play = jiffies - dpcm_play->last_jiffies;
@@ -529,32 +525,39 @@ static unsigned int loopback_pos_update(struct loopback_cable *cable)
 	bytepos_finish(dpcm_play, count1);
 	bytepos_finish(dpcm_capt, count1);
  unlock:
-	spin_unlock_irqrestore(&cable->lock, flags);
 	return running;
 }
 
 static void loopback_timer_function(unsigned long data)
 {
 	struct loopback_pcm *dpcm = (struct loopback_pcm *)data;
-	unsigned int running;
+	unsigned long flags;
 
-	running = loopback_pos_update(dpcm->cable);
-	if (running & (1 << dpcm->substream->stream)) {
+	spin_lock_irqsave(&dpcm->cable->lock, flags);
+	if (loopback_pos_update(dpcm->cable) & (1 << dpcm->substream->stream)) {
 		loopback_timer_start(dpcm);
 		if (dpcm->period_update_pending) {
 			dpcm->period_update_pending = 0;
+			spin_unlock_irqrestore(&dpcm->cable->lock, flags);
+			/* need to unlock before calling below */
 			snd_pcm_period_elapsed(dpcm->substream);
+			return;
 		}
 	}
+	spin_unlock_irqrestore(&dpcm->cable->lock, flags);
 }
 
 static snd_pcm_uframes_t loopback_pointer(struct snd_pcm_substream *substream)
 {
 	struct snd_pcm_runtime *runtime = substream->runtime;
 	struct loopback_pcm *dpcm = runtime->private_data;
+	snd_pcm_uframes_t pos;
 
+	spin_lock(&dpcm->cable->lock);
 	loopback_pos_update(dpcm->cable);
-	return bytes_to_frames(runtime, dpcm->buf_pos);
+	pos = dpcm->buf_pos;
+	spin_unlock(&dpcm->cable->lock);
+	return bytes_to_frames(runtime, pos);
 }
 
 static struct snd_pcm_hardware loopback_pcm_hardware =
@@ -672,7 +675,6 @@ static int loopback_open(struct snd_pcm_substream *substream)
 	dpcm->substream = substream;
 	setup_timer(&dpcm->timer, loopback_timer_function,
 		    (unsigned long)dpcm);
-	spin_lock_init(&dpcm->timer_lock);
 
 	cable = loopback->cables[substream->number][dev];
 	if (!cable) {