On Mon, 1 Jul 2013, Ming Lei wrote: > >> Currently, URB might be probably submitted to HCD too even after > >> usb_hcd_flush_endpoint() completes since both accesses to dev->ep_in[epnum] > >> and ep->enabled aren't protected by effective locks. > > > > The urb_list_lock in hcd.c serves to synchronize changes to > > ep->enabled. An URB might be submitted after usb_hcd_flush_endpoint() > > completes, but the submission will fail in usb_hcd_link_urb_to_ep(). > > But the lock isn't held when setting and clearing ep->enabled in > usb_enable_endpoint and usb_disable_endpoint, is it? No. But it is held in usb_hcd_flush_endpoint() and usb_hcd_link_urb_to_ep(). Therefore, if a thread clears ep->enabled and then flushes the endpoint, further submissions will fail. However, this does still leave one possible race: On the first submission to an isochronous endpoint, itd_submit() and sitd_submit() will allocate a new ehci_iso_stream before calling usb_hcd_link_urb_to_ep(). If the endpoint has been flushed and disabled, the enqueue will fail but the new iso_stream will not be destroyed -- it will leak. But this is an old failure mode, not related to the new changes. > >> So how about not adding the WARN_ON(!list_empty(&qh->qtd_list) > >> now when qh->qh_state is QH_STATE_LINKED? > > > > I think we should add the WARN_ON. Or jump to the default case in that > > switch statement -- maybe the error message there should include a > > WARN_ON. > > How about just adding WARN_ON(!list_empty(&qh->qtd_list) but > continue unlinking the qh like current code, which should be > harmless and avoid the QH leak? I think the end result would be the same. It wouldn't hurt and it wouldn't help. We'd still end up at the "default" case. Alan Stern -- 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
