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@xxxxxxxxxxxxxx> > --- > 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
