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. -- balbi
Attachment:
signature.asc
Description: PGP signature
