Re: [PATCH 1/6] USB: Fix stale usb devices in usb_device_list

[Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com>]

On 07/24/2014 06:09 PM, Sebastian Hesselbarth wrote:
> On 07/24/2014 03:28 PM, Sebastian Hesselbarth wrote:
>> New usb devices are added to a list of usb devices, but when removing
>> the corresponding usb_device it was not removed from that list. Fix it
>> by deleting it properly from the usb_device_list.
>>
>> Signed-off-by: Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com>
>> ---
>> To: Sascha Hauer <s.hauer@pengutronix.de>
>> Cc: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
>> Cc: Ezequiel Garcia <ezequiel.garcia@free-electrons.com>
>> Cc: barebox@lists.infradead.org
>> ---
>>   drivers/usb/core/usb.c | 1 +
>>   1 file changed, 1 insertion(+)
>>
>> diff --git a/drivers/usb/core/usb.c b/drivers/usb/core/usb.c
>> index 351e783b6536..2cc338465539 100644
>> --- a/drivers/usb/core/usb.c
>> +++ b/drivers/usb/core/usb.c
>> @@ -472,6 +472,7 @@ void usb_remove_device(struct usb_device *usbdev)
>>           dev_err(&usbdev->dev, "failed to unregister\n");
>>
>>       usbdev->parent->children[usbdev->portnr - 1] = NULL;
>> +    list_del(&usbdev->list);
> 
> Unfortunately, this breaks usb_remove_device when called on a
> device that has not yet gone through usb_new_device().
> 
> I'll have a closer look at it.

Ok, this patch is fine as is.

The issue comes from  usb_hub_port_connect_change():

/* Disconnect any existing devices under this port */
if (((!(portstatus & USB_PORT_STAT_CONNECTION)) &&
     (!(portstatus & USB_PORT_STAT_ENABLE))) || (dev->children[port])) {
	dev_dbg(&dev->dev, "usb_disconnect(&hub->children[port]);\n");
	usb_remove_device(dev->children[port]);
	/* Return now if nothing is connected */
	if (!(portstatus & USB_PORT_STAT_CONNECTION))
	return;
}

Above will call usb_remove_device() if CONNECTION _and_ ENABLE is not
set *or* dev->children[port] is set.

Although very unlikely we encounter a device removal without having
successfully detected the device (and set dev->children[port]) before,
we should never call usb_remove_device() with NULL.

To me it looks like somebody tried to invert above checks at some
point and failed.

Replacing the check with something more readable solves the issue
I have been suffering (ok, it was during testing USB3.0 stuff):

if (dev->children[port] && !(portstatus & USB_PORT_STAT_CONNECTION)) {
	dev_dbg(&dev->dev, "disconnect detected on port %d\n", port + 1);
	usb_remove_device(dev->children[port]);
	return;
}

/* Deallocate disabled but connected devices */
if (dev->children[port] && !(portstatus & USB_PORT_STAT_ENABLE))
	usb_remove_device(dev->children[port]);

Also, a check for usbdev == NULL in usb_remove_device() to make
sure it never happens again.

I'll send a v2 of xHCI anyway, so I'll add a patch on top.

Sebastian

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox