> -----Original Message----- > From: Sarah Sharp [mailto:sarah.a.sharp@xxxxxxxxxxxxxxx] > Sent: Tuesday, May 25, 2010 6:21 AM > To: Xu, Andiry > Cc: linux-usb@xxxxxxxxxxxxxxx; gregkh@xxxxxxx; Yang, Libin > Subject: Re: [PATCH RESEND v5 0/3] ISOC supporting for xHCI > > I'm not sure if this is an issue, but here's one thing I notice when > looking over the code. I think your code could get messed up because of > how you use urb_done in handle_tx_event(). > > It gets initialized to false at the beginning of the function: > > static int handle_tx_event(struct xhci_hcd *xhci, > struct xhci_transfer_event *event) > { > struct xhci_virt_device *xdev; > ... > bool urb_done = false; > > Later, you added this goto statement here: > > handle_td: > /* This TRB should be in the TD at the head of this ring's TD list > */ > if (list_empty(&ep_ring->td_list)) { > > When you find that all TDs of an URB are processed, you set urb_done to > true: > > urb_priv->td_cnt++; > /* Giveback urb when all the tds are completed */ > if (urb_priv->td_cnt == urb_priv->length) > urb_done = true; > > /* Leave the TD around for the reset endpoint function to > use > * (but only if it's not a control endpoint, since we > already > * queued the Set TR dequeue pointer command for stalled > * control endpoints). > */ > if (usb_endpoint_xfer_control(&urb->ep->desc) || > (trb_comp_code != COMP_STALL && > trb_comp_code != COMP_BABBLE)) { > if (urb_done) > urb_free_priv(xhci, urb_priv); > } > > Later, you look at urb_done to decide whether to give the URB back: > > if (urb && urb_done) { > usb_hcd_unlink_urb_from_ep(xhci_to_hcd(xhci), urb); > xhci_dbg(xhci, "Giveback URB %p, len = %d, status = %d\n", > urb, urb->actual_length, status); > spin_unlock(&xhci->lock); > usb_hcd_giveback_urb(xhci_to_hcd(xhci), urb, status); > spin_lock(&xhci->lock); > } > > If the ep->skip flag is set at the end of the function, you jump back to > handle_td. > > Now, what happens if the host has skipped multiple URB's worth of TDs? > On the last TD in the first skipped URB, urb_done will be set to true. > Then you'll give the URB back. Now you jump to handle_td, and urb_done > is *still* set to true (since you didn't execute the first part of the > function where you set it to false). > > You process the first TD in the next URB, and since the urb_done flag is > set, so you giveback the URB. Then you attempt to process the next TD > in the URB, and you've freed the urb_priv pointer, so you do a NULL > pointer dereference. > > So I'm not sure if that's your exact bug, but it seems likely. I think > if you drop the urb_done flag and replace it with (urb_priv->td_cnt == > urb_priv->length), you could work around this bug. > Thanks for catching this! I'll fix it. > Also, is there a way you can change the code so that you don't have the > handle_td jump? It's likely there will be other bugs like this. I know > that handle_tx_event() is a big mess, and I apologize for that. I had > some patches to break it up into smaller functions, but they no longer > apply against the current tree. > I will try finding a way not to use the handle_td jump. It can be replaced with do-while(), but break the big handle_tx_event() may be necessary. 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
