From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S935622AbcI3B4u (ORCPT ); Thu, 29 Sep 2016 21:56:50 -0400 Received: from szxga03-in.huawei.com ([119.145.14.66]:22226 "EHLO szxga03-in.huawei.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S935359AbcI3B4l (ORCPT ); Thu, 29 Sep 2016 21:56:41 -0400 Subject: Re: BUG: scheduling while atomic in f_fs when gadget remove driver To: Michal Nazarewicz , Felipe Balbi , References: <205cfce1-d54c-262d-f939-ad9f37b0c52c@huawei.com> <878tud4q6x.fsf@linux.intel.com> <261ada71-8a5d-6e89-7fac-6b6ba88218d7@huawei.com> CC: , , , John Stultz , "Amit Pundir" , Guodong Xu From: Chen Yu Message-ID: <38fb334f-20de-f82a-608b-c503bfcd1682@huawei.com> Date: Fri, 30 Sep 2016 09:49:13 +0800 User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:45.0) Gecko/20100101 Thunderbird/45.2.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 8bit X-Originating-IP: [10.142.63.192] X-CFilter-Loop: Reflected Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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 > 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 > Signed-off-by: Michał Nazarewicz > 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. >