echoaudio: Fix some long standing bugs

echoaudio: Fix some long standing bugs

[Mark Hills]

Here follows some patches for the echoaudio PCM handling.

Tested on the Layla3G on x86-64. I do have a Darla somewhere, but it's not 
practical for me to test with that right now.

-- 
Mark

[PATCH 1/3] echoaudio: Race conditions around "opencount"

[Mark Hills]

Use of atomics does not make these statements robust:

       atomic_inc(&chip->opencount);
       if (atomic_read(&chip->opencount) > 1 && chip->rate_set)
               chip->can_set_rate=0;

and

       if (atomic_read(&chip->opencount)) {
               if (chip->opencount) {
                       changed = -EAGAIN;
               } else {
                       changed = set_digital_mode(chip, dmode);

It would be necessary to atomically increment or decrement the value
and use the returned result. And yet we still need to prevent other
threads making use of "can_set_rate" while we set it.

However in all but one case the atomic is misleading as they are already
running with "mode_mutex" held.

Decisions are made on mode setting are often intrinsically connected
to "opencount" because some operations are not permitted unless
there is sole ownership.

So instead simplify this, and use "mode_mutex" as a lock for all reference
counting and mode setting.

Signed-off-by: Mark Hills <mark@xwax.org>
---
 sound/pci/echoaudio/echoaudio.c | 76 ++++++++++++++++++---------------
 sound/pci/echoaudio/echoaudio.h |  6 +--
 2 files changed, 45 insertions(+), 37 deletions(-)

diff --git a/sound/pci/echoaudio/echoaudio.c b/sound/pci/echoaudio/echoaudio.c
index 0941a7a17623..82a49dfd2384 100644
--- a/sound/pci/echoaudio/echoaudio.c
+++ b/sound/pci/echoaudio/echoaudio.c
@@ -245,13 +245,20 @@ static int hw_rule_sample_rate(struct snd_pcm_hw_params *params,
 						      SNDRV_PCM_HW_PARAM_RATE);
 	struct echoaudio *chip = rule->private;
 	struct snd_interval fixed;
+	int err;
+
+	mutex_lock(&chip->mode_mutex);
 
-	if (!chip->can_set_rate) {
+	if (chip->can_set_rate) {
+		err = 0;
+	} else {
 		snd_interval_any(&fixed);
 		fixed.min = fixed.max = chip->sample_rate;
-		return snd_interval_refine(rate, &fixed);
+		err = snd_interval_refine(rate, &fixed);
 	}
-	return 0;
+
+	mutex_unlock(&chip->mode_mutex);
+	return err;
 }
 
 
@@ -322,7 +329,7 @@ static int pcm_open(struct snd_pcm_substream *substream,
 				       SNDRV_PCM_HW_PARAM_RATE, -1)) < 0)
 		return err;
 
-	/* Finally allocate a page for the scatter-gather list */
+	/* Allocate a page for the scatter-gather list */
 	if ((err = snd_dma_alloc_pages(SNDRV_DMA_TYPE_DEV,
 				       &chip->pci->dev,
 				       PAGE_SIZE, &pipe->sgpage)) < 0) {
@@ -330,6 +337,17 @@ static int pcm_open(struct snd_pcm_substream *substream,
 		return err;
 	}
 
+	/*
+	 * Sole ownership required to set the rate
+	 */
+
+	dev_dbg(chip->card->dev, "pcm_open opencount=%d can_set_rate=%d, rate_set=%d",
+		chip->opencount, chip->can_set_rate, chip->rate_set);
+
+	chip->opencount++;
+	if (chip->opencount > 1 && chip->rate_set)
+		chip->can_set_rate = 0;
+
 	return 0;
 }
 
@@ -353,12 +371,7 @@ static int pcm_analog_in_open(struct snd_pcm_substream *substream)
 				       hw_rule_capture_format_by_channels, NULL,
 				       SNDRV_PCM_HW_PARAM_CHANNELS, -1)) < 0)
 		return err;
-	atomic_inc(&chip->opencount);
-	if (atomic_read(&chip->opencount) > 1 && chip->rate_set)
-		chip->can_set_rate=0;
-	dev_dbg(chip->card->dev, "pcm_analog_in_open  cs=%d  oc=%d  r=%d\n",
-		chip->can_set_rate, atomic_read(&chip->opencount),
-		chip->sample_rate);
+
 	return 0;
 }
 
@@ -388,12 +401,7 @@ static int pcm_analog_out_open(struct snd_pcm_substream *substream)
 				       NULL,
 				       SNDRV_PCM_HW_PARAM_CHANNELS, -1)) < 0)
 		return err;
-	atomic_inc(&chip->opencount);
-	if (atomic_read(&chip->opencount) > 1 && chip->rate_set)
-		chip->can_set_rate=0;
-	dev_dbg(chip->card->dev, "pcm_analog_out_open  cs=%d  oc=%d  r=%d\n",
-		chip->can_set_rate, atomic_read(&chip->opencount),
-		chip->sample_rate);
+
 	return 0;
 }
 
@@ -429,10 +437,6 @@ static int pcm_digital_in_open(struct snd_pcm_substream *substream)
 				       SNDRV_PCM_HW_PARAM_CHANNELS, -1)) < 0)
 		goto din_exit;
 
-	atomic_inc(&chip->opencount);
-	if (atomic_read(&chip->opencount) > 1 && chip->rate_set)
-		chip->can_set_rate=0;
-
 din_exit:
 	mutex_unlock(&chip->mode_mutex);
 	return err;
@@ -471,9 +475,7 @@ static int pcm_digital_out_open(struct snd_pcm_substream *substream)
 				       NULL, SNDRV_PCM_HW_PARAM_CHANNELS,
 				       -1)) < 0)
 		goto dout_exit;
-	atomic_inc(&chip->opencount);
-	if (atomic_read(&chip->opencount) > 1 && chip->rate_set)
-		chip->can_set_rate=0;
+
 dout_exit:
 	mutex_unlock(&chip->mode_mutex);
 	return err;
@@ -488,23 +490,29 @@ static int pcm_digital_out_open(struct snd_pcm_substream *substream)
 static int pcm_close(struct snd_pcm_substream *substream)
 {
 	struct echoaudio *chip = snd_pcm_substream_chip(substream);
-	int oc;
 
 	/* Nothing to do here. Audio is already off and pipe will be
 	 * freed by its callback
 	 */
 
-	atomic_dec(&chip->opencount);
-	oc = atomic_read(&chip->opencount);
-	dev_dbg(chip->card->dev, "pcm_close  oc=%d  cs=%d  rs=%d\n", oc,
-		chip->can_set_rate, chip->rate_set);
-	if (oc < 2)
+	mutex_lock(&chip->mode_mutex);
+
+	dev_dbg(chip->card->dev, "pcm_open opencount=%d can_set_rate=%d, rate_set=%d",
+		chip->opencount, chip->can_set_rate, chip->rate_set);
+
+	chip->opencount--;
+
+	switch (chip->opencount) {
+	case 1:
 		chip->can_set_rate = 1;
-	if (oc == 0)
+		break;
+
+	case 0:
 		chip->rate_set = 0;
-	dev_dbg(chip->card->dev, "pcm_close2 oc=%d  cs=%d  rs=%d\n", oc,
-		chip->can_set_rate, chip->rate_set);
+		break;
+	}
 
+	mutex_unlock(&chip->mode_mutex);
 	return 0;
 }
 
