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.

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); }

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,

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.

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.

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.

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 :)

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.

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

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