Messages in this thread | | | Subject | Re: [PATCH 04/26] usb: gadget: introduce 'enabled' flag in struct usb_ep | From | Krzysztof Opasiak <> | Date | Tue, 15 Sep 2015 18:15:25 +0200 |
| |
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@samsung.com> >>> --- >>> 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
| |