[PATCH 0/4] xhci features for usb-next

[PATCH 0/4] xhci features for usb-next

[Mathias Nyman]

Hi Greg

A few xhci features for usb-next, hope they are not too late for 5.5 kernel.
These features mainly prepare driver for handling a flood of xhci events,
but also enables runtime PM as default for a Ice Lake xHCI, and adds a bit
of tracing.

-Mathias

Mathias Nyman (1):
  xhci: Add tracing for xhci doorbell register writes

Mika Westerberg (1):
  xhci-pci: Allow host runtime PM as default also for Intel Ice Lake
    xHCI

Peter Chen (1):
  usb: host: xhci: update event ring dequeue pointer on purpose

Suwan Kim (1):
  usb: host: xhci: Support running urb giveback in tasklet context

 drivers/usb/host/xhci-pci.c   |  4 ++-
 drivers/usb/host/xhci-ring.c  | 68 +++++++++++++++++++++++++++++++------------
 drivers/usb/host/xhci-trace.h | 26 +++++++++++++++++
 drivers/usb/host/xhci.c       |  3 +-
 drivers/usb/host/xhci.h       | 29 ++++++++++++++++++
 5 files changed, 109 insertions(+), 21 deletions(-)

-- 
2.7.4

[PATCH 1/4] usb: host: xhci: update event ring dequeue pointer on purpose

[Mathias Nyman]

From: Peter Chen <peter.chen@nxp.com>

On some situations, the software handles TRB events slower
than adding TRBs, then xhci_handle_event can't return zero
long time, the xHC will consider the event ring is full,
and trigger "Event Ring Full" error, but in fact, the software
has already finished lots of events, just no chance to
update ERDP (event ring dequeue pointer).

In this commit, we force update ERDP if half of TRBS_PER_SEGMENT
events have handled to avoid "Event Ring Full" error.

Signed-off-by: Peter Chen <peter.chen@nxp.com>
Signed-off-by: Mathias Nyman <mathias.nyman@linux.intel.com>
---
 drivers/usb/host/xhci-ring.c | 60 +++++++++++++++++++++++++++++++-------------
 1 file changed, 43 insertions(+), 17 deletions(-)

diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
index e7aab31fd9a5..55084adf1faf 100644
--- a/drivers/usb/host/xhci-ring.c
+++ b/drivers/usb/host/xhci-ring.c
@@ -2741,6 +2741,42 @@ static int xhci_handle_event(struct xhci_hcd *xhci)
 }
 
 /*
+ * Update Event Ring Dequeue Pointer:
+ * - When all events have finished
+ * - To avoid "Event Ring Full Error" condition
+ */
+static void xhci_update_erst_dequeue(struct xhci_hcd *xhci,
+		union xhci_trb *event_ring_deq)
+{
+	u64 temp_64;
+	dma_addr_t deq;
+
+	temp_64 = xhci_read_64(xhci, &xhci->ir_set->erst_dequeue);
+	/* If necessary, update the HW's version of the event ring deq ptr. */
+	if (event_ring_deq != xhci->event_ring->dequeue) {
+		deq = xhci_trb_virt_to_dma(xhci->event_ring->deq_seg,
+				xhci->event_ring->dequeue);
+		if (deq == 0)
+			xhci_warn(xhci, "WARN something wrong with SW event ring dequeue ptr\n");
+		/*
+		 * Per 4.9.4, Software writes to the ERDP register shall
+		 * always advance the Event Ring Dequeue Pointer value.
+		 */
+		if ((temp_64 & (u64) ~ERST_PTR_MASK) ==
+				((u64) deq & (u64) ~ERST_PTR_MASK))
+			return;
+
+		/* Update HC event ring dequeue pointer */
+		temp_64 &= ERST_PTR_MASK;
+		temp_64 |= ((u64) deq & (u64) ~ERST_PTR_MASK);
+	}
+
+	/* Clear the event handler busy flag (RW1C) */
+	temp_64 |= ERST_EHB;
+	xhci_write_64(xhci, temp_64, &xhci->ir_set->erst_dequeue);
+}
+
+/*
  * xHCI spec says we can get an interrupt, and if the HC has an error condition,
  * we might get bad data out of the event ring.  Section 4.10.2.7 has a list of
  * indicators of an event TRB error, but we check the status *first* to be safe.
@@ -2751,9 +2787,9 @@ irqreturn_t xhci_irq(struct usb_hcd *hcd)
 	union xhci_trb *event_ring_deq;
 	irqreturn_t ret = IRQ_NONE;
 	unsigned long flags;
-	dma_addr_t deq;
 	u64 temp_64;
 	u32 status;
+	int event_loop = 0;
 
 	spin_lock_irqsave(&xhci->lock, flags);
 	/* Check if the xHC generated the interrupt, or the irq is shared */
