* [PATCH] usb: cdnsp: fix for Link TRB with TC [not found] <20240821060426.84380-1-pawell@cadence.com> @ 2024-08-21 6:07 ` Pawel Laszczak 2024-08-21 10:43 ` Peter Chen 2024-08-22 11:19 ` Peter Chen 0 siblings, 2 replies; 4+ messages in thread From: Pawel Laszczak @ 2024-08-21 6:07 UTC (permalink / raw) To: peter.chen@kernel.org Cc: gregkh@linuxfoundation.org, linux-usb@vger.kernel.org, linux-kernel@vger.kernel.org, Pawel Laszczak, stable@vger.kernel.org Stop Endpoint command on LINK TRB with TC bit set to 1 causes that internal cycle bit can have incorrect state after command complete. In consequence empty transfer ring can be incorrectly detected when EP is resumed. NOP TRB before LINK TRB avoid such scenario. Stop Endpoint command is then on NOP TRB and internal cycle bit is not changed and have correct value. 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/cdns3/cdnsp-gadget.h | 3 +++ drivers/usb/cdns3/cdnsp-ring.c | 28 ++++++++++++++++++++++++++++ 2 files changed, 31 insertions(+) diff --git a/drivers/usb/cdns3/cdnsp-gadget.h b/drivers/usb/cdns3/cdnsp-gadget.h index e1b5801fdddf..9a5577a772af 100644 --- a/drivers/usb/cdns3/cdnsp-gadget.h +++ b/drivers/usb/cdns3/cdnsp-gadget.h @@ -811,6 +811,7 @@ struct cdnsp_stream_info { * generate Missed Service Error Event. * Set skip flag when receive a Missed Service Error Event and * process the missed tds on the endpoint ring. + * @wa1_nop_trb: hold pointer to NOP trb. */ struct cdnsp_ep { struct usb_ep endpoint; @@ -838,6 +839,8 @@ struct cdnsp_ep { #define EP_UNCONFIGURED BIT(7) bool skip; + union cdnsp_trb *wa1_nop_trb; + }; /** diff --git a/drivers/usb/cdns3/cdnsp-ring.c b/drivers/usb/cdns3/cdnsp-ring.c index 275a6a2fa671..75724e60653c 100644 --- a/drivers/usb/cdns3/cdnsp-ring.c +++ b/drivers/usb/cdns3/cdnsp-ring.c @@ -1904,6 +1904,23 @@ int cdnsp_queue_bulk_tx(struct cdnsp_device *pdev, struct cdnsp_request *preq) if (ret) return ret; + /* + * workaround 1: STOP EP command on LINK TRB with TC bit set to 1 + * causes that internal cycle bit can have incorrect state after + * command complete. In consequence empty transfer ring can be + * incorrectly detected when EP is resumed. + * NOP TRB before LINK TRB avoid such scenario. STOP EP command is + * then on NOP TRB and internal cycle bit is not changed and have + * correct value. + */ + if (pep->wa1_nop_trb) { + field = le32_to_cpu(pep->wa1_nop_trb->trans_event.flags); + field ^= TRB_CYCLE; + + pep->wa1_nop_trb->trans_event.flags = cpu_to_le32(field); + pep->wa1_nop_trb = NULL; + } + /* * Don't give the first TRB to the hardware (by toggling the cycle bit) * until we've finished creating all the other TRBs. The ring's cycle @@ -1999,6 +2016,17 @@ int cdnsp_queue_bulk_tx(struct cdnsp_device *pdev, struct cdnsp_request *preq) send_addr = addr; } + if (cdnsp_trb_is_link(ring->enqueue + 1)) { + field = TRB_TYPE(TRB_TR_NOOP) | TRB_IOC; + if (!ring->cycle_state) + field |= TRB_CYCLE; + + pep->wa1_nop_trb = ring->enqueue; + + cdnsp_queue_trb(pdev, ring, 0, 0x0, 0x0, + TRB_INTR_TARGET(0), field); + } + cdnsp_check_trb_math(preq, enqd_len); ret = cdnsp_giveback_first_trb(pdev, pep, preq->request.stream_id, start_cycle, start_trb); -- 2.43.0 ^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] usb: cdnsp: fix for Link TRB with TC 2024-08-21 6:07 ` [PATCH] usb: cdnsp: fix for Link TRB with TC Pawel Laszczak @ 2024-08-21 10:43 ` Peter Chen 2024-08-22 7:50 ` Pawel Laszczak 2024-08-22 11:19 ` Peter Chen 1 sibling, 1 reply; 4+ messages in thread From: Peter Chen @ 2024-08-21 10:43 UTC (permalink / raw) To: Pawel Laszczak Cc: gregkh@linuxfoundation.org, linux-usb@vger.kernel.org, linux-kernel@vger.kernel.org, stable@vger.kernel.org On 24-08-21 06:07:42, Pawel Laszczak wrote: > Stop Endpoint command on LINK TRB with TC bit set to 1 causes that > internal cycle bit can have incorrect state after command complete. You mean this issue is: the transfer ring is on the LINK TRB when stop endpoint command is going to execute? What's the use case we could find this issue? Peter > In consequence empty transfer ring can be incorrectly detected > when EP is resumed. > NOP TRB before LINK TRB avoid such scenario. Stop Endpoint command > is then on NOP TRB and internal cycle bit is not changed and have > correct value. > > 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/cdns3/cdnsp-gadget.h | 3 +++ > drivers/usb/cdns3/cdnsp-ring.c | 28 ++++++++++++++++++++++++++++ > 2 files changed, 31 insertions(+) > > diff --git a/drivers/usb/cdns3/cdnsp-gadget.h b/drivers/usb/cdns3/cdnsp-gadget.h > index e1b5801fdddf..9a5577a772af 100644 > --- a/drivers/usb/cdns3/cdnsp-gadget.h > +++ b/drivers/usb/cdns3/cdnsp-gadget.h > @@ -811,6 +811,7 @@ struct cdnsp_stream_info { > * generate Missed Service Error Event. > * Set skip flag when receive a Missed Service Error Event and > * process the missed tds on the endpoint ring. > + * @wa1_nop_trb: hold pointer to NOP trb. > */ > struct cdnsp_ep { > struct usb_ep endpoint; > @@ -838,6 +839,8 @@ struct cdnsp_ep { > #define EP_UNCONFIGURED BIT(7) > > bool skip; > + union cdnsp_trb *wa1_nop_trb; > + > }; > > /** > diff --git a/drivers/usb/cdns3/cdnsp-ring.c b/drivers/usb/cdns3/cdnsp-ring.c > index 275a6a2fa671..75724e60653c 100644 > --- a/drivers/usb/cdns3/cdnsp-ring.c > +++ b/drivers/usb/cdns3/cdnsp-ring.c > @@ -1904,6 +1904,23 @@ int cdnsp_queue_bulk_tx(struct cdnsp_device *pdev, struct cdnsp_request *preq) > if (ret) > return ret; > > + /* > + * workaround 1: STOP EP command on LINK TRB with TC bit set to 1 > + * causes that internal cycle bit can have incorrect state after > + * command complete. In consequence empty transfer ring can be > + * incorrectly detected when EP is resumed. > + * NOP TRB before LINK TRB avoid such scenario. STOP EP command is > + * then on NOP TRB and internal cycle bit is not changed and have > + * correct value. > + */ > + if (pep->wa1_nop_trb) { > + field = le32_to_cpu(pep->wa1_nop_trb->trans_event.flags); > + field ^= TRB_CYCLE; > + > + pep->wa1_nop_trb->trans_event.flags = cpu_to_le32(field); > + pep->wa1_nop_trb = NULL; > + } > + > /* > * Don't give the first TRB to the hardware (by toggling the cycle bit) > * until we've finished creating all the other TRBs. The ring's cycle > @@ -1999,6 +2016,17 @@ int cdnsp_queue_bulk_tx(struct cdnsp_device *pdev, struct cdnsp_request *preq) > send_addr = addr; > } > > + if (cdnsp_trb_is_link(ring->enqueue + 1)) { > + field = TRB_TYPE(TRB_TR_NOOP) | TRB_IOC; > + if (!ring->cycle_state) > + field |= TRB_CYCLE; > + > + pep->wa1_nop_trb = ring->enqueue; > + > + cdnsp_queue_trb(pdev, ring, 0, 0x0, 0x0, > + TRB_INTR_TARGET(0), field); > + } > + > cdnsp_check_trb_math(preq, enqd_len); > ret = cdnsp_giveback_first_trb(pdev, pep, preq->request.stream_id, > start_cycle, start_trb); > -- > 2.43.0 > ^ permalink raw reply [flat|nested] 4+ messages in thread
* RE: [PATCH] usb: cdnsp: fix for Link TRB with TC 2024-08-21 10:43 ` Peter Chen @ 2024-08-22 7:50 ` Pawel Laszczak 0 siblings, 0 replies; 4+ messages in thread From: Pawel Laszczak @ 2024-08-22 7:50 UTC (permalink / raw) To: Peter Chen Cc: gregkh@linuxfoundation.org, linux-usb@vger.kernel.org, linux-kernel@vger.kernel.org, stable@vger.kernel.org >On 24-08-21 06:07:42, Pawel Laszczak wrote: >> Stop Endpoint command on LINK TRB with TC bit set to 1 causes that >> internal cycle bit can have incorrect state after command complete. > >You mean this issue is: the transfer ring is on the LINK TRB when stop endpoint >command is going to execute? Yes, but also TC bit must be set in such LINK TRB. > >What's the use case we could find this issue? I was not able to recreate this issue with standard linux and windows drivers. To recreate this issue I used: 1. device with 2 x ACM connected to Windows OS host 2. UartAssist V5.0.10.exe 3. Vendor specific ACM driver Test scenario will open & close port many times during testing. Host driver should send clear feature (halt) request to device when application open the acm function port. Thanks, Pawel > >Peter > >> In consequence empty transfer ring can be incorrectly detected when EP >> is resumed. >> NOP TRB before LINK TRB avoid such scenario. Stop Endpoint command is >> then on NOP TRB and internal cycle bit is not changed and have correct >> value. >> >> 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/cdns3/cdnsp-gadget.h | 3 +++ >> drivers/usb/cdns3/cdnsp-ring.c | 28 ++++++++++++++++++++++++++++ >> 2 files changed, 31 insertions(+) >> >> diff --git a/drivers/usb/cdns3/cdnsp-gadget.h >> b/drivers/usb/cdns3/cdnsp-gadget.h >> index e1b5801fdddf..9a5577a772af 100644 >> --- a/drivers/usb/cdns3/cdnsp-gadget.h >> +++ b/drivers/usb/cdns3/cdnsp-gadget.h >> @@ -811,6 +811,7 @@ struct cdnsp_stream_info { >> * generate Missed Service Error Event. >> * Set skip flag when receive a Missed Service Error Event and >> * process the missed tds on the endpoint ring. >> + * @wa1_nop_trb: hold pointer to NOP trb. >> */ >> struct cdnsp_ep { >> struct usb_ep endpoint; >> @@ -838,6 +839,8 @@ struct cdnsp_ep { >> #define EP_UNCONFIGURED BIT(7) >> >> bool skip; >> + union cdnsp_trb *wa1_nop_trb; >> + >> }; >> >> /** >> diff --git a/drivers/usb/cdns3/cdnsp-ring.c >> b/drivers/usb/cdns3/cdnsp-ring.c index 275a6a2fa671..75724e60653c >> 100644 >> --- a/drivers/usb/cdns3/cdnsp-ring.c >> +++ b/drivers/usb/cdns3/cdnsp-ring.c >> @@ -1904,6 +1904,23 @@ int cdnsp_queue_bulk_tx(struct cdnsp_device >*pdev, struct cdnsp_request *preq) >> if (ret) >> return ret; >> >> + /* >> + * workaround 1: STOP EP command on LINK TRB with TC bit set to 1 >> + * causes that internal cycle bit can have incorrect state after >> + * command complete. In consequence empty transfer ring can be >> + * incorrectly detected when EP is resumed. >> + * NOP TRB before LINK TRB avoid such scenario. STOP EP command is >> + * then on NOP TRB and internal cycle bit is not changed and have >> + * correct value. >> + */ >> + if (pep->wa1_nop_trb) { >> + field = le32_to_cpu(pep->wa1_nop_trb->trans_event.flags); >> + field ^= TRB_CYCLE; >> + >> + pep->wa1_nop_trb->trans_event.flags = cpu_to_le32(field); >> + pep->wa1_nop_trb = NULL; >> + } >> + >> /* >> * Don't give the first TRB to the hardware (by toggling the cycle bit) >> * until we've finished creating all the other TRBs. The ring's >> cycle @@ -1999,6 +2016,17 @@ int cdnsp_queue_bulk_tx(struct >cdnsp_device *pdev, struct cdnsp_request *preq) >> send_addr = addr; >> } >> >> + if (cdnsp_trb_is_link(ring->enqueue + 1)) { >> + field = TRB_TYPE(TRB_TR_NOOP) | TRB_IOC; >> + if (!ring->cycle_state) >> + field |= TRB_CYCLE; >> + >> + pep->wa1_nop_trb = ring->enqueue; >> + >> + cdnsp_queue_trb(pdev, ring, 0, 0x0, 0x0, >> + TRB_INTR_TARGET(0), field); >> + } >> + >> cdnsp_check_trb_math(preq, enqd_len); >> ret = cdnsp_giveback_first_trb(pdev, pep, preq->request.stream_id, >> start_cycle, start_trb); >> -- >> 2.43.0 >> ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] usb: cdnsp: fix for Link TRB with TC 2024-08-21 6:07 ` [PATCH] usb: cdnsp: fix for Link TRB with TC Pawel Laszczak 2024-08-21 10:43 ` Peter Chen @ 2024-08-22 11:19 ` Peter Chen 1 sibling, 0 replies; 4+ messages in thread From: Peter Chen @ 2024-08-22 11:19 UTC (permalink / raw) To: Pawel Laszczak Cc: gregkh@linuxfoundation.org, linux-usb@vger.kernel.org, linux-kernel@vger.kernel.org, stable@vger.kernel.org On 24-08-21 06:07:42, Pawel Laszczak wrote: > Stop Endpoint command on LINK TRB with TC bit set to 1 causes that > internal cycle bit can have incorrect state after command complete. > In consequence empty transfer ring can be incorrectly detected > when EP is resumed. > NOP TRB before LINK TRB avoid such scenario. Stop Endpoint command > is then on NOP TRB and internal cycle bit is not changed and have > correct value. > > 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> Reviewed-by: Peter Chen <peter.chen@kernel.org> Peter > --- > drivers/usb/cdns3/cdnsp-gadget.h | 3 +++ > drivers/usb/cdns3/cdnsp-ring.c | 28 ++++++++++++++++++++++++++++ > 2 files changed, 31 insertions(+) > > diff --git a/drivers/usb/cdns3/cdnsp-gadget.h b/drivers/usb/cdns3/cdnsp-gadget.h > index e1b5801fdddf..9a5577a772af 100644 > --- a/drivers/usb/cdns3/cdnsp-gadget.h > +++ b/drivers/usb/cdns3/cdnsp-gadget.h > @@ -811,6 +811,7 @@ struct cdnsp_stream_info { > * generate Missed Service Error Event. > * Set skip flag when receive a Missed Service Error Event and > * process the missed tds on the endpoint ring. > + * @wa1_nop_trb: hold pointer to NOP trb. > */ > struct cdnsp_ep { > struct usb_ep endpoint; > @@ -838,6 +839,8 @@ struct cdnsp_ep { > #define EP_UNCONFIGURED BIT(7) > > bool skip; > + union cdnsp_trb *wa1_nop_trb; > + > }; > > /** > diff --git a/drivers/usb/cdns3/cdnsp-ring.c b/drivers/usb/cdns3/cdnsp-ring.c > index 275a6a2fa671..75724e60653c 100644 > --- a/drivers/usb/cdns3/cdnsp-ring.c > +++ b/drivers/usb/cdns3/cdnsp-ring.c > @@ -1904,6 +1904,23 @@ int cdnsp_queue_bulk_tx(struct cdnsp_device *pdev, struct cdnsp_request *preq) > if (ret) > return ret; > > + /* > + * workaround 1: STOP EP command on LINK TRB with TC bit set to 1 > + * causes that internal cycle bit can have incorrect state after > + * command complete. In consequence empty transfer ring can be > + * incorrectly detected when EP is resumed. > + * NOP TRB before LINK TRB avoid such scenario. STOP EP command is > + * then on NOP TRB and internal cycle bit is not changed and have > + * correct value. > + */ > + if (pep->wa1_nop_trb) { > + field = le32_to_cpu(pep->wa1_nop_trb->trans_event.flags); > + field ^= TRB_CYCLE; > + > + pep->wa1_nop_trb->trans_event.flags = cpu_to_le32(field); > + pep->wa1_nop_trb = NULL; > + } > + > /* > * Don't give the first TRB to the hardware (by toggling the cycle bit) > * until we've finished creating all the other TRBs. The ring's cycle > @@ -1999,6 +2016,17 @@ int cdnsp_queue_bulk_tx(struct cdnsp_device *pdev, struct cdnsp_request *preq) > send_addr = addr; > } > > + if (cdnsp_trb_is_link(ring->enqueue + 1)) { > + field = TRB_TYPE(TRB_TR_NOOP) | TRB_IOC; > + if (!ring->cycle_state) > + field |= TRB_CYCLE; > + > + pep->wa1_nop_trb = ring->enqueue; > + > + cdnsp_queue_trb(pdev, ring, 0, 0x0, 0x0, > + TRB_INTR_TARGET(0), field); > + } > + > cdnsp_check_trb_math(preq, enqd_len); > ret = cdnsp_giveback_first_trb(pdev, pep, preq->request.stream_id, > start_cycle, start_trb); > -- > 2.43.0 > ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2024-08-22 11:20 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <20240821060426.84380-1-pawell@cadence.com>
2024-08-21 6:07 ` [PATCH] usb: cdnsp: fix for Link TRB with TC Pawel Laszczak
2024-08-21 10:43 ` Peter Chen
2024-08-22 7:50 ` Pawel Laszczak
2024-08-22 11:19 ` Peter Chen
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox