Re: [PATCH 04/26] usb: gadget: introduce 'enabled' flag in struct usb_ep

[Robert Baldyga]

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_headep_list;
>>> struct usb_ep_caps  caps;
>>> boolclaimed;
>>> +   boolenabled;
>>> unsignedmaxpacket:16;
>>> unsignedmaxpacket_limit:16;
>>> unsignedmax_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
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 04/26] usb: gadget: introduce 'enabled' flag in struct usb_ep

[Felipe Balbi]

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 
> >>> ---
> >>>  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_headep_list;
> >>>   struct usb_ep_caps  caps;
> >>>   boolclaimed;
> >>> + boolenabled;
> >>>   unsignedmaxpacket:16;
> >>>   unsignedmaxpacket_limit:16;
> >>>   unsignedmax_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


signature.asc
Description: Digital signature

Re: [PATCH 04/26] usb: gadget: introduce 'enabled' flag in struct usb_ep

[Felipe Balbi]

Hi,

On Tue, Sep 15, 2015 at 06:15:25PM +0200, Krzysztof Opasiak wrote:
> >>>+  }
> >>>+
> >>>+  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

not my best analogy :)

> 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.

I understand that, it's just the difference between adding
usb_ep_is_enabled() to every single gadget/function driver or adding it
to usb_ep_enable() itself to keep gadget/function drivers cleaner.

-- 
balbi


signature.asc
Description: Digital signature

Re: [PATCH 04/26] usb: gadget: introduce 'enabled' flag in struct usb_ep

[Krzysztof Opasiak]

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_headep_list;
struct usb_ep_caps  caps;
boolclaimed;
+   boolenabled;
unsignedmaxpacket:16;
unsignedmaxpacket_limit:16;
unsignedmax_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 Institute Poland
Samsung Electronics
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 04/26] usb: gadget: introduce 'enabled' flag in struct usb_ep

[Krzysztof Opasiak]

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_headep_list;
struct usb_ep_caps  caps;
boolclaimed;
+   boolenabled;
unsignedmaxpacket:16;
unsignedmaxpacket_limit:16;
unsignedmax_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.


Best Regards,
--
Krzysztof Opasiak
Samsung R Institute Poland
Samsung Electronics
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 04/26] usb: gadget: introduce 'enabled' flag in struct usb_ep

[Felipe Balbi]

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_headep_list;
> > struct usb_ep_caps  caps;
> > boolclaimed;
> >+boolenabled;
> > unsignedmaxpacket:16;
> > unsignedmaxpacket_limit:16;
> > unsignedmax_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()

-- 
balbi


signature.asc
Description: Digital signature

[PATCH 04/26] usb: gadget: introduce 'enabled' flag in struct usb_ep

[Robert Baldyga]

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_headep_list;
struct usb_ep_caps  caps;
boolclaimed;
+   boolenabled;
unsignedmaxpacket:16;
unsignedmaxpacket_limit:16;
unsignedmax_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;
 }
 
 /**
-- 
1.9.1

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html