[PATCH 0/3] usb: dwc3: gadget: simple refactoring patches

[PATCH 0/3] usb: dwc3: gadget: simple refactoring patches

[Michael Grzeschik]

This series includes some smaller refactoring patches to improve the
code quality of the dwc3 gadget. It includes no functional changes.

Michael Grzeschik (3):
  usb: dwc3: gadget: ep_queue simplify isoc start condition
  usb: dwc3: gadget: move cmd_endtransfer to extra function
  usb: dwc3: gadget: EP_DELAY_START is only handled for non isoc eps

 drivers/usb/dwc3/gadget.c | 88 +++++++++++++++++----------------------
 1 file changed, 39 insertions(+), 49 deletions(-)

-- 
2.30.2

[PATCH 1/3] usb: dwc3: gadget: ep_queue simplify isoc start condition

[Michael Grzeschik]

To improve reading the code this patch moves the cases to start_isoc or
return the function under one common condition check.

Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de>
---
 drivers/usb/dwc3/gadget.c | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
index 183b90923f51ba..0ed837323f6bd3 100644
--- a/drivers/usb/dwc3/gadget.c
+++ b/drivers/usb/dwc3/gadget.c
@@ -1913,13 +1913,11 @@ static int __dwc3_gadget_ep_queue(struct dwc3_ep *dep, struct dwc3_request *req)
 	 * errors which will force us issue EndTransfer command.
 	 */
 	if (usb_endpoint_xfer_isoc(dep->endpoint.desc)) {
-		if (!(dep->flags & DWC3_EP_PENDING_REQUEST) &&
-				!(dep->flags & DWC3_EP_TRANSFER_STARTED))
-			return 0;
-
-		if ((dep->flags & DWC3_EP_PENDING_REQUEST)) {
-			if (!(dep->flags & DWC3_EP_TRANSFER_STARTED))
+		if (!(dep->flags & DWC3_EP_TRANSFER_STARTED)) {
+			if ((dep->flags & DWC3_EP_PENDING_REQUEST))
 				return __dwc3_gadget_start_isoc(dep);
+
+			return 0;
 		}
 	}
 
-- 
2.30.2

[PATCH 2/3] usb: dwc3: gadget: move cmd_endtransfer to extra function

[Michael Grzeschik]

This patch adds the extra function dwc3_end_transfer to consolidate the
same codepath.

Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de>
---
 drivers/usb/dwc3/gadget.c | 56 +++++++++++++++++----------------------
 1 file changed, 24 insertions(+), 32 deletions(-)

diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
index 0ed837323f6bd3..b89dadaef4db9d 100644
--- a/drivers/usb/dwc3/gadget.c
+++ b/drivers/usb/dwc3/gadget.c
@@ -1788,6 +1788,27 @@ static int dwc3_gadget_start_isoc_quirk(struct dwc3_ep *dep)
 	return __dwc3_gadget_kick_transfer(dep);
 }
 
+static void dwc3_end_transfer(struct dwc3_ep *dep, bool force, bool interrupt)
+{
+	struct dwc3_gadget_ep_cmd_params params;
+	u32 cmd;
+	int ret;
+
+	cmd = DWC3_DEPCMD_ENDTRANSFER;
+	cmd |= force ? DWC3_DEPCMD_HIPRI_FORCERM : 0;
+	cmd |= interrupt ? DWC3_DEPCMD_CMDIOC : 0;
+	cmd |= DWC3_DEPCMD_PARAM(dep->resource_index);
+	memset(&params, 0, sizeof(params));
+	ret = dwc3_send_gadget_ep_cmd(dep, cmd, &params);
+	WARN_ON_ONCE(ret);
+	dep->resource_index = 0;
+
+	if (!interrupt)
+		dep->flags &= ~DWC3_EP_TRANSFER_STARTED;
+	else if (!ret)
+		dep->flags |= DWC3_EP_END_TRANSFER_PENDING;
+}
+
 static int __dwc3_gadget_start_isoc(struct dwc3_ep *dep)
 {
 	const struct usb_endpoint_descriptor *desc = dep->endpoint.desc;
@@ -1842,21 +1863,8 @@ static int __dwc3_gadget_start_isoc(struct dwc3_ep *dep)
 	 * status, issue END_TRANSFER command and retry on the next XferNotReady
 	 * event.
 	 */
-	if (ret == -EAGAIN) {
-		struct dwc3_gadget_ep_cmd_params params;
-		u32 cmd;
-
-		cmd = DWC3_DEPCMD_ENDTRANSFER |
-			DWC3_DEPCMD_CMDIOC |
-			DWC3_DEPCMD_PARAM(dep->resource_index);
-
-		dep->resource_index = 0;
-		memset(&params, 0, sizeof(params));
-
-		ret = dwc3_send_gadget_ep_cmd(dep, cmd, &params);
-		if (!ret)
-			dep->flags |= DWC3_EP_END_TRANSFER_PENDING;
-	}
+	if (ret == -EAGAIN)
+		dwc3_end_transfer(dep, false, true);
 
 	return ret;
 }
