[RFT] xhci: Don't clear hub TT buffer on ep0 protocol stall

Commit Message

Mathias Nyman April 16, 2020, 11:02 a.m. UTC

Hi Jim Lin

The commit: ef513be0a905 ("usb: xhci: Add Clear_TT_Buffer") causes some
regression for other devices. 

https://bugzilla.kernel.org/show_bug.cgi?id=203419
https://bugzilla.kernel.org/show_bug.cgi?id=206897
https://bugzilla.kernel.org/show_bug.cgi?id=207065

Issue is poinpointed to clearing TT buffer for endpoint 0 protocol stalls
which should not be needed.

I have a patch to resolve this but I'm concerned it may cause regression for the
original case your patch solved.

If possible, can you try the patch below and check if the ConferenceCam
device still works with it, and let me know

Thanks 
Mathias

---- >8 ----

xhci: Don't clear hub TT buffer on ep0 protocol stall

The default control endpoint ep0 can return a STALL indicating the
device does not support the control transfer requests. This is called
a protocol stall and does not halt the endpoint.

xHC behaves a bit different. Its internal endpoint state will always
be halted on any stall, even if the device side of the endpiont is not
halted. So we do need to issue the reset endpoint command to clear the
xHC host intenal endpoint halt state, but should not request the HS hub
to clear the TT buffer unless device side of endpoint is halted.

Clearing the hub TT buffer at protocol stall caused ep0 to become
unresponsive for some FS/LS devices behind HS hubs, and class drivers
failed to set the interface due to timeout:

usb 1-2.1: 1:1: usb_set_interface failed (-110)

Signed-off-by: Mathias Nyman <[e-mail]>
---
 drivers/usb/host/xhci-ring.c | 16 +++++++++++-----
 1 file changed, 11 insertions(+), 5 deletions(-)

Comments

Jim Lin April 17, 2020, 12:25 a.m. UTC | #1

I had returned that device to customer.
And won't be able to verify this patch.

Jim

-----Original Message-----
From: Mathias Nyman <[e-mail]> 
Sent: Thursday, April 16, 2020 7:02 PM
To: Jim Lin <[e-mail]>
Cc: USB <[e-mail]>; [e-mail]; [e-mail]
Subject: [RFT PATCH] xhci: Don't clear hub TT buffer on ep0 protocol stall

External email: Use caution opening links or attachments


Hi Jim Lin

The commit: ef513be0a905 ("usb: xhci: Add Clear_TT_Buffer") causes some regression for other devices.

https://bugzilla.kernel.org/show_bug.cgi?id=203419
https://bugzilla.kernel.org/show_bug.cgi?id=206897
https://bugzilla.kernel.org/show_bug.cgi?id=207065

Issue is poinpointed to clearing TT buffer for endpoint 0 protocol stalls which should not be needed.

I have a patch to resolve this but I'm concerned it may cause regression for the original case your patch solved.

If possible, can you try the patch below and check if the ConferenceCam device still works with it, and let me know

Thanks
Mathias

---- >8 ----

xhci: Don't clear hub TT buffer on ep0 protocol stall

The default control endpoint ep0 can return a STALL indicating the device does not support the control transfer requests. This is called a protocol stall and does not halt the endpoint.

xHC behaves a bit different. Its internal endpoint state will always be halted on any stall, even if the device side of the endpiont is not halted. So we do need to issue the reset endpoint command to clear the xHC host intenal endpoint halt state, but should not request the HS hub to clear the TT buffer unless device side of endpoint is halted.

Clearing the hub TT buffer at protocol stall caused ep0 to become unresponsive for some FS/LS devices behind HS hubs, and class drivers failed to set the interface due to timeout:

usb 1-2.1: 1:1: usb_set_interface failed (-110)

Signed-off-by: Mathias Nyman <[e-mail]>
---
 drivers/usb/host/xhci-ring.c | 16 +++++++++++-----
 1 file changed, 11 insertions(+), 5 deletions(-)

diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c index a7f4cd35da55..0fda0c0f4d31 100644
--- a/drivers/usb/host/xhci-ring.c
+++ b/drivers/usb/host/xhci-ring.c
@@ -1876,7 +1876,6 @@ static void xhci_cleanup_halted_endpoint(struct xhci_hcd *xhci,
                ep->ep_state |= EP_HARD_CLEAR_TOGGLE;
                xhci_cleanup_stalled_ring(xhci, slot_id, ep_index, stream_id,
                                          td);
-               xhci_clear_hub_tt_buffer(xhci, td, ep);
        }
        xhci_ring_cmd_db(xhci);
 }
@@ -1997,11 +1996,18 @@ static int finish_td(struct xhci_hcd *xhci, struct xhci_td *td,
        if (trb_comp_code == COMP_STALL_ERROR ||
                xhci_requires_manual_halt_cleanup(xhci, ep_ctx,
                                                trb_comp_code)) {
-               /* Issue a reset endpoint command to clear the host side
-                * halt, followed by a set dequeue command to move the
-                * dequeue pointer past the TD.
-                * The class driver clears the device side halt later.
+               /*
+                * xhci internal endpoint state will go to a "halt" state for
+                * any stall, including default control pipe protocol stall.
+                * To clear the host side halt we need to issue a reset endpoint
+                * command, followed by a set dequeue command to move past the
+                * TD.
+                * Class drivers clear the device side halt from a functional
+                * stall later. Hub TT buffer should only be cleared for FS/LS
+                * devices behind HS hubs for functional stalls.
                 */
+               if ((ep_index != 0) || (trb_comp_code != COMP_STALL_ERROR))
+                       xhci_clear_hub_tt_buffer(xhci, td, ep);
                xhci_cleanup_halted_endpoint(xhci, slot_id, ep_index,
                                        ep_ring->stream_id, td, EP_HARD_RESET);
        } else {
--
2.17.1


-----------------------------------------------------------------------------------
This email message is for the sole use of the intended recipient(s) and may contain
confidential information.  Any unauthorized review, use, disclosure or distribution
is prohibited.  If you are not the intended recipient, please contact the sender by
reply email and destroy all copies of the original message.
-----------------------------------------------------------------------------------

Mathias Nyman April 17, 2020, 8:30 a.m. UTC | #2

On 17.4.2020 3.25, Jim Lin wrote:
> I had returned that device to customer.
> And won't be able to verify this patch.
> 
> Jim

Ok, thanks for letting me know

-Mathias

Patch

diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
index a7f4cd35da55..0fda0c0f4d31 100644
--- a/drivers/usb/host/xhci-ring.c
+++ b/drivers/usb/host/xhci-ring.c
@@ -1876,7 +1876,6 @@  static void xhci_cleanup_halted_endpoint(struct xhci_hcd *xhci,
 		ep->ep_state |= EP_HARD_CLEAR_TOGGLE;
 		xhci_cleanup_stalled_ring(xhci, slot_id, ep_index, stream_id,
 					  td);
-		xhci_clear_hub_tt_buffer(xhci, td, ep);
 	}
 	xhci_ring_cmd_db(xhci);
 }
@@ -1997,11 +1996,18 @@  static int finish_td(struct xhci_hcd *xhci, struct xhci_td *td,
 	if (trb_comp_code == COMP_STALL_ERROR ||
 		xhci_requires_manual_halt_cleanup(xhci, ep_ctx,
 						trb_comp_code)) {
-		/* Issue a reset endpoint command to clear the host side
-		 * halt, followed by a set dequeue command to move the
-		 * dequeue pointer past the TD.
-		 * The class driver clears the device side halt later.
+		/*
+		 * xhci internal endpoint state will go to a "halt" state for
+		 * any stall, including default control pipe protocol stall.
+		 * To clear the host side halt we need to issue a reset endpoint
+		 * command, followed by a set dequeue command to move past the
+		 * TD.
+		 * Class drivers clear the device side halt from a functional
+		 * stall later. Hub TT buffer should only be cleared for FS/LS
+		 * devices behind HS hubs for functional stalls.
 		 */
+		if ((ep_index != 0) || (trb_comp_code != COMP_STALL_ERROR))
+			xhci_clear_hub_tt_buffer(xhci, td, ep);
 		xhci_cleanup_halted_endpoint(xhci, slot_id, ep_index,
 					ep_ring->stream_id, td, EP_HARD_RESET);
 	} else {