[PATCH v2 0/5] xhci: Some improvement for Etron xHCI host

[PATCH v2 0/5] xhci: Some improvement for Etron xHCI host

[Kuangyi Chiang]

Add patch 1 to combine two if statements for Etron xHCI host in
xhci_pci_quirks():
  xhci: Combine two if statements for Etron xHCI host

Add patch 5 to prevent the xHCI driver from printing a "Transfer
event TRB DMA ptr not part of current TD" error message when an
error is detected while processing an one-TRB isoc TD:
  xhci: Correct handling of one-TRB isoc TD on Etron xHCI host

In fact, these problems are unlikely to occur on other host
controllers, so adding XHCI_ETRON_HOST quirk flag to invoke
these workarounds:
  xhci: Don't issue Reset Device command to Etron xHCI host
  xhci: Fix control transfer error on Etron xHCI host
  xhci: Correct handling of one-TRB isoc TD on Etron xHCI host

Apply XHCI_NO_SOFT_RETRY quirk flag to disable Soft Retry:
  xhci: Don't perform Soft Retry for Etron xHCI host

---
Changes in v2:
- Modify commit message
- Remove XHCI_NO_RESET_DEVICE/XHCI_NO_BREAK_CTRL_TD quirk flag
- Add XHCI_ETRON_HOST quirk flag, thanks for the suggestion by Michal
- Check device speed before invoking the workaround
- Add (xhci: Combine two if statements for Etron xHCI host)
- Add (xhci: Correct handling of one-TRB isoc TD on Etron xHCI host)
- Link to v1: https://lore.kernel.org/all/20240911051716.6572-4-ki.chiang65@gmail.com

Kuangyi Chiang (5):
  xhci: Combine two if statements for Etron xHCI host
  xhci: Don't issue Reset Device command to Etron xHCI host
  xhci: Fix control transfer error on Etron xHCI host
  xhci: Don't perform Soft Retry for Etron xHCI host
  xhci: Correct handling of one-TRB isoc TD on Etron xHCI host

 drivers/usb/host/xhci-pci.c  | 10 ++++-----
 drivers/usb/host/xhci-ring.c | 40 ++++++++++++++++++++++++++----------
 drivers/usb/host/xhci.c      | 19 +++++++++++++++++
 drivers/usb/host/xhci.h      |  3 ++-
 4 files changed, 54 insertions(+), 18 deletions(-)

-- 
2.25.1

[PATCH v2 1/5] xhci: Combine two if statements for Etron xHCI host

[Kuangyi Chiang]

Combine two if statements, because these hosts have the same
quirk flags applied.

Fixes: 91f7a1524a92 ("xhci: Apply broken streams quirk to Etron EJ188 xHCI host")
Cc: <stable@vger.kernel.org>
Signed-off-by: Kuangyi Chiang <ki.chiang65@gmail.com>
---
 drivers/usb/host/xhci-pci.c | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/drivers/usb/host/xhci-pci.c b/drivers/usb/host/xhci-pci.c
index 7e538194a0a4..33a6d99afc10 100644
--- a/drivers/usb/host/xhci-pci.c
+++ b/drivers/usb/host/xhci-pci.c
@@ -395,12 +395,8 @@ static void xhci_pci_quirks(struct device *dev, struct xhci_hcd *xhci)
 		xhci->quirks |= XHCI_DEFAULT_PM_RUNTIME_ALLOW;
 
 	if (pdev->vendor == PCI_VENDOR_ID_ETRON &&
-			pdev->device == PCI_DEVICE_ID_EJ168) {
-		xhci->quirks |= XHCI_RESET_ON_RESUME;
-		xhci->quirks |= XHCI_BROKEN_STREAMS;
-	}
-	if (pdev->vendor == PCI_VENDOR_ID_ETRON &&
-			pdev->device == PCI_DEVICE_ID_EJ188) {
+	    (pdev->device == PCI_DEVICE_ID_EJ168 ||
+	     pdev->device == PCI_DEVICE_ID_EJ188)) {
 		xhci->quirks |= XHCI_RESET_ON_RESUME;
 		xhci->quirks |= XHCI_BROKEN_STREAMS;
 	}
