On Tue, Oct 25, 2022, Thinh Nguyen wrote:
> On Tue, Oct 25, 2022, Jeff Vanhoof wrote:
> > Hi Thinh,
> >
> > On Mon, Oct 24, 2022 at 11:51:50PM -0500, Jeff Vanhoof wrote:
> > > On Mon, Oct 24, 2022 at 06:28:04PM -0700, Thinh Nguyen wrote:
> > > > The gadget driver may have a certain expectation of how the request
> > > > completion flow should be from to its configuration. Make sure the
> > > > controller driver respect that. That is, don't set IMI (Interrupt on
> > > > Missed Isoc) when usb_request->no_interrupt is set.
> > > >
> > > > Fixes: 72246da40f37 ("usb: Introduce DesignWare USB3 DRD Driver")
> > > > Cc: stable@xxxxxxxxxxxxxxx
> > > > Signed-off-by: Thinh Nguyen <Thinh.Nguyen@xxxxxxxxxxxx>
> > > > ---
> > > > Changes in v2:
> > > > - None
> > > >
> > > > drivers/usb/dwc3/gadget.c | 4 ++--
> > > > 1 file changed, 2 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
> > > > index 230b3c660054..702bdf42ad2f 100644
> > > > --- a/drivers/usb/dwc3/gadget.c
> > > > +++ b/drivers/usb/dwc3/gadget.c
> > > > @@ -1292,8 +1292,8 @@ static void dwc3_prepare_one_trb(struct dwc3_ep *dep,
> > > > trb->ctrl = DWC3_TRBCTL_ISOCHRONOUS;
> > > > }
> > > >
> > > > - /* always enable Interrupt on Missed ISOC */
> > > > - trb->ctrl |= DWC3_TRB_CTRL_ISP_IMI;
> > > > + if (!req->request.no_interrupt)
> > > > + trb->ctrl |= DWC3_TRB_CTRL_ISP_IMI;
> > > > break;
> > > >
> > > > case USB_ENDPOINT_XFER_BULK:
> > > > --
> > > > 2.28.0
> > > >
> > >
> > <snip>
> >
> > For scatter gather, shouldn't the IMI bit be set only for the TRB associated
> > to the last item in the sg list? Do we need to do something similar to what
> > that was done for IOC in this area?
> >
> > For ex.:
> > + if ((!req->request.no_interrupt && !chain) || must_interrupt)
> > + trb->ctrl |= DWC3_TRB_CTRL_ISP_IMI;
> >
> > BTW, Dan indicated that this seems to help resolve the crash mentioned in
> > [PATCH v2 1/2] of this chain.
> >
>
> Actually, that's correct. Just ignore the "must_interrupt" setting. The
> programming guide actually noted to set the IMI for the last TRB of the
> chain also.
>
> I'll send a fix to this.
>
Hm... I wonder if it's still better to ignore no_interrupt and
immediately notify the gadget driver of missed isoc. It's redundant to
have both IOC and IMI set under the same condition, but I guess it
doesn't hurt to set it for clarity.
So it's either
if (!no_interrupt && !chain)
trb->ctrl |= DWC3_TRB_CTRL_ISP_IMI;
or
if (!chain)
trb->ctrl |= DWC3_TRB_CTRL_ISP_IMI;
Either way, this needs a fix.
Thanks,
Thinh