@@ -1409,7 +1417,7 @@ static int snd_echo_digital_mode_put(struct snd_kcontrol *kcontrol,
 		/* Do not allow the user to change the digital mode when a pcm
 		device is open because it also changes the number of channels
 		and the allowed sample rates */
-		if (atomic_read(&chip->opencount)) {
+		if (chip->opencount) {
 			changed = -EAGAIN;
 		} else {
 			changed = set_digital_mode(chip, dmode);
@@ -1874,7 +1882,7 @@ static int snd_echo_create(struct snd_card *card,
 		chip->card = card;
 		chip->pci = pci;
 		chip->irq = -1;
-		atomic_set(&chip->opencount, 0);
+		chip->opencount = 0;
 		mutex_init(&chip->mode_mutex);
 		chip->can_set_rate = 1;
 	} else {
diff --git a/sound/pci/echoaudio/echoaudio.h b/sound/pci/echoaudio/echoaudio.h
index be4d0489394a..6fd283e4e676 100644
--- a/sound/pci/echoaudio/echoaudio.h
+++ b/sound/pci/echoaudio/echoaudio.h
@@ -336,7 +336,7 @@ struct echoaudio {
 	struct mutex mode_mutex;
 	u16 num_digital_modes, digital_mode_list[6];
 	u16 num_clock_sources, clock_source_list[10];
-	atomic_t opencount;
+	unsigned int opencount;  /* protected by mode_mutex */
 	struct snd_kcontrol *clock_src_ctl;
 	struct snd_pcm *analog_pcm, *digital_pcm;
 	struct snd_card *card;
@@ -353,8 +353,8 @@ struct echoaudio {
 	struct timer_list timer;
 	char tinuse;				/* Timer in use */
 	char midi_full;				/* MIDI output buffer is full */
-	char can_set_rate;
-	char rate_set;
+	char can_set_rate;                      /* protected by mode_mutex */
+	char rate_set;                          /* protected by mode_mutex */
 
 	/* This stuff is used mainly by the lowlevel code */
 	struct comm_page *comm_page;	/* Virtual address of the memory
-- 
2.17.5

[PATCH 2/3] echoaudio: Prevent races in calls to set_audio_format()

[Mark Hills]

The function uses chip->comm_page which needs locking against
other use at the same time.

Signed-off-by: Mark Hills <mark@xwax.org>
---
 sound/pci/echoaudio/echoaudio.c | 15 ++++++++++++++-
 1 file changed, 14 insertions(+), 1 deletion(-)

diff --git a/sound/pci/echoaudio/echoaudio.c b/sound/pci/echoaudio/echoaudio.c
index 82a49dfd2384..8cf08988959f 100644
--- a/sound/pci/echoaudio/echoaudio.c
+++ b/sound/pci/echoaudio/echoaudio.c
@@ -711,9 +711,22 @@ static int pcm_prepare(struct snd_pcm_substream *substream)
 
 	if (snd_BUG_ON(pipe_index >= px_num(chip)))
 		return -EINVAL;
-	if (snd_BUG_ON(!is_pipe_allocated(chip, pipe_index)))
+
+	/*
+	 * We passed checks we can do independently; now take
+	 * exclusive control
+	 */
+
+	spin_lock(&chip->lock);
+
+	if (snd_BUG_ON(!is_pipe_allocated(chip, pipe_index))) {
+		spin_unlock(&chip->lock);
 		return -EINVAL;
+	}
+
 	set_audio_format(chip, pipe_index, &format);
+	spin_unlock(&chip->lock);
+
 	return 0;
 }
 
-- 
2.17.5

Re: [PATCH 2/3] echoaudio: Prevent races in calls to set_audio_format()

[Takashi Iwai]

On Tue, 16 Jun 2020 15:17:42 +0200,
Mark Hills wrote:
> 
> The function uses chip->comm_page which needs locking against
> other use at the same time.
> 
> Signed-off-by: Mark Hills <mark@xwax.org>
> ---
>  sound/pci/echoaudio/echoaudio.c | 15 ++++++++++++++-
>  1 file changed, 14 insertions(+), 1 deletion(-)
> 
> diff --git a/sound/pci/echoaudio/echoaudio.c b/sound/pci/echoaudio/echoaudio.c
> index 82a49dfd2384..8cf08988959f 100644
> --- a/sound/pci/echoaudio/echoaudio.c
> +++ b/sound/pci/echoaudio/echoaudio.c
> @@ -711,9 +711,22 @@ static int pcm_prepare(struct snd_pcm_substream *substream)
>  
>  	if (snd_BUG_ON(pipe_index >= px_num(chip)))
>  		return -EINVAL;
> -	if (snd_BUG_ON(!is_pipe_allocated(chip, pipe_index)))
> +
> +	/*
> +	 * We passed checks we can do independently; now take
> +	 * exclusive control
> +	 */
> +
> +	spin_lock(&chip->lock);

You need spin_lock_irq() and spin_unlock_irq(), as the prepare
callback in in sleepable context.


thanks,

Takashi

[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@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@shiny.it>
+ *  Copyright (C) 2020 Mark Hills <mark@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

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

[Takashi Iwai]

On Tue, 16 Jun 2020 15:17:43 +0200,
Mark Hills wrote:
> 
> +/* 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)

This is a bit confusing name.  One would imagine from the function
name as if it were about the handling of poll() syscall.

> +{
> +	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;

Dose this calculation consider the overlap of 32bit integer of the
counter?  (I'm not sure whether the old code did it right, though.)


thanks,

Takashi

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

[Mark Hills]

On Tue, 16 Jun 2020, Takashi Iwai wrote:

> On Tue, 16 Jun 2020 15:17:43 +0200,
> Mark Hills wrote:
> > 
> > +/* 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)
> 
> This is a bit confusing name.  One would imagine from the function
> name as if it were about the handling of poll() syscall.

Poll felt intuitive to me; maybe from FreeBSD where network drivers can 
poll on a timer instead of interrupts. I do know about poll(), though.

How about snd_echo_update_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;
> 
> Dose this calculation consider the overlap of 32bit integer of the
> counter?  (I'm not sure whether the old code did it right, though.)

I believe it does, since (small - big) as unsigned gives small value. And 
period is checked only for equality (not greater than). I'll add a comment 
as such. I have run it with long streams.

Would it be clearer, or should it be that buffer_bytes to "unsigned"? 
Though size_t conveys that it is a byte length, in memory.

In general I haven't deviated from existing code without need to, so I 
inherited these types.

The same goes for the pattern of calculating "step" with unsigned values, 
and then using a loop to wrap it to the buffer:

    while (pipe->position >= buffer_bytes)
            pipe->position -= buffer_bytes;

I've assumed this was a recognised pattern in ALSA code, preferred over 
modulus.

-- 
Mark

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

[Takashi Iwai]

On Tue, 16 Jun 2020 16:01:11 +0200,
Mark Hills wrote:
> 
> On Tue, 16 Jun 2020, Takashi Iwai wrote:
> 
> > On Tue, 16 Jun 2020 15:17:43 +0200,
> > Mark Hills wrote:
> > > 
> > > +/* 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)
> > 
> > This is a bit confusing name.  One would imagine from the function
> > name as if it were about the handling of poll() syscall.
> 
> Poll felt intuitive to me; maybe from FreeBSD where network drivers can 
> poll on a timer instead of interrupts. I do know about poll(), though.
> 
> How about snd_echo_update_substream()?

Yes, it's more self-explanatory.  Or even better
snd_echo_update_substream_position() :)

> > > +{
> > > +	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;
> > 
> > Dose this calculation consider the overlap of 32bit integer of the
> > counter?  (I'm not sure whether the old code did it right, though.)
> 
> I believe it does, since (small - big) as unsigned gives small value. And 
> period is checked only for equality (not greater than). I'll add a comment 
> as such. I have run it with long streams.

The problem is that the last_period calculation can be wrong if the
period size isn't aligned.  e.g. when period size is 44100, around the
boundary position, it gets a different last_period value although it
still in the same period.


thanks,

Takashi

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

[Mark Hills]

On Tue, 16 Jun 2020, Takashi Iwai wrote:

> On Tue, 16 Jun 2020 16:01:11 +0200,
> Mark Hills wrote:
> > 
> > On Tue, 16 Jun 2020, Takashi Iwai wrote:
> > 
> > > On Tue, 16 Jun 2020 15:17:43 +0200,
> > > Mark Hills wrote:
> > > > 
> > > > +/* 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)
> > > 
> > > This is a bit confusing name.  One would imagine from the function
> > > name as if it were about the handling of poll() syscall.
> > 
> > Poll felt intuitive to me; maybe from FreeBSD where network drivers can 
> > poll on a timer instead of interrupts. I do know about poll(), though.
> > 
> > How about snd_echo_update_substream()?
> 
> Yes, it's more self-explanatory.  Or even better
> snd_echo_update_substream_position() :)

Out of interest, these are static but the names are globally qualified. I 
see this elsewhere, so I followed, but is this agreed convention?

Because it could be update_substream_position() :)

> > > > +{
> > > > +	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;
> > > 
> > > Dose this calculation consider the overlap of 32bit integer of the
> > > counter?  (I'm not sure whether the old code did it right, though.)
> > 
> > I believe it does, since (small - big) as unsigned gives small value. And 
> > period is checked only for equality (not greater than). I'll add a comment 
> > as such. I have run it with long streams.
> 
> The problem is that the last_period calculation can be wrong if the
> period size isn't aligned.  e.g. when period size is 44100, around the
> boundary position, it gets a different last_period value although it
> still in the same period.

Agreed, yes.

In which case I think it's clearer to not infer anything about periods 
from the current counter or position, and (effectively) accumulate it.

Would you please make suggestions on the code below?

Because it allowed for further simplification whilst fixing the bugs.

In the end, modulo became clearer than loops and I think it has the bonus 
of being less risky should the counter make a large step.

I'll run for a longer test today.

---

/* Update software pointer to match the hardware
 *
 * \pre chip->lock is held
 */
static void snd_echo_update_substream_position(struct echoaudio *chip,
					struct snd_pcm_substream *substream)
{
	struct snd_pcm_runtime *runtime = substream->runtime;
	struct audiopipe *pipe = runtime->private_data;
	u32 counter, step;
	size_t period_bytes;

	if (pipe->state != PIPE_STATE_STARTED)
		return;

	period_bytes = frames_to_bytes(runtime, runtime->period_size);

	counter = le32_to_cpu(*pipe->dma_counter);

	step = counter - pipe->last_counter;  /* handles wrapping of counter */
	step -= step % period_bytes;  /* acknowledge whole periods only */

	if (step == 0)
		return;  /* haven't advanced a whole period yet */

	pipe->last_counter += step;  /* does not always wrap on a period */
	pipe->position += step;

	/* wraparound the buffer */
	pipe->position %= frames_to_bytes(runtime, runtime->buffer_size);

	/* notify only once, even if multiple periods elapsed */
	spin_unlock(&chip->lock);
	snd_pcm_period_elapsed(substream);
	spin_lock(&chip->lock);
}

static irqreturn_t snd_echo_interrupt(int irq, void *dev_id)
{
	struct echoaudio *chip = dev_id;
	int ss, st;

	spin_lock(&chip->lock);
	st = service_irq(chip);
	if (st < 0) {
		spin_unlock(&chip->lock);
		return IRQ_NONE;
	}
	/* 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)
			snd_echo_update_substream_position(chip, substream);
	}
	spin_unlock(&chip->lock);

#ifdef ECHOCARD_HAS_MIDI
	if (st > 0 && chip->midi_in) {
		snd_rawmidi_receive(chip->midi_in, chip->midi_buffer, st);
		dev_dbg(chip->card->dev, "rawmidi_iread=%d\n", st);
	}
#endif
	return IRQ_HANDLED;
}

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;

	return bytes_to_frames(runtime, pipe->position);
}

-- 
Mark

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

[Takashi Iwai]

On Wed, 17 Jun 2020 12:51:05 +0200,
Mark Hills wrote:
> 
> On Tue, 16 Jun 2020, Takashi Iwai wrote:
> 
> > On Tue, 16 Jun 2020 16:01:11 +0200,
> > Mark Hills wrote:
> > > 
> > > On Tue, 16 Jun 2020, Takashi Iwai wrote:
> > > 
> > > > On Tue, 16 Jun 2020 15:17:43 +0200,
> > > > Mark Hills wrote:
> > > > > 
> > > > > +/* 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)
> > > > 
> > > > This is a bit confusing name.  One would imagine from the function
> > > > name as if it were about the handling of poll() syscall.
> > > 
> > > Poll felt intuitive to me; maybe from FreeBSD where network drivers can 
> > > poll on a timer instead of interrupts. I do know about poll(), though.
> > > 
> > > How about snd_echo_update_substream()?
> > 
> > Yes, it's more self-explanatory.  Or even better
> > snd_echo_update_substream_position() :)
> 
> Out of interest, these are static but the names are globally qualified. I 
> see this elsewhere, so I followed, but is this agreed convention?
> 
> Because it could be update_substream_position() :)

It's fine to use any name for a local function.


> > > > > +{
> > > > > +	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;
> > > > 
> > > > Dose this calculation consider the overlap of 32bit integer of the
> > > > counter?  (I'm not sure whether the old code did it right, though.)
> > > 
> > > I believe it does, since (small - big) as unsigned gives small value. And 
> > > period is checked only for equality (not greater than). I'll add a comment 
> > > as such. I have run it with long streams.
> > 
> > The problem is that the last_period calculation can be wrong if the
> > period size isn't aligned.  e.g. when period size is 44100, around the
> > boundary position, it gets a different last_period value although it
> > still in the same period.
> 
> Agreed, yes.
> 
> In which case I think it's clearer to not infer anything about periods 
> from the current counter or position, and (effectively) accumulate it.
> 
> Would you please make suggestions on the code below?
> 
> Because it allowed for further simplification whilst fixing the bugs.
> 
> In the end, modulo became clearer than loops and I think it has the bonus 
> of being less risky should the counter make a large step.
> 
> I'll run for a longer test today.
> 
> ---
> 
> /* Update software pointer to match the hardware
>  *
>  * \pre chip->lock is held
>  */
> static void snd_echo_update_substream_position(struct echoaudio *chip,
> 					struct snd_pcm_substream *substream)
> {
> 	struct snd_pcm_runtime *runtime = substream->runtime;
> 	struct audiopipe *pipe = runtime->private_data;
> 	u32 counter, step;
> 	size_t period_bytes;
> 
> 	if (pipe->state != PIPE_STATE_STARTED)
> 		return;
> 
> 	period_bytes = frames_to_bytes(runtime, runtime->period_size);
> 
> 	counter = le32_to_cpu(*pipe->dma_counter);
> 
> 	step = counter - pipe->last_counter;  /* handles wrapping of counter */
> 	step -= step % period_bytes;  /* acknowledge whole periods only */
> 
> 	if (step == 0)
> 		return;  /* haven't advanced a whole period yet */
> 	pipe->last_counter += step;  /* does not always wrap on a period */
> 	pipe->position += step;
> 
> 	/* wraparound the buffer */
> 	pipe->position %= frames_to_bytes(runtime, runtime->buffer_size);
> 
> 	/* notify only once, even if multiple periods elapsed */
> 	spin_unlock(&chip->lock);
> 	snd_pcm_period_elapsed(substream);
> 	spin_lock(&chip->lock);
> }
> 
> static irqreturn_t snd_echo_interrupt(int irq, void *dev_id)
> {
> 	struct echoaudio *chip = dev_id;
> 	int ss, st;
> 
> 	spin_lock(&chip->lock);
> 	st = service_irq(chip);
> 	if (st < 0) {
> 		spin_unlock(&chip->lock);
> 		return IRQ_NONE;
> 	}
> 	/* 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)
> 			snd_echo_update_substream_position(chip, substream);
> 	}
> 	spin_unlock(&chip->lock);
> 
> #ifdef ECHOCARD_HAS_MIDI
> 	if (st > 0 && chip->midi_in) {
> 		snd_rawmidi_receive(chip->midi_in, chip->midi_buffer, st);
> 		dev_dbg(chip->card->dev, "rawmidi_iread=%d\n", st);
> 	}
> #endif
> 	return IRQ_HANDLED;
> }
> 
> 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;
> 
> 	return bytes_to_frames(runtime, pipe->position);

I guess this misses the update of the precise position; in your code,
pipe->position gets updated only with the period size at irq handler.


IMO, we should have the code like:

static bool update_stream_position(struct snd_pcm_substream *substream)
{
	// update pipe->position and others, returns true if period elapsed
}

static irqreturn_t snd_echo_interrupt()
{
	spin_lock(&chip->lock);
	....
	if (update_stream_position(substream)) {
		spin_unlock(&chip->lock);
		snd_pcm_period_elapsed(substream);
		spin_lock(&chip->lock);
	}
	....
	spin_unlock(&chip->lock);
	return IRQ_HANDLED;
}

static snd_pcm_uframes_t pcm_pointer(struct snd_pcm_substream *substream)
{
	....
	update_stream_position(substream);
	return bytes_to_frames(runtime, pipe->position);
}


Note that snd_pcm_period_elapsed() isn't needed (must not be done)
from the pointer callback.  The PCM core would detect and handle
properly if the period gets elapsed there.


thanks,

Takashi

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

[Mark Hills]

On Thu, 18 Jun 2020, Takashi Iwai wrote:

> On Wed, 17 Jun 2020 12:51:05 +0200,
> Mark Hills wrote:
> > 
> > On Tue, 16 Jun 2020, Takashi Iwai wrote:
> > 
> > > On Tue, 16 Jun 2020 16:01:11 +0200,
> > > Mark Hills wrote:
[...]
> > 
> > /* Update software pointer to match the hardware
> >  *
> >  * \pre chip->lock is held
> >  */
> > static void snd_echo_update_substream_position(struct echoaudio *chip,
> > 					struct snd_pcm_substream *substream)
> > {
> > 	struct snd_pcm_runtime *runtime = substream->runtime;
> > 	struct audiopipe *pipe = runtime->private_data;
> > 	u32 counter, step;
> > 	size_t period_bytes;
> > 
> > 	if (pipe->state != PIPE_STATE_STARTED)
> > 		return;
> > 
> > 	period_bytes = frames_to_bytes(runtime, runtime->period_size);
> > 
> > 	counter = le32_to_cpu(*pipe->dma_counter);
> > 
> > 	step = counter - pipe->last_counter;  /* handles wrapping of counter */
> > 	step -= step % period_bytes;  /* acknowledge whole periods only */
> > 
> > 	if (step == 0)
> > 		return;  /* haven't advanced a whole period yet */
> > 	pipe->last_counter += step;  /* does not always wrap on a period */
> > 	pipe->position += step;
> > 
> > 	/* wraparound the buffer */
> > 	pipe->position %= frames_to_bytes(runtime, runtime->buffer_size);
> > 
> > 	/* notify only once, even if multiple periods elapsed */
> > 	spin_unlock(&chip->lock);
> > 	snd_pcm_period_elapsed(substream);
> > 	spin_lock(&chip->lock);
> > }
> > 
> > static irqreturn_t snd_echo_interrupt(int irq, void *dev_id)
> > {
> > 	struct echoaudio *chip = dev_id;
> > 	int ss, st;
> > 
> > 	spin_lock(&chip->lock);
> > 	st = service_irq(chip);
> > 	if (st < 0) {
> > 		spin_unlock(&chip->lock);
> > 		return IRQ_NONE;
> > 	}
> > 	/* 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)
> > 			snd_echo_update_substream_position(chip, substream);
> > 	}
> > 	spin_unlock(&chip->lock);
> > 
> > #ifdef ECHOCARD_HAS_MIDI
> > 	if (st > 0 && chip->midi_in) {
> > 		snd_rawmidi_receive(chip->midi_in, chip->midi_buffer, st);
> > 		dev_dbg(chip->card->dev, "rawmidi_iread=%d\n", st);
> > 	}
> > #endif
> > 	return IRQ_HANDLED;
> > }
> > 
> > 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;
> > 
> > 	return bytes_to_frames(runtime, pipe->position);
> 
> I guess this misses the update of the precise position; in your code,
> pipe->position gets updated only with the period size at irq handler.
> 
> 
> IMO, we should have the code like:
> 
> static bool update_stream_position(struct snd_pcm_substream *substream)
> {
> 	// update pipe->position and others, returns true if period elapsed
> }
> 
> static irqreturn_t snd_echo_interrupt()
> {
> 	spin_lock(&chip->lock);
> 	....
> 	if (update_stream_position(substream)) {
> 		spin_unlock(&chip->lock);
> 		snd_pcm_period_elapsed(substream);
> 		spin_lock(&chip->lock);
> 	}
> 	....
> 	spin_unlock(&chip->lock);
> 	return IRQ_HANDLED;
> }
> 
> static snd_pcm_uframes_t pcm_pointer(struct snd_pcm_substream *substream)
> {
> 	....
> 	update_stream_position(substream);
> 	return bytes_to_frames(runtime, pipe->position);
> }

Thanks.

I certainly understand this in isolation. 

But could I please ask for help with the bigger picture? As it feels 
mismatched.

* Code should take every possible opportunity to update the stream 
  position; interrupts, or explicit pcm_pointer calls (whereas the docs 
  guide towards doing it in the interrupt handler)

* I critiqued (elsewhere in thread) the older interrupt handler for 
  checking the counter, unlocking, calling back into PCM core and checking 
  again a moment later. Whereas this is considered good behaviour.

* Seems like the overall aim is for userland to be able (if it wants to)  
  to poll the soundcard, even outside of periods.

If all the above is true, I would expect interrupt handling to be very 
simple -- it would straight away call into PCM core, join existing if the 
codepaths (to take locks) and do a position update. PCM core would decide 
if a period really elapsed, not the driver. But this is not how it works.

This now relates strongly to a question of locking:

I ran the code (top of this message) all day, with a few instances in 
dmesg (at irregular intervals, not wrapping):

  [161644.076666] snd_echo3g 0000:09:02.0: invalid position: , pos = 4096, buffer size = 4096, period size = 64
  [163232.392501] snd_echo3g 0000:09:02.0: invalid position: , pos = 4096, buffer size = 4096, period size = 64
  [164976.098069] snd_echo3g 0000:09:02.0: invalid position: , pos = 4096, buffer size = 4096, period size = 64
  [165054.946225] snd_echo3g 0000:09:02.0: invalid position: , pos = 4096, buffer size = 4096, period size = 64
  [165312.141545] snd_echo3g 0000:09:02.0: invalid position: , pos = 4096, buffer size = 4096, period size = 64

A definite bug, of course.

However (and I am happy to be corrected) the function never finishes with 
position == buffer size. Only way is a race between interrupt handler and 
pcm_pointer.

Therefore one of these is needed:

* pcm_pointer locks chip->lock

  Even though the docs emphasise PCM core has exclusivity, it it not worth 
  much as it does not protect against the interrupt handler.

  But now interrupt handler becomes ugly in total: take chip->lock, check 
  the counter, release chip->lock, go to PCM core (which takes middle 
  layer locks), take chip->lock again, check counter again, release 
  chip->lock again.

* interrupt handler must make atomic update of pipe->position

  This could have been a solution, but not if we expect pcm_pointer to 
  also invoke the position update. Now we have a race: both the interrupt 
  handler and pcm_position want to read dma_counter and write 
  pipe->position after.

So either do everthing in interrupt, everything in the pointer callback 
(though there isn't the API for this), but doing both does not seem to 
work well (though probably can be made to work if necessary)

If we can clarify the requirements then I do not think it would be hard 
for me to write the code.

[...]
> Takashi

Thanks again,

-- 
Mark

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

[Takashi Iwai]

On Thu, 18 Jun 2020 13:07:33 +0200,
Mark Hills wrote:
> 
> On Thu, 18 Jun 2020, Takashi Iwai wrote:
> 
> > On Wed, 17 Jun 2020 12:51:05 +0200,
> > Mark Hills wrote:
> > > 
> > > On Tue, 16 Jun 2020, Takashi Iwai wrote:
> > > 
> > > > On Tue, 16 Jun 2020 16:01:11 +0200,
> > > > Mark Hills wrote:
> [...]
> > > 
> > > /* Update software pointer to match the hardware
> > >  *
> > >  * \pre chip->lock is held
> > >  */
> > > static void snd_echo_update_substream_position(struct echoaudio *chip,
> > > 					struct snd_pcm_substream *substream)
> > > {
> > > 	struct snd_pcm_runtime *runtime = substream->runtime;
> > > 	struct audiopipe *pipe = runtime->private_data;
> > > 	u32 counter, step;
> > > 	size_t period_bytes;
> > > 
> > > 	if (pipe->state != PIPE_STATE_STARTED)
> > > 		return;
> > > 
> > > 	period_bytes = frames_to_bytes(runtime, runtime->period_size);
> > > 
> > > 	counter = le32_to_cpu(*pipe->dma_counter);
> > > 
> > > 	step = counter - pipe->last_counter;  /* handles wrapping of counter */
> > > 	step -= step % period_bytes;  /* acknowledge whole periods only */
> > > 
> > > 	if (step == 0)
> > > 		return;  /* haven't advanced a whole period yet */
> > > 	pipe->last_counter += step;  /* does not always wrap on a period */
> > > 	pipe->position += step;
> > > 
> > > 	/* wraparound the buffer */
> > > 	pipe->position %= frames_to_bytes(runtime, runtime->buffer_size);
> > > 
> > > 	/* notify only once, even if multiple periods elapsed */
> > > 	spin_unlock(&chip->lock);
> > > 	snd_pcm_period_elapsed(substream);
> > > 	spin_lock(&chip->lock);
> > > }
> > > 
> > > static irqreturn_t snd_echo_interrupt(int irq, void *dev_id)
> > > {
> > > 	struct echoaudio *chip = dev_id;
> > > 	int ss, st;
> > > 
> > > 	spin_lock(&chip->lock);
> > > 	st = service_irq(chip);
> > > 	if (st < 0) {
> > > 		spin_unlock(&chip->lock);
> > > 		return IRQ_NONE;
> > > 	}
> > > 	/* 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)
> > > 			snd_echo_update_substream_position(chip, substream);
> > > 	}
> > > 	spin_unlock(&chip->lock);
> > > 
> > > #ifdef ECHOCARD_HAS_MIDI
> > > 	if (st > 0 && chip->midi_in) {
> > > 		snd_rawmidi_receive(chip->midi_in, chip->midi_buffer, st);
> > > 		dev_dbg(chip->card->dev, "rawmidi_iread=%d\n", st);
> > > 	}
> > > #endif
> > > 	return IRQ_HANDLED;
> > > }
> > > 
> > > 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;
> > > 
> > > 	return bytes_to_frames(runtime, pipe->position);
> > 
> > I guess this misses the update of the precise position; in your code,
> > pipe->position gets updated only with the period size at irq handler.
> > 
> > 
> > IMO, we should have the code like:
> > 
> > static bool update_stream_position(struct snd_pcm_substream *substream)
> > {
> > 	// update pipe->position and others, returns true if period elapsed
> > }
> > 
> > static irqreturn_t snd_echo_interrupt()
> > {
> > 	spin_lock(&chip->lock);
> > 	....
> > 	if (update_stream_position(substream)) {
> > 		spin_unlock(&chip->lock);
> > 		snd_pcm_period_elapsed(substream);
> > 		spin_lock(&chip->lock);
> > 	}
> > 	....
> > 	spin_unlock(&chip->lock);
> > 	return IRQ_HANDLED;
> > }
> > 
> > static snd_pcm_uframes_t pcm_pointer(struct snd_pcm_substream *substream)
> > {
> > 	....
> > 	update_stream_position(substream);
> > 	return bytes_to_frames(runtime, pipe->position);
> > }
> 
> Thanks.
> 
> I certainly understand this in isolation. 
> 
> But could I please ask for help with the bigger picture? As it feels 
> mismatched.
> 
> * Code should take every possible opportunity to update the stream 
>   position; interrupts, or explicit pcm_pointer calls (whereas the docs 
>   guide towards doing it in the interrupt handler)
> 
> * I critiqued (elsewhere in thread) the older interrupt handler for 
>   checking the counter, unlocking, calling back into PCM core and checking 
>   again a moment later. Whereas this is considered good behaviour.
> 
> * Seems like the overall aim is for userland to be able (if it wants to)  
>   to poll the soundcard, even outside of periods.

Right, the user-space can query the current position at any time, and
the driver should return the position as precisely as possible.

Some applications (like PulseAudio) sets the interrupt as minimum as
possible while it does schedule the update by itself, judging the
position via the ioctl.  In such operational mode, the accuracy of the
current position query is vital.

> If all the above is true, I would expect interrupt handling to be very 
> simple -- it would straight away call into PCM core, join existing if the 
> codepaths (to take locks) and do a position update. PCM core would decide 
> if a period really elapsed, not the driver. But this is not how it works.
> 
> This now relates strongly to a question of locking:
> 
> I ran the code (top of this message) all day, with a few instances in 
> dmesg (at irregular intervals, not wrapping):
> 
>   [161644.076666] snd_echo3g 0000:09:02.0: invalid position: , pos = 4096, buffer size = 4096, period size = 64
>   [163232.392501] snd_echo3g 0000:09:02.0: invalid position: , pos = 4096, buffer size = 4096, period size = 64
>   [164976.098069] snd_echo3g 0000:09:02.0: invalid position: , pos = 4096, buffer size = 4096, period size = 64
>   [165054.946225] snd_echo3g 0000:09:02.0: invalid position: , pos = 4096, buffer size = 4096, period size = 64
>   [165312.141545] snd_echo3g 0000:09:02.0: invalid position: , pos = 4096, buffer size = 4096, period size = 64
> 
> A definite bug, of course.
> 
> However (and I am happy to be corrected) the function never finishes with 
> position == buffer size. Only way is a race between interrupt handler and 
> pcm_pointer.
> 
> Therefore one of these is needed:
> 
> * pcm_pointer locks chip->lock
> 
>   Even though the docs emphasise PCM core has exclusivity, it it not worth 
>   much as it does not protect against the interrupt handler.
> 
>   But now interrupt handler becomes ugly in total: take chip->lock, check 
>   the counter, release chip->lock, go to PCM core (which takes middle 
>   layer locks), take chip->lock again, check counter again, release 
>   chip->lock again.

Yes, that's the most robust way to go.  If the lock is really costing
too much, you can consider a fast-path by some flag for the irq ->
snd_pcm_period_elapsed() case.

Basically you can do everything in the pointer callback.  The only
requirement in the interrupt handle is basically judge whether you
need the call of snd_pcm_period_elapsed() and call it.  The rest
update could be done in the other places.

> * interrupt handler must make atomic update of pipe->position
> 
>   This could have been a solution, but not if we expect pcm_pointer to 
>   also invoke the position update. Now we have a race: both the interrupt 
>   handler and pcm_position want to read dma_counter and write 
>   pipe->position after.

As mentioned, the update of position at query is essential in majority
of sound systems.


Takashi

> So either do everthing in interrupt, everything in the pointer callback 
> (though there isn't the API for this), but doing both does not seem to 
> work well (though probably can be made to work if necessary)
> 
> If we can clarify the requirements then I do not think it would be hard 
> for me to write the code.
> 
> [...]
> > Takashi
> 
> Thanks again,
> 
> -- 
> Mark
> 

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

[Mark Hills]

On Thu, 18 Jun 2020, Takashi Iwai wrote:

> On Thu, 18 Jun 2020 13:07:33 +0200,
> Mark Hills wrote:
> > 
> > On Thu, 18 Jun 2020, Takashi Iwai wrote:
> > 
> > > On Wed, 17 Jun 2020 12:51:05 +0200,
> > > Mark Hills wrote:
> > [...]
> > But could I please ask for help with the bigger picture? As it feels 
> > mismatched.
> > 
> > * Code should take every possible opportunity to update the stream 
> >   position; interrupts, or explicit pcm_pointer calls (whereas the docs 
> >   guide towards doing it in the interrupt handler)
> > 
> > * I critiqued (elsewhere in thread) the older interrupt handler for 
> >   checking the counter, unlocking, calling back into PCM core and checking 
> >   again a moment later. Whereas this is considered good behaviour.
> > 
> > * Seems like the overall aim is for userland to be able (if it wants to)  
> >   to poll the soundcard, even outside of periods.
> 
> Right, the user-space can query the current position at any time, and
> the driver should return the position as precisely as possible.
> 
> Some applications (like PulseAudio) sets the interrupt as minimum as
> possible while it does schedule the update by itself, judging the
> position via the ioctl.  In such operational mode, the accuracy of the
> current position query is vital.
> 
> > If all the above is true, I would expect interrupt handling to be very 
> > simple -- it would straight away call into PCM core, join existing if the 
> > codepaths (to take locks) and do a position update. PCM core would decide 
> > if a period really elapsed, not the driver. But this is not how it works.
> > 
> > This now relates strongly to a question of locking:
> > 
> > I ran the code (top of this message) all day, with a few instances in 
> > dmesg (at irregular intervals, not wrapping):
> > 
> >   [161644.076666] snd_echo3g 0000:09:02.0: invalid position: , pos = 4096, buffer size = 4096, period size = 64
> >   [163232.392501] snd_echo3g 0000:09:02.0: invalid position: , pos = 4096, buffer size = 4096, period size = 64
> >   [164976.098069] snd_echo3g 0000:09:02.0: invalid position: , pos = 4096, buffer size = 4096, period size = 64
> >   [165054.946225] snd_echo3g 0000:09:02.0: invalid position: , pos = 4096, buffer size = 4096, period size = 64
> >   [165312.141545] snd_echo3g 0000:09:02.0: invalid position: , pos = 4096, buffer size = 4096, period size = 64
> > 
> > A definite bug, of course.
> > 
> > However (and I am happy to be corrected) the function never finishes with 
> > position == buffer size. Only way is a race between interrupt handler and 
> > pcm_pointer.
> > 
> > Therefore one of these is needed:
> > 
> > * pcm_pointer locks chip->lock
> > 
> >   Even though the docs emphasise PCM core has exclusivity, it it not worth 
> >   much as it does not protect against the interrupt handler.
> > 
> >   But now interrupt handler becomes ugly in total: take chip->lock, check 
> >   the counter, release chip->lock, go to PCM core (which takes middle 
> >   layer locks), take chip->lock again, check counter again, release 
> >   chip->lock again.
> 
> Yes, that's the most robust way to go.  If the lock is really costing
> too much, you can consider a fast-path by some flag for the irq ->
> snd_pcm_period_elapsed() case.

I don't understand how a fast path could be made to work, as it can't pass 
data across snd_pcm_period_elapsed() and it still must syncronise access 
between reading dma_counter and writing pipe->position.

Hence questioning if a better design is simpler interrupt handlers that 
just enter PCM core.
 
> Basically you can do everything in the pointer callback.  The only 
> requirement in the interrupt handle is basically judge whether you need 
> the call of snd_pcm_period_elapsed() and call it.  The rest update could 
> be done in the other places.

Thanks, please just another clarification:

I presume that calls to pcm_pointer are completely independent of the 
period notifications?

ie. period notifications are at regular intervals, regardless of whether 
pcm_pointer is called inbetween. pcm_pointer must not reset any state used 
by the interrupt.

Which means we must handle when non-interrupt call to pcm_pointer causes a 
period to elapse. The next interrupt handler must notify.

I can see in the original code uses chip->last_period exclusively by the 
interrupt handler, and I removed it. Some comments around the intent would 
help. I'll cross reference the original code with my new understanding.

My instinct here is that to preserve

- regular period notifications

- handle period_size not aligning with 32-bit counter

- no races between interrupt and pcm_pointer

that the clearest and bug-free implementation may be to separate the 
interrupt (notifications) and pointer updates to separate state.

Then there's no lock and the only crossover is an atomic read of 
dma_counter.

And that's what I will try -- thanks.

-- 
Mark

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

[Mark Hills]

On Thu, 18 Jun 2020, Mark Hills wrote:

> On Thu, 18 Jun 2020, Takashi Iwai wrote:
> 
> > On Thu, 18 Jun 2020 13:07:33 +0200,
> > Mark Hills wrote:
> > > 
> > > On Thu, 18 Jun 2020, Takashi Iwai wrote:
> > > 
> > > > On Wed, 17 Jun 2020 12:51:05 +0200,
> > > > Mark Hills wrote:
> > > [...]
> > > But could I please ask for help with the bigger picture? As it feels 
> > > mismatched.
> > > 
> > > * Code should take every possible opportunity to update the stream 
> > >   position; interrupts, or explicit pcm_pointer calls (whereas the docs 
> > >   guide towards doing it in the interrupt handler)
> > > 
> > > * I critiqued (elsewhere in thread) the older interrupt handler for 
> > >   checking the counter, unlocking, calling back into PCM core and checking 
> > >   again a moment later. Whereas this is considered good behaviour.
> > > 
> > > * Seems like the overall aim is for userland to be able (if it wants to)  
> > >   to poll the soundcard, even outside of periods.
> > 
> > Right, the user-space can query the current position at any time, and
> > the driver should return the position as precisely as possible.
> > 
> > Some applications (like PulseAudio) sets the interrupt as minimum as
> > possible while it does schedule the update by itself, judging the
> > position via the ioctl.  In such operational mode, the accuracy of the
> > current position query is vital.
> > 
> > > If all the above is true, I would expect interrupt handling to be very 
> > > simple -- it would straight away call into PCM core, join existing if the 
> > > codepaths (to take locks) and do a position update. PCM core would decide 
> > > if a period really elapsed, not the driver. But this is not how it works.
> > > 
> > > This now relates strongly to a question of locking:
> > > 
> > > I ran the code (top of this message) all day, with a few instances in 
> > > dmesg (at irregular intervals, not wrapping):
> > > 
> > >   [161644.076666] snd_echo3g 0000:09:02.0: invalid position: , pos = 4096, buffer size = 4096, period size = 64
> > >   [163232.392501] snd_echo3g 0000:09:02.0: invalid position: , pos = 4096, buffer size = 4096, period size = 64
> > >   [164976.098069] snd_echo3g 0000:09:02.0: invalid position: , pos = 4096, buffer size = 4096, period size = 64
> > >   [165054.946225] snd_echo3g 0000:09:02.0: invalid position: , pos = 4096, buffer size = 4096, period size = 64
> > >   [165312.141545] snd_echo3g 0000:09:02.0: invalid position: , pos = 4096, buffer size = 4096, period size = 64
> > > 
> > > A definite bug, of course.
> > > 
> > > However (and I am happy to be corrected) the function never finishes with 
> > > position == buffer size. Only way is a race between interrupt handler and 
> > > pcm_pointer.
> > > 
> > > Therefore one of these is needed:
> > > 
> > > * pcm_pointer locks chip->lock
> > > 
> > >   Even though the docs emphasise PCM core has exclusivity, it it not worth 
> > >   much as it does not protect against the interrupt handler.
> > > 
> > >   But now interrupt handler becomes ugly in total: take chip->lock, check 
> > >   the counter, release chip->lock, go to PCM core (which takes middle 
> > >   layer locks), take chip->lock again, check counter again, release 
> > >   chip->lock again.
> > 
> > Yes, that's the most robust way to go.  If the lock is really costing
> > too much, you can consider a fast-path by some flag for the irq ->
> > snd_pcm_period_elapsed() case.
> 
> I don't understand how a fast path could be made to work, as it can't pass 
> data across snd_pcm_period_elapsed() and it still must syncronise access 
> between reading dma_counter and writing pipe->position.
> 
> Hence questioning if a better design is simpler interrupt handlers that 
> just enter PCM core.
>  
> > Basically you can do everything in the pointer callback.  The only 
> > requirement in the interrupt handle is basically judge whether you need 
> > the call of snd_pcm_period_elapsed() and call it.  The rest update could 
> > be done in the other places.
> 
> Thanks, please just another clarification:
> 
> I presume that calls to pcm_pointer are completely independent of the 
> period notifications?
> 
> ie. period notifications are at regular intervals, regardless of whether 
> pcm_pointer is called inbetween. pcm_pointer must not reset any state used 
> by the interrupt.
> 
> Which means we must handle when non-interrupt call to pcm_pointer causes a 
> period to elapse. The next interrupt handler must notify.
> 
> I can see in the original code uses chip->last_period exclusively by the 
> interrupt handler, and I removed it. Some comments around the intent would 
> help. I'll cross reference the original code with my new understanding.
> 
> My instinct here is that to preserve
> 
> - regular period notifications
> 
> - handle period_size not aligning with 32-bit counter
> 
> - no races between interrupt and pcm_pointer
> 
> that the clearest and bug-free implementation may be to separate the 
> interrupt (notifications) and pointer updates to separate state.
> 
> Then there's no lock and the only crossover is an atomic read of 
> dma_counter.
> 
> And that's what I will try -- thanks.

Ok so the implementation would look something like this below,
which I will run for the rest of the day:

* Clear separation of the period notification from position updates; only 
  syncronising is around dma_counter, no need for locks

* All counting is accumulated to avoid bugs in the cases of wrapping
  and non-alignment

It's easier to see in the end results but of course I'll work on a clear 
diff.

---

/******************************************************************************
	IRQ Handling
******************************************************************************/
/* Check if a period has elapsed since last interrupt
 *
 * Don't make any updates to state; PCM core handles this with the
 * correct locks.
 *
 * \return true if a period has elapsed, otherwise false
 */
static bool period_has_elapsed(struct snd_pcm_substream *substream)
{
	struct snd_pcm_runtime *runtime = substream->runtime;
	struct audiopipe *pipe = runtime->private_data;
	u32 counter, step;
	size_t period_bytes;

	if (pipe->state != PIPE_STATE_STARTED)
		return false;

	period_bytes = frames_to_bytes(runtime, runtime->period_size);

	counter = le32_to_cpu(*pipe->dma_counter);  /* presumed atomic */

	step = counter - pipe->last_period;  /* handles wrapping */
	step -= step % period_bytes;  /* acknowledge whole periods only */

	if (step == 0)
		return false;  /* haven't advanced a whole period yet */

	pipe->last_period += step;  /* used exclusively by us */
	return true;
}

static irqreturn_t snd_echo_interrupt(int irq, void *dev_id)
{
	struct echoaudio *chip = dev_id;
	int ss, st;

	spin_lock(&chip->lock);
	st = service_irq(chip);
	if (st < 0) {
		spin_unlock(&chip->lock);
		return IRQ_NONE;
	}
	/* 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 && period_has_elapsed(substream)) {
			spin_unlock(&chip->lock);
			snd_pcm_period_elapsed(substream);
			spin_lock(&chip->lock);
		}
	}
	spin_unlock(&chip->lock);

#ifdef ECHOCARD_HAS_MIDI
	if (st > 0 && chip->midi_in) {
		snd_rawmidi_receive(chip->midi_in, chip->midi_buffer, st);
		dev_dbg(chip->card->dev, "rawmidi_iread=%d\n", st);
	}
#endif
	return IRQ_HANDLED;
}


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;
	u32 counter, step;

	/*
	 * IRQ handling runs concurrently. Do not share tracking of
	 * counter with it, which would race or require locking
	 */

	counter = le32_to_cpu(*pipe->dma_counter);  /* presumed atomic */

	step = counter - pipe->last_counter;  /* handles wrapping */
	pipe->last_counter = counter;

	/* counter doesn't neccessarily wrap on a multiple of
	 * buffer_size, so can't derive the position; must
	 * accumulate */

	pipe->position += step;
	pipe->position %= frames_to_bytes(runtime, runtime->buffer_size); /* wrap */

	return bytes_to_frames(runtime, pipe->position);
}

-- 
Mark

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

[Giuliano Pochini]

On Tue, 16 Jun 2020 14:17:43 +0100
Mark Hills <mark@xwax.org> wrote:

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

Yes, it also happens here very rarely (one time every some months) and I failed to reproduce the problem.


> 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.

In that case the stream must be restarted anyway due to xrun.


> * 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.

Ok.


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

Why twice?


> +static int snd_echo_poll_substream(struct snd_pcm_substream *substream)
> [...]
>  static irqreturn_t snd_echo_interrupt(int irq, void *dev_id)
> [...]

Looks fine to me.


-- 
Giuliano.

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

[Mark Hills]

On Tue, 16 Jun 2020, Giuliano Pochini wrote:

> On Tue, 16 Jun 2020 14:17:43 +0100
> Mark Hills <mark@xwax.org> wrote:
> 
> > Distorted audio appears occasionally, affecting either playback
> > or capture and requiring the affected substream to be closed by
> > all applications and re-opened.
> 
> Yes, it also happens here very rarely (one time every some months) and I 
> failed to reproduce the problem.

It frustrated me for years, but I had other work I needed to do. 
Eventually I was working around it several times a day :-/
 
[...]
> > * 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.
> 
> In that case the stream must be restarted anyway due to xrun.

Yes, but I think it will go unnoticed, so you don't know to re-start.

[...]
> > * Remove the accessing of pipe->dma_counter twice in the interrupt
> >   handler:
> 
> Why twice?

Yes, the interrupt handler calls pcm_pointer() directly to make a 
decision. Then it calls snd_pcm_period_elapsed(), which itself goes on to 
make another call to pcm_pointer(), by this time the bus mastering of the 
interface has adjusted the counter.

The new code processes the counter only once, and this is easier to 
rationalise whether there are bugs.

-- 
Mark

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

[Giuliano Pochini]

On Tue, 16 Jun 2020 14:17:43 +0100
Mark Hills <mark@xwax.org> wrote:

> +	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;

Uhmm.. no, when the dma_counter wraps around the comparison gives wrong
result, unless 4G is divisible by the buffer size.

Please consider this patch (to apply after yours). It computes the new period
using pipe->position before and after adding the amount of bytes tranferred
since the previous period.
Briefly tested - It seems ok.


Signed-off-by: Giuliano Pochini <pochini@shiny.it>

--- sound/pci/echoaudio/echoaudio.c__orig	2020-06-16 23:36:02.818601055 +0200
+++ sound/pci/echoaudio/echoaudio.c	2020-06-16 23:52:46.593325066 +0200
@@ -1781,7 +1781,7 @@
 {
 	struct snd_pcm_runtime *runtime = substream->runtime;
 	struct audiopipe *pipe = runtime->private_data;
-	unsigned counter, step, period, last_period;
+	u32 counter, step, period, last_period, pipe_position;
 	size_t buffer_bytes;
 
 	if (pipe->state != PIPE_STATE_STARTED)
@@ -1789,24 +1789,22 @@
 
 	counter = le32_to_cpu(*pipe->dma_counter);
 
-	period = bytes_to_frames(runtime, counter)
+	step = counter - pipe->last_counter;
+	pipe_position = pipe->position + step;	/* bytes */
+	buffer_bytes = frames_to_bytes(runtime, runtime->buffer_size);
+	while (pipe_position >= buffer_bytes)
+		pipe_position -= buffer_bytes;
+
+	period = bytes_to_frames(runtime, pipe_position)
 		/ runtime->period_size;
-	last_period = bytes_to_frames(runtime, pipe->last_counter)
+	last_period = bytes_to_frames(runtime, pipe->position)
 		/ runtime->period_size;
 
 	if (period == last_period)
 		return 0;  /* don't do any work yet */
 
-	step = counter - pipe->last_counter;
+	pipe->position = pipe_position;
 	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;
 }
 


-- 
Giuliano.

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

[Mark Hills]

On Wed, 17 Jun 2020, Giuliano Pochini wrote:

> On Tue, 16 Jun 2020 14:17:43 +0100
> Mark Hills <mark@xwax.org> wrote:
> 
> > +	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;
> 
> Uhmm.. no, when the dma_counter wraps around the comparison gives wrong 
> result, unless 4G is divisible by the buffer size.

Agree.

> Please consider this patch (to apply after yours). It computes the new 
> period using pipe->position before and after adding the amount of bytes 
> tranferred since the previous period. Briefly tested - It seems ok.
> 
> 
> Signed-off-by: Giuliano Pochini <pochini@shiny.it>
> 
> --- sound/pci/echoaudio/echoaudio.c__orig	2020-06-16 23:36:02.818601055 +0200
> +++ sound/pci/echoaudio/echoaudio.c	2020-06-16 23:52:46.593325066 +0200
> @@ -1781,7 +1781,7 @@
>  {
>  	struct snd_pcm_runtime *runtime = substream->runtime;
>  	struct audiopipe *pipe = runtime->private_data;
> -	unsigned counter, step, period, last_period;
> +	u32 counter, step, period, last_period, pipe_position;
>  	size_t buffer_bytes;
>  
>  	if (pipe->state != PIPE_STATE_STARTED)
> @@ -1789,24 +1789,22 @@
>  
>  	counter = le32_to_cpu(*pipe->dma_counter);
>  
> -	period = bytes_to_frames(runtime, counter)
> +	step = counter - pipe->last_counter;
> +	pipe_position = pipe->position + step;	/* bytes */
> +	buffer_bytes = frames_to_bytes(runtime, runtime->buffer_size);
> +	while (pipe_position >= buffer_bytes)
> +		pipe_position -= buffer_bytes;
> +
> +	period = bytes_to_frames(runtime, pipe_position)
>  		/ runtime->period_size;
> -	last_period = bytes_to_frames(runtime, pipe->last_counter)
> +	last_period = bytes_to_frames(runtime, pipe->position)
>  		/ runtime->period_size;
>  
>  	if (period == last_period)
>  		return 0;  /* don't do any work yet */
>  
> -	step = counter - pipe->last_counter;
> +	pipe->position = pipe_position;
>  	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;

I think this risks returning to a case where it concludes nothing advances 
if the counter advances by a whole buffer?

You might be able to do the comparison before wrapping pipe_position, but 
hopefully you'll consider my patch in reply to Takashi has more clarity.

-- 
Mark

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

[Giuliano Pochini]

On Wed, 17 Jun 2020 12:14:42 +0100 (BST)
Mark Hills <mark@xwax.org> wrote:

> On Wed, 17 Jun 2020, Giuliano Pochini wrote:
> [...]
> > -	pipe->position += step;  /* bytes */
> > -
> > -	buffer_bytes = frames_to_bytes(runtime, runtime->buffer_size);
> > -
> > -	while (pipe->position >= buffer_bytes)
> > -		pipe->position -= buffer_bytes;
> > -
> >  	return 1;
> 
> I think this risks returning to a case where it concludes nothing
> advances if the counter advances by a whole buffer?

Yes, it can, but you can detect that case checking for step >= period_bytes.


> You might be able to do the comparison before wrapping pipe_position, but 
> hopefully you'll consider my patch in reply to Takashi has more clarity.

Your patch is very interesting. I didn't take into account the idea of
advancing the position by full periods only. If the PCM subsystem hasn't
changed much since I last checked (I wrote the driver many years ago), it
should work fine (and I'm sure you tested it). But I don't know if
something else requires better resolution.


-- 
Giuliano.

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

[Mark Hills]

On Fri, 19 Jun 2020, Giuliano Pochini wrote:

> On Wed, 17 Jun 2020 12:14:42 +0100 (BST)
> Mark Hills <mark@xwax.org> wrote:
> 
[...]
> > You might be able to do the comparison before wrapping pipe_position, 
> > but hopefully you'll consider my patch in reply to Takashi has more 
> > clarity.
> 
> Your patch is very interesting. I didn't take into account the idea of 
> advancing the position by full periods only. If the PCM subsystem hasn't 
> changed much since I last checked (I wrote the driver many years ago), 
> it should work fine (and I'm sure you tested it). But I don't know if 
> something else requires better resolution.

It's funny, but I didn't take account of the opposite; that there was any 
merits to polling inbetween the interrupts for better resolution.

Takashi pointed out the need for this and we had some discussion. Check 
the other thread, where I provided a newer revision of the code.

The good thing is I think we can have all the things we want and be bug 
free, just I have to understand the specification.

It would be great if you would like to take a look at the newer code for 
any problems you can see. I was going to run it for a few days then turn 
it into some patches.

-- 
Mark

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

[Giuliano Pochini]

On Fri, 19 Jun 2020 22:21:54 +0100 (BST)
Mark Hills <mark@xwax.org> wrote:

> On Fri, 19 Jun 2020, Giuliano Pochini wrote:
> 
> > On Wed, 17 Jun 2020 12:14:42 +0100 (BST)
> > Mark Hills <mark@xwax.org> wrote:
> > 
> [...]
> > > You might be able to do the comparison before wrapping pipe_position, 
> > > but hopefully you'll consider my patch in reply to Takashi has more 
> > > clarity.
> > 
> > Your patch is very interesting. I didn't take into account the idea of 
> > advancing the position by full periods only. If the PCM subsystem
> > hasn't changed much since I last checked (I wrote the driver many years
> > ago), it should work fine (and I'm sure you tested it). But I don't
> > know if something else requires better resolution.
> 
> It's funny, but I didn't take account of the opposite; that there was any 
> merits to polling inbetween the interrupts for better resolution.
> 
> Takashi pointed out the need for this and we had some discussion. Check 
> the other thread, where I provided a newer revision of the code.
> 
> The good thing is I think we can have all the things we want and be bug 
> free, just I have to understand the specification.
> 
> It would be great if you would like to take a look at the newer code for 
> any problems you can see. I was going to run it for a few days then turn 
> it into some patches.

I looked at your code and I think it's OK. I'm using it for some days
without any problem. I also stressed it with pretty tight timings and it
worked fine all the time.

Since I could not reproduce that problem before, except in some rare random
circumstances, I'm not a good tester at all. At most I can say that your
patch does not make things worse :)


-- 
Giuliano.

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

[Mark Hills]

On Mon, 29 Jun 2020, Giuliano Pochini wrote:

> On Fri, 19 Jun 2020 22:21:54 +0100 (BST)
> Mark Hills <mark@xwax.org> wrote:
> 
> > On Fri, 19 Jun 2020, Giuliano Pochini wrote:
> > 
> > > On Wed, 17 Jun 2020 12:14:42 +0100 (BST)
> > > Mark Hills <mark@xwax.org> wrote:
> > > 
> > [...]
> > > > You might be able to do the comparison before wrapping pipe_position, 
> > > > but hopefully you'll consider my patch in reply to Takashi has more 
> > > > clarity.
> > > 
> > > Your patch is very interesting. I didn't take into account the idea of 
> > > advancing the position by full periods only. If the PCM subsystem
> > > hasn't changed much since I last checked (I wrote the driver many years
> > > ago), it should work fine (and I'm sure you tested it). But I don't
> > > know if something else requires better resolution.
> > 
> > It's funny, but I didn't take account of the opposite; that there was any 
> > merits to polling inbetween the interrupts for better resolution.
> > 
> > Takashi pointed out the need for this and we had some discussion. Check 
> > the other thread, where I provided a newer revision of the code.
> > 
> > The good thing is I think we can have all the things we want and be bug 
> > free, just I have to understand the specification.
> > 
> > It would be great if you would like to take a look at the newer code for 
> > any problems you can see. I was going to run it for a few days then turn 
> > it into some patches.
> 
> I looked at your code and I think it's OK. I'm using it for some days 
> without any problem. I also stressed it with pretty tight timings and it 
> worked fine all the time.
> 
> Since I could not reproduce that problem before, except in some rare 
> random circumstances, I'm not a good tester at all. At most I can say 
> that your patch does not make things worse :)

What software are you using on the device, and are you using x86_64 and 
dmix?

I think some issues might be exaggerated by dmix which has a unique way of 
opening the device several times. And then chromium exercises dmix a lot 
with all of its threads/forks. That would I presume be how it exercises 
races between pcm_pointer and interrupts.

-- 
Mark

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

[Giuliano Pochini]

On Wed, 1 Jul 2020 13:25:07 +0100 (BST)
Mark Hills <mark@xwax.org> wrote:

> On Mon, 29 Jun 2020, Giuliano Pochini wrote:
> 
> > Since I could not reproduce that problem before, except in some rare 
> > random circumstances, I'm not a good tester at all. At most I can say 
> > that your patch does not make things worse :)
> 
> What software are you using on the device, and are you using x86_64 and 
> dmix?
> 
> I think some issues might be exaggerated by dmix which has a unique way
> of opening the device several times. And then chromium exercises dmix a
> lot with all of its threads/forks. That would I presume be how it
> exercises races between pcm_pointer and interrupts.

x86_64 now. When I wrote the driver I had a powermac. I also can test it on
a x86_32 laptop with an Indigo-IO card.

I tested both plughw and dmix, but I don't use the latter often.


-- 
Giuliano.

Re: echoaudio: Fix some long standing bugs

[Mark Hills]

Takashi, thank you for the feedback on the previous patches.

I will follow up with the final patches ready for merging. I've been 
running for many days now against kernel 5.6.14 with all issues resolved 
and no side effects.

I think it would be good to include these in a stable/LTS kernel. Is there 
some criteria, and can you assist with this?

Many thanks

-- 
Mark

[PATCH 1/4] echoaudio: Race conditions around "opencount"

[Mark Hills]

Use of atomics does not make these statements robust:

       atomic_inc(&chip->opencount);
       if (atomic_read(&chip->opencount) > 1 && chip->rate_set)
               chip->can_set_rate=0;

and

       if (atomic_read(&chip->opencount)) {
               if (chip->opencount) {
                       changed = -EAGAIN;
               } else {
                       changed = set_digital_mode(chip, dmode);

It would be necessary to atomically increment or decrement the value
and use the returned result. And yet we still need to prevent other
threads making use of "can_set_rate" while we set it.

However in all but one case the atomic is misleading as they are already
running with "mode_mutex" held.

Decisions are made on mode setting are often intrinsically connected
to "opencount" because some operations are not permitted unless
there is sole ownership.

So instead simplify this, and use "mode_mutex" as a lock for all reference
counting and mode setting.

Signed-off-by: Mark Hills <mark@xwax.org>
---
 sound/pci/echoaudio/echoaudio.c | 76 ++++++++++++++++++---------------
 sound/pci/echoaudio/echoaudio.h |  6 +--
 2 files changed, 45 insertions(+), 37 deletions(-)

diff --git a/sound/pci/echoaudio/echoaudio.c b/sound/pci/echoaudio/echoaudio.c
index 0941a7a17623..82a49dfd2384 100644
--- a/sound/pci/echoaudio/echoaudio.c
+++ b/sound/pci/echoaudio/echoaudio.c
@@ -245,13 +245,20 @@ static int hw_rule_sample_rate(struct snd_pcm_hw_params *params,
 						      SNDRV_PCM_HW_PARAM_RATE);
 	struct echoaudio *chip = rule->private;
 	struct snd_interval fixed;
+	int err;
+
+	mutex_lock(&chip->mode_mutex);
 
-	if (!chip->can_set_rate) {
+	if (chip->can_set_rate) {
+		err = 0;
+	} else {
 		snd_interval_any(&fixed);
 		fixed.min = fixed.max = chip->sample_rate;
-		return snd_interval_refine(rate, &fixed);
+		err = snd_interval_refine(rate, &fixed);
 	}
-	return 0;
+
+	mutex_unlock(&chip->mode_mutex);
+	return err;
 }
 
 
@@ -322,7 +329,7 @@ static int pcm_open(struct snd_pcm_substream *substream,
 				       SNDRV_PCM_HW_PARAM_RATE, -1)) < 0)
 		return err;
 
-	/* Finally allocate a page for the scatter-gather list */
+	/* Allocate a page for the scatter-gather list */
 	if ((err = snd_dma_alloc_pages(SNDRV_DMA_TYPE_DEV,
 				       &chip->pci->dev,
 				       PAGE_SIZE, &pipe->sgpage)) < 0) {
@@ -330,6 +337,17 @@ static int pcm_open(struct snd_pcm_substream *substream,
 		return err;
 	}
 
+	/*
+	 * Sole ownership required to set the rate
+	 */
+
+	dev_dbg(chip->card->dev, "pcm_open opencount=%d can_set_rate=%d, rate_set=%d",
+		chip->opencount, chip->can_set_rate, chip->rate_set);
+
+	chip->opencount++;
+	if (chip->opencount > 1 && chip->rate_set)
+		chip->can_set_rate = 0;
+
 	return 0;
 }
 
@@ -353,12 +371,7 @@ static int pcm_analog_in_open(struct snd_pcm_substream *substream)
 				       hw_rule_capture_format_by_channels, NULL,
 				       SNDRV_PCM_HW_PARAM_CHANNELS, -1)) < 0)
 		return err;
-	atomic_inc(&chip->opencount);
-	if (atomic_read(&chip->opencount) > 1 && chip->rate_set)
-		chip->can_set_rate=0;
-	dev_dbg(chip->card->dev, "pcm_analog_in_open  cs=%d  oc=%d  r=%d\n",
-		chip->can_set_rate, atomic_read(&chip->opencount),
-		chip->sample_rate);
+
 	return 0;
 }
 
@@ -388,12 +401,7 @@ static int pcm_analog_out_open(struct snd_pcm_substream *substream)
 				       NULL,
 				       SNDRV_PCM_HW_PARAM_CHANNELS, -1)) < 0)
 		return err;
-	atomic_inc(&chip->opencount);
-	if (atomic_read(&chip->opencount) > 1 && chip->rate_set)
-		chip->can_set_rate=0;
-	dev_dbg(chip->card->dev, "pcm_analog_out_open  cs=%d  oc=%d  r=%d\n",
-		chip->can_set_rate, atomic_read(&chip->opencount),
-		chip->sample_rate);
+
 	return 0;
 }
 
@@ -429,10 +437,6 @@ static int pcm_digital_in_open(struct snd_pcm_substream *substream)
 				       SNDRV_PCM_HW_PARAM_CHANNELS, -1)) < 0)
 		goto din_exit;
 
