[PATCH 3/3] echoaudio: Address bugs in the interrupt handling

[Mark Hills]

Distorted audio appears occasionally, affecting either playback
or capture and requiring the affected substream to be closed by
all applications and re-opened.

The best way I have found to reproduce the bug is to use dmix in
combination with Chromium, which opens the audio device multiple
times in threads. Anecdotally, the problems appear to have increased
with faster CPUs.

Since applying this patch I have not had problems, where previously
they would occur several times a day.

This patch addresses the following issues:

* Check for progress using the counter from the hardware, not after
  it has been truncated to the buffer.

  There appears to be a possible bug if a whole ringbuffer advances
  between interrupts, it goes unnoticed.

* Remove chip->last_period:

  It's trivial to derive from pipe->last_counter, and inside pipe
  is where it more logically belongs. This has less scope for bugs
  as it is not wrapped to the buffer length.

* Remove the accessing of pipe->dma_counter twice in the interrupt
  handler:

  The value will be changing between accesses. It doesn't look like
  this could cause a bug in practice, assuming the value only goes up.
  Except perhaps if another thread were able to reset it to 0 on the
  second access because chip->lock is not held.

  This may improve the performance of the interrupt handler;
  dma_counter is volatile so access is slow.

The resulting interrupt handling resembles more closely the pattern in
the kernel "Writing an ALSA driver" documentation.

Signed-off-by: Mark Hills <mark at xwax.org>
---
 sound/pci/echoaudio/echoaudio.c | 80 +++++++++++++++++++++------------
 sound/pci/echoaudio/echoaudio.h |  1 -
 2 files changed, 52 insertions(+), 29 deletions(-)

diff --git a/sound/pci/echoaudio/echoaudio.c b/sound/pci/echoaudio/echoaudio.c
index 8cf08988959f..12217649fb44 100644
--- a/sound/pci/echoaudio/echoaudio.c
+++ b/sound/pci/echoaudio/echoaudio.c
@@ -2,6 +2,7 @@
 /*
  *  ALSA driver for Echoaudio soundcards.
  *  Copyright (C) 2003-2004 Giuliano Pochini <pochini at shiny.it>
+ *  Copyright (C) 2020 Mark Hills <mark at xwax.org>
  */
 
 #include <linux/module.h>
@@ -590,7 +591,6 @@ static int init_engine(struct snd_pcm_substream *substream,
 	/* This stuff is used by the irq handler, so it must be
 	 * initialized before chip->substream
 	 */
-	chip->last_period[pipe_index] = 0;
 	pipe->last_counter = 0;
 	pipe->position = 0;
 	smp_wmb();
@@ -759,7 +759,6 @@ static int pcm_trigger(struct snd_pcm_substream *substream, int cmd)
 				pipe = chip->substream[i]->runtime->private_data;
 				switch (pipe->state) {
 				case PIPE_STATE_STOPPED:
-					chip->last_period[i] = 0;
 					pipe->last_counter = 0;
 					pipe->position = 0;
 					*pipe->dma_counter = 0;
@@ -807,19 +806,8 @@ static snd_pcm_uframes_t pcm_pointer(struct snd_pcm_substream *substream)
 {
 	struct snd_pcm_runtime *runtime = substream->runtime;
 	struct audiopipe *pipe = runtime->private_data;
-	size_t cnt, bufsize, pos;
 
-	cnt = le32_to_cpu(*pipe->dma_counter);
-	pipe->position += cnt - pipe->last_counter;
-	pipe->last_counter = cnt;
-	bufsize = substream->runtime->buffer_size;
-	pos = bytes_to_frames(substream->runtime, pipe->position);
-
-	while (pos >= bufsize) {
-		pipe->position -= frames_to_bytes(substream->runtime, bufsize);
-		pos -= bufsize;
-	}
-	return pos;
+	return bytes_to_frames(runtime, pipe->position);
 }
 
 
@@ -1782,14 +1770,50 @@ static const struct snd_kcontrol_new snd_echo_channels_info = {
 
 
 /******************************************************************************
-	IRQ Handler
+	IRQ Handling
 ******************************************************************************/
 
+/* Update software pointer to match the hardware
+ *
+ * Return: 1 if we crossed a period threshold, otherwise 0
+ */
+static int snd_echo_poll_substream(struct snd_pcm_substream *substream)
+{
+	struct snd_pcm_runtime *runtime = substream->runtime;
+	struct audiopipe *pipe = runtime->private_data;
+	unsigned counter, step, period, last_period;
+	size_t buffer_bytes;
+
+	if (pipe->state != PIPE_STATE_STARTED)
+		return 0;
+
+	counter = le32_to_cpu(*pipe->dma_counter);
+
+	period = bytes_to_frames(runtime, counter)
+		/ runtime->period_size;
+	last_period = bytes_to_frames(runtime, pipe->last_counter)
+		/ runtime->period_size;
+
+	if (period == last_period)
+		return 0;  /* don't do any work yet */
+
+	step = counter - pipe->last_counter;
+	pipe->last_counter = counter;
+
+	pipe->position += step;  /* bytes */
+
+	buffer_bytes = frames_to_bytes(runtime, runtime->buffer_size);
+
+	while (pipe->position >= buffer_bytes)
+		pipe->position -= buffer_bytes;
+
+	return 1;
+}
+
 static irqreturn_t snd_echo_interrupt(int irq, void *dev_id)
 {
 	struct echoaudio *chip = dev_id;
-	struct snd_pcm_substream *substream;
-	int period, ss, st;
+	int ss, st;
 
 	spin_lock(&chip->lock);
 	st = service_irq(chip);
@@ -1800,18 +1824,18 @@ static irqreturn_t snd_echo_interrupt(int irq, void *dev_id)
 	/* The hardware doesn't tell us which substream caused the irq,
 	thus we have to check all running substreams. */
 	for (ss = 0; ss < DSP_MAXPIPES; ss++) {
+		struct snd_pcm_substream *substream;
+
 		substream = chip->substream[ss];
-		if (substream && ((struct audiopipe *)substream->runtime->
-				private_data)->state == PIPE_STATE_STARTED) {
-			period = pcm_pointer(substream) /
-				substream->runtime->period_size;
-			if (period != chip->last_period[ss]) {
-				chip->last_period[ss] = period;
-				spin_unlock(&chip->lock);
-				snd_pcm_period_elapsed(substream);
-				spin_lock(&chip->lock);
-			}
-		}
+		if (!substream)
+			continue;
+
+		if (!snd_echo_poll_substream(substream))
+			continue;
+
+		spin_unlock(&chip->lock);
+		snd_pcm_period_elapsed(substream);
+		spin_lock(&chip->lock);
 	}
 	spin_unlock(&chip->lock);
 
diff --git a/sound/pci/echoaudio/echoaudio.h b/sound/pci/echoaudio/echoaudio.h
index 6fd283e4e676..7ff5d4de6880 100644
--- a/sound/pci/echoaudio/echoaudio.h
+++ b/sound/pci/echoaudio/echoaudio.h
@@ -332,7 +332,6 @@ struct audioformat {
 struct echoaudio {
 	spinlock_t lock;
 	struct snd_pcm_substream *substream[DSP_MAXPIPES];
-	int last_period[DSP_MAXPIPES];
 	struct mutex mode_mutex;
 	u16 num_digital_modes, digital_mode_list[6];
 	u16 num_clock_sources, clock_source_list[10];
-- 
2.17.5