> From: Alan Stern [mailto:stern@xxxxxxxxxxxxxxxxxxx] > Sent: Thursday, March 07, 2013 7:35 AM > > On Wed, 6 Mar 2013, Paul Zimmerman wrote: > > > > BTW, you _do_ have a race because you don't check ep->hcpriv when > > > ->urb_enqueue() is called. This means that you could queue to an > > > endpoint which is in the process of getting disabled. The urb would > > > either be lost or completed before being started (if I read the code > > > correctly). > > > > Good spotting. Yes, I need to recheck ep->hcpriv after reacquiring > > the spinlock, thanks. Interestingly, ehci-hcd and ohci-hcd do that, > > but uhci-hcd does not. I wonder if that's a bug? > > I'm not sure what you mean. uhci-hcd does this: > > static int uhci_urb_enqueue(struct usb_hcd *hcd, > struct urb *urb, gfp_t mem_flags) > { > int ret; > struct uhci_hcd *uhci = hcd_to_uhci(hcd); > unsigned long flags; > struct urb_priv *urbp; > struct uhci_qh *qh; > > spin_lock_irqsave(&uhci->lock, flags); > ========^ > > ret = usb_hcd_link_urb_to_ep(hcd, urb); > if (ret) > goto done_not_linked; > > ret = -ENOMEM; > urbp = uhci_alloc_urb_priv(uhci, urb); > if (!urbp) > goto done; > > if (urb->ep->hcpriv) > ========^ > qh = urb->ep->hcpriv; > else { > qh = uhci_alloc_qh(uhci, urb->dev, urb->ep); > if (!qh) > goto err_no_qh; > } > > Did you mean that ohci-hcd doesn't check? Yes, that's a mistake, maybe > even a bug. Hi Alan, I was talking about uhci_hcd_endpoint_disable(). It drops the spinlock, reacquires it, but doesn't reload qh from hep->hcpriv before continuing. Maybe that's not required for uhci, I didn't look any deeper. Actually, I see now I misread ohci_endpoint_disable(), it doesn't reload from ep->hcpriv either, only ehci_endpoint_disable() does. -- Paul -- 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