-	atomic_inc(&chip->opencount);
-	if (atomic_read(&chip->opencount) > 1 && chip->rate_set)
-		chip->can_set_rate=0;
-
 din_exit:
 	mutex_unlock(&chip->mode_mutex);
 	return err;
@@ -471,9 +475,7 @@ static int pcm_digital_out_open(struct snd_pcm_substream *substream)
 				       NULL, SNDRV_PCM_HW_PARAM_CHANNELS,
 				       -1)) < 0)
 		goto dout_exit;
-	atomic_inc(&chip->opencount);
-	if (atomic_read(&chip->opencount) > 1 && chip->rate_set)
-		chip->can_set_rate=0;
+
 dout_exit:
 	mutex_unlock(&chip->mode_mutex);
 	return err;
@@ -488,23 +490,29 @@ static int pcm_digital_out_open(struct snd_pcm_substream *substream)
 static int pcm_close(struct snd_pcm_substream *substream)
 {
 	struct echoaudio *chip = snd_pcm_substream_chip(substream);
-	int oc;
 
 	/* Nothing to do here. Audio is already off and pipe will be
 	 * freed by its callback
 	 */
 
-	atomic_dec(&chip->opencount);
-	oc = atomic_read(&chip->opencount);
-	dev_dbg(chip->card->dev, "pcm_close  oc=%d  cs=%d  rs=%d\n", oc,
-		chip->can_set_rate, chip->rate_set);
-	if (oc < 2)
+	mutex_lock(&chip->mode_mutex);
+
+	dev_dbg(chip->card->dev, "pcm_open opencount=%d can_set_rate=%d, rate_set=%d",
+		chip->opencount, chip->can_set_rate, chip->rate_set);
+
+	chip->opencount--;
+
+	switch (chip->opencount) {
+	case 1:
 		chip->can_set_rate = 1;
-	if (oc == 0)
+		break;
+
+	case 0:
 		chip->rate_set = 0;
-	dev_dbg(chip->card->dev, "pcm_close2 oc=%d  cs=%d  rs=%d\n", oc,
-		chip->can_set_rate, chip->rate_set);
+		break;
+	}
 
+	mutex_unlock(&chip->mode_mutex);
 	return 0;
 }
 
