Hi Sarah,
On 10/10/2013 09:25 PM, Sarah Sharp wrote:
Hi Xenia,
On Mon, Oct 07, 2013 at 06:52:39PM +0300, Xenia Ragiadakou wrote:
The previous patch on the endpoint reset uses the already implemented function
xhci_drop_endpoint() to reduce code duplication. However, the way that xhci
updates the last valid endpoint in the Input Slot Context, when an endpoint
is dropped, can lead to incosistent value for the last valid endpoint.
That can happen when the endpoint to be reset has index smaller than the
current last valid endpoint. In that case the last valid endpoint will end up
being updated with a smaller value and if there are valid endpoints with index
higher than the index of the reset endpoint, the xHC will consider them as
invalid and the transactions to these endpoints will break.
This patch updates the last valid endpoint with the index of the first not
disabled endpoint, starting from the current last valid endpoint and skipping
the dropped endpoint.
I notice that in section 4.6.6 of the 08/2012 errata to the xHCI 1.0
spec, that when the host executes the Configure Endpoint command, it
must:
"Set the Context Entries field in the Output Slot Context to the index
of the last valid Endpoint Context in its Output Device Context
structure, which shall be greater to or equal to the value of the
Context Entries field in the Input Slot Context."
There was a discussion about a similar patch to this one in June:
http://marc.info/?l=linux-usb&m=137158978503741&w=2
The end result was that I didn't take the patch, and VMWare fixed their
virtual xHCI host.
Hmmm, I was not aware of that patch.
Is this patch necessary for your first patch to work?
No, it is not necessary that's why before i send this patchset i decided
to send it as second and not first.
The reason i send it is to cover a possible failure case because I have
not found a satisfying way to test the reset endpoint patch exhaustively
under all possible contexts.
What happens if you don't have this patch applied? Did you actually experience the
transactions to those endpoints breaking, or do you just theorize that
it could happen?
Sarah Sharp
While i was working at this fix i met some problems that I thought at
the moment that they were due to the fact that Context Entries were
updated with a value smaller than the actual last valid endpoint and at
that point i changed the implementation of last valid endpoint update.
But since then I had changed the code so maybe they were due to some
other bug. When later I tested my last version (without the last valid
endpoint fix) I have not met problems but I still considered this patch
necessary because I was theorizing that without it something will break.
The main reasons that made me come to this conclusion was the fact that
Context Entries by being updated with a value other than the last valid
endpoint will be inconsistent with the definition in xhci spec (revision
1.0 5/21/10) "Context Entries. This field identifies the index of the
last valid Endpoint Context within this Device Context structure." and
that in 6.4.3.8 Stop Endpoint Command TRB field definitions for Endpoint
ID field it states that "Valid values are ‘1’ to Slot Context Context
Entries", from which i hypothesize that when Context Entries are set
with a smaller value a subsequent Stop Command to an endpoint with
higher index will fail.
I compile now to see if that actually can happen. However, the best
person to answer this question is an xHC architect. Will the xHC take
under consideration the value in Context Entries field of the Output
Slot Context only when a Configure Endpoint command is issued or also
for other commands in order to check if the endpoint affected by the
command is a valid endpoint?
The fact is that if somebody attempts to reset an endpoint with index
smaller than the actual last valid endpoint in Device Context, without
that fix the Context Entries field of the Output Slot Context will be
set to value lower than the actual last valid endpoint.
I don't know if I made clear the reasons of my confusion that made me
consider this fix necessary.
regards,
ksenia
Signed-off-by: Xenia Ragiadakou <burzalodowa@xxxxxxxxx>
---
drivers/usb/host/xhci.c | 22 +++++++++++++++-------
1 file changed, 15 insertions(+), 7 deletions(-)
diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
index e6300b5..97670f5 100644
--- a/drivers/usb/host/xhci.c
+++ b/drivers/usb/host/xhci.c
@@ -1547,7 +1547,7 @@ int xhci_drop_endpoint(struct usb_hcd *hcd, struct usb_device *udev,
struct xhci_container_ctx *in_ctx, *out_ctx;
struct xhci_input_control_ctx *ctrl_ctx;
struct xhci_slot_ctx *slot_ctx;
- unsigned int last_ctx;
+ unsigned int last_ep_index;
unsigned int ep_index;
struct xhci_ep_ctx *ep_ctx;
u32 drop_flag;
@@ -1598,13 +1598,21 @@ int xhci_drop_endpoint(struct usb_hcd *hcd, struct usb_device *udev,
ctrl_ctx->add_flags &= cpu_to_le32(~drop_flag);
new_add_flags = le32_to_cpu(ctrl_ctx->add_flags);
- last_ctx = xhci_last_valid_endpoint(le32_to_cpu(ctrl_ctx->add_flags));
+ slot_ctx = xhci_get_slot_ctx(xhci, out_ctx);
+ last_ep_index = LAST_CTX_TO_EP_NUM(le32_to_cpu(slot_ctx->dev_info));
slot_ctx = xhci_get_slot_ctx(xhci, in_ctx);
- /* Update the last valid endpoint context, if we deleted the last one */
- if ((le32_to_cpu(slot_ctx->dev_info) & LAST_CTX_MASK) >
- LAST_CTX(last_ctx)) {
- slot_ctx->dev_info &= cpu_to_le32(~LAST_CTX_MASK);
- slot_ctx->dev_info |= cpu_to_le32(LAST_CTX(last_ctx));
+ for (; last_ep_index >= 0; --last_ep_index) {
+ /* skip the dropped endpoint's index */
+ if (last_ep_index == ep_index)
+ continue;
+ ep_ctx = xhci_get_ep_ctx(xhci, out_ctx, last_ep_index);
+ if ((ep_ctx->ep_info & cpu_to_le32(EP_STATE_MASK)) !=
+ cpu_to_le32(EP_STATE_DISABLED)) {
+ slot_ctx->dev_info &= cpu_to_le32(~LAST_CTX_MASK);
+ slot_ctx->dev_info |=
+ cpu_to_le32(LAST_CTX(last_ep_index + 1));
+ break;
+ }
}
new_slot_info = le32_to_cpu(slot_ctx->dev_info);
--
1.8.3.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
