Hi Sarah,
Thanks for the review. Please see my comments below.
On Tue, 2010-03-02 at 14:06 -0800, Sarah Sharp wrote:
> On Tue, Mar 02, 2010 at 06:28:45PM +0800, Libin Yang wrote:
> > >From d598ff50c8908bd62c1871ca9bfe68d952e0a949 Mon Sep 17 00:00:00 2001
> > From: Andiry Xu <andiry.xu@xxxxxxx>
> > Date: Fri, 26 Feb 2010 17:34:47 +0800
> > Subject: [PATCH 1/7] xHCI: Introduce urb_priv structure
> >
> > Introduce urb_priv data structure to allow one URB contains multiple TDs.
> > This structure is necessary for isoc transfers, because xHCI can only
> > transfer one isoc TD in a interval, and one isoc URB needs to be split
> > into multiple isoc TDs.
> >
> > Some codes are borrowed from OHCI driver.
>
> Hi Libin and Andiry,
>
> Can you add more description to your commit message? There are some
> semantics about when urb->hcpriv is NULL that I don't quite understand.
> It looks like you're trying to make urb->hcpriv == NULL mean that the
> URB has been given back to the USB core. Is that true?
>
I will refine the description. About the urb->hcpriv, I'll reconsider
when and how to use the comparison.
> 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?
> I would much rather get a null pointer deference oops that reveals that
> assumptions about behavior are incorrect than try to debug behavioral
> errors that are masked by extra null pointer checking.
>
OK.
> > +
> > + 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.
> > +
> > + spin_lock_irqsave(&xhci->lock, flags);
> > + ret = usb_hcd_link_urb_to_ep(hcd, urb);
> > + spin_unlock_irqrestore(&xhci->lock, flags);
>
> I think this breaks the cancellation case. The urb should be linked and
> the TDs should be added to the endpoint list as one atomic operation
> under xhci->lock. If you move this code into prepare_transfer()
> instead, you'll be protected by the lock, and I think your code will
> look cleaner. You can only link the URB if the td_index passed to
> prepare_transfer() is 0.
>
Good point. I'll move the usb_hcd_link_urb_to_ep() into
prepare_transfer() again. I moved it before because xhci_queue_isoc_tx()
will call prepare_transfer() multiple times. But yes, it can be
workaround by using td_index.
> >
> > +void urb_free_priv(struct xhci_hcd *xhci, struct urb_priv *urb_priv)
> > +{
> > + int last = urb_priv->length - 1;
> > +
>
> Why not make this function return if the passed in urb_priv is NULL?
> That would make your clean up code simpler in all the other functions.
>
Good suggestion. Will apply.
> > + if (last >= 0) {
> > + int i;
> > + struct xhci_td *td;
> > +
> > + for (i = 0; i <= last; i++) {
> > + td = urb_priv->td[i];
> > + kfree(td);
>
> Just use kfree(urb_priv->td[i]) and then you can get rid of the curly
> braces and the extra variable.
>
Good suggestion. Will apply.
--
Thanks,
Andiry
--
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
