On Fri, Jun 01, 2012 at 10:06:24AM +0200, Takashi Iwai wrote:
> This patch fixes a few issues introduced in the recent fix
> [f8a9e72d: USB: fix resource leak in xhci power loss path]
>
> - The endpoints listed in bw table are just links and each entry is an
> array member of dev->eps[]. But the commit above adds a kfree() call
> to these instances, and thus it results in memory corruption.
I tested your patch, hoping it would fix some warnings I had seen when
the xHCI driver was unloaded, but it didn't help. I still think this
patch is correct, but we're still leaking some memory from the xHCI
driver.
When I plug in a multi-TT USB 2.0 hub, and then unload the xHCI driver,
I get the warning "Slot 2 endpoint 2 not removed from BW list!":
Jun 7 17:31:08 maggie kernel: [ 218.431583] usb usb1: USB disconnect, device number 1
Jun 7 17:31:08 maggie kernel: [ 218.431585] usb 1-2: USB disconnect, device number 3
Jun 7 17:31:08 maggie kernel: [ 218.431587] usb 1-2: unregistering device
Jun 7 17:31:08 maggie kernel: [ 218.431588] usb 1-2: unregistering interface 1-2:1.0
Jun 7 17:31:08 maggie kernel: [ 218.432118] usb 1-2: usb_disable_device nuking all URBs
Jun 7 17:31:08 maggie kernel: [ 218.433030] xhci_hcd 0000:00:14.0: Slot 2 endpoint 2 not removed from BW list!
Jun 7 17:31:08 maggie kernel: [ 218.433106] usb usb1: unregistering device
Jun 7 17:31:08 maggie kernel: [ 218.433109] usb usb1: unregistering interface 1-0:1.0
Jun 7 17:31:08 maggie kernel: [ 218.433594] usb usb1: usb_disable_device nuking all URBs
Jun 7 17:31:08 maggie kernel: [ 218.433598] xHCI xhci_drop_endpoint called for root hub
Jun 7 17:31:08 maggie kernel: [ 218.433600] xHCI xhci_check_bandwidth called for root hub
Jun 7 17:31:08 maggie kernel: [ 218.434216] xhci_hcd 0000:00:14.0: // Halt the HC
That warning comes from this code in xhci-mem.c:xhci_free_virt_device():
for (i = 0; i < 31; ++i) {
if (dev->eps[i].ring)
xhci_ring_free(xhci, dev->eps[i].ring);
if (dev->eps[i].stream_info)
xhci_free_stream_info(xhci,
dev->eps[i].stream_info);
/* Endpoints on the TT/root port lists should have been removed
* when usb_disable_device() was called for the device.
* We can't drop them anyway, because the udev might have gone
* away by this point, and we can't tell what speed it was.
*/
if (!list_empty(&dev->eps[i].bw_endpoint_list))
xhci_warn(xhci, "Slot %u endpoint %u "
"not removed from BW list!\n",
slot_id, i);
Even though the endpoint bandwidth list head should be removed later in
xhci_mem_cleanup(), I wondered why it wouldn't have been removed by the
USB in usb_disable_device(). That's the way it's supposed to work:
when a USB device is disconnected, we free any resources associated with
its endpoints by calling xhci_drop_endpoint() and
xhci_check_bandwidth().
Long story short, all xHCI functions check whether the xHC is halted
before issuing any commands. When the xHCI driver is unloaded, the USB
core first disables the USB 3.0 roothub portion of the xHCI host, and
then disables the USB 2.0 roothub portion. When it's called with the
USB 3.0 roothub, xhci_stop() will halt the xHCI host controller and
return. The driver will get cleaned up when xhci_stop() is called with
the USB 2.0 roothub later.
After the SS roothub is disabled, but the HS roothub is still enabled,
the USB core comes and tries to clean up all connected USB 2.0 devices.
This includes calling usb_disable_device(). Howevrer,
xhci_check_bandwidth() will return immediately because the xHCI host is
halted, and we will leak memory resources.
That's why resources weren't being deallocated when the driver was
unloaded. This is sort of a separate issue, so I'll still accept your
patch, and send a second patch to fix the driver unload issue.
Sarah Sharp
--
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
