Re: [PATCH] usb: dwc3: gadget: only unmap requests from DMA if mapped

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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



[Index of Archives]     [Linux Kernel]     [Kernel Development Newbies]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite Hiking]     [Linux Kernel]     [Linux SCSI]