Re: [PATCH] usb: cdns3: gadget: fix request skipping after clearing halt

["Peter Chen (CIX)" <peter.chen@kernel.org>]

On 26-04-24 00:06:01, Yongchao Wu wrote:
> According to the cdns3 datasheet, the EPRST (Endpoint Reset) command
> causes the DMA engine to reposition its internal pointer to the next
> Transfer Descriptor (TD) if it was already processing one.
> 
> This issue is consistently observed during the ADB identification
> process on macOS hosts, where the host issues a Clear_Halt. Although
> commit 4bf2dd65135a ("usb: cdns3: gadget: toggle cycle bit before reset
> endpoint") attempted to avoid DMA advance by toggling the cycle bit,
> trace logs show that on certain hosts like macOS, the DMA pointer
> (EP_TRADDR) still shifts after EPRST:
> 
>   cdns3_ctrl_req: Clear Endpoint Feature(Halt ep1out)
>   cdns3_doorbell_epx: ep1out, ep_trbaddr f9c04030  <-- Should be f9c04000
>   cdns3_gadget_giveback: ep1out: req: ... length: 16384/16384
> 
> As shown above, the DMA pointer jumped to index 3 (offset 0x30), causing
> the controller to skip the initial TRBs of the request. This leads to
> data misalignment and ADB protocol hangs on macOS.

Pawel, Is it a hardware issue? The cycle bit has already been toggled
before the endpoint has been reset, why the DMA pointer still advances?

Peter

> 
> Fix this by manually restoring the EP_TRADDR register to the starting
> physical address of the current request after the EPRST operation is
> complete.
> 
> Fixes: 7733f6c32e36 ("usb: cdns3: Add Cadence USB3 DRD Driver")
> Cc: stable@vger.kernel.org
> Cc: Peter Chen <peter.chen@kernel.org>
> Signed-off-by: Yongchao Wu <yongchao.wu@autochips.com>
> ---
>  drivers/usb/cdns3/cdns3-gadget.c | 12 +++++++++++-
>  1 file changed, 11 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/usb/cdns3/cdns3-gadget.c b/drivers/usb/cdns3/cdns3-gadget.c
> index d59a60a16ec77..96653c7d18f20 100644
> --- a/drivers/usb/cdns3/cdns3-gadget.c
> +++ b/drivers/usb/cdns3/cdns3-gadget.c
> @@ -2814,9 +2814,19 @@ int __cdns3_gadget_ep_clear_halt(struct cdns3_endpoint *priv_ep)
>  	priv_ep->flags &= ~(EP_STALLED | EP_STALL_PENDING);
>  
>  	if (request) {
> -		if (trb)
> +		if (trb) {
>  			*trb = trb_tmp;
>  
> +			/*
> +			 * Per datasheet, EPRST causes DMA to reposition to the next TD.
> +			 * Manually reset EP_TRADDR to the current TRB to prevent
> +			 * the hardware from skipping the interrupted request.
> +			 */
> +			writel(EP_TRADDR_TRADDR(priv_ep->trb_pool_dma +
> +						priv_req->start_trb * TRB_SIZE),
> +						&priv_dev->regs->ep_traddr);
> +		}
> +
>  		cdns3_rearm_transfer(priv_ep, 1);
>  	}
>  
> 
> base-commit: 46b513250491a7bfc97d98791dbe6a10bcc8129d
> -- 
> 2.43.0
> 
> 

-- 

Best regards,
Peter