@@ -1409,7 +1417,7 @@ static int snd_echo_digital_mode_put(struct snd_kcontrol *kcontrol,
 		/* Do not allow the user to change the digital mode when a pcm
 		device is open because it also changes the number of channels
 		and the allowed sample rates */
-		if (atomic_read(&chip->opencount)) {
+		if (chip->opencount) {
 			changed = -EAGAIN;
 		} else {
 			changed = set_digital_mode(chip, dmode);
@@ -1874,7 +1882,7 @@ static int snd_echo_create(struct snd_card *card,
 		chip->card = card;
 		chip->pci = pci;
 		chip->irq = -1;
-		atomic_set(&chip->opencount, 0);
+		chip->opencount = 0;
 		mutex_init(&chip->mode_mutex);
 		chip->can_set_rate = 1;
 	} else {
diff --git a/sound/pci/echoaudio/echoaudio.h b/sound/pci/echoaudio/echoaudio.h
index be4d0489394a..6fd283e4e676 100644
--- a/sound/pci/echoaudio/echoaudio.h
+++ b/sound/pci/echoaudio/echoaudio.h
@@ -336,7 +336,7 @@ struct echoaudio {
 	struct mutex mode_mutex;
 	u16 num_digital_modes, digital_mode_list[6];
 	u16 num_clock_sources, clock_source_list[10];
-	atomic_t opencount;
+	unsigned int opencount;  /* protected by mode_mutex */
 	struct snd_kcontrol *clock_src_ctl;
 	struct snd_pcm *analog_pcm, *digital_pcm;
 	struct snd_card *card;
@@ -353,8 +353,8 @@ struct echoaudio {
 	struct timer_list timer;
 	char tinuse;				/* Timer in use */
 	char midi_full;				/* MIDI output buffer is full */
-	char can_set_rate;
-	char rate_set;
+	char can_set_rate;                      /* protected by mode_mutex */
+	char rate_set;                          /* protected by mode_mutex */
 
 	/* This stuff is used mainly by the lowlevel code */
 	struct comm_page *comm_page;	/* Virtual address of the memory
-- 
2.17.5

Re: [PATCH 1/4] echoaudio: Race conditions around "opencount"

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 2751 bytes --]

Hi Mark,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on sound/for-next]
[also build test WARNING on v5.8-rc3 next-20200701]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use  as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Mark-Hills/echoaudio-Race-conditions-around-opencount/20200701-203413
base:   https://git.kernel.org/pub/scm/linux/kernel/git/tiwai/sound.git for-next
config: mips-allyesconfig (attached as .config)
compiler: mips-linux-gcc (GCC) 9.3.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross ARCH=mips 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

   In file included from sound/pci/echoaudio/mona.c:123:
   sound/pci/echoaudio/mona_dsp.c: In function 'set_input_clock':
   sound/pci/echoaudio/mona_dsp.c:305:18: error: passing argument 1 of 'atomic_read' from incompatible pointer type [-Werror=incompatible-pointer-types]
     305 |  if (atomic_read(&chip->opencount))
         |                  ^~~~~~~~~~~~~~~~
         |                  |
         |                  unsigned int *
   In file included from include/linux/atomic.h:7,
                    from include/linux/cpumask.h:13,
                    from include/linux/interrupt.h:8,
                    from sound/pci/echoaudio/mona.c:37:
   arch/mips/include/asm/atomic.h:28:55: note: expected 'const atomic_t *' {aka 'const struct <anonymous> *'} but argument is of type 'unsigned int *'
      28 | static __always_inline type pfx##_read(const pfx##_t *v)  \
         |                                        ~~~~~~~~~~~~~~~^
>> arch/mips/include/asm/atomic.h:49:1: note: in expansion of macro 'ATOMIC_OPS'
      49 | ATOMIC_OPS(atomic, int)
         | ^~~~~~~~~~
   cc1: some warnings being treated as errors

vim +/ATOMIC_OPS +49 arch/mips/include/asm/atomic.h

^1da177e4c3f41 include/asm-mips/atomic.h      Linus Torvalds 2005-04-16  47  
1da7bce8591d58 arch/mips/include/asm/atomic.h Paul Burton    2019-10-01  48  #define ATOMIC_INIT(i)		{ (i) }
1da7bce8591d58 arch/mips/include/asm/atomic.h Paul Burton    2019-10-01 @49  ATOMIC_OPS(atomic, int)
^1da177e4c3f41 include/asm-mips/atomic.h      Linus Torvalds 2005-04-16  50  

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 67060 bytes --]

Re: [PATCH 1/4] echoaudio: Race conditions around "opencount"

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 17775 bytes --]

Hi Mark,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on sound/for-next]
[also build test ERROR on v5.8-rc3 next-20200701]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use  as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Mark-Hills/echoaudio-Race-conditions-around-opencount/20200701-203413
base:   https://git.kernel.org/pub/scm/linux/kernel/git/tiwai/sound.git for-next
config: arm-allyesconfig (attached as .config)
compiler: arm-linux-gnueabi-gcc (GCC) 9.3.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross ARCH=arm 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All error/warnings (new ones prefixed by >>):

   In file included from include/linux/kernel.h:11,
                    from include/linux/delay.h:22,
                    from sound/pci/echoaudio/mona.c:35:
   sound/pci/echoaudio/mona_dsp.c: In function 'set_input_clock':
>> arch/arm/include/asm/atomic.h:27:37: error: request for member 'counter' in something not a structure or union
      27 | #define atomic_read(v) READ_ONCE((v)->counter)
         |                                     ^~
   include/linux/compiler.h:372:9: note: in definition of macro '__compiletime_assert'
     372 |   if (!(condition))     \
         |         ^~~~~~~~~
   include/linux/compiler.h:392:2: note: in expansion of macro '_compiletime_assert'
     392 |  _compiletime_assert(condition, msg, __compiletime_assert_, __COUNTER__)
         |  ^~~~~~~~~~~~~~~~~~~
   include/linux/compiler.h:405:2: note: in expansion of macro 'compiletime_assert'
     405 |  compiletime_assert(__native_word(t) || sizeof(t) == sizeof(long long), \
         |  ^~~~~~~~~~~~~~~~~~
   include/linux/compiler.h:405:21: note: in expansion of macro '__native_word'
     405 |  compiletime_assert(__native_word(t) || sizeof(t) == sizeof(long long), \
         |                     ^~~~~~~~~~~~~
   include/linux/compiler.h:291:2: note: in expansion of macro 'compiletime_assert_rwonce_type'
     291 |  compiletime_assert_rwonce_type(x);    \
         |  ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>> arch/arm/include/asm/atomic.h:27:24: note: in expansion of macro 'READ_ONCE'
      27 | #define atomic_read(v) READ_ONCE((v)->counter)
         |                        ^~~~~~~~~
>> sound/pci/echoaudio/mona_dsp.c:305:6: note: in expansion of macro 'atomic_read'
     305 |  if (atomic_read(&chip->opencount))
         |      ^~~~~~~~~~~
>> arch/arm/include/asm/atomic.h:27:37: error: request for member 'counter' in something not a structure or union
      27 | #define atomic_read(v) READ_ONCE((v)->counter)
         |                                     ^~
   include/linux/compiler.h:372:9: note: in definition of macro '__compiletime_assert'
     372 |   if (!(condition))     \
         |         ^~~~~~~~~
   include/linux/compiler.h:392:2: note: in expansion of macro '_compiletime_assert'
     392 |  _compiletime_assert(condition, msg, __compiletime_assert_, __COUNTER__)
         |  ^~~~~~~~~~~~~~~~~~~
   include/linux/compiler.h:405:2: note: in expansion of macro 'compiletime_assert'
     405 |  compiletime_assert(__native_word(t) || sizeof(t) == sizeof(long long), \
         |  ^~~~~~~~~~~~~~~~~~
   include/linux/compiler.h:405:21: note: in expansion of macro '__native_word'
     405 |  compiletime_assert(__native_word(t) || sizeof(t) == sizeof(long long), \
         |                     ^~~~~~~~~~~~~
   include/linux/compiler.h:291:2: note: in expansion of macro 'compiletime_assert_rwonce_type'
     291 |  compiletime_assert_rwonce_type(x);    \
         |  ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>> arch/arm/include/asm/atomic.h:27:24: note: in expansion of macro 'READ_ONCE'
      27 | #define atomic_read(v) READ_ONCE((v)->counter)
         |                        ^~~~~~~~~
>> sound/pci/echoaudio/mona_dsp.c:305:6: note: in expansion of macro 'atomic_read'
     305 |  if (atomic_read(&chip->opencount))
         |      ^~~~~~~~~~~
>> arch/arm/include/asm/atomic.h:27:37: error: request for member 'counter' in something not a structure or union
      27 | #define atomic_read(v) READ_ONCE((v)->counter)
         |                                     ^~
   include/linux/compiler.h:372:9: note: in definition of macro '__compiletime_assert'
     372 |   if (!(condition))     \
         |         ^~~~~~~~~
   include/linux/compiler.h:392:2: note: in expansion of macro '_compiletime_assert'
     392 |  _compiletime_assert(condition, msg, __compiletime_assert_, __COUNTER__)
         |  ^~~~~~~~~~~~~~~~~~~
   include/linux/compiler.h:405:2: note: in expansion of macro 'compiletime_assert'
     405 |  compiletime_assert(__native_word(t) || sizeof(t) == sizeof(long long), \
         |  ^~~~~~~~~~~~~~~~~~
   include/linux/compiler.h:405:21: note: in expansion of macro '__native_word'
     405 |  compiletime_assert(__native_word(t) || sizeof(t) == sizeof(long long), \
         |                     ^~~~~~~~~~~~~
   include/linux/compiler.h:291:2: note: in expansion of macro 'compiletime_assert_rwonce_type'
     291 |  compiletime_assert_rwonce_type(x);    \
         |  ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>> arch/arm/include/asm/atomic.h:27:24: note: in expansion of macro 'READ_ONCE'
      27 | #define atomic_read(v) READ_ONCE((v)->counter)
         |                        ^~~~~~~~~
>> sound/pci/echoaudio/mona_dsp.c:305:6: note: in expansion of macro 'atomic_read'
     305 |  if (atomic_read(&chip->opencount))
         |      ^~~~~~~~~~~
>> arch/arm/include/asm/atomic.h:27:37: error: request for member 'counter' in something not a structure or union
      27 | #define atomic_read(v) READ_ONCE((v)->counter)
         |                                     ^~
   include/linux/compiler.h:372:9: note: in definition of macro '__compiletime_assert'
     372 |   if (!(condition))     \
         |         ^~~~~~~~~
   include/linux/compiler.h:392:2: note: in expansion of macro '_compiletime_assert'
     392 |  _compiletime_assert(condition, msg, __compiletime_assert_, __COUNTER__)
         |  ^~~~~~~~~~~~~~~~~~~
   include/linux/compiler.h:405:2: note: in expansion of macro 'compiletime_assert'
     405 |  compiletime_assert(__native_word(t) || sizeof(t) == sizeof(long long), \
         |  ^~~~~~~~~~~~~~~~~~
   include/linux/compiler.h:405:21: note: in expansion of macro '__native_word'
     405 |  compiletime_assert(__native_word(t) || sizeof(t) == sizeof(long long), \
         |                     ^~~~~~~~~~~~~
   include/linux/compiler.h:291:2: note: in expansion of macro 'compiletime_assert_rwonce_type'
     291 |  compiletime_assert_rwonce_type(x);    \
         |  ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>> arch/arm/include/asm/atomic.h:27:24: note: in expansion of macro 'READ_ONCE'
      27 | #define atomic_read(v) READ_ONCE((v)->counter)
         |                        ^~~~~~~~~
>> sound/pci/echoaudio/mona_dsp.c:305:6: note: in expansion of macro 'atomic_read'
     305 |  if (atomic_read(&chip->opencount))
         |      ^~~~~~~~~~~
>> arch/arm/include/asm/atomic.h:27:37: error: request for member 'counter' in something not a structure or union
      27 | #define atomic_read(v) READ_ONCE((v)->counter)
         |                                     ^~
   include/linux/compiler.h:372:9: note: in definition of macro '__compiletime_assert'
     372 |   if (!(condition))     \
         |         ^~~~~~~~~
   include/linux/compiler.h:392:2: note: in expansion of macro '_compiletime_assert'
     392 |  _compiletime_assert(condition, msg, __compiletime_assert_, __COUNTER__)
         |  ^~~~~~~~~~~~~~~~~~~
   include/linux/compiler.h:405:2: note: in expansion of macro 'compiletime_assert'
     405 |  compiletime_assert(__native_word(t) || sizeof(t) == sizeof(long long), \
         |  ^~~~~~~~~~~~~~~~~~
   include/linux/compiler.h:291:2: note: in expansion of macro 'compiletime_assert_rwonce_type'
     291 |  compiletime_assert_rwonce_type(x);    \
         |  ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>> arch/arm/include/asm/atomic.h:27:24: note: in expansion of macro 'READ_ONCE'
      27 | #define atomic_read(v) READ_ONCE((v)->counter)
         |                        ^~~~~~~~~
>> sound/pci/echoaudio/mona_dsp.c:305:6: note: in expansion of macro 'atomic_read'
     305 |  if (atomic_read(&chip->opencount))
         |      ^~~~~~~~~~~
   In file included from <command-line>:
>> arch/arm/include/asm/atomic.h:27:37: error: request for member 'counter' in something not a structure or union
      27 | #define atomic_read(v) READ_ONCE((v)->counter)
         |                                     ^~
   include/linux/compiler_types.h:290:13: note: in definition of macro '__unqual_scalar_typeof'
     290 |   _Generic((x),      \
         |             ^
   include/linux/compiler.h:292:2: note: in expansion of macro '__READ_ONCE_SCALAR'
     292 |  __READ_ONCE_SCALAR(x);      \
         |  ^~~~~~~~~~~~~~~~~~
>> arch/arm/include/asm/atomic.h:27:24: note: in expansion of macro 'READ_ONCE'
      27 | #define atomic_read(v) READ_ONCE((v)->counter)
         |                        ^~~~~~~~~
>> sound/pci/echoaudio/mona_dsp.c:305:6: note: in expansion of macro 'atomic_read'
     305 |  if (atomic_read(&chip->opencount))
         |      ^~~~~~~~~~~
>> arch/arm/include/asm/atomic.h:27:37: error: request for member 'counter' in something not a structure or union
      27 | #define atomic_read(v) READ_ONCE((v)->counter)
         |                                     ^~
   include/linux/compiler_types.h:290:13: note: in definition of macro '__unqual_scalar_typeof'
     290 |   _Generic((x),      \
         |             ^
   include/linux/compiler.h:284:34: note: in expansion of macro '__READ_ONCE'
     284 |  __unqual_scalar_typeof(x) __x = __READ_ONCE(x);   \
         |                                  ^~~~~~~~~~~
   include/linux/compiler.h:292:2: note: in expansion of macro '__READ_ONCE_SCALAR'
     292 |  __READ_ONCE_SCALAR(x);      \
         |  ^~~~~~~~~~~~~~~~~~
>> arch/arm/include/asm/atomic.h:27:24: note: in expansion of macro 'READ_ONCE'
      27 | #define atomic_read(v) READ_ONCE((v)->counter)
         |                        ^~~~~~~~~
   sound/pci/echoaudio/mona_dsp.c:305:6: note: in expansion of macro 'atomic_read'
     305 |  if (atomic_read(&chip->opencount))
         |      ^~~~~~~~~~~
   In file included from include/linux/kernel.h:11,
                    from include/linux/delay.h:22,
                    from sound/pci/echoaudio/mona.c:35:
   arch/arm/include/asm/atomic.h:27:37: error: request for member 'counter' in something not a structure or union
      27 | #define atomic_read(v) READ_ONCE((v)->counter)
         |                                     ^~
   include/linux/compiler.h:280:72: note: in definition of macro '__READ_ONCE'
     280 | #define __READ_ONCE(x) (*(const volatile __unqual_scalar_typeof(x) *)&(x))
         |                                                                        ^
   include/linux/compiler.h:292:2: note: in expansion of macro '__READ_ONCE_SCALAR'
     292 |  __READ_ONCE_SCALAR(x);      \
         |  ^~~~~~~~~~~~~~~~~~
   arch/arm/include/asm/atomic.h:27:24: note: in expansion of macro 'READ_ONCE'
      27 | #define atomic_read(v) READ_ONCE((v)->counter)
         |                        ^~~~~~~~~
   sound/pci/echoaudio/mona_dsp.c:305:6: note: in expansion of macro 'atomic_read'
     305 |  if (atomic_read(&chip->opencount))
         |      ^~~~~~~~~~~
   arch/arm/include/asm/atomic.h:27:37: error: request for member 'counter' in something not a structure or union
      27 | #define atomic_read(v) READ_ONCE((v)->counter)
         |                                     ^~
   include/linux/compiler.h:286:10: note: in definition of macro '__READ_ONCE_SCALAR'
     286 |  (typeof(x))__x;       \
         |          ^
   arch/arm/include/asm/atomic.h:27:24: note: in expansion of macro 'READ_ONCE'
      27 | #define atomic_read(v) READ_ONCE((v)->counter)
         |                        ^~~~~~~~~
   sound/pci/echoaudio/mona_dsp.c:305:6: note: in expansion of macro 'atomic_read'
     305 |  if (atomic_read(&chip->opencount))
         |      ^~~~~~~~~~~

vim +/atomic_read +305 sound/pci/echoaudio/mona_dsp.c

dd7b254d8dd3a9 Giuliano Pochini 2006-06-28  295  
dd7b254d8dd3a9 Giuliano Pochini 2006-06-28  296  
dd7b254d8dd3a9 Giuliano Pochini 2006-06-28  297  
dd7b254d8dd3a9 Giuliano Pochini 2006-06-28  298  static int set_input_clock(struct echoaudio *chip, u16 clock)
dd7b254d8dd3a9 Giuliano Pochini 2006-06-28  299  {
dd7b254d8dd3a9 Giuliano Pochini 2006-06-28  300  	u32 control_reg, clocks_from_dsp;
dd7b254d8dd3a9 Giuliano Pochini 2006-06-28  301  	int err;
dd7b254d8dd3a9 Giuliano Pochini 2006-06-28  302  
dd7b254d8dd3a9 Giuliano Pochini 2006-06-28  303  
dd7b254d8dd3a9 Giuliano Pochini 2006-06-28  304  	/* Prevent two simultaneous calls to switch_asic() */
dd7b254d8dd3a9 Giuliano Pochini 2006-06-28 @305  	if (atomic_read(&chip->opencount))
dd7b254d8dd3a9 Giuliano Pochini 2006-06-28  306  		return -EAGAIN;
dd7b254d8dd3a9 Giuliano Pochini 2006-06-28  307  
dd7b254d8dd3a9 Giuliano Pochini 2006-06-28  308  	/* Mask off the clock select bits */
dd7b254d8dd3a9 Giuliano Pochini 2006-06-28  309  	control_reg = le32_to_cpu(chip->comm_page->control_register) &
dd7b254d8dd3a9 Giuliano Pochini 2006-06-28  310  		GML_CLOCK_CLEAR_MASK;
dd7b254d8dd3a9 Giuliano Pochini 2006-06-28  311  	clocks_from_dsp = le32_to_cpu(chip->comm_page->status_clocks);
dd7b254d8dd3a9 Giuliano Pochini 2006-06-28  312  
dd7b254d8dd3a9 Giuliano Pochini 2006-06-28  313  	switch (clock) {
dd7b254d8dd3a9 Giuliano Pochini 2006-06-28  314  	case ECHO_CLOCK_INTERNAL:
dd7b254d8dd3a9 Giuliano Pochini 2006-06-28  315  		chip->input_clock = ECHO_CLOCK_INTERNAL;
dd7b254d8dd3a9 Giuliano Pochini 2006-06-28  316  		return set_sample_rate(chip, chip->sample_rate);
dd7b254d8dd3a9 Giuliano Pochini 2006-06-28  317  	case ECHO_CLOCK_SPDIF:
dd7b254d8dd3a9 Giuliano Pochini 2006-06-28  318  		if (chip->digital_mode == DIGITAL_MODE_ADAT)
dd7b254d8dd3a9 Giuliano Pochini 2006-06-28  319  			return -EAGAIN;
dd7b254d8dd3a9 Giuliano Pochini 2006-06-28  320  		spin_unlock_irq(&chip->lock);
dd7b254d8dd3a9 Giuliano Pochini 2006-06-28  321  		err = switch_asic(chip, clocks_from_dsp &
dd7b254d8dd3a9 Giuliano Pochini 2006-06-28  322  				  GML_CLOCK_DETECT_BIT_SPDIF96);
dd7b254d8dd3a9 Giuliano Pochini 2006-06-28  323  		spin_lock_irq(&chip->lock);
dd7b254d8dd3a9 Giuliano Pochini 2006-06-28  324  		if (err < 0)
dd7b254d8dd3a9 Giuliano Pochini 2006-06-28  325  			return err;
dd7b254d8dd3a9 Giuliano Pochini 2006-06-28  326  		control_reg |= GML_SPDIF_CLOCK;
dd7b254d8dd3a9 Giuliano Pochini 2006-06-28  327  		if (clocks_from_dsp & GML_CLOCK_DETECT_BIT_SPDIF96)
dd7b254d8dd3a9 Giuliano Pochini 2006-06-28  328  			control_reg |= GML_DOUBLE_SPEED_MODE;
dd7b254d8dd3a9 Giuliano Pochini 2006-06-28  329  		else
dd7b254d8dd3a9 Giuliano Pochini 2006-06-28  330  			control_reg &= ~GML_DOUBLE_SPEED_MODE;
dd7b254d8dd3a9 Giuliano Pochini 2006-06-28  331  		break;
dd7b254d8dd3a9 Giuliano Pochini 2006-06-28  332  	case ECHO_CLOCK_WORD:
dd7b254d8dd3a9 Giuliano Pochini 2006-06-28  333  		spin_unlock_irq(&chip->lock);
dd7b254d8dd3a9 Giuliano Pochini 2006-06-28  334  		err = switch_asic(chip, clocks_from_dsp &
dd7b254d8dd3a9 Giuliano Pochini 2006-06-28  335  				  GML_CLOCK_DETECT_BIT_WORD96);
dd7b254d8dd3a9 Giuliano Pochini 2006-06-28  336  		spin_lock_irq(&chip->lock);
dd7b254d8dd3a9 Giuliano Pochini 2006-06-28  337  		if (err < 0)
dd7b254d8dd3a9 Giuliano Pochini 2006-06-28  338  			return err;
dd7b254d8dd3a9 Giuliano Pochini 2006-06-28  339  		control_reg |= GML_WORD_CLOCK;
dd7b254d8dd3a9 Giuliano Pochini 2006-06-28  340  		if (clocks_from_dsp & GML_CLOCK_DETECT_BIT_WORD96)
dd7b254d8dd3a9 Giuliano Pochini 2006-06-28  341  			control_reg |= GML_DOUBLE_SPEED_MODE;
dd7b254d8dd3a9 Giuliano Pochini 2006-06-28  342  		else
dd7b254d8dd3a9 Giuliano Pochini 2006-06-28  343  			control_reg &= ~GML_DOUBLE_SPEED_MODE;
dd7b254d8dd3a9 Giuliano Pochini 2006-06-28  344  		break;
dd7b254d8dd3a9 Giuliano Pochini 2006-06-28  345  	case ECHO_CLOCK_ADAT:
b5b4a41b392960 Sudip Mukherjee  2014-11-03  346  		dev_dbg(chip->card->dev, "Set Mona clock to ADAT\n");
dd7b254d8dd3a9 Giuliano Pochini 2006-06-28  347  		if (chip->digital_mode != DIGITAL_MODE_ADAT)
dd7b254d8dd3a9 Giuliano Pochini 2006-06-28  348  			return -EAGAIN;
dd7b254d8dd3a9 Giuliano Pochini 2006-06-28  349  		control_reg |= GML_ADAT_CLOCK;
dd7b254d8dd3a9 Giuliano Pochini 2006-06-28  350  		control_reg &= ~GML_DOUBLE_SPEED_MODE;
dd7b254d8dd3a9 Giuliano Pochini 2006-06-28  351  		break;
dd7b254d8dd3a9 Giuliano Pochini 2006-06-28  352  	default:
b5b4a41b392960 Sudip Mukherjee  2014-11-03  353  		dev_err(chip->card->dev,
b5b4a41b392960 Sudip Mukherjee  2014-11-03  354  			"Input clock 0x%x not supported for Mona\n", clock);
dd7b254d8dd3a9 Giuliano Pochini 2006-06-28  355  		return -EINVAL;
dd7b254d8dd3a9 Giuliano Pochini 2006-06-28  356  	}
dd7b254d8dd3a9 Giuliano Pochini 2006-06-28  357  
dd7b254d8dd3a9 Giuliano Pochini 2006-06-28  358  	chip->input_clock = clock;
3f6175ece94735 Mark Brown       2015-08-10  359  	return write_control_reg(chip, control_reg, true);
dd7b254d8dd3a9 Giuliano Pochini 2006-06-28  360  }
dd7b254d8dd3a9 Giuliano Pochini 2006-06-28  361  

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 75683 bytes --]

Re: [PATCH 1/4] echoaudio: Race conditions around "opencount"

[Mark Hills]

Takashi, my apologies, it looks like this patch broke the build of the 
Mona driver.

Thankfully the change is simple, as it just looks like a bit of confusion 
of responsibilies in the code for the Mona interface; the correct fix is 
to remove the code.

That is a lesson for working with only the echo3g driver enabled. Now I 
have done a full build of all echoaudio drivers, with no warnings or 
errors.

Here's a patch, or it can be squashed into the original patch if 
necessary.

-- 
Mark


From 3c56faaa51436ca08dfe107aa1b06162904c216f Mon Sep 17 00:00:00 2001
From: Mark Hills <mark@xwax.org>
Date: Thu, 2 Jul 2020 10:25:43 +0100
Subject: [PATCH] echoaudio: The Mona build was broken by changes to opencount

The correct fix is to remove this check as it is always false.

It's not the responsibilty of the device-specific driver to make
this check, as it is already checked in snd_echo_digital_mode_put
before this code is called.

I do not have a Mona interface to test this change.

Signed-off-by: Mark Hills <mark@xwax.org>
---
 sound/pci/echoaudio/mona_dsp.c | 5 -----
 1 file changed, 5 deletions(-)

diff --git a/sound/pci/echoaudio/mona_dsp.c b/sound/pci/echoaudio/mona_dsp.c
index dce9e57d01c4..f77db83dd73d 100644
--- a/sound/pci/echoaudio/mona_dsp.c
+++ b/sound/pci/echoaudio/mona_dsp.c
@@ -300,11 +300,6 @@ static int set_input_clock(struct echoaudio *chip, u16 clock)
 	u32 control_reg, clocks_from_dsp;
 	int err;
 
-
-	/* Prevent two simultaneous calls to switch_asic() */
-	if (atomic_read(&chip->opencount))
-		return -EAGAIN;
-
 	/* Mask off the clock select bits */
 	control_reg = le32_to_cpu(chip->comm_page->control_register) &
 		GML_CLOCK_CLEAR_MASK;
-- 
2.17.5

Re: [PATCH 1/4] echoaudio: Race conditions around "opencount"

[Takashi Iwai]

On Thu, 02 Jul 2020 11:53:51 +0200,
Mark Hills wrote:
> 
> Takashi, my apologies, it looks like this patch broke the build of the 
> Mona driver.
> 
> Thankfully the change is simple, as it just looks like a bit of confusion 
> of responsibilies in the code for the Mona interface; the correct fix is 
> to remove the code.
> 
> That is a lesson for working with only the echo3g driver enabled. Now I 
> have done a full build of all echoaudio drivers, with no warnings or 
> errors.
> 
> Here's a patch, or it can be squashed into the original patch if 
> necessary.

Could you rather fold the fix into the patch and resubmit the whole
patch set?  I'm just back from travel, and it'd be better anyway if I
receive a fresh patch set to apply.


Thanks!

Takashi

Re: [PATCH 1/4] echoaudio: Race conditions around "opencount"

[Mark Hills]

On Tue, 7 Jul 2020, Takashi Iwai wrote:

> On Thu, 02 Jul 2020 11:53:51 +0200,
> Mark Hills wrote:
> > 
> > Takashi, my apologies, it looks like this patch broke the build of the 
> > Mona driver.
> > 
> > Thankfully the change is simple, as it just looks like a bit of confusion 
> > of responsibilies in the code for the Mona interface; the correct fix is 
> > to remove the code.
> > 
> > That is a lesson for working with only the echo3g driver enabled. Now I 
> > have done a full build of all echoaudio drivers, with no warnings or 
> > errors.
> > 
> > Here's a patch, or it can be squashed into the original patch if 
> > necessary.
> 
> Could you rather fold the fix into the patch and resubmit the whole
> patch set?  I'm just back from travel, and it'd be better anyway if I
> receive a fresh patch set to apply.

No problem, I'll follow up.

In the end, I decided it best to keep the patch separate, but re-order so 
as to not break the build.

-- 
Mark

[PATCH 1/5] echoaudio: Remove redundant check

[Mark Hills]

This check is always false, as it's not the responsibilty of the
device-specific code to make this check. It is already checked
in snd_echo_digital_mode_put.

I do not have a Mona interface to test this change.

This patch is in preparation for follow-up patch to modify the
behavior of "opencount".

Signed-off-by: Mark Hills <mark@xwax.org>
---
 sound/pci/echoaudio/mona_dsp.c | 5 -----
 1 file changed, 5 deletions(-)

diff --git a/sound/pci/echoaudio/mona_dsp.c b/sound/pci/echoaudio/mona_dsp.c
index dce9e57d01c4..f77db83dd73d 100644
--- a/sound/pci/echoaudio/mona_dsp.c
+++ b/sound/pci/echoaudio/mona_dsp.c
@@ -300,11 +300,6 @@ static int set_input_clock(struct echoaudio *chip, u16 clock)
 	u32 control_reg, clocks_from_dsp;
 	int err;
 
-
-	/* Prevent two simultaneous calls to switch_asic() */
-	if (atomic_read(&chip->opencount))
-		return -EAGAIN;
-
 	/* Mask off the clock select bits */
 	control_reg = le32_to_cpu(chip->comm_page->control_register) &
 		GML_CLOCK_CLEAR_MASK;
-- 
2.17.5

Re: [PATCH 1/5] echoaudio: Remove redundant check

[Takashi Iwai]

On Wed, 08 Jul 2020 12:18:44 +0200,
Mark Hills wrote:
> 
> This check is always false, as it's not the responsibilty of the
> device-specific code to make this check. It is already checked
> in snd_echo_digital_mode_put.
> 
> I do not have a Mona interface to test this change.
> 
> This patch is in preparation for follow-up patch to modify the
> behavior of "opencount".
> 
> Signed-off-by: Mark Hills <mark@xwax.org>

Applied now.  Thanks.


Takashi

[PATCH 2/5] echoaudio: Race conditions around "opencount"

[Mark Hills]

Use of atomics does not make these statements robust:

       atomic_inc(&chip->opencount);
       if (atomic_read(&chip->opencount) > 1 && chip->rate_set)
               chip->can_set_rate=0;

and

       if (atomic_read(&chip->opencount)) {
               if (chip->opencount) {
                       changed = -EAGAIN;
               } else {
                       changed = set_digital_mode(chip, dmode);

It would be necessary to atomically increment or decrement the value
and use the returned result. And yet we still need to prevent other
threads making use of "can_set_rate" while we set it.

However in all but one case the atomic is misleading as they are already
running with "mode_mutex" held.

Decisions are made on mode setting are often intrinsically connected
to "opencount" because some operations are not permitted unless
there is sole ownership.

So instead simplify this, and use "mode_mutex" as a lock for all reference
counting and mode setting.

Signed-off-by: Mark Hills <mark@xwax.org>
---
 sound/pci/echoaudio/echoaudio.c | 76 ++++++++++++++++++---------------
 sound/pci/echoaudio/echoaudio.h |  6 +--
 2 files changed, 45 insertions(+), 37 deletions(-)

diff --git a/sound/pci/echoaudio/echoaudio.c b/sound/pci/echoaudio/echoaudio.c
index 0941a7a17623..82a49dfd2384 100644
--- a/sound/pci/echoaudio/echoaudio.c
+++ b/sound/pci/echoaudio/echoaudio.c
@@ -245,13 +245,20 @@ static int hw_rule_sample_rate(struct snd_pcm_hw_params *params,
 						      SNDRV_PCM_HW_PARAM_RATE);
 	struct echoaudio *chip = rule->private;
 	struct snd_interval fixed;
+	int err;
+
+	mutex_lock(&chip->mode_mutex);
 
-	if (!chip->can_set_rate) {
+	if (chip->can_set_rate) {
+		err = 0;
+	} else {
 		snd_interval_any(&fixed);
 		fixed.min = fixed.max = chip->sample_rate;
-		return snd_interval_refine(rate, &fixed);
+		err = snd_interval_refine(rate, &fixed);
 	}
-	return 0;
+
+	mutex_unlock(&chip->mode_mutex);
+	return err;
 }
 
 
@@ -322,7 +329,7 @@ static int pcm_open(struct snd_pcm_substream *substream,
 				       SNDRV_PCM_HW_PARAM_RATE, -1)) < 0)
 		return err;
 
-	/* Finally allocate a page for the scatter-gather list */
+	/* Allocate a page for the scatter-gather list */
 	if ((err = snd_dma_alloc_pages(SNDRV_DMA_TYPE_DEV,
 				       &chip->pci->dev,
 				       PAGE_SIZE, &pipe->sgpage)) < 0) {
@@ -330,6 +337,17 @@ static int pcm_open(struct snd_pcm_substream *substream,
 		return err;
 	}
 
+	/*
+	 * Sole ownership required to set the rate
+	 */
+
+	dev_dbg(chip->card->dev, "pcm_open opencount=%d can_set_rate=%d, rate_set=%d",
+		chip->opencount, chip->can_set_rate, chip->rate_set);
+
+	chip->opencount++;
+	if (chip->opencount > 1 && chip->rate_set)
+		chip->can_set_rate = 0;
+
 	return 0;
 }
 
@@ -353,12 +371,7 @@ static int pcm_analog_in_open(struct snd_pcm_substream *substream)
 				       hw_rule_capture_format_by_channels, NULL,
 				       SNDRV_PCM_HW_PARAM_CHANNELS, -1)) < 0)
 		return err;
-	atomic_inc(&chip->opencount);
-	if (atomic_read(&chip->opencount) > 1 && chip->rate_set)
-		chip->can_set_rate=0;
-	dev_dbg(chip->card->dev, "pcm_analog_in_open  cs=%d  oc=%d  r=%d\n",
-		chip->can_set_rate, atomic_read(&chip->opencount),
-		chip->sample_rate);
+
 	return 0;
 }
 