-- 
2.25.1

Re: [PATCH v2 1/5] xhci: Combine two if statements for Etron xHCI host

[Mathias Nyman]

On 28.10.2024 4.53, Kuangyi Chiang wrote:
> Combine two if statements, because these hosts have the same
> quirk flags applied.
> 
> Fixes: 91f7a1524a92 ("xhci: Apply broken streams quirk to Etron EJ188 xHCI host")
> Cc: <stable@vger.kernel.org>
> Signed-off-by: Kuangyi Chiang <ki.chiang65@gmail.com>

Added to queue, but I removed the Fixes and stable tags as this is a small
cleanup with no functional changes.

> ---
>   drivers/usb/host/xhci-pci.c | 8 ++------
>   1 file changed, 2 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/usb/host/xhci-pci.c b/drivers/usb/host/xhci-pci.c
> index 7e538194a0a4..33a6d99afc10 100644
> --- a/drivers/usb/host/xhci-pci.c
> +++ b/drivers/usb/host/xhci-pci.c
> @@ -395,12 +395,8 @@ static void xhci_pci_quirks(struct device *dev, struct xhci_hcd *xhci)
>   		xhci->quirks |= XHCI_DEFAULT_PM_RUNTIME_ALLOW;
>   
>   	if (pdev->vendor == PCI_VENDOR_ID_ETRON &&
> -			pdev->device == PCI_DEVICE_ID_EJ168) {
> -		xhci->quirks |= XHCI_RESET_ON_RESUME;
> -		xhci->quirks |= XHCI_BROKEN_STREAMS;
> -	}
> -	if (pdev->vendor == PCI_VENDOR_ID_ETRON &&
> -			pdev->device == PCI_DEVICE_ID_EJ188) {
> +	    (pdev->device == PCI_DEVICE_ID_EJ168 ||
> +	     pdev->device == PCI_DEVICE_ID_EJ188)) {
>   		xhci->quirks |= XHCI_RESET_ON_RESUME;
>   		xhci->quirks |= XHCI_BROKEN_STREAMS;
>   	}

Re: [PATCH v2 1/5] xhci: Combine two if statements for Etron xHCI host

[Kuangyi Chiang]

Hi,

I noticed that one of the patches in your queue has a typo:

Commit 3456904e4bce ("xhci: pci: Use standard pattern for device IDs")

The Etron xHC device names are EJ168 and EJ188, not J168 and J188.

Thanks,
Kuangyi Chiang

Mathias Nyman <mathias.nyman@linux.intel.com> 於 2024年10月30日 週三 下午8:02寫道：
>
> On 28.10.2024 4.53, Kuangyi Chiang wrote:
> > Combine two if statements, because these hosts have the same
> > quirk flags applied.
> >
> > Fixes: 91f7a1524a92 ("xhci: Apply broken streams quirk to Etron EJ188 xHCI host")
> > Cc: <stable@vger.kernel.org>
> > Signed-off-by: Kuangyi Chiang <ki.chiang65@gmail.com>
>
> Added to queue, but I removed the Fixes and stable tags as this is a small
> cleanup with no functional changes.
>
> > ---
> >   drivers/usb/host/xhci-pci.c | 8 ++------
> >   1 file changed, 2 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/usb/host/xhci-pci.c b/drivers/usb/host/xhci-pci.c
> > index 7e538194a0a4..33a6d99afc10 100644
> > --- a/drivers/usb/host/xhci-pci.c
> > +++ b/drivers/usb/host/xhci-pci.c
> > @@ -395,12 +395,8 @@ static void xhci_pci_quirks(struct device *dev, struct xhci_hcd *xhci)
> >               xhci->quirks |= XHCI_DEFAULT_PM_RUNTIME_ALLOW;
> >
> >       if (pdev->vendor == PCI_VENDOR_ID_ETRON &&
> > -                     pdev->device == PCI_DEVICE_ID_EJ168) {
> > -             xhci->quirks |= XHCI_RESET_ON_RESUME;
> > -             xhci->quirks |= XHCI_BROKEN_STREAMS;
> > -     }
> > -     if (pdev->vendor == PCI_VENDOR_ID_ETRON &&
> > -                     pdev->device == PCI_DEVICE_ID_EJ188) {
> > +         (pdev->device == PCI_DEVICE_ID_EJ168 ||
> > +          pdev->device == PCI_DEVICE_ID_EJ188)) {
> >               xhci->quirks |= XHCI_RESET_ON_RESUME;
> >               xhci->quirks |= XHCI_BROKEN_STREAMS;
> >       }
>

