[PATCH] usb: dwc3: Fix ep0 handling when getting reset while doing control transfer

[PATCH] usb: dwc3: Fix ep0 handling when getting reset while doing control transfer

[Mayank Rana]

According to the databook ep0 should be in setup phase during reset.
If host issues reset between control transfers, ep0 will be  in an
invalid state. Fix this by issuing stall and restart on ep0 if it
is not in setup phase.

Also SW needs to complete pending control transfer and setup core for
next setup stage as per data book. Hence check ep0 state during reset
interrupt handling and make sure active transfers on ep0 out/in
endpoint are stopped by queuing ENDXFER command for that endpoint and
restart ep0 out again to receive next setup packet.

Signed-off-by: Mayank Rana <quic_mrana@quicinc.com>
---
 drivers/usb/dwc3/ep0.c    | 11 ++++++++---
 drivers/usb/dwc3/gadget.c | 27 +++++++++++++++++++++++++--
 drivers/usb/dwc3/gadget.h |  2 ++
 3 files changed, 35 insertions(+), 5 deletions(-)

diff --git a/drivers/usb/dwc3/ep0.c b/drivers/usb/dwc3/ep0.c
index 1064be5..9b6ebc3 100644
--- a/drivers/usb/dwc3/ep0.c
+++ b/drivers/usb/dwc3/ep0.c
@@ -218,7 +218,7 @@ int dwc3_gadget_ep0_queue(struct usb_ep *ep, struct usb_request *request,
 	return ret;
 }
 
-static void dwc3_ep0_stall_and_restart(struct dwc3 *dwc)
+void dwc3_ep0_stall_and_restart(struct dwc3 *dwc)
 {
 	struct dwc3_ep		*dep;
 
@@ -1087,13 +1087,18 @@ void dwc3_ep0_send_delayed_status(struct dwc3 *dwc)
 	__dwc3_ep0_do_control_status(dwc, dwc->eps[direction]);
 }
 
-static void dwc3_ep0_end_control_data(struct dwc3 *dwc, struct dwc3_ep *dep)
+void dwc3_ep0_end_control_data(struct dwc3 *dwc, struct dwc3_ep *dep)
 {
 	struct dwc3_gadget_ep_cmd_params params;
 	u32			cmd;
 	int			ret;
 
-	if (!dep->resource_index)
+	/*
+	 * For status/DATA OUT stage, TRB will be queued on ep0 out
+	 * endpoint for which resource index is zero. Hence allow
+	 * queuing ENDXFER command for ep0 out endpoint.
+	 */
+	if (!dep->resource_index && dep->number)
 		return;
 
 	cmd = DWC3_DEPCMD_ENDTRANSFER;
diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
index ab725d2..82a210f 100644
--- a/drivers/usb/dwc3/gadget.c
+++ b/drivers/usb/dwc3/gadget.c
@@ -882,12 +882,13 @@ static int __dwc3_gadget_ep_enable(struct dwc3_ep *dep, unsigned int action)
 		reg |= DWC3_DALEPENA_EP(dep->number);
 		dwc3_writel(dwc->regs, DWC3_DALEPENA, reg);
 
+		dep->trb_dequeue = 0;
+		dep->trb_enqueue = 0;
+
 		if (usb_endpoint_xfer_control(desc))
 			goto out;
 
 		/* Initialize the TRB ring */
-		dep->trb_dequeue = 0;
-		dep->trb_enqueue = 0;
 		memset(dep->trb_pool, 0,
 		       sizeof(struct dwc3_trb) * DWC3_TRB_NUM);
 
@@ -2745,6 +2746,7 @@ static int __dwc3_gadget_start(struct dwc3 *dwc)
 
 	/* begin to receive SETUP packets */
 	dwc->ep0state = EP0_SETUP_PHASE;
+	dwc->ep0_bounced = false;
 	dwc->link_state = DWC3_LINK_STATE_SS_DIS;
 	dwc->delayed_status = false;
 	dwc3_ep0_out_start(dwc);
@@ -3766,6 +3768,27 @@ static void dwc3_gadget_reset_interrupt(struct dwc3 *dwc)
 	}
 
 	dwc3_reset_gadget(dwc);
