On Mon, 1 Jul 2013, Ming Lei wrote: > On Mon, Jul 1, 2013 at 10:44 PM, Alan Stern <stern@xxxxxxxxxxxxxxxxxxx> wrote: > > 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. > > I am wondering if holding the lock in usb_hcd_flush_endpoint() can > avoid the race completely. Suppose usb_hcd_link_urb_to_ep() in submit > path runs on CPU1 just when usb_hcd_flush_endpoint()(called from > usb_disable_endpoint()) completes on CPU0, there is no any guarantee > that CPU1 can see the up-to-date value of ep->enabled, so the urb might > be submitted to HCD successfully. There is indeed such a guarantee, provided by the spinlock. CPU1 acquires urb_list_lock after CPU0 releases it, right? Therefore, all stores to memory performed by CPU0 before releasing the lock will be visible to CPU1 after it acquires the lock. Therefore CPU1 will see that ep->enabled is 0. > >> 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. > > It may help to avoid one QH leak, even though the problem is caused > by usbcore, so I plan to do that if you have no objection. I don't mind -- but I don't see how it will help. 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
