[PATCH] usb: gadget: ffs: don't allow to open with O_NONBLOCK flag

[PATCH] usb: gadget: ffs: don't allow to open with O_NONBLOCK flag

[Robert Baldyga]

FunctionFS can't support O_NONBLOCK because read/write operatons are
directly translated into USB requests which are asynchoronous, so we
can't know how long we will have to wait for request completion. For
this reason in case of open with O_NONBLOCK flag we return -EWOULDBLOCK.

Signed-off-by: Robert Baldyga <r.baldyga@samsung.com>
---
 drivers/usb/gadget/function/f_fs.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/drivers/usb/gadget/function/f_fs.c b/drivers/usb/gadget/function/f_fs.c
index 175c995..1014911 100644
--- a/drivers/usb/gadget/function/f_fs.c
+++ b/drivers/usb/gadget/function/f_fs.c
@@ -538,6 +538,14 @@ static int ffs_ep0_open(struct inode *inode, struct file *file)
 	if (unlikely(ffs->state == FFS_CLOSING))
 		return -EBUSY;
 
+	/*
+	 * We are not supporting O_NONBLOCK because read/write operatons are
+	 * directly translated into USB requests which are asynchoronous, so
+	 * we can't know how long we will have to wait for request completion.
+	 */
+	if (unlikely(file->f_flags & O_NONBLOCK))
+		return -EWOULDBLOCK;
+
 	file->private_data = ffs;
 	ffs_data_opened(ffs);
 
@@ -874,6 +882,14 @@ ffs_epfile_open(struct inode *inode, struct file *file)
 	if (WARN_ON(epfile->ffs->state != FFS_ACTIVE))
 		return -ENODEV;
 
+	/*
+	 * We are not supporting O_NONBLOCK because read/write operatons are
+	 * directly translated into USB requests which are asynchoronous, so
+	 * we can't know how long we will have to wait for request completion.
+	 */
+	if (unlikely(file->f_flags & O_NONBLOCK))
+		return -EWOULDBLOCK;
+
 	file->private_data = epfile;
 	ffs_data_opened(epfile->ffs);
 
-- 
1.9.1

Re: [PATCH] usb: gadget: ffs: don't allow to open with O_NONBLOCK flag

[Michal Nazarewicz]

On Wed, Apr 01 2015, Robert Baldyga <r.baldyga@samsung.com> wrote:
> FunctionFS can't support O_NONBLOCK because read/write operatons are
> directly translated into USB requests which are asynchoronous, so we
> can't know how long we will have to wait for request completion. For
> this reason in case of open with O_NONBLOCK flag we return
> -EWOULDBLOCK.

‘can’t’ is a bit strong of a word here though.  It can, but in a few
cases it doesn’t.

It kinda saddens me that this undoes all the lines of code that were put
into the file to support O_NONBLOCK (e.g. FFS_NO_SETUP path of
ffs_ep0_read).

I’m also worried this may break existing applications which, for better
or worse, open the file with O_NONBLOCK.

Most importantly though, this does not stop users from using fcntl to
set O_NONBLOCK, so if you really want to stop O_NONBLOCK from being set,
that path should be checked as well (if possible).

> Signed-off-by: Robert Baldyga <r.baldyga@samsung.com>
> ---
>  drivers/usb/gadget/function/f_fs.c | 16 ++++++++++++++++
>  1 file changed, 16 insertions(+)
>
> diff --git a/drivers/usb/gadget/function/f_fs.c b/drivers/usb/gadget/function/f_fs.c
> index 175c995..1014911 100644
> --- a/drivers/usb/gadget/function/f_fs.c
> +++ b/drivers/usb/gadget/function/f_fs.c
> @@ -538,6 +538,14 @@ static int ffs_ep0_open(struct inode *inode, struct file *file)
>  	if (unlikely(ffs->state == FFS_CLOSING))
>  		return -EBUSY;
>  
> +	/*
> +	 * We are not supporting O_NONBLOCK because read/write operatons are
> +	 * directly translated into USB requests which are asynchoronous, so
> +	 * we can't know how long we will have to wait for request completion.
> +	 */
> +	if (unlikely(file->f_flags & O_NONBLOCK))
> +		return -EWOULDBLOCK;
> +
>  	file->private_data = ffs;
>  	ffs_data_opened(ffs);
>  
> @@ -874,6 +882,14 @@ ffs_epfile_open(struct inode *inode, struct file *file)
>  	if (WARN_ON(epfile->ffs->state != FFS_ACTIVE))
>  		return -ENODEV;
>  
> +	/*
> +	 * We are not supporting O_NONBLOCK because read/write operatons are
> +	 * directly translated into USB requests which are asynchoronous, so
> +	 * we can't know how long we will have to wait for request completion.
> +	 */
> +	if (unlikely(file->f_flags & O_NONBLOCK))
> +		return -EWOULDBLOCK;
> +
>  	file->private_data = epfile;
>  	ffs_data_opened(epfile->ffs);
>  
> -- 
> 1.9.1
>

