BUG: scheduling while atomic in f_fs when gadget remove driver

BUG: scheduling while atomic in f_fs when gadget remove driver

[Chen Yu]

Hi All,

I'm working on Hikey board based around the HiSilicon Kirin 620, with linaro kernel version 4.8.rc1 and I
get below BUG error while extracting USB cable from PC.

The funtion using f_fs is adb and usb_gadget_unregister_driver will be called after extracting USB cable from PC.

[   89.456512s][pid:1,cpu1,init]BUG: scheduling while atomic: init/1/0x00000002
[   89.456573s]Modules linked in:
[   89.456604s]Preemption disabled at:[<ffffffc0006a6dc0>] composite_disconnect+0x30/0xac
[   89.456665s][pid:1,cpu1,init]TGID: 1 Comm: init
[   89.456695s][pid:1,cpu1,init]Call trace:
[   89.456726s][pid:1,cpu1,init][<ffffffc00008a5e0>] dump_backtrace+0x0/0x15c
[   89.456756s][pid:1,cpu1,init][<ffffffc00008a75c>] show_stack+0x20/0x28
[   89.456756s][pid:1,cpu1,init][<ffffffc001153714>] dump_stack+0x84/0xa8
[   89.456787s][pid:1,cpu1,init][<ffffffc0000cfc5c>] __schedule_bug+0x88/0xdc
[   89.456817s][pid:1,cpu1,init][<ffffffc00115c4f0>] __schedule+0x714/0x854
[   89.456817s][pid:1,cpu1,init][<ffffffc00115c678>] schedule+0x48/0xa4
[   89.456817s][pid:1,cpu1,init][<ffffffc00115cbf0>] schedule_preempt_disabled+0x4c/0xf4
[   89.456848s][pid:1,cpu1,init][<ffffffc00115ea90>] __mutex_lock_slowpath+0xbc/0x1a4
[   89.456878s][pid:1,cpu1,init][<ffffffc00115ebd8>] mutex_lock+0x60/0x64
[   89.456878s][pid:1,cpu1,init][<ffffffc0006beb00>] ffs_func_eps_disable.isra.17+0x54/0x114
[   89.456909s][pid:1,cpu1,init][<ffffffc0006c05a4>] ffs_func_disable+0x30/0xa0
[   89.456909s][pid:1,cpu1,init][<ffffffc0006a6c4c>] reset_config.isra.8+0x44/0x78
[   89.456939s][pid:1,cpu1,init][<ffffffc0006a6dd8>] composite_disconnect+0x48/0xac
[   89.456939s][pid:1,cpu1,init][<ffffffc0006aafd4>] android_disconnect+0x48/0x54
[   89.456970s][pid:1,cpu1,init][<ffffffc0006ad9d0>] usb_gadget_remove_driver+0x58/0xa0
[   89.456970s][pid:1,cpu1,init][<ffffffc0006ada90>] usb_gadget_unregister_driver+0x78/0xc4

I checked the codes of composite_disconnect and found spin_lock_irqsave called before reset_config in which
ffs_func_eps_disable is called.

void composite_disconnect(struct usb_gadget *gadget)
{
        struct usb_composite_dev        *cdev = get_gadget_data(gadget);
        unsigned long                   flags;

        /* REVISIT:  should we have config and device level
         * disconnect callbacks?
         */
        spin_lock_irqsave(&cdev->lock, flags);
        if (cdev->config)
                reset_config(cdev);
        if (cdev->driver->disconnect)
                cdev->driver->disconnect(cdev);
        spin_unlock_irqrestore(&cdev->lock, flags);
}

