| Message ID | PH7PR07MB9538584F3C0AD11119403F11DD9D2@PH7PR07MB9538.namprd07.prod.outlook.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | usb: xhci: fix loss of data on Cadence xHC | expand |
Hi, Please ignore this patch. I send it again with correct version in subject. Thanks, Pawel, >Subject: FW: [PATCH] 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. > >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: >v3: >- changed patch to patch Cadence specific > >v2: >- removed restoring of EDTLA field > > drivers/usb/cdns3/host.c | 4 +++- > drivers/usb/host/xhci-pci.c | 7 +++++++ drivers/usb/host/xhci-ring.c | 14 >++++++++++++++ > drivers/usb/host/xhci.h | 1 + > 4 files changed, 25 insertions(+), 1 deletion(-) > >diff --git a/drivers/usb/cdns3/host.c b/drivers/usb/cdns3/host.c index >ceca4d839dfd..7ba760ee62e3 100644 >--- a/drivers/usb/cdns3/host.c >+++ b/drivers/usb/cdns3/host.c >@@ -62,7 +62,9 @@ static const struct xhci_plat_priv xhci_plat_cdns3_xhci = >{ > .resume_quirk = xhci_cdns3_resume_quirk, }; > >-static const struct xhci_plat_priv xhci_plat_cdnsp_xhci; >+static const struct xhci_plat_priv xhci_plat_cdnsp_xhci = { >+ .quirks = XHCI_CDNS_SCTX_QUIRK, >+}; > > static int __cdns_host_init(struct cdns *cdns) { diff --git >a/drivers/usb/host/xhci-pci.c b/drivers/usb/host/xhci-pci.c index >b9ae5c2a2527..9199dbfcea07 100644 >--- a/drivers/usb/host/xhci-pci.c >+++ b/drivers/usb/host/xhci-pci.c >@@ -74,6 +74,9 @@ > #define PCI_DEVICE_ID_ASMEDIA_2142_XHCI 0x2142 > #define PCI_DEVICE_ID_ASMEDIA_3242_XHCI 0x3242 > >+#define PCI_DEVICE_ID_CADENCE 0x17CD >+#define PCI_DEVICE_ID_CADENCE_SSP 0x0200 >+ > static const char hcd_name[] = "xhci_hcd"; > > static struct hc_driver __read_mostly xhci_pci_hc_driver; @@ -532,6 +535,10 >@@ static void xhci_pci_quirks(struct device *dev, struct xhci_hcd *xhci) > xhci->quirks |= XHCI_ZHAOXIN_TRB_FETCH; > } > >+ if (pdev->vendor == PCI_DEVICE_ID_CADENCE && >+ pdev->device == PCI_DEVICE_ID_CADENCE_SSP) >+ xhci->quirks |= XHCI_CDNS_SCTX_QUIRK; >+ > /* xHC spec requires PCI devices to support D3hot and D3cold */ > if (xhci->hci_version >= 0x120) > xhci->quirks |= XHCI_DEFAULT_PM_RUNTIME_ALLOW; diff -- >git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c index >1dde53f6eb31..a1ad2658c0c7 100644 >--- a/drivers/usb/host/xhci-ring.c >+++ b/drivers/usb/host/xhci-ring.c >@@ -1386,6 +1386,20 @@ 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; >+ >+ /* >+ * 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. >+ */ >+ if (xhci->quirks & XHCI_CDNS_SCTX_QUIRK) { >+ ctx->reserved[0] = 0; >+ ctx->reserved[1] = 0; >+ } > } else { > deq = le64_to_cpu(ep_ctx->deq) & >~EP_CTX_CYCLE_MASK; > } >diff --git a/drivers/usb/host/xhci.h b/drivers/usb/host/xhci.h index >101e74c9060f..4cbd58eed214 100644 >--- a/drivers/usb/host/xhci.h >+++ b/drivers/usb/host/xhci.h >@@ -1907,6 +1907,7 @@ struct xhci_hcd { > #define XHCI_ZHAOXIN_TRB_FETCH BIT_ULL(45) > #define XHCI_ZHAOXIN_HOST BIT_ULL(46) > #define XHCI_WRITE_64_HI_LO BIT_ULL(47) >+#define XHCI_CDNS_SCTX_QUIRK BIT_ULL(48) > > unsigned int num_active_eps; > unsigned int limit_active_eps; >-- >2.43.0
Thu, Sep 05, 2024 at 07:06:48AM +0000, Pawel Laszczak kirjoitti: > Please ignore this patch. I send it again with correct version in subject. It seems it's in Mathias' tree, never the less, see also below. ... > >+#define PCI_DEVICE_ID_CADENCE 0x17CD First of all this is misleadig as this is VENDOR_ID, second, there is official ID constant for Cadence in pci_ids.h. #define PCI_VENDOR_ID_CDNS 0x17cd
> >Thu, Sep 05, 2024 at 07:06:48AM +0000, Pawel Laszczak kirjoitti: >> Please ignore this patch. I send it again with correct version in subject. > >It seems it's in Mathias' tree, never the less, see also below. > >... > >> >+#define PCI_DEVICE_ID_CADENCE 0x17CD > >First of all this is misleadig as this is VENDOR_ID, second, there is official ID >constant for Cadence in pci_ids.h. > >#define PCI_VENDOR_ID_CDNS 0x17cd You have right. I assume that I should send the next patch v4. I'm going to leave PCI_DEVICE_ID_CADENCE_SSP. I hope that will not be problem. > >-- >With Best Regards, >Andy Shevchenko >
On 12.9.2024 18.53, Andy Shevchenko wrote: > Thu, Sep 05, 2024 at 07:06:48AM +0000, Pawel Laszczak kirjoitti: >> Please ignore this patch. I send it again with correct version in subject. > > It seems it's in Mathias' tree, never the less, see also below. > > ... > >>> +#define PCI_DEVICE_ID_CADENCE 0x17CD > > First of all this is misleadig as this is VENDOR_ID, second, there is official > ID constant for Cadence in pci_ids.h. > > #define PCI_VENDOR_ID_CDNS 0x17cd > Thanks, fixed and rebased. Changes: diff --git a/drivers/usb/host/xhci-pci.c b/drivers/usb/host/xhci-pci.c index 5e7747f80762..4bc6ee57ec42 100644 --- a/drivers/usb/host/xhci-pci.c +++ b/drivers/usb/host/xhci-pci.c @@ -78,8 +78,7 @@ #define PCI_DEVICE_ID_ASMEDIA_2142_XHCI 0x2142 #define PCI_DEVICE_ID_ASMEDIA_3242_XHCI 0x3242 -#define PCI_DEVICE_ID_CADENCE 0x17CD -#define PCI_DEVICE_ID_CADENCE_SSP 0x0200 +#define PCI_DEVICE_ID_CDNS_SSP 0x0200 static const char hcd_name[] = "xhci_hcd"; @@ -470,8 +469,8 @@ static void xhci_pci_quirks(struct device *dev, struct xhci_hcd *xhci) xhci->quirks |= XHCI_ZHAOXIN_TRB_FETCH; } - if (pdev->vendor == PCI_DEVICE_ID_CADENCE && - pdev->device == PCI_DEVICE_ID_CADENCE_SSP) + if (pdev->vendor == PCI_VENDOR_ID_CDNS && + pdev->device == PCI_DEVICE_ID_CDNS_SSP) xhci->quirks |= XHCI_CDNS_SCTX_QUIRK; -Mathias
On Fri, Sep 13, 2024 at 11:59 AM Mathias Nyman <mathias.nyman@linux.intel.com> wrote: > On 12.9.2024 18.53, Andy Shevchenko wrote: > > Thu, Sep 05, 2024 at 07:06:48AM +0000, Pawel Laszczak kirjoitti: > Thanks, fixed and rebased. LGTM, thanks! -- With Best Regards, Andy Shevchenko
diff --git a/drivers/usb/cdns3/host.c b/drivers/usb/cdns3/host.c index ceca4d839dfd..7ba760ee62e3 100644 --- a/drivers/usb/cdns3/host.c +++ b/drivers/usb/cdns3/host.c @@ -62,7 +62,9 @@ static const struct xhci_plat_priv xhci_plat_cdns3_xhci = { .resume_quirk = xhci_cdns3_resume_quirk, }; -static const struct xhci_plat_priv xhci_plat_cdnsp_xhci; +static const struct xhci_plat_priv xhci_plat_cdnsp_xhci = { + .quirks = XHCI_CDNS_SCTX_QUIRK, +}; static int __cdns_host_init(struct cdns *cdns) { diff --git a/drivers/usb/host/xhci-pci.c b/drivers/usb/host/xhci-pci.c index b9ae5c2a2527..9199dbfcea07 100644 --- a/drivers/usb/host/xhci-pci.c +++ b/drivers/usb/host/xhci-pci.c @@ -74,6 +74,9 @@ #define PCI_DEVICE_ID_ASMEDIA_2142_XHCI 0x2142 #define PCI_DEVICE_ID_ASMEDIA_3242_XHCI 0x3242 +#define PCI_DEVICE_ID_CADENCE 0x17CD +#define PCI_DEVICE_ID_CADENCE_SSP 0x0200 + static const char hcd_name[] = "xhci_hcd"; static struct hc_driver __read_mostly xhci_pci_hc_driver; @@ -532,6 +535,10 @@ static void xhci_pci_quirks(struct device *dev, struct xhci_hcd *xhci) xhci->quirks |= XHCI_ZHAOXIN_TRB_FETCH; } + if (pdev->vendor == PCI_DEVICE_ID_CADENCE && + pdev->device == PCI_DEVICE_ID_CADENCE_SSP) + xhci->quirks |= XHCI_CDNS_SCTX_QUIRK; + /* xHC spec requires PCI devices to support D3hot and D3cold */ if (xhci->hci_version >= 0x120) xhci->quirks |= XHCI_DEFAULT_PM_RUNTIME_ALLOW; diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c index 1dde53f6eb31..a1ad2658c0c7 100644 --- a/drivers/usb/host/xhci-ring.c +++ b/drivers/usb/host/xhci-ring.c @@ -1386,6 +1386,20 @@ 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; + + /* + * 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. + */ + if (xhci->quirks & XHCI_CDNS_SCTX_QUIRK) { + ctx->reserved[0] = 0; + ctx->reserved[1] = 0; + } } else { deq = le64_to_cpu(ep_ctx->deq) & ~EP_CTX_CYCLE_MASK; } diff --git a/drivers/usb/host/xhci.h b/drivers/usb/host/xhci.h index 101e74c9060f..4cbd58eed214 100644 --- a/drivers/usb/host/xhci.h +++ b/drivers/usb/host/xhci.h @@ -1907,6 +1907,7 @@ struct xhci_hcd { #define XHCI_ZHAOXIN_TRB_FETCH BIT_ULL(45) #define XHCI_ZHAOXIN_HOST BIT_ULL(46) #define XHCI_WRITE_64_HI_LO BIT_ULL(47) +#define XHCI_CDNS_SCTX_QUIRK BIT_ULL(48) unsigned int num_active_eps; unsigned int limit_active_eps;
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. 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: v3: - changed patch to patch Cadence specific v2: - removed restoring of EDTLA field drivers/usb/cdns3/host.c | 4 +++- drivers/usb/host/xhci-pci.c | 7 +++++++ drivers/usb/host/xhci-ring.c | 14 ++++++++++++++ drivers/usb/host/xhci.h | 1 + 4 files changed, 25 insertions(+), 1 deletion(-)