LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Mathias Nyman <mathias.nyman@linux.intel.com>
To: Alan Stern <stern@rowland.harvard.edu>
Cc: "Jörg Otte" <jrg.otte@gmail.com>,
	"Linux Kernel Mailing List" <linux-kernel@vger.kernel.org>,
	"Greg Kroah-Hartman" <gregkh@linuxfoundation.org>,
	"USB list" <linux-usb@vger.kernel.org>,
	"Linus Torvalds" <torvalds@linux-foundation.org>,
	"Mauro Carvalho Chehab" <mchehab@osg.samsung.com>
Subject: Re: [V4.0.0-rc3] Xhci Regression: ERROR Transfer event TRB DMA ptr not part of current TD
Date: Wed, 11 Mar 2015 11:00:39 +0200	[thread overview]
Message-ID: <55000437.9040004@linux.intel.com> (raw)
In-Reply-To: <Pine.LNX.4.44L0.1503101439040.1461-100000@iolanthe.rowland.org>

On 10.03.2015 20:49, Alan Stern wrote:
> On Tue, 10 Mar 2015, Mathias Nyman wrote:
> 
>>> Mathias:
>>>
>>> Your patch description says this:
>>>
>>>> The endpoint might already processesed some TRBs on the endpiont ring
>>>> before we soft reset the endpoint.
>>>> Make sure we set the dequeue pointer to where we were befere soft reset
>>>
>>> However, if a driver tries to issue an endpoint reset while there are
>>> still some URBs queued, it is a bug.  Host controller drivers shouldn't
>>> have to worry about this -- xhci_endpoint_reset() should simply return 
>>> an error if the endpoint ring isn't empty.
>>>
>>> I suppose we should check for this in the USB core.  I'll write a patch
>>> and CC: you.
>>>
>>> Alan Stern
>>>
>>
>> It's possible that there's something in usb core as well, 
>> but I think the following was what happened:
>>
>> 1. First a normal configure endpoint command is issued, it sets endpoint dequeue pointer
>>    to xxx400 = start of ring segment
>> 2. two urbs get queued -> two TDs put on endpoint ring.
>> 3. xhci executes those, ring is in running (idle) state. sw dequeue at xxx430, No TDs queued.
>>    Endpoint dequeue pointer is not written to the endpoint output context as the ring is still 
>>    in running state (even if idle, not advancing with no TDs queued) it still shows xxx400
>> 4. -> something happends, xhci_endpoint_reset() is called, we do a new configure endpoint
>>    to 'soft reset' the endpiont, but we copy the dequeue pointer from the old endpoint
>>    output context to the configure endpoint input context, which re-initializes the old
>>    dequeue xxx400 pointer to xhci hardware, and it starts executing the old TDs from the ring.
> 
> Obviously that's bad.
> 
> But don't you have to stop the endpoint ring in order to configure it?  
> When you stop the ring, doesn't the controller store the correct
> current value of the dequeue pointer somewhere?
> 

Normally we stop the endpoint before configuring it, but in this case
the endpoint is already configured, and we don't really want to change the configuration,
we just want to reset the toggle so that it's in sync with the device.

As I understand the xhci specs allows us to issue a configure endpoint command 
for a running endpoint as long as it's empty. 

xhci 1.0  4.6.6 Configure Endpoint:  
"An endpoint shall be in the Stopped state or if in the Running state shall be “idle” (e.g. no USB
Transactions are in progress, the Transfer Ring is empty, and software has processed all
outstanding events for the Transfer Ring) if its Drop Context flag is set. If this condition is not met
undefined behavior may occur.

But the output context we copy is from last time the endpoint was stopped or configured.
So we need to update the dequeue pointer to the one we have in the driver, I need to check
if the other old fields in the output context can cause any issues as well.

-Mathias 

  parent reply	other threads:[~2015-03-11  8:59 UTC|newest]

Thread overview: 15+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2015-03-10  9:40 Jörg Otte
2015-03-10 13:06 ` Mathias Nyman
2015-03-10 14:03   ` Jörg Otte
2015-03-10 15:36     ` Jörg Otte
2015-03-10 17:04       ` Mathias Nyman
2015-03-10 17:29         ` Alan Stern
2015-03-10 18:21           ` Mathias Nyman
2015-03-10 18:49             ` Alan Stern
2015-03-11  7:04               ` Lu, Baolu
2015-03-11 14:03                 ` Alan Stern
2015-03-11 15:48                   ` Mathias Nyman
2015-03-11  9:00               ` Mathias Nyman [this message]
2015-03-11 11:01         ` Jörg Otte
2015-03-11 16:16           ` Jörg Otte
2015-03-12  8:32             ` Mathias Nyman

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=55000437.9040004@linux.intel.com \
    --to=mathias.nyman@linux.intel.com \
    --cc=gregkh@linuxfoundation.org \
    --cc=jrg.otte@gmail.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-usb@vger.kernel.org \
    --cc=mchehab@osg.samsung.com \
    --cc=stern@rowland.harvard.edu \
    --cc=torvalds@linux-foundation.org \
    --subject='Re: [V4.0.0-rc3] Xhci Regression: ERROR Transfer event TRB DMA ptr not part of current TD' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).