On 09.06.2017 12:44, Shyam Sundar S K wrote:
On 6/7/2017 4:45 PM, Mathias Nyman wrote:
Hi
The internal variable is just what xhci spec recommends as it says the output context is not immediately
updated for example at endpoint doorbell ring. It's to make sure we don't read stale values from the output context.
The race Alan refers to is a different case.
The endpoint might be halted just before the stop endpoint command is handled by hardware, there is no way
of tracking this from output contexts or local variables.
Working controllers will just give a context state error if we try to stop a halted endpoint.
Agreed. In case of the linux xhci driver, two times stop EP commands are queued up.
one within the loop: xhci_queue_stop_endpoint(xhci, command, slot_id, i, suspend);
and another out of the loop: xhci_queue_stop_endpoint(xhci, cmd, slot_id, 0, suspend);
I could not comprehend what is the actual need to have two stop endpoint command queued up.
within the loop stop active endpoints from LAST_EP_INDEX to 1 (30,29,28,27...3,2,1).
We don't wait for these stop endpoint commands to complete.
After the loop we stop endpoint 0 (default control endpoint) and wait for its completion.
We don't stop the same endpoint twice.
In our case, when we send the first stop endpoint command it is returning with Context State Error
and for the next stop EP HW additionally marks EP to Flow control state in HW which is RTL bug.
I did some experiments to see the impact of not queuing the first stop EP command i.e.
xhci_queue_stop_endpoint(xhci, command, slot_id, i, suspend); the one which is inside the loop
Looks like not queuing this command does not have any impact on the functionality on the connected hub.
So, based on this; I have pushed the v2 patch which guards the first stop EP command.
Also, for the first stop EP command, while allocation we do not set the 'allocate_completion' flag to true.
command = xhci_alloc_command(xhci, false, false, GFP_NOWAIT);
So, ideally we cannot check its completion state with command->status.
Atleast for this SNPS IP issue, we need to prevent this first stop EP command, either by checking the
ep_state or by adding a quirk specific to this revision of IP from SNPS so that it does not hit the internal
RTL bug.
Kindly let me know your thoughts.
I think it's valid to check if the endpoint is already stopped from the endpoint context, and not
queue a stop endpoint command for a already stopped endpoint. Especially as your hardware can't handle it.
That code makes sense.
I also think you got a bit confused what the current code does. It does not stop same endpoint twice.
It stops endpoints 30-1 in a loop, and then endpoint 0 after the loop.
Then I think yo need to consider the race Alan was talking about.
Let me try to clarify that race case again.
Even after checking the endpoint state from endpoint context the following could happen:
-- xhci driver, in xhci_stop_device() ---
1. driver checks endpoint x context shows endpoint is running
2. driver queues a stop endpoint command for endpoint x on command ring
-- xhci hardware --
3. endpoint x is till running,
4. xhci hardware gets a STALL packet on endpoint x from the device.
5. xhci hardware sets endpoint x to HALT state
6. xhci hardware takes stop endpoint command from command ring and start executing it
7. xhci hardware tries to stop endpoint x which is in HALTED state
-> possible RTL bug!
After all this we need to take into account that xhci spec say that there can be
be a small delay before actual endpoint state is updated to the endpoint context.
The spec recommends that the driver keeps track of latest endpoint state changes.
Like when ringing a doorbell it will start the endpoint, but endpoint context state is
updated to running with a small delay
-Mathias
--
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
