Re: [PATCH 1/2] usb: dwc3: gadget: make starting isoc transfers more robust

From: Michael Olbrich
Date: Wed Nov 13 2019 - 03:02:16 EST

Hi,

On Wed, Nov 13, 2019 at 03:55:30AM +0000, Thinh Nguyen wrote:
> Michael Olbrich wrote:
> > Currently __dwc3_gadget_start_isoc must be called very shortly after
> > XferNotReady. Otherwise the frame number is outdated and start transfer
> > will fail, even with several retries.
> >
> > DSTS provides the lower 14 bit of the frame number. Use it in combination
> > with the frame number provided by XferNotReady to guess the current frame
> > number. This will succeed unless more than one 14 rollover has happened
> > since XferNotReady.
> >
> > Start transfer might still fail if the frame number is increased
> > immediately after DSTS is read. So retries are still needed.
> > Don't drop the current request if this happens. This way it is not lost and
> > can be used immediately to try again with the next frame number.
> >
> > With this change, __dwc3_gadget_start_isoc is still not successfully in all
> > cases bit it increases the acceptable delay after XferNotReady
> > significantly.
> >
> > Signed-off-by: Michael Olbrich <m.olbrich@xxxxxxxxxxxxxx>
> > ---
> > drivers/usb/dwc3/core.h | 1 +
> > drivers/usb/dwc3/gadget.c | 31 +++++++++++++++++++++++--------
> > 2 files changed, 24 insertions(+), 8 deletions(-)
> >
> > diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
> > index 3dd783b889cb..c5b223656e08 100644
> > --- a/drivers/usb/dwc3/core.h
> > +++ b/drivers/usb/dwc3/core.h
> > @@ -709,6 +709,7 @@ struct dwc3_ep {
> > u8 type;
> > u8 resource_index;
> > u32 frame_number;
> > + u32 last_frame_number;
>
> There's no need to add a new field for last_frame_number. Just store the
> value in a local variable in __dwc3_gadget_start_isoc().

I'm using it to check for rollover, so __dwc3_gadget_start_isoc does not
help. I introduced it because I caused a second (incorrect) rollover when
the first try failed. But now that I think about it, it should be possible
without the extra variable.

> > u32 interval;
> >
> > char name[20];
> > diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
> > index 173f5329d3d9..ac4673d91939 100644
> > --- a/drivers/usb/dwc3/gadget.c
> > +++ b/drivers/usb/dwc3/gadget.c
> > @@ -1204,7 +1204,7 @@ static void dwc3_prepare_trbs(struct dwc3_ep *dep)
> > }
> > }
> >
> > -static int __dwc3_gadget_kick_transfer(struct dwc3_ep *dep)
> > +static int __dwc3_gadget_kick_transfer(struct dwc3_ep *dep, bool keep_req)
> > {
> > struct dwc3_gadget_ep_cmd_params params;
> > struct dwc3_request *req;
> > @@ -1242,7 +1242,7 @@ static int __dwc3_gadget_kick_transfer(struct dwc3_ep *dep)
> > }
> >
> > ret = dwc3_send_gadget_ep_cmd(dep, cmd, &params);
> > - if (ret < 0) {
> > + if (ret < 0 && (!keep_req || ret != -EAGAIN)) {
> > /*
> > * FIXME we need to iterate over the list of requests
> > * here and stop, unmap, free and del each of the linked
> > @@ -1254,7 +1254,7 @@ static int __dwc3_gadget_kick_transfer(struct dwc3_ep *dep)
> > return ret;
> > }
> >
> > - return 0;
> > + return ret;
> > }
> >
> > static int __dwc3_gadget_get_frame(struct dwc3 *dwc)
> > @@ -1377,7 +1377,7 @@ static int dwc3_gadget_start_isoc_quirk(struct dwc3_ep *dep)
> > dep->start_cmd_status = 0;
> > dep->combo_num = 0;
> >
> > - return __dwc3_gadget_kick_transfer(dep);
> > + return __dwc3_gadget_kick_transfer(dep, false);
> > }
> >
> > static int __dwc3_gadget_start_isoc(struct dwc3_ep *dep)
> > @@ -1402,9 +1402,23 @@ static int __dwc3_gadget_start_isoc(struct dwc3_ep *dep)
> > }
> >
> > for (i = 0; i < DWC3_ISOC_MAX_RETRIES; i++) {
> > - dep->frame_number = DWC3_ALIGN_FRAME(dep, i + 1);
> > + /*
> > + * last_frame_number is set from XferNotReady and may be
> > + * already out of date. DSTS only provides the lower 14 bit
> > + * of the current frame number. So add the upper two bits of
> > + * last_frame_number and handle a possible rollover.
> > + * This will provide the correct frame_number unless more than
> > + * rollover has happened since XferNotReady.
> > + */
> > + u32 frame = __dwc3_gadget_get_frame(dwc);
> > +
> > + dep->frame_number = (dep->last_frame_number & ~0x3fff) | frame;
> > + if (frame < (dep->last_frame_number & 0x3fff))
> > + dep->frame_number += 0x4000;
>
> Use BIT(14) rather than 0x4000? It's clearer in in my opinion. We

Ok.

> started using 0x3fff in multiple places now, can we create a macro for that?

Makes sense. Any preferences for the name? <something>_MASK I guess, but I
don't know what the correct name for the 14 bit frame number should be.

> Also, add an empty line here.

ok.

> > + dep->frame_number = DWC3_ALIGN_FRAME(dep, 1);
> >
> > - ret = __dwc3_gadget_kick_transfer(dep);
> > + ret = __dwc3_gadget_kick_transfer(dep,
> > + i + 1 < DWC3_ISOC_MAX_RETRIES);
> > if (ret != -EAGAIN)
> > break;
> > }
> > @@ -1461,7 +1475,7 @@ static int __dwc3_gadget_ep_queue(struct dwc3_ep *dep, struct dwc3_request *req)
> > }
> > }
> >
> > - return __dwc3_gadget_kick_transfer(dep);
> > + return __dwc3_gadget_kick_transfer(dep, false);
> > }
> >
> > static int dwc3_gadget_ep_queue(struct usb_ep *ep, struct usb_request *request,
> > @@ -2467,7 +2481,7 @@ static int dwc3_gadget_ep_cleanup_completed_request(struct dwc3_ep *dep,
> >
> > if (!dwc3_gadget_ep_request_completed(req) &&
> > req->num_pending_sgs) {
> > - __dwc3_gadget_kick_transfer(dep);
> > + __dwc3_gadget_kick_transfer(dep, false);
> > goto out;
> > }
> >
> > @@ -2497,6 +2511,7 @@ static void dwc3_gadget_endpoint_frame_from_event(struct dwc3_ep *dep,
> > const struct dwc3_event_depevt *event)
> > {
> > dep->frame_number = event->parameters;
> > + dep->last_frame_number = event->parameters;
> > }
> >
> > static void dwc3_gadget_endpoint_transfer_in_progress(struct dwc3_ep *dep,
>
> Other than the comments I provided, this patch looks fine to me.

Great. Thanks for the review.

Regards,
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 |