@@ -2807,24 +2843,14 @@ irqreturn_t xhci_irq(struct usb_hcd *hcd)
 	/* FIXME this should be a delayed service routine
 	 * that clears the EHB.
 	 */
-	while (xhci_handle_event(xhci) > 0) {}
-
-	temp_64 = xhci_read_64(xhci, &xhci->ir_set->erst_dequeue);
-	/* If necessary, update the HW's version of the event ring deq ptr. */
-	if (event_ring_deq != xhci->event_ring->dequeue) {
-		deq = xhci_trb_virt_to_dma(xhci->event_ring->deq_seg,
-				xhci->event_ring->dequeue);
-		if (deq == 0)
-			xhci_warn(xhci, "WARN something wrong with SW event "
-					"ring dequeue ptr.\n");
-		/* Update HC event ring dequeue pointer */
-		temp_64 &= ERST_PTR_MASK;
-		temp_64 |= ((u64) deq & (u64) ~ERST_PTR_MASK);
+	while (xhci_handle_event(xhci) > 0) {
+		if (event_loop++ < TRBS_PER_SEGMENT / 2)
+			continue;
+		xhci_update_erst_dequeue(xhci, event_ring_deq);
+		event_loop = 0;
 	}
 
-	/* Clear the event handler busy flag (RW1C); event ring is empty. */
-	temp_64 |= ERST_EHB;
-	xhci_write_64(xhci, temp_64, &xhci->ir_set->erst_dequeue);
+	xhci_update_erst_dequeue(xhci, event_ring_deq);
 	ret = IRQ_HANDLED;
 
 out:
-- 
2.7.4

[PATCH 2/4] xhci: Add tracing for xhci doorbell register writes

[Mathias Nyman]

Trace when a register in the doorbell array is written,
both for host controller command doorbell and device doorbells,
including for which endpoint and stream

Signed-off-by: Mathias Nyman <mathias.nyman@linux.intel.com>
---
 drivers/usb/host/xhci-ring.c  |  6 ++++++
 drivers/usb/host/xhci-trace.h | 26 ++++++++++++++++++++++++++
 drivers/usb/host/xhci.h       | 29 +++++++++++++++++++++++++++++
 3 files changed, 61 insertions(+)

diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
index 55084adf1faf..bfd4d34c2535 100644
--- a/drivers/usb/host/xhci-ring.c
+++ b/drivers/usb/host/xhci-ring.c
@@ -280,6 +280,9 @@ void xhci_ring_cmd_db(struct xhci_hcd *xhci)
 		return;
 
 	xhci_dbg(xhci, "// Ding dong!\n");
+
+	trace_xhci_ring_host_doorbell(0, DB_VALUE_HOST);
+
 	writel(DB_VALUE_HOST, &xhci->dba->doorbell[0]);
 	/* Flush PCI posted writes */
 	readl(&xhci->dba->doorbell[0]);
@@ -401,6 +404,9 @@ void xhci_ring_ep_doorbell(struct xhci_hcd *xhci,
 	if ((ep_state & EP_STOP_CMD_PENDING) || (ep_state & SET_DEQ_PENDING) ||
 	    (ep_state & EP_HALTED) || (ep_state & EP_CLEARING_TT))
 		return;
+
+	trace_xhci_ring_ep_doorbell(slot_id, DB_VALUE(ep_index, stream_id));
+
 	writel(DB_VALUE(ep_index, stream_id), db_addr);
 	/* The CPU has better things to do at this point than wait for a
 	 * write-posting flush.  It'll get there soon enough.
diff --git a/drivers/usb/host/xhci-trace.h b/drivers/usb/host/xhci-trace.h
index 052a269d86f2..56eb867803a6 100644
--- a/drivers/usb/host/xhci-trace.h
+++ b/drivers/usb/host/xhci-trace.h
@@ -560,6 +560,32 @@ DEFINE_EVENT(xhci_log_portsc, xhci_hub_status_data,
 	     TP_ARGS(portnum, portsc)
 );
 
+DECLARE_EVENT_CLASS(xhci_log_doorbell,
+	TP_PROTO(u32 slot, u32 doorbell),
+	TP_ARGS(slot, doorbell),
+	TP_STRUCT__entry(
+		__field(u32, slot)
+		__field(u32, doorbell)
+	),
+	TP_fast_assign(
+		__entry->slot = slot;
+		__entry->doorbell = doorbell;
+	),
+	TP_printk("Ring doorbell for %s",
+		xhci_decode_doorbell(__entry->slot, __entry->doorbell)
+	)
+);
+
+DEFINE_EVENT(xhci_log_doorbell, xhci_ring_ep_doorbell,
+	     TP_PROTO(u32 slot, u32 doorbell),
+	     TP_ARGS(slot, doorbell)
+);
+
+DEFINE_EVENT(xhci_log_doorbell, xhci_ring_host_doorbell,
+	     TP_PROTO(u32 slot, u32 doorbell),
+	     TP_ARGS(slot, doorbell)
+);
+
 DECLARE_EVENT_CLASS(xhci_dbc_log_request,
 	TP_PROTO(struct dbc_request *req),
 	TP_ARGS(req),
diff --git a/drivers/usb/host/xhci.h b/drivers/usb/host/xhci.h
index f9f88626a57a..dc6f62a4b197 100644
--- a/drivers/usb/host/xhci.h
+++ b/drivers/usb/host/xhci.h
@@ -2580,6 +2580,35 @@ static inline const char *xhci_decode_portsc(u32 portsc)
 	return str;
 }
 
+static inline const char *xhci_decode_doorbell(u32 slot, u32 doorbell)
+{
+	static char str[256];
+	u8 ep;
+	u16 stream;
+	int ret;
+
+	ep = (doorbell & 0xff);
+	stream = doorbell >> 16;
+
+	if (slot == 0) {
+		sprintf(str, "Command Ring %d", doorbell);
+		return str;
+	}
+	ret = sprintf(str, "Slot %d ", slot);
+	if (ep > 0 && ep < 32)
+		ret = sprintf(str + ret, "ep%d%s",
+			      ep / 2,
+			      ep % 2 ? "in" : "out");
+	else if (ep == 0 || ep < 248)
+		ret = sprintf(str + ret, "Reserved %d", ep);
+	else
+		ret = sprintf(str + ret, "Vendor Defined %d", ep);
+	if (stream)
+		ret = sprintf(str + ret, " Stream %d", stream);
+
+	return str;
+}
+
 static inline const char *xhci_ep_state_string(u8 state)
 {
 	switch (state) {
-- 
2.7.4

[PATCH 3/4] usb: host: xhci: Support running urb giveback in tasklet context

[Mathias Nyman]

From: Suwan Kim <suwan.kim027@gmail.com>

Patch "USB: HCD: support giveback of URB in tasklet context"[1]
introduced giveback of urb in tasklet context. [1] This patch was
applied to ehci but not xhci. [2] This patch significantly reduces
the hard irq time of xhci. Especially for uvc driver, the hard irq
including the uvc completion function runs quite long but applying
this patch reduces the hard irq time of xhci.

I have tested four SS devices to check if performance degradation
occurs when urb completion functions run in the tasklet context.

As a result of the test, all devices works well and shows very
similar performance with the upstream kernel. Moreover, usb ethernet
adapter show better performance than the upstream kernel about 5% for
RX and 2% for TX. Four SS devices is as follows.

SS devices for test

1. WD My Passport 2TB (external hard drive)
2. Sandisk Ultra Flair USB 3.0 32GB
3. Logitech Brio webcam
4. Iptime 1gigabit ethernet adapter (Mediatek RTL8153)

Test description

1. Mass storage (hard drive) performance test
- run below command 10 times and compute the average performance

    dd if=/dev/sdN iflag=direct of=/dev/null bs=1G count=1

2. Mass storage (flash memory) performance test
- run below command 10 times and compute the average performance

    dd if=/dev/sdN iflag=direct of=/dev/null bs=1G count=1

3. Webcam streaming performance test
- run simple capture program and get the average frame rate per second
- capture 1500 frames
- program link

    https://github.com/asfaca/Webcam-performance-analyzing-tool

- video resolution : 4096 X 2160 (4K) at 30 or 24 fps
- device (Logitech Brio) spec url for the highest resolution and fps

    https://support.logitech.com/en_gb/product/brio-stream/specs

4. USB Ethernet adapter performance test
- directly connect two linux machines with ethernet cable
- run pktgen of linux kernel and send 1500 bytes packets
- run vnstat to measure the network bandwidth for 180 seconds

Test machine

- CPU : Intel i5-7600 @ 3.5GHz

Test results

1. Mass storage (hard drive) performance test

            WD My Passport 2TB (external hard drive)
--------------------------------------------------------------------
    xhci without tasklet        |          xhci with tasklet
--------------------------------------------------------------------
         103.667MB/s            |            103.692MB/s
--------------------------------------------------------------------

2. Mass storage (flash memory) performance test

               Sandisk Ultra Flair USB 3.0 32GB
--------------------------------------------------------------------
    xhci without tasklet        |          xhci with tasklet
--------------------------------------------------------------------
         129.727MB/s            |            130.2MB/s
--------------------------------------------------------------------

3. Webcam streaming performance test

                     Logitech Brio webcam
--------------------------------------------------------------------
    xhci without tasklet        |          xhci with tasklet
--------------------------------------------------------------------
          26.4451 fps           |            26.3949 fps
--------------------------------------------------------------------

4. USB Ethernet adapter performance test

      Iptime 1gigabit ethernet adapter (Mediatek RTL8153)
--------------------------------------------------------------------
    xhci without tasklet        |          xhci with tasklet
--------------------------------------------------------------------
RX      933.86 Mbit/s           |            983.86 Mbit/s
--------------------------------------------------------------------
TX      830.18 Mbit/s           |            882.75 Mbit/s
--------------------------------------------------------------------

[1], https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=94dfd7edfd5c9b605caf7b562de7a813d216e011
[2], https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=428aac8a81058e2303677a8fbf26670229e51d3a

Signed-off-by: Suwan Kim <suwan.kim027@gmail.com>
Signed-off-by: Mathias Nyman <mathias.nyman@linux.intel.com>
---
 drivers/usb/host/xhci-ring.c | 2 --
 drivers/usb/host/xhci.c      | 3 ++-
 2 files changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
index bfd4d34c2535..6475c3d3b43b 100644
--- a/drivers/usb/host/xhci-ring.c
+++ b/drivers/usb/host/xhci-ring.c
@@ -657,10 +657,8 @@ static void xhci_giveback_urb_in_irq(struct xhci_hcd *xhci,
 	}
 	xhci_urb_free_priv(urb_priv);
 	usb_hcd_unlink_urb_from_ep(hcd, urb);
-	spin_unlock(&xhci->lock);
 	trace_xhci_urb_giveback(urb);
 	usb_hcd_giveback_urb(hcd, urb, status);
-	spin_lock(&xhci->lock);
 }
 
 static void xhci_unmap_td_bounce_buffer(struct xhci_hcd *xhci,
diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
index 6c17e3fe181a..6721d059f58a 100644
--- a/drivers/usb/host/xhci.c
+++ b/drivers/usb/host/xhci.c
@@ -5301,7 +5301,8 @@ static const struct hc_driver xhci_hc_driver = {
 	 * generic hardware linkage
 	 */
 	.irq =			xhci_irq,
-	.flags =		HCD_MEMORY | HCD_DMA | HCD_USB3 | HCD_SHARED,
+	.flags =		HCD_MEMORY | HCD_DMA | HCD_USB3 | HCD_SHARED |
+				HCD_BH,
 
 	/*
 	 * basic lifecycle operations
-- 
2.7.4

[PATCH 4/4] xhci-pci: Allow host runtime PM as default also for Intel Ice Lake xHCI

[Mathias Nyman]

From: Mika Westerberg <mika.westerberg@linux.intel.com>

Intel Ice Lake has two xHCI controllers one on PCH and the other as part
of the CPU itself. The latter is also part of the so called Type C
Subsystem (TCSS) sharing ACPI power resources with the PCIe root ports
and the Thunderbolt controllers. In order to put the whole TCSS block
into D3cold the xHCI needs to be runtime suspended as well when idle.

For this reason allow runtime PM as default for Ice Lake TCSS xHCI
controller.

Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
Signed-off-by: Mathias Nyman <mathias.nyman@linux.intel.com>
---
 drivers/usb/host/xhci-pci.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/host/xhci-pci.c b/drivers/usb/host/xhci-pci.c
index 1e0236e90687..a0025d23b257 100644
--- a/drivers/usb/host/xhci-pci.c
+++ b/drivers/usb/host/xhci-pci.c
@@ -48,6 +48,7 @@
 #define PCI_DEVICE_ID_INTEL_TITAN_RIDGE_2C_XHCI		0x15e9
 #define PCI_DEVICE_ID_INTEL_TITAN_RIDGE_4C_XHCI		0x15ec
 #define PCI_DEVICE_ID_INTEL_TITAN_RIDGE_DD_XHCI		0x15f0
+#define PCI_DEVICE_ID_INTEL_ICE_LAKE_XHCI		0x8a13
 
 #define PCI_DEVICE_ID_AMD_PROMONTORYA_4			0x43b9
 #define PCI_DEVICE_ID_AMD_PROMONTORYA_3			0x43ba
@@ -212,7 +213,8 @@ static void xhci_pci_quirks(struct device *dev, struct xhci_hcd *xhci)
 	     pdev->device == PCI_DEVICE_ID_INTEL_ALPINE_RIDGE_C_4C_XHCI ||
 	     pdev->device == PCI_DEVICE_ID_INTEL_TITAN_RIDGE_2C_XHCI ||
 	     pdev->device == PCI_DEVICE_ID_INTEL_TITAN_RIDGE_4C_XHCI ||
-	     pdev->device == PCI_DEVICE_ID_INTEL_TITAN_RIDGE_DD_XHCI))
+	     pdev->device == PCI_DEVICE_ID_INTEL_TITAN_RIDGE_DD_XHCI ||
+	     pdev->device == PCI_DEVICE_ID_INTEL_ICE_LAKE_XHCI))
 		xhci->quirks |= XHCI_DEFAULT_PM_RUNTIME_ALLOW;
 
 	if (pdev->vendor == PCI_VENDOR_ID_ETRON &&
-- 
2.7.4

Re: [PATCH 4/4] xhci-pci: Allow host runtime PM as default also for Intel Ice Lake xHCI

[Greg KH]

On Fri, Nov 15, 2019 at 06:50:03PM +0200, Mathias Nyman wrote:
> From: Mika Westerberg <mika.westerberg@linux.intel.com>
> 
> Intel Ice Lake has two xHCI controllers one on PCH and the other as part
> of the CPU itself. The latter is also part of the so called Type C
> Subsystem (TCSS) sharing ACPI power resources with the PCIe root ports
> and the Thunderbolt controllers. In order to put the whole TCSS block
> into D3cold the xHCI needs to be runtime suspended as well when idle.
> 
> For this reason allow runtime PM as default for Ice Lake TCSS xHCI
> controller.
> 
> Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> Signed-off-by: Mathias Nyman <mathias.nyman@linux.intel.com>
> ---
>  drivers/usb/host/xhci-pci.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)

Can this also be queued up for the stable tree (or at least for 5.4.y?)

thanks,

greg k-h

Re: [PATCH 4/4] xhci-pci: Allow host runtime PM as default also for Intel Ice Lake xHCI

[Mathias Nyman]

On 16.11.2019 11.25, Greg KH wrote:
> On Fri, Nov 15, 2019 at 06:50:03PM +0200, Mathias Nyman wrote:
>> From: Mika Westerberg <mika.westerberg@linux.intel.com>
>>
>> Intel Ice Lake has two xHCI controllers one on PCH and the other as part
>> of the CPU itself. The latter is also part of the so called Type C
>> Subsystem (TCSS) sharing ACPI power resources with the PCIe root ports
>> and the Thunderbolt controllers. In order to put the whole TCSS block
>> into D3cold the xHCI needs to be runtime suspended as well when idle.
>>
>> For this reason allow runtime PM as default for Ice Lake TCSS xHCI
>> controller.
>>
>> Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
>> Signed-off-by: Mathias Nyman <mathias.nyman@linux.intel.com>
>> ---
>>   drivers/usb/host/xhci-pci.c | 4 +++-
>>   1 file changed, 3 insertions(+), 1 deletion(-)
> 
> Can this also be queued up for the stable tree (or at least for 5.4.y?)
> 

Yes, please, 5.4.y
Ice Lake Thunderbolt support [1] was accepted to 5.4-rc1 so 5.4.y makes sense.
Patch is not that useful for older stable versions on its own.
  
[1] 3cdb9446a117 thunderbolt: Add support for Intel Ice Lake

-Mathias

Re: [PATCH 4/4] xhci-pci: Allow host runtime PM as default also for Intel Ice Lake xHCI

[Greg KH]

On Mon, Nov 18, 2019 at 11:15:21AM +0200, Mathias Nyman wrote:
> On 16.11.2019 11.25, Greg KH wrote:
> > On Fri, Nov 15, 2019 at 06:50:03PM +0200, Mathias Nyman wrote:
> > > From: Mika Westerberg <mika.westerberg@linux.intel.com>
> > > 
> > > Intel Ice Lake has two xHCI controllers one on PCH and the other as part
> > > of the CPU itself. The latter is also part of the so called Type C
> > > Subsystem (TCSS) sharing ACPI power resources with the PCIe root ports
> > > and the Thunderbolt controllers. In order to put the whole TCSS block
> > > into D3cold the xHCI needs to be runtime suspended as well when idle.
> > > 
> > > For this reason allow runtime PM as default for Ice Lake TCSS xHCI
> > > controller.
> > > 
> > > Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> > > Signed-off-by: Mathias Nyman <mathias.nyman@linux.intel.com>
> > > ---
> > >   drivers/usb/host/xhci-pci.c | 4 +++-
> > >   1 file changed, 3 insertions(+), 1 deletion(-)
> > 
> > Can this also be queued up for the stable tree (or at least for 5.4.y?)
> > 
> 
> Yes, please, 5.4.y
> Ice Lake Thunderbolt support [1] was accepted to 5.4-rc1 so 5.4.y makes sense.
> Patch is not that useful for older stable versions on its own.
> [1] 3cdb9446a117 thunderbolt: Add support for Intel Ice Lake

Ok, I'll try to remember it when it goes into Linus's tree.

thanks,

greg k-h