Re: [PATCH] usb: dwc3: gadget: check link trb after free_slot is increased

[Felipe Balbi <balbi@ti.com>]

[-- Attachment #1: Type: text/plain, Size: 3059 bytes --]

Hi,

On Fri, May 16, 2014 at 11:50:13PM +0800, Zhuang Jin Can wrote:
> > On Fri, May 16, 2014 at 05:57:57AM +0800, Zhuang Jin Can wrote:
> > > In ISOC transfers, when free_slot points to the last TRB (i.e. Link
> > > TRB), and all queued requests meet Missed Interval Isoc error, busy_slot
> > > points to trb0.
> > > 	busy_slot->trb0
> > > 		   trb1
> > > 		   ...
> > > 	free_slot->trb31(Link TRB)
> > > 
> > > After end transfer and receiving the XferNotReady event, trb_left is
> > > caculated as 1 which is wrong, and no TRB will be primed to the
> > > endpoint.
> > > 
> > > The root cause is free_slot is not increased the same way as busy_slot.
> > > When busy_slot is increased by one, it checks if points to a link TRB
> > > after increasement, but free_slot checks it before increasement.
> > > free_slot should behave the same as busy_slot to make the trb_left
> > > caculation correct.
> > > 
> > > Signed-off-by: Zhuang Jin Can <jin.can.zhuang@intel.com>
> > > Signed-off-by: Jiebing Li <jiebing.li@intel.com>
> > > ---
> > >  drivers/usb/dwc3/gadget.c |    8 ++++----
> > >  1 file changed, 4 insertions(+), 4 deletions(-)
> > > 
> > > diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
> > > index 54da8c8..2ebe82b 100644
> > > --- a/drivers/usb/dwc3/gadget.c
> > > +++ b/drivers/usb/dwc3/gadget.c
> > > @@ -828,10 +828,6 @@ static void dwc3_prepare_one_trb(struct dwc3_ep *dep,
> > >  			length, last ? " last" : "",
> > >  			chain ? " chain" : "");
> > >  
> > > -	/* Skip the LINK-TRB on ISOC */
> > > -	if (((dep->free_slot & DWC3_TRB_MASK) == DWC3_TRB_NUM - 1) &&
> > > -			usb_endpoint_xfer_isoc(dep->endpoint.desc))
> > > -		dep->free_slot++;
> > >  
> > >  	trb = &dep->trb_pool[dep->free_slot & DWC3_TRB_MASK];
> > 
> > I have a feeling this has a negative side effect of letting us use the
> > link TRB for data transfer... I mean, if we don't increment free_slot
> > before accessing our trb_pool, we have no way to skip link trb on this
> > access here.
> After every free_slot++ Link TRB is checked and increased if appropriate,
> this guarantees you next time access free_slot, it can't be a Link
> TRB.

right, next access will be fine, but you're forgetting about current
access.

> > How did you find the bug ? do you have good instructions on how to
> > reproduce it ? How did you test the patch and for how long ?
> The bug is reproduced on Android with f_audio_source.c enabled, which
> has an isoc-in endpoint keeps sending audio data to host in an interval
> of 1 ms. Normally, you need to run for 12+ hours to hit the issue.
> So I think you can just run some isoc transfers for a long time to
> reproduce it. To accelarte the reproducing, you can run some concurrent
> data transfer as well, so the possibility to meet missed interval error
> is larger.
> 
> The patch is tested for basic functionality like enumeration, data
> transfers. For this bug, it was tested for 20+ hours.

thanks, g_audio loop should be fine.

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]