xhci: Compute last_ctx from complete set of configured endpoints.
Subsystems (2)
PATCH 1/1 xhci: Compute last_ctx from complete set of configured endpoints.
The context entries field of the slot context must be set to one more than the highest endpoint index currently active. The previous logic only included the set of endpoints currently being added, meaning that if an endpoint where dropped then the field would be reset to 1, deactivating all configured endpoints.
The xHCI spec is decidedly unclear on whether this field includes all configured endpoints or only those being modified by a configure endpoint command. My interpretation is the former as when the xHC writes changes into the output device context array it must know how large it is. It does not make sense for this field to only refer to the input context.
--- drivers/usb/host/xhci.c | 52 +++++++++++++++++-------------------------------- 1 file changed, 18 insertions(+), 34 deletions(-)
drivers/usb/host/xhci.c +18 -34
@@ -1571,12 +1571,10 @@ int xhci_drop_endpoint(struct usb_hcd *hcd, struct usb_device *udev,struct xhci_hcd *xhci;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 ep_index;struct xhci_ep_ctx *ep_ctx;u32 drop_flag; -u32 new_add_flags, new_drop_flags, new_slot_info; +u32 new_add_flags, new_drop_flags;int ret; ret =xhci_check_args (hcd, udev, ep,1 , true, __func__);
@@ -1617,24 +1615,13 @@ 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, 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)); - } - new_slot_info =le32_to_cpu (slot_ctx->dev_info); -xhci_endpoint_zero (xhci, xhci->devs[udev->slot_id], ep); -xhci_dbg (xhci,"drop ep 0x%x, slot id %d, new drop flags = %#x, new add flags = %#x, new slot info = %#x\n" , +xhci_dbg (xhci,"drop ep 0x%x, slot id %d, new drop flags = %#x, new add flags = %#x\n" , (unsigned int ) ep->desc.bEndpointAddress, udev->slot_id, (unsigned int ) new_drop_flags, - (unsigned int ) new_add_flags, - (unsigned int ) new_slot_info); + (unsigned int ) new_add_flags);return 0 ; }
@@ -1657,11 +1644,9 @@ int xhci_add_endpoint(struct usb_hcd *hcd, struct usb_device *udev,struct xhci_hcd *xhci;struct xhci_container_ctx *in_ctx, *out_ctx;unsigned int ep_index; -struct xhci_slot_ctx *slot_ctx;struct xhci_input_control_ctx *ctrl_ctx;u32 added_ctxs; -unsigned int last_ctx; -u32 new_add_flags, new_drop_flags, new_slot_info; +u32 new_add_flags, new_drop_flags;struct xhci_virt_device *virt_dev;int ret =0 ;
@@ -1676,7 +1661,6 @@ int xhci_add_endpoint(struct usb_hcd *hcd, struct usb_device *udev,return -ENODEV ; added_ctxs =xhci_get_endpoint_flag (&ep->desc); - last_ctx =xhci_last_valid_endpoint (added_ctxs);if (added_ctxs ==SLOT_FLAG || added_ctxs ==EP0_FLAG ) {/* FIXME when we have to issue an evaluate endpoint command to * deal with ep0 max packet size changing once we get the
@@ -1737,24 +1721,14 @@ int xhci_add_endpoint(struct usb_hcd *hcd, struct usb_device *udev,*/ new_drop_flags =le32_to_cpu (ctrl_ctx->drop_flags); - slot_ctx =xhci_get_slot_ctx (xhci, in_ctx); -/* Update the last valid endpoint context, if we just added one past */ -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)); - } - new_slot_info =le32_to_cpu (slot_ctx->dev_info); -/* Store the usb_device pointer for later use */ ep->hcpriv = udev; -xhci_dbg (xhci,"add ep 0x%x, slot id %d, new drop flags = %#x, new add flags = %#x, new slot info = %#x\n" , +xhci_dbg (xhci,"add ep 0x%x, slot id %d, new drop flags = %#x, new add flags = %#x\n" , (unsigned int ) ep->desc.bEndpointAddress, udev->slot_id, (unsigned int ) new_drop_flags, - (unsigned int ) new_add_flags, - (unsigned int ) new_slot_info); + (unsigned int ) new_add_flags);return 0 ; }
@@ -2698,10 +2672,20 @@ int xhci_check_bandwidth(struct usb_hcd *hcd, struct usb_device *udev) ctrl_ctx->drop_flags == 0)return 0 ; -xhci_dbg (xhci,"New Input Control Context:\n" ); +/* Update the last valid endpoint context. */ slot_ctx =xhci_get_slot_ctx (xhci, virt_dev->in_ctx); +for (i =ARRAY_SIZE (virt_dev->eps) -1 ; i >=0 ; --i) { +if ((virt_dev->eps[i].ring && + !(le32_to_cpu (ctrl_ctx->drop_flags) & (1 << (i +1 )))) || + (le32_to_cpu (ctrl_ctx->add_flags) & (1 << (i +1 )))) +break ; + } + slot_ctx->dev_info &=cpu_to_le32 (~LAST_CTX_MASK ); + slot_ctx->dev_info |=cpu_to_le32 (LAST_CTX (i +1 )); + +xhci_dbg (xhci,"New Input Control Context:\n" );xhci_dbg_ctx (xhci, virt_dev->in_ctx, -LAST_CTX_TO_EP_NUM (le32_to_cpu (slot_ctx->dev_info))); +xhci_last_valid_endpoint (ctrl_ctx->add_flags)); ret =xhci_configure_endpoint (xhci, udev,NULL , false, false);
Signature · 1 line
1.8.1.5
Quoted 6 lines · Reilly Grant
The context entries field of the slot context must be set to one more than the highest endpoint index currently active. The previous logic only included the set of endpoints currently being added, meaning that if an endpoint where dropped then the field would be reset to 1, deactivating all configured endpoints.
Quoted 6 lines
The xHCI spec is decidedly unclear on whether this field includes all configured endpoints or only those being modified by a configure endpoint command. My interpretation is the former as when the xHC writes changes into the output device context array it must know how large it is. It does not make sense for this field to only refer to the input context.
My interpretation of the spec is that the last valid context index in the input context was supposed to be used as the last endpoint context that is valid for this command, not for the output context.
Looking at the spec revision from 08/2012, there is an amendment to section 4.6.6.1 that makes this even more clear. The part of the section that talks about what the hardware must do in order to evaluate each endpoint and update the output context says:
"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."
That makes it fairly clear that software needs to set the last valid endpoint context index based on what endpoints are added or dropped in the input context, not which endpoints are still valid in the output context.
So, no, I can't accept this patch. If this fixes a real problem in some hardware, we'll add a quirk for that hardware.
Sarah Sharp
Quoted 122 lines
--- drivers/usb/host/xhci.c | 52 +++++++++++++++++-------------------------------- 1 file changed, 18 insertions(+), 34 deletions(-)drivers/usb/host/xhci.c +18
-34@@ -1571,12 +1571,10 @@ int xhci_drop_endpoint(struct usb_hcd *hcd, struct usb_device *udev,struct xhci_hcd *xhci;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 ep_index;struct xhci_ep_ctx *ep_ctx;u32 drop_flag; -u32 new_add_flags, new_drop_flags, new_slot_info; +u32 new_add_flags, new_drop_flags;int ret; ret =xhci_check_args (hcd, udev, ep,1 , true, __func__);@@ -1617,24 +1615,13 @@ 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, 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)); - } - new_slot_info =le32_to_cpu (slot_ctx->dev_info); -xhci_endpoint_zero (xhci, xhci->devs[udev->slot_id], ep); -xhci_dbg (xhci,"drop ep 0x%x, slot id %d, new drop flags = %#x, new add flags = %#x, new slot info = %#x\n" , +xhci_dbg (xhci,"drop ep 0x%x, slot id %d, new drop flags = %#x, new add flags = %#x\n" , (unsigned int ) ep->desc.bEndpointAddress, udev->slot_id, (unsigned int ) new_drop_flags, - (unsigned int ) new_add_flags, - (unsigned int ) new_slot_info); + (unsigned int ) new_add_flags);return 0 ; }@@ -1657,11 +1644,9 @@ int xhci_add_endpoint(struct usb_hcd *hcd, struct usb_device *udev,struct xhci_hcd *xhci;struct xhci_container_ctx *in_ctx, *out_ctx;unsigned int ep_index; -struct xhci_slot_ctx *slot_ctx;struct xhci_input_control_ctx *ctrl_ctx;u32 added_ctxs; -unsigned int last_ctx; -u32 new_add_flags, new_drop_flags, new_slot_info; +u32 new_add_flags, new_drop_flags;struct xhci_virt_device *virt_dev;int ret =0 ;@@ -1676,7 +1661,6 @@ int xhci_add_endpoint(struct usb_hcd *hcd, struct usb_device *udev,return -ENODEV ; added_ctxs =xhci_get_endpoint_flag (&ep->desc); - last_ctx =xhci_last_valid_endpoint (added_ctxs);if (added_ctxs ==SLOT_FLAG || added_ctxs ==EP0_FLAG ) {/* FIXME when we have to issue an evaluate endpoint command to * deal with ep0 max packet size changing once we get the @@ -1737,24 +1721,14 @@ int xhci_add_endpoint(struct usb_hcd *hcd, struct usb_device *udev,*/ new_drop_flags =le32_to_cpu (ctrl_ctx->drop_flags); - slot_ctx =xhci_get_slot_ctx (xhci, in_ctx); -/* Update the last valid endpoint context, if we just added one past */ -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)); - } - new_slot_info =le32_to_cpu (slot_ctx->dev_info); -/* Store the usb_device pointer for later use */ ep->hcpriv = udev; -xhci_dbg (xhci,"add ep 0x%x, slot id %d, new drop flags = %#x, new add flags = %#x, new slot info = %#x\n" , +xhci_dbg (xhci,"add ep 0x%x, slot id %d, new drop flags = %#x, new add flags = %#x\n" , (unsigned int ) ep->desc.bEndpointAddress, udev->slot_id, (unsigned int ) new_drop_flags, - (unsigned int ) new_add_flags, - (unsigned int ) new_slot_info); + (unsigned int ) new_add_flags);return 0 ; }@@ -2698,10 +2672,20 @@ int xhci_check_bandwidth(struct usb_hcd *hcd, struct usb_device *udev) ctrl_ctx->drop_flags == 0)return 0 ; -xhci_dbg (xhci,"New Input Control Context:\n" ); +/* Update the last valid endpoint context. */ slot_ctx =xhci_get_slot_ctx (xhci, virt_dev->in_ctx); +for (i =ARRAY_SIZE (virt_dev->eps) -1 ; i >=0 ; --i) { +if ((virt_dev->eps[i].ring && + !(le32_to_cpu (ctrl_ctx->drop_flags) & (1 << (i +1 )))) || + (le32_to_cpu (ctrl_ctx->add_flags) & (1 << (i +1 )))) +break ; + } + slot_ctx->dev_info &=cpu_to_le32 (~LAST_CTX_MASK ); + slot_ctx->dev_info |=cpu_to_le32 (LAST_CTX (i +1 )); + +xhci_dbg (xhci,"New Input Control Context:\n" );xhci_dbg_ctx (xhci, virt_dev->in_ctx, -LAST_CTX_TO_EP_NUM (le32_to_cpu (slot_ctx->dev_info))); +xhci_last_valid_endpoint (ctrl_ctx->add_flags)); ret =xhci_configure_endpoint (xhci, udev,NULL , false, false);Signature · 1 line
1.8.1.5
Quoted 28 lines · Sarah Sharp
Quoted 6 lines · Reilly Grant
The context entries field of the slot context must be set to one more than the highest endpoint index currently active. The previous logic only included the set of endpoints currently being added, meaning that if an endpoint where dropped then the field would be reset to 1, deactivating all configured endpoints.
Quoted 6 lines
The xHCI spec is decidedly unclear on whether this field includes all configured endpoints or only those being modified by a configure endpoint command. My interpretation is the former as when the xHC writes changes into the output device context array it must know how large it is. It does not make sense for this field to only refer to the input context.
My interpretation of the spec is that the last valid context index in the input context was supposed to be used as the last endpoint context that is valid for this command, not for the output context.
Looking at the spec revision from 08/2012, there is an amendment to section 4.6.6.1 that makes this even more clear. The part of the section that talks about what the hardware must do in order to evaluate each endpoint and update the output context says:
"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."
Quoted 7 lines
That makes it fairly clear that software needs to set the last valid endpoint context index based on what endpoints are added or dropped in the input context, not which endpoints are still valid in the output context.
So, no, I can't accept this patch. If this fixes a real problem in some hardware, we'll add a quirk for that hardware.
Thank you for the clarification. I was not aware of this addendum. Could you forward me a link to it? The last revision I can find on the Intel web site is from 05/2010. This fixed a problem on VMware's virtual hardware. Assuming changing behavior will not affect any other systems I will fix it based on this information.
Quoted 134 lines
Quoted 133 lines
--- drivers/usb/host/xhci.c | 52 +++++++++++++++++-------------------------------- 1 file changed, 18 insertions(+), 34 deletions(-)drivers/usb/host/xhci.c +1
-3@@ -1571,12 +1571,10 @@ int xhci_drop_endpoint(struct usb_hcd *hcd, structusb_device *udev, struct xhci_hcd *xhci;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 ep_index;struct xhci_ep_ctx *ep_ctx;u32 drop_flag; -u32 new_add_flags, new_drop_flags, new_slot_info; +u32 new_add_flags, new_drop_flags;int ret;drivers/usb/host/xhci.c +2
-13ret =xhci_check_args (hcd, udev, ep,1 , true, __func__);@@ -1617,24 +1615,13 @@ 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, 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)); - } - new_slot_info =le32_to_cpu (slot_ctx->dev_info); -xhci_endpoint_zero (xhci, xhci->devs[udev->slot_id], ep); - xhci_dbg(xhci, "drop ep 0x%x, slot id %d, new drop flags = %#x, new add flags = %#x, new slot info = %#x\n", +xhci_dbg (xhci,"drop ep 0x%x, slot id %d, new drop flags = %#x, new add flags = %#x\n", (unsigned int ) ep->desc.bEndpointAddress, udev->slot_id, (unsigned int ) new_drop_flags, - (unsigned int ) new_add_flags, - (unsigned int) new_slot_info); + (unsigned int ) new_add_flags);drivers/usb/host/xhci.c +1
-4return 0 ; }@@ -1657,11 +1644,9 @@ int xhci_add_endpoint(struct usb_hcd *hcd, structusb_device *udev, struct xhci_hcd *xhci;struct xhci_container_ctx *in_ctx, *out_ctx;unsigned int ep_index; -struct xhci_slot_ctx *slot_ctx;struct xhci_input_control_ctx *ctrl_ctx;u32 added_ctxs; -unsigned int last_ctx; -u32 new_add_flags, new_drop_flags, new_slot_info; +u32 new_add_flags, new_drop_flags;struct xhci_virt_device *virt_dev;int ret =0 ;@@ -1676,7 +1661,6 @@ int xhci_add_endpoint(struct usb_hcd *hcd, struct usb_device *udev, return -ENODEV ; added_ctxs =xhci_get_endpoint_flag (&ep->desc); - last_ctx =xhci_last_valid_endpoint (added_ctxs);if (added_ctxs ==SLOT_FLAG || added_ctxs ==EP0_FLAG ) {/* FIXME when we have to issue an evaluate endpoint command to drivers/usb/host/xhci.c +2
-12* deal with ep0 max packet size changing once we get the @@ -1737,24 +1721,14 @@ int xhci_add_endpoint(struct usb_hcd *hcd, structusb_device *udev, */ new_drop_flags =le32_to_cpu (ctrl_ctx->drop_flags); - slot_ctx =xhci_get_slot_ctx (xhci, in_ctx); -/* Update the last valid endpoint context, if we just added one past */ -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)); - } - new_slot_info =le32_to_cpu (slot_ctx->dev_info); -/* Store the usb_device pointer for later use */ ep->hcpriv = udev; - xhci_dbg(xhci, "add ep 0x%x, slot id %d, new drop flags = %#x, new add flags = %#x, new slot info = %#x\n", +xhci_dbg (xhci,"add ep 0x%x, slot id %d, new drop flags = %#x, new add flags = %#x\n", (unsigned int ) ep->desc.bEndpointAddress, udev->slot_id, (unsigned int ) new_drop_flags, - (unsigned int ) new_add_flags, - (unsigned int) new_slot_info); + (unsigned int ) new_add_flags);drivers/usb/host/xhci.c +12
-2return 0 ; }@@ -2698,10 +2672,20 @@ int xhci_check_bandwidth(struct usb_hcd *hcd, struct usb_device *udev) ctrl_ctx->drop_flags == 0) return 0; -xhci_dbg (xhci,"New Input Control Context:\n" ); +/* Update the last valid endpoint context. */ slot_ctx =xhci_get_slot_ctx (xhci, virt_dev->in_ctx); +for (i =ARRAY_SIZE (virt_dev->eps) -1 ; i >=0 ; --i) { +if ((virt_dev->eps[i].ring && + !(le32_to_cpu (ctrl_ctx->drop_flags) & (1 << (i +1 )))) || + (le32_to_cpu (ctrl_ctx->add_flags) & (1 << (i +1 )))) +break ; + } + slot_ctx->dev_info &=cpu_to_le32 (~LAST_CTX_MASK ); + slot_ctx->dev_info |=cpu_to_le32 (LAST_CTX (i +1 )); + +xhci_dbg (xhci,"New Input Control Context:\n" );xhci_dbg_ctx (xhci, virt_dev->in_ctx, -LAST_CTX_TO_EP_NUM (le32_to_cpu (slot_ctx->dev_info))); +xhci_last_valid_endpoint (ctrl_ctx->add_flags)); ret = xhci_configure_endpoint(xhci, udev, NULL,false, false);
--
1.8.1.5
Quoted 9 lines · Reilly Grant
Quoted 4 lines · Sarah Sharp
On Tue, Jun 18, 2013 at 02:09:13PM -0700, Reilly Grant wrote:
So, no, I can't accept this patch. If this fixes a real problem in some hardware, we'll add a quirk for that hardware.
Thank you for the clarification. I was not aware of this addendum. Could you forward me a link to it? The last revision I can find on the Intel web site is from 05/2010.
The xHCI architect hasn't done an official release on the Intel site for a while now. I think he's working on a bigger 1.1 release that includes all the errata over the past couple years.
I think you can get the latest revision by emailing [email protected]. If you don't hear back within a week or so, email me back and I'll send you a PDF.
Quoted 3 lines
This fixed a problem on VMware's virtual hardware. Assuming changing behavior will not affect any other systems I will fix it based on this information.
Yes, please fix this in VMware's virtual hardware.
Thanks,
Sarah Sharp