Hi,
On Wed, Nov 14, 2012 at 06:14:55PM +0100, Sebastian Andrzej Siewior wrote:
> 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 an allocation function. The name is used for
> automaticaly loading the module if it is not yet present and request the
> function from the gadget because we don't include the functions anymore.
> The allocate function is mostly the "old" bind-callback which was passed
> to usb_add_config() with a minor change:
> - a function de-allocate function
> This is mostly the same thing that is done by the unbind function. It
> is called from within the function on "free" instead from the unbind
> path on gadget removal.
>
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@xxxxxxxxxxxxx>
> ---
> drivers/usb/gadget/Makefile | 2 +-
> drivers/usb/gadget/composite.c | 53 +++++++++++++---------
> drivers/usb/gadget/functions.c | 96 ++++++++++++++++++++++++++++++++++++++++
> include/linux/usb/composite.h | 44 ++++++++++++++++++
> 4 files changed, 174 insertions(+), 21 deletions(-)
> create mode 100644 drivers/usb/gadget/functions.c
>
> diff --git a/drivers/usb/gadget/Makefile b/drivers/usb/gadget/Makefile
> index 307be5f..fa65050 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..c46ae24 100644
> --- a/drivers/usb/gadget/composite.c
> +++ b/drivers/usb/gadget/composite.c
> @@ -664,6 +664,35 @@ 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;
> +
> + DBG(cdev, "adding config #%u '%s'/%p\n",
> + config->bConfigurationValue,
> + config->label, config);
> +
> + if (!config->bConfigurationValue)
> + return -EINVAL;
> +
> + /* Prevent duplicate configuration identifiers */
> + list_for_each_entry(c, &cdev->configs, list) {
> + if (c->bConfigurationValue == config->bConfigurationValue)
> + return -EBUSY;
> + }
no locking while traversing the list ?
> + 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,29 +713,13 @@ int usb_add_config(struct usb_composite_dev *cdev,
> int (*bind)(struct usb_configuration *))
> {
> int status = -EINVAL;
> - struct usb_configuration *c;
> -
> - DBG(cdev, "adding config #%u '%s'/%p\n",
> - config->bConfigurationValue,
> - config->label, config);
>
> - if (!config->bConfigurationValue || !bind)
> + if (!bind)
> 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 = usb_add_config_only(cdev, config);
> + if (status)
> + goto done;
>
> status = bind(config);
> if (status < 0) {
> diff --git a/drivers/usb/gadget/functions.c b/drivers/usb/gadget/functions.c
> new file mode 100644
> index 0000000..5e5fee5
> --- /dev/null
> +++ b/drivers/usb/gadget/functions.c
> @@ -0,0 +1,96 @@
> +#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 *try_get_usb_function(const char *name)
> +{
> + struct usb_d_function *f;
this usb_d_function stuff still looks a bit 'peculiar' to me :-)
> + struct usb_function *uf;
> +
> + uf = ERR_PTR(-ENOENT);
> + mutex_lock(&func_lock);
> + list_for_each_entry(f, &func_list, list) {
> +
> + if (strcmp(name, f->name))
> + continue;
> +
> + if (!try_module_get(f->mod)) {
> + uf = ERR_PTR(-EBUSY);
> + break;
> + }
> + uf = f->alloc();
> + if (uf) {
> + uf->mod = f->mod;
> + } else {
> + module_put(f->mod);
> + uf = ERR_PTR(-ENOMEM);
> + }
> + break;
> + }
> + mutex_unlock(&func_lock);
> + return uf;
> +}
> +
> +struct usb_function *usb_get_function(const char *name)
> +{
> + struct usb_function *uf;
> + int ret;
> +
> + uf = try_get_usb_function(name);
> + if (!IS_ERR(uf))
> + return uf;
> + ret = PTR_ERR(uf);
> + if (ret != -ENOENT)
> + return uf;
> + ret = request_module("usbfunc:%s", name);
> + if (ret < 0)
> + return ERR_PTR(ret);
> + return try_get_usb_function(name);
> +}
> +EXPORT_SYMBOL_GPL(usb_get_function);
this is neat :-)
> +void usb_put_function(struct usb_function *f)
> +{
> + struct module *mod;
> +
> + if (!f)
> + return;
I'd add a blank line here, just saying it ;-)
> + mod = f->mod;
> + f->free_func(f);
> + module_put(mod);
> +}
> +EXPORT_SYMBOL_GPL(usb_put_function);
> +
> +int usb_function_register(struct usb_d_function *newf)
> +{
> + struct usb_d_function *f;
> + int ret;
> +
> + ret = -EEXIST;
> +
> + mutex_lock(&func_lock);
> + list_for_each_entry(f, &func_list, list) {
> + if (!strcmp(f->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_d_function *f)
> +{
> + mutex_lock(&func_lock);
> + list_del(&f->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..0e2745e 100644
> --- a/include/linux/usb/composite.h
> +++ b/include/linux/usb/composite.h
> @@ -116,6 +116,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 +137,9 @@ struct usb_function {
> struct usb_function *);
> void (*unbind)(struct usb_configuration *,
> struct usb_function *);
> + /* */
-ENOCOMMENT ?
> + void (*free_func)(struct usb_function *f);
> + struct module *mod;
>
> /* runtime state management */
> int (*set_alt)(struct usb_function *,
> @@ -431,6 +435,46 @@ static inline u16 get_default_bcdDevice(void)
> return bcdDevice;
> }
>
> +typedef struct usb_function *(*usb_func_alloc)(void);
> +typedef void (*usb_func_free)(struct usb_function *f);
isn't checkpatch.pl crying about this ? :-)
In any case, if you really want to typedef, then most other type
definitions in the kernel will append _t. irqreturn_t, irq_handler_t,
acc_t, async_cookie_t, sector_t, region_t and so on.
> +struct usb_d_function {
> + const char *name;
> + struct module *mod;
> + struct list_head list;
> + usb_func_alloc alloc;
> +};
this is killing me and I really don't know why. I guess it just looks
weird and error prone. usb_function and usb_d_function are pretty close
to each other. Also without proper documentation it's quite unclear what
usb_d_function is doing. It doesn't contain a usb_function so it's not a
wrapper structure. What are the semantics you have for this ?
I wonder if you *really* a new structure or could usb_function itself be
used.
> +void usb_function_unregister(struct usb_d_function *f);
> +int usb_function_register(struct usb_d_function *newf);
> +void usb_put_function(struct usb_function *f);
> +struct usb_function *usb_get_function(const char *name);
> +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, _alloc) \
> + static struct usb_d_function _name ## usb_func = { \
I would require uses to add the static *and* const modifiers, no strong
feelings however.
> + .name = __stringify(_name), \
> + .mod = THIS_MODULE, \
> + .alloc = _alloc, \
> + }; \
> + MODULE_ALIAS("usbfunc:"__stringify(_name));
> +
> +#define DECLARE_USB_FUNCTION_INIT(_name, _alloc) \
> + DECLARE_USB_FUNCTION(_name, _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)
> +
> /* messaging utils */
> #define DBG(d, fmt, args...) \
> dev_dbg(&(d)->gadget->dev , fmt , ## args)
--
balbi
Attachment:
signature.asc
Description: Digital signature
