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
--
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
