> -----Original Message----- > From: linux-usb-owner@xxxxxxxxxxxxxxx [mailto:linux-usb-owner@xxxxxxxxxxxxxxx] On Behalf Of Sergei > Shtylyov > Sent: Wednesday, January 28, 2009 3:03 AM > To: felipe.balbi@xxxxxxxxx; gregkh@xxxxxxx > Cc: linux-usb@xxxxxxxxxxxxxxx; davinci-linux-open-source@xxxxxxxxxxxxxxxxxxxx; david-b@xxxxxxxxxxx > Subject: [PATCH 4/8] musb_host: fix endpoint_disable() method (take 2) > > The end of musb_h_disable() is totally insane: list_for_each_entry_safe_from() > will obviously cause a kernel oops given a NULL pointer and otherwise it'd be > too late to give back the first URB in the list as musb_cleanup_urb() called > beforehand would have already done that, unlinking it from the URB list and > thus causing the interator to loop on this URB forever. The list interator is > not safe to use either as URBs may be explicitly dequeued by upper layers while > it's running. As if that was not enough, musb_giveback() is not safe to call > from here either because: > - when we're disabling an endpoint with an URB being currently active, once > URB list empties, we must immediately switch to serving the next endpoint > (represented by 'struct musb_qh') or risk the schedule list being stalled; > - when we're disabling an endpoint not being currently active, musb_giveback() > may e.g. spoil the saved data toggle since it'll be reading it while another > logical endpoint ('struct musb_qh') is active. > Finally, it doesn't seem to be safe to read 'hep->hcpriv' before taking hold of > 'musb->lock'... phew. :-) Even in musb_urb_enqueue() we should read 'hep->hcpriv' within 'musb->lock'. -Ajay > > Signed-off-by: Sergei Shtylyov <sshtylyov@xxxxxxxxxxxxx> > > --- > The patch is against the recent Linus' kernel. > I wonder who wrote the original code... :-/ > > drivers/usb/musb/musb_host.c | 32 +++++++++++++++++++++----------- > 1 files changed, 21 insertions(+), 11 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 > @@ -2099,15 +2099,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; > @@ -2141,13 +2142,22 @@ 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 others */ > + while (!list_empty(&hep->urb_list)) { > + urb = next_urb(qh); > + urb->status = -ESHUTDOWN; > + musb_advance_schedule(musb, urb, qh->hw_ep, is_in); > + } > + } else { > + 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 -- 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