static void ffs_func_eps_disable(struct ffs_function *func)
{
        struct ffs_ep *ep         = func->eps;
        struct ffs_epfile *epfile = func->ffs->epfiles;
        unsigned count            = func->ffs->eps_count;
        unsigned long flags;

        do {
                if (epfile)
                        mutex_lock(&epfile->mutex);
                spin_lock_irqsave(&func->ffs->eps_lock, flags);
                /* pending requests get nuked */
                if (likely(ep->ep))
                        usb_ep_disable(ep->ep);
                ++ep;
                spin_unlock_irqrestore(&func->ffs->eps_lock, flags);

                if (epfile) {
                        epfile->ep = NULL;
                        kfree(epfile->read_buffer);
                        epfile->read_buffer = NULL;
                        mutex_unlock(&epfile->mutex);
                        ++epfile;
                }
        } while (--count);
}

Should the epfile->read_buffer be cleared another place and the mutex_lock can be removed in ffs_func_eps_disable?

Thanks and Regards
Chen Yu

Re: BUG: scheduling while atomic in f_fs when gadget remove driver

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 4294 bytes --]


Hi,

Chen Yu <chenyu56@huawei.com> writes:
> Hi All,
>
> I'm working on Hikey board based around the HiSilicon Kirin 620, with
> linaro kernel version 4.8.rc1 and I get below BUG error while
> extracting USB cable from PC.

which peripheral controller does this one have? Is it dwc3?

I'm very interested in knowing about throughtput of adb push with dwc3 + f_fs.

Also, do you know if adb can run outside of android environment? I've
been looking for a proper functionfs user for quite some time now :-(

> The funtion using f_fs is adb and usb_gadget_unregister_driver will be
> called after extracting USB cable from PC.
>
> [   89.456512s][pid:1,cpu1,init]BUG: scheduling while atomic: init/1/0x00000002
> [   89.456573s]Modules linked in:
> [   89.456604s]Preemption disabled at:[<ffffffc0006a6dc0>] composite_disconnect+0x30/0xac
> [   89.456665s][pid:1,cpu1,init]TGID: 1 Comm: init
> [   89.456695s][pid:1,cpu1,init]Call trace:
> [   89.456726s][pid:1,cpu1,init][<ffffffc00008a5e0>] dump_backtrace+0x0/0x15c
> [   89.456756s][pid:1,cpu1,init][<ffffffc00008a75c>] show_stack+0x20/0x28
> [   89.456756s][pid:1,cpu1,init][<ffffffc001153714>] dump_stack+0x84/0xa8
> [   89.456787s][pid:1,cpu1,init][<ffffffc0000cfc5c>] __schedule_bug+0x88/0xdc
> [   89.456817s][pid:1,cpu1,init][<ffffffc00115c4f0>] __schedule+0x714/0x854
> [   89.456817s][pid:1,cpu1,init][<ffffffc00115c678>] schedule+0x48/0xa4
> [   89.456817s][pid:1,cpu1,init][<ffffffc00115cbf0>] schedule_preempt_disabled+0x4c/0xf4
> [   89.456848s][pid:1,cpu1,init][<ffffffc00115ea90>] __mutex_lock_slowpath+0xbc/0x1a4
> [   89.456878s][pid:1,cpu1,init][<ffffffc00115ebd8>] mutex_lock+0x60/0x64
> [   89.456878s][pid:1,cpu1,init][<ffffffc0006beb00>] ffs_func_eps_disable.isra.17+0x54/0x114
> [   89.456909s][pid:1,cpu1,init][<ffffffc0006c05a4>] ffs_func_disable+0x30/0xa0
> [   89.456909s][pid:1,cpu1,init][<ffffffc0006a6c4c>] reset_config.isra.8+0x44/0x78
> [   89.456939s][pid:1,cpu1,init][<ffffffc0006a6dd8>] composite_disconnect+0x48/0xac
> [   89.456939s][pid:1,cpu1,init][<ffffffc0006aafd4>] android_disconnect+0x48/0x54
> [   89.456970s][pid:1,cpu1,init][<ffffffc0006ad9d0>] usb_gadget_remove_driver+0x58/0xa0
> [   89.456970s][pid:1,cpu1,init][<ffffffc0006ada90>] usb_gadget_unregister_driver+0x78/0xc4
>
> I checked the codes of composite_disconnect and found
> spin_lock_irqsave called before reset_config in which
> ffs_func_eps_disable is called.
>
> void composite_disconnect(struct usb_gadget *gadget)
> {
>         struct usb_composite_dev        *cdev = get_gadget_data(gadget);
>         unsigned long                   flags;
>
>         /* REVISIT:  should we have config and device level
>          * disconnect callbacks?
>          */
>         spin_lock_irqsave(&cdev->lock, flags);
>         if (cdev->config)
>                 reset_config(cdev);
>         if (cdev->driver->disconnect)
>                 cdev->driver->disconnect(cdev);
>         spin_unlock_irqrestore(&cdev->lock, flags);
> }
>
> static void ffs_func_eps_disable(struct ffs_function *func)
> {
>         struct ffs_ep *ep         = func->eps;
>         struct ffs_epfile *epfile = func->ffs->epfiles;
>         unsigned count            = func->ffs->eps_count;
>         unsigned long flags;
>
>         do {
>                 if (epfile)
>                         mutex_lock(&epfile->mutex);
>                 spin_lock_irqsave(&func->ffs->eps_lock, flags);
>                 /* pending requests get nuked */
>                 if (likely(ep->ep))
>                         usb_ep_disable(ep->ep);
>                 ++ep;
>                 spin_unlock_irqrestore(&func->ffs->eps_lock, flags);
>
>                 if (epfile) {
>                         epfile->ep = NULL;
>                         kfree(epfile->read_buffer);
>                         epfile->read_buffer = NULL;
>                         mutex_unlock(&epfile->mutex);
>                         ++epfile;
>                 }
>         } while (--count);
> }
>
> Should the epfile->read_buffer be cleared another place and the
> mutex_lock can be removed in ffs_func_eps_disable?

You are correct. There's a bug there. Can you try to propose a fix for
it?

thanks

-- 
balbi

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 800 bytes --]

