On Wed, Nov 07 2012, Sebastian Andrzej Siewior wrote:
> This patch provides an infrastructure to register & unregister an 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 functions defines the DECLARE_USB_FUNCTION 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 some minor changes:
> - a generic configuration struct is attached
> This struct describes all argumens which can be passed to the
> function. It includes basically two entries: The name of the argument
> and its value. With this piece we will have a "generic" query of all
> arguments which can be set from the outside. This should be also handy
> for configfs because it can query each function for its options and
> expose to the user, that needs to do the thinking. This can be used
> the set the MAC address for one of the network gadgets (which is a
> string) or the number of current serial ports for the ACM function
> (which is an interger).
The code looks promising, and definitely a step in right direction, but
I don't think this configuration structure is good solution for
ConfigFS.
Sure, many (most) functions have same static unstructured configuration
arguments which can be expressed in a flat array, but if you look at
mass storage, this is not really a good solution.
At the moment, mass storage is using a static and flat list of arguments
because it uses module parameters, but the “file” argument is very
ugly... It accepts a list of file names to use for each LUN.
With ConfigFS I believe we can do much better than to use inflexible
list of arguments. In particular, the way I would see mass storage
function be configured from ConfigFS is:
$ cd /path/to/mass/storage/function/configfs/directory
$ ls
nluns
$ echo 2 >nluns
$ ls
nluns lun0 lun1
$ echo 0 >lun0/ro
$ echo 0 >lun0/cdrom
$ echo 0 >lun0/nofua
$ echo 0 >lun0/removable
$ echo /path/to/some/file >lun0/file
$ echo 1 >lun0/ro
$ echo 1 >lun0/cdrom
$ echo 0 >lun0/nofua
$ echo 1 >lun0/removable
> - a generic function to configure the function
> This passes the struct and sets the arguments. The "old" gadgets would
> most likely set all values at once but the "new configfs" interface
> would prefer to set one value at a time that once the use did
> "echo value > option"
> kind of thing.
> - a function de-allocate function
> This is mostly the same thing that is done by the unbind function.
>
> composite gains usb_add_config_only() which a subset of
> usb_add_config(). The bind/attaching can be deferred to a later point in
> time.
>
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@xxxxxxxxxxxxx>
> ---
> drivers/usb/gadget/Makefile | 2 +-
> drivers/usb/gadget/composite.c | 53 +++++++++++++---------
> drivers/usb/gadget/functions.c | 97 ++++++++++++++++++++++++++++++++++++++++
> include/linux/usb/composite.h | 68 ++++++++++++++++++++++++++++
> 4 files changed, 199 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;
> + }
> +
> + 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..79f9472f3
> --- /dev/null
> +++ b/drivers/usb/gadget/functions.c
> @@ -0,0 +1,97 @@
> +#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;
> + struct usb_function *uf;
> +
> + mutex_lock(&func_lock);
> + list_for_each_entry(f, &func_list, list) {
> + if (!strcmp(name, f->name)) {
Alternatively
+ if (strcmp(name, f->name))
+ continue;
which will make the rest one indention less, but feel free to stick to
your preference here.
> + bool ok;
> + ok = try_module_get(f->mod);
> + uf = ERR_PTR(-EBUSY);
> + if (!ok)
> + goto out;
This temporary variable looks strange to me. How about:
+ if (!try_module_get(f->mod)) {
+ uf = ERR_PTR(-EBUSY);
+ goto out;
+ }
> + uf = f->alloc();
> + if (uf) {
> + uf->mod = f->mod;
> + } else {
> + module_put(f->mod);
> + uf = ERR_PTR(-ENOMEM);
> + }
> + mutex_unlock(&func_lock);
> + return uf;
“goto out;” instead of those two lines perhaps? This will keep a single
mutex_unlock() location.
> + }
> + }
> + uf = ERR_PTR(-ENOENT);
> +out:
Actually, if you initialise uf with ERR_PTR(-ENOENT) at the beginning,
and apply my change with the “bool ok” variable, you can get rid of the
out label and use “break” instead of “goto”.
> + 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);
> +
> +void usb_put_function(struct usb_function *f)
> +{
> + struct module *mod;
> +
> + if (!f)
> + return;
> + 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..ced2910 100644
> --- a/include/linux/usb/composite.h
> +++ b/include/linux/usb/composite.h
> @@ -116,6 +116,22 @@ struct usb_configuration;
> * two or more distinct instances within the same configuration, providing
> * several independent logical data links to a USB host.
> */
> +
> +enum USBF_OPTION_TYPE {
> + USBF_OPTION_INVALID,
> + USBF_OPTION_STRING,
> + USBF_OPTION_INT,
> +};
> +
> +struct usbf_option {
> + enum USBF_OPTION_TYPE type;
> + char *name;
> + union {
> + char *o_charp;
> + int o_int;
> + } val;
> +};
> +
> struct usb_function {
> const char *name;
> struct usb_gadget_strings **strings;
> @@ -136,6 +152,13 @@ struct usb_function {
> struct usb_function *);
> void (*unbind)(struct usb_configuration *,
> struct usb_function *);
> + /* */
> + void (*free_func)(struct usb_function *f);
> + const struct usbf_option *avail_options;
> + unsigned avail_options_num;
Why not make the avail_options array be terminated with a NULL-named
option or something like that? I think that's simpler than having to
specify the number of arguments explicitly.
> + int (*configure)(struct usb_function *f,
> + struct usbf_option *options, int num);
> + struct module *mod;
>
> /* runtime state management */
> int (*set_alt)(struct usb_function *,
> @@ -158,6 +181,14 @@ struct usb_function {
> DECLARE_BITMAP(endpoints, 32);
> };
>
> +static inline int _usbf_configure(struct usb_function *f,
> + struct usbf_option *options, int num)
> +{
> + return f->configure(f, options, num);
> +}
> +
> +#define usbf_configure(f, o) _usbf_configure(f, o, ARRAY_SIZE(o))
> +
> int usb_add_function(struct usb_configuration *, struct usb_function *);
>
> int usb_function_deactivate(struct usb_function *);
> @@ -431,6 +462,43 @@ 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);
> +
> +struct usb_d_function {
> + const char *name;
> + struct module *mod;
> + struct list_head list;
> + usb_func_alloc alloc;
> +};
> +
> +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 = { \
> + .name = __stringify(_name), \
> + .mod = THIS_MODULE, \
> + .alloc = _alloc, \
> + }; \
> + MODULE_ALIAS("usbfunc:"__stringify(_name)); \
> + 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)
> --
> 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
--
Best regards, _ _
.o. | Liege of Serenely Enlightened Majesty of o' \,=./ `o
..o | Computer Science, Michał “mina86” Nazarewicz (o o)
ooo +----<email/xmpp: mpn@xxxxxxxxxx>--------------ooO--(_)--Ooo--Attachment:
pgphdcV0oga4T.pgp
Description: PGP signature
