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

Michael Grzeschik
2024-03-07T16:15:45Z

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