> 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. > > + /* > > + * Hold spinlock here. Not needed in that case if below function is > > + * being called from ISR. > > + */ > > + spin_lock_irqsave(&hsotg->lock, flags); > > + > > + /* Ensure there are no QTDs or URBs left */ > > + dwc2_kill_urbs_in_qh_list(hsotg, qh_list); > > + spin_unlock_irqrestore(&hsotg->lock, flags); > > releasing the lock here is wrong and will likely cause quite some > headache if a race is triggered here. Looks like you have some > re-factoring to do. Yup, I did not notice that before. I think I see how to fix that, and also simplify the code a bit. > 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. > > + if (hsotg->core_params->dma_enable <= 0) { > > + /* Flush out any channel requests in slave mode */ > > + for (i = 0; i < num_channels; i++) { > > + channel = hsotg->hc_ptr_array[i]; > > + if (list_empty(&channel->hc_list_entry)) { > > + hcchar = readl(hsotg->regs + HCCHAR(i)); > > + if (hcchar & HCCHAR_CHENA) { > > + hcchar &= ~(HCCHAR_CHENA | > > + HCCHAR_EPDIR); > > + hcchar |= HCCHAR_CHDIS; > > + writel(hcchar, hsotg->regs + > > + HCCHAR(i)); > > + } > > + } > > + } > > + } > > please decrease some indentation levels here: Will do. > likewise Will do. > > +void dwc2_hcd_disconnect(struct dwc2_hsotg *hsotg) > > +{ > > + u32 intr; > > + > > + /* Set status flags for the hub driver */ > > + hsotg->flags.b.port_connect_status_change = 1; > > + hsotg->flags.b.port_connect_status = 0; > > + > > + /* > > + * Shutdown any transfers in process by clearing the Tx FIFO Empty > > + * interrupt mask and status bits and disabling subsequent host > > + * channel interrupts. > > + */ > > + intr = readl(hsotg->regs + GINTMSK); > > + intr &= ~(GINTSTS_NPTXFEMP | GINTSTS_PTXFEMP | GINTSTS_HCHINT); > > + writel(intr, hsotg->regs + GINTMSK); > > + intr = GINTSTS_NPTXFEMP | GINTSTS_PTXFEMP | GINTSTS_HCHINT; > > + writel(intr, hsotg->regs + GINTSTS); > > + > > + /* > > + * Turn off the vbus power only if the core has transitioned to device > > + * mode. If still in host mode, need to keep power on to detect a > > + * reconnection. > > + */ > > + if (dwc2_is_device_mode(hsotg)) { > > + if (hsotg->op_state != OTG_STATE_A_SUSPEND) { > > just a sidenote here: eventually this should be moved to a PHY driver. Yup, understood. > > +static int dwc2_hcd_urb_dequeue(struct dwc2_hsotg *hsotg, > > + struct dwc2_hcd_urb *urb) > > +{ > > + struct dwc2_qh *qh; > > + struct dwc2_qtd *urb_qtd; > > + > > + urb_qtd = urb->qtd; > > + if (!urb_qtd) { > > + dev_dbg(hsotg->dev, "## Urb QTD is NULL ##\n"); > > + return 1; > > errors are usually negative numbers. Actually the return value isn't even being used ;) I'll fix it. > > +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. > 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. > > +static void dwc2_hcd_reinit(struct dwc2_hsotg *hsotg) > > +{ > > + struct dwc2_host_chan *chan, *chan_tmp; > > + int num_channels; > > + int i; > > + > > + hsotg->flags.d32 = 0; > > + > > + hsotg->non_periodic_qh_ptr = &hsotg->non_periodic_sched_active; > > + hsotg->non_periodic_channels = 0; > > + hsotg->periodic_channels = 0; > > + > > + /* > > + * Put all channels in the free channel list and clean up channel > > + * states > > + */ > > + list_for_each_entry_safe(chan, chan_tmp, &hsotg->free_hc_list, > > + hc_list_entry) > > + list_del_init(&chan->hc_list_entry); > > not sure you need to initialize the list on every delete. Actually, here I think it's needed, because the entry might not get readded to a list right away. But I see some other places where the _init is not necessary, I'll fix those up. > These comments summarize what needs to be changes all over the patch. New patch coming in a day or so. -- 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
