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

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

 



On Tue, Oct 03, 2017 at 06:41:54PM +0000, Thinh Nguyen wrote:
> 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")

Now queued up, thanks.

greg k-h



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