On Wed, Oct 23 2013, Andrzej Pietrasiewicz wrote:
> This is required in order to integrate configfs support.
> f_fs needs to be a separately compiled module and so it needs to use the new
> interface.
>
> Signed-off-by: Andrzej Pietrasiewicz <andrzej.p@xxxxxxxxxxx>
> Signed-off-by: Kyungmin Park <kyungmin.park@xxxxxxxxxxx>
Acked-by: Michal Nazarewicz <mina86@xxxxxxxxxx>
> @@ -2216,8 +2096,70 @@ static int __ffs_func_bind_do_nums(enum ffs_entity_type type, u8 *valuep,
> return 0;
> }
>
> -static int ffs_func_bind(struct usb_configuration *c,
> - struct usb_function *f)
> +static inline void mutex_lock_if_nonnull(struct mutex *mutex)
> +{
> + if (mutex)
> + mutex_lock(mutex);
> +}
> +
> +static inline void mutex_unlock_if_nonnull(struct mutex *mutex)
> +{
> + if (mutex)
> + mutex_unlock(mutex);
> +}
> +
> +#ifndef USB_FFS_INCLUDED
> +static inline struct f_fs_opts *ffs_do_functionfs_bind(struct usb_function *f,
> + struct usb_configuration *c)
> +{
> + struct ffs_function *func = ffs_func_from_usb(f);
> + struct f_fs_opts *ffs_opts =
> + container_of(f->fi, struct f_fs_opts, func_inst);
> + int ret;
> +
> + ENTER();
> +
> + /*
> + * Legacy gadget triggers binding in functionfs_ready_callback,
> + * which already uses locking; taking the same lock here would
> + * cause a deadlock.
> + *
> + * Configfs-enabled gadgets however do need ffs_dev_lock.
> + */
> + mutex_lock_if_nonnull(ffs_dev_lock);
> + if (!ffs_opts->dev->desc_ready) {
> + mutex_unlock_if_nonnull(ffs_dev_lock);
> + return ERR_PTR(-ENODEV);
> + }
> + mutex_unlock_if_nonnull(ffs_dev_lock);
The *_if_nonnull functions are used only here; it almost seems like it's
not worth creating them. Furthermore, the above can be easily rewritten
to avoid two unlock call sites:
if (ffs_dev_lock)
mutex_lock(ffs_dev_lock);
ret = ffs_opts->dev->desc_ready ? : - ENODEV;
if (ffs_dev_lock)
mutex_unlock(ffs_dev_lock);
if (ret)
return ERR_PTR(ret);
> +
> + func->conf = c;
> + func->gadget = c->cdev->gadget;
> +
> + func->ffs = ffs_opts->dev->ffs_data;
> + ffs_data_get(func->ffs);
> +
> + /*
> + * in drivers/usb/gadget/configfs.c:configfs_composite_bind()
> + * configurations are bound in sequence with list_for_each_entry,
> + * in each configuration its functions are bound in sequence
> + * with list_for_each_entry, so we assume no race condition
> + * with regard to ffs_opts->bound access
> + */
> + if (!ffs_opts->refcnt) {
> + ret = functionfs_bind(func->ffs, c->cdev);
> + if (ret)
> + return ERR_PTR(ret);
> + }
> + ffs_opts->refcnt++;
> + func->function.strings = ffs_opts->dev->ffs_data->stringtabs;
> +
> + return ffs_opts;
> +}
> +#endif
> +
> +static int _ffs_func_bind(struct usb_configuration *c,
> + struct usb_function *f)
> {
> struct ffs_function *func = ffs_func_from_usb(f);
> struct ffs_data *ffs = func->ffs;
> +static struct usb_function_instance *ffs_alloc_inst(void)
> +{
> + struct f_fs_opts *opts;
> + struct ffs_dev *dev;
> + int ret = 0;
> +
> + opts = kzalloc(sizeof(*opts), GFP_KERNEL);
> + if (!opts)
> + return ERR_PTR(-ENOMEM);
> +
> + opts->func_inst.free_func_inst = ffs_free_inst;
> + dev = ffs_alloc_dev();
> + if (IS_ERR(dev)) {
> + ret = PTR_ERR(dev);
> + goto error;
> + }
> + opts->dev = dev;
> +
> + mutex_lock(&auto_init_lock);
> + if (auto_init_active && do_auto_init) {
> + kref_init(&auto_init_ref);
> + /*
> + * ffs_set_acquire_dev_cb();
> + * ffs_set_release_dev_cb();
> + * ffs_set_ready_cb();
> + * ffs_set_closed_cb();
> + */
> + /* fails if callbacks not set */
Not sure about purpose of the above comments.
> + ret = functionfs_init(NULL);
> + if (ret) {
> + mutex_unlock(&auto_init_lock);
> + goto error;
> + }
> + do_init_kref = do_auto_init = false;
Instead of doing goto, do:
if (!ret)
do_init_kref = do_auto_init = false;
and then…
> + } else {
> + if (do_init_kref) {
> + kref_init(&auto_init_ref);
> + do_init_kref = false;
> + } else {
> + kref_get(&auto_init_ref);
> + }
> + }
> + mutex_unlock(&auto_init_lock);
> +
> + return &opts->func_inst;
…after the if and mutex_unlock:
if (!ret)
return &opts->func_inst;
> +error:
> + ffs_free_dev(opts->dev);
> + kfree(opts);
> + return ERR_PTR(ret);
> +}
> +
> +static void ffs_free(struct usb_function *f)
> +{
> + struct ffs_function *func = ffs_func_from_usb(f);
> +
> + kfree(func);
Why complicate things:
kfree(ffs_func_from_usb(f));
> +}
> +
> +static void ffs_func_unbind(struct usb_configuration *c,
> + struct usb_function *f)
> +{
> + struct ffs_function *func = ffs_func_from_usb(f);
> + struct ffs_data *ffs = func->ffs;
> + struct f_fs_opts *opts = container_of(f->fi, struct f_fs_opts,
> + func_inst);
I'd prefer:
struct f_fs_opts *opts =
container_of(f->fi, struct f_fs_opts, func_inst);
but whatever floats your boat.
> + struct ffs_ep *ep = func->eps;
> + unsigned count = ffs->eps_count;
> + unsigned long flags;
> +
> + ENTER();
> + if (ffs->func == func) {
> + ffs_func_eps_disable(func);
> + ffs->func = NULL;
> + }
> +
> + if (!--opts->refcnt)
> + functionfs_unbind(ffs);
> +
> + /* cleanup after autoconfig */
> + spin_lock_irqsave(&func->ffs->eps_lock, flags);
> + do {
> + if (ep->ep && ep->req)
> + usb_ep_free_request(ep->ep, ep->req);
> + ep->req = NULL;
> + ++ep;
> + } while (--count);
> + spin_unlock_irqrestore(&func->ffs->eps_lock, flags);
> + kfree(func->eps);
> + func->eps = NULL;
> + /*
> + * eps and interfaces_nums are allocated in the same chunk so
> + * only one free is required. Descriptors are also allocated
> + * in the same chunk.
> + */
/*
* eps, descriptors and interfaces_nums are allocated in the
* same chunk so only one free is required.
*/
> + func->function.fs_descriptors = NULL;
> + func->function.hs_descriptors = NULL;
> + func->interfaces_nums = NULL;
> +
> + ffs_event_add(ffs, FUNCTIONFS_UNBIND);
> +}
> +
> +static struct usb_function *ffs_alloc(struct usb_function_instance *fi)
> +{
> + struct ffs_function *func;
> + struct f_fs_opts *opts;
> +
> + ENTER();
> +
> + opts = container_of(fi, struct f_fs_opts, func_inst);
Never used.
> +
> + func = kzalloc(sizeof(*func), GFP_KERNEL);
> + if (unlikely(!func))
> + return ERR_PTR(-ENOMEM);
> +
> + func->function.name = "Function FS Gadget";
> +
> + func->function.bind = ffs_func_bind;
> + func->function.unbind = ffs_func_unbind;
> + func->function.set_alt = ffs_func_set_alt;
> + func->function.disable = ffs_func_disable;
> + func->function.setup = ffs_func_setup;
> + func->function.suspend = ffs_func_suspend;
> + func->function.resume = ffs_func_resume;
> + func->function.free_func = ffs_free;
> +
> + return &func->function;
> +}
> +
> +#endif
--
Best regards, _ _
.o. | Liege of Serenely Enlightened Majesty of o' \,=./ `o
..o | Computer Science, Michał “mina86” Nazarewicz (o o)
ooo +--<mpn@xxxxxxxxxx>--<xmpp:mina86@xxxxxxxxxx>--ooO--(_)--Ooo--
Attachment:
signature.asc
Description: PGP signature