@@ -3597,10 +3605,6 @@ static void dwc3_reset_gadget(struct dwc3 *dwc)
 static void dwc3_stop_active_transfer(struct dwc3_ep *dep, bool force,
 	bool interrupt)
 {
-	struct dwc3_gadget_ep_cmd_params params;
-	u32 cmd;
-	int ret;
-
 	if (!(dep->flags & DWC3_EP_TRANSFER_STARTED) ||
 	    (dep->flags & DWC3_EP_END_TRANSFER_PENDING))
 		return;
@@ -3632,19 +3636,7 @@ static void dwc3_stop_active_transfer(struct dwc3_ep *dep, bool force,
 	 * This mode is NOT available on the DWC_usb31 IP.
 	 */
 
-	cmd = DWC3_DEPCMD_ENDTRANSFER;
-	cmd |= force ? DWC3_DEPCMD_HIPRI_FORCERM : 0;
-	cmd |= interrupt ? DWC3_DEPCMD_CMDIOC : 0;
-	cmd |= DWC3_DEPCMD_PARAM(dep->resource_index);
-	memset(&params, 0, sizeof(params));
-	ret = dwc3_send_gadget_ep_cmd(dep, cmd, &params);
-	WARN_ON_ONCE(ret);
-	dep->resource_index = 0;
-
-	if (!interrupt)
-		dep->flags &= ~DWC3_EP_TRANSFER_STARTED;
-	else
-		dep->flags |= DWC3_EP_END_TRANSFER_PENDING;
+	dwc3_end_transfer(dep, force, interrupt);
 }
 
 static void dwc3_clear_stall_all_ep(struct dwc3 *dwc)
-- 
2.30.2

[PATCH 3/3] usb: dwc3: gadget: EP_DELAY_START is only handled for non isoc eps

[Michael Grzeschik]

Refactor the codepath for handling DWC3_EP_DELAY_START condition
only being checked on non isoc endpoints.

Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de>
---
 drivers/usb/dwc3/gadget.c | 22 +++++++++++-----------
 1 file changed, 11 insertions(+), 11 deletions(-)

diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
index b89dadaef4db9d..d09bd66f498a69 100644
--- a/drivers/usb/dwc3/gadget.c
+++ b/drivers/usb/dwc3/gadget.c
@@ -1901,17 +1901,6 @@ static int __dwc3_gadget_ep_queue(struct dwc3_ep *dep, struct dwc3_request *req)
 	if (dep->flags & DWC3_EP_WAIT_TRANSFER_COMPLETE)
 		return 0;
 
-	/*
-	 * Start the transfer only after the END_TRANSFER is completed
-	 * and endpoint STALL is cleared.
-	 */
-	if ((dep->flags & DWC3_EP_END_TRANSFER_PENDING) ||
-	    (dep->flags & DWC3_EP_WEDGE) ||
-	    (dep->flags & DWC3_EP_STALL)) {
-		dep->flags |= DWC3_EP_DELAY_START;
-		return 0;
-	}
-
 	/*
 	 * NOTICE: Isochronous endpoints should NEVER be prestarted. We must
 	 * wait for a XferNotReady event so we will know what's the current
@@ -1927,6 +1916,17 @@ static int __dwc3_gadget_ep_queue(struct dwc3_ep *dep, struct dwc3_request *req)
 
 			return 0;
 		}
+	} else {
+		/*
+		 * Start the transfer only after the END_TRANSFER is completed
+		 * and endpoint STALL is cleared.
+		 */
+		if ((dep->flags & DWC3_EP_END_TRANSFER_PENDING) ||
+		    (dep->flags & DWC3_EP_WEDGE) ||
+		    (dep->flags & DWC3_EP_STALL)) {
+			dep->flags |= DWC3_EP_DELAY_START;
+			return 0;
+		}
 	}
 
 	__dwc3_gadget_kick_transfer(dep);
-- 
2.30.2

Re: [PATCH 3/3] usb: dwc3: gadget: EP_DELAY_START is only handled for non isoc eps

[Thinh Nguyen]

Hi,

Michael Grzeschik wrote:
> Refactor the codepath for handling DWC3_EP_DELAY_START condition
> only being checked on non isoc endpoints.

The DWC3_EP_DELAY_START should still be applicable to isoc and End
Transfer pending. While the End Transfer command is active, don't issue
Start Transfer command.