+
+	/*
+	 * From SNPS databook section 8.1.2, the EP0 should be in setup
+	 * phase. So ensure that EP0 is in setup phase by issuing a stall
+	 * and restart if EP0 is not in setup phase.
+	 */
+	if (dwc->ep0state != EP0_SETUP_PHASE) {
+		unsigned int	dir;
+
+		dir = !!dwc->ep0_expect_in;
+		if (dwc->ep0state == EP0_DATA_PHASE)
+			dwc3_ep0_end_control_data(dwc, dwc->eps[dir]);
+		else
+			dwc3_ep0_end_control_data(dwc, dwc->eps[!dir]);
+
+		dwc->eps[0]->trb_enqueue = 0;
+		dwc->eps[1]->trb_enqueue = 0;
+
+		dwc3_ep0_stall_and_restart(dwc);
+	}
+
 	/*
 	 * In the Synopsis DesignWare Cores USB3 Databook Rev. 3.30a
 	 * Section 4.1.2 Table 4-2, it states that during a USB reset, the SW
diff --git a/drivers/usb/dwc3/gadget.h b/drivers/usb/dwc3/gadget.h
index f763380..55a56cf67 100644
--- a/drivers/usb/dwc3/gadget.h
+++ b/drivers/usb/dwc3/gadget.h
@@ -110,6 +110,8 @@ void dwc3_gadget_giveback(struct dwc3_ep *dep, struct dwc3_request *req,
 void dwc3_ep0_interrupt(struct dwc3 *dwc,
 		const struct dwc3_event_depevt *event);
 void dwc3_ep0_out_start(struct dwc3 *dwc);
+void dwc3_ep0_end_control_data(struct dwc3 *dwc, struct dwc3_ep *dep);
+void dwc3_ep0_stall_and_restart(struct dwc3 *dwc);
 int __dwc3_gadget_ep0_set_halt(struct usb_ep *ep, int value);
 int dwc3_gadget_ep0_set_halt(struct usb_ep *ep, int value);
 int dwc3_gadget_ep0_queue(struct usb_ep *ep, struct usb_request *request,
-- 
2.7.4

Re: [PATCH] usb: dwc3: Fix ep0 handling when getting reset while doing control transfer

[Thinh Nguyen]

Hi,

Mayank Rana wrote:
> According to the databook ep0 should be in setup phase during reset.
> If host issues reset between control transfers, ep0 will be  in an
> invalid state. Fix this by issuing stall and restart on ep0 if it
> is not in setup phase.

I don't think the programming guide indicate to stall and start on ep0.
It says to "complete" it. Maybe there's a misinterpretation here.

> 
> Also SW needs to complete pending control transfer and setup core for
> next setup stage as per data book. Hence check ep0 state during reset
> interrupt handling and make sure active transfers on ep0 out/in
> endpoint are stopped by queuing ENDXFER command for that endpoint and
> restart ep0 out again to receive next setup packet.
> 
> Signed-off-by: Mayank Rana <quic_mrana@quicinc.com>
> ---
>  drivers/usb/dwc3/ep0.c    | 11 ++++++++---
>  drivers/usb/dwc3/gadget.c | 27 +++++++++++++++++++++++++--
>  drivers/usb/dwc3/gadget.h |  2 ++
>  3 files changed, 35 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/usb/dwc3/ep0.c b/drivers/usb/dwc3/ep0.c
> index 1064be5..9b6ebc3 100644
> --- a/drivers/usb/dwc3/ep0.c
> +++ b/drivers/usb/dwc3/ep0.c
> @@ -218,7 +218,7 @@ int dwc3_gadget_ep0_queue(struct usb_ep *ep, struct usb_request *request,
>  	return ret;
>  }
>  
> -static void dwc3_ep0_stall_and_restart(struct dwc3 *dwc)
> +void dwc3_ep0_stall_and_restart(struct dwc3 *dwc)
>  {
>  	struct dwc3_ep		*dep;
>  
> @@ -1087,13 +1087,18 @@ void dwc3_ep0_send_delayed_status(struct dwc3 *dwc)
>  	__dwc3_ep0_do_control_status(dwc, dwc->eps[direction]);
>  }
>  
> -static void dwc3_ep0_end_control_data(struct dwc3 *dwc, struct dwc3_ep *dep)
> +void dwc3_ep0_end_control_data(struct dwc3 *dwc, struct dwc3_ep *dep)
>  {
>  	struct dwc3_gadget_ep_cmd_params params;
>  	u32			cmd;
>  	int			ret;
>  
> -	if (!dep->resource_index)
> +	/*
> +	 * For status/DATA OUT stage, TRB will be queued on ep0 out
> +	 * endpoint for which resource index is zero. Hence allow
> +	 * queuing ENDXFER command for ep0 out endpoint.
> +	 */
> +	if (!dep->resource_index && dep->number)
>  		return;

This doesn't make sense. This condition will not be true. It's basically

if (false)
	return;

>  
>  	cmd = DWC3_DEPCMD_ENDTRANSFER;
> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
> index ab725d2..82a210f 100644
> --- a/drivers/usb/dwc3/gadget.c
> +++ b/drivers/usb/dwc3/gadget.c
> @@ -882,12 +882,13 @@ static int __dwc3_gadget_ep_enable(struct dwc3_ep *dep, unsigned int action)
>  		reg |= DWC3_DALEPENA_EP(dep->number);
>  		dwc3_writel(dwc->regs, DWC3_DALEPENA, reg);
>  
> +		dep->trb_dequeue = 0;
> +		dep->trb_enqueue = 0;
> +
>  		if (usb_endpoint_xfer_control(desc))
>  			goto out;
>  
>  		/* Initialize the TRB ring */
> -		dep->trb_dequeue = 0;
> -		dep->trb_enqueue = 0;

This change should be done in a separate patch.

