From: John Keeping <john@metanate.com> To: Takashi Iwai <tiwai@suse.com> Cc: John Keeping <john@metanate.com>, Jaroslav Kysela <perex@perex.cz>, alsa-devel@alsa-project.org (moderated list:SOUND), linux-kernel@vger.kernel.org (open list) Subject: [PATCH] ALSA: usb-audio: Fix recursive locking on XRUN Date: Fri, 17 Mar 2023 19:51:27 +0000 [thread overview] Message-ID: <20230317195128.3911155-1-john@metanate.com> (raw) 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@metanate.com> --- include/sound/pcm.h | 1 + sound/core/pcm_native.c | 28 ++++++++++++++++++++++++---- sound/usb/endpoint.c | 18 +++++++++++------- 3 files changed, 36 insertions(+), 11 deletions(-) diff --git a/include/sound/pcm.h b/include/sound/pcm.h index 27040b472a4f..98551907453a 100644 --- a/include/sound/pcm.h +++ b/include/sound/pcm.h @@ -571,6 +571,7 @@ int snd_pcm_status64(struct snd_pcm_substream *substream, int snd_pcm_start(struct snd_pcm_substream *substream); int snd_pcm_stop(struct snd_pcm_substream *substream, snd_pcm_state_t status); int snd_pcm_drain_done(struct snd_pcm_substream *substream); +int snd_pcm_stop_xrun_under_stream_lock(struct snd_pcm_substream *substream); int snd_pcm_stop_xrun(struct snd_pcm_substream *substream); #ifdef CONFIG_PM int snd_pcm_suspend_all(struct snd_pcm *pcm); diff --git a/sound/core/pcm_native.c b/sound/core/pcm_native.c index 331380c2438b..617f5dc74df0 100644 --- a/sound/core/pcm_native.c +++ b/sound/core/pcm_native.c @@ -1559,24 +1559,44 @@ int snd_pcm_drain_done(struct snd_pcm_substream *substream) SNDRV_PCM_STATE_SETUP); } +/** + * snd_pcm_stop_xrun_under_stream_lock - stop the running stream as XRUN under the lock of + * the PCM substream. + * @substream: the PCM substream instance + * + * This stops the given running substream (and all linked substreams) as XRUN. + * This function assumes that the substream lock is already held. + * + * Return: Zero if successful, or a negative error core. + */ +int snd_pcm_stop_xrun_under_stream_lock(struct snd_pcm_substream *substream) +{ + if (substream->runtime && snd_pcm_running(substream)) + __snd_pcm_xrun(substream); + + return 0; +} + /** * snd_pcm_stop_xrun - stop the running streams as XRUN * @substream: the PCM substream instance * + * This function is similar to ``snd_pcm_stop_xrun_under_stream_lock()`` except that it + * acquires the substream lock itself. + * * This stops the given running substream (and all linked substreams) as XRUN. - * Unlike snd_pcm_stop(), this function takes the substream lock by itself. * * Return: Zero if successful, or a negative error code. */ int snd_pcm_stop_xrun(struct snd_pcm_substream *substream) { unsigned long flags; + int ret; snd_pcm_stream_lock_irqsave(substream, flags); - if (substream->runtime && snd_pcm_running(substream)) - __snd_pcm_xrun(substream); + ret = snd_pcm_stop_xrun_under_stream_lock(substream); snd_pcm_stream_unlock_irqrestore(substream, flags); - return 0; + return ret; } EXPORT_SYMBOL_GPL(snd_pcm_stop_xrun); diff --git a/sound/usb/endpoint.c b/sound/usb/endpoint.c index 1e0af1179ca8..83a6b6d41374 100644 --- a/sound/usb/endpoint.c +++ b/sound/usb/endpoint.c @@ -400,13 +400,17 @@ static int prepare_inbound_urb(struct snd_usb_endpoint *ep, } /* notify an error as XRUN to the assigned PCM data substream */ -static void notify_xrun(struct snd_usb_endpoint *ep) +static void notify_xrun(struct snd_usb_endpoint *ep, bool in_stream_lock) { struct snd_usb_substream *data_subs; data_subs = READ_ONCE(ep->data_subs); - if (data_subs && data_subs->pcm_substream) - snd_pcm_stop_xrun(data_subs->pcm_substream); + if (data_subs && data_subs->pcm_substream) { + if (in_stream_lock) + snd_pcm_stop_xrun_under_stream_lock(data_subs->pcm_substream); + else + snd_pcm_stop_xrun(data_subs->pcm_substream); + } } static struct snd_usb_packet_info * @@ -498,7 +502,7 @@ void snd_usb_queue_pending_output_urbs(struct snd_usb_endpoint *ep, if (err == -EAGAIN) push_back_to_ready_list(ep, ctx); else - notify_xrun(ep); + notify_xrun(ep, in_stream_lock); return; } @@ -507,7 +511,7 @@ 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); + notify_xrun(ep, in_stream_lock); return; } @@ -574,7 +578,7 @@ static void snd_complete_urb(struct urb *urb) return; usb_audio_err(ep->chip, "cannot submit urb (err = %d)\n", err); - notify_xrun(ep); + notify_xrun(ep, false); exit_clear: clear_bit(ctx->index, &ep->active_mask); @@ -1762,7 +1766,7 @@ static void snd_usb_handle_sync_urb(struct snd_usb_endpoint *ep, usb_audio_err(ep->chip, "next package FIFO overflow EP 0x%x\n", ep->ep_num); - notify_xrun(ep); + notify_xrun(ep, false); return; } -- 2.40.0
WARNING: multiple messages have this Message-ID (diff)
From: John Keeping <john@metanate.com> To: Takashi Iwai <tiwai@suse.com> Cc: John Keeping <john@metanate.com>, "moderated list:SOUND" <alsa-devel@alsa-project.org>, open list <linux-kernel@vger.kernel.org> Subject: [PATCH] ALSA: usb-audio: Fix recursive locking on XRUN Date: Fri, 17 Mar 2023 19:51:27 +0000 [thread overview] Message-ID: <20230317195128.3911155-1-john@metanate.com> (raw) 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@metanate.com> --- include/sound/pcm.h | 1 + sound/core/pcm_native.c | 28 ++++++++++++++++++++++++---- sound/usb/endpoint.c | 18 +++++++++++------- 3 files changed, 36 insertions(+), 11 deletions(-) diff --git a/include/sound/pcm.h b/include/sound/pcm.h index 27040b472a4f..98551907453a 100644 --- a/include/sound/pcm.h +++ b/include/sound/pcm.h @@ -571,6 +571,7 @@ int snd_pcm_status64(struct snd_pcm_substream *substream, int snd_pcm_start(struct snd_pcm_substream *substream); int snd_pcm_stop(struct snd_pcm_substream *substream, snd_pcm_state_t status); int snd_pcm_drain_done(struct snd_pcm_substream *substream); +int snd_pcm_stop_xrun_under_stream_lock(struct snd_pcm_substream *substream); int snd_pcm_stop_xrun(struct snd_pcm_substream *substream); #ifdef CONFIG_PM int snd_pcm_suspend_all(struct snd_pcm *pcm); diff --git a/sound/core/pcm_native.c b/sound/core/pcm_native.c index 331380c2438b..617f5dc74df0 100644 --- a/sound/core/pcm_native.c +++ b/sound/core/pcm_native.c @@ -1559,24 +1559,44 @@ int snd_pcm_drain_done(struct snd_pcm_substream *substream) SNDRV_PCM_STATE_SETUP); } +/** + * snd_pcm_stop_xrun_under_stream_lock - stop the running stream as XRUN under the lock of + * the PCM substream. + * @substream: the PCM substream instance + * + * This stops the given running substream (and all linked substreams) as XRUN. + * This function assumes that the substream lock is already held. + * + * Return: Zero if successful, or a negative error core. + */ +int snd_pcm_stop_xrun_under_stream_lock(struct snd_pcm_substream *substream) +{ + if (substream->runtime && snd_pcm_running(substream)) + __snd_pcm_xrun(substream); + + return 0; +} + /** * snd_pcm_stop_xrun - stop the running streams as XRUN * @substream: the PCM substream instance * + * This function is similar to ``snd_pcm_stop_xrun_under_stream_lock()`` except that it + * acquires the substream lock itself. + * * This stops the given running substream (and all linked substreams) as XRUN. - * Unlike snd_pcm_stop(), this function takes the substream lock by itself. * * Return: Zero if successful, or a negative error code. */ int snd_pcm_stop_xrun(struct snd_pcm_substream *substream) { unsigned long flags; + int ret; snd_pcm_stream_lock_irqsave(substream, flags); - if (substream->runtime && snd_pcm_running(substream)) - __snd_pcm_xrun(substream); + ret = snd_pcm_stop_xrun_under_stream_lock(substream); snd_pcm_stream_unlock_irqrestore(substream, flags); - return 0; + return ret; } EXPORT_SYMBOL_GPL(snd_pcm_stop_xrun); diff --git a/sound/usb/endpoint.c b/sound/usb/endpoint.c index 1e0af1179ca8..83a6b6d41374 100644 --- a/sound/usb/endpoint.c +++ b/sound/usb/endpoint.c @@ -400,13 +400,17 @@ static int prepare_inbound_urb(struct snd_usb_endpoint *ep, } /* notify an error as XRUN to the assigned PCM data substream */ -static void notify_xrun(struct snd_usb_endpoint *ep) +static void notify_xrun(struct snd_usb_endpoint *ep, bool in_stream_lock) { struct snd_usb_substream *data_subs; data_subs = READ_ONCE(ep->data_subs); - if (data_subs && data_subs->pcm_substream) - snd_pcm_stop_xrun(data_subs->pcm_substream); + if (data_subs && data_subs->pcm_substream) { + if (in_stream_lock) + snd_pcm_stop_xrun_under_stream_lock(data_subs->pcm_substream); + else + snd_pcm_stop_xrun(data_subs->pcm_substream); + } } static struct snd_usb_packet_info * @@ -498,7 +502,7 @@ void snd_usb_queue_pending_output_urbs(struct snd_usb_endpoint *ep, if (err == -EAGAIN) push_back_to_ready_list(ep, ctx); else - notify_xrun(ep); + notify_xrun(ep, in_stream_lock); return; } @@ -507,7 +511,7 @@ 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); + notify_xrun(ep, in_stream_lock); return; } @@ -574,7 +578,7 @@ static void snd_complete_urb(struct urb *urb) return; usb_audio_err(ep->chip, "cannot submit urb (err = %d)\n", err); - notify_xrun(ep); + notify_xrun(ep, false); exit_clear: clear_bit(ctx->index, &ep->active_mask); @@ -1762,7 +1766,7 @@ static void snd_usb_handle_sync_urb(struct snd_usb_endpoint *ep, usb_audio_err(ep->chip, "next package FIFO overflow EP 0x%x\n", ep->ep_num); - notify_xrun(ep); + notify_xrun(ep, false); return; } -- 2.40.0
next reply other threads:[~2023-03-17 19:52 UTC|newest] Thread overview: 13+ messages / expand[flat|nested] mbox.gz Atom feed top 2023-03-17 19:51 John Keeping [this message] 2023-03-17 19:51 ` [PATCH] ALSA: usb-audio: Fix recursive locking on XRUN John Keeping 2023-03-18 0:20 ` Takashi Sakamoto 2023-03-18 10:59 ` Takashi Sakamoto 2023-03-19 3:28 ` Takashi Sakamoto 2023-03-19 7:57 ` Takashi Iwai 2023-03-19 9:15 ` Takashi Iwai 2023-03-20 12:04 ` John Keeping 2023-03-20 12:04 ` John Keeping 2023-03-20 14:28 ` Takashi Iwai 2023-03-20 14:28 ` Takashi Iwai 2023-03-18 2:29 ` kernel test robot 2023-03-18 5:46 ` kernel test robot
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=20230317195128.3911155-1-john@metanate.com \ --to=john@metanate.com \ --cc=alsa-devel@alsa-project.org \ --cc=linux-kernel@vger.kernel.org \ --cc=perex@perex.cz \ --cc=tiwai@suse.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.