Previously I think we have a check for the isoc endpoint and
DWC3_EP_DELAY_START because it was intended to check against the halt
condition, but it was done incorrectly. (Note that isoc endpoint doesn't
halt and there's no STALL handshake).

This change should not be applied. If we're to apply the fix for isoc
and delay start check, it should be done separately.

Thanks,
Thinh

> 
> Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de>
> ---
>  drivers/usb/dwc3/gadget.c | 22 +++++++++++-----------
>  1 file changed, 11 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
> index b89dadaef4db9d..d09bd66f498a69 100644
> --- a/drivers/usb/dwc3/gadget.c
> +++ b/drivers/usb/dwc3/gadget.c
> @@ -1901,17 +1901,6 @@ static int __dwc3_gadget_ep_queue(struct dwc3_ep *dep, struct dwc3_request *req)
>  	if (dep->flags & DWC3_EP_WAIT_TRANSFER_COMPLETE)
>  		return 0;
>  
> -	/*
> -	 * Start the transfer only after the END_TRANSFER is completed
> -	 * and endpoint STALL is cleared.
> -	 */
> -	if ((dep->flags & DWC3_EP_END_TRANSFER_PENDING) ||
> -	    (dep->flags & DWC3_EP_WEDGE) ||
> -	    (dep->flags & DWC3_EP_STALL)) {
> -		dep->flags |= DWC3_EP_DELAY_START;
> -		return 0;
> -	}
> -
>  	/*
>  	 * NOTICE: Isochronous endpoints should NEVER be prestarted. We must
>  	 * wait for a XferNotReady event so we will know what's the current
> @@ -1927,6 +1916,17 @@ static int __dwc3_gadget_ep_queue(struct dwc3_ep *dep, struct dwc3_request *req)
>  
>  			return 0;
>  		}
> +	} else {
> +		/*
> +		 * Start the transfer only after the END_TRANSFER is completed
> +		 * and endpoint STALL is cleared.
> +		 */
> +		if ((dep->flags & DWC3_EP_END_TRANSFER_PENDING) ||
> +		    (dep->flags & DWC3_EP_WEDGE) ||
> +		    (dep->flags & DWC3_EP_STALL)) {
> +			dep->flags |= DWC3_EP_DELAY_START;
> +			return 0;
> +		}
>  	}
>  
>  	__dwc3_gadget_kick_transfer(dep);

Re: [PATCH 3/3] usb: dwc3: gadget: EP_DELAY_START is only handled for non isoc eps

[Michael Grzeschik]

[-- Attachment #1: Type: text/plain, Size: 3071 bytes --]

On Tue, Mar 01, 2022 at 01:12:37AM +0000, Thinh Nguyen wrote:
>Hi,
>
>Michael Grzeschik wrote:
>> Refactor the codepath for handling DWC3_EP_DELAY_START condition
>> only being checked on non isoc endpoints.
>
>The DWC3_EP_DELAY_START should still be applicable to isoc and End
>Transfer pending. While the End Transfer command is active, don't issue
>Start Transfer command.
>
>Previously I think we have a check for the isoc endpoint and
>DWC3_EP_DELAY_START because it was intended to check against the halt
>condition, but it was done incorrectly. (Note that isoc endpoint doesn't
>halt and there's no STALL handshake).
>
>This change should not be applied. If we're to apply the fix for isoc
>and delay start check, it should be done separately.

Right! I just realized that we indeed at least also have to check for
DWC3_EP_END_TRANSFER_PENDING flag on isoc transfers. Without that,
we would open up a race where we kick(update) transfers, that are
potentially to late for that current transfer.

So yes, please drop that patch. The other patches on the other hand are
fine I think.

Regards,
Michael

>> Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de>
>> ---
>>  drivers/usb/dwc3/gadget.c | 22 +++++++++++-----------
>>  1 file changed, 11 insertions(+), 11 deletions(-)
>>
>> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
>> index b89dadaef4db9d..d09bd66f498a69 100644
>> --- a/drivers/usb/dwc3/gadget.c
>> +++ b/drivers/usb/dwc3/gadget.c
>> @@ -1901,17 +1901,6 @@ static int __dwc3_gadget_ep_queue(struct dwc3_ep *dep, struct dwc3_request *req)
>>  	if (dep->flags & DWC3_EP_WAIT_TRANSFER_COMPLETE)
>>  		return 0;
>>
>> -	/*
>> -	 * Start the transfer only after the END_TRANSFER is completed
>> -	 * and endpoint STALL is cleared.
>> -	 */
>> -	if ((dep->flags & DWC3_EP_END_TRANSFER_PENDING) ||
>> -	    (dep->flags & DWC3_EP_WEDGE) ||
>> -	    (dep->flags & DWC3_EP_STALL)) {
>> -		dep->flags |= DWC3_EP_DELAY_START;
>> -		return 0;
>> -	}
>> -
>>  	/*
>>  	 * NOTICE: Isochronous endpoints should NEVER be prestarted. We must
>>  	 * wait for a XferNotReady event so we will know what's the current
>> @@ -1927,6 +1916,17 @@ static int __dwc3_gadget_ep_queue(struct dwc3_ep *dep, struct dwc3_request *req)
>>
>>  			return 0;
>>  		}
>> +	} else {
>> +		/*
>> +		 * Start the transfer only after the END_TRANSFER is completed
>> +		 * and endpoint STALL is cleared.
>> +		 */
>> +		if ((dep->flags & DWC3_EP_END_TRANSFER_PENDING) ||
>> +		    (dep->flags & DWC3_EP_WEDGE) ||
>> +		    (dep->flags & DWC3_EP_STALL)) {
>> +			dep->flags |= DWC3_EP_DELAY_START;
>> +			return 0;
>> +		}
>>  	}
>>
>>  	__dwc3_gadget_kick_transfer(dep);
>

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 2/3] usb: dwc3: gadget: move cmd_endtransfer to extra function

