| Message ID | 20220228150843.870809-4-m.grzeschik@pengutronix.de (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | usb: dwc3: gadget: simple refactoring patches | expand |
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); >
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);
Refactor the codepath for handling DWC3_EP_DELAY_START condition only being checked on non isoc endpoints. Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de> --- drivers/usb/dwc3/gadget.c | 22 +++++++++++----------- 1 file changed, 11 insertions(+), 11 deletions(-)