From: Michal Nazarewicz <mina86@mina86.com>
To: Chen Yu <chenyu56@huawei.com>,
Felipe Balbi <felipe.balbi@linux.intel.com>,
gregkh@linuxfoundation.org
Cc: wangbinghui@hisilicon.com, linux-kernel@vger.kernel.org,
linux-usb@vger.kernel.org, John Stultz <john.stultz@linaro.org>,
Amit Pundir <amit.pundir@linaro.org>,
Guodong Xu <guodong.xu@linaro.org>
Subject: Re: BUG: scheduling while atomic in f_fs when gadget remove driver
Date: Wed, 28 Sep 2016 23:38:56 +0200 [thread overview]
Message-ID: <xa1ty42bu2kv.fsf@mina86.com> (raw)
In-Reply-To: <xa1t4m50ugsk.fsf@mina86.com>
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:
---- >8 -------------------------------------------------- -------------
>From 6416a1065203a39328311f6c58083089efe169aa Mon Sep 17 00:00:00 2001
From: Michal Nazarewicz <mina86@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 <chenyu56@huawei.com>
Signed-off-by: Michał Nazarewicz <mina86@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.
--
Best regards
ミハウ “𝓶𝓲𝓷𝓪86” ナザレヴイツ
«If at first you don’t succeed, give up skydiving»
next prev parent reply other threads:[~2016-09-28 21:39 UTC|newest]
Thread overview: 14+ messages / expand[flat|nested] mbox.gz Atom feed top
2016-09-27 9:44 BUG: scheduling while atomic in f_fs when gadget remove driver Chen Yu
2016-09-27 10:01 ` Felipe Balbi
2016-09-28 9:47 ` Chen Yu
2016-09-28 16:31 ` Michal Nazarewicz
2016-09-28 21:38 ` Michal Nazarewicz [this message]
2016-09-30 1:49 ` Chen Yu
2016-10-03 19:19 ` John Stultz
2016-10-03 20:07 ` Michal Nazarewicz
2016-10-03 20:16 ` John Stultz
2016-10-03 23:36 ` John Stultz
2016-10-04 0:07 ` [PATCH 1/2] usb: gadget: f_fs: edit epfile->ep under lock Michal Nazarewicz
2016-10-04 0:07 ` [PATCH 2/2] usb: gadget: f_fs: stop sleeping in ffs_func_eps_disable Michal Nazarewicz
2016-10-04 0:26 ` John Stultz
2016-10-08 6:52 ` Chen Yu
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=xa1ty42bu2kv.fsf@mina86.com \
--to=mina86@mina86.com \
--cc=amit.pundir@linaro.org \
--cc=chenyu56@huawei.com \
--cc=felipe.balbi@linux.intel.com \
--cc=gregkh@linuxfoundation.org \
--cc=guodong.xu@linaro.org \
--cc=john.stultz@linaro.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-usb@vger.kernel.org \
--cc=wangbinghui@hisilicon.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).