[Thinh Nguyen]

Hi,

Michael Grzeschik wrote:
> This patch adds the extra function dwc3_end_transfer to consolidate the
> same codepath.
> 
> Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de>
> ---
>  drivers/usb/dwc3/gadget.c | 56 +++++++++++++++++----------------------
>  1 file changed, 24 insertions(+), 32 deletions(-)
> 
> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
> index 0ed837323f6bd3..b89dadaef4db9d 100644
> --- a/drivers/usb/dwc3/gadget.c
> +++ b/drivers/usb/dwc3/gadget.c
> @@ -1788,6 +1788,27 @@ static int dwc3_gadget_start_isoc_quirk(struct dwc3_ep *dep)
>  	return __dwc3_gadget_kick_transfer(dep);
>  }
>  
> +static void dwc3_end_transfer(struct dwc3_ep *dep, bool force, bool interrupt)

Maybe we can name this as __dwc3_stop_active_transfer() to be a bit more
consistent in other places of this driver?

Place this function above dwc3_gadget_start_isoc_quirk(). Also, can we
write a brief doc describing this function here as well? I got a feeling
that not many know what setting "force" mean to the controller. If
ForceRM is set, then the controller won't update the TRB progress on
command completion or clearing HWO bit. It doesn't mean that the command
will complete immediately.

Thanks,
Thinh