-- 
Best regards,                                         _     _
.o. | Liege of Serenely Enlightened Majesty of      o' \,=./ `o
..o | Computer Science,  Michał “mina86” Nazarewicz    (o o)
ooo +--<mpn@google.com>--<xmpp:mina86@jabber.org>--ooO--(_)--Ooo--

Re: [PATCH] usb: gadget: ffs: don't allow to open with O_NONBLOCK flag

[Robert Baldyga]

Hi Michal,

On 04/01/2015 05:17 PM, Michal Nazarewicz wrote:
> On Wed, Apr 01 2015, Robert Baldyga <r.baldyga@samsung.com> wrote:
>> FunctionFS can't support O_NONBLOCK because read/write operatons are
>> directly translated into USB requests which are asynchoronous, so we
>> can't know how long we will have to wait for request completion. For
>> this reason in case of open with O_NONBLOCK flag we return
>> -EWOULDBLOCK.
> 
> ‘can’t’ is a bit strong of a word here though.  It can, but in a few
> cases it doesn’t.
> 
> It kinda saddens me that this undoes all the lines of code that were put
> into the file to support O_NONBLOCK (e.g. FFS_NO_SETUP path of
> ffs_ep0_read).
> 
> I’m also worried this may break existing applications which, for better
> or worse, open the file with O_NONBLOCK.
> 
> Most importantly though, this does not stop users from using fcntl to
> set O_NONBLOCK, so if you really want to stop O_NONBLOCK from being set,
> that path should be checked as well (if possible).

I want rather to inform users that non-blocking i/o wouldn't work for
epfiles. Indeed we can handle O_NONBLOCK for ep0 (for the same reason we
can have poll), but for other epfiles there is no way to check if
read/write operation can end up in short time. Everything is up to host.

When we can open file with O_NONBLOCK we expect that non-blocking API
will work, which isn't true in our case. Then it causes problems like
this one: https://lkml.org/lkml/2015/3/31/688

However it looks like you're right that my patch can cause ABI break, so
it shouldn't be applied.

Do you have any idea what can we do about that?

Best regards,
Robert Baldyga

> 
>> Signed-off-by: Robert Baldyga <r.baldyga@samsung.com>
>> ---
>>  drivers/usb/gadget/function/f_fs.c | 16 ++++++++++++++++
>>  1 file changed, 16 insertions(+)
>>
>> diff --git a/drivers/usb/gadget/function/f_fs.c b/drivers/usb/gadget/function/f_fs.c
>> index 175c995..1014911 100644
>> --- a/drivers/usb/gadget/function/f_fs.c
>> +++ b/drivers/usb/gadget/function/f_fs.c
>> @@ -538,6 +538,14 @@ static int ffs_ep0_open(struct inode *inode, struct file *file)
>>  	if (unlikely(ffs->state == FFS_CLOSING))
>>  		return -EBUSY;
>>  
>> +	/*
>> +	 * We are not supporting O_NONBLOCK because read/write operatons are
>> +	 * directly translated into USB requests which are asynchoronous, so
>> +	 * we can't know how long we will have to wait for request completion.
>> +	 */
>> +	if (unlikely(file->f_flags & O_NONBLOCK))
>> +		return -EWOULDBLOCK;
>> +
>>  	file->private_data = ffs;
>>  	ffs_data_opened(ffs);
>>  
>> @@ -874,6 +882,14 @@ ffs_epfile_open(struct inode *inode, struct file *file)
>>  	if (WARN_ON(epfile->ffs->state != FFS_ACTIVE))
>>  		return -ENODEV;
>>  
>> +	/*
>> +	 * We are not supporting O_NONBLOCK because read/write operatons are
>> +	 * directly translated into USB requests which are asynchoronous, so
>> +	 * we can't know how long we will have to wait for request completion.
>> +	 */
>> +	if (unlikely(file->f_flags & O_NONBLOCK))
>> +		return -EWOULDBLOCK;
>> +
>>  	file->private_data = epfile;
>>  	ffs_data_opened(epfile->ffs);
>>  
>> -- 
>> 1.9.1
>>
> 

Re: [PATCH] usb: gadget: ffs: don't allow to open with O_NONBLOCK flag

[Michal Nazarewicz]

