Re: [PATCH v2 2/2] usb: dwc3: gadget: Don't set IMI for no_interrupt

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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




[Index of Archives]     [Linux Kernel]     [Kernel Development Newbies]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite Hiking]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux