Re: [PATCH v3 2/6] usb: dwc3: gadget: cancel requests instead of release after missed isoc

[Michael Grzeschik <mgr@pengutronix.de>]

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

On Thu, Mar 07, 2024 at 01:57:44AM +0000, Thinh Nguyen wrote:
>On Tue, Feb 27, 2024, Michael Grzeschik wrote:
>> On Thu, Feb 22, 2024 at 01:20:04AM +0000, Thinh Nguyen wrote:
>> > On Thu, Feb 22, 2024, Michael Grzeschik wrote:
>> > > For #2: I found an issue in the handling of the completion of requests in
>> > > the started list. When the interrupt handler is *explicitly* calling
>> > > stop_active_transfer if the overall event of the request was an missed
>> > > event. This event value only represents the value of the request that
>> > > was actually triggering the interrupt.
>> > >
>> > > It also calls ep_cleanup_completed_requests and is iterating over the
>> > > started requests and will call giveback/complete functions of the
>> > > requests with the proper request status.
>> > >
>> > > So this will also catch missed requests in the queue. However, since
>> > > there might be, lets say 5 good requests and one missed request, what
>> > > will happen is, that each complete call for the first good requests will
>> > > enqueue new requests into the started list and will also call the
>> > > updatecmd on that transfer that was already missed until the loop will
>> > > reach the one request with the MISSED status bit set.
>> > >
>> > > So in my opinion the patch from Jeff makes sense when adding the
>> > > following change aswell. With those both changes the underruns and
>> > > broken frames finally disappear. I am still unsure about the complete
>> > > solution about that, since with this the mentioned 5 good requests
>> > > will be cancelled aswell. So this is still a WIP status here.
>> > >
>> >
>> > When the dwc3 driver issues stop_active_transfer(), that means that the
>> > started_list is empty and there is an underrun.
>>
>> At this moment this is only the case when both, pending and started list
>> are empty. Or the interrupt event was EXDEV.
>>
>> The main problem is that the function
>> dwc3_gadget_ep_cleanup_completed_requests(dep, event, status); will
>> issue an complete for each started request, which on the other hand will
>> refill the pending list, and therefor after that refill the
>> stop_active_transfer is currently never hit.
>>
>> > It treats the incoming requests as staled. However, for UVC, they are
>> > still "good".
>>
>> Right, so in that case we can requeue them anyway. But this will have to
>> be done after the stop transfer cmd has finished.
>>
>> > I think you can just check if the started_list is empty before queuing
>> > new requests. If it is, perform stop_active_transfer() to reschedule the
>> > incoming requests. None of the newly queue requests will be released
>> > yet since they are in the pending_list.
>>
>> So that is basically exactly what my patch is doing. However in the case
>> of an underrun it is not safe to call dwc3_gadget_ep_cleanup_completed_requests
>> as jeff stated. So his underlying patch is really fixing an issue here.
>
>What I mean is to actively check for started list on every
>usb_ep_queue() call. Checking during
>dwc3_gadget_ep_cleanup_completed_requests() is already too late.

I see.

>>
>> > For UVC, perhaps you can introduce a new flag to usb_request called
>> > "ignore_queue_latency" or something equivalent. The dwc3 is already
>> > partially doing this for UVC. With this new flag, we can rework dwc3 to
>> > clearly separate the expected behavior from the function driver.
>>
>> I don't know why this "extra" flag is even necessary. The code example
>> is already working without that extra flag.
>
>The flag is for controller to determine what kinds of behavior the
>function driver expects. My intention is if this extra flag is not set,
>the dwc3 driver will not attempt to reshcedule isoc request at all (ie.
>no stop_active_transfer()).

Ok.