On Fri, Apr 03 2015, Robert Baldyga <r.baldyga@samsung.com> wrote:
> Hi Michal,
>
> On 04/01/2015 05:17 PM, Michal Nazarewicz wrote:
>> On Wed, Apr 01 2015, Robert Baldyga <r.baldyga@samsung.com> wrote:
>>> FunctionFS can't support O_NONBLOCK because read/write operatons are
>>> directly translated into USB requests which are asynchoronous, so we
>>> can't know how long we will have to wait for request completion. For
>>> this reason in case of open with O_NONBLOCK flag we return
>>> -EWOULDBLOCK.
>> 
>> ‘can’t’ is a bit strong of a word here though.  It can, but in a few
>> cases it doesn’t.
>> 
>> It kinda saddens me that this undoes all the lines of code that were put
>> into the file to support O_NONBLOCK (e.g. FFS_NO_SETUP path of
>> ffs_ep0_read).
>> 
>> I’m also worried this may break existing applications which, for better
>> or worse, open the file with O_NONBLOCK.
>> 
>> Most importantly though, this does not stop users from using fcntl to
>> set O_NONBLOCK, so if you really want to stop O_NONBLOCK from being set,
>> that path should be checked as well (if possible).
>
> I want rather to inform users that non-blocking i/o wouldn't work for
> epfiles. Indeed we can handle O_NONBLOCK for ep0 (for the same reason we
> can have poll), but for other epfiles there is no way to check if
> read/write operation can end up in short time.

There is potentially a way to implement O_NONBLOCK for epfiles.  This
would require adding a new state property to epfile and moving
completion structure from stack to epfile.  In pseudo code we would
have:

epfile->state = FREE

epfile_io(direction)
	if epfile->state == FREE
		queue(ep->req)
		epfile->state = PENDING

	if epfile->state == PENDING
		if O_NONBLOCK
			return -EAGAIN
		wait
		epfile->state = FINISHED

	// epfile->state == FINISHED
	copy data
	epfile->state = FREE
	return

I think this is the only ‘technically correct’ solution, but I dunno if
it is worth implementing especially since AIO is available.
        
-- 
Best regards,                                         _     _
.o. | Liege of Serenely Enlightened Majesty of      o' \,=./ `o
..o | Computer Science,  Michał “mina86” Nazarewicz    (o o)
ooo +--<mpn@google.com>--<xmpp:mina86@jabber.org>--ooO--(_)--Ooo--

RE: [PATCH] usb: gadget: ffs: don't allow to open with O_NONBLOCK flag

[David Laight]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 1616 bytes --]

From: Robert Baldyga
> Hi Michal,
> 
> On 04/01/2015 05:17 PM, Michal Nazarewicz wrote:
> > On Wed, Apr 01 2015, Robert Baldyga <r.baldyga@samsung.com> wrote:
> >> FunctionFS can't support O_NONBLOCK because read/write operatons are
> >> directly translated into USB requests which are asynchoronous, so we
> >> can't know how long we will have to wait for request completion. For
> >> this reason in case of open with O_NONBLOCK flag we return
> >> -EWOULDBLOCK.
> >
> > cant is a bit strong of a word here though.  It can, but in a few
> > cases it doesnt.
> >
> > It kinda saddens me that this undoes all the lines of code that were put
> > into the file to support O_NONBLOCK (e.g. FFS_NO_SETUP path of
> > ffs_ep0_read).
> >
> > Im also worried this may break existing applications which, for better
> > or worse, open the file with O_NONBLOCK.
> >
> > Most importantly though, this does not stop users from using fcntl to
> > set O_NONBLOCK, so if you really want to stop O_NONBLOCK from being set,
> > that path should be checked as well (if possible).
> 
> I want rather to inform users that non-blocking i/o wouldn't work for
> epfiles. Indeed we can handle O_NONBLOCK for ep0 (for the same reason we
> can have poll), but for other epfiles there is no way to check if
> read/write operation can end up in short time. Everything is up to host.

Is that really necessary?
I'm sure there are a lot of device drivers that ignore O_NONBLOCK.

	David

ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

Re: [PATCH] usb: gadget: ffs: don't allow to open with O_NONBLOCK flag

[Michal Nazarewicz]

> From: Robert Baldyga
>> I want rather to inform users that non-blocking i/o wouldn't work for
>> epfiles. Indeed we can handle O_NONBLOCK for ep0 (for the same reason we
>> can have poll), but for other epfiles there is no way to check if
>> read/write operation can end up in short time. Everything is up to host.

On Tue, Apr 07 2015, David Laight wrote:
> Is that really necessary?
> I'm sure there are a lot of device drivers that ignore O_NONBLOCK.

FFS partially supports O_NONBLOCK which may fool people into thinking it
has full support.

epfiles don’t implement poll though so I’m not sure how users would
imagine O_NONBLOCK being used with them.  ep0, on the other hand,
implements poll and (as far as I can see and despite what Robert wrote)
suffers from the same problem so it may be considered a bigger issue.

Overall though, I do agree that we need to consider whether the current
situation is really a serious problem.

-- 
Best regards,                                         _     _
.o. | Liege of Serenely Enlightened Majesty of      o' \,=./ `o
..o | Computer Science,  Michał “mina86” Nazarewicz    (o o)
ooo +--<mpn@google.com>--<xmpp:mina86@jabber.org>--ooO--(_)--Ooo--