Re: [PATCH] usb: cdnsp: fix for Link TRB with TC

[Peter Chen <peter.chen@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
>