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

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

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

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