[PATCH v4] usb: xhci: quirk for data loss in ISOC transfers

[PATCH v4] usb: xhci: quirk for data loss in ISOC transfers

[Raju Rangoju]

During the High-Speed Isochronous Audio transfers, xHCI
controller on certain AMD platforms experiences momentary data
loss. This results in Missed Service Errors (MSE) being
generated by the xHCI.

The root cause of the MSE is attributed to the ISOC OUT endpoint
being omitted from scheduling. This can happen either when an IN
endpoint with a 64ms service interval is pre-scheduled prior to
the ISOC OUT endpoint or when the interval of the ISOC OUT
endpoint is shorter than that of the IN endpoint. Consequently,
the OUT service is neglected when an IN endpoint with a service
interval exceeding 32ms is scheduled concurrently (every 64ms in
this scenario).

This issue is particularly seen on certain older AMD platforms.
To mitigate this problem, it is recommended to adjust the service
interval of the IN endpoint to not exceed 32ms (interval 8). This
adjustment ensures that the OUT endpoint will not be bypassed,
even if a smaller interval value is utilized.

Cc: stable@vger.kernel.org
Signed-off-by: Raju Rangoju <Raju.Rangoju@amd.com>
---
Changes since v3:
 - Bump up the enum number XHCI_LIMIT_ENDPOINT_INTERVAL_9

Changes since v2:
 - added stable tag to backport to all stable kernels

Changes since v1:
 - replaced hex values with pci device names
 - corrected the commit message

 drivers/usb/host/xhci-mem.c |  5 +++++
 drivers/usb/host/xhci-pci.c | 25 +++++++++++++++++++++++++
 drivers/usb/host/xhci.h     |  1 +
 3 files changed, 31 insertions(+)

