From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752781AbbIOQPf (ORCPT ); Tue, 15 Sep 2015 12:15:35 -0400 Received: from mailout1.w1.samsung.com ([210.118.77.11]:57018 "EHLO mailout1.w1.samsung.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751622AbbIOQPd (ORCPT ); Tue, 15 Sep 2015 12:15:33 -0400 X-AuditID: cbfec7f5-f794b6d000001495-4b-55f84422b801 Subject: Re: [PATCH 04/26] usb: gadget: introduce 'enabled' flag in struct usb_ep To: balbi@ti.com 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> Cc: Robert Baldyga , gregkh@linuxfoundation.org, linux-usb@vger.kernel.org, linux-kernel@vger.kernel.org, b.zolnierkie@samsung.com, m.szyprowski@samsung.com, andrzej.p@samsung.com From: Krzysztof Opasiak Message-id: <55F8441D.5080301@samsung.com> Date: Tue, 15 Sep 2015 18:15:25 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.2.0 MIME-version: 1.0 In-reply-to: <20150915154324.GI19948@saruman.tx.rr.com> Content-type: text/plain; charset=windows-1252; format=flowed Content-transfer-encoding: 7bit X-Brightmail-Tracker: H4sIAAAAAAAAA+NgFrrNLMWRmVeSWpSXmKPExsVy+t/xa7pKLj9CDa6eVLKY9bKdxWLjjPWs Fgfv11s0L17PZnF51xw2i0XLWpkt1h65y27x4PBOdgcOj/1z17B79G1Zxehx/MZ2Jo/Pm+QC WKK4bFJSczLLUov07RK4Mhp/dLAXTJOsOLb1JVsDY59IFyMnh4SAicT5hnNMELaYxIV769lA bCGBpYwSqy/ndDFyAdnPGSXWf1jPCJIQFgiWODR7FQuILSIgILH+xSV2iKLTjBI33m1lBXGY Bc4xSix/uRIow8HBJqAvMW+XKEgDr4CWxOIT/8G2sQioSrS82wc2VFQgQuLU2bdsEDWCEj8m 3wNbwClgLrFv3XGwGmYBW4kF79exQNjyEpvXvGWewCgwC0nLLCRls5CULWBkXsUomlqaXFCc lJ5rpFecmFtcmpeul5yfu4kREuRfdzAuPWZ1iFGAg1GJhzei/XuoEGtiWXFl7iFGCQ5mJRFe VccfoUK8KYmVValF+fFFpTmpxYcYpTlYlMR5Z+56HyIkkJ5YkpqdmlqQWgSTZeLglAKGs/bO 9SoWOcasXQXldza2NWTH7wj9r1rB8O9EWNE77keHH89RnJ5xomndb5v04zHfN/Z4f/m2zGon l7icNcuZ/uuT8nmur1B42p17KXLhXflu/w1OIc8PbSj1/311jd6TB99npmhUpRgW/kuoPO0x s9Dp2p4XDW3yB5/c+dmhPt95b3N85Ds9JZbijERDLeai4kQAiik3JG4CAAA= Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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(struct usb_ep *ep, >>> */ >>> static inline int usb_ep_enable(struct usb_ep *ep) >>> { >>> - return ep->ops->enable(ep, ep->desc); >>> + int ret = 0; >>> + >>> + if (!ep->enabled) { >>> + ret = ep->ops->enable(ep, ep->desc); >>> + if (!ret) >>> + ep->enabled = true; >>> + } >>> + >>> + return ret; >>> } >>> >>> /** >>> @@ -281,7 +290,15 @@ static inline int usb_ep_enable(struct usb_ep *ep) >>> */ >>> static inline int usb_ep_disable(struct usb_ep *ep) >>> { >>> - return ep->ops->disable(ep); >>> + int ret = 0; >>> + >>> + if (ep->enabled) { >>> + ret = ep->ops->disable(ep); >>> + if (!ret) >>> + ep->enabled = 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 should >> have usb_ep_is_enabled()) and call disable only when it is really enabled. > > 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. > > 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 > Hmmm not sure right now, didn't test this recently :D as usually I check if light isn't already "on" before I touch the switch to turn it on:P Just joking. Personally I just prefer to don't touch things which are already in desired condition. Let's take close() as example which could be a little bit equivalent of our usb_ep_disable(). It is not legal to call it twice on some fd and second call ends up with error. Best regards, -- Krzysztof Opasiak Samsung R&D Institute Poland Samsung Electronics