On Tue, Mar 05, 2013 at 10:43:38PM +0000, Paul Zimmerman wrote: > > From: Felipe Balbi [mailto:balbi@xxxxxx] > > Sent: Tuesday, March 05, 2013 2:48 AM > > > > On Mon, Mar 04, 2013 at 12:21:46PM -0800, Paul Zimmerman wrote: > > > +#include "hcd.h" > > > + > > > +#ifdef VERBOSE_DEBUG > > > > could move this inside the function body so you can call it > > unconditionally. > > Will do. > > > Also, add a comment stating that this should be called with irqs > > disabled and lock held. > > Are you sure that is necessary? That means to be consistent, I would need > to go through pretty much every function in the driver, and add a comment > about whether it needs to be called with irqs disabled and the lock held. I > don't think I have seen that level of detail in the comments of any other > driver I have looked at. > > If someone is modifying the driver, I would rather they follow the code > to see whether the lock is required to be taken, instead of depending on > the comment to be correct in every case. fair enough. > > Also, almost every single function related to qHs and qTDs is iterating > > over those lists again and again and again. Eventually, if you have too > > many of them, you will start to feel the pain :-p > > > > I suggest going over this part of the code carefully and making sure you > > can make the right assumptions (for example, assume that by the time > > qh_remove_and_free() is called, you can assume all qHs to be empty or > > something similar). > > I think these functions are all in the abort/cleanup path. So in the normal > case the lists would be empty, so the loop would break on the first > iteration. Only in the abnormal case would there be anything in the list > to be dealt with. So I think this is OK. suit yourself, I still think your cleanup path is iterating far too much ;-) > > > +static int dwc2_hcd_endpoint_disable(struct dwc2_hsotg *hsotg, > > > + struct usb_host_endpoint *ep, int retry) > > > +{ > > > + struct dwc2_qh *qh; > > > + unsigned long flags; > > > + > > > + spin_lock_irqsave(&hsotg->lock, flags); > > > + > > > + qh = ep->hcpriv; > > > + if (!qh) { > > > + spin_unlock_irqrestore(&hsotg->lock, flags); > > > + return -EINVAL; > > > + } > > > + > > > + while (!list_empty(&qh->qtd_list) && retry--) { > > > + if (retry == 0) { > > > + ep->hcpriv = NULL; > > > + spin_unlock_irqrestore(&hsotg->lock, flags); > > > + dev_err(hsotg->dev, > > > + "## timeout in dwc2_hcd_endpoint_disable() ##\n"); > > > + return -EBUSY; > > > + } > > > + spin_unlock_irqrestore(&hsotg->lock, flags); > > > + usleep_range(20000, 40000); > > > + spin_lock_irqsave(&hsotg->lock, flags); > > > + } > > > + > > > + dwc2_hcd_qh_remove(hsotg, qh); > > > + ep->hcpriv = NULL; > > > + spin_unlock_irqrestore(&hsotg->lock, flags); > > > > locking here is getting convoluted already, you release the lock in > > multiple places, it might be better to figure out you if you grab it in > > one place and release it in one place. Also, releasing the lock for that > > usleep_range() creates a race. What if the endpoint gets reenabled when > > release that lock ? > > I will fix it to use goto's and release the lock in a single place. thanks > > Perhaps using udelay() would be better in that case ? > > That's not a good idea, because this can spin for quite a long time. Since > this is the endpoint disable function, it is not possible for the endpoint > to be re-enabled until this function has exited. So I don't think there's a > race here, unless I'm missing something. 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. 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). > > These comments summarize what needs to be changes all over the patch. > > New patch coming in a day or so. ok cool. -- balbi
Attachment:
signature.asc
Description: Digital signature
