From: Henry Lin <henryl@nvidia.com>
To: unlisted-recipients:; (no To-header on input)
Cc: Henry Lin <henryl@nvidia.com>, Jaroslav Kysela <perex@perex.cz>,
Takashi Iwai <tiwai@suse.com>,
Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
Allison Randal <allison@lohutok.net>,
Thomas Gleixner <tglx@linutronix.de>,
Richard Fontana <rfontana@redhat.com>,
<alsa-devel@alsa-project.org>, <linux-kernel@vger.kernel.org>
Subject: [PATCH] usb-audio: not submit urb for stopped endpoint
Date: Tue, 12 Nov 2019 14:51:06 +0800 [thread overview]
Message-ID: <20191112065108.7766-1-henryl@nvidia.com> (raw)
While output urb's snd_complete_urb() is executing, calling
prepare_outbound_urb() may cause endpoint stopped before
prepare_outbound_urb() returns and result in next urb submitted
to stopped endpoint. usb-audio driver cannot re-use it afterwards as
the urb is still hold by usb stack.
This change checks EP_FLAG_RUNNING flag after prepare_outbound_urb() again
to let snd_complete_urb() know the endpoint already stopped and does not
submit next urb.
We observed two scenario have this issue:
1. While executing snd_complete_urb() to complete an output urb, calling
prepare_outbound_urb() let deactive_urbs() get called to unlink all
active urbs.
[ 268.097066] [<ffffffc000af7638>] deactivate_urbs+0xd4/0x108
[ 268.102633] [<ffffffc000af87fc>] snd_usb_endpoint_stop+0x30/0x58
[ 268.108636] [<ffffffc000b0272c>] snd_usb_substream_playback_trigger+0xa4/0xf4
[ 268.115765] [<ffffffc000acdbd0>] snd_pcm_do_stop+0x4c/0x58
[ 268.121245] [<ffffffc000acda24>] snd_pcm_action_single+0x40/0x88
[ 268.127245] [<ffffffc000ace984>] snd_pcm_action+0x30/0xf0
[ 268.132632] [<ffffffc000acea68>] snd_pcm_stop+0x24/0x2c
[ 268.137851] [<ffffffc000ad5e14>] xrun+0x60/0x6c
[ 268.142374] [<ffffffc000ad7a98>] snd_pcm_update_state+0xa8/0x10c
[ 268.148374] [<ffffffc000ad7e24>] snd_pcm_update_hw_ptr0+0x328/0x344
[ 268.154635] [<ffffffc000ad7ed8>] snd_pcm_period_elapsed+0x98/0xb0
[ 268.160723] [<ffffffc000b02510>] prepare_playback_urb+0x46c/0x488
[ 268.166810] [<ffffffc000af7d60>] prepare_outbound_urb+0x60/0x1d4
[ 268.172805] [<ffffffc000af8d60>] snd_complete_urb+0x244/0x264
[ 268.178548] [<ffffffc00081fb38>] __usb_hcd_giveback_urb+0x94/0x104
[ 268.184721] [<ffffffc00081fbe4>] usb_hcd_giveback_urb+0x3c/0x114
[ 268.190724] [<ffffffc00084d4b4>] handle_tx_event+0x1304/0x1434
[ 268.196552] [<ffffffc00084dbc0>] xhci_handle_event+0x5dc/0x788
[ 268.202378] [<ffffffc00084dee4>] xhci_irq+0x178/0x280
2. Userspace application stops playback from sound subsystem with below
call stack:
[ 28.506477] CPU: 5 PID: 1274 Comm: AudioOut_25 Not tainted 4.4.38-tegra #31
[ 28.513430] Hardware name: quill (DT)
[ 28.517085] Call trace:
[ 28.519531] [<ffffffc000089a84>] dump_backtrace+0x0/0xf8
[ 28.524837] [<ffffffc000089c44>] show_stack+0x14/0x1c
[ 28.529885] [<ffffffc000401c54>] dump_stack+0xac/0xe0
[ 28.534931] [<ffffffc000b35f94>] deactivate_urbs+0x148/0x180
[ 28.540578] [<ffffffc000b37160>] snd_usb_endpoint_stop+0x30/0x58
[ 28.546571] [<ffffffc000b410d8>] snd_usb_substream_playback_trigger+0xa4/0xf4
[ 28.553699] [<ffffffc000b0c160>] snd_pcm_do_stop+0x4c/0x58
[ 28.559179] [<ffffffc000b0bfb4>] snd_pcm_action_single+0x40/0x88
[ 28.565178] [<ffffffc000b0cf14>] snd_pcm_action+0x30/0xf0
[ 28.570568] [<ffffffc000b0fbc8>] snd_pcm_drop+0xac/0x140
[ 28.575873] [<ffffffc000b0fc84>] snd_pcm_release_substream+0x28/0xb0
[ 28.582212] [<ffffffc000b0fd48>] snd_pcm_release+0x3c/0x98
[ 28.587686] [<ffffffc0001e3210>] __fput+0xe0/0x1ac
[ 28.592469] [<ffffffc0001e3334>] ____fput+0xc/0x14
[ 28.597253] [<ffffffc0000c2904>] task_work_run+0xa0/0xc0
[ 28.602558] [<ffffffc0000897bc>] do_notify_resume+0x48/0x60
[ 28.608123] [<ffffffc000084ee8>] work_pending+0x1c/0x20
In the call path, snd_pcm_stream spinlock has been acquired in
snd_pcm_drop(). If an output urb is completed between the spinlock
acquired and deactivate_urbs() clears EP_FLAG_RUNNING for the endpoint,
its executing of snd_complete_urb() will be blocked for acquiring
snd_pcm_stream spinlock in snd_pcm_period_elapsed() until the lock is
released in snd_pcm_drop(). When snd_complete_urb() continues, all jobs
for deactivate_urbs() are finished.
Signed-off-by: Henry Lin <henryl@nvidia.com>
---
sound/usb/endpoint.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/sound/usb/endpoint.c b/sound/usb/endpoint.c
index a2ab8e8d3a93..4a9a2f6ef5a4 100644
--- a/sound/usb/endpoint.c
+++ b/sound/usb/endpoint.c
@@ -388,6 +388,9 @@ static void snd_complete_urb(struct urb *urb)
}
prepare_outbound_urb(ep, ctx);
+ /* can be stopped during prepare callback */
+ if (unlikely(!test_bit(EP_FLAG_RUNNING, &ep->flags)))
+ goto exit_clear;
} else {
retire_inbound_urb(ep, ctx);
/* can be stopped during retire callback */
--
2.17.1
WARNING: multiple messages have this Message-ID (diff)
From: Henry Lin <henryl@nvidia.com>
Cc: alsa-devel@alsa-project.org, linux-kernel@vger.kernel.org,
Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
Takashi Iwai <tiwai@suse.com>,
Richard Fontana <rfontana@redhat.com>,
Thomas Gleixner <tglx@linutronix.de>,
Henry Lin <henryl@nvidia.com>,
Allison Randal <allison@lohutok.net>
Subject: [alsa-devel] [PATCH] usb-audio: not submit urb for stopped endpoint
Date: Tue, 12 Nov 2019 14:51:06 +0800 [thread overview]
Message-ID: <20191112065108.7766-1-henryl@nvidia.com> (raw)
While output urb's snd_complete_urb() is executing, calling
prepare_outbound_urb() may cause endpoint stopped before
prepare_outbound_urb() returns and result in next urb submitted
to stopped endpoint. usb-audio driver cannot re-use it afterwards as
the urb is still hold by usb stack.
This change checks EP_FLAG_RUNNING flag after prepare_outbound_urb() again
to let snd_complete_urb() know the endpoint already stopped and does not
submit next urb.
We observed two scenario have this issue:
1. While executing snd_complete_urb() to complete an output urb, calling
prepare_outbound_urb() let deactive_urbs() get called to unlink all
active urbs.
[ 268.097066] [<ffffffc000af7638>] deactivate_urbs+0xd4/0x108
[ 268.102633] [<ffffffc000af87fc>] snd_usb_endpoint_stop+0x30/0x58
[ 268.108636] [<ffffffc000b0272c>] snd_usb_substream_playback_trigger+0xa4/0xf4
[ 268.115765] [<ffffffc000acdbd0>] snd_pcm_do_stop+0x4c/0x58
[ 268.121245] [<ffffffc000acda24>] snd_pcm_action_single+0x40/0x88
[ 268.127245] [<ffffffc000ace984>] snd_pcm_action+0x30/0xf0
[ 268.132632] [<ffffffc000acea68>] snd_pcm_stop+0x24/0x2c
[ 268.137851] [<ffffffc000ad5e14>] xrun+0x60/0x6c
[ 268.142374] [<ffffffc000ad7a98>] snd_pcm_update_state+0xa8/0x10c
[ 268.148374] [<ffffffc000ad7e24>] snd_pcm_update_hw_ptr0+0x328/0x344
[ 268.154635] [<ffffffc000ad7ed8>] snd_pcm_period_elapsed+0x98/0xb0
[ 268.160723] [<ffffffc000b02510>] prepare_playback_urb+0x46c/0x488
[ 268.166810] [<ffffffc000af7d60>] prepare_outbound_urb+0x60/0x1d4
[ 268.172805] [<ffffffc000af8d60>] snd_complete_urb+0x244/0x264
[ 268.178548] [<ffffffc00081fb38>] __usb_hcd_giveback_urb+0x94/0x104
[ 268.184721] [<ffffffc00081fbe4>] usb_hcd_giveback_urb+0x3c/0x114
[ 268.190724] [<ffffffc00084d4b4>] handle_tx_event+0x1304/0x1434
[ 268.196552] [<ffffffc00084dbc0>] xhci_handle_event+0x5dc/0x788
[ 268.202378] [<ffffffc00084dee4>] xhci_irq+0x178/0x280
2. Userspace application stops playback from sound subsystem with below
call stack:
[ 28.506477] CPU: 5 PID: 1274 Comm: AudioOut_25 Not tainted 4.4.38-tegra #31
[ 28.513430] Hardware name: quill (DT)
[ 28.517085] Call trace:
[ 28.519531] [<ffffffc000089a84>] dump_backtrace+0x0/0xf8
[ 28.524837] [<ffffffc000089c44>] show_stack+0x14/0x1c
[ 28.529885] [<ffffffc000401c54>] dump_stack+0xac/0xe0
[ 28.534931] [<ffffffc000b35f94>] deactivate_urbs+0x148/0x180
[ 28.540578] [<ffffffc000b37160>] snd_usb_endpoint_stop+0x30/0x58
[ 28.546571] [<ffffffc000b410d8>] snd_usb_substream_playback_trigger+0xa4/0xf4
[ 28.553699] [<ffffffc000b0c160>] snd_pcm_do_stop+0x4c/0x58
[ 28.559179] [<ffffffc000b0bfb4>] snd_pcm_action_single+0x40/0x88
[ 28.565178] [<ffffffc000b0cf14>] snd_pcm_action+0x30/0xf0
[ 28.570568] [<ffffffc000b0fbc8>] snd_pcm_drop+0xac/0x140
[ 28.575873] [<ffffffc000b0fc84>] snd_pcm_release_substream+0x28/0xb0
[ 28.582212] [<ffffffc000b0fd48>] snd_pcm_release+0x3c/0x98
[ 28.587686] [<ffffffc0001e3210>] __fput+0xe0/0x1ac
[ 28.592469] [<ffffffc0001e3334>] ____fput+0xc/0x14
[ 28.597253] [<ffffffc0000c2904>] task_work_run+0xa0/0xc0
[ 28.602558] [<ffffffc0000897bc>] do_notify_resume+0x48/0x60
[ 28.608123] [<ffffffc000084ee8>] work_pending+0x1c/0x20
In the call path, snd_pcm_stream spinlock has been acquired in
snd_pcm_drop(). If an output urb is completed between the spinlock
acquired and deactivate_urbs() clears EP_FLAG_RUNNING for the endpoint,
its executing of snd_complete_urb() will be blocked for acquiring
snd_pcm_stream spinlock in snd_pcm_period_elapsed() until the lock is
released in snd_pcm_drop(). When snd_complete_urb() continues, all jobs
for deactivate_urbs() are finished.
Signed-off-by: Henry Lin <henryl@nvidia.com>
---
sound/usb/endpoint.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/sound/usb/endpoint.c b/sound/usb/endpoint.c
index a2ab8e8d3a93..4a9a2f6ef5a4 100644
--- a/sound/usb/endpoint.c
+++ b/sound/usb/endpoint.c
@@ -388,6 +388,9 @@ static void snd_complete_urb(struct urb *urb)
}
prepare_outbound_urb(ep, ctx);
+ /* can be stopped during prepare callback */
+ if (unlikely(!test_bit(EP_FLAG_RUNNING, &ep->flags)))
+ goto exit_clear;
} else {
retire_inbound_urb(ep, ctx);
/* can be stopped during retire callback */
--
2.17.1
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel
next reply other threads:[~2019-11-12 6:51 UTC|newest]
Thread overview: 14+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-11-12 6:51 Henry Lin [this message]
2019-11-12 6:51 ` [alsa-devel] [PATCH] usb-audio: not submit urb for stopped endpoint Henry Lin
2019-11-12 7:31 ` Takashi Iwai
2019-11-12 7:31 ` [alsa-devel] " Takashi Iwai
2019-11-12 16:13 ` Henry Lin
2019-11-12 16:13 ` [alsa-devel] " Henry Lin
2019-11-12 16:25 ` Takashi Iwai
2019-11-12 16:25 ` [alsa-devel] " Takashi Iwai
2019-11-12 16:41 ` Henry Lin
2019-11-12 16:41 ` [alsa-devel] " Henry Lin
2019-11-12 16:48 ` Takashi Iwai
2019-11-12 16:48 ` [alsa-devel] " Takashi Iwai
2019-11-13 2:14 ` [PATCH v2] " Henry Lin
2019-11-13 2:14 ` [alsa-devel] " Henry Lin
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=20191112065108.7766-1-henryl@nvidia.com \
--to=henryl@nvidia.com \
--cc=allison@lohutok.net \
--cc=alsa-devel@alsa-project.org \
--cc=gregkh@linuxfoundation.org \
--cc=linux-kernel@vger.kernel.org \
--cc=perex@perex.cz \
--cc=rfontana@redhat.com \
--cc=tglx@linutronix.de \
--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: link
Be 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.