> +{
> +	struct dwc3_gadget_ep_cmd_params params;
> +	u32 cmd;
> +	int ret;
> +
> +	cmd = DWC3_DEPCMD_ENDTRANSFER;
> +	cmd |= force ? DWC3_DEPCMD_HIPRI_FORCERM : 0;
> +	cmd |= interrupt ? DWC3_DEPCMD_CMDIOC : 0;
> +	cmd |= DWC3_DEPCMD_PARAM(dep->resource_index);
> +	memset(&params, 0, sizeof(params));
> +	ret = dwc3_send_gadget_ep_cmd(dep, cmd, &params);
> +	WARN_ON_ONCE(ret);
> +	dep->resource_index = 0;
> +
> +	if (!interrupt)
> +		dep->flags &= ~DWC3_EP_TRANSFER_STARTED;
> +	else if (!ret)
> +		dep->flags |= DWC3_EP_END_TRANSFER_PENDING;
> +}
> +
>  static int __dwc3_gadget_start_isoc(struct dwc3_ep *dep)
>  {
>  	const struct usb_endpoint_descriptor *desc = dep->endpoint.desc;
> @@ -1842,21 +1863,8 @@ static int __dwc3_gadget_start_isoc(struct dwc3_ep *dep)
>  	 * status, issue END_TRANSFER command and retry on the next XferNotReady
>  	 * event.
>  	 */
> -	if (ret == -EAGAIN) {
> -		struct dwc3_gadget_ep_cmd_params params;
> -		u32 cmd;
> -
> -		cmd = DWC3_DEPCMD_ENDTRANSFER |
> -			DWC3_DEPCMD_CMDIOC |
> -			DWC3_DEPCMD_PARAM(dep->resource_index);
> -
> -		dep->resource_index = 0;
> -		memset(&params, 0, sizeof(params));
> -
> -		ret = dwc3_send_gadget_ep_cmd(dep, cmd, &params);
> -		if (!ret)
> -			dep->flags |= DWC3_EP_END_TRANSFER_PENDING;
> -	}
> +	if (ret == -EAGAIN)
> +		dwc3_end_transfer(dep, false, true);
>  
>  	return ret;
>  }
> @@ -3597,10 +3605,6 @@ static void dwc3_reset_gadget(struct dwc3 *dwc)
>  static void dwc3_stop_active_transfer(struct dwc3_ep *dep, bool force,
>  	bool interrupt)
>  {
> -	struct dwc3_gadget_ep_cmd_params params;
> -	u32 cmd;
> -	int ret;
> -
>  	if (!(dep->flags & DWC3_EP_TRANSFER_STARTED) ||
>  	    (dep->flags & DWC3_EP_END_TRANSFER_PENDING))
>  		return;
> @@ -3632,19 +3636,7 @@ static void dwc3_stop_active_transfer(struct dwc3_ep *dep, bool force,
>  	 * This mode is NOT available on the DWC_usb31 IP.
>  	 */
>  
> -	cmd = DWC3_DEPCMD_ENDTRANSFER;
> -	cmd |= force ? DWC3_DEPCMD_HIPRI_FORCERM : 0;
> -	cmd |= interrupt ? DWC3_DEPCMD_CMDIOC : 0;
> -	cmd |= DWC3_DEPCMD_PARAM(dep->resource_index);
> -	memset(&params, 0, sizeof(params));
> -	ret = dwc3_send_gadget_ep_cmd(dep, cmd, &params);
> -	WARN_ON_ONCE(ret);
> -	dep->resource_index = 0;
> -
> -	if (!interrupt)
> -		dep->flags &= ~DWC3_EP_TRANSFER_STARTED;
> -	else
> -		dep->flags |= DWC3_EP_END_TRANSFER_PENDING;
> +	dwc3_end_transfer(dep, force, interrupt);
>  }
>  
>  static void dwc3_clear_stall_all_ep(struct dwc3 *dwc)

Re: [PATCH 1/3] usb: dwc3: gadget: ep_queue simplify isoc start condition

[Thinh Nguyen]

Michael Grzeschik wrote:
> To improve reading the code this patch moves the cases to start_isoc or
> return the function under one common condition check.
> 
> Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de>
> ---
>  drivers/usb/dwc3/gadget.c | 10 ++++------
>  1 file changed, 4 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
> index 183b90923f51ba..0ed837323f6bd3 100644
> --- a/drivers/usb/dwc3/gadget.c
> +++ b/drivers/usb/dwc3/gadget.c
> @@ -1913,13 +1913,11 @@ static int __dwc3_gadget_ep_queue(struct dwc3_ep *dep, struct dwc3_request *req)
>  	 * errors which will force us issue EndTransfer command.
>  	 */
>  	if (usb_endpoint_xfer_isoc(dep->endpoint.desc)) {
> -		if (!(dep->flags & DWC3_EP_PENDING_REQUEST) &&
> -				!(dep->flags & DWC3_EP_TRANSFER_STARTED))
> -			return 0;
> -
> -		if ((dep->flags & DWC3_EP_PENDING_REQUEST)) {
> -			if (!(dep->flags & DWC3_EP_TRANSFER_STARTED))
> +		if (!(dep->flags & DWC3_EP_TRANSFER_STARTED)) {
> +			if ((dep->flags & DWC3_EP_PENDING_REQUEST))
>  				return __dwc3_gadget_start_isoc(dep);
> +
> +			return 0;
>  		}
>  	}
>  

This looks much better.

Reviewed-by: Thinh Nguyen <Thinh.Nguyen@synopsys.com>

Thanks!
Thinh

[PATCH v2] usb: dwc3: gadget: move cmd_endtransfer to extra function

[Michael Grzeschik]

This patch adds the extra function __dwc3_stop_active_transfer to
consolidate the same codepath.

Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de>
---
 drivers/usb/dwc3/gadget.c | 68 +++++++++++++++++++++------------------
 1 file changed, 36 insertions(+), 32 deletions(-)

diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
index 37dbf6132731b7..baddf6196ceee2 100644
--- a/drivers/usb/dwc3/gadget.c
+++ b/drivers/usb/dwc3/gadget.c
@@ -1673,6 +1673,39 @@ static int __dwc3_gadget_get_frame(struct dwc3 *dwc)
 	return DWC3_DSTS_SOFFN(reg);
 }
 
+/**
+ * __dwc3_stop_active_transfer - stop the current active transfer
+ * @dep: isoc endpoint
+ * @force: set forcerm bit in the command
+ * @interrupt: command complete interrupt after End Transfer command
+ *
+ * When setting force, the ForceRM bit will be set. In that case
+ * the controller won't update the TRB progress on command
+ * completion. It also won't clear the HWO bit in the TRB.
+ * The command will also not complete immediatly in that case.
+ */
+
+static void __dwc3_stop_active_transfer(struct dwc3_ep *dep, bool force, bool interrupt)
+{
+	struct dwc3_gadget_ep_cmd_params params;
+	u32 cmd;
+	int ret;
+
+	cmd = DWC3_DEPCMD_ENDTRANSFER;
+	cmd |= force ? DWC3_DEPCMD_HIPRI_FORCERM : 0;
+	cmd |= interrupt ? DWC3_DEPCMD_CMDIOC : 0;
+	cmd |= DWC3_DEPCMD_PARAM(dep->resource_index);
+	memset(&params, 0, sizeof(params));
+	ret = dwc3_send_gadget_ep_cmd(dep, cmd, &params);
+	WARN_ON_ONCE(ret);
+	dep->resource_index = 0;
+
+	if (!interrupt)
+		dep->flags &= ~DWC3_EP_TRANSFER_STARTED;
+	else if (!ret)
+		dep->flags |= DWC3_EP_END_TRANSFER_PENDING;
+}
+
 /**
  * dwc3_gadget_start_isoc_quirk - workaround invalid frame number
  * @dep: isoc endpoint
@@ -1842,21 +1875,8 @@ static int __dwc3_gadget_start_isoc(struct dwc3_ep *dep)
 	 * status, issue END_TRANSFER command and retry on the next XferNotReady
 	 * event.
 	 */
-	if (ret == -EAGAIN) {
-		struct dwc3_gadget_ep_cmd_params params;
-		u32 cmd;
-
-		cmd = DWC3_DEPCMD_ENDTRANSFER |
-			DWC3_DEPCMD_CMDIOC |
-			DWC3_DEPCMD_PARAM(dep->resource_index);
-
-		dep->resource_index = 0;
-		memset(&params, 0, sizeof(params));
-
-		ret = dwc3_send_gadget_ep_cmd(dep, cmd, &params);
-		if (!ret)
-			dep->flags |= DWC3_EP_END_TRANSFER_PENDING;
-	}
+	if (ret == -EAGAIN)
+		__dwc3_stop_active_transfer(dep, false, true);
 
 	return ret;
 }
