[PATCH] usb: gadget: f_fs: Use stream_open() for endpoint files

[PATCH] usb: gadget: f_fs: Use stream_open() for endpoint files

[Pavankumar Kondeti]

Function fs endpoint files does not have the notion of file position.
So switch to stream like functionality. This allows concurrent threads
to be blocked in the ffs read/write operations which use ffs_mutex_lock().
The ffs mutex lock deploys interruptible wait. Otherwise, threads are
blocking for the mutex lock in __fdget_pos(). For whatever reason, ff the
host does not send/receive data for longer time, hung task warnings
are observed.

Change-Id: I602fa56fb5ed4c8c46e19df68c3335c4b12cae81
Signed-off-by: Pavankumar Kondeti <quic_pkondeti@quicinc.com>
---
 drivers/usb/gadget/function/f_fs.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/gadget/function/f_fs.c b/drivers/usb/gadget/function/f_fs.c
index e20c19a..3c584da 100644
--- a/drivers/usb/gadget/function/f_fs.c
+++ b/drivers/usb/gadget/function/f_fs.c
@@ -614,7 +614,7 @@ static int ffs_ep0_open(struct inode *inode, struct file *file)
 	file->private_data = ffs;
 	ffs_data_opened(ffs);
 
-	return 0;
+	return stream_open(inode, file);
 }
 
 static int ffs_ep0_release(struct inode *inode, struct file *file)
@@ -1154,7 +1154,7 @@ ffs_epfile_open(struct inode *inode, struct file *file)
 	file->private_data = epfile;
 	ffs_data_opened(epfile->ffs);
 
-	return 0;
+	return stream_open(inode, file);
 }
 
 static int ffs_aio_cancel(struct kiocb *kiocb)
-- 
2.7.4

Re: [PATCH] usb: gadget: f_fs: Use stream_open() for endpoint files

[Greg Kroah-Hartman]

On Thu, Nov 11, 2021 at 04:41:55PM +0530, Pavankumar Kondeti wrote:
> Function fs endpoint files does not have the notion of file position.
> So switch to stream like functionality. This allows concurrent threads
> to be blocked in the ffs read/write operations which use ffs_mutex_lock().
> The ffs mutex lock deploys interruptible wait. Otherwise, threads are
> blocking for the mutex lock in __fdget_pos(). For whatever reason, ff the
> host does not send/receive data for longer time, hung task warnings
> are observed.
> 
> Change-Id: I602fa56fb5ed4c8c46e19df68c3335c4b12cae81

Always run scripts/checkpatch.pl on your patches so you do not get
grumpy maintainers asking you to run scripts/checkpatch.pl on them...

thanks,

greg k-h

Re: [PATCH] usb: gadget: f_fs: Use stream_open() for endpoint files

[Pavan Kondeti]

Hi Greg,

On Thu, Nov 11, 2021 at 12:38:26PM +0100, Greg Kroah-Hartman wrote:
> On Thu, Nov 11, 2021 at 04:41:55PM +0530, Pavankumar Kondeti wrote:
> > Function fs endpoint files does not have the notion of file position.
> > So switch to stream like functionality. This allows concurrent threads
> > to be blocked in the ffs read/write operations which use ffs_mutex_lock().
> > The ffs mutex lock deploys interruptible wait. Otherwise, threads are
> > blocking for the mutex lock in __fdget_pos(). For whatever reason, ff the
> > host does not send/receive data for longer time, hung task warnings
> > are observed.
> > 
> > Change-Id: I602fa56fb5ed4c8c46e19df68c3335c4b12cae81
> 
> Always run scripts/checkpatch.pl on your patches so you do not get
> grumpy maintainers asking you to run scripts/checkpatch.pl on them...
> 
Thanks for taking a look at the patch. My bad, I have applied the patch
from a recent android tree and carry forwarded this tag. I will fix it.

Thanks,
Pavan

[PATCH v2] usb: gadget: f_fs: Use stream_open() for endpoint files

[Pavankumar Kondeti]

Function fs endpoint files does not have the notion of file position.
So switch to stream like functionality. This allows concurrent threads
to be blocked in the ffs read/write operations which use ffs_mutex_lock().
The ffs mutex lock deploys interruptible wait. Otherwise, threads are
blocking for the mutex lock in __fdget_pos(). For whatever reason, ff the
host does not send/receive data for longer time, hung task warnings
are observed.

