Hi, On 9/11/2017 12:42 AM, Felipe Balbi wrote: > > Hi, > > Thinh Nguyen <Thinh.Nguyen@xxxxxxxxxxxx> writes: >>> Felipe Balbi <balbi@xxxxxxxxxx> writes: >>>> Thinh Nguyen <Thinh.Nguyen@xxxxxxxxxxxx> writes: >>>> >>>>> Hi Felipe, >>>>> >>>>> On 9/7/2017 12:16 AM, Felipe Balbi wrote: >>>>>>>>> drivers/usb/dwc3/gadget.c | 8 +++++--- >>>>>>>>> 1 file changed, 5 insertions(+), 3 deletions(-) >>>>>>>>> >>>>>>>>> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c >>>>>>>>> index 9e41605a..6b299c7 100644 >>>>>>>>> --- a/drivers/usb/dwc3/gadget.c >>>>>>>>> +++ b/drivers/usb/dwc3/gadget.c >>>>>>>>> @@ -191,14 +191,16 @@ void dwc3_gadget_giveback(struct dwc3_ep *dep, struct dwc3_request *req, >>>>>>>>> >>>>>>>>> req->started = false; >>>>>>>>> list_del(&req->list); >>>>>>>>> - req->trb = NULL; >>>>>>>>> req->remaining = 0; >>>>>>>>> >>>>>>>>> if (req->request.status == -EINPROGRESS) >>>>>>>>> req->request.status = status; >>>>>>>>> >>>>>>>>> - usb_gadget_unmap_request_by_dev(dwc->sysdev, >>>>>>>>> - &req->request, req->direction); >>>>>>>>> + if (req->trb) >>>>>>>> This check does not account for control data transfer. TRBs for ep0 are >>>>>>>> not set to its req->trb. ep0out request needs to be unmapped, otherwise >>>>>>>> device will receive bogus data. >>>>>>>> >>>>>>>> Our internal test showed that the device failed to interpret control >>>>>>>> data from host. I bisected to this patch. >>>>>> >>>>>> what was the testing? How can I reproduce it? >>>>> >>>>> This is a regression. The internal test found the issue when it does a >>>>> 3-stage Control Write Transfer. You can reproduce this issue with just 1 >>>>> out data packet of size > 0. Read and check the data on control request >>>>> completion. >>>> >>>> okay, is this enough to fix the problem for you? >>>> >>>> modified drivers/usb/dwc3/ep0.c >>>> @@ -48,6 +48,9 @@ static void dwc3_ep0_prepare_one_trb(struct dwc3_ep *dep, >>>> dwc = dep->dwc; >>>> trb = &dwc->ep0_trb[dep->trb_enqueue]; >>>> >>>> + if (!req->trb) >>>> + req->trb = trb; >>>> + >>>> if (chain) >>>> dep->trb_enqueue++; >>> >>> sorry, no. this is totally wrong :-) Here's a better version: >>> >>> modified drivers/usb/dwc3/ep0.c >>> @@ -990,6 +990,8 @@ static void __dwc3_ep0_do_control_data(struct dwc3 *dwc, >>> DWC3_TRBCTL_CONTROL_DATA, >>> true); >>> >>> + req->trb = &dwc->ep0_trb[dep->trb_enqueue - 1]; > + >>> /* Now prepare one extra TRB to align transfer size */ >>> dwc3_ep0_prepare_one_trb(dep, dwc->bounce_addr, >>> maxpacket - rem, >>> @@ -1015,6 +1017,8 @@ static void __dwc3_ep0_do_control_data(struct dwc3 *dwc, >>> DWC3_TRBCTL_CONTROL_DATA, >>> true); >>> >>> + req->trb = &dwc->ep0_trb[dep->trb_enqueue - 1]; > + >>> /* Now prepare one extra TRB to align transfer size */ >>> dwc3_ep0_prepare_one_trb(dep, dwc->bounce_addr, >>> 0, DWC3_TRBCTL_CONTROL_DATA, >>> @@ -1029,6 +1033,9 @@ static void __dwc3_ep0_do_control_data(struct dwc3 *dwc, >>> dwc3_ep0_prepare_one_trb(dep, req->request.dma, >>> req->request.length, DWC3_TRBCTL_CONTROL_DATA, >>> false); >>> + >>> + req->trb = &dwc->ep0_trb[dep->trb_enqueue]; >>> + >>> ret = dwc3_ep0_start_trans(dep); >>> } >>> >>> (didn't even compile test) >>> >>> >> >> Yes this works. > > Thank you, I'll send this as a formal patch after merge window closes. > The fix for this patch is now in mainline. Please also apply it to stable kernel 4.13.x. Otherwise this regression will remain for the kernel 4.13.x. Upstream: commit 551684708356 ("usb: dwc3: ep0: fix DMA starvation by assigning req->trb on ep0") Thanks, Thinh