Hi, On Mon, Mar 20, 2023 at 03:28:38PM +0100, Takashi Iwai wrote: > The recent support of low latency playback in USB-audio driver made > the snd_usb_queue_pending_output_urbs() function to be called via PCM > ack ops. In the new code path, the function is performed alread in 's/alread/already/' or slang. > the PCM stream lock. The problem is that, when an XRUN is detected, > the function calls snd_pcm_xrun() to notify, but snd_pcm_xrun() is 's/the function/the function/' > supposed to be called only outside the stream lock. As a result, it > leads to a deadlock of PCM stream locking. > > For avoiding such a recursive locking, this patch adds an additional > check to the code paths in PCM core that call the ack callback; now it > checks the error code from the callback, and if it's -EPIPE, the XRUN > is handled in the PCM core side gracefully. Along with it, the > USB-audio driver code is changed to follow that, i.e. -EPIPE is > returned instead of the explicit snd_pcm_xrun() call when the function > is performed already in the stream lock. Practically, the implementation of 'pcm_hw' in alsa-lib never evaluates the return value (see 'snd_pcm_hw_mmap_commit()' and the others). I guess that it is inconvenient for the low-latency mode of USB Audio device class driver for the case of failure. My additional concern is PCM indirect layer, since typically the layer is used by drivers with SNDRV_PCM_INFO_SYNC_APPLPTR. But as long as I read, the change does not matter to them. > Fixes: d5f871f89e21 ("ALSA: usb-audio: Improved lowlatency playback support") > Reported-and-tested-by: John Keeping <john@xxxxxxxxxxxx> > Link: https://lore.kernel.org/r/20230317195128.3911155-1-john@xxxxxxxxxxxx > Cc: Takashi Sakamoto <o-takashi@xxxxxxxxxxxxx> > Signed-off-by: Takashi Iwai <tiwai@xxxxxxx> > --- > sound/core/pcm_lib.c | 2 ++ > sound/usb/endpoint.c | 22 ++++++++++++++-------- > sound/usb/endpoint.h | 4 ++-- > sound/usb/pcm.c | 2 +- > 4 files changed, 19 insertions(+), 11 deletions(-) Anyway, John Keeping reports the change works well to solve the issue. I have no objection to it. Reviewed-by; Takashi Sakamoto <o-takashi@xxxxxxxxxxxxx> It is also convenient to IEC 61883-1/6 packet streaming engine for audio and music unit in IEEE 1394 bus. I will post patches enough later for it. > diff --git a/sound/core/pcm_lib.c b/sound/core/pcm_lib.c > index 8b6aeb8a78f7..02fd65993e7e 100644 > --- a/sound/core/pcm_lib.c > +++ b/sound/core/pcm_lib.c > @@ -2155,6 +2155,8 @@ int pcm_lib_apply_appl_ptr(struct snd_pcm_substream *substream, > ret = substream->ops->ack(substream); > if (ret < 0) { > runtime->control->appl_ptr = old_appl_ptr; > + if (ret == -EPIPE) > + __snd_pcm_xrun(substream); > return ret; > } > } > diff --git a/sound/usb/endpoint.c b/sound/usb/endpoint.c > index 419302e2057e..647fa054d8b1 100644 > --- a/sound/usb/endpoint.c > +++ b/sound/usb/endpoint.c > @@ -455,8 +455,8 @@ static void push_back_to_ready_list(struct snd_usb_endpoint *ep, > * This function is used both for implicit feedback endpoints and in low- > * latency playback mode. > */ > -void snd_usb_queue_pending_output_urbs(struct snd_usb_endpoint *ep, > - bool in_stream_lock) > +int snd_usb_queue_pending_output_urbs(struct snd_usb_endpoint *ep, > + bool in_stream_lock) > { > bool implicit_fb = snd_usb_endpoint_implicit_feedback_sink(ep); > > @@ -480,7 +480,7 @@ void snd_usb_queue_pending_output_urbs(struct snd_usb_endpoint *ep, > spin_unlock_irqrestore(&ep->lock, flags); > > if (ctx == NULL) > - return; > + break; > > /* copy over the length information */ > if (implicit_fb) { > @@ -495,11 +495,14 @@ void snd_usb_queue_pending_output_urbs(struct snd_usb_endpoint *ep, > break; > if (err < 0) { > /* push back to ready list again for -EAGAIN */ > - if (err == -EAGAIN) > + if (err == -EAGAIN) { > push_back_to_ready_list(ep, ctx); > - else > + break; > + } > + > + if (!in_stream_lock) > notify_xrun(ep); > - return; > + return -EPIPE; > } > > err = usb_submit_urb(ctx->urb, GFP_ATOMIC); > @@ -507,13 +510,16 @@ void snd_usb_queue_pending_output_urbs(struct snd_usb_endpoint *ep, > usb_audio_err(ep->chip, > "Unable to submit urb #%d: %d at %s\n", > ctx->index, err, __func__); > - notify_xrun(ep); > - return; > + if (!in_stream_lock) > + notify_xrun(ep); > + return -EPIPE; > } > > set_bit(ctx->index, &ep->active_mask); > atomic_inc(&ep->submitted_urbs); > } > + > + return 0; > } > > /* > diff --git a/sound/usb/endpoint.h b/sound/usb/endpoint.h > index 924f4351588c..c09f68ce08b1 100644 > --- a/sound/usb/endpoint.h > +++ b/sound/usb/endpoint.h > @@ -52,7 +52,7 @@ int snd_usb_endpoint_implicit_feedback_sink(struct snd_usb_endpoint *ep); > int snd_usb_endpoint_next_packet_size(struct snd_usb_endpoint *ep, > struct snd_urb_ctx *ctx, int idx, > unsigned int avail); > -void snd_usb_queue_pending_output_urbs(struct snd_usb_endpoint *ep, > - bool in_stream_lock); > +int snd_usb_queue_pending_output_urbs(struct snd_usb_endpoint *ep, > + bool in_stream_lock); > > #endif /* __USBAUDIO_ENDPOINT_H */ > diff --git a/sound/usb/pcm.c b/sound/usb/pcm.c > index d959da7a1afb..eec5232f9fb2 100644 > --- a/sound/usb/pcm.c > +++ b/sound/usb/pcm.c > @@ -1639,7 +1639,7 @@ static int snd_usb_pcm_playback_ack(struct snd_pcm_substream *substream) > * outputs here > */ > if (!ep->active_mask) > - snd_usb_queue_pending_output_urbs(ep, true); > + return snd_usb_queue_pending_output_urbs(ep, true); > return 0; > } > > -- > 2.35.3 Thanks Takashi Sakamoto