The end of musb_h_disable() is totally broken: list_for_each_entry_safe_from()
will cause a kernel oops given a NULL pointer, and otherwise it's illegal to
call musb_giveback() on the first URB in the list as musb_cleanup_urb() will
have already done that (which will unlink it from the URB list and thus e. g.
cause the iterator to loop on this URB forever). The list iterator isn't safe
to use either as URBs may be explicitly dequeued by the upper layers while it's
running.
As if that was not enough, musb_giveback() isn't safe to call from here either:
- when we're disabling an endpoint with an URB being currently active, once
the URB list empties, we must immediately switch to serving next endpoint
represented by 'struct musb_qh', or risk the schedule being staleld (thus,
musb_advance_schedule() should've been called intead);
- when we're disabling an endpoint not being currently active on the hardware,
musb_giveback() will clobber the 'in_qh'/'out_qh' pointers in the 'struct
musb_hw_ep' pointing to the active endpoint's 'struct musb_qh' which will
result in a kernel oops (thus, __musb_giveback() should've been called).
Finally, it doesn't seem to be safe to read 'hep->hcpriv' before taking hold
of 'musb->lock'... phew. :-)
Signed-off-by: Sergei Shtylyov <sshtylyov@xxxxxxxxxxxxx>
---
This patch, with the description rewritten and comments added, is intended to
replace usb/usb-musb_host-fix-endpoint_disable-method.patch in Greg's series...
drivers/usb/musb/musb_host.c | 38 ++++++++++++++++++++++++++++----------
1 files changed, 28 insertions(+), 10 deletions(-)
Index: linux-2.6/drivers/usb/musb/musb_host.c
===================================================================
--- linux-2.6.orig/drivers/usb/musb/musb_host.c
+++ linux-2.6/drivers/usb/musb/musb_host.c
@@ -2094,15 +2094,16 @@ musb_h_disable(struct usb_hcd *hcd, stru
unsigned long flags;
struct musb *musb = hcd_to_musb(hcd);
u8 is_in = epnum & USB_DIR_IN;
- struct musb_qh *qh = hep->hcpriv;
- struct urb *urb, *tmp;
+ struct musb_qh *qh;
+ struct urb *urb;
struct list_head *sched;
- if (!qh)
- return;
-
spin_lock_irqsave(&musb->lock, flags);
+ qh = hep->hcpriv;
+ if (qh == NULL)
+ goto exit;
+
switch (qh->type) {
case USB_ENDPOINT_XFER_CONTROL:
sched = &musb->control;
@@ -2136,13 +2137,30 @@ musb_h_disable(struct usb_hcd *hcd, stru
/* cleanup */
musb_cleanup_urb(urb, qh, urb->pipe & USB_DIR_IN);
- } else
- urb = NULL;
- /* then just nuke all the others */
- list_for_each_entry_safe_from(urb, tmp, &hep->urb_list, urb_list)
- musb_giveback(qh, urb, -ESHUTDOWN);
+ /*
+ * Then just nuke all the other URBs and advance
+ * the schedule on this hw_ep when we're done.
+ */
+ while (!list_empty(&hep->urb_list)) {
+ urb = next_urb(qh);
+ urb->status = -ESHUTDOWN;
+ musb_advance_schedule(musb, urb, qh->hw_ep, is_in);
+ }
+ } else {
+ /*
+ * Just empty the URB list; the hardware is busy with
+ * other transfers, and since !qh->is_ready nothing
+ * will activate any of these URBs as it advances.
+ */
+ while (!list_empty(&hep->urb_list))
+ __musb_giveback(musb, next_urb(qh), -ESHUTDOWN);
+ hep->hcpriv = NULL;
+ list_del(&qh->ring);
+ kfree(qh);
+ }
+exit:
spin_unlock_irqrestore(&musb->lock, flags);
}
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
