* Felipe Balbi | 2012-11-20 14:13:12 [+0200]:
>Hi,
Hi,
>> 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 ?
This is pulled out of usb_add_config() and we had no locking there.
Right now we don't have multilpe users at the same time and until this
changes we don't need locking.
>> 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 :-)
I started with usb_function and got conflicts :) We can settle on
whatever fits better.
>> +void usb_put_function(struct usb_function *f)
>> +{
>> + struct module *mod;
>> +
>> + if (!f)
>> + return;
>
>I'd add a blank line here, just saying it ;-)
You got it.
>> + mod = f->mod;
>> + f->free_func(f);
>> + module_put(mod);
>> +}
>> +EXPORT_SYMBOL_GPL(usb_put_function);
>> 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 ?
I used this a marker. I would remove it and ad stuff in the block above
where the members are described.
>> + 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 ? :-)
I think the series was checkpatch clean.
>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.
I don't like hidding a struct behind a typedef but here it is just a
function prototype. If something changes and you need to alter the
arguments then you don't have to a bunch of functions.
>> +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 ?
The name is bad, I don't disagree but I did not have anything else.
The semantic as of now:
- it is defined in each function for registration to the system. That
means we would have one for F_ACM, one for F_NCM, â?Š
- the new function.c is using the alloc hook to create a new
usb_function struct. This is mostly the bind callback except we don't
have config to which we add it/this.
- the mod member is used to grab the module in which usb_d_function is
defined. That means as long as there struct usb_function floating
around, you can't remove the module (f_acm.ko for instance).
- You have one usb_d_function from f_acm.ko but you could have >1
usb_function of this instance. This is used in 11/16 where g_serial
can have X f_acm functions where X is defined by the n_ports module
parameter.
>I wonder if you *really* a new structure or could usb_function itself be
>used.
We probably need another one: Lets say you two configs and each config
has one ACM function. Should both configs share the same ttyACM/GS port?
Currently we pass an integer to the bind function which maps to a local
variableâ?Š
>> +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.
We can't have const (I think) because the list member needs write
access.
>> + .name = __stringify(_name), \
>> + .mod = THIS_MODULE, \
>> + .alloc = _alloc, \
>> + }; \
>> + MODULE_ALIAS("usbfunc:"__stringify(_name));
>> +
Sebastian
--
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
