[PATCH] xhci-hub.c: handle command_trb that may be link TRB

[PATCH] xhci-hub.c: handle command_trb that may be link TRB

[Xiao Jin]

From: xiao jin <jin.xiao@intel.com>
Date: Wed, 9 Oct 2013 09:38:45 +0800
Subject: [PATCH] xhci-hub.c: handle command_trb that may be link TRB

When xhci stop device, it's possible cmd_ring enqueue point to
link TRB after queue the last but one stop endpoint. We must
handle the command_trb point to the next segment trb. Otherwise
xhci stop devie will timeout because command_trb can't match
with cmd_ring dequeue.

The patch is to let command_trb point to the next segment trb if
cmd_ring enqueue point to link TRB.

Signed-off-by: xiao jin <jin.xiao@intel.com>
---
 drivers/usb/host/xhci-hub.c |    7 +++++++
 1 files changed, 7 insertions(+), 0 deletions(-)

diff --git a/drivers/usb/host/xhci-hub.c b/drivers/usb/host/xhci-hub.c
index 1d35459..4872640 100644
--- a/drivers/usb/host/xhci-hub.c
+++ b/drivers/usb/host/xhci-hub.c
@@ -287,6 +287,13 @@ static int xhci_stop_device(struct xhci_hcd *xhci, int slot_id, int suspend)
 			xhci_queue_stop_endpoint(xhci, slot_id, i, suspend);
 	}
 	cmd->command_trb = xhci->cmd_ring->enqueue;
+	/* Enqueue pointer can be left pointing to the link TRB,
+	 * we must handle that
+	 */
+	if (TRB_TYPE_LINK_LE32(cmd->command_trb->link.control))
+		cmd->command_trb =
+			xhci->cmd_ring->enq_seg->next->trbs;
+
 	list_add_tail(&cmd->cmd_list, &virt_dev->cmd_list);
 	xhci_queue_stop_endpoint(xhci, slot_id, 0, suspend);
 	xhci_ring_cmd_db(xhci);
-- 
1.7.1

Re: [PATCH] xhci-hub.c: handle command_trb that may be link TRB

[Sarah Sharp]

Hi Xiao,

Thanks for taking the time to submit this patch.  Comments below.

On Wed, Oct 09, 2013 at 09:42:36AM +0800, Xiao Jin wrote:
> From: xiao jin <jin.xiao@intel.com>
> Date: Wed, 9 Oct 2013 09:38:45 +0800
> Subject: [PATCH] xhci-hub.c: handle command_trb that may be link TRB

I won't be able to apply your patch  because it has these extra lines in
the body ^^^.  I suspect you used `git format-patch` to produce this,
and then tried to copy-paste it into your mail client?  You can't do
that, because your mail client will probably word-wrap your patch, and
possibly turn tabs into spaces.  You need to use `git send-email`
instead to send your patches.

> When xhci stop device, it's possible cmd_ring enqueue point to
> link TRB after queue the last but one stop endpoint. We must
> handle the command_trb point to the next segment trb. Otherwise
> xhci stop devie will timeout because command_trb can't match
> with cmd_ring dequeue.
> 
> The patch is to let command_trb point to the next segment trb if
> cmd_ring enqueue point to link TRB.
> 
> Signed-off-by: xiao jin <jin.xiao@intel.com>
> ---
>  drivers/usb/host/xhci-hub.c |    7 +++++++
>  1 files changed, 7 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/usb/host/xhci-hub.c b/drivers/usb/host/xhci-hub.c
> index 1d35459..4872640 100644
> --- a/drivers/usb/host/xhci-hub.c
> +++ b/drivers/usb/host/xhci-hub.c
> @@ -287,6 +287,13 @@ static int xhci_stop_device(struct xhci_hcd *xhci, int slot_id, int suspend)
>  			xhci_queue_stop_endpoint(xhci, slot_id, i, suspend);
>  	}
>  	cmd->command_trb = xhci->cmd_ring->enqueue;
> +	/* Enqueue pointer can be left pointing to the link TRB,
> +	 * we must handle that
> +	 */
> +	if (TRB_TYPE_LINK_LE32(cmd->command_trb->link.control))
> +		cmd->command_trb =
> +			xhci->cmd_ring->enq_seg->next->trbs;
> +
>  	list_add_tail(&cmd->cmd_list, &virt_dev->cmd_list);
>  	xhci_queue_stop_endpoint(xhci, slot_id, 0, suspend);
>  	xhci_ring_cmd_db(xhci);

What kernel version or tree is it against?  I ask because there's
already a fix in the 3.12-rc4 kernel for this:

