From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga02.intel.com ([134.134.136.20]:14307 "EHLO mga02.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753941AbcKCIhO (ORCPT ); Thu, 3 Nov 2016 04:37:14 -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: <20161103003226.GB894@b29397-desktop> References: <20161101112959.19640-1-felipe.balbi@linux.intel.com> <20161102060202.GD28525@b29397-desktop> <87vaw6nwn3.fsf@linux.intel.com> <20161102083632.GA22181@b29397-desktop> <87lgx2nnz0.fsf@linux.intel.com> <20161103003226.GB894@b29397-desktop> Date: Thu, 03 Nov 2016 10:36:49 +0200 Message-ID: <87d1idnen2.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 Content-Transfer-Encoding: quoted-printable Hi, Peter Chen writes: >> Peter Chen writes: >> >> >> diff --git a/drivers/usb/gadget/function/u_ether.c b/drivers/usb/g= adget/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_b= uff *skb, >> >> >>=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_HI= GH || >> >> >> - 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 there, >> >> > it may affect the SoC which has lower cpu frequency. This code exis= ted >> >> > many years, why this problem has only reported at dwc3 recently? >> >>=20 >> >> No idea, but at least for networking gadgets we shouldn't throttle. T= his >> >> has been a bug since the beginning. Read Dave Miller's explanation at >> >> [1] >> >>=20 >> >> moreover, dwc3 seems to be the only one actually throttling IRQ. Here= 's >> >> a rundown of a few of the UDCs: >> >>=20 >> >> - chipidea: uses TD_IOC conditionally, but always sets TD_TERMINATE >> >>=20 >> >> lastnode->ptr->next =3D cpu_to_le32(TD_TERMINATE); >> >> if (!hwreq->req.no_interrupt) >> >> lastnode->ptr->token |=3D cpu_to_le32(TD_IOC); >> >>=20 >> >> 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 w= ill >> > be updated when the new request is added. The interrupt is only trigge= red >> > 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. >>=20 >> 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. 13 > At least, you can throttle interrupt within SoF, it is useful for=20 > high throughout use case. And that's still a bug for Networking drivers, that's what Dave Miller is saying. >> > 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. >>=20 >> there's no such thing in dwc3 >>=20 > > So, how about add another parameter to support throttling interrupt > separately. Current parameter 'mult' combined user request number > and throttle interrupt together. sorry, but no. =2D-=20 balbi --=-=-= Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQIcBAEBCAAGBQJYGvchAAoJEMy+uJnhGpkGiOkP/2h2a71Iim6nGVZSdIHufZoK OAJ1Gxc2bCcgeOcuNscqJ9J8ES2+YHK5R6NTT5RtJDzchQdAImm7UQUg9avYYM8z baeZhoYgMnDEYNG62cQVAelZb23DNOOsjWsTMpdHKj9/AqdRDPqz5NKBTjHJ9oyS VaiMiHvdniB9lFc+COZU9YsTCIO8LdgnHWprdn3Dx1FpPTY+cP3Q7VLyYgbtSVql 4RpvG2rG4jVJORQsreNDn5kz84ZDHMlBRaxpyygAOI7+/UbFFw+w43uepGkr+E8Y F1LcHkv4vzAOggmR16RNuyEzuQNvtcrgfOS4NW/Z+WRtO6wXixLbvFvPHcHWqVxU x9K4/D+z+tG05mpqD/nDQ6WEIH/Zvp/li1IiUvEvC4kqCX/Ek6COatonXCGFwTmJ gL9SfLsiPaLqn2V03k/qr0DiRCeYIwHW1/NB71xyv8MevWfpksUT4JJJk6RjmfrM 9i2BZGpzLCgUX/G0uTI0d+ur3erKV6IEV0tWhTTECZGO4LseGMoBCXow8xElydEE oQQ2e9exoWhDCP5zhCRdXaccFmV8jt72yi8YVwOO6y3lU8FCGXXQ3GxJIQKUxgUz wAzpNODgw0Xndw/dWBZdaGkjVwe+5YNZL6Exrqw3b1wARko9BpwN3y5w2B7lRfqO YubsQZTDvAnpHZsxJ++y =lHuS -----END PGP SIGNATURE----- --=-=-=--