Re: [PATCH 2/2] xhci: fix last valid endpoint when dropping an endpoint

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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




[Index of Archives]     [Linux Media]     [Linux Input]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [Old Linux USB Devel Archive]

  Powered by Linux