[PATCH] usb: gadget: add claimed field in struct usb_ep

[PATCH] usb: gadget: add claimed field in struct usb_ep

[Robert Baldyga]

This field allows to mark ep as claimed in more clear way. Claiming
endpoint by setting driver_data to non-null value is leaky solution
and makes code unreadable.

Signed-off-by: Robert Baldyga <r.baldyga@samsung.com>
---
 drivers/usb/gadget/epautoconf.c | 11 ++++++-----
 include/linux/usb/gadget.h      |  1 +
 2 files changed, 7 insertions(+), 5 deletions(-)

diff --git a/drivers/usb/gadget/epautoconf.c b/drivers/usb/gadget/epautoconf.c
index b1c5c0d..7acb933 100644
--- a/drivers/usb/gadget/epautoconf.c
+++ b/drivers/usb/gadget/epautoconf.c
@@ -54,7 +54,7 @@ ep_matches (
 	int		num_req_streams = 0;
 
 	/* endpoint already claimed? */
-	if (NULL != ep->driver_data)
+	if (ep->claimed)
 		return 0;
 
 	/* only support ep0 for portable CONTROL traffic */
@@ -241,7 +241,7 @@ find_ep (struct usb_gadget *gadget, const char *name)
  * updated with the assigned number of streams if it is
  * different from the original value. To prevent the endpoint
  * from being returned by a later autoconfig call, claim it by
- * assigning ep->driver_data to some non-null value.
+ * assigning ep->claimed to true.
  *
  * On failure, this returns a null endpoint descriptor.
  */
@@ -314,6 +314,7 @@ struct usb_ep *usb_ep_autoconfig_ss(
 found_ep:
 	ep->desc = NULL;
 	ep->comp_desc = NULL;
+	ep->claimed = true;
 	return ep;
 }
 EXPORT_SYMBOL_GPL(usb_ep_autoconfig_ss);
@@ -345,7 +346,7 @@ EXPORT_SYMBOL_GPL(usb_ep_autoconfig_ss);
  * descriptor bEndpointAddress.  For bulk endpoints, the wMaxPacket value
  * is initialized as if the endpoint were used at full speed.  To prevent
  * the endpoint from being returned by a later autoconfig call, claim it
- * by assigning ep->driver_data to some non-null value.
+ * by assigning ep->claimed to true.
  *
  * On failure, this returns a null endpoint descriptor.
  */
@@ -364,7 +365,7 @@ EXPORT_SYMBOL_GPL(usb_ep_autoconfig);
  *
  * Use this for devices where one configuration may need to assign
  * endpoint resources very differently from the next one.  It clears
- * state such as ep->driver_data and the record of assigned endpoints
+ * state such as ep->claimed and the record of assigned endpoints
  * used by usb_ep_autoconfig().
  */
 void usb_ep_autoconfig_reset (struct usb_gadget *gadget)
@@ -372,7 +373,7 @@ void usb_ep_autoconfig_reset (struct usb_gadget *gadget)
 	struct usb_ep	*ep;
 
 	list_for_each_entry (ep, &gadget->ep_list, ep_list) {
-		ep->driver_data = NULL;
+		ep->claimed = false;
 	}
 	gadget->in_epnum = 0;
 	gadget->out_epnum = 0;
diff --git a/include/linux/usb/gadget.h b/include/linux/usb/gadget.h
index a7ae4db..b3a9297 100644
--- a/include/linux/usb/gadget.h
+++ b/include/linux/usb/gadget.h
@@ -172,6 +172,7 @@ struct usb_ep {
 	const char		*name;
 	const struct usb_ep_ops	*ops;
 	struct list_head	ep_list;
+	bool			claimed;
 	unsigned		maxpacket:16;
 	unsigned		maxpacket_limit:16;
 	unsigned		max_streams:16;
-- 
1.9.1

Re: [PATCH] usb: gadget: add claimed field in struct usb_ep

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 314 bytes --]

On Mon, Jun 16, 2014 at 10:20:36AM +0200, Robert Baldyga wrote:
> This field allows to mark ep as claimed in more clear way. Claiming
> endpoint by setting driver_data to non-null value is leaky solution
> and makes code unreadable.

how come ? How can it be unreadable ? how can it be leaky ?

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH] usb: gadget: add claimed field in struct usb_ep

