[PATCH v2] usb: xhci: fixes lost of data for xHCI Cadence Controllers

[PATCH v2] usb: xhci: fixes lost of data for xHCI Cadence Controllers

[Pawel Laszczak]

Stream endpoint can skip part of TD during next transfer initialization
after beginning stopped during active stream data transfer.
The Set TR Dequeue Pointer command does not clear all internal
transfer-related variables that position stream endpoint on transfer ring.

USB Controller stores all endpoint state information within RsvdO fields
inside endpoint context structure. For stream endpoints, all relevant
information regarding particular StreamID is stored within corresponding
Stream Endpoint context.
Whenever driver wants to stop stream endpoint traffic, it invokes
Stop Endpoint command which forces the controller to dump all endpoint
state-related variables into RsvdO spaces into endpoint context and stream
endpoint context. Whenever driver wants to reinitialize endpoint starting
point on Transfer Ring, it uses the Set TR Dequeue Pointer command
to update dequeue pointer for particular stream in Stream Endpoint
Context. When stream endpoint is forced to stop active transfer in the
middle of TD, it dumps an information about TRB bytes left in RsvdO fields
in Stream Endpoint Context which will be used in next transfer
initialization to designate starting point for XDMA. This field is not
cleared during Set TR Dequeue Pointer command which causes XDMA to skip
over transfer ring and leads to data loss on stream pipe.

Patch fixes this by clearing out all RsvdO fields before initializing new
transfer via that StreamID.

Field Rsvd0 is reserved field, so patch should not have impact for other
xHCI controllers.

Fixes: 3d82904559f4 ("usb: cdnsp: cdns3 Add main part of Cadence USBSSP DRD Driver")
cc: <stable@vger.kernel.org>
Signed-off-by: Pawel Laszczak <pawell@cadence.com>

---
Changelog:
v2:
- removed restoring of EDTLA field 

 drivers/usb/host/xhci-ring.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
index 1dde53f6eb31..e5e1d665adab 100644
--- a/drivers/usb/host/xhci-ring.c
+++ b/drivers/usb/host/xhci-ring.c
@@ -1386,6 +1386,16 @@ static void xhci_handle_cmd_set_deq(struct xhci_hcd *xhci, int slot_id,
 			struct xhci_stream_ctx *ctx =
 				&ep->stream_info->stream_ctx_array[stream_id];
 			deq = le64_to_cpu(ctx->stream_ring) & SCTX_DEQ_MASK;
+
+			/*
+			 * Existing Cadence xHCI controllers store some endpoint state information
+			 * within Rsvd0 fields of Stream Endpoint context. This field is not
+			 * cleared during Set TR Dequeue Pointer command which causes XDMA to skip
+			 * over transfer ring and leads to data loss on stream pipe.
+			 * To fix this issue driver must clear Rsvd0 field.
+			 */
+			ctx->reserved[0] = 0;
+			ctx->reserved[1] = 0;
 		} else {
 			deq = le64_to_cpu(ep_ctx->deq) & ~EP_CTX_CYCLE_MASK;
 		}
-- 
2.43.0

Re: [PATCH v2] usb: xhci: fixes lost of data for xHCI Cadence Controllers

[Mathias Nyman]

On 21.8.2024 9.01, Pawel Laszczak wrote:
> Stream endpoint can skip part of TD during next transfer initialization
> after beginning stopped during active stream data transfer.
> The Set TR Dequeue Pointer command does not clear all internal
> transfer-related variables that position stream endpoint on transfer ring.
> 
> USB Controller stores all endpoint state information within RsvdO fields
> inside endpoint context structure. For stream endpoints, all relevant
> information regarding particular StreamID is stored within corresponding
> Stream Endpoint context.
> Whenever driver wants to stop stream endpoint traffic, it invokes
> Stop Endpoint command which forces the controller to dump all endpoint
> state-related variables into RsvdO spaces into endpoint context and stream
> endpoint context. Whenever driver wants to reinitialize endpoint starting
> point on Transfer Ring, it uses the Set TR Dequeue Pointer command
> to update dequeue pointer for particular stream in Stream Endpoint
> Context. When stream endpoint is forced to stop active transfer in the
> middle of TD, it dumps an information about TRB bytes left in RsvdO fields
> in Stream Endpoint Context which will be used in next transfer
> initialization to designate starting point for XDMA. This field is not
> cleared during Set TR Dequeue Pointer command which causes XDMA to skip
> over transfer ring and leads to data loss on stream pipe.
> 
> Patch fixes this by clearing out all RsvdO fields before initializing new
> transfer via that StreamID.
> 
> Field Rsvd0 is reserved field, so patch should not have impact for other
> xHCI controllers.
> 
> Fixes: 3d82904559f4 ("usb: cdnsp: cdns3 Add main part of Cadence USBSSP DRD Driver")
> cc: <stable@vger.kernel.org>
> Signed-off-by: Pawel Laszczak <pawell@cadence.com>