@@ -3597,10 +3617,6 @@ static void dwc3_reset_gadget(struct dwc3 *dwc)
 static void dwc3_stop_active_transfer(struct dwc3_ep *dep, bool force,
 	bool interrupt)
 {
-	struct dwc3_gadget_ep_cmd_params params;
-	u32 cmd;
-	int ret;
-
 	if (!(dep->flags & DWC3_EP_TRANSFER_STARTED) ||
 	    (dep->flags & DWC3_EP_END_TRANSFER_PENDING))
 		return;
@@ -3632,19 +3648,7 @@ static void dwc3_stop_active_transfer(struct dwc3_ep *dep, bool force,
 	 * This mode is NOT available on the DWC_usb31 IP.
 	 */
 
-	cmd = DWC3_DEPCMD_ENDTRANSFER;
-	cmd |= force ? DWC3_DEPCMD_HIPRI_FORCERM : 0;
-	cmd |= interrupt ? DWC3_DEPCMD_CMDIOC : 0;
-	cmd |= DWC3_DEPCMD_PARAM(dep->resource_index);
-	memset(&params, 0, sizeof(params));
-	ret = dwc3_send_gadget_ep_cmd(dep, cmd, &params);
-	WARN_ON_ONCE(ret);
-	dep->resource_index = 0;
-
-	if (!interrupt)
-		dep->flags &= ~DWC3_EP_TRANSFER_STARTED;
-	else
-		dep->flags |= DWC3_EP_END_TRANSFER_PENDING;
+	__dwc3_stop_active_transfer(dep, force, interrupt);
 }
 
 static void dwc3_clear_stall_all_ep(struct dwc3 *dwc)
-- 
2.30.2

Re: [PATCH v2] usb: dwc3: gadget: move cmd_endtransfer to extra function

[Thinh Nguyen]

Hi,

Michael Grzeschik wrote:
> This patch adds the extra function __dwc3_stop_active_transfer to
> consolidate the same codepath.
> 
> Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de>
> ---

Add a comment here on what's changed since previous version.

>  drivers/usb/dwc3/gadget.c | 68 +++++++++++++++++++++------------------
>  1 file changed, 36 insertions(+), 32 deletions(-)
> 
> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
> index 37dbf6132731b7..baddf6196ceee2 100644
> --- a/drivers/usb/dwc3/gadget.c
> +++ b/drivers/usb/dwc3/gadget.c
> @@ -1673,6 +1673,39 @@ static int __dwc3_gadget_get_frame(struct dwc3 *dwc)
>  	return DWC3_DSTS_SOFFN(reg);
>  }
>  
> +/**
> + * __dwc3_stop_active_transfer - stop the current active transfer
> + * @dep: isoc endpoint
> + * @force: set forcerm bit in the command
> + * @interrupt: command complete interrupt after End Transfer command
> + *
> + * When setting force, the ForceRM bit will be set. In that case
> + * the controller won't update the TRB progress on command
> + * completion. It also won't clear the HWO bit in the TRB.
> + * The command will also not complete immediatly in that case.

"immediately"

> + */

Remove blank line

> +
> +static void __dwc3_stop_active_transfer(struct dwc3_ep *dep, bool force, bool interrupt)

Can we make this return "int"?