>>
>> Actually I even came up with an better solution. Additionally of checking if
>> one of the requests in the started list was missed, we can activly check if
>> the trb ring did run dry and if dwc3_gadget_endpoint_trbs_complete is
>> going to enqueue in to the empty trb ring.
>>
>> So my whole change looks like that:
>>
>> diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
>> index efe6caf4d0e87..2c8047dcd1612 100644
>> --- a/drivers/usb/dwc3/core.h
>> +++ b/drivers/usb/dwc3/core.h
>> @@ -952,6 +952,7 @@ struct dwc3_request {
>>  #define DWC3_REQUEST_STATUS_DEQUEUED		3
>>  #define DWC3_REQUEST_STATUS_STALLED		4
>>  #define DWC3_REQUEST_STATUS_COMPLETED		5
>> +#define DWC3_REQUEST_STATUS_MISSED_ISOC		6
>>  #define DWC3_REQUEST_STATUS_UNKNOWN		-1
>>  	u8			epnum;
>> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
>> index 858fe4c299b7a..a31f4d3502bd3 100644
>> --- a/drivers/usb/dwc3/gadget.c
>> +++ b/drivers/usb/dwc3/gadget.c
>> @@ -2057,6 +2057,9 @@ static void dwc3_gadget_ep_cleanup_cancelled_requests(struct dwc3_ep *dep)
>>  		req = next_request(&dep->cancelled_list);
>>  		dwc3_gadget_ep_skip_trbs(dep, req);
>>  		switch (req->status) {
>> +		case 0:
>> +			dwc3_gadget_giveback(dep, req, 0);
>> +			break;
>>  		case DWC3_REQUEST_STATUS_DISCONNECTED:
>>  			dwc3_gadget_giveback(dep, req, -ESHUTDOWN);
>>  			break;
>> @@ -2066,6 +2069,9 @@ static void dwc3_gadget_ep_cleanup_cancelled_requests(struct dwc3_ep *dep)
>>  		case DWC3_REQUEST_STATUS_STALLED:
>>  			dwc3_gadget_giveback(dep, req, -EPIPE);
>>  			break;
>> +		case DWC3_REQUEST_STATUS_MISSED_ISOC:
>> +			dwc3_gadget_giveback(dep, req, -EXDEV);
>> +			break;
>>  		default:
>>  			dev_err(dwc->dev, "request cancelled with wrong reason:%d\n", req->status);
>>  			dwc3_gadget_giveback(dep, req, -ECONNRESET);
>> @@ -3509,6 +3515,36 @@ static int dwc3_gadget_ep_cleanup_completed_request(struct dwc3_ep *dep,
>>  	return ret;
>>  }
>> +static int dwc3_gadget_ep_check_missed_requests(struct dwc3_ep *dep)
>> +{
>> +	struct dwc3_request	*req;
>> +	struct dwc3_request	*tmp;
>> +	int ret = 0;
>> +
>> +	list_for_each_entry_safe(req, tmp, &dep->started_list, list) {
>> +		struct dwc3_trb *trb;
>> +
>> +		trb = req->trb;
>> +		switch (DWC3_TRB_SIZE_TRBSTS(trb->size)) {
>> +		case DWC3_TRBSTS_MISSED_ISOC:
>> +			/* Isoc endpoint only */
>> +			ret = -EXDEV;
>> +			break;
>> +		case DWC3_TRB_STS_XFER_IN_PROG:
>> +			/* Applicable when End Transfer with ForceRM=0 */
>> +		case DWC3_TRBSTS_SETUP_PENDING:
>> +			/* Control endpoint only */
>> +		case DWC3_TRBSTS_OK:
>> +		default:
>> +			ret = 0;
>> +			break;
>> +		}
>> +	}
>> +
>> +	return ret;
>> +}
>> +
>>  static void dwc3_gadget_ep_cleanup_completed_requests(struct dwc3_ep *dep,
>>  		const struct dwc3_event_depevt *event, int status)
>>  {
>> @@ -3565,22 +3601,51 @@ static bool dwc3_gadget_endpoint_trbs_complete(struct dwc3_ep *dep,
>>  {
>>  	struct dwc3		*dwc = dep->dwc;
>>  	bool			no_started_trb = true;
>> +	unsigned int		transfer_in_flight = 0;
>> +
>> +	/* It is possible that the interrupt thread was delayed by
>> +	 * scheduling in the system, and therefor the HW has 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 likelely
>> +	 * if not every trb has the IOC bit set and therefor does not
>> +	 * trigger the interrupt thread fewer.
>> +	 */
>> +	if (dep->number && usb_endpoint_xfer_isoc(dep->endpoint.desc)) {
>> +		struct dwc3_trb *trb;
>> -	dwc3_gadget_ep_cleanup_completed_requests(dep, event, status);
>> +		trb = dwc3_ep_prev_trb(dep, dep->trb_enqueue);
>> +		transfer_in_flight = trb->ctrl & DWC3_TRB_CTRL_HWO;
>> +	}
>> -	if (dep->flags & DWC3_EP_END_TRANSFER_PENDING)
>> -		goto out;
>> +	if (status == -EXDEV || !transfer_in_flight) {
>> +		struct dwc3_request *tmp;
>> +		struct dwc3_request *req;
>> -	if (!dep->endpoint.desc)
>> -		return no_started_trb;
>> +		if (!(dep->flags & DWC3_EP_END_TRANSFER_PENDING))
>> +			dwc3_stop_active_transfer(dep, true, true);
>> -	if (usb_endpoint_xfer_isoc(dep->endpoint.desc) &&
>> -		list_empty(&dep->started_list) &&
>> -		(list_empty(&dep->pending_list) || status == -EXDEV))

@[!!here!!]

>> -		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;
>> +		list_for_each_entry_safe(req, tmp, &dep->started_list, list) {
>> +			dwc3_gadget_move_cancelled_request(req,
>> +					(DWC3_TRB_SIZE_TRBSTS(req->trb->size) == DWC3_TRBSTS_MISSED_ISOC) ?
>> +					DWC3_REQUEST_STATUS_MISSED_ISOC : 0);
>> +		}
>> +	} else {
>> +		dwc3_gadget_ep_cleanup_completed_requests(dep, event, status);
>> +
>> +		if (dep->flags & DWC3_EP_END_TRANSFER_PENDING)
>> +			goto out;
>> +
>> +		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))
>> +			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:
>>  	/*
>>
>> I will seperate the whole hunk into smaller changes and send an v1
>> the next days to review.
>>

I finally send a v1 of my series.

https://lore.kernel.org/linux-usb/20240307-dwc3-gadget-complete-irq-v1-0-4fe9ac0ba2b7@pengutronix.de/

For the rest of the discussion, I would like to move the conversation to
the newly send series.

>No, we should not reschedule for every missed-isoc. We only want to
>target underrun condition.

As you stated above, with reschedule what you mean is calling
stop_transfer after a missed transfer was seen?

If so, why is this condition in there already? (@[!!here!!])

Michael

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