Andiry,
I think there is a bug in this patch, but I'm not sure exactly what
you're doing with virt_dev->configured, so I can't suggest how to fix
it.
The problem happens when the USB core tries to reset and re-address a
device that had been active and had some control transfers complete. An
example would be when the usb-storage driver tries to reset the device
after it gets an answer to a SCSI command that it doesn't like.
With this patch, after a Device Reset command finishes,
virt_dev->configured is set to false. When the USB core tries to
address the device, xhci_address_device() will call
xhci_setup_addressable_virt_dev(). This initializes the slot and
endpoint 0 contexts to their defaults, including setting the control
endpoint ring dequeue pointer to the top of the ring.
However, the control endpoint ring was used before, so the internal xHCI
driver internal dequeue pointer still points somewhere in the middle of
the ring. Worse, the hardware may go re-execute the control transfers
between the top of the ring and the internal dequeue pointer, because
the cycle bit says it owns those TRBs. Then the driver will see a bunch
of spurious events from the control endpoint ring the next time the
doorbell is rung. See the attached log for an example.
In the case of a device reset when control transfers have already
occurred, I think you really want to call
xhci_copy_ep0_dequeue_into_input_ctx() in xhci_address_device().
However, the code in xhci_address_device() won't do that, since
virt_dev->configured is always set to false after the Device Reset
command completes.
Were you trying to use virt_dev->configured to set up the slot context
for an address device command after a resume where the xHCI host lost
power and the virt_dev had to be reallocated? Doesn't the place where
you set it to false in xhci_alloc_dev() cover that case?
You could remove the statement where virt_dev->configured gets set to
false in after the Device Reset command completes, and that would fix
the problem with active devices being reset. But why make
xhci_address_device() just test some field in the slot context to see if
it's set properly, and call xhci_setup_addressable_virt_dev() if not?
Then you wouldn't need virt_dev->configured at all. You could check
that, say, slot_ctx->dev_info isn't zero (since the speed field in that
has to be set to some non-zero number).
Sarah Sharp
On Tue, Aug 31, 2010 at 04:44:53PM +0800, Andiry Xu wrote:
> >From bc53db5a7c84c2afad0f0dd938dec6c336e8ebe4 Mon Sep 17 00:00:00 2001
> From: Andiry Xu <andiry.xu@xxxxxxx>
> Date: Mon, 23 Aug 2010 13:29:35 +0800
> Subject: [PATCH 2/3] xHCI: change xhci_reset_device() to allocate new device
>
> Rename xhci_reset_device() to xhci_discover_or_reset_device().
> If xhci_discover_or_reset_device() is called to reset a device which does
> not exist or does not match the udev, it calls xhci_alloc_dev() to
> re-allocate the device.
>
> This would prevent the reset device failure, possibly due to the xHC restore
> error during S3/S4 resume. Note this change would affect xhci_address_device()
> behavior: udev->config exists, but the device is actually not configured yet.
> Add a flag in struct xhci_virt_device to indicate its actual state.
>
> Signed-off-by: Andiry Xu <andiry.xu@xxxxxxx>
> ---
> drivers/usb/host/xhci-mem.c | 1 +
> drivers/usb/host/xhci-pci.c | 2 +-
> drivers/usb/host/xhci.c | 37 ++++++++++++++++++++++++++++++++++---
> drivers/usb/host/xhci.h | 4 +++-
> 4 files changed, 39 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/usb/host/xhci-mem.c b/drivers/usb/host/xhci-mem.c
> index be90180..e7947ea 100644
> --- a/drivers/usb/host/xhci-mem.c
> +++ b/drivers/usb/host/xhci-mem.c
> @@ -779,6 +779,7 @@ int xhci_alloc_virt_device(struct xhci_hcd *xhci, int slot_id,
> init_completion(&dev->cmd_completion);
> INIT_LIST_HEAD(&dev->cmd_list);
> dev->udev = udev;
> + dev->configured = false;
>
> /* Point to output device context in dcbaa. */
> xhci->dcbaa->dev_context_ptrs[slot_id] = dev->out_ctx->dma;
> diff --git a/drivers/usb/host/xhci-pci.c b/drivers/usb/host/xhci-pci.c
> index f7efe02..aefc349 100644
> --- a/drivers/usb/host/xhci-pci.c
> +++ b/drivers/usb/host/xhci-pci.c
> @@ -152,7 +152,7 @@ static const struct hc_driver xhci_pci_hc_driver = {
> .reset_bandwidth = xhci_reset_bandwidth,
> .address_device = xhci_address_device,
> .update_hub_device = xhci_update_hub_device,
> - .reset_device = xhci_reset_device,
> + .reset_device = xhci_discover_or_reset_device,
>
> /*
> * scheduling support
> diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
> index 4201119..a0dca7c 100644
> --- a/drivers/usb/host/xhci.c
> +++ b/drivers/usb/host/xhci.c
> @@ -1372,6 +1372,7 @@ int xhci_check_bandwidth(struct usb_hcd *hcd, struct usb_device *udev)
> return ret;
> }
>
> + virt_dev->configured = true;
> xhci_dbg(xhci, "Output context after successful config ep cmd:\n");
> xhci_dbg_ctx(xhci, virt_dev->out_ctx,
> LAST_CTX_TO_EP_NUM(slot_ctx->dev_info));
> @@ -1943,8 +1944,13 @@ int xhci_free_streams(struct usb_hcd *hcd, struct usb_device *udev,
> * Wait for the Reset Device command to finish. Remove all structures
> * associated with the endpoints that were disabled. Clear the input device
> * structure? Cache the rings? Reset the control endpoint 0 max packet size?
> + *
> + * If the virt_dev to be reset does not exist or does not match the udev,
> + * it means the device is lost, possibly due to the xHC restore error and
> + * re-initialization during S3/S4. In this case, call xhci_alloc_dev() to
> + * re-allocate the device.
> */
> -int xhci_reset_device(struct usb_hcd *hcd, struct usb_device *udev)
> +int xhci_discover_or_reset_device(struct usb_hcd *hcd, struct usb_device *udev)
> {
> int ret, i;
> unsigned long flags;
> @@ -1955,12 +1961,36 @@ int xhci_reset_device(struct usb_hcd *hcd, struct usb_device *udev)
> int timeleft;
> int last_freed_endpoint;
>
> - ret = xhci_check_args(hcd, udev, NULL, 0, true, __func__);
> + ret = xhci_check_args(hcd, udev, NULL, 0, false, __func__);
> if (ret <= 0)
> return ret;
> xhci = hcd_to_xhci(hcd);
> slot_id = udev->slot_id;
> virt_dev = xhci->devs[slot_id];
> + if (!virt_dev) {
> + xhci_dbg(xhci, "The device to be reset with slot ID %u does "
> + "not exist. Re-allocate the device\n", slot_id);
> + ret = xhci_alloc_dev(hcd, udev);
> + if (ret == 1)
> + return 0;
> + else
> + return -EINVAL;
> + }
> +
> + if (virt_dev->udev != udev) {
> + /* If the virt_dev and the udev does not match, this virt_dev
> + * may belong to another udev.
> + * Re-allocate the device.
> + */
> + xhci_dbg(xhci, "The device to be reset with slot ID %u does "
> + "not match the udev. Re-allocate the device\n",
> + slot_id);
> + ret = xhci_alloc_dev(hcd, udev);
> + if (ret == 1)
> + return 0;
> + else
> + return -EINVAL;
> + }
>
> xhci_dbg(xhci, "Resetting device with slot ID %u\n", slot_id);
> /* Allocate the command structure that holds the struct completion.
> @@ -2024,6 +2054,7 @@ int xhci_reset_device(struct usb_hcd *hcd, struct usb_device *udev)
> goto command_cleanup;
> case COMP_SUCCESS:
> xhci_dbg(xhci, "Successful reset device command.\n");
> + virt_dev->configured = false;
> break;
> default:
> if (xhci_is_vendor_info_code(xhci, ret))
> @@ -2177,7 +2208,7 @@ int xhci_address_device(struct usb_hcd *hcd, struct usb_device *udev)
> virt_dev = xhci->devs[udev->slot_id];
>
> /* If this is a Set Address to an unconfigured device, setup ep 0 */
> - if (!udev->config)
> + if (!udev->config || !virt_dev->configured)
> xhci_setup_addressable_virt_dev(xhci, udev);
> else
> xhci_copy_ep0_dequeue_into_input_ctx(xhci, udev);
> diff --git a/drivers/usb/host/xhci.h b/drivers/usb/host/xhci.h
> index f03f140..6ab0a52 100644
> --- a/drivers/usb/host/xhci.h
> +++ b/drivers/usb/host/xhci.h
> @@ -746,6 +746,8 @@ struct xhci_virt_device {
> /* Rings saved to ensure old alt settings can be re-instated */
> struct xhci_ring **ring_cache;
> int num_rings_cached;
> + /* Indicate whether the device is configured */
> + bool configured;
> #define XHCI_MAX_RINGS_CACHED 31
> struct xhci_virt_ep eps[31];
> struct completion cmd_completion;
> @@ -1389,7 +1391,7 @@ int xhci_urb_dequeue(struct usb_hcd *hcd, struct urb *urb, int status);
> int xhci_add_endpoint(struct usb_hcd *hcd, struct usb_device *udev, struct usb_host_endpoint *ep);
> int xhci_drop_endpoint(struct usb_hcd *hcd, struct usb_device *udev, struct usb_host_endpoint *ep);
> void xhci_endpoint_reset(struct usb_hcd *hcd, struct usb_host_endpoint *ep);
> -int xhci_reset_device(struct usb_hcd *hcd, struct usb_device *udev);
> +int xhci_discover_or_reset_device(struct usb_hcd *hcd, struct usb_device *udev);
> int xhci_check_bandwidth(struct usb_hcd *hcd, struct usb_device *udev);
> void xhci_reset_bandwidth(struct usb_hcd *hcd, struct usb_device *udev);
>
> --
> 1.7.0.4
>
>
>
--
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