@@ -388,12 +401,7 @@ static int pcm_analog_out_open(struct snd_pcm_substream *substream)
 				       NULL,
 				       SNDRV_PCM_HW_PARAM_CHANNELS, -1)) < 0)
 		return err;
-	atomic_inc(&chip->opencount);
-	if (atomic_read(&chip->opencount) > 1 && chip->rate_set)
-		chip->can_set_rate=0;
-	dev_dbg(chip->card->dev, "pcm_analog_out_open  cs=%d  oc=%d  r=%d\n",
-		chip->can_set_rate, atomic_read(&chip->opencount),
-		chip->sample_rate);
+
 	return 0;
 }
 
@@ -429,10 +437,6 @@ static int pcm_digital_in_open(struct snd_pcm_substream *substream)
 				       SNDRV_PCM_HW_PARAM_CHANNELS, -1)) < 0)
 		goto din_exit;
 
-	atomic_inc(&chip->opencount);
-	if (atomic_read(&chip->opencount) > 1 && chip->rate_set)
-		chip->can_set_rate=0;
-
 din_exit:
 	mutex_unlock(&chip->mode_mutex);
 	return err;
@@ -471,9 +475,7 @@ static int pcm_digital_out_open(struct snd_pcm_substream *substream)
 				       NULL, SNDRV_PCM_HW_PARAM_CHANNELS,
 				       -1)) < 0)
 		goto dout_exit;