diff --git a/drivers/usb/host/xhci-mem.c b/drivers/usb/host/xhci-mem.c
index 92703efda1f7..d3182ba98788 100644
--- a/drivers/usb/host/xhci-mem.c
+++ b/drivers/usb/host/xhci-mem.c
@@ -1420,6 +1420,11 @@ int xhci_endpoint_init(struct xhci_hcd *xhci,
 	/* Periodic endpoint bInterval limit quirk */
 	if (usb_endpoint_xfer_int(&ep->desc) ||
 	    usb_endpoint_xfer_isoc(&ep->desc)) {
+		if ((xhci->quirks & XHCI_LIMIT_ENDPOINT_INTERVAL_9) &&
+		    usb_endpoint_xfer_int(&ep->desc) &&
+		    interval >= 9) {
+			interval = 8;
+		}
 		if ((xhci->quirks & XHCI_LIMIT_ENDPOINT_INTERVAL_7) &&
 		    udev->speed >= USB_SPEED_HIGH &&
 		    interval >= 7) {
diff --git a/drivers/usb/host/xhci-pci.c b/drivers/usb/host/xhci-pci.c
index 2d1e205c14c6..d23884afdf3f 100644
--- a/drivers/usb/host/xhci-pci.c
+++ b/drivers/usb/host/xhci-pci.c
@@ -69,12 +69,22 @@
 #define PCI_DEVICE_ID_INTEL_TITAN_RIDGE_4C_XHCI		0x15ec
 #define PCI_DEVICE_ID_INTEL_TITAN_RIDGE_DD_XHCI		0x15f0
 
+#define PCI_DEVICE_ID_AMD_ARIEL_TYPEC_XHCI		0x13ed
+#define PCI_DEVICE_ID_AMD_ARIEL_TYPEA_XHCI		0x13ee
+#define PCI_DEVICE_ID_AMD_STARSHIP_XHCI			0x148c
+#define PCI_DEVICE_ID_AMD_FIREFLIGHT_15D4_XHCI		0x15d4
+#define PCI_DEVICE_ID_AMD_FIREFLIGHT_15D5_XHCI		0x15d5
+#define PCI_DEVICE_ID_AMD_RAVEN_15E0_XHCI		0x15e0
+#define PCI_DEVICE_ID_AMD_RAVEN_15E1_XHCI		0x15e1
+#define PCI_DEVICE_ID_AMD_RAVEN2_XHCI			0x15e5
 #define PCI_DEVICE_ID_AMD_RENOIR_XHCI			0x1639
 #define PCI_DEVICE_ID_AMD_PROMONTORYA_4			0x43b9
 #define PCI_DEVICE_ID_AMD_PROMONTORYA_3			0x43ba
 #define PCI_DEVICE_ID_AMD_PROMONTORYA_2			0x43bb
 #define PCI_DEVICE_ID_AMD_PROMONTORYA_1			0x43bc
 
+#define PCI_DEVICE_ID_ATI_NAVI10_7316_XHCI		0x7316
+
 #define PCI_DEVICE_ID_ASMEDIA_1042_XHCI			0x1042
 #define PCI_DEVICE_ID_ASMEDIA_1042A_XHCI		0x1142
 #define PCI_DEVICE_ID_ASMEDIA_1142_XHCI			0x1242
@@ -278,6 +288,21 @@ static void xhci_pci_quirks(struct device *dev, struct xhci_hcd *xhci)
 	if (pdev->vendor == PCI_VENDOR_ID_NEC)
 		xhci->quirks |= XHCI_NEC_HOST;
 
+	if (pdev->vendor == PCI_VENDOR_ID_AMD &&
+	    (pdev->device == PCI_DEVICE_ID_AMD_ARIEL_TYPEC_XHCI ||
+	     pdev->device == PCI_DEVICE_ID_AMD_ARIEL_TYPEA_XHCI ||
+	     pdev->device == PCI_DEVICE_ID_AMD_STARSHIP_XHCI ||
+	     pdev->device == PCI_DEVICE_ID_AMD_FIREFLIGHT_15D4_XHCI ||
+	     pdev->device == PCI_DEVICE_ID_AMD_FIREFLIGHT_15D5_XHCI ||
+	     pdev->device == PCI_DEVICE_ID_AMD_RAVEN_15E0_XHCI ||
+	     pdev->device == PCI_DEVICE_ID_AMD_RAVEN_15E1_XHCI ||
+	     pdev->device == PCI_DEVICE_ID_AMD_RAVEN2_XHCI))
+		xhci->quirks |= XHCI_LIMIT_ENDPOINT_INTERVAL_9;
+
+	if (pdev->vendor == PCI_VENDOR_ID_ATI &&
+	    pdev->device == PCI_DEVICE_ID_ATI_NAVI10_7316_XHCI)
+		xhci->quirks |= XHCI_LIMIT_ENDPOINT_INTERVAL_9;
+
 	if (pdev->vendor == PCI_VENDOR_ID_AMD && xhci->hci_version == 0x96)
 		xhci->quirks |= XHCI_AMD_0x96_HOST;
 
diff --git a/drivers/usb/host/xhci.h b/drivers/usb/host/xhci.h
index 4914f0a10cff..36b77d3c0e7b 100644
--- a/drivers/usb/host/xhci.h
+++ b/drivers/usb/host/xhci.h
@@ -1633,6 +1633,7 @@ struct xhci_hcd {
 #define XHCI_WRITE_64_HI_LO	BIT_ULL(47)
 #define XHCI_CDNS_SCTX_QUIRK	BIT_ULL(48)
 #define XHCI_ETRON_HOST	BIT_ULL(49)
+#define XHCI_LIMIT_ENDPOINT_INTERVAL_9 BIT_ULL(50)
 
 	unsigned int		num_active_eps;
 	unsigned int		limit_active_eps;
-- 
2.34.1

Re: [PATCH v4] usb: xhci: quirk for data loss in ISOC transfers

[Mathias Nyman]

On 27.1.2025 14.06, Raju Rangoju wrote:
> During the High-Speed Isochronous Audio transfers, xHCI
> controller on certain AMD platforms experiences momentary data
> loss. This results in Missed Service Errors (MSE) being
> generated by the xHCI.
> 
> The root cause of the MSE is attributed to the ISOC OUT endpoint
> being omitted from scheduling. This can happen either when an IN
> endpoint with a 64ms service interval is pre-scheduled prior to
> the ISOC OUT endpoint or when the interval of the ISOC OUT
> endpoint is shorter than that of the IN endpoint. Consequently,
> the OUT service is neglected when an IN endpoint with a service
> interval exceeding 32ms is scheduled concurrently (every 64ms in
> this scenario).
> 
> This issue is particularly seen on certain older AMD platforms.
> To mitigate this problem, it is recommended to adjust the service
> interval of the IN endpoint to not exceed 32ms (interval 8). This
> adjustment ensures that the OUT endpoint will not be bypassed,
> even if a smaller interval value is utilized.
> 
> Cc: stable@vger.kernel.org
> Signed-off-by: Raju Rangoju <Raju.Rangoju@amd.com>
> ---
> Changes since v3:
>   - Bump up the enum number XHCI_LIMIT_ENDPOINT_INTERVAL_9
> 
> Changes since v2:
>   - added stable tag to backport to all stable kernels
> 
> Changes since v1:
>   - replaced hex values with pci device names
>   - corrected the commit message
> 
>   drivers/usb/host/xhci-mem.c |  5 +++++
>   drivers/usb/host/xhci-pci.c | 25 +++++++++++++++++++++++++
>   drivers/usb/host/xhci.h     |  1 +
>   3 files changed, 31 insertions(+)
> 
> diff --git a/drivers/usb/host/xhci-mem.c b/drivers/usb/host/xhci-mem.c
> index 92703efda1f7..d3182ba98788 100644
> --- a/drivers/usb/host/xhci-mem.c
> +++ b/drivers/usb/host/xhci-mem.c
> @@ -1420,6 +1420,11 @@ int xhci_endpoint_init(struct xhci_hcd *xhci,
>   	/* Periodic endpoint bInterval limit quirk */
>   	if (usb_endpoint_xfer_int(&ep->desc) ||
>   	    usb_endpoint_xfer_isoc(&ep->desc)) {
> +		if ((xhci->quirks & XHCI_LIMIT_ENDPOINT_INTERVAL_9) &&
> +		    usb_endpoint_xfer_int(&ep->desc) &&
> +		    interval >= 9) {
> +			interval = 8;

Commit message describes this as an issue triggered by High-Speed Isoc In
endpoints that have interval larger than 32ms.

This code limits interval to 32ms for Interrupt endpoints (any speed),
should it be isoc instead?

Are Full-/Low-speed devices really also affected?

Thanks
Mathias

Re: [PATCH v4] usb: xhci: quirk for data loss in ISOC transfers

[Rangoju, Raju]

On 1/29/2025 6:08 PM, Mathias Nyman wrote:

Hi Mathias,

Thanks for reviewing the patch and apologies for delayed response.

> On 27.1.2025 14.06, Raju Rangoju wrote:
>> During the High-Speed Isochronous Audio transfers, xHCI
>> controller on certain AMD platforms experiences momentary data
>> loss. This results in Missed Service Errors (MSE) being
>> generated by the xHCI.
>>
>> The root cause of the MSE is attributed to the ISOC OUT endpoint
>> being omitted from scheduling. This can happen either when an IN
>> endpoint with a 64ms service interval is pre-scheduled prior to
>> the ISOC OUT endpoint or when the interval of the ISOC OUT
>> endpoint is shorter than that of the IN endpoint. Consequently,
>> the OUT service is neglected when an IN endpoint with a service
>> interval exceeding 32ms is scheduled concurrently (every 64ms in
>> this scenario).
>>
>> This issue is particularly seen on certain older AMD platforms.
>> To mitigate this problem, it is recommended to adjust the service
>> interval of the IN endpoint to not exceed 32ms (interval 8). This
>> adjustment ensures that the OUT endpoint will not be bypassed,
>> even if a smaller interval value is utilized.
>>
>> Cc: stable@vger.kernel.org
>> Signed-off-by: Raju Rangoju <Raju.Rangoju@amd.com>
>> ---
>> Changes since v3:
>>   - Bump up the enum number XHCI_LIMIT_ENDPOINT_INTERVAL_9
>>
>> Changes since v2:
>>   - added stable tag to backport to all stable kernels
>>
>> Changes since v1:
>>   - replaced hex values with pci device names
>>   - corrected the commit message
>>
>>   drivers/usb/host/xhci-mem.c |  5 +++++
>>   drivers/usb/host/xhci-pci.c | 25 +++++++++++++++++++++++++
>>   drivers/usb/host/xhci.h     |  1 +
>>   3 files changed, 31 insertions(+)
>>
>> diff --git a/drivers/usb/host/xhci-mem.c b/drivers/usb/host/xhci-mem.c
>> index 92703efda1f7..d3182ba98788 100644
>> --- a/drivers/usb/host/xhci-mem.c
>> +++ b/drivers/usb/host/xhci-mem.c
>> @@ -1420,6 +1420,11 @@ int xhci_endpoint_init(struct xhci_hcd *xhci,
>>       /* Periodic endpoint bInterval limit quirk */
>>       if (usb_endpoint_xfer_int(&ep->desc) ||
>>           usb_endpoint_xfer_isoc(&ep->desc)) {
>> +        if ((xhci->quirks & XHCI_LIMIT_ENDPOINT_INTERVAL_9) &&
>> +            usb_endpoint_xfer_int(&ep->desc) &&
>> +            interval >= 9) {
>> +            interval = 8;
> 
> Commit message describes this as an issue triggered by High-Speed Isoc In
> endpoints that have interval larger than 32ms.
>
> This code limits interval to 32ms for Interrupt endpoints (any speed),
> should it be isoc instead?

The affected transfer is ISOC. However, due to INT EP service interval 
of 64ms causing the ISO EP to be skipped, the WA is to reduce the INT EP 
service to be less than 64ms (32ms).
> 
> Are Full-/Low-speed devices really also affected?
> 
No, Full-/Low-speed devices are not affected. Thanks for reviewing, I'll 
re-spin the patch adding this change.

> Thanks
> Mathias
> 

Re: [PATCH v4] usb: xhci: quirk for data loss in ISOC transfers

[Michał Pecio]

> >> The root cause of the MSE is attributed to the ISOC OUT endpoint
> >> being omitted from scheduling. This can happen either when an IN
> >> endpoint with a 64ms service interval is pre-scheduled prior to
> >> the ISOC OUT endpoint or when the interval of the ISOC OUT
> >> endpoint is shorter than that of the IN endpoint.

To me this reads like the condition is

(IN ESIT >= 64ms && IN pre-scheduled before OUT) ||
(OUT ESIT < IN ESIT)

but I suspect it really is

(IN ESIT >= 64ms) &&
(IN pre-scheduled before OUT || OUT ESIT < IN ESIT)

because otherwise this workaround wouldn't really help:
ISOC OUT ESIT < INT IN ESIT is almost always true in practice.


Moving "either" later maybe makes it more clear:

This can happen when an IN endpoint with a 64ms service interval either
is pre-scheduled prior to the ISOC OUT endpoint or the interval of the
ISOC OUT endpoint is shorter than that of the IN endpoint.

> > This code limits interval to 32ms for Interrupt endpoints (any
> > speed), should it be isoc instead?
>
> The affected transfer is ISOC. However, due to INT EP service
> interval of 64ms causing the ISO EP to be skipped, the WA is to
> reduce the INT EP service to be less than 64ms (32ms).

What if there is an ISOC IN endpoint with 64ms ESIT? I haven't yet seen
such a slow isoc endpoint, but I think they are allowed by the spec.
Your changelog suggests any periodic IN endpoint can trigger this bug.

> > Are Full-/Low-speed devices really also affected?
> > 
> No, Full-/Low-speed devices are not affected.

The interesting question here is whether LS/FS devices with long
interval IN endpoints can disrupt a HS OUT endpoint or not, because
the patch solves the problem from the IN endpoint's side.

(I assume that SS probably has no effect on HS schedule.)

Regards,
Michal

Re: [PATCH v4] usb: xhci: quirk for data loss in ISOC transfers

[Rangoju, Raju]

On 3/26/2025 12:17 PM, Michał Pecio wrote:
>>>> The root cause of the MSE is attributed to the ISOC OUT endpoint
>>>> being omitted from scheduling. This can happen either when an IN
>>>> endpoint with a 64ms service interval is pre-scheduled prior to
>>>> the ISOC OUT endpoint or when the interval of the ISOC OUT
>>>> endpoint is shorter than that of the IN endpoint.
> 
> To me this reads like the condition is
> 
> (IN ESIT >= 64ms && IN pre-scheduled before OUT) ||
> (OUT ESIT < IN ESIT)
> 
> but I suspect it really is
> 
> (IN ESIT >= 64ms) &&
> (IN pre-scheduled before OUT || OUT ESIT < IN ESIT)
> 
> because otherwise this workaround wouldn't really help:
> ISOC OUT ESIT < INT IN ESIT is almost always true in practice.
> 
> 
> Moving "either" later maybe makes it more clear:
> 
> This can happen when an IN endpoint with a 64ms service interval either
> is pre-scheduled prior to the ISOC OUT endpoint or the interval of the
> ISOC OUT endpoint is shorter than that of the IN endpoint.
>

Hi Michal,
Sure, I'll take care of this in commit message when re spinning.

>>> This code limits interval to 32ms for Interrupt endpoints (any
>>> speed), should it be isoc instead?
>>
>> The affected transfer is ISOC. However, due to INT EP service
>> interval of 64ms causing the ISO EP to be skipped, the WA is to
>> reduce the INT EP service to be less than 64ms (32ms).
> 
> What if there is an ISOC IN endpoint with 64ms ESIT? I haven't yet seen
> such a slow isoc endpoint, but I think they are allowed by the spec.
> Your changelog suggests any periodic IN endpoint can trigger this bug.
>

If such an endpoint is implemented, it could theoretically contribute to 
scheduling conflicts similar to those caused by INT endpoints in this 
context. However, our observations and testing on affected platforms 
primarily involved periodic IN endpoints with service intervals greater 
than 32ms interfering with ISOC OUT endpoints.


>>> Are Full-/Low-speed devices really also affected?
>>>
>> No, Full-/Low-speed devices are not affected.
> 
> The interesting question here is whether LS/FS devices with long
> interval IN endpoints can disrupt a HS OUT endpoint or not, because
> the patch solves the problem from the IN endpoint's side.
>
I'm not completely sure about this corner case if HS OUT endpoints can 
inadvertently get affected when co-existing with long-interval LS/FS IN 
endpoints. Our IP vendor confirmed that LS/FS devices are not affected.

> (I assume that SS probably has no effect on HS schedule.)
> 
> Regards,
> Michal

Re: [PATCH v4] usb: xhci: quirk for data loss in ISOC transfers

[Michał Pecio]

On Thu, 27 Mar 2025 12:08:53 +0530, Rangoju, Raju wrote:
> > What if there is an ISOC IN endpoint with 64ms ESIT? I haven't yet
> > seen such a slow isoc endpoint, but I think they are allowed by the
> > spec. Your changelog suggests any periodic IN endpoint can trigger
> > this bug. 
> 
> If such an endpoint is implemented, it could theoretically contribute
> to scheduling conflicts similar to those caused by INT endpoints in
> this context. However, our observations and testing on affected
> platforms primarily involved periodic IN endpoints with service
> intervals greater than 32ms interfering with ISOC OUT endpoints.

In such case it would make sense to drop the check for
usb_endpoint_xfer_int(&ep->desc)
and rely on existing (xfer_int || xfer_isoc) in the outer 'if'.

> I'm not completely sure about this corner case if HS OUT endpoints
> can inadvertently get affected when co-existing with long-interval
> LS/FS IN endpoints. Our IP vendor confirmed that LS/FS devices are
> not affected.

There is also a third case of a FS device behind an external HS hub.
The device will look like FS to this code here, but the xHC will need
to schedule HS transactions to service it.

Regards,
Michal

Re: [PATCH v4] usb: xhci: quirk for data loss in ISOC transfers

[Rangoju, Raju]

On 3/27/2025 3:04 PM, Michał Pecio wrote:
> On Thu, 27 Mar 2025 12:08:53 +0530, Rangoju, Raju wrote:
>>> What if there is an ISOC IN endpoint with 64ms ESIT? I haven't yet
>>> seen such a slow isoc endpoint, but I think they are allowed by the
>>> spec. Your changelog suggests any periodic IN endpoint can trigger
>>> this bug.
>>
>> If such an endpoint is implemented, it could theoretically contribute
>> to scheduling conflicts similar to those caused by INT endpoints in
>> this context. However, our observations and testing on affected
>> platforms primarily involved periodic IN endpoints with service
>> intervals greater than 32ms interfering with ISOC OUT endpoints.
> 
> In such case it would make sense to drop the check for
> usb_endpoint_xfer_int(&ep->desc)
> and rely on existing (xfer_int || xfer_isoc) in the outer 'if'.
>

Got it. I'll address this in subsequent patch.

>> I'm not completely sure about this corner case if HS OUT endpoints
>> can inadvertently get affected when co-existing with long-interval
>> LS/FS IN endpoints. Our IP vendor confirmed that LS/FS devices are
>> not affected.
> 
> There is also a third case of a FS device behind an external HS hub.
> The device will look like FS to this code here, but the xHC will need
> to schedule HS transactions to service it.

In that case, the original code I provided in this patch doesn't include 
a check for 'udev->speed' and is applicable to FS devices as well. I 
think this code should remain unchanged to properly address the third 
scenario you mentioned.

> 
> Regards,
> Michal