Signed-off-by: Pavankumar Kondeti <quic_pkondeti@quicinc.com>
---
V2:
- Removed Change-Id tag

 drivers/usb/gadget/function/f_fs.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/gadget/function/f_fs.c b/drivers/usb/gadget/function/f_fs.c
index e20c19a..3c584da 100644
--- a/drivers/usb/gadget/function/f_fs.c
+++ b/drivers/usb/gadget/function/f_fs.c
@@ -614,7 +614,7 @@ static int ffs_ep0_open(struct inode *inode, struct file *file)
 	file->private_data = ffs;
 	ffs_data_opened(ffs);
 
-	return 0;
+	return stream_open(inode, file);
 }
 
 static int ffs_ep0_release(struct inode *inode, struct file *file)
@@ -1154,7 +1154,7 @@ ffs_epfile_open(struct inode *inode, struct file *file)
 	file->private_data = epfile;
 	ffs_data_opened(epfile->ffs);
 
-	return 0;
+	return stream_open(inode, file);
 }
 
 static int ffs_aio_cancel(struct kiocb *kiocb)
-- 
2.7.4

Re: [PATCH v2] usb: gadget: f_fs: Use stream_open() for endpoint files

[Greg Kroah-Hartman]

On Thu, Nov 11, 2021 at 05:45:56PM +0530, Pavankumar Kondeti wrote:
> Function fs endpoint files does not have the notion of file position.
> So switch to stream like functionality. This allows concurrent threads
> to be blocked in the ffs read/write operations which use ffs_mutex_lock().
> The ffs mutex lock deploys interruptible wait. Otherwise, threads are
> blocking for the mutex lock in __fdget_pos(). For whatever reason, ff the
> host does not send/receive data for longer time, hung task warnings
> are observed.

So the current code is broken?  What commit caused it to break?

Doesn't this change cause a change in behavior for existing userspace
tools, or will they still work as-is?

thanks,

greg k-h

Re: [PATCH v2] usb: gadget: f_fs: Use stream_open() for endpoint files

[Pavan Kondeti]

Hi Greg,

On Thu, Nov 11, 2021 at 02:12:28PM +0100, Greg Kroah-Hartman wrote:
> On Thu, Nov 11, 2021 at 05:45:56PM +0530, Pavankumar Kondeti wrote:
> > Function fs endpoint files does not have the notion of file position.
> > So switch to stream like functionality. This allows concurrent threads
> > to be blocked in the ffs read/write operations which use ffs_mutex_lock().
> > The ffs mutex lock deploys interruptible wait. Otherwise, threads are
> > blocking for the mutex lock in __fdget_pos(). For whatever reason, ff the
> > host does not send/receive data for longer time, hung task warnings
> > are observed.
> 
> So the current code is broken?  What commit caused it to break?

This is not a serious bug that can affect functionality. if hung_task_panic
sysctl is not enabled, probably nobody would notice this except an obscure
warning in the kernel dmesg log. It is all about the task state while
it is blocked for I/O. The function fs code uses interruptible wait but
we are not reaching there and getting blocked at VFS layer due to the below
commit introduced from 3.14 kernel.

commit 9c225f2655e36a470c4f58dbbc99244c5fc7f2d4
Author: Linus Torvalds <torvalds@linux-foundation.org>
Date:   Mon Mar 3 09:36:58 2014 -0800

    vfs: atomic f_pos accesses as per POSIX

    Our write() system call has always been atomic in the sense that you get
    the expected thread-safe contiguous write, but we haven't actually
    guaranteed that concurrent writes are serialized wrt f_pos accesses, so
    threads (or processes) that share a file descriptor and use "write()"
    concurrently would quite likely overwrite each others data.

We have uncovered this issue via customer bug report which happens very rarely.
It only happens like when host does not pull the data for a very long time.
Since function fs does not care about file position, thought stream_open()
is the right thing to do here.

> 
> Doesn't this change cause a change in behavior for existing userspace
> tools, or will they still work as-is?
> 

I don't think it affects user space as it just changes the task state from 
UNINTERRUPTIBLE to INTERRUPTIBLE while waiting for the USB transfers to
finish. However there is a slight change to the O_NONBLOCK behavior.
Earlier threads that are using O_NONBLOCK are also getting blocked
inside fdget_pos(). Now they reach to f_fs and error code is returned. IOW,
we are actually fixing the non blocking behavior here.

