On Tue, Mar 05, 2013 at 10:43:38PM +0000, Paul Zimmerman wrote: > > From: Felipe Balbi [mailto:[email protected]] > > 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
signature.asc
Description: Digital signature