Re: [PATCH v2 1/5] xhci: Combine two if statements for Etron xHCI host

[Mathias Nyman]

On 1.11.2024 4.30, Kuangyi Chiang wrote:
> Hi,
> 
> I noticed that one of the patches in your queue has a typo:
> 
> Commit 3456904e4bce ("xhci: pci: Use standard pattern for device IDs")
> 
> The Etron xHC device names are EJ168 and EJ188, not J168 and J188.

Thanks for reporting, now fixed

-Mathias

[PATCH v2 2/5] xhci: Don't issue Reset Device command to Etron xHCI host

[Kuangyi Chiang]

Sometimes the hub driver does not recognize the USB device connected
to the external USB2.0 hub when the system resumes from S4.

After the SetPortFeature(PORT_RESET) request is completed, the hub
driver calls the HCD reset_device callback, which will issue a Reset
Device command and free all structures associated with endpoints
that were disabled.

This happens when the xHCI driver issue a Reset Device command to
inform the Etron xHCI host that the USB device associated with a
device slot has been reset. Seems that the Etron xHCI host can not
perform this command correctly, affecting the USB device.

To work around this, the xHCI driver should obtain a new device slot
with reference to commit 651aaf36a7d7 ("usb: xhci: Handle USB transaction
error on address command"), which is another way to inform the Etron
xHCI host that the USB device has been reset.

Add a new XHCI_ETRON_HOST quirk flag to invoke the workaround in
xhci_discover_or_reset_device().

Fixes: 2a8f82c4ceaf ("USB: xhci: Notify the xHC when a device is reset.")
Cc: <stable@vger.kernel.org>
Signed-off-by: Kuangyi Chiang <ki.chiang65@gmail.com>
---
 drivers/usb/host/xhci-pci.c |  1 +
 drivers/usb/host/xhci.c     | 19 +++++++++++++++++++
 drivers/usb/host/xhci.h     |  1 +
 3 files changed, 21 insertions(+)

diff --git a/drivers/usb/host/xhci-pci.c b/drivers/usb/host/xhci-pci.c
index 33a6d99afc10..ddc9a82cceec 100644
--- a/drivers/usb/host/xhci-pci.c
+++ b/drivers/usb/host/xhci-pci.c
@@ -397,6 +397,7 @@ static void xhci_pci_quirks(struct device *dev, struct xhci_hcd *xhci)
 	if (pdev->vendor == PCI_VENDOR_ID_ETRON &&
 	    (pdev->device == PCI_DEVICE_ID_EJ168 ||
 	     pdev->device == PCI_DEVICE_ID_EJ188)) {
+		xhci->quirks |= XHCI_ETRON_HOST;
 		xhci->quirks |= XHCI_RESET_ON_RESUME;
 		xhci->quirks |= XHCI_BROKEN_STREAMS;
 	}
diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
index 899c0effb5d3..ef7ead6393d4 100644
--- a/drivers/usb/host/xhci.c
+++ b/drivers/usb/host/xhci.c
@@ -3692,6 +3692,8 @@ void xhci_free_device_endpoint_resources(struct xhci_hcd *xhci,
 				xhci->num_active_eps);
 }
 
