| Message ID | 20180417011742.192184-3-zhangjerry@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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
> >> @@ -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
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,
> [...] > > > > + 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
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,
> 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
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);
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(-)