[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20240821104308.GB652432@nchen-desktop>
Date: Wed, 21 Aug 2024 18:43:08 +0800
From: Peter Chen <peter.chen@...nel.org>
To: Pawel Laszczak <pawell@...ence.com>
Cc: "gregkh@...uxfoundation.org" <gregkh@...uxfoundation.org>,
"linux-usb@...r.kernel.org" <linux-usb@...r.kernel.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
"stable@...r.kernel.org" <stable@...r.kernel.org>
Subject: Re: [PATCH] usb: cdnsp: fix for Link TRB with TC
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@...r.kernel.org>
> Signed-off-by: Pawel Laszczak <pawell@...ence.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
>
Powered by blists - more mailing lists