On Mon, Sep 23, 2013 at 04:29:40PM -0700, Dan Williams wrote:
> Change the enumeration scheme for xhci attached devices from:
>
> SetAddress
> GetDescriptor(8)
> GetDescriptor(18)
>
> ...to:
>
> GetDescriptor(64)
> SetAddress
> GetDescriptor(18)
Thinking about this sequence again, I'm trying to figure out if that
first 64-byte descriptor fetch is going to be an issue. We set up the
control endpoint context with a max packet size of 8 bytes, but it can
be larger, depending on the device. We don't know what it is until we
fetch the device descriptors.
So if the USB core asks for 64-bytes of the device descriptor before the
xHCI driver sets the real max packet size in the control endpoint
context, won't the host controller expect to receive 8-byte packets, but
may instead get larger packets?
Alan, how would this work under EHCI?
Dan, I was also testing under HSW-ULT with a (possibly buggy) device,
and I managed to get the device context into a bad state. The device
refused to respond to the first 64-byte control transfer (it timed out
three times), and then the USB core reset the device. The xHCI driver
then attempted to issue a Reset Device command, which failed because the
device was in the default state.
I think this meant the endpoint toggles in the control endpoint on the
device were mis-matched to the host's toggle state. That would cause
protocol errors. I'm not quite sure what the solution for this is yet,
but I think it may be why you get the context state error on the second
Set Address command. Because the Reset Device command didn't complete
successfully, the host thinks the device is still in the default state.
Not sure why the Set Address with BSR=1 fails on a different system.
Sarah Sharp
> ...as some devices misbehave when encountering a SetAddress command
> prior to GetDescriptor. There are known devices that require this
> enumeration scheme but is unknown how much, if any, regression there
> will be of xhci-attached devices that can not tolerate the change. For
> now, follow the ehci case and enable 'new scheme' by default. Of course
> the existing 'old_scheme_first' and 'use_both_schemes' usbcore module
> parameters are available to debug / workaround regressions.
>
> 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.
>
> Reported-by: David Moore <david.mo...@gmail.com>
> Suggested-by: Sarah Sharp <sarah.a.sh...@linux.intel.com>
> Signed-off-by: Dan Williams <dan.j.willi...@intel.com>
> ---
> drivers/usb/core/hub.c | 22 ++++++++++++++++++++--
> 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, 52 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
> index 69bbb51..355a09d 100644
> --- a/drivers/usb/core/hub.c
> +++ b/drivers/usb/core/hub.c
> @@ -3943,6 +3943,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 +4077,7 @@ 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)) {
> + if (USE_NEW_SCHEME(retry_counter)) {
> struct usb_device_descriptor *buf;
> int r = 0;
>
> @@ -4074,6 +4088,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]).
> @@ -4155,7 +4173,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 (USE_NEW_SCHEME(retry_counter))
> break;
> }
>
> diff --git a/drivers/usb/host/xhci-pci.c b/drivers/usb/host/xhci-pci.c
> index c2d4950..df564d1 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 d9c169f..5413861 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 7c043ec..644e2c6 100644
> --- a/drivers/usb/host/xhci-ring.c
> +++ b/drivers/usb/host/xhci-ring.c
> @@ -3929,12 +3929,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 763d0a5..61ecbee 100644
> --- a/drivers/usb/host/xhci.c
> +++ b/drivers/usb/host/xhci.c
> @@ -3720,12 +3720,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;
> @@ -3784,7 +3785,7 @@ int xhci_address_device(struct usb_hcd *hcd, struct
> usb_device *udev)
> spin_lock_irqsave(&xhci->lock, flags);
> cmd_trb = xhci->cmd_ring->dequeue;
> 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,
> @@ -3879,6 +3880,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 6c7aa90..d71a8d7 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)
> @@ -1777,6 +1785,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);
> @@ -1800,7 +1809,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 a9c7d44..baca889f 100644
> --- a/include/linux/usb/hcd.h
> +++ b/include/linux/usb/hcd.h
> @@ -350,6 +350,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 majord...@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