Re: BUG: scheduling while atomic in f_fs when gadget remove driver

[Chen Yu]

Hi,

在 2016/9/27 18:01, Felipe Balbi 写道:
> 
> Hi,
> 
> Chen Yu <chenyu56@huawei.com> writes:
>> Hi All,
>>
>> I'm working on Hikey board based around the HiSilicon Kirin 620, with
>> linaro kernel version 4.8.rc1 and I get below BUG error while
>> extracting USB cable from PC.
> 
> which peripheral controller does this one have? Is it dwc3?
> 
> I'm very interested in knowing about throughtput of adb push with dwc3 + f_fs.
> 

Sorry for a delay, the peripheral controller on HiSilicon Kirin 620 is dwc2.

> Also, do you know if adb can run outside of android environment? I've
> been looking for a proper functionfs user for quite some time now :-(
> 

I'm not sure if adb can run outside of android environment, there are some requirements:
1.adbd can run outside of android environment.
2.Do initialization for configfs and f_fs as android. There is an example on Hikey:
  https://github.com/96boards-hikey/android_device_linaro_hikey/blob/android-5.0/init.hikey.rc
  line 46-65 and line 161-163.

Hope this can help to you!

>> The funtion using f_fs is adb and usb_gadget_unregister_driver will be
>> called after extracting USB cable from PC.
>>
>> [   89.456512s][pid:1,cpu1,init]BUG: scheduling while atomic: init/1/0x00000002
>> [   89.456573s]Modules linked in:
>> [   89.456604s]Preemption disabled at:[<ffffffc0006a6dc0>] composite_disconnect+0x30/0xac
>> [   89.456665s][pid:1,cpu1,init]TGID: 1 Comm: init
>> [   89.456695s][pid:1,cpu1,init]Call trace:
>> [   89.456726s][pid:1,cpu1,init][<ffffffc00008a5e0>] dump_backtrace+0x0/0x15c
>> [   89.456756s][pid:1,cpu1,init][<ffffffc00008a75c>] show_stack+0x20/0x28
>> [   89.456756s][pid:1,cpu1,init][<ffffffc001153714>] dump_stack+0x84/0xa8
>> [   89.456787s][pid:1,cpu1,init][<ffffffc0000cfc5c>] __schedule_bug+0x88/0xdc
>> [   89.456817s][pid:1,cpu1,init][<ffffffc00115c4f0>] __schedule+0x714/0x854
>> [   89.456817s][pid:1,cpu1,init][<ffffffc00115c678>] schedule+0x48/0xa4
>> [   89.456817s][pid:1,cpu1,init][<ffffffc00115cbf0>] schedule_preempt_disabled+0x4c/0xf4
>> [   89.456848s][pid:1,cpu1,init][<ffffffc00115ea90>] __mutex_lock_slowpath+0xbc/0x1a4
>> [   89.456878s][pid:1,cpu1,init][<ffffffc00115ebd8>] mutex_lock+0x60/0x64
>> [   89.456878s][pid:1,cpu1,init][<ffffffc0006beb00>] ffs_func_eps_disable.isra.17+0x54/0x114
>> [   89.456909s][pid:1,cpu1,init][<ffffffc0006c05a4>] ffs_func_disable+0x30/0xa0
>> [   89.456909s][pid:1,cpu1,init][<ffffffc0006a6c4c>] reset_config.isra.8+0x44/0x78
>> [   89.456939s][pid:1,cpu1,init][<ffffffc0006a6dd8>] composite_disconnect+0x48/0xac
>> [   89.456939s][pid:1,cpu1,init][<ffffffc0006aafd4>] android_disconnect+0x48/0x54
>> [   89.456970s][pid:1,cpu1,init][<ffffffc0006ad9d0>] usb_gadget_remove_driver+0x58/0xa0
>> [   89.456970s][pid:1,cpu1,init][<ffffffc0006ada90>] usb_gadget_unregister_driver+0x78/0xc4
>>
>> I checked the codes of composite_disconnect and found
>> spin_lock_irqsave called before reset_config in which
>> ffs_func_eps_disable is called.
>>
>> void composite_disconnect(struct usb_gadget *gadget)
>> {
>>         struct usb_composite_dev        *cdev = get_gadget_data(gadget);
>>         unsigned long                   flags;
>>
>>         /* REVISIT:  should we have config and device level
>>          * disconnect callbacks?
>>          */
>>         spin_lock_irqsave(&cdev->lock, flags);
>>         if (cdev->config)
>>                 reset_config(cdev);
>>         if (cdev->driver->disconnect)
>>                 cdev->driver->disconnect(cdev);
>>         spin_unlock_irqrestore(&cdev->lock, flags);
>> }
>>
>> static void ffs_func_eps_disable(struct ffs_function *func)
>> {
>>         struct ffs_ep *ep         = func->eps;
>>         struct ffs_epfile *epfile = func->ffs->epfiles;
>>         unsigned count            = func->ffs->eps_count;
>>         unsigned long flags;
>>
>>         do {
>>                 if (epfile)
>>                         mutex_lock(&epfile->mutex);
>>                 spin_lock_irqsave(&func->ffs->eps_lock, flags);
>>                 /* pending requests get nuked */
>>                 if (likely(ep->ep))
>>                         usb_ep_disable(ep->ep);
>>                 ++ep;
>>                 spin_unlock_irqrestore(&func->ffs->eps_lock, flags);
>>
>>                 if (epfile) {
>>                         epfile->ep = NULL;
>>                         kfree(epfile->read_buffer);
>>                         epfile->read_buffer = NULL;
>>                         mutex_unlock(&epfile->mutex);
>>                         ++epfile;
>>                 }
>>         } while (--count);
>> }
>>
>> Should the epfile->read_buffer be cleared another place and the
>> mutex_lock can be removed in ffs_func_eps_disable?
> 
> You are correct. There's a bug there. Can you try to propose a fix for
> it?
> 
> thanks
> 

I will try to fix it, but I'm engaged in other tasks and can not spend much time on it.

Do you have any suggestions about how to fix it?

thanks

Re: BUG: scheduling while atomic in f_fs when gadget remove driver

[Michal Nazarewicz]

On Wed, Sep 28 2016, Chen Yu wrote:
> I will try to fix it, but I'm engaged in other tasks and can not spend
> much time on it.
>
> Do you have any suggestions about how to fix it?

epfile->ep is protected by ffs->eps_lock which brings us to realisation
that there is another bug in the code and we need to do this:

------- >8 -------------------------------------------------------------
>From 0ce6cc5e2440800243eff06c6952cba0f976da2f Mon Sep 17 00:00:00 2001
From: Michal Nazarewicz <mina86@mina86.com>
Date: Wed, 28 Sep 2016 18:10:42 +0200
Subject: [PATCH] usb: gadget: f_fs: edit epfile->ep under lock

epfile->ep is protected by ffs->eps_lock (not epfile->mutex) so clear it
while holding the spin lock.

Signed-off-by: Michal Nazarewicz <mina86@mina86.com>
Fixes: 9353afbbfa7b ("buffer data from ‘oversized’ OUT requests")
---
 drivers/usb/gadget/function/f_fs.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/usb/gadget/function/f_fs.c b/drivers/usb/gadget/function/f_fs.c
index 0aeed85..759f5d4 100644
--- a/drivers/usb/gadget/function/f_fs.c
+++ b/drivers/usb/gadget/function/f_fs.c
@@ -1725,17 +1725,17 @@ static void ffs_func_eps_disable(struct ffs_function *func)
 	unsigned long flags;
 
 	do {
-		if (epfile)
-			mutex_lock(&epfile->mutex);
 		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) {
-			epfile->ep = NULL;
+			mutex_lock(&epfile->mutex);
 			kfree(epfile->read_buffer);
 			epfile->read_buffer = NULL;
 			mutex_unlock(&epfile->mutex);
------- >8 -------------------------------------------------------------

With that done, the only thing which needs a mutex is
epfile->read_buffer.

The read_buffer pointer shouldn’t be that big of an issue (it could be
protected by the same eps_lock).  The real problem is freeing the
memory.

We cannot do it while __ffs_epfile_read_buffered is reading data from
it.  We cannot blindly schedule it to happen later either since in the
meanwhile __ffs_epfile_read_buffered could have freed it.

-- 
Best regards
ミハウ “𝓶𝓲𝓷𝓪86” ナザレヴイツ
«If at first you don’t succeed, give up skydiving»

Re: BUG: scheduling while atomic in f_fs when gadget remove driver

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

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

Re: BUG: scheduling while atomic in f_fs when gadget remove driver

[Chen Yu]

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

Re: BUG: scheduling while atomic in f_fs when gadget remove driver

[John Stultz]

On Wed, Sep 28, 2016 at 2:38 PM, Michal Nazarewicz <mina86@mina86.com> wrote:
> 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")

So the patch here seems to be in some odd encoding? Can you resend it
using git-send-email or in some way other then embedding it inline
here? Maybe just point me to a git tree that has it?

thanks
-john

Re: BUG: scheduling while atomic in f_fs when gadget remove driver

[Michal Nazarewicz]

On Mon, Oct 03 2016, John Stultz wrote:
> On Wed, Sep 28, 2016 at 2:38 PM, Michal Nazarewicz <mina86@mina86.com> wrote:
>> 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")
>
> So the patch here seems to be in some odd encoding?

O_o
It’s UTF-8.

> Can you resend it using git-send-email or in some way other then
> embedding it inline here? Maybe just point me to a git tree that has
> it?

	https://github.com/mina86/linux.git f-fs-fix

Regardless, I’ll prepare a proper patchset within a few days.  Maybe
even now.

-- 
Best regards
ミハウ “𝓶𝓲𝓷𝓪86” ナザレヴイツ
«If at first you don’t succeed, give up skydiving»

Re: BUG: scheduling while atomic in f_fs when gadget remove driver

[John Stultz]

On Mon, Oct 3, 2016 at 1:07 PM, Michal Nazarewicz <mina86@mina86.com> wrote:
> On Mon, Oct 03 2016, John Stultz wrote:
>> On Wed, Sep 28, 2016 at 2:38 PM, Michal Nazarewicz <mina86@mina86.com> wrote:
>>> 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")
>>
>> So the patch here seems to be in some odd encoding?
>
> O_o
> It’s UTF-8.

Yea, I dunno. Simply copied it out of mutt and git am didn't like it.

I think its the "Content-Transfer-Encoding: quoted-printable" main email with
"Content-Transfer-Encoding: 8bit" embedded inside.

Ends up looking like:
Content-Type: text/plain; charset=3DUTF-8

With lots of =3D and =20 values spread about.

>> Can you resend it using git-send-email or in some way other then
>> embedding it inline here? Maybe just point me to a git tree that has
>> it?
>
>         https://github.com/mina86/linux.git f-fs-fix

Thanks so much! I'll fetch that and get testing on my side with it as well!

thanks again!
-john

Re: BUG: scheduling while atomic in f_fs when gadget remove driver

[John Stultz]

On Mon, Oct 3, 2016 at 1:16 PM, John Stultz <john.stultz@linaro.org> wrote:
> On Mon, Oct 3, 2016 at 1:07 PM, Michal Nazarewicz <mina86@mina86.com> wrote:
>> On Mon, Oct 03 2016, John Stultz wrote:
>>> Can you resend it using git-send-email or in some way other then
>>> embedding it inline here? Maybe just point me to a git tree that has
>>> it?
>>
>>         https://github.com/mina86/linux.git f-fs-fix
>
> Thanks so much! I'll fetch that and get testing on my side with it as well!

Thanks! Looking good on my side as well, so for both of those changes
(if its helpful):

Tested-by: John Stultz <john.stultz@linaro.org>

thanks
-john

[PATCH 1/2] usb: gadget: f_fs: edit epfile->ep under lock

[Michal Nazarewicz]

epfile->ep is protected by ffs->eps_lock (not epfile->mutex) so clear it
while holding the spin lock.

Signed-off-by: Michal Nazarewicz <mina86@mina86.com>
---
 drivers/usb/gadget/function/f_fs.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/usb/gadget/function/f_fs.c b/drivers/usb/gadget/function/f_fs.c
index 0aeed85..759f5d4 100644
--- a/drivers/usb/gadget/function/f_fs.c
+++ b/drivers/usb/gadget/function/f_fs.c
@@ -1725,17 +1725,17 @@ static void ffs_func_eps_disable(struct ffs_function *func)
 	unsigned long flags;
 
 	do {
-		if (epfile)
-			mutex_lock(&epfile->mutex);
 		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) {
-			epfile->ep = NULL;
+			mutex_lock(&epfile->mutex);
 			kfree(epfile->read_buffer);
 			epfile->read_buffer = NULL;
 			mutex_unlock(&epfile->mutex);
-- 
2.8.0.rc3.226.g39d4020

[PATCH 2/2] usb: gadget: f_fs: stop sleeping in ffs_func_eps_disable

[Michal Nazarewicz]

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")
Tested-by: Chen Yu <chenyu56@huawei.com>
---
 drivers/usb/gadget/function/f_fs.c | 109 +++++++++++++++++++++++++++++++------
 1 file changed, 93 insertions(+), 16 deletions(-)

Compared to the previous version:
• this one has a bit more comments (I feel like it’s a bad sign that
  this needs so much documentation);
• ffs_epfile_realese sets read_buffer to READ_BUFFER_DROP (which
  doesn’t matter since on entry __ffs_epfile_read_buffered behaves
  the same way when read_buffer is NULL or READ_BUFFER_DROP); and
• __ffs_epfile_read_data will drop the temporary data if read_buffer
  is READ_BUFFER_DROP (which may happen if ep is disabled between ep
  request finishes and data is copied to user space).

Chen, John, if you could test this version as well, that would be
swell.

diff --git a/drivers/usb/gadget/function/f_fs.c b/drivers/usb/gadget/function/f_fs.c
index 759f5d4..e15fc1b 100644
--- a/drivers/usb/gadget/function/f_fs.c
+++ b/drivers/usb/gadget/function/f_fs.c
@@ -136,8 +136,60 @@ 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 is initialised with NULL value and may be set by
+	 * __ffs_epfile_read_data function to point to a temporary buffer.
+	 *
+	 * In normal operation, calls to __ffs_epfile_read_buffered will consume
+	 * data from said 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 (they are synchronised by epfile->mutex)
+	 * so the latter will not assign a new value to the pointer.
+	 *
+	 * 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.
+	 *
+	 * == State transitions ==
+	 *
+	 * • ptr == NULL:  (initial state)
+	 *   ◦ __ffs_epfile_read_buffer_free: go to ptr == DROP
+	 *   ◦ __ffs_epfile_read_buffered:    nop
+	 *   ◦ __ffs_epfile_read_data allocates temp buffer: go to ptr == buf
+	 *   ◦ reading finishes:              n/a, not in ‘and reading’ state
+	 * • ptr == DROP:
+	 *   ◦ __ffs_epfile_read_buffer_free: nop
+	 *   ◦ __ffs_epfile_read_buffered:    go to ptr == NULL
+	 *   ◦ __ffs_epfile_read_data allocates temp buffer: free buf, nop
+	 *   ◦ reading finishes:              n/a, not in ‘and reading’ state
+	 * • ptr == buf:
+	 *   ◦ __ffs_epfile_read_buffer_free: free buf, go to ptr == DROP
+	 *   ◦ __ffs_epfile_read_buffered:    go to ptr == NULL and reading
+	 *   ◦ __ffs_epfile_read_data:        n/a, __ffs_epfile_read_buffered
+	 *                                    is always called first
+	 *   ◦ reading finishes:              n/a, not in ‘and reading’ state
+	 * • ptr == NULL and reading:
+	 *   ◦ __ffs_epfile_read_buffer_free: go to ptr == DROP and reading
+	 *   ◦ __ffs_epfile_read_buffered:    n/a, mutex is held
+	 *   ◦ __ffs_epfile_read_data:        n/a, mutex is held
+	 *   ◦ reading finishes and …
+	 *     … all data read:               free buf, go to ptr == NULL
+	 *     … otherwise:                   go to ptr == buf and reading
+	 * • ptr == DROP and reading:
+	 *   ◦ __ffs_epfile_read_buffer_free: nop
+	 *   ◦ __ffs_epfile_read_buffered:    n/a, mutex is held
+	 *   ◦ __ffs_epfile_read_data:        n/a, mutex is held
+	 *   ◦ reading finishes:              free buf, go to ptr == DROP
 	 */
-	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];
 
@@ -736,25 +788,47 @@ static void ffs_epfile_async_io_complete(struct usb_ep *_ep,
 	schedule_work(&io_data->work);
 }
 
+static void __ffs_epfile_read_buffer_free(struct ffs_epfile *epfile)
+{
+	/*
+	 * See comment in struct ffs_epfile for full read_buffer pointer
+	 * synchronisation story.
+	 */
+	struct ffs_buffer *buf = xchg(&epfile->read_buffer, READ_BUFFER_DROP);
+	if (buf && buf != READ_BUFFER_DROP)
+		kfree(buf);
+}
+
 /* Assumes epfile->mutex is held. */
 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.  See comment in struct ffs_epfile
+	 * for full read_buffer pointer synchronisation story.
+	 */
+	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 +857,15 @@ 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;
+
+	/*
+	 * At this point read_buffer is NULL or READ_BUFFER_DROP (if
+	 * ffs_func_eps_disable has been called in the meanwhile).  See comment
+	 * in struct ffs_epfile for full read_buffer pointer synchronisation
+	 * story.
+	 */
+	if (unlikely(cmpxchg(&epfile->read_buffer, NULL, buf)))
+		kfree(buf);
 
 	return ret;
 }
@@ -1097,8 +1179,7 @@ ffs_epfile_release(struct inode *inode, struct file *file)
 
 	ENTER();
 
-	kfree(epfile->read_buffer);
-	epfile->read_buffer = NULL;
+	__ffs_epfile_read_buffer_free(epfile);
 	ffs_data_closed(epfile->ffs);
 
 	return 0;
@@ -1724,24 +1805,20 @@ static void ffs_func_eps_disable(struct ffs_function *func)
 	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;
+			__ffs_epfile_read_buffer_free(epfile);
 			++epfile;
 		}
 	} while (--count);
