| Message ID | PH7PR07MB9538B91A187B4EB8654BB2EBDD8C2@PH7PR07MB9538.namprd07.prod.outlook.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | usb: xhci: fixes lost of data for xHCI Cadence Controllers | expand |
On 19.8.2024 7.58, 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. xHC should clear the EDTLA field when processing a Set TR Dequeue Pointer command: xhci spec v1.2, section 4.6.10 Set TR Dequeue Pointer: "The xHC shall perform the following operations when setting a ring address: ... Clear any prior transfer state, e.g. setting the EDTLA to 0, clearing any partially completed USB2 split transactions, etc." > > Patch fixes this by clearing out all RsvdO fields before initializing new > transfer via that StreamID. Looks like patch also writes edtl back to ctx->reserved[0], is there a reason for this? > > 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> > --- > drivers/usb/host/xhci-ring.c | 13 +++++++++++++ > 1 file changed, 13 insertions(+) > > diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c > index 1dde53f6eb31..7fc1c4efcae2 100644 > --- a/drivers/usb/host/xhci-ring.c > +++ b/drivers/usb/host/xhci-ring.c > @@ -1385,7 +1385,20 @@ static void xhci_handle_cmd_set_deq(struct xhci_hcd *xhci, int slot_id, > if (ep->ep_state & EP_HAS_STREAMS) { > struct xhci_stream_ctx *ctx = > &ep->stream_info->stream_ctx_array[stream_id]; > + u32 edtl; > + > deq = le64_to_cpu(ctx->stream_ring) & SCTX_DEQ_MASK; > + edtl = EVENT_TRB_LEN(le32_to_cpu(ctx->reserved[1])); Isn't edtl in reserved[0], not in reserved[1]? Also unclear why we read it at all, it should be set to 0 by controller, right? > + > + /* > + * Existing Cadence xHCI controllers store some endpoint state information > + * within Rsvd0 fields of Stream Endpoint context. This field is not Aren't these fields RsvdO (Reserved and Opaque)? Driver shouldn't normally touch these fields, they are used by xHC as temporary workspace. > + * 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[1] = 0; > + ctx->reserved[0] = cpu_to_le32(edtl); why are we writing back edtl?, also note that it's read from ctx->reserved[1] above, and now written to back to ctx->reserved[0]. I understood that issue was those RsvdO fields needs to be cleared manually for this host as hardware fails to clear them during a "Set TR Dequeue Pointer" command. Am I misunderstanding something? Thanks Mathias
>> 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. > >xHC should clear the EDTLA field when processing a Set TR Dequeue Pointer >command: > >xhci spec v1.2, section 4.6.10 Set TR Dequeue Pointer: >"The xHC shall perform the following operations when setting a ring address: > ... >Clear any prior transfer state, e.g. setting the EDTLA to 0, clearing any partially >completed USB2 split transactions, etc." > >> >> Patch fixes this by clearing out all RsvdO fields before initializing >> new transfer via that StreamID. > >Looks like patch also writes edtl back to ctx->reserved[0], is there a reason for >this? > >> >> 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> >> --- >> drivers/usb/host/xhci-ring.c | 13 +++++++++++++ >> 1 file changed, 13 insertions(+) >> >> diff --git a/drivers/usb/host/xhci-ring.c >> b/drivers/usb/host/xhci-ring.c index 1dde53f6eb31..7fc1c4efcae2 100644 >> --- a/drivers/usb/host/xhci-ring.c >> +++ b/drivers/usb/host/xhci-ring.c >> @@ -1385,7 +1385,20 @@ static void xhci_handle_cmd_set_deq(struct >xhci_hcd *xhci, int slot_id, >> if (ep->ep_state & EP_HAS_STREAMS) { >> struct xhci_stream_ctx *ctx = >> &ep->stream_info- >>stream_ctx_array[stream_id]; >> + u32 edtl; >> + >> deq = le64_to_cpu(ctx->stream_ring) & >SCTX_DEQ_MASK; >> + edtl = EVENT_TRB_LEN(le32_to_cpu(ctx- >>reserved[1])); > >Isn't edtl in reserved[0], not in reserved[1]? > >Also unclear why we read it at all, it should be set to 0 by controller, right? > >> + >> + /* >> + * Existing Cadence xHCI controllers store some >endpoint state information >> + * within Rsvd0 fields of Stream Endpoint context. This >field is >> +not > >Aren't these fields RsvdO (Reserved and Opaque)? >Driver shouldn't normally touch these fields, they are used by xHC as >temporary workspace. Yes, it should be. This a real issue that occurred during testing UASP disk and this workaround fixes it, so it's looks like that this Rsvd0 field can be changed. > >> + * 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[1] = 0; >> + ctx->reserved[0] = cpu_to_le32(edtl); > >why are we writing back edtl?, also note that it's read from ctx->reserved[1] >above, and now written to back to ctx->reserved[0]. > >I understood that issue was those RsvdO fields needs to be cleared manually >for this host as hardware fails to clear them during a "Set TR Dequeue >Pointer" command. > >Am I misunderstanding something? Yes, you understand it so well. In fact edtl should be zeroed and simple clearing of reserved[0] and reserved[1] should be sufficient. Thanks Pawel > >Thanks >Mathias
diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c index 1dde53f6eb31..7fc1c4efcae2 100644 --- a/drivers/usb/host/xhci-ring.c +++ b/drivers/usb/host/xhci-ring.c @@ -1385,7 +1385,20 @@ static void xhci_handle_cmd_set_deq(struct xhci_hcd *xhci, int slot_id, if (ep->ep_state & EP_HAS_STREAMS) { struct xhci_stream_ctx *ctx = &ep->stream_info->stream_ctx_array[stream_id]; + u32 edtl; + deq = le64_to_cpu(ctx->stream_ring) & SCTX_DEQ_MASK; + edtl = EVENT_TRB_LEN(le32_to_cpu(ctx->reserved[1])); + + /* + * 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[1] = 0; + ctx->reserved[0] = cpu_to_le32(edtl); } else { deq = le64_to_cpu(ep_ctx->deq) & ~EP_CTX_CYCLE_MASK; }
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> --- drivers/usb/host/xhci-ring.c | 13 +++++++++++++ 1 file changed, 13 insertions(+)