On Thu, May 05, 2011 at 06:14:11PM +0800, Andiry Xu wrote:
> From: Alex He <alex.he@xxxxxxx>
>
> This is a new TRB Completion Code of the xHCI spec 1.0.
> Asserted if the xHC detects a problem with a device that does not allow it to
> be successfully accessed, e.g. due to a device commpliance or compatibility
> problem. This error may be returned by any command or transfer, and is fatal
> as far as the Slot is concerned. Software shall issue a Disable Slot Commmand
> to recover.
How is the upper layer supposed to know the slot is now disabled? It
seems like a policy violation to let the xHCI driver just randomly
disable devices. Shouldn't you let the USB core decide whether it needs
to logically disconnect the device and disable the slot?
You say we can get this error code on any command. What happens if we
get this error for something like a Configure Endpoint command while the
USB core is trying to set a new alternate interface setting. The call
to usb_hcd_alloc_bandwidth() is going to fail, and the device driver
will just assume the set interface command failed. But it can still try
to talk to the control endpoint, which suddenly will start failing.
Meanwhile, the USB core thinks the device still exists, but it's
basically dead to the xHCI driver. Plus, if there's URBs in flight when
we receive this command completion code, the xHCI driver is going to try
to free the endpoint rings when the disable slot command finishes. That
seems very bad.
Maybe it would be better to just return -ENODEV in this case and let the
USB core disable the device?
>
> Signed-off-by: Alex He <alex.he@xxxxxxx>
> Signed-off-by: Andiry Xu <andiry.xu@xxxxxxx>
> ---
> drivers/usb/host/xhci-ring.c | 14 ++++++++++++++
> drivers/usb/host/xhci.h | 2 ++
> 2 files changed, 16 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
> index 85503c1..6862135 100644
> --- a/drivers/usb/host/xhci-ring.c
> +++ b/drivers/usb/host/xhci-ring.c
> @@ -1083,6 +1083,15 @@ static void handle_cmd_completion(struct xhci_hcd *xhci,
> xhci->error_bitmask |= 1 << 5;
> return;
> }
> +
> + if (GET_COMP_CODE(event->status) == COMP_DEV_ERR) {
> + xhci_warn(xhci, "WARN: detect an incompatible device\n");
> + /* Issue a Disable Slot Command */
> + xhci_queue_slot_control(xhci, TRB_DISABLE_SLOT, slot_id);
> + inc_deq(xhci, xhci->cmd_ring, false);
> + return;
> + }
> +
> switch (le32_to_cpu(xhci->cmd_ring->dequeue->generic.field[3])
> & TRB_TYPE_BITMASK) {
> case TRB_TYPE(TRB_ENABLE_SLOT):
> @@ -2077,6 +2086,11 @@ static int handle_tx_event(struct xhci_hcd *xhci,
> TRB_TO_SLOT_ID(le32_to_cpu(event->flags)),
> ep_index);
> goto cleanup;
> + case COMP_DEV_ERR:
> + xhci_warn(xhci, "WARN: detect an incompatible device\n");
> + /* Issue a Disable Slot Command */
> + xhci_queue_slot_control(xhci, TRB_DISABLE_SLOT, slot_id);
> + goto cleanup;
> case COMP_MISSED_INT:
> /*
> * When encounter missed service error, one or more isoc tds
> diff --git a/drivers/usb/host/xhci.h b/drivers/usb/host/xhci.h
> index dd45929..424b3ed 100644
> --- a/drivers/usb/host/xhci.h
> +++ b/drivers/usb/host/xhci.h
> @@ -872,6 +872,8 @@ struct xhci_transfer_event {
> #define COMP_PING_ERR 20
> /* Event Ring is full */
> #define COMP_ER_FULL 21
> +/* Incompatible Device Error */
> +#define COMP_DEV_ERR 22
> /* Missed Service Error - HC couldn't service an isoc ep within interval */
> #define COMP_MISSED_INT 23
> /* Successfully stopped command ring */
> --
> 1.7.1
>
>
>
--
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