> +{
> +	struct dwc3_gadget_ep_cmd_params params;
> +	u32 cmd;
> +	int ret;
> +
> +	cmd = DWC3_DEPCMD_ENDTRANSFER;
> +	cmd |= force ? DWC3_DEPCMD_HIPRI_FORCERM : 0;
> +	cmd |= interrupt ? DWC3_DEPCMD_CMDIOC : 0;
> +	cmd |= DWC3_DEPCMD_PARAM(dep->resource_index);
> +	memset(&params, 0, sizeof(params));
> +	ret = dwc3_send_gadget_ep_cmd(dep, cmd, &params);
> +	WARN_ON_ONCE(ret);
> +	dep->resource_index = 0;
> +
> +	if (!interrupt)
> +		dep->flags &= ~DWC3_EP_TRANSFER_STARTED;
> +	else if (!ret)
> +		dep->flags |= DWC3_EP_END_TRANSFER_PENDING;

So we can return ret here.

> +}
> +
>  /**
>   * dwc3_gadget_start_isoc_quirk - workaround invalid frame number
>   * @dep: isoc endpoint
> @@ -1842,21 +1875,8 @@ static int __dwc3_gadget_start_isoc(struct dwc3_ep *dep)
>  	 * status, issue END_TRANSFER command and retry on the next XferNotReady
>  	 * event.
>  	 */
> -	if (ret == -EAGAIN) {
> -		struct dwc3_gadget_ep_cmd_params params;
> -		u32 cmd;
> -
> -		cmd = DWC3_DEPCMD_ENDTRANSFER |
> -			DWC3_DEPCMD_CMDIOC |
> -			DWC3_DEPCMD_PARAM(dep->resource_index);
> -
> -		dep->resource_index = 0;
> -		memset(&params, 0, sizeof(params));
> -
> -		ret = dwc3_send_gadget_ep_cmd(dep, cmd, &params);
> -		if (!ret)
> -			dep->flags |= DWC3_EP_END_TRANSFER_PENDING;
> -	}
> +	if (ret == -EAGAIN)
> +		__dwc3_stop_active_transfer(dep, false, true);

if (ret == -EAGAIN)
	ret = __dwc3_stop_active_transfer(dep, false, true);

>  
>  	return ret;
>  }
> @@ -3597,10 +3617,6 @@ static void dwc3_reset_gadget(struct dwc3 *dwc)
>  static void dwc3_stop_active_transfer(struct dwc3_ep *dep, bool force,
>  	bool interrupt)
>  {
> -	struct dwc3_gadget_ep_cmd_params params;
> -	u32 cmd;
> -	int ret;
> -
>  	if (!(dep->flags & DWC3_EP_TRANSFER_STARTED) ||
>  	    (dep->flags & DWC3_EP_END_TRANSFER_PENDING))
>  		return;
> @@ -3632,19 +3648,7 @@ static void dwc3_stop_active_transfer(struct dwc3_ep *dep, bool force,
>  	 * This mode is NOT available on the DWC_usb31 IP.
>  	 */
>  
> -	cmd = DWC3_DEPCMD_ENDTRANSFER;
> -	cmd |= force ? DWC3_DEPCMD_HIPRI_FORCERM : 0;
> -	cmd |= interrupt ? DWC3_DEPCMD_CMDIOC : 0;
> -	cmd |= DWC3_DEPCMD_PARAM(dep->resource_index);
> -	memset(&params, 0, sizeof(params));
> -	ret = dwc3_send_gadget_ep_cmd(dep, cmd, &params);
> -	WARN_ON_ONCE(ret);
> -	dep->resource_index = 0;
> -
> -	if (!interrupt)
> -		dep->flags &= ~DWC3_EP_TRANSFER_STARTED;
> -	else
> -		dep->flags |= DWC3_EP_END_TRANSFER_PENDING;
> +	__dwc3_stop_active_transfer(dep, force, interrupt);
>  }
>  
>  static void dwc3_clear_stall_all_ep(struct dwc3 *dwc)

Thanks,
Thinh

[PATCH v3] usb: dwc3: gadget: move cmd_endtransfer to extra function

[Michael Grzeschik]

This patch adds the extra function __dwc3_stop_active_transfer to
consolidate the same codepath.

Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de>

---
v1 -> v2: - renamed the function to __dwc3_stop_active_transfer
          - added function description
v2 -> v3: - fixed spelling and removed extra line
	  - make __dwc3_stop_active_transfer return ret
	  - use ret in __dwc3_gadget_start_isoc
---
 drivers/usb/dwc3/gadget.c | 69 +++++++++++++++++++++------------------
 1 file changed, 37 insertions(+), 32 deletions(-)

diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
index 37dbf6132731b7..bd9d8494c90b73 100644
--- a/drivers/usb/dwc3/gadget.c
+++ b/drivers/usb/dwc3/gadget.c
@@ -1673,6 +1673,40 @@ static int __dwc3_gadget_get_frame(struct dwc3 *dwc)
 	return DWC3_DSTS_SOFFN(reg);
 }
 
