From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754468AbbLJR0W (ORCPT ); Thu, 10 Dec 2015 12:26:22 -0500 Received: from devils.ext.ti.com ([198.47.26.153]:34803 "EHLO devils.ext.ti.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751512AbbLJR0V (ORCPT ); Thu, 10 Dec 2015 12:26:21 -0500 From: Felipe Balbi To: , CC: , , , "Du, Changbin" Subject: Re: [PATCH 1/2] usb: dwc2: add ep enabled flag to avoid double enable/disable In-Reply-To: <1448860888-9841-2-git-send-email-changbin.du@intel.com> References: <1448860888-9841-1-git-send-email-changbin.du@intel.com> <1448860888-9841-2-git-send-email-changbin.du@intel.com> User-Agent: Notmuch/0.21 (http://notmuchmail.org) Emacs/24.5.1 (x86_64-pc-linux-gnu) Date: Thu, 10 Dec 2015 11:26:15 -0600 Message-ID: <87zixii5ag.fsf@saruman.tx.rr.com> MIME-Version: 1.0 Content-Type: multipart/signed; boundary="=-=-="; micalg=pgp-sha1; protocol="application/pgp-signature" Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org --=-=-= Content-Type: text/plain Content-Transfer-Encoding: quoted-printable Hi, changbin.du@intel.com writes: > From: "Du, Changbin" > > Enabling a already enabled ep is illegal, because the ep may has trbs > running. Reprogram the ep may break running transfer. So udc driver > must avoid this happening by return an error -EBUSY. Gadget function > driver also should avoid such things, but that is out of udc driver. > > Similarly, disable a disabled ep makes no sense, but no need return > an error here. > > Signed-off-by: Du, Changbin > --- > drivers/usb/dwc2/core.h | 1 + > drivers/usb/dwc2/gadget.c | 20 +++++++++++++++++++- > 2 files changed, 20 insertions(+), 1 deletion(-) > > diff --git a/drivers/usb/dwc2/core.h b/drivers/usb/dwc2/core.h > index a66d3cb..cf7eccd 100644 > --- a/drivers/usb/dwc2/core.h > +++ b/drivers/usb/dwc2/core.h > @@ -162,6 +162,7 @@ struct dwc2_hsotg_ep { > unsigned char mc; > unsigned char interval; >=20=20 > + unsigned int enabled:1; > unsigned int halted:1; > unsigned int periodic:1; > unsigned int isochronous:1; > diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c > index 0abf73c..586bbcd 100644 > --- a/drivers/usb/dwc2/gadget.c > +++ b/drivers/usb/dwc2/gadget.c > @@ -2423,6 +2423,7 @@ void dwc2_hsotg_core_init_disconnected(struct dwc2_= hsotg *hsotg, > /* enable, but don't activate EP0in */ > dwc2_writel(dwc2_hsotg_ep0_mps(hsotg->eps_out[0]->ep.maxpacket) | > DXEPCTL_USBACTEP, hsotg->regs + DIEPCTL0); > + hsotg->eps_out[0]->enabled =3D 1; >=20=20 > dwc2_hsotg_enqueue_setup(hsotg); >=20=20 > @@ -2680,6 +2681,14 @@ static int dwc2_hsotg_ep_enable(struct usb_ep *ep, > return -EINVAL; > } >=20=20 > + spin_lock_irqsave(&hsotg->lock, flags); > + if (hs_ep->enabled) { > + dev_warn(hsotg->dev, "%s: ep %s already enabled\n", > + __func__, hs_ep->name); this is a rather serious condition. I'd rather use dev_WARN_ONCE(): if (dev_WARN_ONCE(hsotg->dev, hs_ep->enabled, "ep %s already enabled\n", hs_ep->name)) { =2D-=20 balbi --=-=-= Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v1 iQIcBAEBAgAGBQJWabW3AAoJEIaOsuA1yqRExMAP/0HiBx7saBm078J8+FMiNkru /DNBaa+ZAZXOPt+G8P4Yb9SxzJzlTNRzECg9RpLeiviw6S4+Z/YZqMveX9y9ZJRv bMmwSK9qlNd6mG+ckJpHtR9a3g4jhqR3I/rM28PpHBcUfwPj8G0mtFqtKEL/YKKL /bSLGLuzwMv6HXfCfr8jG3+1Y40vHvAhxoQwBNzlTcczTzPGbK2LeoHIaXO6DPK6 RmsdQeaOVXAt/kIgpHEP0SqS1tr7SBejGBK2H7aQjDD1NLQW5W/9LeRvyaNeTtbN 7+ZtqvPG3ExFXhlHtg9WIfs/Qehv58CXASmaeV8B5arTwiNlxa13yJrIVTYeCoRQ OEgc8SqnI2hTD3BDMXNzit4zC6zd+v5rknWafuz+sXCRvJUJoic6YBUE5KA+fnXl 82w1LIsJ8u0xxZr7s3JrjHKxn54I63E8EEMLxx7R8FK3H0gcCIajyuTFfhor2gcH nuhPRlAoy/MJWuRzfseYGaWJyo+vzIfRvB0sM3z3mDCsvIjctBbU6jyByOiOzUke 7toISo5GPaJA/IuYseYw9WXNWR/LN4qyjBWA30r1nS1boEZWpVqtjTCgN9aAoJnu kxwPQsohKsCSsZvgnJh/RZXJjW6lRaZgx62lC5NvM7Mf+gGKpae2TnPTvzljHWSg cRF1YWRHsU/AhXRqMqsC =SlAM -----END PGP SIGNATURE----- --=-=-=--