Re: [PATCH v2] usb: dwc3: gadget: check drained isoc ep

[Thinh Nguyen <Thinh.Nguyen@synopsys.com>]

On Tue, Apr 02, 2024, Michael Grzeschik wrote:
> To avoid a potential underrun of an currently drained transfer
> we add a check for that scenario in the dwc3_gadget_endpoint_trbs_complete
> function. In the case of an empty trb ring, we call the stop_transfer
> cmd and avoid the underrun to occur.
> 
> Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de>
> ---
> Changes in v2:
> - dropped patch "usb: dwc3: gadget: reclaim the whole started list when request was missed"
> - fixed patch "usb: dwc3: gadget: check drained isoc ep"
> - dropped patch "usb: dwc3: gadget: check the whole started queue for missed requests in complete"
> - Link to v1: https://urldefense.com/v3/__https://lore.kernel.org/r/20240307-dwc3-gadget-complete-irq-v1-0-4fe9ac0ba2b7@pengutronix.de__;!!A4F2R9G_pg!YJuK4jZdVlYzdzdE5lt1cz-ajd_iGtlOl4G9ERlp_n9C5VPBPQpikhvJ3BXM2AYNMg1t-qq-vDxQ2f7cj2zZj8Es0vvzknZw$ 
> ---
>  drivers/usb/dwc3/gadget.c | 23 ++++++++++++++++++-----
>  1 file changed, 18 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
> index 4df2661f66751..3e9c67799259a 100644
> --- a/drivers/usb/dwc3/gadget.c
> +++ b/drivers/usb/dwc3/gadget.c
> @@ -3582,13 +3582,26 @@ static bool dwc3_gadget_endpoint_trbs_complete(struct dwc3_ep *dep,
>  	if (!dep->endpoint.desc)
>  		return no_started_trb;
>  
> -	if (usb_endpoint_xfer_isoc(dep->endpoint.desc) &&
> -		list_empty(&dep->started_list) &&
> -		(list_empty(&dep->pending_list) || status == -EXDEV))
> -		dwc3_stop_active_transfer(dep, true, true);
> -	else if (dwc3_gadget_ep_should_continue(dep))
> +	if (usb_endpoint_xfer_isoc(dep->endpoint.desc)) {

Can we follow this comment block style:

/*
 * abc defg xyz
 * 123 1234
 */

> +		/* It is possible that the interrupt thread was delayed
> +		 * by scheduling in the system, and therefor the HW has

therefor -> therefore

> +		 * already run dry. In that case the last trb in the
> +		 * queue is already handled by the hw. By checking the
> +		 * HWO bit we know to restart the whole transfer. The
> +		 * condition to appear is more likely if not every req
> +		 * has the IOC bit set and therefor does not trigger the

therefor -> therefore

> +		 * interrupt thread frequently.
> +		 */
> +		struct dwc3_trb *trb = dwc3_ep_prev_trb(dep, dep->trb_enqueue);
> +		unsigned int transfer_in_flight = trb->ctrl & DWC3_TRB_CTRL_HWO;

Use boolean.

bool transfer_in_flight = !!(trb->ctrl & DWC3_TRB_CTRL_HWO);

Can we add an additional condition for in-flight check:
(trb->ctrl & DWC3_TRB_CTRL_HWO) || (trb->ctrl & DWC3_TRB_CTRL_CHN)

There can be a case where only partial SG request is prepared because
the TRB ring is full.

> +
> +		if ((list_empty(&dep->started_list) || !transfer_in_flight) &&
> +		    (list_empty(&dep->pending_list) || status == -EXDEV))
> +			dwc3_stop_active_transfer(dep, true, true);
> +	} else if (dwc3_gadget_ep_should_continue(dep)) {
>  		if (__dwc3_gadget_kick_transfer(dep) == 0)
>  			no_started_trb = false;
> +	}
>  
>  out:
>  	/*
> 

My concern here is for the case where transfer_in_flight == true and
list_empty(started_list) == false. That means that the requests in the
started_list are completed but are not given back to the gadget driver.

Since they remained in the started_list, they will be resubmitted again
on the next usb_ep_queue. We may send duplicate transfers right?

You can try to cleanup requests in the started_list, but you need to be
careful to make sure you're not out of sync with the transfer completion
events and new requests from gadget driver.

BR,
Thinh