* [PATCH] ALSA: usb-audio: Fix recursive locking at XRUN during syncing @ 2023-03-20 14:28 Takashi Iwai 2023-03-20 14:42 ` Jaroslav Kysela 2023-03-21 4:02 ` Takashi Sakamoto 0 siblings, 2 replies; 7+ messages in thread From: Takashi Iwai @ 2023-03-20 14:28 UTC (permalink / raw) To: alsa-devel; +Cc: John Keeping 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 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 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. Fixes: d5f871f89e21 ("ALSA: usb-audio: Improved lowlatency playback support") Reported-and-tested-by: John Keeping <john@metanate.com> Link: https://lore.kernel.org/r/20230317195128.3911155-1-john@metanate.com Cc: Takashi Sakamoto <o-takashi@sakamocchi.jp> Signed-off-by: Takashi Iwai <tiwai@suse.de> --- 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(-) 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 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] ALSA: usb-audio: Fix recursive locking at XRUN during syncing 2023-03-20 14:28 [PATCH] ALSA: usb-audio: Fix recursive locking at XRUN during syncing Takashi Iwai @ 2023-03-20 14:42 ` Jaroslav Kysela 2023-03-21 4:02 ` Takashi Sakamoto 1 sibling, 0 replies; 7+ messages in thread From: Jaroslav Kysela @ 2023-03-20 14:42 UTC (permalink / raw) To: Takashi Iwai, alsa-devel; +Cc: John Keeping On 20. 03. 23 15:28, 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 > 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 > 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. > > Fixes: d5f871f89e21 ("ALSA: usb-audio: Improved lowlatency playback support") > Reported-and-tested-by: John Keeping <john@metanate.com> > Link: https://lore.kernel.org/r/20230317195128.3911155-1-john@metanate.com > Cc: Takashi Sakamoto <o-takashi@sakamocchi.jp> > Signed-off-by: Takashi Iwai <tiwai@suse.de> I think that this is the best variant of the proposed solutions. Reviewed-by: Jaroslav Kysela <perex@perex.cz> Jaroslav -- Jaroslav Kysela <perex@perex.cz> Linux Sound Maintainer; ALSA Project; Red Hat, Inc. ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] ALSA: usb-audio: Fix recursive locking at XRUN during syncing 2023-03-20 14:28 [PATCH] ALSA: usb-audio: Fix recursive locking at XRUN during syncing Takashi Iwai 2023-03-20 14:42 ` Jaroslav Kysela @ 2023-03-21 4:02 ` Takashi Sakamoto 2023-03-21 4:23 ` Takashi Sakamoto 2023-03-23 6:19 ` Takashi Iwai 1 sibling, 2 replies; 7+ messages in thread From: Takashi Sakamoto @ 2023-03-21 4:02 UTC (permalink / raw) To: Takashi Iwai; +Cc: alsa-devel, John Keeping 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@metanate.com> > Link: https://lore.kernel.org/r/20230317195128.3911155-1-john@metanate.com > Cc: Takashi Sakamoto <o-takashi@sakamocchi.jp> > Signed-off-by: Takashi Iwai <tiwai@suse.de> > --- > 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@sakamocchi.jp> 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] ALSA: usb-audio: Fix recursive locking at XRUN during syncing 2023-03-21 4:02 ` Takashi Sakamoto @ 2023-03-21 4:23 ` Takashi Sakamoto 2023-03-23 6:19 ` Takashi Iwai 1 sibling, 0 replies; 7+ messages in thread From: Takashi Sakamoto @ 2023-03-21 4:23 UTC (permalink / raw) To: Takashi Iwai, alsa-devel, John Keeping On Tue, Mar 21, 2023 at 01:02:58PM +0900, Takashi Sakamoto wrote: > 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@metanate.com> > > Link: https://lore.kernel.org/r/20230317195128.3911155-1-john@metanate.com > > Cc: Takashi Sakamoto <o-takashi@sakamocchi.jp> > > Signed-off-by: Takashi Iwai <tiwai@suse.de> > > --- > > 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@sakamocchi.jp> I forgot to mention that it is preferable to update 'ack callback' clause of API Documentation (i.e. writing-an-alsa-driver.rst) as well. > 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] ALSA: usb-audio: Fix recursive locking at XRUN during syncing 2023-03-21 4:02 ` Takashi Sakamoto 2023-03-21 4:23 ` Takashi Sakamoto @ 2023-03-23 6:19 ` Takashi Iwai 2023-03-23 13:29 ` Takashi Sakamoto 1 sibling, 1 reply; 7+ messages in thread From: Takashi Iwai @ 2023-03-23 6:19 UTC (permalink / raw) To: Takashi Sakamoto; +Cc: alsa-devel, John Keeping On Tue, 21 Mar 2023 05:02:58 +0100, Takashi Sakamoto wrote: > > 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/' Corrected at applying. > > 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. I find rather that's an extra bonus :) It allows the existing code to give a more proper error handling. For example, the indirect PCM helper returned -EINVAL, but this can be switched to -EPIPE for stopping the stream. I'm going to submit the patch together with the documentation updates. thanks, Takashi ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] ALSA: usb-audio: Fix recursive locking at XRUN during syncing 2023-03-23 6:19 ` Takashi Iwai @ 2023-03-23 13:29 ` Takashi Sakamoto 2023-03-23 13:37 ` Takashi Iwai 0 siblings, 1 reply; 7+ messages in thread From: Takashi Sakamoto @ 2023-03-23 13:29 UTC (permalink / raw) To: Takashi Iwai; +Cc: alsa-devel, John Keeping On Thu, Mar 23, 2023 at 07:19:45AM +0100, Takashi Iwai wrote: > On Tue, 21 Mar 2023 05:02:58 +0100, > Takashi Sakamoto wrote: > > > > 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/' > > Corrected at applying. > > > > 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. > > I find rather that's an extra bonus :) It allows the existing code to > give a more proper error handling. For example, the indirect PCM > helper returned -EINVAL, but this can be switched to -EPIPE for > stopping the stream. > > I'm going to submit the patch together with the documentation > updates. In my opinion, extra care is required for the simple replacement of -EINVAL with -EPIPE, since it may not come from any operation relevant to hardware. In my understanding, it comes from just careless implementation of user space PCM application, thus it is still reasonable to return -EINVAL. ... However, you are the best person who has enough knowledge about the layer. I would not mind the replacement if you did. Regards Takashi Sakamoto ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] ALSA: usb-audio: Fix recursive locking at XRUN during syncing 2023-03-23 13:29 ` Takashi Sakamoto @ 2023-03-23 13:37 ` Takashi Iwai 0 siblings, 0 replies; 7+ messages in thread From: Takashi Iwai @ 2023-03-23 13:37 UTC (permalink / raw) To: Takashi Sakamoto; +Cc: alsa-devel, John Keeping On Thu, 23 Mar 2023 14:29:16 +0100, Takashi Sakamoto wrote: > > On Thu, Mar 23, 2023 at 07:19:45AM +0100, Takashi Iwai wrote: > > On Tue, 21 Mar 2023 05:02:58 +0100, > > Takashi Sakamoto wrote: > > > > > > 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/' > > > > Corrected at applying. > > > > > > 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. > > > > I find rather that's an extra bonus :) It allows the existing code to > > give a more proper error handling. For example, the indirect PCM > > helper returned -EINVAL, but this can be switched to -EPIPE for > > stopping the stream. > > > > I'm going to submit the patch together with the documentation > > updates. > > In my opinion, extra care is required for the simple replacement of > -EINVAL with -EPIPE, since it may not come from any operation relevant > to hardware. In my understanding, it comes from just careless > implementation of user space PCM application, thus it is still reasonable > to return -EINVAL. Yeah, we can't always replace such condition blindly. But in the case of indirect PCM helpers, it's the place where the bogus values have been already set and processed, so we can't return -EINVAL, hence an XRUN notification would be appreciate. Takashi ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2023-03-23 13:38 UTC | newest] Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2023-03-20 14:28 [PATCH] ALSA: usb-audio: Fix recursive locking at XRUN during syncing Takashi Iwai 2023-03-20 14:42 ` Jaroslav Kysela 2023-03-21 4:02 ` Takashi Sakamoto 2023-03-21 4:23 ` Takashi Sakamoto 2023-03-23 6:19 ` Takashi Iwai 2023-03-23 13:29 ` Takashi Sakamoto 2023-03-23 13:37 ` Takashi Iwai
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).