From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754107AbbIOQBk (ORCPT ); Tue, 15 Sep 2015 12:01:40 -0400 Received: from arroyo.ext.ti.com ([192.94.94.40]:42976 "EHLO arroyo.ext.ti.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751816AbbIOQBi (ORCPT ); Tue, 15 Sep 2015 12:01:38 -0400 Date: Tue, 15 Sep 2015 11:01:31 -0500 From: Felipe Balbi To: Robert Baldyga CC: , Krzysztof Opasiak , , , , , , Subject: Re: [PATCH 04/26] usb: gadget: introduce 'enabled' flag in struct usb_ep Message-ID: <20150915160131.GJ19948@saruman.tx.rr.com> Reply-To: References: <1442327229-28320-1-git-send-email-r.baldyga@samsung.com> <1442327229-28320-5-git-send-email-r.baldyga@samsung.com> <55F83B37.20408@samsung.com> <20150915154324.GI19948@saruman.tx.rr.com> <55F84001.3060902@samsung.com> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="pN9MePJoZbRKbUk1" Content-Disposition: inline In-Reply-To: <55F84001.3060902@samsung.com> User-Agent: Mutt/1.5.23 (2014-03-12) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org --pN9MePJoZbRKbUk1 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable Hi, On Tue, Sep 15, 2015 at 05:57:53PM +0200, Robert Baldyga wrote: > On 09/15/2015 05:43 PM, Felipe Balbi wrote: > > On Tue, Sep 15, 2015 at 05:37:27PM +0200, Krzysztof Opasiak wrote: > >> Hello, > >> > >> On 09/15/2015 04:26 PM, Robert Baldyga wrote: > >>> This patch introduces 'enabled' flag in struct usb_ep, and modifies > >>> usb_ep_enable() and usb_ep_disable() functions to encapsulate endpoint > >>> enabled/disabled state. It helps to avoid enabling endpoints which are > >>> already enabled, and disabling endpoints which are already disables. > >>> > >>> >From now USB functions don't have to remember current endpoint > >>> enable/disable state, as this state is now handled automatically which > >>> makes this API less bug-prone. > >>> > >>> Signed-off-by: Robert Baldyga > >>> --- > >>> include/linux/usb/gadget.h | 21 +++++++++++++++++++-- > >>> 1 file changed, 19 insertions(+), 2 deletions(-) > >>> > >>> diff --git a/include/linux/usb/gadget.h b/include/linux/usb/gadget.h > >>> index 3f299e2..63375cd 100644 > >>> --- a/include/linux/usb/gadget.h > >>> +++ b/include/linux/usb/gadget.h > >>> @@ -215,6 +215,7 @@ struct usb_ep { > >>> struct list_head ep_list; > >>> struct usb_ep_caps caps; > >>> bool claimed; > >>> + bool enabled; > >>> unsigned maxpacket:16; > >>> unsigned maxpacket_limit:16; > >>> unsigned max_streams:16; > >>> @@ -264,7 +265,15 @@ static inline void usb_ep_set_maxpacket_limit(st= ruct usb_ep *ep, > >>> */ > >>> static inline int usb_ep_enable(struct usb_ep *ep) > >>> { > >>> - return ep->ops->enable(ep, ep->desc); > >>> + int ret =3D 0; > >>> + > >>> + if (!ep->enabled) { > >>> + ret =3D ep->ops->enable(ep, ep->desc); > >>> + if (!ret) > >>> + ep->enabled =3D true; > >>> + } > >>> + > >>> + return ret; > >>> } > >>> > >>> /** > >>> @@ -281,7 +290,15 @@ static inline int usb_ep_enable(struct usb_ep *e= p) > >>> */ > >>> static inline int usb_ep_disable(struct usb_ep *ep) > >>> { > >>> - return ep->ops->disable(ep); > >>> + int ret =3D 0; > >>> + > >>> + if (ep->enabled) { > >>> + ret =3D ep->ops->disable(ep); > >>> + if (!ret) > >>> + ep->enabled =3D false; > >>> + } > >>> + > >>> + return ret; > >>> } > >>> > >> > >> Personally I don't like this convention. In my opinion usb_ep_disable(= ) & > >> usb_ep_enable() should fail if ep is already disabled/enabled. Then in > >> function code we should check if endpoint is enabled (maybe even we sh= ould > >> have usb_ep_is_enabled()) and call disable only when it is really enab= led. > >=20 > > usb_ep_is_enabled() should be a good addition but I don't see an issue > > ignoring usb_ep_enabled() for something that's already enabled. > >=20 > > Imagine if you got an error when you tried to push the light switch to > > the 'on' position while the light was already on :-p > >=20 > > I do think, though, that this can be simplified by returning early if > > already enabled: > >=20 > > usb_ep_enable() > > { > > if (ep->enabled) > > return 0; > >=20 > > return ep->ops->enable(ep, ep->desc); > > } > >=20 > > and likewise for usb_ep_disable() >=20 > We can't do that, because we need to toggle ep->enable flag. man, things have to be spelled out to the last comma... The point was to avoid the extra identation level usb_ep_enable() { int ret; if (ep->enabled) return 0; ret =3D ep->ops->enable(ep, ep->desc); if (ret) return ret; ep->enabled =3D true; return 0; } --=20 balbi --pN9MePJoZbRKbUk1 Content-Type: application/pgp-signature; name="signature.asc" Content-Description: Digital signature -----BEGIN PGP SIGNATURE----- Version: GnuPG v1 iQIcBAEBAgAGBQJV+EDbAAoJEIaOsuA1yqREk4kP/RlxZBjQctD+FXss4UHO63kj NNslhDb1WD2CJEfevruXu4ww5c45/ECWsA0JVV032SyIRzR1hmYK8t7NNgoKeIyZ p/I2hLUXpJFcW2119PwmEr0PtCeI01AtTtdqQMIvk1XKrZjMHHmVdZ0bBvecXQK2 Izv2nbzvtfRVv8SOOJLNz9R3vQPBeLAj5hY5aO9sqU0OvDAz229JQLBPCXnH07Ch X/5QFTpEhB/ENJ9+V5WC4xB+bACa4W2R5yL9EMjgCJanCDZ8VF8FkHfFfHDsojyZ lZHPUT0/4pMOU74Mas5ETcfH1zOovtc1V3pHgv/GNmnF850N29mk0ct0OsCX1t9l asGUG6GPG1EVedPBBPBw0Cf0Z1OPZVhn4021KDBbasrWkYED22/7zSPT7rebNdhT e2gH+ybQG9gt6280HxKeA1EWY2uWf99djkzqNfXQLs7b/6cLB4m2vG0YbojitxpH RiU50zkmxDfumPWUuZ+zoKUtgalPTTVChwhqxf6LeaIsa7iXhibrde/2nIhuTj8Y dB60iQl5j71vuZfWzOwe5ZN0Jwd13A4HECxjVMOaEnpO69X1oikrT9f8ztRIzbxB K43ui6X7Qe5j+owpOPALMwdTG41+SjQHWtGaGenKSRuCQVboOzTghbaPVGnfzbLX fpaTZ5zLnBS7Drnsg1m6 =u1Aj -----END PGP SIGNATURE----- --pN9MePJoZbRKbUk1--