| Message ID | 1636712682-1226-1-git-send-email-quic_pkondeti@quicinc.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | c76ef96fc00eb398c8fc836b0eb2f82bcc619dc7 |
| Headers | show |
| Series | [v3] usb: gadget: f_fs: Use stream_open() for endpoint files | expand |
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 >
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)
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(-)