-	atomic_inc(&chip->opencount);
-	if (atomic_read(&chip->opencount) > 1 && chip->rate_set)
-		chip->can_set_rate=0;
+
 dout_exit:
 	mutex_unlock(&chip->mode_mutex);
 	return err;
@@ -488,23 +490,29 @@ static int pcm_digital_out_open(struct snd_pcm_substream *substream)
 static int pcm_close(struct snd_pcm_substream *substream)
 {
 	struct echoaudio *chip = snd_pcm_substream_chip(substream);
-	int oc;
 
 	/* Nothing to do here. Audio is already off and pipe will be
 	 * freed by its callback
 	 */
 
-	atomic_dec(&chip->opencount);
-	oc = atomic_read(&chip->opencount);
-	dev_dbg(chip->card->dev, "pcm_close  oc=%d  cs=%d  rs=%d\n", oc,
-		chip->can_set_rate, chip->rate_set);
-	if (oc < 2)
+	mutex_lock(&chip->mode_mutex);
+
+	dev_dbg(chip->card->dev, "pcm_open opencount=%d can_set_rate=%d, rate_set=%d",
+		chip->opencount, chip->can_set_rate, chip->rate_set);
+
+	chip->opencount--;
+
+	switch (chip->opencount) {
+	case 1:
 		chip->can_set_rate = 1;
-	if (oc == 0)
+		break;
+
+	case 0:
 		chip->rate_set = 0;
-	dev_dbg(chip->card->dev, "pcm_close2 oc=%d  cs=%d  rs=%d\n", oc,
-		chip->can_set_rate, chip->rate_set);
+		break;
+	}
 
+	mutex_unlock(&chip->mode_mutex);
 	return 0;
 }
 
