Re: [PATCH] usb: gadget: u_ether: remove interrupt throttling

[Peter Chen <hzpeterchen@gmail.com>]

On Wed, Nov 02, 2016 at 01:02:59PM +0200, Felipe Balbi wrote:
> 
> Hi,
> 
> Peter Chen <hzpeterchen@gmail.com> writes:
> >> >> diff --git a/drivers/usb/gadget/function/u_ether.c b/drivers/usb/gadget/function/u_ether.c
> >> >> index f4a640216913..119a2e5848e8 100644
> >> >> --- a/drivers/usb/gadget/function/u_ether.c
> >> >> +++ b/drivers/usb/gadget/function/u_ether.c
> >> >> @@ -589,14 +589,6 @@ static netdev_tx_t eth_start_xmit(struct sk_buff *skb,
> >> >>  
> >> >>  	req->length = length;
> >> >>  
> >> >> -	/* throttle high/super speed IRQ rate back slightly */
> >> >> -	if (gadget_is_dualspeed(dev->gadget))
> >> >> -		req->no_interrupt = (((dev->gadget->speed == USB_SPEED_HIGH ||
> >> >> -				       dev->gadget->speed == USB_SPEED_SUPER)) &&
> >> >> -					!list_empty(&dev->tx_reqs))
> >> >> -			? ((atomic_read(&dev->tx_qlen) % dev->qmult) != 0)
> >> >> -			: 0;
> >> >> -
> >> >>  	retval = usb_ep_queue(in, req, GFP_ATOMIC);
> >> >>  	switch (retval) {
> >> >>  	default:
> >> >> -- 
> >> >
> >> > Felipe, it may increase cpu utilization since more interrupts will be there,
> >> > it may affect the SoC which has lower cpu frequency. This code existed
> >> > many years, why this problem has only reported at dwc3 recently?
> >> 
> >> No idea, but at least for networking gadgets we shouldn't throttle. This
> >> has been a bug since the beginning. Read Dave Miller's explanation at
> >> [1]
> >> 
> >> moreover, dwc3 seems to be the only one actually throttling IRQ. Here's
> >> a rundown of a few of the UDCs:
> >> 
> >> - chipidea: uses TD_IOC conditionally, but always sets TD_TERMINATE
> >> 
> >> 	lastnode->ptr->next = cpu_to_le32(TD_TERMINATE);
> >> 	if (!hwreq->req.no_interrupt)
> >> 		lastnode->ptr->token |= cpu_to_le32(TD_IOC);
> >> 
> >> 	I'm guessing TD_TERMINATE works similar to dwc3's LST bit. If
> >> 	it's set, it will force an interrupt.
> >
> > No, TD_TERMINATE just stands for it is the last TD, and this pointer will
> > be updated when the new request is added. The interrupt is only triggered
> > by IOC (Interrupt On Complete) bit at TD.
> >
> > I am not sure if dwc3 supports ITC (Interrupt Threshold Control)
> > software control, it is an EHCI compliant register entry, and
> > the device mode is supported for chipidea too. It is a timeout
> > mechanism from controller side for pending requests.
> >
> > The interrupt will be triggered either the request has completed for TD
> > which IOC bit is set or the ITC is fired (125us currently) and the
> > request has completed, so the problem David described should not exist,
> > at least for chipidea.
> 
> In other words, you don't *really* throttle interrupt as they'll fire
> after the micro-frame expires :-p

No, even in one uFrame, there are at most ~10 packets for bulk at USB2.
At least, you can throttle interrupt within SoF, it is useful for 
high throughout use case.

> 
> > If DWC3 has similar ITC bits, would you try to tune it? The default ITC
> > value for chipidea is not enough, and we tuned it before.
> 
> there's no such thing in dwc3
> 

So, how about add another parameter to support throttling interrupt
separately. Current parameter 'mult' combined user request number
and throttle interrupt together.

-- 

Best Regards,
Peter Chen