On Wed, Mar 06, 2013 at 11:26:08PM +0000, Paul Zimmerman wrote: > > From: Felipe Balbi [mailto:balbi@xxxxxx] > > Sent: Wednesday, March 06, 2013 12:05 AM > > ... > > > looking at that loop, is it really necessary ? I mean, what you do is: > > > > spin_lock_irqsave(); > > > > while (!list_empty(list) && retry--) { > > if (!retry) { > > spin_unlock_irqrestore(); > > boom(); > > } > > > > spin_unlock_irqrestore(); > > usleep_range(); > > spin_lock_irqrestore(); > > } > > > > are you really relying on that race to cleanup your qH list ? Doesn't > > look to me that usleep is at all necessary here. > > Hi Felipe, > > ehci-hcd, ohci-hcd, and uhci-hcd all spin in their endpoint_disable > functions too, so yes, I'm pretty sure it's necessary ;) I'm asking because there are no checks in this loop to make sure what you're waiting for has happened. Unlinke the others you mention where they either wait for a particular event with wait_for_event() or they go back to a point where they check the HW conditions again. > > 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'll leave this to Alan since he's the USB Host master here ;-) -- balbi
Attachment:
signature.asc
Description: Digital signature
