Hello Sebastian,
On Sunday, December 23, 2012 9:10 PM Sebastian Andrzej Siewior wrote:
> Subject: [PATCH 06/30] usb/gadget: add some infracture to
> register/unregister functions
>
> This patch provides an infrastructure to register & unregister a USB
> function. This allows to turn a function into a module and avoid the
> '#include "f_.*.c"' magic and we get a clear API / cut between the bare
> gadget and its functions.
> The concept is simple:
> Each function defines the DECLARE_USB_FUNCTION_INIT macro whith an unique
> name of the function and two allocation functions.
> - one to create an "instance". The instance holds the current
> configuration
> set. In case there are two usb_configudations with one function there
> will
> be one instance and two usb_functions
> - one to create an "function" from the instance.
>
This naming scheme seems confusing when compared with the
object-oriented (OO) parlance. In OO there is a class which
can be instantiated in many instances. It is exactly the opposite
here: there is one instance for which many functions can be created.
The idea of having this "instance" struct is not bad itself.
As far as I understand it there are usb_function_drivers, which are
used to keep the information about currently loaded modules which
implement usb functions. Then there are "usb_function_instances",
which are used to actually cause a module implementing a usb function
to load, e.g. so that configfs has something to operate on before
usb_functions are created. And then there are usb_functions
which do the usb stuff proper and can be created in many instances per
each usb_function_driver.
I would call the structure in question "usb_function_module".
Or even better: "usb_function_pool" - you get usb_functions from it.
Please see inline for what it looks like.
> The name of the instance is used to automaticaly load the module if it the
> instance is not yet available.
> The usb_function callbacks are slightly modified and extended:
> - usb_get_function()
> creates a struct usb_function inclunding all pointers (bind,
> unbind,…). It uses the "instance" to map its configuration. So we can
> have _two_ struct usb_function, one for each usb_configuration.
> - ->unbind()
> Since the struct usb_function was not allocated in ->bind() it should
> not kfree()d here. This function should only reverse what happens in
> ->bind() that is request cleanup and the cleanup of allocated
> descriptors.
> - ->free_func()
> a simple kfree() of the struct usb_function
>
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@xxxxxxxxxxxxx>
> ---
> drivers/usb/gadget/Makefile | 2 +-
> drivers/usb/gadget/composite.c | 47 ++++++++++-------
> drivers/usb/gadget/functions.c | 110
> ++++++++++++++++++++++++++++++++++++++++
> include/linux/usb/composite.h | 52 +++++++++++++++++++
> 4 files changed, 193 insertions(+), 18 deletions(-)
> create mode 100644 drivers/usb/gadget/functions.c
>
> diff --git a/drivers/usb/gadget/Makefile b/drivers/usb/gadget/Makefile
> index 8b4acfd..fef41f5 100644
> --- a/drivers/usb/gadget/Makefile
> +++ b/drivers/usb/gadget/Makefile
> @@ -6,7 +6,7 @@ ccflags-$(CONFIG_USB_GADGET_DEBUG) := -DDEBUG
> obj-$(CONFIG_USB_GADGET) += udc-core.o
> obj-$(CONFIG_USB_LIBCOMPOSITE) += libcomposite.o
> libcomposite-y := usbstring.o config.o epautoconf.o
> -libcomposite-y += composite.o
> +libcomposite-y += composite.o functions.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/composite.c
> b/drivers/usb/gadget/composite.c
> index 4eb07c7..3c460ff 100644
> --- a/drivers/usb/gadget/composite.c
> +++ b/drivers/usb/gadget/composite.c
> @@ -664,6 +664,31 @@ static int set_config(struct usb_composite_dev *cdev,
> return result;
> }
>
> +int usb_add_config_only(struct usb_composite_dev *cdev,
> + struct usb_configuration *config)
> +{
> + struct usb_configuration *c;
> +
> + if (!config->bConfigurationValue)
> + return -EINVAL;
> +
> + /* Prevent duplicate configuration identifiers */
> + list_for_each_entry(c, &cdev->configs, list) {
> + if (c->bConfigurationValue == config->bConfigurationValue)
> + return -EBUSY;
> + }
> +
> + config->cdev = cdev;
> + list_add_tail(&config->list, &cdev->configs);
> +
> + INIT_LIST_HEAD(&config->functions);
> + config->next_interface_id = 0;
> + memset(config->interface, 0, sizeof(config->interface));
> +
> + return 0;
> +}
> +EXPORT_SYMBOL_GPL(usb_add_config_only);
> +
> /**
> * usb_add_config() - add a configuration to a device.
> * @cdev: wraps the USB gadget
> @@ -684,30 +709,18 @@ int usb_add_config(struct usb_composite_dev *cdev,
> int (*bind)(struct usb_configuration *))
> {
> int status = -EINVAL;
> - struct usb_configuration *c;
> +
> + if (!bind)
> + goto done;
>
> DBG(cdev, "adding config #%u '%s'/%p\n",
> config->bConfigurationValue,
> config->label, config);
>
> - if (!config->bConfigurationValue || !bind)
> + status = usb_add_config_only(cdev, config);
> + if (status)
> goto done;
>
> - /* Prevent duplicate configuration identifiers */
> - list_for_each_entry(c, &cdev->configs, list) {
> - if (c->bConfigurationValue == config->bConfigurationValue) {
> - status = -EBUSY;
> - goto done;
> - }
> - }
> -
> - config->cdev = cdev;
> - list_add_tail(&config->list, &cdev->configs);
> -
> - INIT_LIST_HEAD(&config->functions);
> - config->next_interface_id = 0;
> - memset(config->interface, 0, sizeof(config->interface));
> -
> status = bind(config);
> if (status < 0) {
> while (!list_empty(&config->functions)) {
> diff --git a/drivers/usb/gadget/functions.c
> b/drivers/usb/gadget/functions.c
> new file mode 100644
> index 0000000..f9cdf8e
> --- /dev/null
> +++ b/drivers/usb/gadget/functions.c
> @@ -0,0 +1,110 @@
> +#include <linux/kernel.h>
> +#include <linux/slab.h>
> +#include <linux/module.h>
> +#include <linux/err.h>
> +
> +#include <linux/usb/composite.h>
> +
> +static LIST_HEAD(func_list);
> +static DEFINE_MUTEX(func_lock);
> +
> +static struct usb_function_instance *try_get_usb_function_instance(const
> char *name)
Consider
+static struct usb_function_pool
+*try_get_usb_function_pool(const char *name) {
and so on...
> +{
> + struct usb_function_driver *fd;
> + struct usb_function_instance *fi;
> +
> + fi = ERR_PTR(-ENOENT);
> + mutex_lock(&func_lock);
> + list_for_each_entry(fd, &func_list, list) {
> +
> + if (strcmp(name, fd->name))
> + continue;
> +
> + if (!try_module_get(fd->mod)) {
> + fi = ERR_PTR(-EBUSY);
> + break;
> + }
> + fi = fd->alloc_inst();
> + if (IS_ERR(fi))
> + module_put(fd->mod);
> + else
> + fi->fd = fd;
> + break;
> + }
> + mutex_unlock(&func_lock);
> + return fi;
> +}
> +
> +struct usb_function_instance *usb_get_function_instance(const char *name)
> +{
> + struct usb_function_instance *fi;
> + int ret;
> +
> + fi = try_get_usb_function_instance(name);
> + if (!IS_ERR(fi))
> + return fi;
> + ret = PTR_ERR(fi);
> + if (ret != -ENOENT)
> + return fi;
> + ret = request_module("usbfunc:%s", name);
> + if (ret < 0)
> + return ERR_PTR(ret);
> + return try_get_usb_function_instance(name);
> +}
> +EXPORT_SYMBOL_GPL(usb_get_function_instance);
> +
> +struct usb_function *usb_get_function(struct usb_function_instance *fi)
... and now this can look a lot nicer:
+struct usb_function *usb_get_function(struct usb_function_pool *fp)
> +{
> + return fi->fd->alloc_func(fi);
> +}
> +EXPORT_SYMBOL_GPL(usb_get_function);
> +
> +void usb_put_function_instance(struct usb_function_instance *fi)
> +{
> + struct module *mod;
> +
> + if (!fi)
> + return;
> +
> + mod = fi->fd->mod;
> + fi->free_func_inst(fi);
> + module_put(mod);
> +}
> +EXPORT_SYMBOL_GPL(usb_put_function_instance);
> +
> +void usb_put_function(struct usb_function *f)
> +{
> + if (!f)
> + return;
> +
> + f->free_func(f);
> +}
> +EXPORT_SYMBOL_GPL(usb_put_function);
> +
> +int usb_function_register(struct usb_function_driver *newf)
> +{
> + struct usb_function_driver *fd;
> + int ret;
> +
> + ret = -EEXIST;
> +
> + mutex_lock(&func_lock);
> + list_for_each_entry(fd, &func_list, list) {
> + if (!strcmp(fd->name, newf->name))
> + goto out;
> + }
> + ret = 0;
> + list_add_tail(&newf->list, &func_list);
> +out:
> + mutex_unlock(&func_lock);
> + return ret;
> +}
> +EXPORT_SYMBOL_GPL(usb_function_register);
> +
> +void usb_function_unregister(struct usb_function_driver *fd)
> +{
> + mutex_lock(&func_lock);
> + list_del(&fd->list);
> + mutex_unlock(&func_lock);
> +}
> +EXPORT_SYMBOL_GPL(usb_function_unregister);
> diff --git a/include/linux/usb/composite.h b/include/linux/usb/composite.h
> index b09c37e..d0886c8 100644
> --- a/include/linux/usb/composite.h
> +++ b/include/linux/usb/composite.h
> @@ -77,6 +77,8 @@ struct usb_configuration;
> * in interface or class descriptors; endpoints; I/O buffers; and so on.
> * @unbind: Reverses @bind; called as a side effect of unregistering the
> * driver which added this function.
> + * @free_func: free the struct usb_function.
> + * @mod: (internal) points to the module that created this structure.
> * @set_alt: (REQUIRED) Reconfigures altsettings; function drivers may
> * initialize usb_ep.driver data at this time (when it is used).
> * Note that setting an interface to its current altsetting resets
> @@ -116,6 +118,7 @@ struct usb_configuration;
> * two or more distinct instances within the same configuration,
> providing
> * several independent logical data links to a USB host.
> */
> +
> struct usb_function {
> const char *name;
> struct usb_gadget_strings **strings;
> @@ -136,6 +139,8 @@ struct usb_function {
> struct usb_function *);
> void (*unbind)(struct usb_configuration *,
> struct usb_function *);
> + void (*free_func)(struct usb_function *f);
> + struct module *mod;
>
> /* runtime state management */
> int (*set_alt)(struct usb_function *,
> @@ -431,6 +436,53 @@ static inline u16 get_default_bcdDevice(void)
> return bcdDevice;
> }
>
> +struct usb_function_driver {
> + const char *name;
> + struct module *mod;
> + struct list_head list;
> + struct usb_function_instance *(*alloc_inst)(void);
> + struct usb_function *(*alloc_func)(struct usb_function_instance
> *inst);
... and then this looks nice too:
+ struct usb_function_pool *(*alloc_pool)(void);
+ struct usb_function *(*alloc_func)(struct usb_function_pool *pool);
> +};
> +
> +struct usb_function_instance {
> + struct usb_function_driver *fd;
> + void (*free_func_inst)(struct usb_function_instance *inst);
> +};
> +
> +void usb_function_unregister(struct usb_function_driver *f);
> +int usb_function_register(struct usb_function_driver *newf);
> +void usb_put_function_instance(struct usb_function_instance *fi);
> +void usb_put_function(struct usb_function *f);
> +struct usb_function_instance *usb_get_function_instance(const char *name);
... and here:
+struct usb_function_pool *usb_get_function_pool(const char *name);
> +struct usb_function *usb_get_function(struct usb_function_instance *fi);
> +
> +struct usb_configuration *usb_get_config(struct usb_composite_dev *cdev,
> + int val);
> +int usb_add_config_only(struct usb_composite_dev *cdev,
> + struct usb_configuration *config);
> +
> +#define DECLARE_USB_FUNCTION(_name, _inst_alloc, _func_alloc) \
> + static struct usb_function_driver _name ## usb_func = { \
> + .name = __stringify(_name), \
> + .mod = THIS_MODULE, \
> + .alloc_inst = _inst_alloc, \
> + .alloc_func = _func_alloc, \
> + }; \
> + MODULE_ALIAS("usbfunc:"__stringify(_name));
> +
> +#define DECLARE_USB_FUNCTION_INIT(_name, _inst_alloc, _func_alloc) \
> + DECLARE_USB_FUNCTION(_name, _inst_alloc, _func_alloc) \
> + static int __init _name ## mod_init(void) \
> + { \
> + return usb_function_register(&_name ## usb_func); \
> + } \
> + static void __exit _name ## mod_exit(void) \
> + { \
> + usb_function_unregister(&_name ## usb_func); \
> + } \
> + module_init(_name ## mod_init); \
> + module_exit(_name ## mod_exit)
> +
I don't understand splitting the macro in two. The latter macro is
to be used in modules and this is what you recommend, and it calls
the first macro. So the first macro should be used in places other
than modules, but the very reason of this patch is to make f_xxxx.c
files modules. Code clarity improvements because of this are
disputable; I find this split rather confusing.
Can you give an example situation where the first macro
must be called without the second macro?
> /* messaging utils */
> #define DBG(d, fmt, args...) \
> dev_dbg(&(d)->gadget->dev , fmt , ## args)
> --
> 1.7.10.4
--
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
