lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
Open Source and information security mailing list archives
| ||
|
Date: Tue, 15 Sep 2015 18:15:25 +0200 From: Krzysztof Opasiak <k.opasiak@...sung.com> To: balbi@...com Cc: Robert Baldyga <r.baldyga@...sung.com>, gregkh@...uxfoundation.org, linux-usb@...r.kernel.org, linux-kernel@...r.kernel.org, b.zolnierkie@...sung.com, m.szyprowski@...sung.com, andrzej.p@...sung.com Subject: Re: [PATCH 04/26] usb: gadget: introduce 'enabled' flag in struct usb_ep 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@...sung.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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@...r.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists