[2/3] usb: gadget: configfs: Create control_config group

[2/3] usb: gadget: configfs: Create control_config group

[Jerry Zhang]

> >> @@ -1461,6 +1499,18 @@ static struct config_group *gadgets_make(
> >>      if (!gi->composite.gadget_driver.function)
> >>              goto err;
> >>
> >> +    gi->control_config.c.label = "control_config";
> >> +    /* composite requires some value, but it doesn't matter */
> >> +    gi->control_config.c.bConfigurationValue = 42;
> >
> > If I understand correctly this is never exposed to the host, is it?
That's right. It won't be in cdev so it doesn't show up in the actual
descriptors and as such can't be enabled.

> >
> >> +    INIT_LIST_HEAD(&gi->control_config.func_list);
> >> +    config_group_init_type_name(&gi->control_config.group,
> >> +                    "control_config", &gadget_config_type);
> >> +    configfs_add_default_group(&gi->control_config.group, &gi->group);
> >
> > Since it is a config I'd rather this be put inside the "configs" group.
> > Configs created by the user must be named following the
> > <config name>.<bConfigurationValue> pattern, so there will be no
conflict
> > with any other conf. The name can be "control" then.
> >

> Answering my own doubts: this could be ok from the kernel point of view,
> but existing userspace (libusbgx) already assumes that in the configs
directory
> there are only entries of the form <config>.<number> and anything other
than that
> will cause it to report error.
Makes sense. I would be fine either way but it sounds like we're stuck
because of that library.
---
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[2/3] usb: gadget: configfs: Create control_config group

[Jerry Zhang]

> The purpose would be:
> 1) Allow writing no descriptors (maybe also skip the strings) when this
> flag is set
This should be straightforward. I'd rather not skip the strings though
since we can already indicate zero strings in the current struct. If we
skip strings then the difference between writing zero strings and skipping
them would become confusing.

> 2) Disallow linking such an instance to real configuration
> 3) Disallow linking real function implementation to our "magic config"
Hmm the issue with these is that descriptors aren't available at link time,
but rather at bind time. Its currently valid (and possibly useful) to link
a ffs instance and then write descriptors after.
We could do a mount flag but those aren't visible from configfs. How about
we fail to bind() if an empty function is in the normal config, or a
nonempty function is in the control config? This should be easy since it is
all visible in struct usb_function.

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

[2/3] usb: gadget: configfs: Create control_config group

[Krzysztof Opasiak]

On 19.04.2018 21:02, Jerry Zhang wrote:
[...]
> 
>> As main usecase for this code is FunctionFS I think that we should
>> consider adding some flag to FunctionFS to mark instance as only for
>> such purpouse. I mean sth like FFS_CONTROL_ONLY which would make
>> FunctionFS igore the descs (or allow to provide 0 of them) and make this
>> function usable only for this purpouse (disallow linking to real config
>> and allow only for linking to this fake one).
> I'm not sure what you mean the purpose of the flag to be. Would it be
> required for it to handle requests (so both ALL_CTRL and CONTROL_ONLY must
> be enabled)? Since userspace already has to link the functions, this seems
> more like an "are you sure?" switch as opposed to providing concrete
> functionality.
> Unless you mean that it wouldn't be required, but allowed in order for user
> to write no descriptors. Ffs allows for pretty bare bones descriptors
> already (1 speed, 1 interface, with no endpoints). If we want to allow 0
> speeds to be provided we might as well allow that generally. It wouldn't be
> useful in most cases, but that is similar to providing no endpoints anyway.

The purpose would be:
1) Allow writing no descriptors (maybe also skip the strings) when this 
flag is set
2) Disallow linking such an instance to real configuration
3) Disallow linking real function implementation to our "magic config"

Obviously you are right that it's not required but this improves 
usability. Even now our ConfigFS interface is pretty complicated and 
gives user many chances for "silent misconfiguration". It would be nice 
to protect user against stupid and very hard to debug mistakes rather 
than giving this child even more weapon;)

Best regards,

[2/3] usb: gadget: configfs: Create control_config group

[Jerry Zhang]

> [...]
> >
> > +     cfg = &gi->control_config;
> > +     c = &cfg->c;
> > +     list_for_each_entry_safe(f, tmp, &cfg->func_list, list) {
> > +             if (f->req_match && f->setup) {
> > +                     list_del(&f->list);
> > +                     if (usb_add_function(&cfg->c, f))
> > +                             list_add(&f->list, &cfg->func_list);
> > +             }
> > +     }

