From: Sarah> Sharp
> On Wed, Dec 18, 2013 at 11:24:47AM -0000, David Laight wrote:
> > This saves a kzalloc() call on every transfer and some memory
> > indirections.
> >
...
> Hi David,
>
> The patch looks good in general and applies fine. However, in testing
> this with a USB mass storage device, I ran across a warning, followed by
> an oops that hung the system:
I've just had a look at the xhci_urb_dequeue() code paths and I think
I know why it doesn't work.
I'd rather made the assumption that the existing code was correct!
The 'giveaway' lines are these from handle_tx_event() in xhci-ring.c
2655 urb_priv = urb->hcpriv;
2656 /* Leave the TD around for the reset endpoint function
2657 * to use(but only if it's not a control endpoint,
2658 * since we already queued the Set TR dequeue pointer
2659 * command for stalled control endpoints).
2660 */
2661 if (usb_endpoint_xfer_control(&urb->ep->desc) ||
2662 (trb_comp_code != COMP_STALL &&
2663 trb_comp_code != COMP_BABBLE))
2664 xhci_urb_free_priv(xhci, urb_priv);
2665 else
2666 kfree(urb_priv);
I think the TD from the second kmalloc() are on the cancelled_td_list
and get freed much later on, and one at a time.
I've not checked that code for memory leaks or double frees, however
consider what happens for an isoc urb that requested multiple TD.
xhci_urb_dequeue() adds each TD separately to the cancelled_td_list.
Since the urb itself and the urb_priv are freed there is nothing
left to indicate that the TD were allocated from by a single kmalloc().
So eventually kfree() will be called separately for each TD.
The location of the kernel oops is left as an exercise to the reader.
David
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
