Re: [PATCH 2/3] usb: gadget: configfs: Create control_config group
> 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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/3] usb: gadget: configfs: Create control_config group
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, -- 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 2/3] usb: gadget: configfs: Create control_config group
> [...]
> >
> > + cfg = >control_config;
> > + c = >c;
> > + list_for_each_entry_safe(f, tmp, >func_list, list) {
> > + if (f->req_match && f->setup) {
> > + list_del(>list);
> > + if (usb_add_function(>c, f))
> > + list_add(>list, >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 = >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, >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(>control_config.func_list);
> > + config_group_init_type_name(>control_config.group,
> > + "control_config", _config_type);
> > + configfs_add_default_group(>control_config.group, >group);
> > +
> > + if (usb_add_config_only(>cdev, >control_config.c))
> > + goto err;
> > + list_del(>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 majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/3] usb: gadget: configfs: Create control_config group
On 17.04.2018 03:17, Jerry Zhang wrote:
[...]
+ cfg = >control_config;
+ c = >c;
+ list_for_each_entry_safe(f, tmp, >func_list, list) {
+ if (f->req_match && f->setup) {
+ list_del(>list);
+ if (usb_add_function(>c, f))
+ list_add(>list, >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 = >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, >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(>control_config.func_list);
+ config_group_init_type_name(>control_config.group,
+ "control_config", _config_type);
+ configfs_add_default_group(>control_config.group, >group);
+
+ if (usb_add_config_only(>cdev, >control_config.c))
+ goto err;
+ list_del(>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,
--
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 2/3] usb: gadget: configfs: Create control_config group
> >> @@ -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(>control_config.func_list); > >> +config_group_init_type_name(>control_config.group, > >> +"control_config", _config_type); > >> +configfs_add_default_group(>control_config.group, >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 > > . 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 . 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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/3] usb: gadget: configfs: Create control_config group
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 > > > >> >> @@ -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(>control_config.func_list); >> +config_group_init_type_name(>control_config.group, >> +"control_config", _config_type); >> +configfs_add_default_group(>control_config.group, >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 > . 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 . 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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/3] usb: gadget: configfs: Create control_config group
Hi, W dniu 17.04.2018 o 03:17, Jerry Zhang pisze: > Control_config is a group under gadget that acts > > @@ -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(>control_config.func_list); > + config_group_init_type_name(>control_config.group, > + "control_config", _config_type); > + configfs_add_default_group(>control_config.group, >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 . pattern, so there will be no conflict with any other conf. The name can be "control" then. > + > + if (usb_add_config_only(>cdev, >control_config.c)) > + goto err; > + list_del(>control_config.c.list); > + > return >group; > err: > kfree(gi); > Andrzej -- 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
