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

[Felipe Balbi <felipe.balbi@linux.intel.com>]

[-- Attachment #1: Type: text/plain, Size: 3291 bytes --]


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

> 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

>> - musb: no_interrupt only used for tracing
>> 
>> - atmel_usba_udc: no_interrupt only used for tracing
>> 
>> - mv_u3d_core: probably throttles interrupt, but probably exhibits same
>>   behavior. It's just that it hasn't been reported.
>> 
>> - fsl_udc_core: probably throttles interrupt, but probably exhibits same
>>   behavior. It's just that it hasn't been reported.
>
> The above two uses chipidea IP core too.

so why do we still have these drivers in tree? Might as well remove them
already.

-- 
balbi

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 800 bytes --]