On Wednesday, November 28, 2012 7:51 PM Sebastian Andrzej Siewior wrote:
> Subject: [RFC] usb/gadget: slow start of the configfs interface
>
> |# modprobe dummy_hcd num=2
>
> |# find /sys/kernel/config/ -ls
> | 6547 0 drwxr-xr-x 5 root root 0 Nov 28 19:39
> /sys/kernel/config/
> | 532 0 drwxr-xr-x 2 root root 0 Nov 28 19:39
> /sys/kernel/config/dummy_udc.1
> | 531 0 drwxr-xr-x 2 root root 0 Nov 28 19:39
> /sys/kernel/config/dummy_udc.0
> | 6548 0 drwxr-xr-x 4 root root 0 Nov 28 19:38
> /sys/kernel/config/usb_gadget
> | 6550 0 drwxr-xr-x 2 root root 0 Nov 28 19:38
> /sys/kernel/config/usb_gadget/gadgets
> | 6549 0 drwxr-xr-x 2 root root 0 Nov 28 19:38
> /sys/kernel/config/usb_gadget/functions
>
> | # lsmod
> | Module Size Used by
> | dummy_hcd 20287 0
> | libcomposite 17052 0
>
> |# mkdir /sys/kernel/config/usb_gadget/functions/acm_one
>
> | # lsmod
> | Module Size Used by
> | f_acm 5306 0
> | u_serial 9644 1 f_acm
> | dummy_hcd 20287 0
> | libcomposite 17052 1 f_acm
>
> Now:
> - the UDC (dummy_udc.[01]) is not within /usb_gadget/ folder where we
> have /gadgets/ and /functions/ subfolder. This is what
> spear13xx_pcie_gadget does as well. Is this okay or do we want to have
> them enumerated below /usb_gadget/ as well? If so we got to convince
> Joel to take Andrzej's patch for that.
I would want udcs under /usb_gadget. This way all the usb gadget-related
stuff is grouped into one configfs subsystem. I am also not sure,
but I would just guess that creating symlinks between different
configfs subsystems can be tricky.
> - the function name for acm is taken from the dir name. Now MichaÅ?, is
> it too much of hackery?
>
> This is a slow start. Other comments?
>
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@xxxxxxxxxxxxx>
> ---
> drivers/usb/gadget/Makefile | 4 +-
> drivers/usb/gadget/configfs.c | 189
> +++++++++++++++++++++++++++++++++++++++++
> drivers/usb/gadget/udc-core.c | 13 +++
> include/linux/usb/gadget.h | 5 ++
> 4 files changed, 209 insertions(+), 2 deletions(-) create mode 100644
> drivers/usb/gadget/configfs.c
>
> diff --git a/drivers/usb/gadget/Makefile b/drivers/usb/gadget/Makefile
> index 97a13c3..d678e49 100644
> --- a/drivers/usb/gadget/Makefile
> +++ b/drivers/usb/gadget/Makefile
> @@ -3,10 +3,10 @@
> #
> ccflags-$(CONFIG_USB_GADGET_DEBUG) := -DDEBUG
>
> -obj-$(CONFIG_USB_GADGET) += udc-core.o
> +obj-$(CONFIG_USB_GADGET) += udc-core.o configfs.o functions.o
> obj-$(CONFIG_USB_LIBCOMPOSITE) += libcomposite.o
> libcomposite-y := usbstring.o config.o epautoconf.o
> -libcomposite-y += composite.o functions.o
> +libcomposite-y += composite.o
> obj-$(CONFIG_USB_DUMMY_HCD) += dummy_hcd.o
> obj-$(CONFIG_USB_NET2272) += net2272.o
> obj-$(CONFIG_USB_NET2280) += net2280.o
> diff --git a/drivers/usb/gadget/configfs.c b/drivers/usb/gadget/configfs.c
> new file mode 100644 index 0000000..536c122
> --- /dev/null
> +++ b/drivers/usb/gadget/configfs.c
> @@ -0,0 +1,189 @@
> +#include <linux/configfs.h>
> +#include <linux/module.h>
> +#include <linux/slab.h>
> +#include <linux/device.h>
> +#include <linux/usb/composite.h>
> +
> +static struct config_group *gadget_core_register(
> + struct config_group *group,
> + const char *name)
> +{
> + pr_err("%s() %s\n", __func__, name);
> + return ERR_PTR(-EINVAL);
> +}
> +
I would use a name which is more configfs-specific, e.g.
+static struct config_group *make_gagdet_core(
or
+static struct config_group *gadget_core_make(
but the former seems more natural to me, though.
All group/item operations are never called directly so there
is no need to make the name more meaningful to a
hypothetical user.
Instead, these operations names are used only from a
configfs-specific code in an assignment, so it is better
for them to have names meaningful to configfs-specific code.
In other words: their user is a configfs-specific code
so the names should be friendly to the actual user.
> +static void gadget_core_deregister(
> + struct config_group *group,
> + struct config_item *item)
> +{
> + pr_err("%s()\n", __func__);
> +}
> +
+static void drop_gadget_core(
And in general, what are the above two functions for
other than failing and printing an error? If you use
them in order to have _something_ in make_group and drop_item
then don't - group operations _can_ be NULL in which case
the mkdir just fails with -EPERM.
> +static struct configfs_group_operations gadget_core_group_ops = {
> + .make_group = &gadget_core_register,
> + .drop_item = &gadget_core_deregister,
> +};
> +
> +static struct config_item_type gadget_core_item = {
> + .ct_group_ops = &gadget_core_group_ops,
> + .ct_owner = THIS_MODULE,
> +};
> +
This is confusing. You call something which is a config_item_type
an item, which is similar to a config_item, which is in turn
a completely different thing than config_item_type. I would go for
+static struct config_item_type gadget_core_type = {
> +static struct config_group *gadget_create_gadget(
> + struct config_group *group,
> + const char *name)
> +{
> + pr_err("%s() %s\n", __func__, name);
> + return ERR_PTR(-EINVAL);
> +}
> +
+static struct config_group *make_gadget(
> +static void gadget_destroy_gadget(
> + struct config_group *group,
> + struct config_item *item)
> +{
> + pr_err("%s()\n", __func__);
> +}
> +
+static struct config_group *drop_gadget(
> +static struct configfs_group_operations gadget_attach = {
> + .make_group = &gadget_create_gadget,
> + .drop_item = &gadget_destroy_gadget,
> +};
> +
Why not
+static struct configfs_group_operations gadget_group_ops = {
?
This would be consistent with gadget_core_group_ops.
> +static struct config_item_type gadget_item = {
> + .ct_group_ops = &gadget_attach,
> + .ct_owner = THIS_MODULE,
> +};
> +
+static struct config_item_type gadget_type = {
> +static struct config_group gadget_group = {
> + .cg_item = {
> + .ci_namebuf = "gadgets",
> + .ci_type = &gadget_item,
+ .ci_type = &gadget_type,
> + },
> +};
> +
> +#define MAX_NAME_LEN 40
> +static struct config_group *gadget_load_func(
> + struct config_group *group,
> + const char *name)
+static struct config_group *make_func(
but see below
> +{
> + struct usb_function *f;
> + char buf[MAX_NAME_LEN];
> + char *func_name;
> + char *instance_name;
> + int ret;
> +
> + ret = snprintf(buf, MAX_NAME_LEN, "%s", name);
> + if (ret >= MAX_NAME_LEN)
> + ERR_PTR(-ENAMETOOLONG);
> +
> + func_name = buf;
> + instance_name = strstr(func_name, "_");
> + if (!instance_name) {
> + pr_err("Unable to locate _ in FUNC_INSTANCE\n");
> + return ERR_PTR(-EINVAL);
> + }
> + *instance_name = '\0';
> + instance_name++;
> +
> + pr_err("%s() try to get %s for %s\n", __func__, func_name,
> instance_name);
> + f = usb_get_function(func_name);
> + usb_put_function(f);
This kind of code belongs to other places, e.g. usb_functions.c.
You get the function only to put it the very next line which indicates
that this wants to be called from somewhere else.
I think there should be one configfs subsystem for all the gadget
stuff, but the config_groups/config_items with their components
should live in files which deal with their corresponding entities.
"gadgets" and "functions" can still be implemented as default groups,
but _their_ make_group & co can be provided in other files.
> + pr_err("%s() %s\n", __func__,
> + IS_ERR(f) ? "failed" : "good");
> + return ERR_PTR(-EINVAL);
> +}
> +
> +static void gadget_unload_func(
+static void drop_func(
but see above
> + struct config_group *group,
> + struct config_item *item)
> +{
> + pr_err("%s()\n", __func__);
> +}
> +
Any other use of the function than printing an error?
> +static struct configfs_group_operations gadget_attach_func = {
> +static struct configfs_group_operations func_ops = {
> + .make_group = &gadget_load_func,
> + .drop_item = &gadget_unload_func,
> +};
> +
> +static struct config_item_type gadget_func_item = {
+static struct config_item_type func_type = {
> + .ct_group_ops = &gadget_attach_func,
> + .ct_owner = THIS_MODULE,
> +};
> +
> +static struct config_group func_group = {
> + .cg_item = {
> + .ci_namebuf = "functions",
> + .ci_type = &gadget_func_item,
> + },
> +};
> +
> +static struct config_group *root_groups[] = {
> + &func_group,
> + &gadget_group,
> + NULL
> +};
> +
> +static struct configfs_subsystem gadget_subsys = {
> + .su_group = {
> + .cg_item = {
> + .ci_namebuf = "usb_gadget",
> + .ci_type = &gadget_core_item,
> + },
> + .default_groups = root_groups,
> + },
> + .su_mutex = __MUTEX_INITIALIZER(gadget_subsys.su_mutex),
> +};
> +
> +struct udc_configfs {
> + struct configfs_subsystem subsys;
> +};
> +
Like I said, I would want udcs in the same subsystem as gadgets.
> +struct configfs_subsystem *udc_add_configfs(struct device *dev) {
> + struct udc_configfs *udc_cfs;
> + struct configfs_subsystem *subsys;
> + int ret;
> +
> + udc_cfs = kzalloc(sizeof(struct udc_configfs), GFP_KERNEL);
> + if (!udc_cfs)
> + return ERR_PTR(-ENOMEM);
> +
> + subsys = &udc_cfs->subsys;
> + config_group_init_type_name(&subsys->su_group,
> + kobject_name(&dev->kobj),
> + &gadget_core_item);
> + ret = configfs_register_subsystem(subsys);
> + if (ret)
> + goto err;
> + return subsys;
> +err:
> + kfree(udc_cfs);
> + return ERR_PTR(ret);
> +}
> +
> +void udc_del_configfs(struct configfs_subsystem *subsys) {
> + struct udc_configfs *udc_cfs;
> +
> + udc_cfs = container_of(subsys, struct udc_configfs, subsys);
> +
> + configfs_unregister_subsystem(subsys);
> + kfree(udc_cfs);
> +}
> +
> +int __init gadget_cfs_init(void)
> +{
> + int ret;
> +
> + config_group_init(&gadget_subsys.su_group);
> + config_group_init(&func_group);
> + config_group_init(&gadget_group);
> +
> + ret = configfs_register_subsystem(&gadget_subsys);
> + return ret;
Perhaps:
+ return configfs_register_subsystem(&gadget_subsys);
since ret is not used for anything else?
> +}
> +
> +void __exit gadget_cfs_exit(void)
> +{
> + configfs_unregister_subsystem(&gadget_subsys);
> +}
> diff --git a/drivers/usb/gadget/udc-core.c b/drivers/usb/gadget/udc-core.c
> index 4d90a80..2c52b82 100644
> --- a/drivers/usb/gadget/udc-core.c
> +++ b/drivers/usb/gadget/udc-core.c
> @@ -26,6 +26,7 @@
>
> #include <linux/usb/ch9.h>
> #include <linux/usb/gadget.h>
> +#include <linux/configfs.h>
>
> /**
> * struct usb_udc - describes one usb device controller @@ -42,6 +43,7 @@
> struct usb_udc {
> struct usb_gadget *gadget;
> struct device dev;
> struct list_head list;
> + struct configfs_subsystem *subsys;
> };
>
> static struct class *udc_class;
> @@ -231,6 +233,7 @@ int usb_add_gadget_udc(struct device *parent, struct
> usb_gadget *gadget)
> if (ret)
> goto err3;
>
> + udc->subsys = udc_add_configfs(&udc->dev);
> mutex_unlock(&udc_lock);
>
> return 0;
> @@ -305,6 +308,7 @@ void usb_del_gadget_udc(struct usb_gadget *gadget)
> usb_gadget_remove_driver(udc);
>
> kobject_uevent(&udc->dev.kobj, KOBJ_REMOVE);
> + udc_del_configfs(udc->subsys);
> device_unregister(&udc->dev);
> }
> EXPORT_SYMBOL_GPL(usb_del_gadget_udc);
> @@ -516,8 +520,17 @@ static int __init usb_udc_init(void) }
> subsys_initcall(usb_udc_init);
>
> +static int __init usb_udc_init_mod(void) {
> + if (IS_ERR(udc_class))
> + return PTR_ERR(udc_class);
> + return gadget_cfs_init();
> +}
> +module_init(usb_udc_init_mod);
> +
> static void __exit usb_udc_exit(void)
> {
> + gadget_cfs_exit();
> class_destroy(udc_class);
> }
> module_exit(usb_udc_exit);
> diff --git a/include/linux/usb/gadget.h b/include/linux/usb/gadget.h index
> 0af6569..4d45962 100644
> --- a/include/linux/usb/gadget.h
> +++ b/include/linux/usb/gadget.h
> @@ -883,6 +883,11 @@ extern void usb_del_gadget_udc(struct usb_gadget
> *gadget);
>
> /*-----------------------------------------------------------------------
> --*/
>
> +struct configfs_subsystem *udc_add_configfs(struct device *dev); void
> +udc_del_configfs(struct configfs_subsystem *subsys); int
> +gadget_cfs_init(void); void __exit gadget_cfs_exit(void);
> +
> /* utility to simplify dealing with string descriptors */
>
> /**
> --
> 1.7.10.4
I like the udc part from
> +struct configfs_subsystem *udc_add_configfs(struct device *dev) {
onwards since this was missing (taking into account that there should
be one configfs subsystem).
But the gadget, function, interface, endpoint config groups and handling
thereof is already implemented in usb_functions.c. Why not use them?
Andrzej
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
