From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754164AbbIOP6A (ORCPT ); Tue, 15 Sep 2015 11:58:00 -0400 Received: from mailout1.w1.samsung.com ([210.118.77.11]:56398 "EHLO mailout1.w1.samsung.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751797AbbIOP55 (ORCPT ); Tue, 15 Sep 2015 11:57:57 -0400 X-AuditID: cbfec7f4-f79c56d0000012ee-f0-55f8400292b0 Message-id: <55F84001.3060902@samsung.com> Date: Tue, 15 Sep 2015 17:57:53 +0200 From: Robert Baldyga User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.8.0 MIME-version: 1.0 To: balbi@ti.com, Krzysztof Opasiak Cc: 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 Subject: Re: [PATCH 04/26] usb: gadget: introduce 'enabled' flag in struct usb_ep 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> In-reply-to: <20150915154324.GI19948@saruman.tx.rr.com> Content-type: text/plain; charset=windows-1252 Content-transfer-encoding: 7bit X-Brightmail-Tracker: H4sIAAAAAAAAA+NgFrrNLMWRmVeSWpSXmKPExsVy+t/xy7pMDj9CDbp+81vMetnOYrFxxnpW i4P36y2aF69ns7g9cRqbxeVdc9gsFi1rZbZYe+QuuwOHx/65a9g9+rasYvQ4fmM7k8fnTXIB LFFcNimpOZllqUX6dglcGfMenWEpOC5e8e3WVdYGxrdCXYycHBICJhLb/7SxQNhiEhfurWfr YuTiEBJYyihx4dp6ZgjnGaPEtoZ/zCBVvAJaEhOWH2AFsVkEVCWePD/MCGKzCehIbPk+AcwW FYiQWL76JCNEvaDEj8n3wDaICFhLPF/whAlkKLPAMkaJJxMWs4MkhAWCJQ7NXsUCse00o8SN d1vBNnAKmEvsW3ccaBIHUIeexP2LWiBhZgF5ic1r3jJPYBSYhWTHLISqWUiqFjAyr2IUTS1N LihOSs811CtOzC0uzUvXS87P3cQICfIvOxgXH7M6xCjAwajEwxvR/j1UiDWxrLgy9xCjBAez kghvo86PUCHelMTKqtSi/Pii0pzU4kOM0hwsSuK8c3e9DxESSE8sSc1OTS1ILYLJMnFwSjUw hsyQ7f/staJp+4vu51uehaXseO2q4//Gl8FmdcKS/wqf5Du/zL9TvaGj1HGD55EdH04z7pVU /L428IuMTgF7ZsQXqanT5ps96e55PF9w3ZaHBQqa6jeFlC7dtor5XbKmdePzIPu5p/YyvMhy MeIvjQtev/TOgyPHqximHfvVf+xqwhaDn+3skkosxRmJhlrMRcWJAP0iB7puAgAA 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 > > I do think, though, that this can be simplified by returning early if > already enabled: > > usb_ep_enable() > { > if (ep->enabled) > return 0; > > return ep->ops->enable(ep, ep->desc); > } > > and likewise for usb_ep_disable() We can't do that, because we need to toggle ep->enable flag. Thanks, Robert