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

[Robert Baldyga <r.baldyga@samsung.com>]

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