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

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

[Yongchao Wu]

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.

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

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

[Peter Chen (CIX)]

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

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

[Pawel Laszczak]

>
>
>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, do you remember what the TD looked like in your case?
Maybe that’s where the difference lies. The patch description states that
it jumps from 0xf9c04000 to 0xf9c04030, which would suggest that the TD
consists of three TRBs. The driver only changes the cycle bit on the
first one. 
I’m not entirely sure how the controller assembles this TD.
I need some time to try explain the controller's behavior in this case.

Yongchao, could you confirm if the TD consists of three TRBs?

Pawel

>
>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

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

[Peter Chen (CIX)]

On 26-04-27 09:01:47, Pawel Laszczak wrote:
> >
> >
> >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, do you remember what the TD looked like in your case?

Sorry, it was almost 6 years ago, I could not remember it well.

> Maybe that’s where the difference lies. The patch description states that
> it jumps from 0xf9c04000 to 0xf9c04030, which would suggest that the TD
> consists of three TRBs. The driver only changes the cycle bit on the
> first one. 

According to Yongchao's statement:

	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.

My points are the cycle bit has toggled by SW before EPRST command, and EPRST
command would reset DMA pointer to the 1st TRB within TD, and it is executed
later than cycle bit toggles, why hardware could go on handling TRBs even the
first TRB's cycle bit is for software?

Peter

> I’m not entirely sure how the controller assembles this TD.
> I need some time to try explain the controller's behavior in this case.
> 
> Yongchao, could you confirm if the TD consists of three TRBs?
> 
> Pawel
-- 

Best regards,
Peter

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

[Yongchao Wu]

On 26-04-27 09:01:47, Pawel Laszczak wrote:
 >>
 >>
 >> 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?
 >
 > Yongchao, could you confirm if the TD consists of three TRBs?
In our case, each TD consists of 4 TRBs.
The DMA pointer appears to advance within the same TD after EPRST.

Each 16KB request is split into 4 TRBs (4KB each):
- TRB0 - TRB2: CHAIN
- TRB3: IOC (last TRB of the TD)

After enqueue, the initial EP_TRADDR points to the first TRB:
   EP_TRADDR = 0xf9c04000 (TRB0)

After Clear_Halt (EPRST), it becomes:
   EP_TRADDR = 0xf9c04030 (TRB3)

Since each TRB is 12 bytes, the offset 0x30 corresponds to 4 TRBs.
This indicates that after EPRST, the DMA pointer skipped the entire
current Request and jumped directly to the start of the next Request
at 0xf9c04030

Below is the relevant trace (trimmed):

// enqueue request (16KB -> 4 TRBs)
cdns3_prepare_trb: dma buf: 0xf7abc000, size: 4096, ctrl: 0x00200415
cdns3_prepare_trb: dma buf: 0xf7abd000, size: 4096, ctrl: 0x00000415
cdns3_prepare_trb: dma buf: 0xf7abe000, size: 4096, ctrl: 0x00000415
cdns3_prepare_trb: dma buf: 0xf7abf000, size: 4096, ctrl: 0x00000425

cdns3_doorbell_epx: ep1out, ep_trbaddr f9c04000

// Clear_Halt
cdns3_ctrl_req: Clear Endpoint Feature(Halt ep1out)
cdns3_doorbell_epx: ep1out, ep_trbaddr f9c04030

This behavior is consistently observed on macOS hosts during
ADB enumeration.

So even though the cycle bit is toggled on the first TRB, the
controller still appears to advance the DMA pointer within the same TD
after EPRST.

Please let me know if you need more detailed logs or a full TRB
ring dump. I'd also appreciate any insight into how the controller
determines the next position after EPRST in this case.

Best regards,
Yongchao