PS: I believe you asked these questions since the commit description does not
cover it. I can happily add all this information to it. Since it is all historical,
I did not mention it.

Thanks,
Pavan

Re: [PATCH v2] usb: gadget: f_fs: Use stream_open() for endpoint files

[Greg Kroah-Hartman]

On Fri, Nov 12, 2021 at 08:47:30AM +0530, Pavan Kondeti wrote:
> Hi Greg,
> 
> On Thu, Nov 11, 2021 at 02:12:28PM +0100, Greg Kroah-Hartman wrote:
> > On Thu, Nov 11, 2021 at 05:45:56PM +0530, Pavankumar Kondeti wrote:
> > > Function fs endpoint files does not have the notion of file position.
> > > So switch to stream like functionality. This allows concurrent threads
> > > to be blocked in the ffs read/write operations which use ffs_mutex_lock().
> > > The ffs mutex lock deploys interruptible wait. Otherwise, threads are
> > > blocking for the mutex lock in __fdget_pos(). For whatever reason, ff the
> > > host does not send/receive data for longer time, hung task warnings
> > > are observed.
> > 
> > So the current code is broken?  What commit caused it to break?
> 
> This is not a serious bug that can affect functionality. if hung_task_panic
> sysctl is not enabled, probably nobody would notice this except an obscure
> warning in the kernel dmesg log. It is all about the task state while
> it is blocked for I/O. The function fs code uses interruptible wait but
> we are not reaching there and getting blocked at VFS layer due to the below
> commit introduced from 3.14 kernel.
> 
> commit 9c225f2655e36a470c4f58dbbc99244c5fc7f2d4
> Author: Linus Torvalds <torvalds@linux-foundation.org>
> Date:   Mon Mar 3 09:36:58 2014 -0800
> 
>     vfs: atomic f_pos accesses as per POSIX
> 
>     Our write() system call has always been atomic in the sense that you get
>     the expected thread-safe contiguous write, but we haven't actually
>     guaranteed that concurrent writes are serialized wrt f_pos accesses, so
>     threads (or processes) that share a file descriptor and use "write()"
>     concurrently would quite likely overwrite each others data.
> 
> We have uncovered this issue via customer bug report which happens very rarely.
> It only happens like when host does not pull the data for a very long time.
> Since function fs does not care about file position, thought stream_open()
> is the right thing to do here.
> 
> > 
> > Doesn't this change cause a change in behavior for existing userspace
> > tools, or will they still work as-is?
> > 
> 
> I don't think it affects user space as it just changes the task state from 
> UNINTERRUPTIBLE to INTERRUPTIBLE while waiting for the USB transfers to
> finish. However there is a slight change to the O_NONBLOCK behavior.
> Earlier threads that are using O_NONBLOCK are also getting blocked
> inside fdget_pos(). Now they reach to f_fs and error code is returned. IOW,
> we are actually fixing the non blocking behavior here.
> 
> PS: I believe you asked these questions since the commit description does not
> cover it. I can happily add all this information to it. Since it is all historical,
> I did not mention it.

Please add all of this to the commit log description so that we can
properly understand it in the future.

thnaks,

greg k-h

[PATCH v3] usb: gadget: f_fs: Use stream_open() for endpoint files

[Pavankumar Kondeti]

Function fs endpoint file operations are synchronized via an interruptible
mutex wait. However we see threads that do ep file operations concurrently
are getting blocked for the mutex lock in __fdget_pos(). This is an
uninterruptible wait and we see hung task warnings and kernel panic
if hung_task_panic systcl is enabled if host does not send/receive
the data for long time.

The reason for threads getting blocked in __fdget_pos() is due to
the file position protection introduced by the commit 9c225f2655e3
("vfs: atomic f_pos accesses as per POSIX"). Since function fs
endpoint files does not have the notion of the file position, switch
to the stream mode. This will bypass the file position mutex and
threads will be blocked in interruptible state for the function fs
mutex.

It should not affects user space as we are only changing the task state
changes the task state from UNINTERRUPTIBLE to INTERRUPTIBLE while waiting
for the USB transfers to be finished. However there is a slight change to
the O_NONBLOCK behavior. Earlier threads that are using O_NONBLOCK are also
getting blocked inside fdget_pos(). Now they reach to function fs and error
code is returned. The non blocking behavior is actually honoured now.