static int xhci_stop_device(struct xhci_hcd *xhci, int slot_id, int suspend)
{
...
        spin_lock_irqsave(&xhci->lock, flags);
        for (i = LAST_EP_INDEX; i > 0; i--) {
                if (virt_dev->eps[i].ring && virt_dev->eps[i].ring->dequeue)
                        xhci_queue_stop_endpoint(xhci, slot_id, i, suspend);
        }
        cmd->command_trb = xhci_find_next_enqueue(xhci->cmd_ring);
...

Where xhci_find_next_enqueue is defined as:

union xhci_trb *xhci_find_next_enqueue(struct xhci_ring *ring)
{
        /* Enqueue pointer can be left pointing to the link TRB,
         * we must handle that
         */
        if (TRB_TYPE_LINK_LE32(ring->enqueue->link.control))
                return ring->enq_seg->next->trbs;
        return ring->enqueue;
}

That was added in commit ec7e43e2d98173483866fe2e4e690143626b659c "xhci:
Ensure a command structure points to the correct trb on the command
ring"  It's in (or should be in soon) the stable kernels as well.

Sarah Sharp

Re: [PATCH] xhci-hub.c: handle command_trb that may be link TRB

[Xiao Jin]

Sarah,

Thanks for the reminding. The kernel base of the patch is k3.10, I
didn't notice k3.12 commit ec7e43e2d98173483866fe2e4e690143626b659c at
that time. I will backport the patch from k3.12 for use.

As you mentioned in another email, we meet with dpm timeout when xhci
suspend, the root cause are what the patch aimed to solve. FYI.

On Wed, 2013-10-09 at 13:18 -0700, Sarah Sharp wrote:
> Hi Xiao,
> 
> Thanks for taking the time to submit this patch.  Comments below.
> 
> On Wed, Oct 09, 2013 at 09:42:36AM +0800, Xiao Jin wrote:
> > From: xiao jin <jin.xiao@intel.com>
> > Date: Wed, 9 Oct 2013 09:38:45 +0800
> > Subject: [PATCH] xhci-hub.c: handle command_trb that may be link TRB
> 
> I won't be able to apply your patch  because it has these extra lines in
> the body ^^^.  I suspect you used `git format-patch` to produce this,
> and then tried to copy-paste it into your mail client?  You can't do
> that, because your mail client will probably word-wrap your patch, and
> possibly turn tabs into spaces.  You need to use `git send-email`
> instead to send your patches.
> 
> > When xhci stop device, it's possible cmd_ring enqueue point to
> > link TRB after queue the last but one stop endpoint. We must
> > handle the command_trb point to the next segment trb. Otherwise
> > xhci stop devie will timeout because command_trb can't match
> > with cmd_ring dequeue.
> > 
> > The patch is to let command_trb point to the next segment trb if
> > cmd_ring enqueue point to link TRB.
> > 
> > Signed-off-by: xiao jin <jin.xiao@intel.com>
> > ---
> >  drivers/usb/host/xhci-hub.c |    7 +++++++
> >  1 files changed, 7 insertions(+), 0 deletions(-)
> > 
> > diff --git a/drivers/usb/host/xhci-hub.c b/drivers/usb/host/xhci-hub.c
> > index 1d35459..4872640 100644
> > --- a/drivers/usb/host/xhci-hub.c
> > +++ b/drivers/usb/host/xhci-hub.c
> > @@ -287,6 +287,13 @@ static int xhci_stop_device(struct xhci_hcd *xhci, int slot_id, int suspend)
> >  			xhci_queue_stop_endpoint(xhci, slot_id, i, suspend);
> >  	}
> >  	cmd->command_trb = xhci->cmd_ring->enqueue;
> > +	/* Enqueue pointer can be left pointing to the link TRB,
> > +	 * we must handle that
> > +	 */
> > +	if (TRB_TYPE_LINK_LE32(cmd->command_trb->link.control))
> > +		cmd->command_trb =
> > +			xhci->cmd_ring->enq_seg->next->trbs;
> > +
> >  	list_add_tail(&cmd->cmd_list, &virt_dev->cmd_list);
> >  	xhci_queue_stop_endpoint(xhci, slot_id, 0, suspend);
> >  	xhci_ring_cmd_db(xhci);
> 
> What kernel version or tree is it against?  I ask because there's
> already a fix in the 3.12-rc4 kernel for this:
> 
> static int xhci_stop_device(struct xhci_hcd *xhci, int slot_id, int suspend)
> {
> ...
>         spin_lock_irqsave(&xhci->lock, flags);
>         for (i = LAST_EP_INDEX; i > 0; i--) {
>                 if (virt_dev->eps[i].ring && virt_dev->eps[i].ring->dequeue)
>                         xhci_queue_stop_endpoint(xhci, slot_id, i, suspend);
>         }
>         cmd->command_trb = xhci_find_next_enqueue(xhci->cmd_ring);
> ...
> 
> Where xhci_find_next_enqueue is defined as:
> 
> union xhci_trb *xhci_find_next_enqueue(struct xhci_ring *ring)
> {
>         /* Enqueue pointer can be left pointing to the link TRB,
>          * we must handle that
>          */
>         if (TRB_TYPE_LINK_LE32(ring->enqueue->link.control))
>                 return ring->enq_seg->next->trbs;
>         return ring->enqueue;
> }
> 
> That was added in commit ec7e43e2d98173483866fe2e4e690143626b659c "xhci:
> Ensure a command structure points to the correct trb on the command
> ring"  It's in (or should be in soon) the stable kernels as well.
> 
> Sarah Sharp