On Wed, Mar 03, 2010 at 03:53:30PM +0800, Andiry Xu wrote: > Hi Sarah, > > Thanks for the review. Please see my comments below. > > On Tue, 2010-03-02 at 14:06 -0800, Sarah Sharp wrote: > > There's a lot of conditional code based on this comparison, so your > > assumptions about behavior needs to be clear. I think the extra NULL > > pointer checking complicates things quite a bit, and it makes the > > cancellation case much harder to think about. > > > > For instance, you only give back the URB in xhci_giveback_urb_in_irq() > > if urb->hcpriv != NULL. But how can xhci_giveback_urb_in_irq() be > > called if urb->hcpriv is NULL? That would mean there's a TD dangling on > > the endpoint's cancellation list when its URB has already been given > > back (which I've tried very hard to prevent), or the URB was somehow > > given back and its TD was left on the normal endpoint ring TD list. > > > > In your current code, one TD just refer to one urb. So in > xhci_giveback_urb_in_irq() you can just unlink and giveback the > cur_td->urb. But in my implementation, multiple TDs can be refer to one > urb, and the behavior of xhci_giveback_urb_in_irq() must be modified, or > one urb will be unlinked and given back multiple times. Any > suggestions? Why not keep a count of completed TDs in urb_priv and only unlink and giveback the URB if that matches urb_priv->td_cnt? The other option is to not set the IOC flag on the last TRB of the beginning and middle TDs. However, I think that breaks the cancellation case when the xHCI driver needs to stop the endpoint rings, so the count of completed TDs is probably better. > > > + > > > + if (usb_endpoint_xfer_isoc(&urb->ep->desc)) > > > + size = urb->number_of_packets; > > > + else > > > + size = 1; > > > + > > > + urb_priv = kzalloc(sizeof(struct urb_priv) + > > > + size * sizeof(struct xhci_td *), mem_flags); > > > > Don't you allocate one struct xhci_td pointer too many here? The struct > > urb_priv already contains one xhci_td pointer (urb_priv->td[0]), which > > is enough for the non-isochronous endpoints. I think the line should > > be: > > (size - 1) * sizeof(struct xhci_td *), mem_flags); > > > > I'm afraid I can't agree with your suggestion. The space for xhci_td > pointer must be allocated, even for non-isoc endpoints. Please note it's > *td[0], only allocate the struct urb_priv will not allocate the space > for xhci_td pointer. > Please refer to the corresponding implementation in OHCI driver. Ok, you're right. :) I always have to think hard about double pointers. Sarah Sharp -- 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