Thanks,

Code looks good but maybe we should skip adding this to stable until we are really
sure modifying the RsvdO fields for _all_ host controllers doesn't cause any issues.

I simplified and changed the commit message, is the following ok with you:

usb: xhci: fix loss of data on Cadence xHC

Streams should flush their TRB cache, re-read TRBs, and start executing
TRBs from the beginning of the new dequeue pointer after a 'Set TR
Dequeue Pointer' command.

Cadence controllers may fail to start from the beginning of the dequeue
TRB as it doesn't clear the Opaque 'RsvdO' field of the stream context
during 'Set TR Dequeue' command.
This stream context area is where xHC stores information about the last
partially executed TD when a stream is stopped.
xHC uses this information to resume the transfer where it left mid TD,
when the stream is restarted.

Patch fixes this by clearing out all RsvdO fields before initializing new
Stream transfer using a 'Set TR Dequeue Pointer' command.

Field RsvdO is reserved field, so patch should not have impact on other
xHCI controllers, but don't add this patch to stable kernels yet
before it has worked flawlessly upstream on different hosts for a while.

[simplify and edit commit message -Mathias]

Signed-off-by: Pawel Laszczak <pawell@cadence.com>
Signed-off-by: Mathias Nyman <mathias.nyman@linux.intel.com>

Re: [PATCH v2] usb: xhci: fixes lost of data for xHCI Cadence Controllers

[Michał Pecio]

Hi,

> Field Rsvd0 is reserved field, so patch should not have impact for
> other xHCI controllers.
Wait, is this a case of Linux failing to zero-initialize something that
should be zero initialized before use (why not explain it as such?), or
are you suggesting monkeying with internal xHC data at run time, in area
which is known to actually be used by at least one implementation?

There is no mention of Rsvd0 in the xHCI spec, did you mean RsvdO?

Reserved and Opaque, 
For exclusive use by the xHC.
Software shall *not* write this, unless allowed by the vendor.

Cadence isn't the only xHC vendor...

(There is no mention of "Stream Endpoint Context" either, so maybe you
meant "Stream Context" or "Endpoint Context"...)

Regards,
Michal

Re: [PATCH v2] usb: xhci: fixes lost of data for xHCI Cadence Controllers

[Mathias Nyman]

On 30.8.2024 18.45, Michał Pecio wrote:
> Hi,
> 
>> Field Rsvd0 is reserved field, so patch should not have impact for
>> other xHCI controllers.
> Wait, is this a case of Linux failing to zero-initialize something that
> should be zero initialized before use (why not explain it as such?), or
> are you suggesting monkeying with internal xHC data at run time, in area
> which is known to actually be used by at least one implementation?
> 

Patch is monkeying with internal xHC RsvdO field.


> There is no mention of Rsvd0 in the xHCI spec, did you mean RsvdO?
> 
> Reserved and Opaque,
> For exclusive use by the xHC.
> Software shall *not* write this, unless allowed by the vendor.
> 
> Cadence isn't the only xHC vendor...
> 

Makes sense,  Pawel Laszczak, could you make this patch Cadence
specific.

Thanks
Mathias

RE: [PATCH v2] usb: xhci: fixes lost of data for xHCI Cadence Controllers

[Pawel Laszczak]

Hi,

>On 30.8.2024 18.45, Michał Pecio wrote:
>> Hi,
>>
>>> Field Rsvd0 is reserved field, so patch should not have impact for
>>> other xHCI controllers.
>> Wait, is this a case of Linux failing to zero-initialize something
>> that should be zero initialized before use (why not explain it as
>> such?), or are you suggesting monkeying with internal xHC data at run
>> time, in area which is known to actually be used by at least one
>implementation?
>>
>
>Patch is monkeying with internal xHC RsvdO field.
>
>
>> There is no mention of Rsvd0 in the xHCI spec, did you mean RsvdO?
>>
>> Reserved and Opaque,
>> For exclusive use by the xHC.
>> Software shall *not* write this, unless allowed by the vendor.
>>
>> Cadence isn't the only xHC vendor...
>>
>
>Makes sense,  Pawel Laszczak, could you make this patch Cadence specific.

I understand your concerns. I will prepare new patch. 
For this purpose I'm going to add some extra quirk definition.

Thanks
Pawel

>
>Thanks
>Mathias