+static void xhci_free_dev(struct usb_hcd *hcd, struct usb_device *udev);
+
 /*
  * This submits a Reset Device Command, which will set the device state to 0,
  * set the device address to 0, and disable all the endpoints except the default
@@ -3762,6 +3764,23 @@ static int xhci_discover_or_reset_device(struct usb_hcd *hcd,
 						SLOT_STATE_DISABLED)
 		return 0;
 
+	if (xhci->quirks & XHCI_ETRON_HOST) {
+		/*
+		 * Obtaining a new device slot to inform the xHCI host that
+		 * the USB device has been reset.
+		 */
+		ret = xhci_disable_slot(xhci, udev->slot_id);
+		xhci_free_virt_device(xhci, udev->slot_id);
+		if (!ret) {
+			ret = xhci_alloc_dev(hcd, udev);
+			if (ret == 1)
+				ret = 0;
+			else
+				ret = -EINVAL;
+		}
+		return ret;
+	}
+
 	trace_xhci_discover_or_reset_device(slot_ctx);
 
 	xhci_dbg(xhci, "Resetting device with slot ID %u\n", slot_id);
diff --git a/drivers/usb/host/xhci.h b/drivers/usb/host/xhci.h
index f0fb696d5619..4f5b732e8944 100644
--- a/drivers/usb/host/xhci.h
+++ b/drivers/usb/host/xhci.h
@@ -1624,6 +1624,7 @@ struct xhci_hcd {
 #define XHCI_ZHAOXIN_HOST	BIT_ULL(46)
 #define XHCI_WRITE_64_HI_LO	BIT_ULL(47)
 #define XHCI_CDNS_SCTX_QUIRK	BIT_ULL(48)
+#define XHCI_ETRON_HOST	BIT_ULL(49)
 
 	unsigned int		num_active_eps;
 	unsigned int		limit_active_eps;
-- 
2.25.1

Re: [PATCH v2 2/5] xhci: Don't issue Reset Device command to Etron xHCI host

[Mathias Nyman]

On 28.10.2024 4.53, Kuangyi Chiang wrote:
> Sometimes the hub driver does not recognize the USB device connected
> to the external USB2.0 hub when the system resumes from S4.
> 
> After the SetPortFeature(PORT_RESET) request is completed, the hub
> driver calls the HCD reset_device callback, which will issue a Reset
> Device command and free all structures associated with endpoints
> that were disabled.
> 
> This happens when the xHCI driver issue a Reset Device command to
> inform the Etron xHCI host that the USB device associated with a
> device slot has been reset. Seems that the Etron xHCI host can not
> perform this command correctly, affecting the USB device.
> 
> To work around this, the xHCI driver should obtain a new device slot
> with reference to commit 651aaf36a7d7 ("usb: xhci: Handle USB transaction
> error on address command"), which is another way to inform the Etron
> xHCI host that the USB device has been reset.
> 
> Add a new XHCI_ETRON_HOST quirk flag to invoke the workaround in
> xhci_discover_or_reset_device().
> 
> Fixes: 2a8f82c4ceaf ("USB: xhci: Notify the xHC when a device is reset.")
> Cc: <stable@vger.kernel.org>
> Signed-off-by: Kuangyi Chiang <ki.chiang65@gmail.com>

Ok, I see, this patch depends on previous one, that's why it had the tags

Added this as well

Thanks
Mathias

[PATCH v2 3/5] xhci: Fix control transfer error on Etron xHCI host

[Kuangyi Chiang]

Performing a stability stress test on a USB3.0 2.5G ethernet adapter
results in errors like this:

[   91.441469] r8152 2-3:1.0 eth3: get_registers -71
[   91.458659] r8152 2-3:1.0 eth3: get_registers -71
[   91.475911] r8152 2-3:1.0 eth3: get_registers -71
[   91.493203] r8152 2-3:1.0 eth3: get_registers -71
[   91.510421] r8152 2-3:1.0 eth3: get_registers -71

The r8152 driver will periodically issue lots of control-IN requests
to access the status of ethernet adapter hardware registers during
the test.

This happens when the xHCI driver enqueue a control TD (which cross
over the Link TRB between two ring segments, as shown) in the endpoint
zero's transfer ring. Seems the Etron xHCI host can not perform this
TD correctly, causing the USB transfer error occurred, maybe the upper
driver retry that control-IN request can solve problem, but not all
drivers do this.

|     |
-------
| TRB | Setup Stage
-------
| TRB | Link
-------
-------
| TRB | Data Stage
-------
| TRB | Status Stage
-------
|     |

To work around this, the xHCI driver should enqueue a No Op TRB if
next available TRB is the Link TRB in the ring segment, this can
prevent the Setup and Data Stage TRB to be breaked by the Link TRB.

Check if the XHCI_ETRON_HOST quirk flag is set before invoking the
workaround in xhci_queue_ctrl_tx().

Fixes: d0e96f5a71a0 ("USB: xhci: Control transfer support.")
Cc: <stable@vger.kernel.org>
Signed-off-by: Kuangyi Chiang <ki.chiang65@gmail.com>
---
 drivers/usb/host/xhci-ring.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
index b6eb928e260f..9e132b08bfde 100644
--- a/drivers/usb/host/xhci-ring.c
+++ b/drivers/usb/host/xhci-ring.c
@@ -3727,6 +3727,20 @@ int xhci_queue_ctrl_tx(struct xhci_hcd *xhci, gfp_t mem_flags,
 	if (!urb->setup_packet)
 		return -EINVAL;
 
+	if ((xhci->quirks & XHCI_ETRON_HOST) &&
+	    urb->dev->speed >= USB_SPEED_SUPER) {
+		/*
+		 * If next available TRB is the Link TRB in the ring segment then
+		 * enqueue a No Op TRB, this can prevent the Setup and Data Stage
+		 * TRB to be breaked by the Link TRB.
+		 */
+		if (trb_is_link(ep_ring->enqueue + 1)) {
+			field = TRB_TYPE(TRB_TR_NOOP) | ep_ring->cycle_state;
+			queue_trb(xhci, ep_ring, false, 0, 0,
+					TRB_INTR_TARGET(0), field);
+		}
+	}
+
 	/* 1 TRB for setup, 1 for status */
 	num_trbs = 2;
 	/*
-- 
2.25.1

[PATCH v2 4/5] xhci: Don't perform Soft Retry for Etron xHCI host

[Kuangyi Chiang]

Since commit f8f80be501aa ("xhci: Use soft retry to recover faster from
transaction errors"), unplugging USB device while enumeration results in
errors like this:

[ 364.855321] xhci_hcd 0000:0b:00.0: ERROR Transfer event for disabled endpoint slot 5 ep 2
[ 364.864622] xhci_hcd 0000:0b:00.0: @0000002167656d70 67f03000 00000021 0c000000 05038001
[ 374.934793] xhci_hcd 0000:0b:00.0: Abort failed to stop command ring: -110
[ 374.958793] xhci_hcd 0000:0b:00.0: xHCI host controller not responding, assume dead
[ 374.967590] xhci_hcd 0000:0b:00.0: HC died; cleaning up
[ 374.973984] xhci_hcd 0000:0b:00.0: Timeout while waiting for configure endpoint command

Seems that Etorn xHCI host can not perform Soft Retry correctly, apply
XHCI_NO_SOFT_RETRY quirk to disable Soft Retry and then issue is gone.

This patch depends on commit a4a251f8c235 ("usb: xhci: do not perform
Soft Retry for some xHCI hosts").

Fixes: f8f80be501aa ("xhci: Use soft retry to recover faster from transaction errors")
Cc: <stable@vger.kernel.org>
Signed-off-by: Kuangyi Chiang <ki.chiang65@gmail.com>
---
 drivers/usb/host/xhci-pci.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/usb/host/xhci-pci.c b/drivers/usb/host/xhci-pci.c
index ddc9a82cceec..f2ca0b912977 100644
--- a/drivers/usb/host/xhci-pci.c
+++ b/drivers/usb/host/xhci-pci.c
@@ -400,6 +400,7 @@ static void xhci_pci_quirks(struct device *dev, struct xhci_hcd *xhci)
 		xhci->quirks |= XHCI_ETRON_HOST;
 		xhci->quirks |= XHCI_RESET_ON_RESUME;
 		xhci->quirks |= XHCI_BROKEN_STREAMS;
+		xhci->quirks |= XHCI_NO_SOFT_RETRY;
 	}
 
 	if (pdev->vendor == PCI_VENDOR_ID_RENESAS &&
-- 
2.25.1

[PATCH v2 5/5] xhci: Correct handling of one-TRB isoc TD on Etron xHCI host

[Kuangyi Chiang]

Unplugging a USB3.0 webcam while streaming results in errors
like this:

[ 132.646387] xhci_hcd 0000:03:00.0: ERROR Transfer event TRB DMA ptr not part of current TD ep_index 18 comp_code 13
[ 132.646446] xhci_hcd 0000:03:00.0: Looking for event-dma 000000002fdf8630 trb-start 000000002fdf8640 trb-end 000000002fdf8650 seg-start 000000002fdf8000 seg-end 000000002fdf8ff0
[ 132.646560] xhci_hcd 0000:03:00.0: ERROR Transfer event TRB DMA ptr not part of current TD ep_index 18 comp_code 13
[ 132.646568] xhci_hcd 0000:03:00.0: Looking for event-dma 000000002fdf8660 trb-start 000000002fdf8670 trb-end 000000002fdf8670 seg-start 000000002fdf8000 seg-end 000000002fdf8ff0

If an error is detected while processing an one-TRB isoc TD,
the Etron xHC generates two transfer events for the TRB that
the error was detected on. The first event is "USB Transcation
Error", and the second event is "Success".

The xHCI driver will handle the TD after the first event and
remove it from its internal list, and then print an "Transfer
event TRB DMA ptr not part of current TD" error message after
the second event.

As a solution, we can set the flag after the first error event
and don't print the error message after the second event if
the flag is set.

Commit ad808333d820 ("Intel xhci: Ignore spurious successful
event.") implements a similar mechanism that we can reuse to
solve this problem since short transfer and transfer error
doesn't occur concurrently. Also, rename the flag to make it
more meaningful.

Check if the XHCI_ETRON_HOST quirk flag is set before invoking
the workaround in process_isoc_td().

This patch doesn't affect other host controllers that have the
XHCI_SPURIOUS_SUCCESS quirk flag applied.

Signed-off-by: Kuangyi Chiang <ki.chiang65@gmail.com>
---
 drivers/usb/host/xhci-ring.c | 26 +++++++++++++++-----------
 drivers/usb/host/xhci.h      |  2 +-
 2 files changed, 16 insertions(+), 12 deletions(-)

diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
index 9e132b08bfde..33fa8a11c934 100644
--- a/drivers/usb/host/xhci-ring.c
+++ b/drivers/usb/host/xhci-ring.c
@@ -2437,6 +2437,10 @@ static int process_isoc_td(struct xhci_hcd *xhci, struct xhci_virt_ep *ep,
 		sum_trbs_for_length = true;
 		if (ep_trb != td->last_trb)
 			td->error_mid_td = true;
+		if ((xhci->quirks & XHCI_ETRON_HOST) &&
+		    td->urb->dev->speed >= USB_SPEED_SUPER &&
+		    td->first_trb == td->last_trb)
+			ep_ring->spurious_event = true;
 		break;
 	case COMP_STOPPED:
 		sum_trbs_for_length = true;
@@ -2655,8 +2659,8 @@ static int handle_tx_event(struct xhci_hcd *xhci,
 	case COMP_SUCCESS:
 		if (EVENT_TRB_LEN(le32_to_cpu(event->transfer_len)) != 0) {
 			trb_comp_code = COMP_SHORT_PACKET;
-			xhci_dbg(xhci, "Successful completion on short TX for slot %u ep %u with last td short %d\n",
-				 slot_id, ep_index, ep_ring->last_td_was_short);
+			xhci_dbg(xhci, "Successful completion on short TX for slot %u ep %u with spurious event %d\n",
+				 slot_id, ep_index, ep_ring->spurious_event);
 		}
 		break;
 	case COMP_SHORT_PACKET:
@@ -2801,13 +2805,13 @@ static int handle_tx_event(struct xhci_hcd *xhci,
 	if (list_empty(&ep_ring->td_list)) {
 		/*
 		 * Don't print wanings if ring is empty due to a stopped endpoint generating an
-		 * extra completion event if the device was suspended. Or, a event for the last TRB
-		 * of a short TD we already got a short event for. The short TD is already removed
-		 * from the TD list.
+		 * extra completion event if the device was suspended. Or, the spurious event flag
+		 * is set at the last TD of the TD list due to a short transfer or an one-TRB isoc
+		 * TD error, and such TD is already removed from the TD list.
 		 */
 		if (trb_comp_code != COMP_STOPPED &&
 		    trb_comp_code != COMP_STOPPED_LENGTH_INVALID &&
-		    !ep_ring->last_td_was_short) {
+		    !ep_ring->spurious_event) {
 			xhci_warn(xhci, "Event TRB for slot %u ep %u with no TDs queued\n",
 				  slot_id, ep_index);
 		}
@@ -2851,11 +2855,11 @@ static int handle_tx_event(struct xhci_hcd *xhci,
 
 			/*
 			 * Some hosts give a spurious success event after a short
-			 * transfer. Ignore it.
+			 * transfer or an one-TRB isoc TD error. Ignore it.
 			 */
 			if ((xhci->quirks & XHCI_SPURIOUS_SUCCESS) &&
-			    ep_ring->last_td_was_short) {
-				ep_ring->last_td_was_short = false;
+			    ep_ring->spurious_event) {
+				ep_ring->spurious_event = false;
 				return 0;
 			}
 
@@ -2884,9 +2888,9 @@ static int handle_tx_event(struct xhci_hcd *xhci,
 	} while (ep->skip);
 
 	if (trb_comp_code == COMP_SHORT_PACKET)
-		ep_ring->last_td_was_short = true;
+		ep_ring->spurious_event = true;
 	else
-		ep_ring->last_td_was_short = false;
+		ep_ring->spurious_event = false;
 
 	ep_trb = &ep_seg->trbs[(ep_trb_dma - ep_seg->dma) / sizeof(*ep_trb)];
 	trace_xhci_handle_transfer(ep_ring, (struct xhci_generic_trb *) ep_trb);
diff --git a/drivers/usb/host/xhci.h b/drivers/usb/host/xhci.h
index 4f5b732e8944..dca9091b8134 100644
--- a/drivers/usb/host/xhci.h
+++ b/drivers/usb/host/xhci.h
@@ -1359,7 +1359,7 @@ struct xhci_ring {
 	unsigned int		num_trbs_free; /* used only by xhci DbC */
 	unsigned int		bounce_buf_len;
 	enum xhci_ring_type	type;
-	bool			last_td_was_short;
+	bool			spurious_event;
 	struct radix_tree_root	*trb_address_map;
 };
 
-- 
2.25.1

Re: [PATCH v2 5/5] xhci: Correct handling of one-TRB isoc TD on Etron xHCI host

[Michał Pecio]

Hi,

That's a bug I'm familiar with.

> Unplugging a USB3.0 webcam while streaming results in errors
> like this

Not only unplugging but also any random error due to EMI or bad cable.

> If an error is detected while processing an one-TRB isoc TD,
> the Etron xHC generates two transfer events for the TRB that
> the error was detected on. The first event is "USB Transcation
> Error", and the second event is "Success".

IIRC, it wasn't just Transaction Errors but any sort of error, like
Babble or Bandwidth Overrun. But not sure about Missed Service, etc.

And IIRC I confirmed that it was *not* the case on Short Packet.

Also, I'm 99% sure the problem is not limited to one-TRB TDs, but
it occurs every time there is an error on the last TRB of any TD.

> As a solution, we can set the flag after the first error event
> and don't print the error message after the second event if the
> flag is set.

Yes, but I think it would be better to use error_mid_td instead of
last_td_was_short, so that the TD is only freed on the final event,
not on the first one.

The spec is clear that we should only free TRBs when the xHC is done
with them. Maybe it wouldn't be a problem in this case, and it surely
wouldn't be worse than what happens with Etron today, but IMO it could
be a real (even if rare) problem in other cases when this flag is used,
so I would rather remove the flag and handle short packets as per spec.

Regards,
Michal

Re: [PATCH v2 5/5] xhci: Correct handling of one-TRB isoc TD on Etron xHCI host

[Kuangyi Chiang]

Hi,

Thank you for the review.

Michał Pecio <michal.pecio@gmail.com> 於 2024年10月28日 週一 下午5:54寫道：
>
> Hi,
>
> That's a bug I'm familiar with.
>
> > Unplugging a USB3.0 webcam while streaming results in errors
> > like this
>
> Not only unplugging but also any random error due to EMI or bad cable.
>
> > If an error is detected while processing an one-TRB isoc TD,
> > the Etron xHC generates two transfer events for the TRB that
> > the error was detected on. The first event is "USB Transcation
> > Error", and the second event is "Success".
>
> IIRC, it wasn't just Transaction Errors but any sort of error, like
> Babble or Bandwidth Overrun. But not sure about Missed Service, etc.
>
> And IIRC I confirmed that it was *not* the case on Short Packet.

Yes, it is not.

>
> Also, I'm 99% sure the problem is not limited to one-TRB TDs, but
> it occurs every time there is an error on the last TRB of any TD.

Yes, this can happen, I didn't account for this scenario.

>
> > As a solution, we can set the flag after the first error event
> > and don't print the error message after the second event if the
> > flag is set.
>
> Yes, but I think it would be better to use error_mid_td instead of
> last_td_was_short, so that the TD is only freed on the final event,
> not on the first one.
>
> The spec is clear that we should only free TRBs when the xHC is done
> with them. Maybe it wouldn't be a problem in this case, and it surely
> wouldn't be worse than what happens with Etron today, but IMO it could
> be a real (even if rare) problem in other cases when this flag is used,
> so I would rather remove the flag and handle short packets as per spec.

Thank you for the explanation and suggestion. Maybe I should start
trying to use error_mid_td to solve this problem.

>
> Regards,
> Michal

Thanks,
Kuangyi Chiang

Re: [PATCH v2 5/5] xhci: Correct handling of one-TRB isoc TD on Etron xHCI host

[Mathias Nyman]

On 28.10.2024 4.53, Kuangyi Chiang wrote:
> Unplugging a USB3.0 webcam while streaming results in errors
> like this:
> 
> [ 132.646387] xhci_hcd 0000:03:00.0: ERROR Transfer event TRB DMA ptr not part of current TD ep_index 18 comp_code 13
> [ 132.646446] xhci_hcd 0000:03:00.0: Looking for event-dma 000000002fdf8630 trb-start 000000002fdf8640 trb-end 000000002fdf8650 seg-start 000000002fdf8000 seg-end 000000002fdf8ff0
> [ 132.646560] xhci_hcd 0000:03:00.0: ERROR Transfer event TRB DMA ptr not part of current TD ep_index 18 comp_code 13
> [ 132.646568] xhci_hcd 0000:03:00.0: Looking for event-dma 000000002fdf8660 trb-start 000000002fdf8670 trb-end 000000002fdf8670 seg-start 000000002fdf8000 seg-end 000000002fdf8ff0
> 
> If an error is detected while processing an one-TRB isoc TD,
> the Etron xHC generates two transfer events for the TRB that
> the error was detected on. The first event is "USB Transcation
> Error", and the second event is "Success".
> 
> The xHCI driver will handle the TD after the first event and
> remove it from its internal list, and then print an "Transfer
> event TRB DMA ptr not part of current TD" error message after
> the second event.
> 
> As a solution, we can set the flag after the first error event
> and don't print the error message after the second event if
> the flag is set.
> 
> Commit ad808333d820 ("Intel xhci: Ignore spurious successful
> event.") implements a similar mechanism that we can reuse to
> solve this problem since short transfer and transfer error
> doesn't occur concurrently. Also, rename the flag to make it
> more meaningful.
> 
> Check if the XHCI_ETRON_HOST quirk flag is set before invoking
> the workaround in process_isoc_td().
> 
> This patch doesn't affect other host controllers that have the
> XHCI_SPURIOUS_SUCCESS quirk flag applied.
> 
> Signed-off-by: Kuangyi Chiang <ki.chiang65@gmail.com>

I'm leaving this out of the series due to both ongoing discussion about
this patch, and because it conflicts with another series touching
handle_tx_event()

All other patches in series are added

Thanks
Mathias