On Wed, 16 Jan 2013, Sarah Sharp wrote: > > Sarah, I looked through the xhci-hcd driver. There does appear to be a > > leak in xhci-ring.c:handle_tx_event(). > > Thanks for catching that. > > > The routine looks like this: > > > > /* 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)) > > xhci_urb_free_priv(xhci, urb_priv); > > > > ... > > > > /* EHCI, UHCI, and OHCI always unconditionally set the > > * urb->status of an isochronous endpoint to 0. > > */ > > if (usb_pipetype(urb->pipe) == PIPE_ISOCHRONOUS) > > status = 0; > > usb_hcd_giveback_urb(bus_to_hcd(urb->dev->bus), urb, status); > > > > If the condition on the first "if" statement fails, urb_priv won't be > > deallocated. It needs something like > > > > if (...) > > xhci_urb_free_priv(xhci, urb_priv); > > + else > > + kfree(urb_priv); > > Ok, so you're proposing freeing the urb_priv, but leaving the TD > allocated for the Set TR dequeue functions to use? Yes, that looks like > the right solution, feel free to submit a patch. Okay, if Martin confirms the diagnosis. > > Martin, can you tell if adding these two lines fixes the problem? > > > > Also, the comment is wrong. urb->status is not set to 0 > > unconditionally for isochonrous endpoints in [eou]hcd-hcd. If any of > > the iso packets gets a nonzero status then urb->status is set to one of > > those values, not to 0. My mistake -- these drivers _do_ return 0 status for all isochronous URBs. However, ehci-hcd and uhci-hcd set urb->error_count to the number of packets getting an error, whereas ohci-hcd and xhci-hcd don't. Fixing ohci-hcd will be very easy. Alan Stern -- 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
