[kernel/f16] fix list_del corruption warning on USB audio with twinkle (rhbz 871078)
Justin M. Forbes
jforbes at fedoraproject.org
Mon Nov 12 18:28:01 UTC 2012
commit 02c38dbef5a274584620afc4c3b218a987a56c74
Author: Justin M. Forbes <jforbes at redhat.com>
Date: Mon Nov 12 12:23:45 2012 -0600
fix list_del corruption warning on USB audio with twinkle (rhbz 871078)
USB-EHCI-urb-hcpriv-should-not-be-NULL.patch | 72 +++++++++++
USB-report-submission-of-active-URBs.patch | 46 +++++++
kernel.spec | 15 ++-
...-fix-crash-at-re-preparing-the-PCM-stream.patch | 125 ++++++++++++++++++++
4 files changed, 257 insertions(+), 1 deletions(-)
---
diff --git a/USB-EHCI-urb-hcpriv-should-not-be-NULL.patch b/USB-EHCI-urb-hcpriv-should-not-be-NULL.patch
new file mode 100644
index 0000000..4b8e537
--- /dev/null
+++ b/USB-EHCI-urb-hcpriv-should-not-be-NULL.patch
@@ -0,0 +1,72 @@
+commit 2656a9abcf1ec8dd5fee6a75d6997a0f2fa0094e
+Author: Alan Stern <stern at rowland.harvard.edu>
+Date: Thu Nov 8 10:17:01 2012 -0500
+
+ USB: EHCI: bugfix: urb->hcpriv should not be NULL
+
+ This patch (as1632b) fixes a bug in ehci-hcd. The USB core uses
+ urb->hcpriv to determine whether or not an URB is active; host
+ controller drivers are supposed to set this pointer to a non-NULL
+ value when an URB is queued. However ehci-hcd sets it to NULL for
+ isochronous URBs, which defeats the check in usbcore.
+
+ In itself this isn't a big deal. But people have recently found that
+ certain sequences of actions will cause the snd-usb-audio driver to
+ reuse URBs without waiting for them to complete. In the absence of
+ proper checking by usbcore, the URBs get added to their endpoint list
+ twice. This leads to list corruption and a system freeze.
+
+ The patch makes ehci-hcd assign a meaningful value to urb->hcpriv for
+ isochronous URBs. Improving robustness always helps.
+
+ Signed-off-by: Alan Stern <stern at rowland.harvard.edu>
+ Reported-by: Artem S. Tashkinov <t.artem at lycos.com>
+ Reported-by: Christof Meerwald <cmeerw at cmeerw.org>
+ CC: <stable at vger.kernel.org>
+ Signed-off-by: Greg Kroah-Hartman <gregkh at linuxfoundation.org>
+
+diff --git a/drivers/usb/host/ehci-q.c b/drivers/usb/host/ehci-q.c
+index 4b66374..3d98902 100644
+--- a/drivers/usb/host/ehci-q.c
++++ b/drivers/usb/host/ehci-q.c
+@@ -264,15 +264,9 @@ ehci_urb_done(struct ehci_hcd *ehci, struct urb *urb, int status)
+ __releases(ehci->lock)
+ __acquires(ehci->lock)
+ {
+- if (likely (urb->hcpriv != NULL)) {
+- struct ehci_qh *qh = (struct ehci_qh *) urb->hcpriv;
+-
+- /* S-mask in a QH means it's an interrupt urb */
+- if ((qh->hw->hw_info2 & cpu_to_hc32(ehci, QH_SMASK)) != 0) {
+-
+- /* ... update hc-wide periodic stats (for usbfs) */
+- ehci_to_hcd(ehci)->self.bandwidth_int_reqs--;
+- }
++ if (usb_pipetype(urb->pipe) == PIPE_INTERRUPT) {
++ /* ... update hc-wide periodic stats */
++ ehci_to_hcd(ehci)->self.bandwidth_int_reqs--;
+ }
+
+ if (unlikely(urb->unlinked)) {
+diff --git a/drivers/usb/host/ehci-sched.c b/drivers/usb/host/ehci-sched.c
+index 2e14714..69ebee7 100644
+--- a/drivers/usb/host/ehci-sched.c
++++ b/drivers/usb/host/ehci-sched.c
+@@ -1630,7 +1630,7 @@ static void itd_link_urb(
+
+ /* don't need that schedule data any more */
+ iso_sched_free (stream, iso_sched);
+- urb->hcpriv = NULL;
++ urb->hcpriv = stream;
+
+ ++ehci->isoc_count;
+ enable_periodic(ehci);
+@@ -2029,7 +2029,7 @@ static void sitd_link_urb(
+
+ /* don't need that schedule data any more */
+ iso_sched_free (stream, sched);
+- urb->hcpriv = NULL;
++ urb->hcpriv = stream;
+
+ ++ehci->isoc_count;
+ enable_periodic(ehci);
diff --git a/USB-report-submission-of-active-URBs.patch b/USB-report-submission-of-active-URBs.patch
new file mode 100644
index 0000000..4496c91
--- /dev/null
+++ b/USB-report-submission-of-active-URBs.patch
@@ -0,0 +1,46 @@
+commit 2f02bc8af3abb846823811af65ec6cc46a4d525d
+Author: Alan Stern <stern at rowland.harvard.edu>
+Date: Wed Nov 7 16:35:00 2012 -0500
+
+ USB: report submission of active URBs
+
+ This patch (as1633) changes slightly the way usbcore handled
+ submissions of URBs that are already active. It will now return
+ -EBUSY rather than -EINVAL, and it will call WARN_ONCE to draw
+ people's attention to the bug.
+
+ Signed-off-by: Alan Stern <stern at rowland.harvard.edu>
+ Signed-off-by: Greg Kroah-Hartman <gregkh at linuxfoundation.org>
+
+diff --git a/Documentation/usb/error-codes.txt b/Documentation/usb/error-codes.txt
+index 8d1e2a9..9c3eb84 100644
+--- a/Documentation/usb/error-codes.txt
++++ b/Documentation/usb/error-codes.txt
+@@ -21,6 +21,8 @@ Non-USB-specific:
+
+ USB-specific:
+
++-EBUSY The URB is already active.
++
+ -ENODEV specified USB-device or bus doesn't exist
+
+ -ENOENT specified interface or endpoint does not exist or
+diff --git a/drivers/usb/core/urb.c b/drivers/usb/core/urb.c
+index 3662287..e0d9d94 100644
+--- a/drivers/usb/core/urb.c
++++ b/drivers/usb/core/urb.c
+@@ -321,8 +321,13 @@ int usb_submit_urb(struct urb *urb, gfp_t mem_flags)
+ struct usb_host_endpoint *ep;
+ int is_out;
+
+- if (!urb || urb->hcpriv || !urb->complete)
++ if (!urb || !urb->complete)
+ return -EINVAL;
++ if (urb->hcpriv) {
++ WARN_ONCE(1, "URB %p submitted while active\n", urb);
++ return -EBUSY;
++ }
++
+ dev = urb->dev;
+ if ((!dev) || (dev->state < USB_STATE_UNAUTHENTICATED))
+ return -ENODEV;
diff --git a/kernel.spec b/kernel.spec
index 34b56a7..14b1886 100644
--- a/kernel.spec
+++ b/kernel.spec
@@ -54,7 +54,7 @@ Summary: The Linux kernel
# For non-released -rc kernels, this will be appended after the rcX and
# gitX tags, so a 3 here would become part of release "0.rcX.gitX.3"
#
-%global baserelease 1
+%global baserelease 2
%global fedora_build %{baserelease}
# base_sublevel is the kernel version we're starting with and patching
@@ -705,6 +705,11 @@ Patch22092: net-fix-divide-by-zero-in-tcp-algorithm-illinois.patch
Patch30000: weird-root-dentry-name-debug.patch
Patch30010: debug-808990.patch
+#rhbz 871078
+Patch22110: usb-audio-fix-crash-at-re-preparing-the-PCM-stream.patch
+Patch22111: USB-EHCI-urb-hcpriv-should-not-be-NULL.patch
+Patch22112: USB-report-submission-of-active-URBs.patch
+
# END OF PATCH DEFINITIONS
%endif
@@ -1327,6 +1332,11 @@ ApplyPatch 0012-ext4-serialize-fallocate-with-ext4_convert_unwritten.patch
#rhbz 871923 871848 CVE-2012-4565
ApplyPatch net-fix-divide-by-zero-in-tcp-algorithm-illinois.patch
+#rhbz 871078
+ApplyPatch usb-audio-fix-crash-at-re-preparing-the-PCM-stream.patch
+ApplyPatch USB-EHCI-urb-hcpriv-should-not-be-NULL.patch
+ApplyPatch USB-report-submission-of-active-URBs.patch
+
# END OF PATCH APPLICATIONS
%endif
@@ -2027,6 +2037,9 @@ fi
# and build.
%changelog
+* Mon Nov 12 2012 Justin M. Forbes <jforbes at redhat.com>
+- fix list_del corruption warning on USB audio with twinkle (rhbz 871078)
+
* Mon Nov 05 2012 Justin M. Forbes <jforbes at redhat.com> 3.6.6-1
- Linux 3.6.6
diff --git a/usb-audio-fix-crash-at-re-preparing-the-PCM-stream.patch b/usb-audio-fix-crash-at-re-preparing-the-PCM-stream.patch
new file mode 100644
index 0000000..6d840ea
--- /dev/null
+++ b/usb-audio-fix-crash-at-re-preparing-the-PCM-stream.patch
@@ -0,0 +1,125 @@
+At Thu, 08 Nov 2012 08:31:35 +0100,
+Daniel Mack wrote:
+(snip)
+> >> We can't simply stop both endpoints in the prepare callback.
+> >
+> > The new function doesn't stop the stream by itself but it just syncs
+> > if the stream is being stopped beforehand. So, it's safe to call it
+> > there.
+> >
+> > Maybe the name was confusing. It should have been like
+> > snd_usb_endpoint_sync_pending_stop() or such.
+>
+> Ah, right. I was errornously looking closer to Alan's patch but then
+> replied to yours. Alright then - thanks for explaining :)
+
+OK, thanks for checking.
+
+FWIW, below is the patch I applied now to for-linus branch.
+Renamed the function, added the comment and put NULL check to the
+function to simplify.
+
+
+Takashi
+
+---
+From: Takashi Iwai <tiwai at suse.de>
+Subject: [PATCH] ALSA: usb-audio: Fix crash at re-preparing the PCM stream
+
+There are bug reports of a crash with USB-audio devices when PCM
+prepare is performed immediately after the stream is stopped via
+trigger callback. It turned out that the problem is that we don't
+wait until all URBs are killed.
+
+This patch adds a new function to synchronize the pending stop
+operation on an endpoint, and calls in the prepare callback for
+avoiding the crash above.
+
+Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=49181
+
+Reported-and-tested-by: Artem S. Tashkinov <t.artem at lycos.com>
+Cc: <stable at vger.kernel.org> [v3.6]
+Signed-off-by: Takashi Iwai <tiwai at suse.de>
+---
+ sound/usb/endpoint.c | 13 +++++++++++++
+ sound/usb/endpoint.h | 1 +
+ sound/usb/pcm.c | 3 +++
+ 3 files changed, 17 insertions(+)
+
+diff --git a/sound/usb/endpoint.c b/sound/usb/endpoint.c
+index 7f78c6d..34de6f2 100644
+--- a/sound/usb/endpoint.c
++++ b/sound/usb/endpoint.c
+@@ -35,6 +35,7 @@
+
+ #define EP_FLAG_ACTIVATED 0
+ #define EP_FLAG_RUNNING 1
++#define EP_FLAG_STOPPING 2
+
+ /*
+ * snd_usb_endpoint is a model that abstracts everything related to an
+@@ -502,10 +503,20 @@ static int wait_clear_urbs(struct snd_usb_endpoint *ep)
+ if (alive)
+ snd_printk(KERN_ERR "timeout: still %d active urbs on EP #%x\n",
+ alive, ep->ep_num);
++ clear_bit(EP_FLAG_STOPPING, &ep->flags);
+
+ return 0;
+ }
+
++/* sync the pending stop operation;
++ * this function itself doesn't trigger the stop operation
++ */
++void snd_usb_endpoint_sync_pending_stop(struct snd_usb_endpoint *ep)
++{
++ if (ep && test_bit(EP_FLAG_STOPPING, &ep->flags))
++ wait_clear_urbs(ep);
++}
++
+ /*
+ * unlink active urbs.
+ */
+@@ -918,6 +929,8 @@ void snd_usb_endpoint_stop(struct snd_usb_endpoint *ep,
+
+ if (wait)
+ wait_clear_urbs(ep);
++ else
++ set_bit(EP_FLAG_STOPPING, &ep->flags);
+ }
+ }
+
+diff --git a/sound/usb/endpoint.h b/sound/usb/endpoint.h
+index 6376ccf..3d4c970 100644
+--- a/sound/usb/endpoint.h
++++ b/sound/usb/endpoint.h
+@@ -19,6 +19,7 @@ int snd_usb_endpoint_set_params(struct snd_usb_endpoint *ep,
+ int snd_usb_endpoint_start(struct snd_usb_endpoint *ep, int can_sleep);
+ void snd_usb_endpoint_stop(struct snd_usb_endpoint *ep,
+ int force, int can_sleep, int wait);
++void snd_usb_endpoint_sync_pending_stop(struct snd_usb_endpoint *ep);
+ int snd_usb_endpoint_activate(struct snd_usb_endpoint *ep);
+ int snd_usb_endpoint_deactivate(struct snd_usb_endpoint *ep);
+ void snd_usb_endpoint_free(struct list_head *head);
+diff --git a/sound/usb/pcm.c b/sound/usb/pcm.c
+index 37428f7..5c12a3f 100644
+--- a/sound/usb/pcm.c
++++ b/sound/usb/pcm.c
+@@ -552,6 +552,9 @@ static int snd_usb_pcm_prepare(struct snd_pcm_substream *substream)
+ if (snd_BUG_ON(!subs->data_endpoint))
+ return -EIO;
+
++ snd_usb_endpoint_sync_pending_stop(subs->sync_endpoint);
++ snd_usb_endpoint_sync_pending_stop(subs->data_endpoint);
++
+ /* some unit conversions in runtime */
+ subs->data_endpoint->maxframesize =
+ bytes_to_frames(runtime, subs->data_endpoint->maxpacksize);
+--
+1.8.0
+
+
+--
+To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
+the body of a message to majordomo at vger.kernel.org
+More majordomo info at http://vger.kernel.org/majordomo-info.html
+Please read the FAQ at http://www.tux.org/lkml/
More information about the scm-commits
mailing list