@@ -1409,7 +1417,7 @@ static int snd_echo_digital_mode_put(struct snd_kcontrol *kcontrol,
 		/* Do not allow the user to change the digital mode when a pcm
 		device is open because it also changes the number of channels
 		and the allowed sample rates */
-		if (atomic_read(&chip->opencount)) {
+		if (chip->opencount) {
 			changed = -EAGAIN;
 		} else {
 			changed = set_digital_mode(chip, dmode);
@@ -1874,7 +1882,7 @@ static int snd_echo_create(struct snd_card *card,
 		chip->card = card;
 		chip->pci = pci;
 		chip->irq = -1;
-		atomic_set(&chip->opencount, 0);
+		chip->opencount = 0;
 		mutex_init(&chip->mode_mutex);
 		chip->can_set_rate = 1;
 	} else {
diff --git a/sound/pci/echoaudio/echoaudio.h b/sound/pci/echoaudio/echoaudio.h
index be4d0489394a..6fd283e4e676 100644
--- a/sound/pci/echoaudio/echoaudio.h
+++ b/sound/pci/echoaudio/echoaudio.h
@@ -336,7 +336,7 @@ struct echoaudio {
 	struct mutex mode_mutex;
 	u16 num_digital_modes, digital_mode_list[6];
 	u16 num_clock_sources, clock_source_list[10];
-	atomic_t opencount;
+	unsigned int opencount;  /* protected by mode_mutex */
 	struct snd_kcontrol *clock_src_ctl;
 	struct snd_pcm *analog_pcm, *digital_pcm;
 	struct snd_card *card;
@@ -353,8 +353,8 @@ struct echoaudio {
 	struct timer_list timer;
 	char tinuse;				/* Timer in use */
 	char midi_full;				/* MIDI output buffer is full */
-	char can_set_rate;
-	char rate_set;
+	char can_set_rate;                      /* protected by mode_mutex */
+	char rate_set;                          /* protected by mode_mutex */
 
 	/* This stuff is used mainly by the lowlevel code */
 	struct comm_page *comm_page;	/* Virtual address of the memory
-- 
2.17.5

Re: [PATCH 2/5] echoaudio: Race conditions around "opencount"

[Takashi Iwai]

On Wed, 08 Jul 2020 12:18:45 +0200,
Mark Hills wrote:
> 
> Use of atomics does not make these statements robust:
> 
>        atomic_inc(&chip->opencount);
>        if (atomic_read(&chip->opencount) > 1 && chip->rate_set)
>                chip->can_set_rate=0;
> 
> and
> 
>        if (atomic_read(&chip->opencount)) {
>                if (chip->opencount) {
>                        changed = -EAGAIN;
>                } else {
>                        changed = set_digital_mode(chip, dmode);
> 
> It would be necessary to atomically increment or decrement the value
> and use the returned result. And yet we still need to prevent other
> threads making use of "can_set_rate" while we set it.
> 
> However in all but one case the atomic is misleading as they are already
> running with "mode_mutex" held.
> 
> Decisions are made on mode setting are often intrinsically connected
> to "opencount" because some operations are not permitted unless
> there is sole ownership.
> 
> So instead simplify this, and use "mode_mutex" as a lock for all reference
> counting and mode setting.
> 
> Signed-off-by: Mark Hills <mark@xwax.org>

Applied now.  Thanks.


Takashi

[PATCH 3/5] echoaudio: Prevent races in calls to set_audio_format()

[Mark Hills]

The function uses chip->comm_page which needs locking against
other use at the same time.

Signed-off-by: Mark Hills <mark@xwax.org>
---
 sound/pci/echoaudio/echoaudio.c | 15 ++++++++++++++-
 1 file changed, 14 insertions(+), 1 deletion(-)

diff --git a/sound/pci/echoaudio/echoaudio.c b/sound/pci/echoaudio/echoaudio.c
index 82a49dfd2384..19002d785d8d 100644
--- a/sound/pci/echoaudio/echoaudio.c
+++ b/sound/pci/echoaudio/echoaudio.c
@@ -711,9 +711,22 @@ static int pcm_prepare(struct snd_pcm_substream *substream)
 
 	if (snd_BUG_ON(pipe_index >= px_num(chip)))
 		return -EINVAL;
-	if (snd_BUG_ON(!is_pipe_allocated(chip, pipe_index)))
+
+	/*
+	 * We passed checks we can do independently; now take
+	 * exclusive control
+	 */
+
+	spin_lock_irq(&chip->lock);
+
+	if (snd_BUG_ON(!is_pipe_allocated(chip, pipe_index))) {
+		spin_unlock(&chip->lock);
 		return -EINVAL;
+	}
+
 	set_audio_format(chip, pipe_index, &format);
+	spin_unlock_irq(&chip->lock);
+
 	return 0;
 }
 
-- 
2.17.5

Re: [PATCH 3/5] echoaudio: Prevent races in calls to set_audio_format()

[Takashi Iwai]

On Wed, 08 Jul 2020 12:18:46 +0200,
Mark Hills wrote:
> 
> The function uses chip->comm_page which needs locking against
> other use at the same time.
> 
> Signed-off-by: Mark Hills <mark@xwax.org>

Applied now.  Thanks.


Takashi

[PATCH 4/5] echoaudio: Prevent some noise on unloading the module

[Mark Hills]

These are valid conditions in normal circumstances, so do not "warn" but
make them for debugging.

Signed-off-by: Mark Hills <mark@xwax.org>
---
 sound/pci/echoaudio/echoaudio_dsp.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/sound/pci/echoaudio/echoaudio_dsp.c b/sound/pci/echoaudio/echoaudio_dsp.c
index f02f5b1568de..d10d0e460f0b 100644
--- a/sound/pci/echoaudio/echoaudio_dsp.c
+++ b/sound/pci/echoaudio/echoaudio_dsp.c
@@ -898,7 +898,7 @@ static int pause_transport(struct echoaudio *chip, u32 channel_mask)
 		return 0;
 	}
 
-	dev_warn(chip->card->dev, "pause_transport: No pipes to stop!\n");
+	dev_dbg(chip->card->dev, "pause_transport: No pipes to stop!\n");
 	return 0;
 }
 
@@ -924,7 +924,7 @@ static int stop_transport(struct echoaudio *chip, u32 channel_mask)
 		return 0;
 	}
 
-	dev_warn(chip->card->dev, "stop_transport: No pipes to stop!\n");
+	dev_dbg(chip->card->dev, "stop_transport: No pipes to stop!\n");
 	return 0;
 }
 
-- 
2.17.5

Re: [PATCH 4/5] echoaudio: Prevent some noise on unloading the module

[Takashi Iwai]

On Wed, 08 Jul 2020 12:18:47 +0200,
Mark Hills wrote:
> 
> These are valid conditions in normal circumstances, so do not "warn" but
> make them for debugging.
> 
> Signed-off-by: Mark Hills <mark@xwax.org>

Applied now.  Thanks.


Takashi

[PATCH 5/5] 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. I ruled out 32-bit counter wrapping; it often happens
much earlier.

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

The patch targets the following issues:

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

  This is a clean way to address a possible bug where if a whole
  ringbuffer advances between interrupts, it goes unnoticed.

* Move last_period state from chip to pipe

  This more logically belongs as part of pipe, and code is reasier to
  read if it is "counter position last time a period elapsed".

  Now the code has no references to period count. A period is just
  when the regular counter crosses a threshold. This increases
  readability and reduces scope for bugs.

* Treat period notification and buffer advance independently:

  This helps to clarify what is the responsibility of the interrupt
  handler, and what is pcm_pointer().

  Removing shared state between these operations means race conditions
  are fixed without introducing locks. Synchronisation is only around
  the read of pipe->dma_counter. There may be cache line contention
  around "struct audiopipe" but I did not have cause to profile this.

Pay attention to be robust where dma_counter wrapping is not a
multiple of period_size or buffer_size.

This is a revised patch based on feedback from Takashi and Giuliano.

Signed-off-by: Mark Hills <mark@xwax.org>
---
 sound/pci/echoaudio/echoaudio.c | 85 +++++++++++++++++++++++----------
 sound/pci/echoaudio/echoaudio.h |  8 +++-
 2 files changed, 65 insertions(+), 28 deletions(-)

diff --git a/sound/pci/echoaudio/echoaudio.c b/sound/pci/echoaudio/echoaudio.c
index 19002d785d8d..347e96038908 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@shiny.it>
+ *  Copyright (C) 2020 Mark Hills <mark@xwax.org>
  */
 
 #include <linux/module.h>
@@ -590,7 +591,7 @@ 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_period = 0;
 	pipe->last_counter = 0;
 	pipe->position = 0;
 	smp_wmb();
@@ -759,7 +760,7 @@ 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_period = 0;
 					pipe->last_counter = 0;
 					pipe->position = 0;
 					*pipe->dma_counter = 0;
@@ -807,19 +808,26 @@ 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;
+	u32 counter, step;
 
-	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);
+	/*
+	 * IRQ handling runs concurrently. Do not share tracking of
+	 * counter with it, which would race or require locking
+	 */
 