>  		memset(dep->trb_pool, 0,
>  		       sizeof(struct dwc3_trb) * DWC3_TRB_NUM);
>  
> @@ -2745,6 +2746,7 @@ static int __dwc3_gadget_start(struct dwc3 *dwc)
>  
>  	/* begin to receive SETUP packets */
>  	dwc->ep0state = EP0_SETUP_PHASE;
> +	dwc->ep0_bounced = false;

Same with this.

>  	dwc->link_state = DWC3_LINK_STATE_SS_DIS;
>  	dwc->delayed_status = false;
>  	dwc3_ep0_out_start(dwc);
> @@ -3766,6 +3768,27 @@ static void dwc3_gadget_reset_interrupt(struct dwc3 *dwc)
>  	}
>  
>  	dwc3_reset_gadget(dwc);
> +
> +	/*
> +	 * From SNPS databook section 8.1.2, the EP0 should be in setup
> +	 * phase. So ensure that EP0 is in setup phase by issuing a stall
> +	 * and restart if EP0 is not in setup phase.
> +	 */

We should not issue End Transfer to control endpoint unless it's error
case such as invalid direction. The databook also specify under section
USB reset initialization to not send End Transfer to control endpoint.

> +	if (dwc->ep0state != EP0_SETUP_PHASE) {
> +		unsigned int	dir;
> +
> +		dir = !!dwc->ep0_expect_in;
> +		if (dwc->ep0state == EP0_DATA_PHASE)
> +			dwc3_ep0_end_control_data(dwc, dwc->eps[dir]);
> +		else
> +			dwc3_ep0_end_control_data(dwc, dwc->eps[!dir]);

Also note that the status stage direction can be IN or OUT depending on
the data direction.

> +
> +		dwc->eps[0]->trb_enqueue = 0;
> +		dwc->eps[1]->trb_enqueue = 0;
> +
> +		dwc3_ep0_stall_and_restart(dwc);
> +	}
> +
>  	/*
>  	 * In the Synopsis DesignWare Cores USB3 Databook Rev. 3.30a
>  	 * Section 4.1.2 Table 4-2, it states that during a USB reset, the SW
> diff --git a/drivers/usb/dwc3/gadget.h b/drivers/usb/dwc3/gadget.h
> index f763380..55a56cf67 100644
> --- a/drivers/usb/dwc3/gadget.h
> +++ b/drivers/usb/dwc3/gadget.h
> @@ -110,6 +110,8 @@ void dwc3_gadget_giveback(struct dwc3_ep *dep, struct dwc3_request *req,
>  void dwc3_ep0_interrupt(struct dwc3 *dwc,
>  		const struct dwc3_event_depevt *event);
>  void dwc3_ep0_out_start(struct dwc3 *dwc);
> +void dwc3_ep0_end_control_data(struct dwc3 *dwc, struct dwc3_ep *dep);
> +void dwc3_ep0_stall_and_restart(struct dwc3 *dwc);
>  int __dwc3_gadget_ep0_set_halt(struct usb_ep *ep, int value);
>  int dwc3_gadget_ep0_set_halt(struct usb_ep *ep, int value);
>  int dwc3_gadget_ep0_queue(struct usb_ep *ep, struct usb_request *request,

How we should handle this is if there's a disconnect in the middle of a
control transfer, on the next connection, the controller will complete
the Data/Status TRB with "Setup Pending Status" bit set if the host
issues a Setup packet. In that case, we would prepare the TRB for the
Setup stage on Data completion.

Please review the control transfer programming flow on how to handle
this case and fix it.

Thanks,
Thinh

Re: [PATCH] usb: dwc3: Fix ep0 handling when getting reset while doing control transfer

[Thinh Nguyen]

Thinh Nguyen wrote:

> Mayank Rana wrote:
>> @@ -3766,6 +3768,27 @@ static void dwc3_gadget_reset_interrupt(struct dwc3 *dwc)
>>  	}
>>  
>>  	dwc3_reset_gadget(dwc);
>> +
>> +	/*
>> +	 * From SNPS databook section 8.1.2, the EP0 should be in setup
>> +	 * phase. So ensure that EP0 is in setup phase by issuing a stall
>> +	 * and restart if EP0 is not in setup phase.
>> +	 */
> 
> We should not issue End Transfer to control endpoint unless it's error
> case such as invalid direction. The databook also specify under section
> USB reset initialization to not send End Transfer to control endpoint.
> 
>> +	if (dwc->ep0state != EP0_SETUP_PHASE) {
>> +		unsigned int	dir;
>> +
>> +		dir = !!dwc->ep0_expect_in;
>> +		if (dwc->ep0state == EP0_DATA_PHASE)
>> +			dwc3_ep0_end_control_data(dwc, dwc->eps[dir]);
>> +		else
>> +			dwc3_ep0_end_control_data(dwc, dwc->eps[!dir]);

On second thought, it seems cleaner to do this though the behavior is
undefined.

Do you have the driver tracepoint log capture of this scenario? It'd be
great if I can also review it.

Thanks!
Thinh