Acked-by: Michal Nazarewicz <mina86@xxxxxxxxxx>
Only some small comments.
On Fri, Nov 22 2013, Andrzej Pietrasiewicz wrote:
> @@ -2465,6 +2474,209 @@ static int ffs_func_revmap_intf(struct ffs_function *func, u8 intf)
> }
>
>
> +/* Devices management *******************************************************/
> +
> +static LIST_HEAD(ffs_devices);
> +
> +void ffs_dev_lock(void)
> +{
> + mutex_lock(&ffs_lock);
> +}
> +
> +void ffs_dev_unlock(void)
> +{
> + mutex_unlock(&ffs_lock);
> +}
Perhaps it will be more efficient to have those as static inlines and
extern ffs_lock in header file instead?
> +
> +static struct ffs_dev *_ffs_find_dev(const char *name)
> +{
> + struct ffs_dev *dev;
> +
> + list_for_each_entry(dev, &ffs_devices, entry) {
> + if (!dev->name || !name)
> + continue;
> + if (strcmp(dev->name, name) == 0)
> + return dev;
> + }
> +
> + return NULL;
> +}
> +
> +/*
> + * ffs_lock must be taken by the caller of this function
> + */
> +static struct ffs_dev *ffs_find_dev(const char *name)
> +{
> + struct ffs_dev *dev;
> +
> + if (list_is_singular(&ffs_devices)) {
> + dev = list_first_entry(&ffs_devices, struct ffs_dev, entry);
> + if (dev->single)
> + return dev;
Wouldn't it be easier to do this check in _ffs_find_dev? It is possible
to have non-singular list with some devs having dev->single set?
I don't think that it is since ffs_alloc_dev checks whether list is
singular with the only element having dev->single.
> + }
> +
> + return _ffs_find_dev(name);
> +}
> +
> +/*
> + * ffs_lock must be taken by the caller of this function
> + */
> +struct ffs_dev *ffs_alloc_dev(void)
> +{
> + struct ffs_dev *dev;
> + int ret;
> +
> + if (list_is_singular(&ffs_devices)) {
> + dev = list_first_entry(&ffs_devices, struct ffs_dev, entry);
> + if (dev->single)
> + return ERR_PTR(-EBUSY);
> + }
This condition is used for the second time, perhaps it would be better
to have a function:
static struct ffs_dev *ffs_get_single_dev(void)
{
if (list_is_singular(&ffs_devices)) {
dev = list_first_entry(&ffs_devices, struct ffs_dev, entry);
if (dev->single)
return dev;
}
return NULL;
}
Then, ffs_find_dev would do:
dev = ffs_get_single_dev();
if (dev)
return dev;
And ffs_alloc_dev would do:
if (ffs_get_single_dev())
return ERR_PTR(-EBUSY);
This of course is mutually exclusive with my previous comment. Also,
it's up to you, I don't have strong opinions.
> +
> + dev = kzalloc(sizeof(*dev), GFP_KERNEL);
> + if (!dev)
> + return ERR_PTR(-ENOMEM);
> +
> + if (list_empty(&ffs_devices)) {
> + ret = functionfs_init();
> + if (ret) {
> + kfree(dev);
> + return ERR_PTR(ret);
> + }
> + }
> +
> + list_add(&dev->entry, &ffs_devices);
> +
> + return dev;
> +}
> +
> +/*
> + * ffs_lock must be taken by the caller of this function
> + * The caller is responsible for "name" being available whenever f_fs needs it
> + */
> +int ffs_name_dev(struct ffs_dev *dev, const char *name)
> +{
> + struct ffs_dev *existing;
> +
> + existing = _ffs_find_dev(name);
> + if (existing)
> + return -EBUSY;
> +
> + dev->name = name;
> +
> + return 0;
> +}
> +
> +/*
> + * ffs_lock must be taken by the caller of this function
> + */
> +int ffs_single_dev(struct ffs_dev *dev, bool single)
> +{
> + if (single && !list_is_singular(&ffs_devices))
> + return -EBUSY;
> +
> + dev->single = single;
> +
> + return 0;
> +}
> +
> +/*
> + * ffs_lock must be taken by the caller of this function
> + */
> +void ffs_free_dev(struct ffs_dev *dev)
> +{
> + list_del(&dev->entry);
> + if (list_empty(&ffs_devices))
> + functionfs_cleanup();
> +
> + kfree(dev);
I'd free before functionfs_cleanup, but up to you.
> +}
> +
> +static void *ffs_acquire_dev(const char *dev_name)
> +{
> + struct ffs_dev *ffs_dev;
> +
> + ENTER();
> + ffs_dev_lock();
> +
> + ffs_dev = ffs_find_dev(dev_name);
> + if (!ffs_dev) {
> + ffs_dev = ERR_PTR(-ENODEV);
> + goto done;
> + }
> +
> + if (ffs_dev->mounted) {
> + ffs_dev = ERR_PTR(-EBUSY);
> + goto done;
> + }
> + ffs_dev->mounted = true;
This could be rewritten to avoid goto and in fact shorten the code:
if (!ffs_dev)
ffs_dev = ERR_PTR(-ENODEV);
else if (ffs_dev->mounted)
ffs_dev = ERR_PTR(-EBUSY);
else
ffs_dev->mounted = true;
> +
> +done:
> + ffs_dev_unlock();
> + return ffs_dev;
> +}
> /* Misc helper functions ****************************************************/
>
> static int ffs_mutex_lock(struct mutex *mutex, unsigned nonblock)
> diff --git a/drivers/usb/gadget/g_ffs.c b/drivers/usb/gadget/g_ffs.c
> index 1aaa103..d9d1e66 100644
> --- a/drivers/usb/gadget/g_ffs.c
> +++ b/drivers/usb/gadget/g_ffs.c
> @@ -157,6 +157,8 @@ struct gfs_configuration {
> #endif
> };
>
> +static int functionfs_ready_callback(struct ffs_data *ffs);
> +static void functionfs_closed_callback(struct ffs_data *ffs);
> static int gfs_bind(struct usb_composite_dev *cdev);
> static int gfs_unbind(struct usb_composite_dev *cdev);
> static int gfs_do_config(struct usb_configuration *c);
> @@ -170,172 +172,113 @@ static __refdata struct usb_composite_driver gfs_driver = {
> .unbind = gfs_unbind,
> };
>
> -static DEFINE_MUTEX(gfs_lock);
> static unsigned int missing_funcs;
> static bool gfs_registered;
> static bool gfs_single_func;
> -static struct ffs_dev *ffs_tab;
> +static struct ffs_dev **ffs_tab;
>
> static int __init gfs_init(void)
> {
> int i;
> + int ret = 0;
>
> ENTER();
>
> - if (!func_num) {
> + if (func_num < 2) {
> gfs_single_func = true;
> func_num = 1;
> }
>
> - ffs_tab = kcalloc(func_num, sizeof *ffs_tab, GFP_KERNEL);
> + ffs_tab = kcalloc(func_num, sizeof(*ffs_tab), GFP_KERNEL);
> if (!ffs_tab)
> return -ENOMEM;
>
> - if (!gfs_single_func)
> - for (i = 0; i < func_num; i++)
> - ffs_tab[i].name = func_names[i];
> + ffs_dev_lock();
> + for (i = 0; i < func_num; i++) {
> + ffs_tab[i] = ffs_alloc_dev();
> + if (IS_ERR(ffs_tab[i])) {
> + ret = PTR_ERR(ffs_tab[i--]);
I'd put the decrement in a separate instruction to be more obvious.
> + goto no_dev;
> + }
> + ret = ffs_single_dev(ffs_tab[i], gfs_single_func);
> + if (ret)
> + goto no_dev;
> + if (!gfs_single_func) {
> + ret = ffs_name_dev(ffs_tab[i], func_names[i]);
> + if (ret)
> + goto no_dev;
> + }
> + ffs_tab[i]->ffs_ready_callback = functionfs_ready_callback;
> + ffs_tab[i]->ffs_closed_callback = functionfs_closed_callback;
> + }
> + ffs_dev_unlock();
>
> missing_funcs = func_num;
>
> - return functionfs_init();
> + return 0;
> +no_dev:
> + while (i >= 0)
> + ffs_free_dev(ffs_tab[i--]);
> + ffs_dev_unlock();
> + kfree(ffs_tab);
> + return ret;
> }
> module_init(gfs_init);
--
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