-	while (pos >= bufsize) {
-		pipe->position -= frames_to_bytes(substream->runtime, bufsize);
-		pos -= bufsize;
-	}
-	return pos;
+	counter = le32_to_cpu(*pipe->dma_counter);  /* presumed atomic */
+
+	step = counter - pipe->last_counter;  /* handles wrapping */
+	pipe->last_counter = counter;
+
+	/* counter doesn't neccessarily wrap on a multiple of
+	 * buffer_size, so can't derive the position; must
+	 * accumulate */
+
+	pipe->position += step;
+	pipe->position %= frames_to_bytes(runtime, runtime->buffer_size); /* wrap */
+
+	return bytes_to_frames(runtime, pipe->position);
 }
 
 
@@ -1782,14 +1790,43 @@ static const struct snd_kcontrol_new snd_echo_channels_info = {
 
 
 /******************************************************************************
-	IRQ Handler
+	IRQ Handling
 ******************************************************************************/
+/* Check if a period has elapsed since last interrupt
+ *
+ * Don't make any updates to state; PCM core handles this with the
+ * correct locks.
+ *
+ * \return true if a period has elapsed, otherwise false
+ */
+static bool period_has_elapsed(struct snd_pcm_substream *substream)
+{
+	struct snd_pcm_runtime *runtime = substream->runtime;
+	struct audiopipe *pipe = runtime->private_data;
+	u32 counter, step;
+	size_t period_bytes;
+
+	if (pipe->state != PIPE_STATE_STARTED)
+		return false;
+
+	period_bytes = frames_to_bytes(runtime, runtime->period_size);
+
+	counter = le32_to_cpu(*pipe->dma_counter);  /* presumed atomic */
+
+	step = counter - pipe->last_period;  /* handles wrapping */
+	step -= step % period_bytes;  /* acknowledge whole periods only */
+
+	if (step == 0)
+		return false;  /* haven't advanced a whole period yet */
+
+	pipe->last_period += step;  /* used exclusively by us */
+	return true;
+}
 
 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,17 +1837,13 @@ 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 && period_has_elapsed(substream)) {
+			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..30c640931f1e 100644
--- a/sound/pci/echoaudio/echoaudio.h
+++ b/sound/pci/echoaudio/echoaudio.h
@@ -298,7 +298,12 @@ struct audiopipe {
 					 * the current dma position
 					 * (lower 32 bits only)
 					 */
-	u32 last_counter;		/* The last position, which is used
+	u32 last_period;                /* Counter position last time a
+					 * period elapsed
+					 */
+	u32 last_counter;		/* Used exclusively by pcm_pointer
+					 * under PCM core locks.
+					 * The last position, which is used
 					 * to compute...
 					 */
 	u32 position;			/* ...the number of bytes tranferred
@@ -332,7 +337,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

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

[Takashi Iwai]

On Wed, 08 Jul 2020 12:18:48 +0200,
Mark Hills wrote:
> 
> 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. I ruled out 32-bit counter wrapping; it often happens
> much earlier.
> 
> Since applying this patch I have not had problems, where previously
> they would occur several times a day.
> 
> The patch targets the following issues:
> 
> * Check for progress using the counter from the hardware, not after it
>   has been truncated to the buffer.
> 
>   This is a clean way to address a possible bug where if a whole
>   ringbuffer advances between interrupts, it goes unnoticed.
> 
> * Move last_period state from chip to pipe
> 
>   This more logically belongs as part of pipe, and code is reasier to
>   read if it is "counter position last time a period elapsed".
> 
>   Now the code has no references to period count. A period is just
>   when the regular counter crosses a threshold. This increases
>   readability and reduces scope for bugs.
> 
> * Treat period notification and buffer advance independently:
> 
>   This helps to clarify what is the responsibility of the interrupt
>   handler, and what is pcm_pointer().
> 
>   Removing shared state between these operations means race conditions
>   are fixed without introducing locks. Synchronisation is only around
>   the read of pipe->dma_counter. There may be cache line contention
>   around "struct audiopipe" but I did not have cause to profile this.
> 
> Pay attention to be robust where dma_counter wrapping is not a
> multiple of period_size or buffer_size.
> 
> This is a revised patch based on feedback from Takashi and Giuliano.
> 
> Signed-off-by: Mark Hills <mark@xwax.org>

Applied now.  Thanks.


Takashi

[PATCH 2/4] echoaudio: Prevent races in calls to set_audio_format()

[Mark Hills]

The function uses chip->comm_page which needs locking against
other use at the same time.

Signed-off-by: Mark Hills <mark@xwax.org>
---
 sound/pci/echoaudio/echoaudio.c | 15 ++++++++++++++-
 1 file changed, 14 insertions(+), 1 deletion(-)

diff --git a/sound/pci/echoaudio/echoaudio.c b/sound/pci/echoaudio/echoaudio.c
index 82a49dfd2384..19002d785d8d 100644
--- a/sound/pci/echoaudio/echoaudio.c
+++ b/sound/pci/echoaudio/echoaudio.c
@@ -711,9 +711,22 @@ static int pcm_prepare(struct snd_pcm_substream *substream)
 
 	if (snd_BUG_ON(pipe_index >= px_num(chip)))
 		return -EINVAL;
-	if (snd_BUG_ON(!is_pipe_allocated(chip, pipe_index)))
+
+	/*
+	 * We passed checks we can do independently; now take
+	 * exclusive control
+	 */
+
+	spin_lock_irq(&chip->lock);
+
+	if (snd_BUG_ON(!is_pipe_allocated(chip, pipe_index))) {
+		spin_unlock(&chip->lock);
 		return -EINVAL;
+	}
+
 	set_audio_format(chip, pipe_index, &format);
+	spin_unlock_irq(&chip->lock);
+
 	return 0;
 }
 
-- 
2.17.5

[PATCH 3/4] echoaudio: Prevent some noise on unloading the module

[Mark Hills]

These are valid conditions in normal circumstances, so do not "warn" but
make them for debugging.

Signed-off-by: Mark Hills <mark@xwax.org>
---
 sound/pci/echoaudio/echoaudio_dsp.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/sound/pci/echoaudio/echoaudio_dsp.c b/sound/pci/echoaudio/echoaudio_dsp.c
index f02f5b1568de..d10d0e460f0b 100644
--- a/sound/pci/echoaudio/echoaudio_dsp.c
+++ b/sound/pci/echoaudio/echoaudio_dsp.c
@@ -898,7 +898,7 @@ static int pause_transport(struct echoaudio *chip, u32 channel_mask)
 		return 0;
 	}
 
-	dev_warn(chip->card->dev, "pause_transport: No pipes to stop!\n");
+	dev_dbg(chip->card->dev, "pause_transport: No pipes to stop!\n");
 	return 0;
 }
 
@@ -924,7 +924,7 @@ static int stop_transport(struct echoaudio *chip, u32 channel_mask)
 		return 0;
 	}
 
-	dev_warn(chip->card->dev, "stop_transport: No pipes to stop!\n");
+	dev_dbg(chip->card->dev, "stop_transport: No pipes to stop!\n");
 	return 0;
 }
 
-- 
2.17.5

[PATCH 4/4] 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. I ruled out 32-bit counter wrapping; it often happens
much earlier.

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

The patch targets the following issues:

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

  This is a clean way to address a possible bug where if a whole
  ringbuffer advances between interrupts, it goes unnoticed.

* Move last_period state from chip to pipe

  This more logically belongs as part of pipe, and code is reasier to
  read if it is "counter position last time a period elapsed".

  Now the code has no references to period count. A period is just
  when the regular counter crosses a threshold. This increases
  readability and reduces scope for bugs.

* Treat period notification and buffer advance independently:

  This helps to clarify what is the responsibility of the interrupt
  handler, and what is pcm_pointer().

  Removing shared state between these operations means race conditions
  are fixed without introducing locks. Synchronisation is only around
  the read of pipe->dma_counter. There may be cache line contention
  around "struct audiopipe" but I did not have cause to profile this.

Pay attention to be robust where dma_counter wrapping is not a
multiple of period_size or buffer_size.

This is a revised patch based on feedback from Takashi and Giuliano.

Signed-off-by: Mark Hills <mark@xwax.org>
---
 sound/pci/echoaudio/echoaudio.c | 85 +++++++++++++++++++++++----------
 sound/pci/echoaudio/echoaudio.h |  8 +++-
 2 files changed, 65 insertions(+), 28 deletions(-)

diff --git a/sound/pci/echoaudio/echoaudio.c b/sound/pci/echoaudio/echoaudio.c
index 19002d785d8d..347e96038908 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@shiny.it>
+ *  Copyright (C) 2020 Mark Hills <mark@xwax.org>
  */
 
 #include <linux/module.h>
@@ -590,7 +591,7 @@ 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_period = 0;
 	pipe->last_counter = 0;
 	pipe->position = 0;
 	smp_wmb();
@@ -759,7 +760,7 @@ 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_period = 0;
 					pipe->last_counter = 0;
 					pipe->position = 0;
 					*pipe->dma_counter = 0;
@@ -807,19 +808,26 @@ 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;
+	u32 counter, step;
 
-	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);
+	/*
+	 * IRQ handling runs concurrently. Do not share tracking of
+	 * counter with it, which would race or require locking
+	 */
 
-	while (pos >= bufsize) {
-		pipe->position -= frames_to_bytes(substream->runtime, bufsize);
-		pos -= bufsize;
-	}
-	return pos;
+	counter = le32_to_cpu(*pipe->dma_counter);  /* presumed atomic */
+
+	step = counter - pipe->last_counter;  /* handles wrapping */
+	pipe->last_counter = counter;
+
+	/* counter doesn't neccessarily wrap on a multiple of
+	 * buffer_size, so can't derive the position; must
+	 * accumulate */
+
+	pipe->position += step;
+	pipe->position %= frames_to_bytes(runtime, runtime->buffer_size); /* wrap */
+
+	return bytes_to_frames(runtime, pipe->position);
 }
 
 
@@ -1782,14 +1790,43 @@ static const struct snd_kcontrol_new snd_echo_channels_info = {
 
 
 /******************************************************************************
-	IRQ Handler
+	IRQ Handling
 ******************************************************************************/
+/* Check if a period has elapsed since last interrupt
+ *
+ * Don't make any updates to state; PCM core handles this with the
+ * correct locks.
+ *
+ * \return true if a period has elapsed, otherwise false
+ */
+static bool period_has_elapsed(struct snd_pcm_substream *substream)
+{
+	struct snd_pcm_runtime *runtime = substream->runtime;
+	struct audiopipe *pipe = runtime->private_data;
+	u32 counter, step;
+	size_t period_bytes;
+
+	if (pipe->state != PIPE_STATE_STARTED)
+		return false;
+
+	period_bytes = frames_to_bytes(runtime, runtime->period_size);
+
+	counter = le32_to_cpu(*pipe->dma_counter);  /* presumed atomic */
+
+	step = counter - pipe->last_period;  /* handles wrapping */
+	step -= step % period_bytes;  /* acknowledge whole periods only */
+
+	if (step == 0)
+		return false;  /* haven't advanced a whole period yet */
+
+	pipe->last_period += step;  /* used exclusively by us */
+	return true;
+}
 
 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,17 +1837,13 @@ 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 && period_has_elapsed(substream)) {
+			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..30c640931f1e 100644
--- a/sound/pci/echoaudio/echoaudio.h
+++ b/sound/pci/echoaudio/echoaudio.h
@@ -298,7 +298,12 @@ struct audiopipe {
 					 * the current dma position
 					 * (lower 32 bits only)
 					 */
-	u32 last_counter;		/* The last position, which is used
+	u32 last_period;                /* Counter position last time a
+					 * period elapsed
+					 */
+	u32 last_counter;		/* Used exclusively by pcm_pointer
+					 * under PCM core locks.
+					 * The last position, which is used
 					 * to compute...
 					 */
 	u32 position;			/* ...the number of bytes tranferred
@@ -332,7 +337,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