ALSA: line6: Reorganize PCM stream handling

The current code deals with the stream start / stop solely via
line6_pcm_acquire() and line6_pcm_release().  This was (supposedly)
intended to avoid the races, but it doesn't work as expected.  The
concurrent acquire and release calls can be performed without proper
protections, thus this might result in memory corruption.
Furthermore, we can't take a mutex to protect the whole function
because it can be called from the PCM trigger callback that is an
atomic context.  Also spinlock isn't appropriate because the function
allocates with kmalloc with GFP_KERNEL.  That is, these function just
lead to singular problems.

This is an attempt to reduce the existing races.  First off, separate
both the stream buffer management and the stream URB management.  The
former is protected via a newly introduced state_mutex while the
latter is protected via each line6_pcm_stream lock.

Secondly, the stream state are now managed in opened and running bit
flags of each line6_pcm_stream.  Not only this a bit clearer than
previous combined bit flags, this also gives a better abstraction.
These rewrites allows us to make common hw_params and hw_free
callbacks for both playback and capture directions.

For the monitor and impulse operations, still line6_pcm_acquire() and
line6_pcm_release() are used.  They call internally the corresponding
functions for both playback and capture streams with proper lock or
mutex.  Unlike the previous versions, these function don't take the
bit masks but the only single type value.  Also they are supposed to
be applied only as duplex operations.

Tested-by: Chris Rorvick <chris@rorvick.com>
Signed-off-by: Takashi Iwai <tiwai@suse.de>

1 files changed, 187 insertions, 146 deletions

diff --git a/sound/usb/line6/pcm.c b/sound/usb/line6/pcm.c
index dedbe24cbf60..470fc1049d54 100644
--- a/sound/usb/line6/pcm.c
+++ b/sound/usb/line6/pcm.c
@@ -51,9 +51,9 @@ static int snd_line6_impulse_volume_put(struct snd_kcontrol *kcontrol,
 
 	line6pcm->impulse_volume = value;
 	if (value > 0)
-		line6_pcm_acquire(line6pcm, LINE6_BITS_PCM_IMPULSE);
+		line6_pcm_acquire(line6pcm, LINE6_STREAM_IMPULSE);
 	else
-		line6_pcm_release(line6pcm, LINE6_BITS_PCM_IMPULSE);
+		line6_pcm_release(line6pcm, LINE6_STREAM_IMPULSE);
 	return 1;
 }
 
@@ -132,176 +132,226 @@ static void line6_wait_clear_audio_urbs(struct snd_line6_pcm *line6pcm,
 			"timeout: still %d active urbs..\n", alive);
 }
 
-static int line6_alloc_stream_buffer(struct snd_line6_pcm *line6pcm,
-				     struct line6_pcm_stream *pcms)
+static inline struct line6_pcm_stream *
+get_stream(struct snd_line6_pcm *line6pcm, int direction)
 {
-	/* Invoked multiple times in a row so allocate once only */
-	if (pcms->buffer)
-		return 0;
-
-	pcms->buffer = kmalloc(LINE6_ISO_BUFFERS * LINE6_ISO_PACKETS *
-			       line6pcm->max_packet_size, GFP_KERNEL);
-	if (!pcms->buffer)
-		return -ENOMEM;
-	return 0;
+	return (direction == SNDRV_PCM_STREAM_PLAYBACK) ?
+		&line6pcm->out : &line6pcm->in;
 }
 
-static void line6_free_stream_buffer(struct snd_line6_pcm *line6pcm,
-				     struct line6_pcm_stream *pcms)
+/* allocate a buffer if not opened yet;
+ * call this in line6pcm.state_change mutex
+ */
+static int line6_buffer_acquire(struct snd_line6_pcm *line6pcm,
+				struct line6_pcm_stream *pstr, int type)
 {
-	kfree(pcms->buffer);
-	pcms->buffer = NULL;
+	/* Invoked multiple times in a row so allocate once only */
+	if (!test_and_set_bit(type, &pstr->opened) && !pstr->buffer) {
+		pstr->buffer = kmalloc(LINE6_ISO_BUFFERS * LINE6_ISO_PACKETS *
+				       line6pcm->max_packet_size, GFP_KERNEL);
+		if (!pstr->buffer)
+			return -ENOMEM;
+	}
+	return 0;
 }
 
