Hi Michal,
Thanks for the patch.
在 2016/9/29 5:38, Michal Nazarewicz 写道:
> On Wed, Sep 28 2016, Michal Nazarewicz wrote:
>> With that done, the only thing which needs a mutex is
>> epfile->read_buffer.
>
> Perhaps this would do:
>
I tested the patch on Hikey board with adb function on android, it does fix the
problem.
thanks
Chen Yu
> ---- >8 -------------------------------------------------- -------------
>>From 6416a1065203a39328311f6c58083089efe169aa Mon Sep 17 00:00:00 2001
> From: Michal Nazarewicz <min...@mina86.com>
> Date: Wed, 28 Sep 2016 23:36:56 +0200
> Subject: [RFC] usb: gadget: f_fs: stop sleeping in ffs_func_eps_disable
> MIME-Version: 1.0
> Content-Type: text/plain; charset=UTF-8
> Content-Transfer-Encoding: 8bit
>
> ffs_func_eps_disable is called from atomic context so it cannot sleep
> thus cannot grab a mutex. Change the handling of epfile->read_buffer
> to use non-sleeping synchronisation method.
>
> Reported-by: Chen Yu <cheny...@huawei.com>
> Signed-off-by: Michał Nazarewicz <min...@mina86.com>
> Fixes: 9353afbbfa7b ("buffer data from ‘oversized’ OUT requests")
> ---
> drivers/usb/gadget/function/f_fs.c | 89
> +++++++++++++++++++++++++++++++-------
> 1 file changed, 73 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/usb/gadget/function/f_fs.c
> b/drivers/usb/gadget/function/f_fs.c
> index 759f5d4..8db53da 100644
> --- a/drivers/usb/gadget/function/f_fs.c
> +++ b/drivers/usb/gadget/function/f_fs.c
> @@ -136,8 +136,50 @@ struct ffs_epfile {
> /*
> * Buffer for holding data from partial reads which may happen since
> * we’re rounding user read requests to a multiple of a max packet size.
> + *
> + * The pointer starts with NULL value and may be initialised to other
> + * value by __ffs_epfile_read_data function which may need to allocate
> + * the temporary buffer.
> + *
> + * In normal operation, subsequent calls to __ffs_epfile_read_buffered
> + * will consume data from the buffer and eventually free it.
> + * Importantly, while the function is using the buffer, it sets the
> + * pointer to NULL. This is all right since __ffs_epfile_read_data and
> + * __ffs_epfile_read_buffered can never run concurrently (as they are
> + * protected by epfile->mutex) so the latter will not assign a new value
> + * to the buffer.
> + *
> + * Meanwhile __ffs_func_eps_disable frees the buffer (if the pointer is
> + * valid) and sets the pointer to READ_BUFFER_DROP value. This special
> + * value is crux of the synchronisation between __ffs_func_eps_disable
> + * and __ffs_epfile_read_data.
> + *
> + * Once __ffs_epfile_read_data is about to finish it will try to set the
> + * pointer back to its old value (as described above), but seeing as the
> + * pointer is not-NULL (namely READ_BUFFER_DROP) it will instead free
> + * the buffer.
> + *
> + * This how concurrent calls to the two functions would look like (‘<->’
> + * denotes xchg operation):
> + *
> + * read_buffer = some buffer
> + *
> + * THREAD A THREAD B
> + * __ffs_epfile_read_data:
> + * buf = NULL
> + * buf <-> read_buffer
> + * … do stuff on buf …
> + * __ffs_func_eps_disable:
> + * buf = READ_BUFFER_DROP
> + * buf <-> read_buffer
> + * kfree(buf);
> + *
> + * old = cmpxchg(read_buffer, NULL, buf)
> + * if (old)
> + * kfree(buf)
> */
> - struct ffs_buffer *read_buffer; /* P: epfile->mutex */
> + struct ffs_buffer *read_buffer;
> +#define READ_BUFFER_DROP ((struct ffs_buffer *)ERR_PTR(-ESHUTDOWN))
>
> char name[5];
>
> @@ -740,21 +782,31 @@ static void ffs_epfile_async_io_complete(struct usb_ep
> *_ep,
> static ssize_t __ffs_epfile_read_buffered(struct ffs_epfile *epfile,
> struct iov_iter *iter)
> {
> - struct ffs_buffer *buf = epfile->read_buffer;
> + /*
> + * Null out epfile->read_buffer so ffs_func_eps_disable does not free
> + * the buffer while we are using it.
> + */
> + struct ffs_buffer *buf = xchg(&epfile->read_buffer, NULL);
> ssize_t ret;
> - if (!buf)
> + if (!buf || buf == READ_BUFFER_DROP)
> return 0;
>
> ret = copy_to_iter(buf->data, buf->length, iter);
> if (buf->length == ret) {
> kfree(buf);
> - epfile->read_buffer = NULL;
> - } else if (unlikely(iov_iter_count(iter))) {
> + return ret;
> + }
> +
> + if (unlikely(iov_iter_count(iter))) {
> ret = -EFAULT;
> } else {
> buf->length -= ret;
> buf->data += ret;
> }
> +
> + if (cmpxchg(&epfile->read_buffer, NULL, buf))
> + kfree(buf);
> +
> return ret;
> }
>
> @@ -783,7 +835,10 @@ static ssize_t __ffs_epfile_read_data(struct ffs_epfile
> *epfile,
> buf->length = data_len;
> buf->data = buf->storage;
> memcpy(buf->storage, data + ret, data_len);
> - epfile->read_buffer = buf;
> +
> + buf = xchg(&epfile->read_buffer, buf);
> + if (buf && buf != READ_BUFFER_DROP)
> + kfree(buf);
>
> return ret;
> }
> @@ -1094,11 +1149,14 @@ static int
> ffs_epfile_release(struct inode *inode, struct file *file)
> {
> struct ffs_epfile *epfile = inode->i_private;
> + struct ffs_buffer *buf;
>
> ENTER();
>
> - kfree(epfile->read_buffer);
> - epfile->read_buffer = NULL;
> + buf = xchg(&epfile->read_buffer, NULL);
> + if (buf && buf != READ_BUFFER_DROP)
> + kfree(buf);
> +
> ffs_data_closed(epfile->ffs);
>
> return 0;
> @@ -1721,27 +1779,26 @@ static void ffs_func_eps_disable(struct ffs_function
> *func)
> {
> struct ffs_ep *ep = func->eps;
> struct ffs_epfile *epfile = func->ffs->epfiles;
> + struct ffs_buffer *buf;
> unsigned count = func->ffs->eps_count;
> unsigned long flags;
>
> + spin_lock_irqsave(&func->ffs->eps_lock, flags);
> do {
> - spin_lock_irqsave(&func->ffs->eps_lock, flags);
> /* pending requests get nuked */
> if (likely(ep->ep))
> usb_ep_disable(ep->ep);
> ++ep;
> - if (epfile)
> - epfile->ep = NULL;
> - spin_unlock_irqrestore(&func->ffs->eps_lock, flags);
>
> if (epfile) {
> - mutex_lock(&epfile->mutex);
> - kfree(epfile->read_buffer);
> - epfile->read_buffer = NULL;
> - mutex_unlock(&epfile->mutex);
> + epfile->ep = NULL;
> + buf = xchg(&epfile->read_buffer, READ_BUFFER_DROP);
> + if (buf && buf != READ_BUFFER_DROP)
> + kfree(buf);
> ++epfile;
> }
> } while (--count);
> + spin_unlock_irqrestore(&func->ffs->eps_lock, flags);
> }
>
> static int ffs_func_eps_enable(struct ffs_function *func)
> ---- >8 -------------------------------------------------- -------------
>
> Note: This has not been tested in *any* way. It’s more to demonstrate
> the concept even though it is likely that it does actually work.
>
