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 majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