[Robert Baldyga]

On 06/19/2014 05:08 PM, Felipe Balbi wrote:
> On Mon, Jun 16, 2014 at 10:20:36AM +0200, Robert Baldyga wrote:
>> This field allows to mark ep as claimed in more clear way. Claiming
>> endpoint by setting driver_data to non-null value is leaky solution
>> and makes code unreadable.
> 
> how come ? How can it be unreadable ? how can it be leaky ?
> 

What if gadget will not assign any value to driver_data (just like
Gadget Zero do)? Endpoint will be seen as not used, and autoconfig will
return it more than one time. That's what I call leaky solution.

Information if endpoint is claimed or not is its internal state and
should not depend on assigning non-null value to driver_data field.

Re: [PATCH] usb: gadget: add claimed field in struct usb_ep

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 986 bytes --]

Hi,

On Mon, Jun 23, 2014 at 08:07:43AM +0200, Robert Baldyga wrote:
> On 06/19/2014 05:08 PM, Felipe Balbi wrote:
> > On Mon, Jun 16, 2014 at 10:20:36AM +0200, Robert Baldyga wrote:
> >> This field allows to mark ep as claimed in more clear way. Claiming
> >> endpoint by setting driver_data to non-null value is leaky solution
> >> and makes code unreadable.
> > 
> > how come ? How can it be unreadable ? how can it be leaky ?
> > 
> 
> What if gadget will not assign any value to driver_data (just like
> Gadget Zero do)? Endpoint will be seen as not used, and autoconfig will

huh ??? The gadget isn't the endpoint user, the function is. Look at
f_sourcesink.c and f_loopback.c. If the function doesn't set anything to
driver_data, then that's a bug on the function which needs fixing.

Moreover, if there's a function which doesn't set driver_data, we could
just as well have a function which doesn't set "claimed", so the problem
is the same.

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH] usb: gadget: add claimed field in struct usb_ep

[Robert Baldyga]

On 06/23/2014 08:27 PM, Felipe Balbi wrote:
> Hi,
> 
> On Mon, Jun 23, 2014 at 08:07:43AM +0200, Robert Baldyga wrote:
>> On 06/19/2014 05:08 PM, Felipe Balbi wrote:
>>> On Mon, Jun 16, 2014 at 10:20:36AM +0200, Robert Baldyga wrote:
>>>> This field allows to mark ep as claimed in more clear way. Claiming
>>>> endpoint by setting driver_data to non-null value is leaky solution
>>>> and makes code unreadable.
>>>
>>> how come ? How can it be unreadable ? how can it be leaky ?
>>>
>>
>> What if gadget will not assign any value to driver_data (just like
>> Gadget Zero do)? Endpoint will be seen as not used, and autoconfig will
> 
> huh ??? The gadget isn't the endpoint user, the function is. Look at
> f_sourcesink.c and f_loopback.c. If the function doesn't set anything to
> driver_data, then that's a bug on the function which needs fixing.
> 
> Moreover, if there's a function which doesn't set driver_data, we could
> just as well have a function which doesn't set "claimed", so the problem
> is the same.
> 

I mean the function, not the gadget. Sorry for confusion.
Mechanism I developed marks endpoint as claimed *inside autoconfig
function*. It's significant difference, because there's not possible to
forget to mark that endpoint is claimed. When endpoint is returned from
autoconfig function, it belongs to function, and the function doesn't
need to do anything to claim obtained endpoint - it's already done.

Re: [PATCH] usb: gadget: add claimed field in struct usb_ep

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 1874 bytes --]

On Tue, Jun 24, 2014 at 02:16:35PM +0200, Robert Baldyga wrote:
> On 06/23/2014 08:27 PM, Felipe Balbi wrote:
> > Hi,
> > 
> > On Mon, Jun 23, 2014 at 08:07:43AM +0200, Robert Baldyga wrote:
> >> On 06/19/2014 05:08 PM, Felipe Balbi wrote:
> >>> On Mon, Jun 16, 2014 at 10:20:36AM +0200, Robert Baldyga wrote:
> >>>> This field allows to mark ep as claimed in more clear way. Claiming
> >>>> endpoint by setting driver_data to non-null value is leaky solution
> >>>> and makes code unreadable.
> >>>
> >>> how come ? How can it be unreadable ? how can it be leaky ?
> >>>
> >>
> >> What if gadget will not assign any value to driver_data (just like
> >> Gadget Zero do)? Endpoint will be seen as not used, and autoconfig will
> > 
> > huh ??? The gadget isn't the endpoint user, the function is. Look at
> > f_sourcesink.c and f_loopback.c. If the function doesn't set anything to
> > driver_data, then that's a bug on the function which needs fixing.
> > 
> > Moreover, if there's a function which doesn't set driver_data, we could
> > just as well have a function which doesn't set "claimed", so the problem
> > is the same.
> > 
> 
> I mean the function, not the gadget. Sorry for confusion.
> Mechanism I developed marks endpoint as claimed *inside autoconfig
> function*. It's significant difference, because there's not possible to
> forget to mark that endpoint is claimed. When endpoint is returned from
> autoconfig function, it belongs to function, and the function doesn't
> need to do anything to claim obtained endpoint - it's already done.

we still might need to keep driver_data though. Some functions might
need it. But now that you explained your goal, I can see how that might
help. Please send a complete patchset also with the implementation for
ep_autoconfig so we can all review your idea.

cheers

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH] usb: gadget: add claimed field in struct usb_ep

[Robert Baldyga]

On 06/30/2014 08:33 PM, Felipe Balbi wrote:
> On Tue, Jun 24, 2014 at 02:16:35PM +0200, Robert Baldyga wrote:
>> On 06/23/2014 08:27 PM, Felipe Balbi wrote:
>>> Hi,
>>>
>>> On Mon, Jun 23, 2014 at 08:07:43AM +0200, Robert Baldyga wrote:
>>>> On 06/19/2014 05:08 PM, Felipe Balbi wrote:
>>>>> On Mon, Jun 16, 2014 at 10:20:36AM +0200, Robert Baldyga wrote:
>>>>>> This field allows to mark ep as claimed in more clear way. Claiming
>>>>>> endpoint by setting driver_data to non-null value is leaky solution
>>>>>> and makes code unreadable.
>>>>>
>>>>> how come ? How can it be unreadable ? how can it be leaky ?
>>>>>
>>>>
>>>> What if gadget will not assign any value to driver_data (just like
>>>> Gadget Zero do)? Endpoint will be seen as not used, and autoconfig will
>>>
>>> huh ??? The gadget isn't the endpoint user, the function is. Look at
>>> f_sourcesink.c and f_loopback.c. If the function doesn't set anything to
>>> driver_data, then that's a bug on the function which needs fixing.
>>>
>>> Moreover, if there's a function which doesn't set driver_data, we could
>>> just as well have a function which doesn't set "claimed", so the problem
>>> is the same.
>>>
>>
>> I mean the function, not the gadget. Sorry for confusion.
>> Mechanism I developed marks endpoint as claimed *inside autoconfig
>> function*. It's significant difference, because there's not possible to
>> forget to mark that endpoint is claimed. When endpoint is returned from
>> autoconfig function, it belongs to function, and the function doesn't
>> need to do anything to claim obtained endpoint - it's already done.
> 
> we still might need to keep driver_data though. Some functions might
> need it. But now that you explained your goal, I can see how that might
> help. Please send a complete patchset also with the implementation for
> ep_autoconfig so we can all review your idea.
> 

The patch that I send contains entire implementation. It's quite simple
idea:
- in ep_matches() we check if ep->claimed is true instead of checking if
ep->driver_data is set
- in usb_ep_autoconfig_ss() we set ep->claimed to true if we have found
suitable endpoint
- usb_ep_autoconfig_reset() we set ep->claimed to false

The struct usb_ep still have field driver_data, which can be used by
function driver. The only difference is that this field is not used to
indicate if endpoint is claimed or not, it just contains assigned pointer.

Best regards
Robert Baldyga