On Sun, Mar 19, 2023 at 10:15:55AM +0100, Takashi Iwai wrote: > On Sun, 19 Mar 2023 08:57:03 +0100, > Takashi Iwai wrote: > > > > On Sun, 19 Mar 2023 04:28:53 +0100, > > Takashi Sakamoto wrote: > > > > > > Hi, > > > > > > On Sat, Mar 18, 2023 at 09:20:05AM +0900, Takashi Sakamoto wrote: > > > > On Fri, Mar 17, 2023 at 07:51:27PM +0000, John Keeping wrote: > > > > > snd_usb_queue_pending_output_urbs() may be called from > > > > > snd_pcm_ops::ack() which means the PCM stream is locked. > > > > > > > > > > For the normal case where the call back into the PCM core is via > > > > > prepare_output_urb() the "_under_stream_lock" variant of > > > > > snd_pcm_period_elapsed() is called, but when an error occurs and the > > > > > stream is stopped as XRUN then snd_pcm_xrun() tries to recursively lock > > > > > the stream which results in deadlock. > > > > > > > > > > Follow the example of snd_pcm_period_elapsed() by adding > > > > > snd_pcm_xrun_under_stream_lock() and use this when the PCM substream > > > > > lock is already held. > > > > > > > > > > Signed-off-by: John Keeping <john@xxxxxxxxxxxx> > > > > > --- > > > > > include/sound/pcm.h | 1 + > > > > > sound/core/pcm_native.c | 28 ++++++++++++++++++++++++---- > > > > > sound/usb/endpoint.c | 18 +++++++++++------- > > > > > 3 files changed, 36 insertions(+), 11 deletions(-) > > > > > > > > The name of added kernel API implies me that you refer to existent > > > > 'snd_pcm_period_elapsed_under_stream_lock()' which I added to Linux > > > > v5.14. > > > > > > > > In my opinion, unlike the version of period elapsed API, the version of > > > > XRUN API seems not to be necessarily required to ALSA PCM core, since PCM > > > > device drivers can implement .pointer callback in the part of PCM operation. > > > > When the callback returns SNDRV_PCM_POS_XRUN, ALSA PCM application get > > > > occurence of XRUN as a result of any operation relevant to hwptr movement > > > > (e.g. SNDRV_PCM_IOCTL_HWSYNC). > > > > > > > > Therefore I think it possible to fix the issue without the proposed > > > > kernel API. I can assume some scenario: > > > > > > > > 1. Failure at tasklet for URB completion > > > > > > > > It is softIRQ context. The stream lock is not acquired. It doesn't > > > > matter to call current XRUN API. > > > > > > > > 2. Failure at PCM operation called by ALSA PCM application > > > > > > > > It is process context. The stream lock is acquired before calling driver > > > > code. When detecting any type of failure, driver code stores the state. > > > > Then .pointer callback should return SNDRV_PCM_POS_XRUNrefering to > > > > the state. > > > > > > Although being inexperienced to hack driver for USB audio device class, > > > I attempt to post the patch to fix the issue of recursive stream lock. > > > I apologies in advance since the patch is not tested yet... > > > > > > The 'in_xrun' member is newly added to 'struct snd_usb_substream'. When > > > detecting any failure, false is assigned to the member. The assignment > > > is expected to be done in both softIRQ context, and process context with > > > stream lock, thus no need to take care of cocurrent access (e.g. by usage > > > of WRITE_ONCE/READ_ONCE). > > > > > > Typical ALSA PCM application periodically calls PCM operation which calls > > > .pointer in driver code. As I described, returning SNDRV_PCM_POS_XRUN > > > takes ALSA PCM core to handle XRUN state of PCM substream in the timing. > > > > > > The negative point of the patch is the delay of XRUN notification to user > > > space application. In the point, I think the new kernel API introduced by > > > your patch has advantage. > > > > > > The in_xrun member can be replaced with a kind of EP_STATE_ > > > enumerations; i.e. EP_STATE_XRUN. In the case, we need some care so that > > > the state should be referred from pcm.c. > > > > Thanks for the patch. That would work, but the shortcoming side of > > this implementation is that it misses stopping / reporting the error > > immediately but waiting for the next pointer update. > > > > It might be simpler if we perform the xrun handling in the caller > > side, i.e. a change like below: > > > > --- 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; > > } > > } > > > > ... and let the caller returning -EPIPE for XRUN: > > and that misses the XRUN in the case of non-stream-lock. > A revised version is below. Yes, it looks like this also solves the problem. If you roll this into a proper patch feel free to add: Tested-by: John Keeping <john@xxxxxxxxxxxx> > > -- 8< -- > --- 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; > } > } > --- 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; > } > > /* > --- 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 */ > --- 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; > } >