Hi, On 9/8/2017 2:34 AM, Felipe Balbi wrote: > 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. Thanks, Thinh -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
