On Fri, Feb 18, 2011 at 11:02:19AM -0500, Alan Stern wrote:
> The USB stack historically has conflated device numbers (i.e., the
> value of udev->devnum) with device addresses. This is understandable,
> because until recently the two values were always the same.
>
> But with USB-3.0 they aren't the same, so we should start calling
> these things by their correct names. This patch (as1449) changes many
> of the references to "address" in the hub driver to "device number"
> or "devnum".
>
> Signed-off-by: Alan Stern <stern@xxxxxxxxxxxxxxxxxxx>
> Reported-by: Luben Tuikov <ltuikov@xxxxxxxxx>
Reviewed-by: Sarah Sharp <sarah.a.sharp@xxxxxxxxxxxxxxx>
Looks good!
> Sarah:
>
> While doing this, I noticed a comment in hub_port_init():
>
> /*
> * An xHCI controller cannot send any packets to a device until
> * a set address command successfully completes.
> */
>
> While strictly true, this is very misleading because a casual reader
> will think it refers to a USB Set-Address request, not realizing it
> actually refers to an xHCI SET_ADDRESS command. (Even more
> misleadingly, a SET_ADDRESS command doesn't necessarily send a
> Set-Address request to the device!) Furthermore, the comment seems a
> little out of place and unnecessary here. Do you want to remove it?
> Or should I update this patch to remove it?
Eh, you should just remove it in this patch.
> On a related note... The reason for the USB_NEW_SCHEME() stuff in
> hub_port_init() was that some buggy devices won't enumerate properly if
> the host doesn't send a Get-Device-Descriptor request before sending a
> Set-Address request. (That's how Windows has always worked.) As far
> as I can tell, these buggy devices won't work with the current USB-3.0
> enumeration code. Do you think we need to handle this? Maybe the same
> approach should be used for all devices, regardless of the controller
> type or the device speed.
In the past two years we've had the xHCI driver available, only one
person with a really really old mouse complained about it not working
with the new enumeration sequence. The xHCI driver can be changed to
use the Windows enumeration sequence, but that requires a new USB core
API. I sent mail about the API when the person complained, but since I
didn't get any more complaints and the person didn't really seem to care
about their mouse, I never took the time to actually do it.
I'd like to wait a bit and see if any more users still have these buggy
devices and care about them. If I start receiving lots of complaints
then, yes, I'll write the code. Until then, I'd rather keep the
simpler, faster enumeration sequence.
Sarah Sharp
> Index: usb-2.6/drivers/usb/core/hub.c
> ===================================================================
> --- usb-2.6.orig/drivers/usb/core/hub.c
> +++ usb-2.6/drivers/usb/core/hub.c
> @@ -1499,6 +1499,13 @@ void usb_set_device_state(struct usb_dev
> EXPORT_SYMBOL_GPL(usb_set_device_state);
>
> /*
> + * Choose a device number.
> + *
> + * Device numbers are used as filenames in usbfs. On USB-1.1 and
> + * USB-2.0 buses they are also used as device addresses, however on
> + * USB-3.0 buses the address is assigned by the controller hardware
> + * and it usually is not the same as the device number.
> + *
> * WUSB devices are simple: they have no hubs behind, so the mapping
> * device <-> virtual port number becomes 1:1. Why? to simplify the
> * life of the device connection logic in
> @@ -1520,7 +1527,7 @@ EXPORT_SYMBOL_GPL(usb_set_device_state);
> * the HCD must setup data structures before issuing a set address
> * command to the hardware.
> */
> -static void choose_address(struct usb_device *udev)
> +static void choose_devnum(struct usb_device *udev)
> {
> int devnum;
> struct usb_bus *bus = udev->bus;
> @@ -1545,7 +1552,7 @@ static void choose_address(struct usb_de
> }
> }
>
> -static void release_address(struct usb_device *udev)
> +static void release_devnum(struct usb_device *udev)
> {
> if (udev->devnum > 0) {
> clear_bit(udev->devnum, udev->bus->devmap.devicemap);
> @@ -1553,7 +1560,7 @@ static void release_address(struct usb_d
> }
> }
>
> -static void update_address(struct usb_device *udev, int devnum)
> +static void update_devnum(struct usb_device *udev, int devnum)
> {
> /* The address for a WUSB device is managed by wusbcore. */
> if (!udev->wusb)
> @@ -1600,7 +1607,8 @@ void usb_disconnect(struct usb_device **
> * this quiesces everyting except pending urbs.
> */
> usb_set_device_state(udev, USB_STATE_NOTATTACHED);
> - dev_info (&udev->dev, "USB disconnect, address %d\n", udev->devnum);
> + dev_info(&udev->dev, "USB disconnect, device number %d\n",
> + udev->devnum);
>
> usb_lock_device(udev);
>
> @@ -1630,7 +1638,7 @@ void usb_disconnect(struct usb_device **
> /* Free the device number and delete the parent's children[]
> * (or root_hub) pointer.
> */
> - release_address(udev);
> + release_devnum(udev);
>
> /* Avoid races with recursively_mark_NOTATTACHED() */
> spin_lock_irq(&device_state_lock);
> @@ -2071,7 +2079,7 @@ static int hub_port_reset(struct usb_hub
> case 0:
> /* TRSTRCY = 10 ms; plus some extra */
> msleep(10 + 40);
> - update_address(udev, 0);
> + update_devnum(udev, 0);
> if (hcd->driver->reset_device) {
> status = hcd->driver->reset_device(hcd, udev);
> if (status < 0) {
> @@ -2619,7 +2627,7 @@ static int hub_set_address(struct usb_de
> USB_REQ_SET_ADDRESS, 0, devnum, 0,
> NULL, 0, USB_CTRL_SET_TIMEOUT);
> if (retval == 0) {
> - update_address(udev, devnum);
> + update_devnum(udev, devnum);
> /* Device now using proper address. */
> usb_set_device_state(udev, USB_STATE_ADDRESS);
> usb_ep0_reinit(udev);
> @@ -2728,9 +2736,9 @@ hub_port_init (struct usb_hub *hub, stru
> }
> if (udev->speed != USB_SPEED_SUPER)
> dev_info(&udev->dev,
> - "%s %s speed %sUSB device using %s and address %d\n",
> + "%s %s speed %sUSB device number %d using %s\n",
> (udev->config) ? "reset" : "new", speed, type,
> - udev->bus->controller->driver->name, devnum);
> + devnum, udev->bus->controller->driver->name);
>
> /* Set up TT records, if needed */
> if (hdev->tt) {
> @@ -2846,9 +2854,9 @@ hub_port_init (struct usb_hub *hub, stru
> if (udev->speed == USB_SPEED_SUPER) {
> devnum = udev->devnum;
> dev_info(&udev->dev,
> - "%s SuperSpeed USB device using %s and address %d\n",
> + "%s SuperSpeed USB device number %d using %s\n",
> (udev->config) ? "reset" : "new",
> - udev->bus->controller->driver->name, devnum);
> + devnum, udev->bus->controller->driver->name);
> }
>
> /* cope with hardware quirkiness:
> @@ -2911,7 +2919,7 @@ hub_port_init (struct usb_hub *hub, stru
> fail:
> if (retval) {
> hub_port_disable(hub, port1, 0);
> - update_address(udev, devnum); /* for disconnect processing */
> + update_devnum(udev, devnum); /* for disconnect processing */
> }
> mutex_unlock(&usb_address0_mutex);
> return retval;
> @@ -3120,15 +3128,7 @@ static void hub_port_connect_change(stru
> else
> udev->speed = USB_SPEED_UNKNOWN;
>
> - /*
> - * Set the address.
> - * Note xHCI needs to issue an address device command later
> - * in the hub_port_init sequence for SS/HS/FS/LS devices,
> - * and xHC will assign an address to the device. But use
> - * kernel assigned address here, to avoid any address conflict
> - * issue.
> - */
> - choose_address(udev);
> + choose_devnum(udev);
> if (udev->devnum <= 0) {
> status = -ENOTCONN; /* Don't retry */
> goto loop;
> @@ -3220,7 +3220,7 @@ loop_disable:
> hub_port_disable(hub, port1, 1);
> loop:
> usb_ep0_reinit(udev);
> - release_address(udev);
> + release_devnum(udev);
> hub_free_dev(udev);
> usb_put_dev(udev);
> if ((status == -ENOTCONN) || (status == -ENOTSUPP))
>
--
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
