On Thu, Mar 07, 2013 at 09:15:49PM +0000, Paul Zimmerman wrote: > > From: Felipe Balbi [mailto:balbi@xxxxxx] > > Sent: Wednesday, March 06, 2013 11:52 PM > > > > 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. > > "while (!list_empty(&qh->qtd_list) && retry--)" is waiting for the > QTDs attached to the QH to be completed. The completions are done > in the interrupt handler. Once all QTDs are completed, qh->qtd_list > will be empty and the loop will exit. this isn't safe, since you drop the lock, a separate thread could list_add() to the same list. Alan has given good information on a separate subthread. -- balbi
Attachment:
signature.asc
Description: Digital signature