+/**
+ * __dwc3_stop_active_transfer - stop the current active transfer
+ * @dep: isoc endpoint
+ * @force: set forcerm bit in the command
+ * @interrupt: command complete interrupt after End Transfer command
+ *
+ * When setting force, the ForceRM bit will be set. In that case
+ * the controller won't update the TRB progress on command
+ * completion. It also won't clear the HWO bit in the TRB.
+ * The command will also not complete immediately in that case.
+ */
+static int __dwc3_stop_active_transfer(struct dwc3_ep *dep, bool force, bool interrupt)
+{
+	struct dwc3_gadget_ep_cmd_params params;
+	u32 cmd;
+	int ret;
+
+	cmd = DWC3_DEPCMD_ENDTRANSFER;
+	cmd |= force ? DWC3_DEPCMD_HIPRI_FORCERM : 0;
+	cmd |= interrupt ? DWC3_DEPCMD_CMDIOC : 0;
+	cmd |= DWC3_DEPCMD_PARAM(dep->resource_index);
+	memset(&params, 0, sizeof(params));
+	ret = dwc3_send_gadget_ep_cmd(dep, cmd, &params);
+	WARN_ON_ONCE(ret);
+	dep->resource_index = 0;
+
+	if (!interrupt)
+		dep->flags &= ~DWC3_EP_TRANSFER_STARTED;
+	else if (!ret)
+		dep->flags |= DWC3_EP_END_TRANSFER_PENDING;
+
+	return ret;
+}
+
 /**
  * dwc3_gadget_start_isoc_quirk - workaround invalid frame number
  * @dep: isoc endpoint
@@ -1842,21 +1876,8 @@ static int __dwc3_gadget_start_isoc(struct dwc3_ep *dep)
 	 * status, issue END_TRANSFER command and retry on the next XferNotReady
 	 * event.
 	 */
-	if (ret == -EAGAIN) {
-		struct dwc3_gadget_ep_cmd_params params;
-		u32 cmd;
-
-		cmd = DWC3_DEPCMD_ENDTRANSFER |
-			DWC3_DEPCMD_CMDIOC |
-			DWC3_DEPCMD_PARAM(dep->resource_index);
-
-		dep->resource_index = 0;
-		memset(&params, 0, sizeof(params));
-
-		ret = dwc3_send_gadget_ep_cmd(dep, cmd, &params);
-		if (!ret)
-			dep->flags |= DWC3_EP_END_TRANSFER_PENDING;
-	}
+	if (ret == -EAGAIN)
+		ret = __dwc3_stop_active_transfer(dep, false, true);
 
 	return ret;
 }
@@ -3597,10 +3618,6 @@ static void dwc3_reset_gadget(struct dwc3 *dwc)
 static void dwc3_stop_active_transfer(struct dwc3_ep *dep, bool force,
 	bool interrupt)
 {
-	struct dwc3_gadget_ep_cmd_params params;
-	u32 cmd;
-	int ret;
-
 	if (!(dep->flags & DWC3_EP_TRANSFER_STARTED) ||
 	    (dep->flags & DWC3_EP_END_TRANSFER_PENDING))
 		return;
@@ -3632,19 +3649,7 @@ static void dwc3_stop_active_transfer(struct dwc3_ep *dep, bool force,
 	 * This mode is NOT available on the DWC_usb31 IP.
 	 */
 
-	cmd = DWC3_DEPCMD_ENDTRANSFER;
-	cmd |= force ? DWC3_DEPCMD_HIPRI_FORCERM : 0;
-	cmd |= interrupt ? DWC3_DEPCMD_CMDIOC : 0;
-	cmd |= DWC3_DEPCMD_PARAM(dep->resource_index);
-	memset(&params, 0, sizeof(params));
-	ret = dwc3_send_gadget_ep_cmd(dep, cmd, &params);
-	WARN_ON_ONCE(ret);
-	dep->resource_index = 0;
-
-	if (!interrupt)
-		dep->flags &= ~DWC3_EP_TRANSFER_STARTED;
-	else
-		dep->flags |= DWC3_EP_END_TRANSFER_PENDING;
+	__dwc3_stop_active_transfer(dep, force, interrupt);
 }
 
 static void dwc3_clear_stall_all_ep(struct dwc3 *dwc)
-- 
2.30.2

Re: [PATCH v3] usb: dwc3: gadget: move cmd_endtransfer to extra function

[Greg KH]

On Sat, Mar 05, 2022 at 01:53:56AM +0100, Michael Grzeschik wrote:
> This patch adds the extra function __dwc3_stop_active_transfer to
> consolidate the same codepath.
> 
> Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de>
> 
> ---
> v1 -> v2: - renamed the function to __dwc3_stop_active_transfer
>           - added function description
> v2 -> v3: - fixed spelling and removed extra line
> 	  - make __dwc3_stop_active_transfer return ret
> 	  - use ret in __dwc3_gadget_start_isoc

So is this v3 of a single patch in this series, or the whole series?

I'm totally confused, please resend the whole, updated series, as v4
please.

thanks,

greg k-h