+	spin_unlock_irqrestore(&func->ffs->eps_lock, flags);
 }
 
 static int ffs_func_eps_enable(struct ffs_function *func)
-- 
2.8.0.rc3.226.g39d4020

Re: [PATCH 2/2] usb: gadget: f_fs: stop sleeping in ffs_func_eps_disable

[John Stultz]

On Mon, Oct 3, 2016 at 5:07 PM, Michal Nazarewicz <mina86@mina86.com> wrote:
> 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")
> Tested-by: Chen Yu <chenyu56@huawei.com>
> ---
>  drivers/usb/gadget/function/f_fs.c | 109 +++++++++++++++++++++++++++++++------
>  1 file changed, 93 insertions(+), 16 deletions(-)
>
> Compared to the previous version:
> • this one has a bit more comments (I feel like it’s a bad sign that
>   this needs so much documentation);
> • ffs_epfile_realese sets read_buffer to READ_BUFFER_DROP (which
>   doesn’t matter since on entry __ffs_epfile_read_buffered behaves
>   the same way when read_buffer is NULL or READ_BUFFER_DROP); and
> • __ffs_epfile_read_data will drop the temporary data if read_buffer
>   is READ_BUFFER_DROP (which may happen if ep is disabled between ep
>   request finishes and data is copied to user space).
>
> Chen, John, if you could test this version as well, that would be
> swell.

