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 <[email protected]>
Signed-off-by: Takashi Iwai <[email protected]>

sound/usb/line6/capture.c

@@ -67,20 +67,18 @@ static int submit_audio_in_urb(struct snd_line6_pcm *line6pcm)
 
 /*
 	Submit all currently available capture URBs.
+	must be called in line6pcm->in.lock context
 */
 int line6_submit_audio_in_all_urbs(struct snd_line6_pcm *line6pcm)
 {
-	unsigned long flags;
 	int ret = 0, i;
 
-	spin_lock_irqsave(&line6pcm->in.lock, flags);
 	for (i = 0; i < LINE6_ISO_BUFFERS; ++i) {
 		ret = submit_audio_in_urb(line6pcm);
 		if (ret < 0)
 			break;
 	}
 
-	spin_unlock_irqrestore(&line6pcm->in.lock, flags);
 	return ret;
 }
 
@@ -187,10 +185,10 @@ static void audio_in_callback(struct urb *urb)
 		line6pcm->prev_fbuf = fbuf;
 		line6pcm->prev_fsize = fsize;
 
-		if (!(line6pcm->flags & LINE6_BITS_PCM_IMPULSE))
-			if (test_bit(LINE6_INDEX_PCM_ALSA_CAPTURE_STREAM,
-				     &line6pcm->flags) && (fsize > 0))
-				line6_capture_copy(line6pcm, fbuf, fsize);
+		if (!test_bit(LINE6_STREAM_IMPULSE, &line6pcm->in.running) &&
+		    test_bit(LINE6_STREAM_PCM, &line6pcm->in.running) &&
+		    fsize > 0)
+			line6_capture_copy(line6pcm, fbuf, fsize);
 	}
 
 	clear_bit(index, &line6pcm->in.active_urbs);
@@ -201,10 +199,9 @@ static void audio_in_callback(struct urb *urb)
 	if (!shutdown) {
 		submit_audio_in_urb(line6pcm);
 
-		if (!(line6pcm->flags & LINE6_BITS_PCM_IMPULSE))
-			if (test_bit(LINE6_INDEX_PCM_ALSA_CAPTURE_STREAM,
-				     &line6pcm->flags))
-				line6_capture_check_period(line6pcm, length);
+		if (!test_bit(LINE6_STREAM_IMPULSE, &line6pcm->in.running) &&
+		    test_bit(LINE6_STREAM_PCM, &line6pcm->in.running))
+			line6_capture_check_period(line6pcm, length);
 	}
 
 	spin_unlock_irqrestore(&line6pcm->in.lock, flags);
@@ -234,71 +231,6 @@ static int snd_line6_capture_close(struct snd_pcm_substream *substream)
 	return 0;
 }
 
-/* hw_params capture callback */
-static int snd_line6_capture_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);
-
-	ret = line6_pcm_acquire(line6pcm, LINE6_BIT_PCM_ALSA_CAPTURE_BUFFER);
-
-	if (ret < 0)
-		return ret;
-
-	ret = snd_pcm_lib_malloc_pages(substream,
-				       params_buffer_bytes(hw_params));
-	if (ret < 0) {
-		line6_pcm_release(line6pcm, LINE6_BIT_PCM_ALSA_CAPTURE_BUFFER);
-		return ret;
-	}
-
-	line6pcm->in.period = params_period_bytes(hw_params);
-	return 0;
-}
-
-/* hw_free capture callback */
-static int snd_line6_capture_hw_free(struct snd_pcm_substream *substream)
-{
-	struct snd_line6_pcm *line6pcm = snd_pcm_substream_chip(substream);
-
-	line6_pcm_release(line6pcm, LINE6_BIT_PCM_ALSA_CAPTURE_BUFFER);
-	return snd_pcm_lib_free_pages(substream);
-}
-
-/* trigger callback */
-int snd_line6_capture_trigger(struct snd_line6_pcm *line6pcm, int cmd)
-{
-	int err;
-
-	switch (cmd) {
-	case SNDRV_PCM_TRIGGER_START:
-	case SNDRV_PCM_TRIGGER_RESUME:
-		err = line6_pcm_acquire(line6pcm,
-					LINE6_BIT_PCM_ALSA_CAPTURE_STREAM);
-
-		if (err < 0)
-			return err;
-
-		break;
-
-	case SNDRV_PCM_TRIGGER_STOP:
-	case SNDRV_PCM_TRIGGER_SUSPEND:
-		err = line6_pcm_release(line6pcm,
-					LINE6_BIT_PCM_ALSA_CAPTURE_STREAM);
-
-		if (err < 0)
-			return err;
-
-		break;
-
-	default:
-		return -EINVAL;
-	}
-
-	return 0;
-}
-
 /* capture pointer callback */
 static snd_pcm_uframes_t
 snd_line6_capture_pointer(struct snd_pcm_substream *substream)
@@ -313,8 +245,8 @@ struct snd_pcm_ops snd_line6_capture_ops = {
 	.open = snd_line6_capture_open,
 	.close = snd_line6_capture_close,
 	.ioctl = snd_pcm_lib_ioctl,
-	.hw_params = snd_line6_capture_hw_params,
-	.hw_free = snd_line6_capture_hw_free,
+	.hw_params = snd_line6_hw_params,
+	.hw_free = snd_line6_hw_free,
 	.prepare = snd_line6_prepare,
 	.trigger = snd_line6_trigger,
 	.pointer = snd_line6_capture_pointer,

sound/usb/line6/capture.h

@@ -25,6 +25,5 @@ extern void line6_capture_check_period(struct snd_line6_pcm *line6pcm,
 				       int length);
 extern int line6_create_audio_in_urbs(struct snd_line6_pcm *line6pcm);
 extern int line6_submit_audio_in_all_urbs(struct snd_line6_pcm *line6pcm);
-extern int snd_line6_capture_trigger(struct snd_line6_pcm *line6pcm, int cmd);
 
 #endif