[PATCH v4 1/1] usb: xhci: Add Clear_TT_Buffer

[PATCH v4 1/1] usb: xhci: Add Clear_TT_Buffer

[Jim Lin]

USB 2.0 specification chapter 11.17.5 says "as part of endpoint halt
processing for full-/low-speed endpoints connected via a TT, the host
software must use the Clear_TT_Buffer request to the TT to ensure
that the buffer is not in the busy state".

In our case, a full-speed speaker (ConferenceCam) is behind a high-
speed hub (ConferenceCam Connect), sometimes once we get STALL on a
request we may continue to get STALL with the folllowing requests,
like Set_Interface.

Here we add Clear_TT_Buffer for the following Set_Interface requests
to get ACK successfully.

Originally usb_hub_clear_tt_buffer uses urb->dev->devnum as device
address while sending Clear_TT_Buffer command, but this doesn't work
for XHCI.
We have to extend usb_hub_clear_tt_buffer parameter to specify device
number that will have the TT state of its hub cleared.

Signed-off-by: Jim Lin <jilin@nvidia.com>
---
v2: xhci_clear_tt_buffer_complete: add static, shorter indentation
    , remove its claiming in xhci.h
v3: Add description for clearing_tt (xhci.h)
v4: Remove clearing_tt flag because hub_tt_work has hub->tt.lock
    to protect for Clear_TT_Buffer to be run serially.
    Remove xhci_clear_tt_buffer_complete as it's not necessary.
    Same reason as the above.
    Extend usb_hub_clear_tt_buffer parameter

 drivers/usb/core/hub.c            |  5 +++--
 drivers/usb/dwc2/hcd_intr.c       |  2 +-
 drivers/usb/host/ehci-q.c         |  2 +-
 drivers/usb/host/fotg210-hcd.c    |  2 +-
 drivers/usb/host/xhci-ring.c      | 22 ++++++++++++++++++++++
 drivers/usb/isp1760/isp1760-hcd.c |  5 +++--
 include/linux/usb/hcd.h           |  2 +-
 7 files changed, 32 insertions(+), 8 deletions(-)

diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
index 15a2934dc29d..4dec054c9776 100644
--- a/drivers/usb/core/hub.c
+++ b/drivers/usb/core/hub.c
@@ -840,6 +840,7 @@ int usb_hub_set_port_power(struct usb_device *hdev, struct usb_hub *hub,
 /**
  * usb_hub_clear_tt_buffer - clear control/bulk TT state in high speed hub
  * @urb: an URB associated with the failed or incomplete split transaction
+ * @devnum : device number that will have the TT state of its hub cleared
  *
  * High speed HCDs use this to tell the hub driver that some split control or
  * bulk transaction failed in a way that requires clearing internal state of
@@ -851,7 +852,7 @@ int usb_hub_set_port_power(struct usb_device *hdev, struct usb_hub *hub,
  *
  * Return: 0 if successful. A negative error code otherwise.
  */
-int usb_hub_clear_tt_buffer(struct urb *urb)
+int usb_hub_clear_tt_buffer(struct urb *urb, int devnum)
 {
 	struct usb_device	*udev = urb->dev;
 	int			pipe = urb->pipe;
@@ -873,7 +874,7 @@ int usb_hub_clear_tt_buffer(struct urb *urb)
 	/* info that CLEAR_TT_BUFFER needs */
 	clear->tt = tt->multi ? udev->ttport : 1;
 	clear->devinfo = usb_pipeendpoint (pipe);
-	clear->devinfo |= udev->devnum << 4;
+	clear->devinfo |= devnum << 4;
 	clear->devinfo |= usb_pipecontrol(pipe)
 			? (USB_ENDPOINT_XFER_CONTROL << 11)
 			: (USB_ENDPOINT_XFER_BULK << 11);
diff --git a/drivers/usb/dwc2/hcd_intr.c b/drivers/usb/dwc2/hcd_intr.c
index 88b5dcf3aefc..a3bb6911a62a 100644
--- a/drivers/usb/dwc2/hcd_intr.c
+++ b/drivers/usb/dwc2/hcd_intr.c
@@ -125,7 +125,7 @@ static void dwc2_hc_handle_tt_clear(struct dwc2_hsotg *hsotg,
 
 	if (qtd->urb->status != -EPIPE && qtd->urb->status != -EREMOTEIO) {
 		chan->qh->tt_buffer_dirty = 1;
-		if (usb_hub_clear_tt_buffer(usb_urb))
+		if (usb_hub_clear_tt_buffer(usb_urb, usb_urb->dev->devnum))
 			/* Clear failed; let's hope things work anyway */
 			chan->qh->tt_buffer_dirty = 0;
 	}
diff --git a/drivers/usb/host/ehci-q.c b/drivers/usb/host/ehci-q.c
index aa2f77f1506d..9f64c260239e 100644
--- a/drivers/usb/host/ehci-q.c
+++ b/drivers/usb/host/ehci-q.c
@@ -169,7 +169,7 @@ static void ehci_clear_tt_buffer(struct ehci_hcd *ehci, struct ehci_qh *qh,
 		if (!ehci_is_TDI(ehci)
 				|| urb->dev->tt->hub !=
 				   ehci_to_hcd(ehci)->self.root_hub) {
-			if (usb_hub_clear_tt_buffer(urb) == 0)
+			if (!usb_hub_clear_tt_buffer(urb, urb->dev->devnum))
 				qh->clearing_tt = 1;
 		} else {
 
diff --git a/drivers/usb/host/fotg210-hcd.c b/drivers/usb/host/fotg210-hcd.c
index 0da68df259c8..bd29ed0fbd46 100644
--- a/drivers/usb/host/fotg210-hcd.c
+++ b/drivers/usb/host/fotg210-hcd.c
@@ -2123,7 +2123,7 @@ static void fotg210_clear_tt_buffer(struct fotg210_hcd *fotg210,
 
 		if (urb->dev->tt->hub !=
 				fotg210_to_hcd(fotg210)->self.root_hub) {
-			if (usb_hub_clear_tt_buffer(urb) == 0)
+			if (!usb_hub_clear_tt_buffer(urb, urb->dev->devnum))
 				qh->clearing_tt = 1;
 		}
 	}
diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
index 9215a28dad40..aa2a21e6cde3 100644
--- a/drivers/usb/host/xhci-ring.c
+++ b/drivers/usb/host/xhci-ring.c
@@ -1786,6 +1786,27 @@ struct xhci_segment *trb_in_td(struct xhci_hcd *xhci,
 	return NULL;
 }
 
+static void xhci_clear_hub_tt_buffer(struct xhci_hcd *xhci,
+	unsigned int slot_id, struct xhci_td *td)
+{
+	struct xhci_virt_device *dev;
+	struct xhci_slot_ctx *slot_ctx;
+	int devnum;
+
+	/*
+	 * As part of low/full-speed endpoint-halt processing
+	 * we must clear the TT buffer (USB 2.0 specification 11.17.5).
+	 */
+	if (td->urb->dev->tt && !usb_pipeint(td->urb->pipe) &&
+	    (td->urb->dev->tt->hub != xhci_to_hcd(xhci)->self.root_hub)) {
+		dev = xhci->devs[slot_id];
+		slot_ctx = xhci_get_slot_ctx(xhci, dev->out_ctx);
+		devnum = (int) le32_to_cpu(slot_ctx->dev_state) &
+			DEV_ADDR_MASK;
+		usb_hub_clear_tt_buffer(td->urb, devnum);
+	}
+}
+
 static void xhci_cleanup_halted_endpoint(struct xhci_hcd *xhci,
 		unsigned int slot_id, unsigned int ep_index,
 		unsigned int stream_id, struct xhci_td *td,
@@ -1804,6 +1825,7 @@ static void xhci_cleanup_halted_endpoint(struct xhci_hcd *xhci,
 	if (reset_type == EP_HARD_RESET) {
 		ep->ep_state |= EP_HARD_CLEAR_TOGGLE;
 		xhci_cleanup_stalled_ring(xhci, ep_index, stream_id, td);
+		xhci_clear_hub_tt_buffer(xhci, slot_id, td);
 	}
 	xhci_ring_cmd_db(xhci);
 }
diff --git a/drivers/usb/isp1760/isp1760-hcd.c b/drivers/usb/isp1760/isp1760-hcd.c
index 8142c6b4c4cf..f1d68092becb 100644
--- a/drivers/usb/isp1760/isp1760-hcd.c
+++ b/drivers/usb/isp1760/isp1760-hcd.c
@@ -1166,7 +1166,8 @@ static void handle_done_ptds(struct usb_hcd *hcd)
 					(qtd->urb->status != -EPIPE) &&
 					(qtd->urb->status != -EREMOTEIO)) {
 				qh->tt_buffer_dirty = 1;
-				if (usb_hub_clear_tt_buffer(qtd->urb))
+				if (usb_hub_clear_tt_buffer(qtd->urb,
+						qtd->urb->dev->devnum))
 					/* Clear failed; let's hope things work
 					   anyway */
 					qh->tt_buffer_dirty = 0;
@@ -1633,7 +1634,7 @@ static void dequeue_urb_from_qtd(struct usb_hcd *hcd, struct isp1760_qh *qh,
 
 	if ((urb->dev->speed != USB_SPEED_HIGH) && urb_was_running) {
 		qh->tt_buffer_dirty = 1;
-		if (usb_hub_clear_tt_buffer(urb))
+		if (usb_hub_clear_tt_buffer(urb, urb->dev->devnum))
 			/* Clear failed; let's hope things work anyway */
 			qh->tt_buffer_dirty = 0;
 	}
diff --git a/include/linux/usb/hcd.h b/include/linux/usb/hcd.h
index 695931b03684..01720f1f7d62 100644
--- a/include/linux/usb/hcd.h
+++ b/include/linux/usb/hcd.h
@@ -559,7 +559,7 @@ struct usb_tt_clear {
 	struct usb_host_endpoint	*ep;
 };
 
-extern int usb_hub_clear_tt_buffer(struct urb *urb);
+extern int usb_hub_clear_tt_buffer(struct urb *urb, int devnum);
 extern void usb_ep0_reinit(struct usb_device *);
 
 /* (shifted) direction/type/recipient from the USB 2.0 spec, table 9.2 */
-- 
2.1.4

Re: [PATCH v4 1/1] usb: xhci: Add Clear_TT_Buffer

[Alan Stern]

On Mon, 6 May 2019, Jim Lin wrote:

> USB 2.0 specification chapter 11.17.5 says "as part of endpoint halt
> processing for full-/low-speed endpoints connected via a TT, the host
> software must use the Clear_TT_Buffer request to the TT to ensure
> that the buffer is not in the busy state".
> 
> In our case, a full-speed speaker (ConferenceCam) is behind a high-
> speed hub (ConferenceCam Connect), sometimes once we get STALL on a
> request we may continue to get STALL with the folllowing requests,
> like Set_Interface.
> 
> Here we add Clear_TT_Buffer for the following Set_Interface requests
> to get ACK successfully.
> 
> Originally usb_hub_clear_tt_buffer uses urb->dev->devnum as device
> address while sending Clear_TT_Buffer command, but this doesn't work
> for XHCI.

Why doesn't it work for xHCI?  Clear-TT-Buffer is part of the USB 2.0 
spec; it should work exactly the same for xHCI as for a USB-2.0 host 
controller.

Alan Stern

Re: [PATCH v4 1/1] usb: xhci: Add Clear_TT_Buffer

[Mathias Nyman]

On 6.5.2019 17.57, Alan Stern wrote:
> On Mon, 6 May 2019, Jim Lin wrote:
> 
>> USB 2.0 specification chapter 11.17.5 says "as part of endpoint halt
>> processing for full-/low-speed endpoints connected via a TT, the host
>> software must use the Clear_TT_Buffer request to the TT to ensure
>> that the buffer is not in the busy state".
>>
>> In our case, a full-speed speaker (ConferenceCam) is behind a high-
>> speed hub (ConferenceCam Connect), sometimes once we get STALL on a
>> request we may continue to get STALL with the folllowing requests,
>> like Set_Interface.
>>
>> Here we add Clear_TT_Buffer for the following Set_Interface requests
>> to get ACK successfully.
>>
>> Originally usb_hub_clear_tt_buffer uses urb->dev->devnum as device
>> address while sending Clear_TT_Buffer command, but this doesn't work
>> for XHCI.
> 
> Why doesn't it work for xHCI?  Clear-TT-Buffer is part of the USB 2.0
> spec; it should work exactly the same for xHCI as for a USB-2.0 host
> controller.
> 
> Alan Stern
> 

For other host controllers udev->devnum is the same as the address of the
usb device, chosen and set by usb core.

With xHC the controller hardware assigns the address, and won't be the same as
devnum.

The Clear-TT-Buffer request sent to the hub includes the address of the LS/FS
child device in wValue field. usb_hub_clear_tt_buffer() uses udev->devnum to set the
address wValue. This won't work for devices connected to xHC
    
-Mathias

Re: [PATCH v4 1/1] usb: xhci: Add Clear_TT_Buffer

[Alan Stern]

On Tue, 7 May 2019, Mathias Nyman wrote:

> On 6.5.2019 17.57, Alan Stern wrote:
> > On Mon, 6 May 2019, Jim Lin wrote:
> > 
> >> USB 2.0 specification chapter 11.17.5 says "as part of endpoint halt
> >> processing for full-/low-speed endpoints connected via a TT, the host
> >> software must use the Clear_TT_Buffer request to the TT to ensure
> >> that the buffer is not in the busy state".
> >>
> >> In our case, a full-speed speaker (ConferenceCam) is behind a high-
> >> speed hub (ConferenceCam Connect), sometimes once we get STALL on a
> >> request we may continue to get STALL with the folllowing requests,
> >> like Set_Interface.
> >>
> >> Here we add Clear_TT_Buffer for the following Set_Interface requests
> >> to get ACK successfully.
> >>
> >> Originally usb_hub_clear_tt_buffer uses urb->dev->devnum as device
> >> address while sending Clear_TT_Buffer command, but this doesn't work
> >> for XHCI.
> > 
> > Why doesn't it work for xHCI?  Clear-TT-Buffer is part of the USB 2.0
> > spec; it should work exactly the same for xHCI as for a USB-2.0 host
> > controller.
> > 
> > Alan Stern
> > 
> 
> For other host controllers udev->devnum is the same as the address of the
> usb device, chosen and set by usb core.
> 
> With xHC the controller hardware assigns the address, and won't be the same as
> devnum.
> 
> The Clear-TT-Buffer request sent to the hub includes the address of the LS/FS
> child device in wValue field. usb_hub_clear_tt_buffer() uses udev->devnum to set the
> address wValue. This won't work for devices connected to xHC

I see.  Thanks for the explanation; it makes sense now.  The patch
description should explain this too.

Wouldn't it be better to add a field containing the device address to
struct usb_device?  And also export it, either in sysfs or debugfs?  
It seems like the kind of thing that might be important for debugging.  
If we did this then the usb_hub_clear_tt_buffer API wouldn't need to be
changed.

Alan Stern

Re: [PATCH v4 1/1] usb: xhci: Add Clear_TT_Buffer

[Mathias Nyman]

On 7.5.2019 17.29, Alan Stern wrote:
> On Tue, 7 May 2019, Mathias Nyman wrote:
> 
>> On 6.5.2019 17.57, Alan Stern wrote:
>>> On Mon, 6 May 2019, Jim Lin wrote:
>>>
>>>> USB 2.0 specification chapter 11.17.5 says "as part of endpoint halt
>>>> processing for full-/low-speed endpoints connected via a TT, the host
>>>> software must use the Clear_TT_Buffer request to the TT to ensure
>>>> that the buffer is not in the busy state".
>>>>
>>>> In our case, a full-speed speaker (ConferenceCam) is behind a high-
>>>> speed hub (ConferenceCam Connect), sometimes once we get STALL on a
>>>> request we may continue to get STALL with the folllowing requests,
>>>> like Set_Interface.
>>>>
>>>> Here we add Clear_TT_Buffer for the following Set_Interface requests
>>>> to get ACK successfully.
>>>>
>>>> Originally usb_hub_clear_tt_buffer uses urb->dev->devnum as device
>>>> address while sending Clear_TT_Buffer command, but this doesn't work
>>>> for XHCI.
>>>
>>> Why doesn't it work for xHCI?  Clear-TT-Buffer is part of the USB 2.0
>>> spec; it should work exactly the same for xHCI as for a USB-2.0 host
>>> controller.
>>>
>>> Alan Stern
>>>
>>
>> For other host controllers udev->devnum is the same as the address of the
>> usb device, chosen and set by usb core.
>>
>> With xHC the controller hardware assigns the address, and won't be the same as
>> devnum.
>>
>> The Clear-TT-Buffer request sent to the hub includes the address of the LS/FS
>> child device in wValue field. usb_hub_clear_tt_buffer() uses udev->devnum to set the
>> address wValue. This won't work for devices connected to xHC
> 
> I see.  Thanks for the explanation; it makes sense now.  The patch
> description should explain this too.
> 
> Wouldn't it be better to add a field containing the device address to
> struct usb_device?  And also export it, either in sysfs or debugfs?
> It seems like the kind of thing that might be important for debugging.
> If we did this then the usb_hub_clear_tt_buffer API wouldn't need to be
> changed.
> 

Agree, adding address to struct usb_device sounds better.

-Mathias