for both patches:

Tested-by: John Stultz <john.stultz@linaro.org>

Thanks so much for these!
-john

Re: [PATCH 2/2] usb: gadget: f_fs: stop sleeping in ffs_func_eps_disable

[Chen Yu]

On 2016/10/4 8:07, Michal Nazarewicz wrote:
> 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")
> Tested-by: Chen Yu <chenyu56@huawei.com>
> ---
>  drivers/usb/gadget/function/f_fs.c | 109 +++++++++++++++++++++++++++++++------
>  1 file changed, 93 insertions(+), 16 deletions(-)
> 
> Compared to the previous version:
> • this one has a bit more comments (I feel like it’s a bad sign that
>   this needs so much documentation);
> • ffs_epfile_realese sets read_buffer to READ_BUFFER_DROP (which
>   doesn’t matter since on entry __ffs_epfile_read_buffered behaves
>   the same way when read_buffer is NULL or READ_BUFFER_DROP); and
> • __ffs_epfile_read_data will drop the temporary data if read_buffer
>   is READ_BUFFER_DROP (which may happen if ep is disabled between ep
>   request finishes and data is copied to user space).
> 
> Chen, John, if you could test this version as well, that would be
> swell.
> 

for both patches:

Tested-by: Chen Yu <chenyu56@huawei.com>

thanks again!
Chen Yu