From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga04.intel.com ([192.55.52.120]:50457 "EHLO mga04.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751223AbcKBH4L (ORCPT ); Wed, 2 Nov 2016 03:56:11 -0400 From: Felipe Balbi To: Peter Chen Cc: Linux USB , David Miller , Ville =?utf-8?B?U3lyasOkbMOk?= , stable@vger.kernel.org Subject: Re: [PATCH] usb: gadget: u_ether: remove interrupt throttling In-Reply-To: <20161102060202.GD28525@b29397-desktop> References: <20161101112959.19640-1-felipe.balbi@linux.intel.com> <20161102060202.GD28525@b29397-desktop> Date: Wed, 02 Nov 2016 09:55:44 +0200 Message-ID: <87vaw6nwn3.fsf@linux.intel.com> MIME-Version: 1.0 Content-Type: multipart/signed; boundary="=-=-="; micalg=pgp-sha256; protocol="application/pgp-signature" Sender: stable-owner@vger.kernel.org List-ID: --=-=-= Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable Hi, Peter Chen 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." >>=20 >> 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. >>=20 >> Cc: >> Reported-by: Ville Syrj=C3=A4l=C3=A4 >> Suggested-by: David Miller >> Signed-off-by: Felipe Balbi >> --- >> drivers/usb/gadget/function/u_ether.c | 8 -------- >> 1 file changed, 8 deletions(-) >>=20 >> 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 *s= kb, >>=20=20 >> req->length =3D length; >>=20=20 >> - /* throttle high/super speed IRQ rate back slightly */ >> - if (gadget_is_dualspeed(dev->gadget)) >> - req->no_interrupt =3D (((dev->gadget->speed =3D=3D USB_SPEED_HIGH || >> - dev->gadget->speed =3D=3D USB_SPEED_SUPER)) && >> - !list_empty(&dev->tx_reqs)) >> - ? ((atomic_read(&dev->tx_qlen) % dev->qmult) !=3D 0) >> - : 0; >> - >> retval =3D usb_ep_queue(in, req, GFP_ATOMIC); >> switch (retval) { >> default: >> --=20 > > Felipe, it may increase cpu utilization since more interrupts will be the= re, > 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: =2D chipidea: uses TD_IOC conditionally, but always sets TD_TERMINATE lastnode->ptr->next =3D cpu_to_le32(TD_TERMINATE); if (!hwreq->req.no_interrupt) lastnode->ptr->token |=3D 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. =2D musb: no_interrupt only used for tracing =2D atmel_usba_udc: no_interrupt only used for tracing =2D mv_u3d_core: probably throttles interrupt, but probably exhibits same behavior. It's just that it hasn't been reported. =2D 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=3Dlinux-usb&m=3D147793992922064&w=3D2 =2D-=20 balbi --=-=-= Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQIcBAEBCAAGBQJYGZwAAAoJEMy+uJnhGpkG09sP/0GFmpofwkQIZSE4FvtvDRax gVbY3HYUDq0X0uxwhKflu89fqD7IWOiGuGv0SI49/9NBG39pf1AcQRtVv2f58qiw 82MOlngLzmOxnS+enVQr/IR3AhTKU+r7RueKDgENCFjMa31jz+ZfGK8yHRtS69yo IfH9Gq+sTcON7FM+HoKJ9guQ1BqCwMKGyq+DuP25YWeNXyrt37ZolyOOFhRT+L55 QGhPE3rV0dttpRCec1YMeXEvgEmDU4UV5kvpgVY9EwikOWNBOifXZBUGHIJQPviE +oZJYg5z0C7oosw/zVXPjofX6SlJBPB1rPjkY5Z1dVnV6Oa8Sy324k0MWV4qmn4V hZwUvWGBwoW+EbMhHRQ+FmPF24ltmbAI+ZWwlIQYG+nKeHf78EGeXzMynCkzmfWp K3BEKJJxsrFnHtEt17e87QRIx9gFKtBynbzkyNcqjG6skrjaZVuHiv4o+lWEzaxZ s53hUBCtfhQ52vHIhmpynopI42oopIdzkF+/eWLg2s+T3BgtHoj7TWehIQ9dkdM4 Cdt79hqUGsqCTVYXTvpeM9dpS7d3UkkIVSjAF2ipFMHTTkvV+Oi9YD0KvL4VV1Mj liQZqc/nSFjc1WJdPi/MZNWmoWgyKFAmeZJcipC82Iscsha9WQlsxpfeaxdgqfNm 16SWW3vWB1ajYNYR49oL =1VRj -----END PGP SIGNATURE----- --=-=-=--