Re: [PATCH v4 14/16] usb/gadget: g_ffs: convert to new interface of f_fs

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Mon, Dec 02 2013, Andrzej Pietrasiewicz wrote:
> -	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;
> +		/*
> +		 * usb_get_function_instance() takes ffs_lock,
> +		 * so we must not take it here...
> +		 */
> +		fi_ffs[i] = usb_get_function_instance("ffs");

Technically, usb_get_function_instance doesn't take ffs_lock, but
perhaps some function called from it does.  Could you explain in the
comment what *actually* takes the lock and how it is called from
usb_get_function_instance?

> +		if (IS_ERR(fi_ffs[i])) {
> +			ret = PTR_ERR(fi_ffs[i]);
>  			goto no_dev;
>  		}
> -		ret = ffs_single_dev(ffs_tab[i], gfs_single_func);
> +		/*
> +		 * ... but we need it below
> +		 *
> +		 * What happens if some thread of execution kicks in between
> +		 * usb_get_function_instance() above and ffs_dev_lock() below
> +		 * and (after taking the lock) accesses the devices list?
> +		 *
> +		 * The other thread can be a configfs-based gadget only,
> +		 * since we cannot load this (g_ffs) module twice.
> +		 *
> +		 * For the device to be used it needs so be acquired first.
> +		 * ffs_acquire_dev() finds the device by name and it will get
> +		 * some string to match against as a device name given for
> +		 * mount by the user. So devices which don't have their name set
> +		 * will never be matched, so our device just created above will
> +		 * not be abused.
> +		 *
> +		 * There is an exception to the matching rule: the g_ffs, if
> +		 * no functions= parameter is given on module load, will assume
> +		 * there is only one "device" and will accept any "device" name
> +		 * given for mount. If this is the case here, then the other
> +		 * thread potentially creates another device and the device list
> +		 * consists no more of a single device. Then setting the device
> +		 * created above as "single" will not succeed and g_ffs will
> +		 * refuse loading with EBUSY.
> +		 */
> +		ffs_dev_lock();

I'm wondering whether it would be feasible to change
usb_get_function_instance's path so that ffs_lock is not taken.  This
would save us this long comment… ;)

> +		opts = to_f_fs_opts(fi_ffs[i]);
> +		ret = ffs_single_dev(opts->dev, gfs_single_func);
>  		if (ret)
> -			goto no_dev;
> +			goto no_dev_unlock;
>  		if (!gfs_single_func) {
> -			ret = ffs_name_dev(ffs_tab[i], func_names[i]);
> +			ret = ffs_name_dev(opts->dev, func_names[i]);
>  			if (ret)
> -				goto no_dev;
> +				goto no_dev_unlock;
>  		}
> -		ffs_tab[i]->ffs_ready_callback = functionfs_ready_callback;
> -		ffs_tab[i]->ffs_closed_callback = functionfs_closed_callback;
> +		opts->dev->ffs_ready_callback = functionfs_ready_callback;
> +		opts->dev->ffs_closed_callback = functionfs_closed_callback;
> +		opts->dev->ffs_acquire_dev_callback = functionfs_acquire_dev;
> +		opts->dev->ffs_release_dev_callback = functionfs_release_dev;
> +		opts->no_configfs = true;
> +		ffs_dev_unlock();
>  	}
> -	ffs_dev_unlock();

-- 
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


[Index of Archives]     [Linux Media]     [Linux Input]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [Old Linux USB Devel Archive]

  Powered by Linux