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 <r.baldyga@xxxxxxxxxxx> > >>> --- > >>> 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. 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 = ep->ops->enable(ep, ep->desc); if (ret) return ret; ep->enabled = true; return 0; } -- balbi
Attachment:
signature.asc
Description: Digital signature