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

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

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


Hi,

Peter Chen <hzpeterchen@gmail.com> writes:
> On Tue, Nov 01, 2016 at 01:29:59PM +0200, Felipe Balbi wrote:
>> According to Dave Miller "the networking stack has a
>> hard requirement that all SKBs which are transmitted
>> must have their completion signalled in a fininte
>> amount of time. This is because, until the SKB is
>> freed by the driver, it holds onto socket,
>> netfilter, and other subsystem resources."
>> 
>> In summary, this means that using TX IRQ throttling
>> for the networking gadgets is, at least, complex and
>> we should avoid it for the time being.
>> 
>> Cc: <stable@vger.kernel.org>
>> Reported-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
>> Suggested-by: David Miller <davem@davemloft.net>
>> Signed-off-by: Felipe Balbi <felipe.balbi@linux.intel.com>
>> ---
>>  drivers/usb/gadget/function/u_ether.c | 8 --------
>>  1 file changed, 8 deletions(-)
>> 
>> 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.

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

and there are the only uses for the no_interrupt flag.

[1] https://marc.info/?l=linux-usb&m=147793992922064&w=2

-- 
balbi

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