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@xxxxxxxxxxxxxxx>
> Signed-off-by: Pawel Laszczak <pawell@xxxxxxxxxxx>
> ---
> 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
>
