RE: [PATCH V2] usb: dwc3: gadget: trb_dequeue is not updated properly

[Felipe Balbi]

"Yang, Fei" <fei.yang@xxxxxxxxx> writes:

Hi,

>> Can only be true for last TRB
>>
> | 	if (event->status & DEPEVT_STATUS_IOC)
> | 		return 1;
>
> This is the problem. The whole USB request gets only one interrupt
> when the last TRB completes, so dwc3_gadget_ep_reclaim_trb_sg() gets
> called with event->status = 0x6 which has DEPEVT_STATUS_IOC bit
> set. Thus dwc3_gadget_ep_reclaim_completed_trb() returns 1 for the
> first TRB and the for-loop ends without having a chance to iterate
> through the sg list.

IOC is only set for the last TRB, so this will iterate over and over
again until it reaches the last TRB. Please collect tracepoints of the
failure case.

>> If we have a short packet, then we may fall here. Is that the case?
>
> No need for a short packet to make it fail. In my case below, a 16384
> byte request got slipt into 4 TRBs of 4096 bytes. All TRBs were
> completed normally, but the for-loop in
> dwc3_gadget_ep_reclaim_trb_sg() was terminated right after handling
> the first TRB. After that the trb_dequeue is messed up.

I need tracepoints to se what's going on, please collect tracepoints.

> buffer_addr,size,type,ioc,isp_imi,csp,chn,lst,hwo
> 0000000077849000, 4096,normal,0,0,1,1,0,0
> 000000007784a000, 4096,normal,0,0,1,1,0,0
> 000000007784b000, 4096,normal,0,0,1,1,0,0
> 000000007784c000, 4096,normal,1,0,1,0,0,0
> 000000007784d000, 512,normal,1,0,1,0,0,0
>
> My first version of the patch was trying to address the issue in
> dwc3_gadget_ep_reclaim_completed_trb(), but then I thought it's a bad
> idea to touch this function because that is also called from non
> scatter_gather list case, and I was not sure if returning 1 for the
> linear case is correct or not.

That function *must* be called for all cases. We want to reduce the
amount of special cases so code is more straight forward and easier to
maintain. Again, please collect tracepoints of the failure case with the
latest tag from Linus, otherwise you won't be able to convince me we
need your patch.

I also think your version is the wrong way to sort it out.

-- 
balbi

Attachment: signature.asc
Description: PGP signature