> shouldn't we goto error here instead of silently ignoring error?
yeah you're right


> > +
> >       usb_ep_autoconfig_reset(cdev->gadget);
> >       return 0;
> >
> > @@ -1391,11 +1407,33 @@ static void configfs_composite_unbind(struct
usb_gadget *gadget)
> >       set_gadget_data(gadget, NULL);
> >   }
> >
> > +static int configfs_composite_setup(struct usb_gadget *gadget,
> > +                     const struct usb_ctrlrequest *c)
> > +{
> > +     struct usb_composite_dev *cdev = get_gadget_data(gadget);
> > +     struct gadget_info *gi = container_of(cdev, struct gadget_info,
cdev);
> > +
> > +     struct config_usb_cfg *cfg = &gi->control_config;
> > +     struct usb_function *f;
> > +     int value = composite_setup(gadget, c);

> I think it would be more readable if you call this function later in the
> code instead of during initialization of variable.
ack


> > +
> > +     if (value >= 0)
> > +             return value;
> > +
> > +     list_for_each_entry(f, &cfg->c.functions, list) {
> > +             if (f->req_match(f, c, false)) {
> > +                     value = f->setup(f, c);
> > +                     break;
> > +             }
> > +     }
> > +     return value;
> > +}
> > +
> >   static const struct usb_gadget_driver configfs_driver_template = {
> >       .bind           = configfs_composite_bind,
> >       .unbind         = configfs_composite_unbind,
> >
> > -     .setup          = composite_setup,
> > +     .setup          = configfs_composite_setup,
> >       .reset          = composite_disconnect,
> >       .disconnect     = composite_disconnect,
> >
> > @@ -1461,6 +1499,18 @@ static struct config_group *gadgets_make(
> >       if (!gi->composite.gadget_driver.function)
> >               goto err;
> >
> > +     gi->control_config.c.label = "control_config";
> > +     /* composite requires some value, but it doesn't matter */
> > +     gi->control_config.c.bConfigurationValue = 42;
> > +     INIT_LIST_HEAD(&gi->control_config.func_list);
> > +     config_group_init_type_name(&gi->control_config.group,
> > +                     "control_config", &gadget_config_type);
> > +     configfs_add_default_group(&gi->control_config.group, &gi->group);
> > +
> > +     if (usb_add_config_only(&gi->cdev, &gi->control_config.c))
> > +             goto err;
> > +     list_del(&gi->control_config.c.list);
> > +

> I know that you are trying to reuse as much code as possible but in my
> humble opinion we should make a separate config_item_type for this to
> drop things related to string bMaxPower etc as all those values are then
> later ignored in the kernel so it looks like it doesn't make any sense
> to allow users to set them.
Fair enough. I was debating between the two but chose this route to get my
patch out faster


> As main usecase for this code is FunctionFS I think that we should
> consider adding some flag to FunctionFS to mark instance as only for
> such purpouse. I mean sth like FFS_CONTROL_ONLY which would make
> FunctionFS igore the descs (or allow to provide 0 of them) and make this
> function usable only for this purpouse (disallow linking to real config
> and allow only for linking to this fake one).
I'm not sure what you mean the purpose of the flag to be. Would it be
required for it to handle requests (so both ALL_CTRL and CONTROL_ONLY must
be enabled)? Since userspace already has to link the functions, this seems
more like an "are you sure?" switch as opposed to providing concrete
functionality.
Unless you mean that it wouldn't be required, but allowed in order for user
to write no descriptors. Ffs allows for pretty bare bones descriptors
already (1 speed, 1 interface, with no endpoints). If we want to allow 0
speeds to be provided we might as well allow that generally. It wouldn't be
useful in most cases, but that is similar to providing no endpoints anyway.

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

[2/3] usb: gadget: configfs: Create control_config group

[Krzysztof Opasiak]

On 17.04.2018 03:17, Jerry Zhang wrote:
[...]
>   
> +	cfg = &gi->control_config;
> +	c = &cfg->c;
> +	list_for_each_entry_safe(f, tmp, &cfg->func_list, list) {
> +		if (f->req_match && f->setup) {
> +			list_del(&f->list);
> +			if (usb_add_function(&cfg->c, f))
> +				list_add(&f->list, &cfg->func_list);
> +		}
> +	}

shouldn't we goto error here instead of silently ignoring error?

> +
>   	usb_ep_autoconfig_reset(cdev->gadget);
>   	return 0;
>   
> @@ -1391,11 +1407,33 @@ static void configfs_composite_unbind(struct usb_gadget *gadget)
>   	set_gadget_data(gadget, NULL);
>   }
>   
> +static int configfs_composite_setup(struct usb_gadget *gadget,
> +			const struct usb_ctrlrequest *c)
> +{
> +	struct usb_composite_dev *cdev = get_gadget_data(gadget);
> +	struct gadget_info *gi = container_of(cdev, struct gadget_info, cdev);
> +
> +	struct config_usb_cfg *cfg = &gi->control_config;
> +	struct usb_function *f;
> +	int value = composite_setup(gadget, c);

I think it would be more readable if you call this function later in the 
code instead of during initialization of variable.

> +
> +	if (value >= 0)
> +		return value;
> +
> +	list_for_each_entry(f, &cfg->c.functions, list) {
> +		if (f->req_match(f, c, false)) {
> +			value = f->setup(f, c);
> +			break;
> +		}
> +	}
> +	return value;
> +}
> +
>   static const struct usb_gadget_driver configfs_driver_template = {
>   	.bind           = configfs_composite_bind,
>   	.unbind         = configfs_composite_unbind,
>   
> -	.setup          = composite_setup,
> +	.setup          = configfs_composite_setup,
>   	.reset          = composite_disconnect,
>   	.disconnect     = composite_disconnect,
>   
> @@ -1461,6 +1499,18 @@ static struct config_group *gadgets_make(
>   	if (!gi->composite.gadget_driver.function)
>   		goto err;
>   
> +	gi->control_config.c.label = "control_config";
> +	/* composite requires some value, but it doesn't matter */
> +	gi->control_config.c.bConfigurationValue = 42;
> +	INIT_LIST_HEAD(&gi->control_config.func_list);
> +	config_group_init_type_name(&gi->control_config.group,
> +			"control_config", &gadget_config_type);
> +	configfs_add_default_group(&gi->control_config.group, &gi->group);
> +
> +	if (usb_add_config_only(&gi->cdev, &gi->control_config.c))
> +		goto err;
> +	list_del(&gi->control_config.c.list);
> +

I know that you are trying to reuse as much code as possible but in my 
humble opinion we should make a separate config_item_type for this to 
drop things related to string bMaxPower etc as all those values are then 
later ignored in the kernel so it looks like it doesn't make any sense 
to allow users to set them.

As main usecase for this code is FunctionFS I think that we should 
consider adding some flag to FunctionFS to mark instance as only for 
such purpouse. I mean sth like FFS_CONTROL_ONLY which would make 
FunctionFS igore the descs (or allow to provide 0 of them) and make this 
function usable only for this purpouse (disallow linking to real config 
and allow only for linking to this fake one).

Best regards,

[2/3] usb: gadget: configfs: Create control_config group

[Andrzej Pietrasiewicz]

Hi again,

W dniu 17.04.2018 o 09:55, Andrzej Pietrasiewicz pisze:
> Hi,
> 
> W dniu 17.04.2018 o 03:17, Jerry Zhang pisze:
>> Control_config is a group under gadget that acts
> 
> <snip>
> 
>>    
>> @@ -1461,6 +1499,18 @@ static struct config_group *gadgets_make(
>>    	if (!gi->composite.gadget_driver.function)
>>    		goto err;
>>    
>> +	gi->control_config.c.label = "control_config";
>> +	/* composite requires some value, but it doesn't matter */
>> +	gi->control_config.c.bConfigurationValue = 42;
> 
> If I understand correctly this is never exposed to the host, is it?
> 
>> +	INIT_LIST_HEAD(&gi->control_config.func_list);
>> +	config_group_init_type_name(&gi->control_config.group,
>> +			"control_config", &gadget_config_type);
>> +	configfs_add_default_group(&gi->control_config.group, &gi->group);
> 
> Since it is a config I'd rather this be put inside the "configs" group.
> Configs created by the user must be named following the
> <config name>.<bConfigurationValue> pattern, so there will be no conflict
> with any other conf. The name can be "control" then.
> 

Answering my own doubts: this could be ok from the kernel point of view,
but existing userspace (libusbgx) already assumes that in the configs directory
there are only entries of the form <config>.<number> and anything other than that
will cause it to report error.

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

[2/3] usb: gadget: configfs: Create control_config group

[Andrzej Pietrasiewicz]

Hi,

W dniu 17.04.2018 o 03:17, Jerry Zhang pisze:
> Control_config is a group under gadget that acts

<snip>

>   
> @@ -1461,6 +1499,18 @@ static struct config_group *gadgets_make(
>   	if (!gi->composite.gadget_driver.function)
>   		goto err;
>   
> +	gi->control_config.c.label = "control_config";
> +	/* composite requires some value, but it doesn't matter */
> +	gi->control_config.c.bConfigurationValue = 42;

If I understand correctly this is never exposed to the host, is it?

> +	INIT_LIST_HEAD(&gi->control_config.func_list);
> +	config_group_init_type_name(&gi->control_config.group,
> +			"control_config", &gadget_config_type);
> +	configfs_add_default_group(&gi->control_config.group, &gi->group);

Since it is a config I'd rather this be put inside the "configs" group.
Configs created by the user must be named following the
<config name>.<bConfigurationValue> pattern, so there will be no conflict
with any other conf. The name can be "control" then.

> +
> +	if (usb_add_config_only(&gi->cdev, &gi->control_config.c))
> +		goto err;
> +	list_del(&gi->control_config.c.list);
> +
>   	return &gi->group;
>   err:
>   	kfree(gi);
> 

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

[2/3] usb: gadget: configfs: Create control_config group

[Jerry Zhang]

Control_config is a group under gadget that acts
as a normal config group, except it does not
appear in cdev->configs. Functions can be linked
into the config as normal, and those functions
are bound and unbound with the rest of the gadget.

Create configfs_setup(), which will first attempt
composite setup. If that fails, it will go through
functions in control_config and use req_match to
find one that can handle the control request.

This allows the user to create a functionfs instance
dedicated to handling non-standard control requests
no matter what functions or configurations are
currently active.

Signed-off-by: Jerry Zhang <zhangjerry@google.com>
---
 drivers/usb/gadget/configfs.c | 86 +++++++++++++++++++++++++++--------
 1 file changed, 68 insertions(+), 18 deletions(-)

diff --git a/drivers/usb/gadget/configfs.c b/drivers/usb/gadget/configfs.c
index efba66ca0719..b3acddda24c1 100644
--- a/drivers/usb/gadget/configfs.c
+++ b/drivers/usb/gadget/configfs.c
@@ -44,12 +44,22 @@ int check_user_usb_string(const char *name,
 
 static const struct usb_descriptor_header *otg_desc[2];
 
+struct config_usb_cfg {
+	struct config_group group;
+	struct config_group strings_group;
+	struct list_head string_list;
+	struct usb_configuration c;
+	struct list_head func_list;
+	struct usb_gadget_strings *gstrings[MAX_USB_STRING_LANGS + 1];
+};
+
 struct gadget_info {
 	struct config_group group;
 	struct config_group functions_group;
 	struct config_group configs_group;
 	struct config_group strings_group;
 	struct config_group os_desc_group;
+	struct config_usb_cfg control_config;
 
 	struct mutex lock;
 	struct usb_gadget_strings *gstrings[MAX_USB_STRING_LANGS + 1];
@@ -68,15 +78,6 @@ static inline struct gadget_info *to_gadget_info(struct config_item *item)
 	 return container_of(to_config_group(item), struct gadget_info, group);
 }
 
-struct config_usb_cfg {
-	struct config_group group;
-	struct config_group strings_group;
-	struct list_head string_list;
-	struct usb_configuration c;
-	struct list_head func_list;
-	struct usb_gadget_strings *gstrings[MAX_USB_STRING_LANGS + 1];
-};
-
 static inline struct config_usb_cfg *to_config_usb_cfg(struct config_item *item)
 {
 	return container_of(to_config_group(item), struct config_usb_cfg,
@@ -1205,11 +1206,10 @@ int composite_os_desc_req_prepare(struct usb_composite_dev *cdev,
 static void purge_configs_funcs(struct gadget_info *gi)
 {
 	struct usb_configuration	*c;
+	struct usb_function *f, *tmp;
+	struct config_usb_cfg *cfg;
 
 	list_for_each_entry(c, &gi->cdev.configs, list) {
-		struct usb_function *f, *tmp;
-		struct config_usb_cfg *cfg;
-
 		cfg = container_of(c, struct config_usb_cfg, c);
 
 		list_for_each_entry_safe(f, tmp, &c->functions, list) {
@@ -1229,6 +1229,14 @@ static void purge_configs_funcs(struct gadget_info *gi)
 		c->highspeed = 0;
 		c->fullspeed = 0;
 	}
+
+	cfg = &gi->control_config;
+	c = &cfg->c;
+	list_for_each_entry_safe(f, tmp, &c->functions, list) {
+		list_move_tail(&f->list, &cfg->func_list);
+		if (f->unbind)
+			f->unbind(c, f);
+	}
 }
 
 static int configfs_composite_bind(struct usb_gadget *gadget,
@@ -1242,6 +1250,9 @@ static int configfs_composite_bind(struct usb_gadget *gadget,
 	struct usb_string		*s;
 	unsigned			i;
 	int				ret;
+	struct config_usb_cfg *cfg;
+	struct usb_function *f;
+	struct usb_function *tmp;
 
 	/* the gi->lock is hold by the caller */
 	cdev->gadget = gadget;
@@ -1260,8 +1271,6 @@ static int configfs_composite_bind(struct usb_gadget *gadget,
 
 
 	list_for_each_entry(c, &gi->cdev.configs, list) {
-		struct config_usb_cfg *cfg;
-
 		cfg = container_of(c, struct config_usb_cfg, c);
 		if (list_empty(&cfg->func_list)) {
 			pr_err("Config %s/%d of %s needs at least one function.\n",
@@ -1320,9 +1329,6 @@ static int configfs_composite_bind(struct usb_gadget *gadget,
 
 	/* Go through all configs, attach all functions */
 	list_for_each_entry(c, &gi->cdev.configs, list) {
-		struct config_usb_cfg *cfg;
-		struct usb_function *f;
-		struct usb_function *tmp;
 		struct gadget_config_name *cn;
 
 		if (gadget_is_otg(gadget))
@@ -1362,6 +1368,16 @@ static int configfs_composite_bind(struct usb_gadget *gadget,
 			goto err_purge_funcs;
 	}
 
+	cfg = &gi->control_config;
+	c = &cfg->c;
+	list_for_each_entry_safe(f, tmp, &cfg->func_list, list) {
+		if (f->req_match && f->setup) {
+			list_del(&f->list);
+			if (usb_add_function(&cfg->c, f))
+				list_add(&f->list, &cfg->func_list);
+		}
+	}
+
 	usb_ep_autoconfig_reset(cdev->gadget);
 	return 0;
 
@@ -1391,11 +1407,33 @@ static void configfs_composite_unbind(struct usb_gadget *gadget)
 	set_gadget_data(gadget, NULL);
 }
 
+static int configfs_composite_setup(struct usb_gadget *gadget,
+			const struct usb_ctrlrequest *c)
+{
+	struct usb_composite_dev *cdev = get_gadget_data(gadget);
+	struct gadget_info *gi = container_of(cdev, struct gadget_info, cdev);
+
+	struct config_usb_cfg *cfg = &gi->control_config;
+	struct usb_function *f;
+	int value = composite_setup(gadget, c);
+
+	if (value >= 0)
+		return value;
+
+	list_for_each_entry(f, &cfg->c.functions, list) {
+		if (f->req_match(f, c, false)) {
+			value = f->setup(f, c);
+			break;
+		}
+	}
+	return value;
+}
+
 static const struct usb_gadget_driver configfs_driver_template = {
 	.bind           = configfs_composite_bind,
 	.unbind         = configfs_composite_unbind,
 
-	.setup          = composite_setup,
+	.setup          = configfs_composite_setup,
 	.reset          = composite_disconnect,
 	.disconnect     = composite_disconnect,
 
@@ -1461,6 +1499,18 @@ static struct config_group *gadgets_make(
 	if (!gi->composite.gadget_driver.function)
 		goto err;
 
+	gi->control_config.c.label = "control_config";
+	/* composite requires some value, but it doesn't matter */
+	gi->control_config.c.bConfigurationValue = 42;
+	INIT_LIST_HEAD(&gi->control_config.func_list);
+	config_group_init_type_name(&gi->control_config.group,
+			"control_config", &gadget_config_type);
+	configfs_add_default_group(&gi->control_config.group, &gi->group);
+
+	if (usb_add_config_only(&gi->cdev, &gi->control_config.c))
+		goto err;
+	list_del(&gi->control_config.c.list);
+
 	return &gi->group;
 err:
 	kfree(gi);