-static bool test_flags(unsigned long flags0, unsigned long flags1,
-		       unsigned long mask)
+/* free a buffer if all streams are closed;
+ * call this in line6pcm.state_change mutex
+ */
+static void line6_buffer_release(struct snd_line6_pcm *line6pcm,
+				 struct line6_pcm_stream *pstr, int type)
 {
-	return ((flags0 & mask) == 0) && ((flags1 & mask) != 0);
+
+	clear_bit(type, &pstr->opened);
+	if (!pstr->opened) {
+		line6_wait_clear_audio_urbs(line6pcm, pstr);
+		kfree(pstr->buffer);
+		pstr->buffer = NULL;
+	}
 }
 
-int line6_pcm_acquire(struct snd_line6_pcm *line6pcm, int channels)
+/* start a PCM stream */
+static int line6_stream_start(struct snd_line6_pcm *line6pcm, int direction,
+			      int type)
 {
-	unsigned long flags_old, flags_new, flags_final;
-	int err;
-
-	do {
-		flags_old = ACCESS_ONCE(line6pcm->flags);
-		flags_new = flags_old | channels;
-	} while (cmpxchg(&line6pcm->flags, flags_old, flags_new) != flags_old);
-
-	flags_final = 0;
+	unsigned long flags;
+	struct line6_pcm_stream *pstr = get_stream(line6pcm, direction);
+	int ret = 0;
+
+	spin_lock_irqsave(&pstr->lock, flags);
+	if (!test_and_set_bit(type, &pstr->running)) {
+		if (pstr->active_urbs || pstr->unlink_urbs) {
+			ret = -EBUSY;
+			goto error;
+		}
 
-	if (test_flags(flags_old, flags_new, LINE6_BITS_CAPTURE_BUFFER)) {
-		err = line6_alloc_stream_buffer(line6pcm, &line6pcm->in);
-		if (err < 0)
-			goto pcm_acquire_error;
-		flags_final |= channels & LINE6_BITS_CAPTURE_BUFFER;
+		pstr->count = 0;
+		/* Submit all currently available URBs */
+		if (direction == SNDRV_PCM_STREAM_PLAYBACK)
+			ret = line6_submit_audio_out_all_urbs(line6pcm);
+		else
+			ret = line6_submit_audio_in_all_urbs(line6pcm);
 	}
+ error:
+	if (ret < 0)
+		clear_bit(type, &pstr->running);
+	spin_unlock_irqrestore(&pstr->lock, flags);
+	return ret;
+}
 
-	if (test_flags(flags_old, flags_new, LINE6_BITS_CAPTURE_STREAM)) {
-		/*
-		   Waiting for completion of active URBs in the stop handler is
-		   a bug, we therefore report an error if capturing is restarted
-		   too soon.
-		 */
-		if (line6pcm->in.active_urbs || line6pcm->in.unlink_urbs) {
-			dev_err(line6pcm->line6->ifcdev, "Device not yet ready\n");
-			err = -EBUSY;
-			goto pcm_acquire_error;
+/* stop a PCM stream; this doesn't sync with the unlinked URBs */
+static void line6_stream_stop(struct snd_line6_pcm *line6pcm, int direction,
+			  int type)
+{
+	unsigned long flags;
+	struct line6_pcm_stream *pstr = get_stream(line6pcm, direction);
+
+	spin_lock_irqsave(&pstr->lock, flags);
+	clear_bit(type, &pstr->running);
+	if (!pstr->running) {
+		line6_unlink_audio_urbs(line6pcm, pstr);
+		if (direction == SNDRV_PCM_STREAM_CAPTURE) {
+			line6pcm->prev_fbuf = NULL;
+			line6pcm->prev_fsize = 0;
 		}
+	}
+	spin_unlock_irqrestore(&pstr->lock, flags);
+}
 
-		line6pcm->in.count = 0;
-		err = line6_submit_audio_in_all_urbs(line6pcm);
+/* common PCM trigger callback */
+int snd_line6_trigger(struct snd_pcm_substream *substream, int cmd)
+{
+	struct snd_line6_pcm *line6pcm = snd_pcm_substream_chip(substream);
+	struct snd_pcm_substream *s;
+	int err;
 
-		if (err < 0)
-			goto pcm_acquire_error;
+	clear_bit(LINE6_FLAG_PREPARED, &line6pcm->flags);
 
-		flags_final |= channels & LINE6_BITS_CAPTURE_STREAM;
-	}
+	snd_pcm_group_for_each_entry(s, substream) {
+		if (s->pcm->card != substream->pcm->card)
+			continue;
 
-	if (test_flags(flags_old, flags_new, LINE6_BITS_PLAYBACK_BUFFER)) {
-		err = line6_alloc_stream_buffer(line6pcm, &line6pcm->out);
-		if (err < 0)
-			goto pcm_acquire_error;
-		flags_final |= channels & LINE6_BITS_PLAYBACK_BUFFER;
-	}
+		switch (cmd) {
+		case SNDRV_PCM_TRIGGER_START:
+		case SNDRV_PCM_TRIGGER_RESUME:
+			err = line6_stream_start(line6pcm, s->stream,
+						 LINE6_STREAM_PCM);
+			if (err < 0)
+				return err;
+			break;
 
-	if (test_flags(flags_old, flags_new, LINE6_BITS_PLAYBACK_STREAM)) {
-		/*
-		  See comment above regarding PCM restart.
-		*/
-		if (line6pcm->out.active_urbs || line6pcm->out.unlink_urbs) {
-			dev_err(line6pcm->line6->ifcdev, "Device not yet ready\n");
-			return -EBUSY;
-		}
+		case SNDRV_PCM_TRIGGER_STOP:
+		case SNDRV_PCM_TRIGGER_SUSPEND:
+			line6_stream_stop(line6pcm, s->stream,
+					  LINE6_STREAM_PCM);
+			break;
 
-		line6pcm->out.count = 0;
-		err = line6_submit_audio_out_all_urbs(line6pcm);
+		case SNDRV_PCM_TRIGGER_PAUSE_PUSH:
+			if (s->stream != SNDRV_PCM_STREAM_PLAYBACK)
+				return -EINVAL;
+			set_bit(LINE6_FLAG_PAUSE_PLAYBACK, &line6pcm->flags);
+			break;
 
-		if (err < 0)
-			goto pcm_acquire_error;
+		case SNDRV_PCM_TRIGGER_PAUSE_RELEASE:
+			if (s->stream != SNDRV_PCM_STREAM_PLAYBACK)
+				return -EINVAL;
+			clear_bit(LINE6_FLAG_PAUSE_PLAYBACK, &line6pcm->flags);
+			break;
 
-		flags_final |= channels & LINE6_BITS_PLAYBACK_STREAM;
+		default:
+			return -EINVAL;
+		}
 	}
 
 	return 0;
-
-pcm_acquire_error:
-	/*
-	   If not all requested resources/streams could be obtained, release
-	   those which were successfully obtained (if any).
-	*/
-	line6_pcm_release(line6pcm, flags_final);
-	return err;
 }
-EXPORT_SYMBOL_GPL(line6_pcm_acquire);
 
-int line6_pcm_release(struct snd_line6_pcm *line6pcm, int channels)
+/* Acquire and start duplex streams:
+ * type is either LINE6_STREAM_IMPULSE or LINE6_STREAM_MONITOR
+ */
+int line6_pcm_acquire(struct snd_line6_pcm *line6pcm, int type)
 {
-	unsigned long flags_old, flags_new;
-
-	do {
-		flags_old = ACCESS_ONCE(line6pcm->flags);
-		flags_new = flags_old & ~channels;
-	} while (cmpxchg(&line6pcm->flags, flags_old, flags_new) != flags_old);
-
-	if (test_flags(flags_new, flags_old, LINE6_BITS_CAPTURE_STREAM)) {
-		line6_unlink_audio_urbs(line6pcm, &line6pcm->in);
-		line6pcm->prev_fbuf = NULL;
-		line6pcm->prev_fsize = 0;
+	struct line6_pcm_stream *pstr;
+	int ret = 0, dir;
+
+	mutex_lock(&line6pcm->state_mutex);
+	for (dir = 0; dir < 2; dir++) {
+		pstr = get_stream(line6pcm, dir);
+		ret = line6_buffer_acquire(line6pcm, pstr, type);
+		if (ret < 0)
+			goto error;
+		if (!pstr->running)
+			line6_wait_clear_audio_urbs(line6pcm, pstr);
 	}
-
-	if (test_flags(flags_new, flags_old, LINE6_BITS_CAPTURE_BUFFER)) {
-		line6_wait_clear_audio_urbs(line6pcm, &line6pcm->in);
-		line6_free_stream_buffer(line6pcm, &line6pcm->in);
+	for (dir = 0; dir < 2; dir++) {
+		ret = line6_stream_start(line6pcm, dir, type);
+		if (ret < 0)
+			goto error;
 	}
+ error:
+	mutex_unlock(&line6pcm->state_mutex);
+	if (ret < 0)
+		line6_pcm_release(line6pcm, type);
+	return ret;
+}
+EXPORT_SYMBOL_GPL(line6_pcm_acquire);
 
-	if (test_flags(flags_new, flags_old, LINE6_BITS_PLAYBACK_STREAM))
-		line6_unlink_audio_urbs(line6pcm, &line6pcm->out);
-
-	if (test_flags(flags_new, flags_old, LINE6_BITS_PLAYBACK_BUFFER)) {
-		line6_wait_clear_audio_urbs(line6pcm, &line6pcm->out);
-		line6_free_stream_buffer(line6pcm, &line6pcm->out);
+/* Stop and release duplex streams */
+void line6_pcm_release(struct snd_line6_pcm *line6pcm, int type)
+{
+	struct line6_pcm_stream *pstr;
+	int dir;
+
+	mutex_lock(&line6pcm->state_mutex);
+	for (dir = 0; dir < 2; dir++)
+		line6_stream_stop(line6pcm, dir, type);
+	for (dir = 0; dir < 2; dir++) {
+		pstr = get_stream(line6pcm, dir);
+		line6_buffer_release(line6pcm, pstr, type);
 	}
-
-	return 0;
+	mutex_unlock(&line6pcm->state_mutex);
 }
 EXPORT_SYMBOL_GPL(line6_pcm_release);
 
-/* trigger callback */
-int snd_line6_trigger(struct snd_pcm_substream *substream, int cmd)
+/* common PCM hw_params callback */
+int snd_line6_hw_params(struct snd_pcm_substream *substream,
+			struct snd_pcm_hw_params *hw_params)
 {
+	int ret;
 	struct snd_line6_pcm *line6pcm = snd_pcm_substream_chip(substream);
-	struct snd_pcm_substream *s;
-	int err = 0;
-
-	clear_bit(LINE6_INDEX_PREPARED, &line6pcm->flags);
-
-	snd_pcm_group_for_each_entry(s, substream) {
-		if (s->pcm->card != substream->pcm->card)
-			continue;
-		switch (s->stream) {
-		case SNDRV_PCM_STREAM_PLAYBACK:
-			err = snd_line6_playback_trigger(line6pcm, cmd);
-			break;
+	struct line6_pcm_stream *pstr = get_stream(line6pcm, substream->stream);
+
+	mutex_lock(&line6pcm->state_mutex);
+	ret = line6_buffer_acquire(line6pcm, pstr, LINE6_STREAM_PCM);
+	if (ret < 0)
+		goto error;
+
+	ret = snd_pcm_lib_malloc_pages(substream,
+				       params_buffer_bytes(hw_params));
+	if (ret < 0) {
+		line6_buffer_release(line6pcm, pstr, LINE6_STREAM_PCM);
+		goto error;
+	}
 
-		case SNDRV_PCM_STREAM_CAPTURE:
-			err = snd_line6_capture_trigger(line6pcm, cmd);
-			break;
+	pstr->period = params_period_bytes(hw_params);
+ error:
+	mutex_unlock(&line6pcm->state_mutex);
+	return ret;
+}
 
-		default:
-			dev_err(line6pcm->line6->ifcdev,
-				"Unknown stream direction %d\n", s->stream);
-			err = -EINVAL;
-			break;
-		}
-		if (err < 0)
-			break;
-	}
+/* common PCM hw_free callback */
+int snd_line6_hw_free(struct snd_pcm_substream *substream)
+{
+	struct snd_line6_pcm *line6pcm = snd_pcm_substream_chip(substream);
+	struct line6_pcm_stream *pstr = get_stream(line6pcm, substream->stream);
 
-	return err;
+	mutex_lock(&line6pcm->state_mutex);
+	line6_buffer_release(line6pcm, pstr, LINE6_STREAM_PCM);
+	mutex_unlock(&line6pcm->state_mutex);
+	return snd_pcm_lib_free_pages(substream);
 }
 
+
 /* control info callback */
 static int snd_line6_control_playback_info(struct snd_kcontrol *kcontrol,
 					   struct snd_ctl_elem_info *uinfo)
@@ -454,6 +504,7 @@ int line6_init_pcm(struct usb_line6 *line6,
 	if (!line6pcm)
 		return -ENOMEM;
 
+	mutex_init(&line6pcm->state_mutex);
 	line6pcm->pcm = pcm;
 	line6pcm->properties = properties;
 	line6pcm->volume_playback[0] = line6pcm->volume_playback[1] = 255;
@@ -500,24 +551,13 @@ EXPORT_SYMBOL_GPL(line6_init_pcm);
 int snd_line6_prepare(struct snd_pcm_substream *substream)
 {
 	struct snd_line6_pcm *line6pcm = snd_pcm_substream_chip(substream);
+	struct line6_pcm_stream *pstr = get_stream(line6pcm, substream->stream);
 
-	switch (substream->stream) {
-	case SNDRV_PCM_STREAM_PLAYBACK:
-		if ((line6pcm->flags & LINE6_BITS_PLAYBACK_STREAM) == 0) {
-			line6_unlink_audio_urbs(line6pcm, &line6pcm->out);
-			line6_wait_clear_audio_urbs(line6pcm, &line6pcm->out);
-		}
-		break;
-
-	case SNDRV_PCM_STREAM_CAPTURE:
-		if ((line6pcm->flags & LINE6_BITS_CAPTURE_STREAM) == 0) {
-			line6_unlink_audio_urbs(line6pcm, &line6pcm->in);
-			line6_wait_clear_audio_urbs(line6pcm, &line6pcm->in);
-		}
-		break;
-	}
+	mutex_lock(&line6pcm->state_mutex);
+	if (!pstr->running)
+		line6_wait_clear_audio_urbs(line6pcm, pstr);
 
-	if (!test_and_set_bit(LINE6_INDEX_PREPARED, &line6pcm->flags)) {
+	if (!test_and_set_bit(LINE6_FLAG_PREPARED, &line6pcm->flags)) {
 		line6pcm->out.count = 0;
 		line6pcm->out.pos = 0;
 		line6pcm->out.pos_done = 0;
@@ -527,5 +567,6 @@ int snd_line6_prepare(struct snd_pcm_substream *substream)
 		line6pcm->in.bytes = 0;
 	}
 
+	mutex_unlock(&line6pcm->state_mutex);
 	return 0;
 }