Hi Dan,
I'm attempting to put my queue together for usb-next, and this patch
doesn't apply, due to conflicts in the USB core. Can you rebase this
patchset on top of my for-usb-next-queue branch, and resend the last two
patches? The first three applied fine.
Thanks,
Sarah Sharp
On Tue, Oct 08, 2013 at 12:48:12AM +0000, Williams, Dan J wrote:
> Subject: usb: xhci: change enumeration scheme to 'new scheme' by default
>
> From: Dan Williams <dan.j.williams@xxxxxxxxx>
>
> Change the default enumeration scheme for xhci attached non-SuperSpeed
> devices from:
>
> Reset
> SetAddress [xhci address-device BSR = 0]
> GetDescriptor(8)
> GetDescriptor(18)
>
> ...to:
>
> Reset
> [xhci address-device BSR = 1]
> GetDescriptor(64)
> Reset
> SetAddress [xhci address-device BSR = 0]
> GetDescriptor(18)
>
> ...as some devices misbehave when encountering a SetAddress command
> prior to GetDescriptor. There are known legacy devices that require
> this scheme, but testing has found at least one USB3 device that fails
> enumeration when presented with this ordering. For now, follow the ehci
> case and enable 'new scheme' by default for non-SuperSpeed devices.
>
> To support this enumeration scheme on xhci the AddressDevice operation
> needs to be performed twice. The first instance of the command enables
> the HC's device and slot context info for the device, but omits sending
> the device a SetAddress command (BSR == block set address request).
> Then, after GetDescriptor completes, follow up with the full
> AddressDevice+SetAddress operation.
>
> As mentioned before, this ordering of events with USB3 devices causes an
> extra state transition to be exposed to xhci. Previously USB3 devices
> would transition directly from 'enabled' to 'addressed' and never need
> to underrun responses to 'get descriptor'. We do see the 64-byte
> descriptor fetch the correct data, but the following 18-byte descriptor
> read after the reset gets:
>
> bLength = 0
> bDescriptorType = 0
> bcdUSB = 0
> bDeviceClass = 0
> bDeviceSubClass = 0
> bDeviceProtocol = 0
> bMaxPacketSize0 = 9
>
> instead of:
>
> bLength = 12
> bDescriptorType = 1
> bcdUSB = 300
> bDeviceClass = 0
> bDeviceSubClass = 0
> bDeviceProtocol = 0
> bMaxPacketSize0 = 9
>
> which results in the discovery process looping until falling back to
> 'old scheme' enumeration.
>
> Acked-by: Alan Stern <stern@xxxxxxxxxxxxxxxxxxx>
> Reported-by: David Moore <david.moore@xxxxxxxxx>
> Suggested-by: Sarah Sharp <sarah.a.sharp@xxxxxxxxxxxxxxx>
> Signed-off-by: Dan Williams <dan.j.williams@xxxxxxxxx>
> ---
>
> Here is the patch again with the changelog clarifications. Alan, I took
> your comments to mean acked-by, let me know it that's not the case.
>
> --
> Dan
>
>
> drivers/usb/core/hub.c | 44 ++++++++++++++++++++++++++++++++++++++----
> drivers/usb/host/xhci-pci.c | 1 +
> drivers/usb/host/xhci-plat.c | 1 +
> drivers/usb/host/xhci-ring.c | 6 +++---
> drivers/usb/host/xhci.c | 19 ++++++++++++++----
> drivers/usb/host/xhci.h | 11 ++++++++++-
> include/linux/usb/hcd.h | 2 ++
> 7 files changed, 72 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
> index c2c5f5adb464..42069ac9c6b2 100644
> --- a/drivers/usb/core/hub.c
> +++ b/drivers/usb/core/hub.c
> @@ -2519,6 +2519,21 @@ static unsigned hub_is_wusb(struct usb_hub *hub)
> #define HUB_LONG_RESET_TIME 200
> #define HUB_RESET_TIMEOUT 800
>
> +/*
> + * "New scheme" enumeration causes an extra state transition to be
> + * exposed to an xhci host and causes USB3 devices to receive control
> + * commands in the default state. This has been seen to cause
> + * enumeration failures, so disable this enumeration scheme for USB3
> + * devices.
> + */
> +static bool use_new_scheme(struct usb_device *udev, int retry)
> +{
> + if (udev->speed == USB_SPEED_SUPER)
> + return false;
> +
> + return USE_NEW_SCHEME(retry);
> +}
> +
> static int hub_port_reset(struct usb_hub *hub, int port1,
> struct usb_device *udev, unsigned int delay, bool warm);
>
> @@ -3951,6 +3966,20 @@ static int hub_set_address(struct usb_device *udev, int devnum)
> return retval;
> }
>
> +static int hub_enable_device(struct usb_device *udev)
> +{
> + struct usb_hcd *hcd = bus_to_hcd(udev->bus);
> +
> + if (!hcd->driver->enable_device)
> + return 0;
> + if (udev->state == USB_STATE_ADDRESS)
> + return 0;
> + if (udev->state != USB_STATE_DEFAULT)
> + return -EINVAL;
> +
> + return hcd->driver->enable_device(hcd, udev);
> +}
> +
> /* Reset device, (re)assign address, get device descriptor.
> * Device connection must be stable, no more debouncing needed.
> * Returns device in USB_STATE_ADDRESS, except on error.
> @@ -4063,7 +4092,7 @@ hub_port_init (struct usb_hub *hub, struct usb_device *udev, int port1,
> * this area, and this is how Linux has done it for ages.
> * Change it cautiously.
> *
> - * NOTE: If USE_NEW_SCHEME() is true we will start by issuing
> + * NOTE: If use_new_scheme() is true we will start by issuing
> * a 64-byte GET_DESCRIPTOR request. This is what Windows does,
> * so it may help with some non-standards-compliant devices.
> * Otherwise we start with SET_ADDRESS and then try to read the
> @@ -4071,10 +4100,13 @@ hub_port_init (struct usb_hub *hub, struct usb_device *udev, int port1,
> * value.
> */
> for (i = 0; i < GET_DESCRIPTOR_TRIES; (++i, msleep(100))) {
> - if (USE_NEW_SCHEME(retry_counter) && !(hcd->driver->flags & HCD_USB3)) {
> - struct usb_device_descriptor *buf;
> + struct usb_device_descriptor *buf;
> + bool did_new_scheme = false;
> +
> + if (use_new_scheme(udev, retry_counter)) {
> int r = 0;
>
> + did_new_scheme = true;
> #define GET_DESCRIPTOR_BUFSIZE 64
> buf = kmalloc(GET_DESCRIPTOR_BUFSIZE, GFP_NOIO);
> if (!buf) {
> @@ -4082,6 +4114,10 @@ hub_port_init (struct usb_hub *hub, struct usb_device *udev, int port1,
> continue;
> }
>
> + retval = hub_enable_device(udev);
> + if (retval < 0)
> + goto fail;
> +
> /* Retry on all errors; some devices are flakey.
> * 255 is for WUSB devices, we actually need to use
> * 512 (WUSB1.0[4.8.1]).
> @@ -4163,7 +4199,7 @@ hub_port_init (struct usb_hub *hub, struct usb_device *udev, int port1,
> * - read ep0 maxpacket even for high and low speed,
> */
> msleep(10);
> - if (USE_NEW_SCHEME(retry_counter) && !(hcd->driver->flags & HCD_USB3))
> + if (did_new_scheme)
> break;
> }
>
> diff --git a/drivers/usb/host/xhci-pci.c b/drivers/usb/host/xhci-pci.c
> index 236c3aabe940..96e51949c8b0 100644
> --- a/drivers/usb/host/xhci-pci.c
> +++ b/drivers/usb/host/xhci-pci.c
> @@ -306,6 +306,7 @@ static const struct hc_driver xhci_pci_hc_driver = {
> .check_bandwidth = xhci_check_bandwidth,
> .reset_bandwidth = xhci_reset_bandwidth,
> .address_device = xhci_address_device,
> + .enable_device = xhci_enable_device,
> .update_hub_device = xhci_update_hub_device,
> .reset_device = xhci_discover_or_reset_device,
>
> diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c
> index d9c169f470d3..5413861ac0f5 100644
> --- a/drivers/usb/host/xhci-plat.c
> +++ b/drivers/usb/host/xhci-plat.c
> @@ -69,6 +69,7 @@ static const struct hc_driver xhci_plat_xhci_driver = {
> .check_bandwidth = xhci_check_bandwidth,
> .reset_bandwidth = xhci_reset_bandwidth,
> .address_device = xhci_address_device,
> + .enable_device = xhci_enable_device,
> .update_hub_device = xhci_update_hub_device,
> .reset_device = xhci_discover_or_reset_device,
>
> diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
> index 46d7fb8de990..515a21009d7e 100644
> --- a/drivers/usb/host/xhci-ring.c
> +++ b/drivers/usb/host/xhci-ring.c
> @@ -3962,12 +3962,12 @@ int xhci_queue_slot_control(struct xhci_hcd *xhci, u32 trb_type, u32 slot_id)
>
> /* Queue an address device command TRB */
> int xhci_queue_address_device(struct xhci_hcd *xhci, dma_addr_t in_ctx_ptr,
> - u32 slot_id)
> + u32 slot_id, enum xhci_setup_dev setup)
> {
> return queue_command(xhci, lower_32_bits(in_ctx_ptr),
> upper_32_bits(in_ctx_ptr), 0,
> - TRB_TYPE(TRB_ADDR_DEV) | SLOT_ID_FOR_TRB(slot_id),
> - false);
> + TRB_TYPE(TRB_ADDR_DEV) | SLOT_ID_FOR_TRB(slot_id)
> + | (setup == SETUP_CONTEXT_ONLY ? TRB_BSR : 0), false);
> }
>
> int xhci_queue_vendor_command(struct xhci_hcd *xhci,
> diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
> index d05ff362ddc7..b08369ed2f87 100644
> --- a/drivers/usb/host/xhci.c
> +++ b/drivers/usb/host/xhci.c
> @@ -3705,12 +3705,13 @@ disable_slot:
> }
>
> /*
> - * Issue an Address Device command (which will issue a SetAddress request to
> - * the device).
> + * Issue an Address Device command and optionally send a corresponding
> + * SetAddress request to the device.
> * We should be protected by the usb_address0_mutex in khubd's hub_port_init, so
> * we should only issue and wait on one address command at the same time.
> */
> -int xhci_address_device(struct usb_hcd *hcd, struct usb_device *udev)
> +static int xhci_setup_device(struct usb_hcd *hcd, struct usb_device *udev,
> + enum xhci_setup_dev setup)
> {
> unsigned long flags;
> int timeleft;
> @@ -3769,7 +3770,7 @@ int xhci_address_device(struct usb_hcd *hcd, struct usb_device *udev)
> spin_lock_irqsave(&xhci->lock, flags);
> cmd_trb = xhci_find_next_enqueue(xhci->cmd_ring);
> ret = xhci_queue_address_device(xhci, virt_dev->in_ctx->dma,
> - udev->slot_id);
> + udev->slot_id, setup);
> if (ret) {
> spin_unlock_irqrestore(&xhci->lock, flags);
> xhci_dbg_trace(xhci, trace_xhci_dbg_address,
> @@ -3864,6 +3865,16 @@ int xhci_address_device(struct usb_hcd *hcd, struct usb_device *udev)
> return 0;
> }
>
> +int xhci_address_device(struct usb_hcd *hcd, struct usb_device *udev)
> +{
> + return xhci_setup_device(hcd, udev, SETUP_CONTEXT_ADDRESS);
> +}
> +
> +int xhci_enable_device(struct usb_hcd *hcd, struct usb_device *udev)
> +{
> + return xhci_setup_device(hcd, udev, SETUP_CONTEXT_ONLY);
> +}
> +
> /*
> * Transfer the port index into real index in the HW port status
> * registers. Caculate offset between the port's PORTSC register
> diff --git a/drivers/usb/host/xhci.h b/drivers/usb/host/xhci.h
> index 5f9dc1c1b538..e880776151ae 100644
> --- a/drivers/usb/host/xhci.h
> +++ b/drivers/usb/host/xhci.h
> @@ -1096,6 +1096,14 @@ struct xhci_event_cmd {
> };
>
> /* flags bitmasks */
> +
> +/* Address device - disable SetAddress */
> +#define TRB_BSR (1<<9)
> +enum xhci_setup_dev {
> + SETUP_CONTEXT_ONLY,
> + SETUP_CONTEXT_ADDRESS,
> +};
> +
> /* bits 16:23 are the virtual function ID */
> /* bits 24:31 are the slot ID */
> #define TRB_TO_SLOT_ID(p) (((p) & (0xff<<24)) >> 24)
> @@ -1787,6 +1795,7 @@ int xhci_free_streams(struct usb_hcd *hcd, struct usb_device *udev,
> struct usb_host_endpoint **eps, unsigned int num_eps,
> gfp_t mem_flags);
> int xhci_address_device(struct usb_hcd *hcd, struct usb_device *udev);
> +int xhci_enable_device(struct usb_hcd *hcd, struct usb_device *udev);
> int xhci_update_device(struct usb_hcd *hcd, struct usb_device *udev);
> int xhci_set_usb2_hardware_lpm(struct usb_hcd *hcd,
> struct usb_device *udev, int enable);
> @@ -1810,7 +1819,7 @@ int xhci_is_vendor_info_code(struct xhci_hcd *xhci, unsigned int trb_comp_code);
> void xhci_ring_cmd_db(struct xhci_hcd *xhci);
> int xhci_queue_slot_control(struct xhci_hcd *xhci, u32 trb_type, u32 slot_id);
> int xhci_queue_address_device(struct xhci_hcd *xhci, dma_addr_t in_ctx_ptr,
> - u32 slot_id);
> + u32 slot_id, enum xhci_setup_dev);
> int xhci_queue_vendor_command(struct xhci_hcd *xhci,
> u32 field1, u32 field2, u32 field3, u32 field4);
> int xhci_queue_stop_endpoint(struct xhci_hcd *xhci, int slot_id,
> diff --git a/include/linux/usb/hcd.h b/include/linux/usb/hcd.h
> index fc64b6825f5e..21a8a4286063 100644
> --- a/include/linux/usb/hcd.h
> +++ b/include/linux/usb/hcd.h
> @@ -352,6 +352,8 @@ struct hc_driver {
> void (*reset_bandwidth)(struct usb_hcd *, struct usb_device *);
> /* Returns the hardware-chosen device address */
> int (*address_device)(struct usb_hcd *, struct usb_device *udev);
> + /* prepares the hardware to send commands to the device */
> + int (*enable_device)(struct usb_hcd *, struct usb_device *udev);
> /* Notifies the HCD after a hub descriptor is fetched.
> * Will block.
> */
>
--
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