Signed-off-by: Pavankumar Kondeti <quic_pkondeti@quicinc.com>
---
v3:
- Added more background and user space impact in the commit description

v2:
- Removed Change-Id tag

 drivers/usb/gadget/function/f_fs.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/gadget/function/f_fs.c b/drivers/usb/gadget/function/f_fs.c
index e20c19a..3c584da 100644
--- a/drivers/usb/gadget/function/f_fs.c
+++ b/drivers/usb/gadget/function/f_fs.c
@@ -614,7 +614,7 @@ static int ffs_ep0_open(struct inode *inode, struct file *file)
 	file->private_data = ffs;
 	ffs_data_opened(ffs);
 
-	return 0;
+	return stream_open(inode, file);
 }
 
 static int ffs_ep0_release(struct inode *inode, struct file *file)
@@ -1154,7 +1154,7 @@ ffs_epfile_open(struct inode *inode, struct file *file)
 	file->private_data = epfile;
 	ffs_data_opened(epfile->ffs);
 
-	return 0;
+	return stream_open(inode, file);
 }
 
 static int ffs_aio_cancel(struct kiocb *kiocb)
-- 
2.7.4

Re: [PATCH v3] usb: gadget: f_fs: Use stream_open() for endpoint files

[John Keeping]

On Fri, Nov 12, 2021 at 03:54:40PM +0530, Pavankumar Kondeti wrote:
> Function fs endpoint file operations are synchronized via an interruptible
> mutex wait. However we see threads that do ep file operations concurrently
> are getting blocked for the mutex lock in __fdget_pos(). This is an
> uninterruptible wait and we see hung task warnings and kernel panic
> if hung_task_panic systcl is enabled if host does not send/receive
> the data for long time.
> 
> The reason for threads getting blocked in __fdget_pos() is due to
> the file position protection introduced by the commit 9c225f2655e3
> ("vfs: atomic f_pos accesses as per POSIX"). Since function fs
> endpoint files does not have the notion of the file position, switch
> to the stream mode. This will bypass the file position mutex and
> threads will be blocked in interruptible state for the function fs
> mutex.
> 
> It should not affects user space as we are only changing the task state
> changes the task state from UNINTERRUPTIBLE to INTERRUPTIBLE while waiting
> for the USB transfers to be finished. However there is a slight change to
> the O_NONBLOCK behavior. Earlier threads that are using O_NONBLOCK are also
> getting blocked inside fdget_pos(). Now they reach to function fs and error
> code is returned. The non blocking behavior is actually honoured now.
> 
> Signed-off-by: Pavankumar Kondeti <quic_pkondeti@quicinc.com>

It might be worth noting that this also changes pread & pwrite which
will now return ESPIPE instead of succeeding but ignoring the offset
argument.

Those operations have always been broken and I doubt anyone is using
them, but there is slightly more user visible change than just the task
state.

With that said, I think this is a good change, so:

Reviewed-by: John Keeping <john@metanate.com>

> ---
> v3:
> - Added more background and user space impact in the commit description
> 
> v2:
> - Removed Change-Id tag
> 
>  drivers/usb/gadget/function/f_fs.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/usb/gadget/function/f_fs.c b/drivers/usb/gadget/function/f_fs.c
> index e20c19a..3c584da 100644
> --- a/drivers/usb/gadget/function/f_fs.c
> +++ b/drivers/usb/gadget/function/f_fs.c
> @@ -614,7 +614,7 @@ static int ffs_ep0_open(struct inode *inode, struct file *file)
>  	file->private_data = ffs;
>  	ffs_data_opened(ffs);
>  
> -	return 0;
> +	return stream_open(inode, file);
>  }
>  
>  static int ffs_ep0_release(struct inode *inode, struct file *file)
> @@ -1154,7 +1154,7 @@ ffs_epfile_open(struct inode *inode, struct file *file)
>  	file->private_data = epfile;
>  	ffs_data_opened(epfile->ffs);
>  
> -	return 0;
> +	return stream_open(inode, file);
>  }
>  
>  static int ffs_aio_cancel(struct kiocb *kiocb)
> -- 
> 2.7.4
>