[PATCH] usb: gadget: f_fs: add "zombie" mode

[PATCH] usb: gadget: f_fs: add "zombie" mode

[Robert Baldyga]

Since we can compose gadgets from many functions, there is the problem
related to gadget breakage while FunctionFS daemon being closed. In some
cases it's strongly desired to keep gadget alive for a while, despite
FunctionFS files are closed, to allow another functions to complete
some presumably critical operations.

For this purpose this patch introduces "zombie" mode. It can be enabled
by setting mount option "zombie=1", and results with defering function
closure to the moment of reopening ep0 file or filesystem umount.

When ffs->state == FFS_ZOMBIE:
- function is still binded and visible to host,
- setup requests are automatically stalled,
- all another transfers are refused,
- opening ep0 causes function close, and then FunctionFS is ready for
  descriptors and string write,
- umount of functionfs cause function close.

Signed-off-by: Robert Baldyga <r.baldyga@samsung.com>
---
 drivers/usb/gadget/function/f_fs.c | 25 ++++++++++++++++++++++---
 drivers/usb/gadget/function/u_fs.h |  4 ++++
 2 files changed, 26 insertions(+), 3 deletions(-)

diff --git a/drivers/usb/gadget/function/f_fs.c b/drivers/usb/gadget/function/f_fs.c
index 7c6771d..5581bec 100644
--- a/drivers/usb/gadget/function/f_fs.c
+++ b/drivers/usb/gadget/function/f_fs.c
@@ -606,6 +606,8 @@ static unsigned int ffs_ep0_poll(struct file *file, poll_table *wait)
 		}
 	case FFS_CLOSING:
 		break;
+	case FFS_ZOMBIE:
+		break;
 	}
 
 	mutex_unlock(&ffs->mutex);
@@ -1152,6 +1154,7 @@ struct ffs_sb_fill_data {
 	struct ffs_file_perms perms;
 	umode_t root_mode;
 	const char *dev_name;
+	bool zombie_mode;
 	struct ffs_data *ffs_data;
 };
 
@@ -1222,6 +1225,12 @@ static int ffs_fs_parse_opts(struct ffs_sb_fill_data *data, char *opts)
 
 		/* Interpret option */
 		switch (eq - opts) {
+		case 6:
+			if (!memcmp(opts, "zombie", 6))
+				data->zombie_mode  = !!value;
+			else
+				goto invalid;
+			break;
 		case 5:
 			if (!memcmp(opts, "rmode", 5))
 				data->root_mode  = (value & 0555) | S_IFDIR;
@@ -1286,6 +1295,7 @@ ffs_fs_mount(struct file_system_type *t, int flags,
 			.gid = GLOBAL_ROOT_GID,
 		},
 		.root_mode = S_IFDIR | 0500,
+		.zombie_mode = false,
 	};
 	struct dentry *rv;
 	int ret;
@@ -1302,6 +1312,7 @@ ffs_fs_mount(struct file_system_type *t, int flags,
 	if (unlikely(!ffs))
 		return ERR_PTR(-ENOMEM);
 	ffs->file_perms = data.perms;
+	ffs->zombie_mode = data.zombie_mode;
 
 	ffs->dev_name = kstrdup(dev_name, GFP_KERNEL);
 	if (unlikely(!ffs->dev_name)) {
@@ -1389,7 +1400,9 @@ static void ffs_data_opened(struct ffs_data *ffs)
 	ENTER();
 
 	atomic_inc(&ffs->ref);
-	atomic_inc(&ffs->opened);
+	if (atomic_add_return(1, &ffs->opened) == 1)
+		if (ffs->state == FFS_ZOMBIE)
+			ffs_data_reset(ffs);
 }
 
 static void ffs_data_put(struct ffs_data *ffs)
@@ -1411,8 +1424,14 @@ static void ffs_data_closed(struct ffs_data *ffs)
 	ENTER();
 
 	if (atomic_dec_and_test(&ffs->opened)) {
-		ffs->state = FFS_CLOSING;
-		ffs_data_reset(ffs);
+		if (ffs->zombie_mode) {
+			ffs->state = FFS_ZOMBIE;
+			if (ffs->setup_state == FFS_SETUP_PENDING)
+				__ffs_ep0_stall(ffs);
+		} else {
+			ffs->state = FFS_CLOSING;
+			ffs_data_reset(ffs);
+		}
 	}
 
 	ffs_data_put(ffs);
diff --git a/drivers/usb/gadget/function/u_fs.h b/drivers/usb/gadget/function/u_fs.h
index cd128e3..7a4abbe 100644
--- a/drivers/usb/gadget/function/u_fs.h
+++ b/drivers/usb/gadget/function/u_fs.h
@@ -92,6 +92,8 @@ enum ffs_state {
 	 */
 	FFS_ACTIVE,
 
+	FFS_ZOMBIE,
+
 	/*
 	 * All endpoints have been closed.  This state is also set if
 	 * we encounter an unrecoverable error.  The only
@@ -251,6 +253,8 @@ struct ffs_data {
 		kgid_t				gid;
 	}				file_perms;
 
+	bool zombie_mode;
+
 	/*
 	 * The endpoint files, filled by ffs_epfiles_create(),
 	 * destroyed by ffs_epfiles_destroy().
-- 
1.9.1

Re: [PATCH] usb: gadget: f_fs: add "zombie" mode

[Michal Nazarewicz]

On Mon, Oct 06 2014, Robert Baldyga <r.baldyga@samsung.com> wrote:
> Since we can compose gadgets from many functions, there is the problem
> related to gadget breakage while FunctionFS daemon being closed. In some
> cases it's strongly desired to keep gadget alive for a while, despite
> FunctionFS files are closed, to allow another functions to complete
> some presumably critical operations.
>
> For this purpose this patch introduces "zombie" mode. It can be enabled
> by setting mount option "zombie=1", and results with defering function
> closure to the moment of reopening ep0 file or filesystem umount.
>
> When ffs->state == FFS_ZOMBIE:
> - function is still binded and visible to host,
> - setup requests are automatically stalled,
> - all another transfers are refused,
> - opening ep0 causes function close, and then FunctionFS is ready for
>   descriptors and string write,
> - umount of functionfs cause function close.

However, all the ep# files will still exist on the filesystem.  This may
be a bit confusing and error-prone, no?

>
> Signed-off-by: Robert Baldyga <r.baldyga@samsung.com>
> ---
>  drivers/usb/gadget/function/f_fs.c | 25 ++++++++++++++++++++++---
>  drivers/usb/gadget/function/u_fs.h |  4 ++++
>  2 files changed, 26 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/usb/gadget/function/f_fs.c b/drivers/usb/gadget/function/f_fs.c
> @@ -1222,6 +1225,12 @@ static int ffs_fs_parse_opts(struct ffs_sb_fill_data *data, char *opts)
>  
>  		/* Interpret option */
>  		switch (eq - opts) {
> +		case 6:
> +			if (!memcmp(opts, "zombie", 6))
> +				data->zombie_mode  = !!value;

Unnecessary double space before =.

> +			else
> +				goto invalid;
> +			break;
>  		case 5:
>  			if (!memcmp(opts, "rmode", 5))
>  				data->root_mode  = (value & 0555) | S_IFDIR;
> diff --git a/drivers/usb/gadget/function/u_fs.h b/drivers/usb/gadget/function/u_fs.h
> @@ -92,6 +92,8 @@ enum ffs_state {
>  	 */
>  	FFS_ACTIVE,
>  
> +	FFS_ZOMBIE,
> +

Add comment describing the state.

>  	/*
>  	 * All endpoints have been closed.  This state is also set if
>  	 * we encounter an unrecoverable error.  The only

-- 
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: f_fs: add "zombie" mode

[Robert Baldyga]

On 10/06/2014 02:36 PM, Michal Nazarewicz wrote:
> On Mon, Oct 06 2014, Robert Baldyga <r.baldyga@samsung.com> wrote:
>> Since we can compose gadgets from many functions, there is the problem
>> related to gadget breakage while FunctionFS daemon being closed. In some
>> cases it's strongly desired to keep gadget alive for a while, despite
>> FunctionFS files are closed, to allow another functions to complete
>> some presumably critical operations.
>>
>> For this purpose this patch introduces "zombie" mode. It can be enabled
>> by setting mount option "zombie=1", and results with defering function
>> closure to the moment of reopening ep0 file or filesystem umount.
>>
>> When ffs->state == FFS_ZOMBIE:
>> - function is still binded and visible to host,
>> - setup requests are automatically stalled,
>> - all another transfers are refused,
>> - opening ep0 causes function close, and then FunctionFS is ready for
>>   descriptors and string write,
>> - umount of functionfs cause function close.
> 
> However, all the ep# files will still exist on the filesystem.  This may
> be a bit confusing and error-prone, no?

Shouldn't be error-prone, because opening them will fail with -ENODEV,
but indeed it can be confusing. I will try to do something about that :)

> 
>>
>> Signed-off-by: Robert Baldyga <r.baldyga@samsung.com>
>> ---
>>  drivers/usb/gadget/function/f_fs.c | 25 ++++++++++++++++++++++---
>>  drivers/usb/gadget/function/u_fs.h |  4 ++++
>>  2 files changed, 26 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/usb/gadget/function/f_fs.c b/drivers/usb/gadget/function/f_fs.c
>> @@ -1222,6 +1225,12 @@ static int ffs_fs_parse_opts(struct ffs_sb_fill_data *data, char *opts)
>>  
>>  		/* Interpret option */
>>  		switch (eq - opts) {
>> +		case 6:
>> +			if (!memcmp(opts, "zombie", 6))
>> +				data->zombie_mode  = !!value;
> 
> Unnecessary double space before =.
> 
>> +			else
>> +				goto invalid;
>> +			break;
>>  		case 5:
>>  			if (!memcmp(opts, "rmode", 5))
>>  				data->root_mode  = (value & 0555) | S_IFDIR;
>> diff --git a/drivers/usb/gadget/function/u_fs.h b/drivers/usb/gadget/function/u_fs.h
>> @@ -92,6 +92,8 @@ enum ffs_state {
>>  	 */
>>  	FFS_ACTIVE,
>>  
>> +	FFS_ZOMBIE,
>> +
> 
> Add comment describing the state.
> 
>>  	/*
>>  	 * All endpoints have been closed.  This state is also set if
>>  	 * we encounter an unrecoverable error.  The only
> 

Thanks,
Robert Baldyga

Re: [PATCH] usb: gadget: f_fs: add "zombie" mode

[Michal Nazarewicz]

> On 10/06/2014 02:36 PM, Michal Nazarewicz wrote:
>> However, all the ep# files will still exist on the filesystem.  This may
>> be a bit confusing and error-prone, no?

On Mon, Oct 06 2014, Robert Baldyga <r.baldyga@samsung.com> wrote:
> Shouldn't be error-prone, because opening them will fail with -ENODEV,
> but indeed it can be confusing. I will try to do something about that

I could imagine someone will write shell script like so:

	ffs_active() {
		test -d "$1" || return 1
		set -- "$1"/ep*
		test $# -gt 1
	}

	if ffs_active /dev/foo-ffs; then
		# …
	fi

With such a script, non-functional ep# files in the functionfs mount
point, could lead to some errors in user-space.  I'm not saying that
this should block on any kind of changes to the way the filesystem works
when the function is inactive, but if possible, w/o a lot of additional
code, I'd rather if all the files disappeared in a zombie state.

-- 
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: f_fs: add "zombie" mode

[Felipe Balbi]

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

Hi,

On Mon, Oct 06, 2014 at 01:25:14PM +0200, Robert Baldyga wrote:
> Since we can compose gadgets from many functions, there is the problem
> related to gadget breakage while FunctionFS daemon being closed. In some
> cases it's strongly desired to keep gadget alive for a while, despite
> FunctionFS files are closed, to allow another functions to complete
> some presumably critical operations.
> 
> For this purpose this patch introduces "zombie" mode. It can be enabled
> by setting mount option "zombie=1", and results with defering function
> closure to the moment of reopening ep0 file or filesystem umount.
> 
> When ffs->state == FFS_ZOMBIE:
> - function is still binded and visible to host,
> - setup requests are automatically stalled,
> - all another transfers are refused,
> - opening ep0 causes function close, and then FunctionFS is ready for
>   descriptors and string write,
> - umount of functionfs cause function close.
> 
> Signed-off-by: Robert Baldyga <r.baldyga@samsung.com>

Can you further explain how do you trigger this ? Do I understand
correctly that you composed a gadget using configfs and that gadget has
functionfs + another gadget ?

Then what do you need to do the trigger the issue, and what really _is_
the issue ? Is gadget disconnecting from host too early ? Do you see a
crash ? Memory leak ? Any logs available ? Any steps to reproduce ?

Quite frankly, I don't really like this "zombie" mode. <joke> I know
there's a "The Walking Dead" hype right now, but this is too much.
</joke>

Anyway, please giver me further details of how to get this done.

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH] usb: gadget: f_fs: add "zombie" mode

[Robert Baldyga]

On 10/07/2014 04:28 AM, Felipe Balbi wrote:
> Hi,
> 
> On Mon, Oct 06, 2014 at 01:25:14PM +0200, Robert Baldyga wrote:
>> Since we can compose gadgets from many functions, there is the problem
>> related to gadget breakage while FunctionFS daemon being closed. In some
>> cases it's strongly desired to keep gadget alive for a while, despite
>> FunctionFS files are closed, to allow another functions to complete
>> some presumably critical operations.
>>
>> For this purpose this patch introduces "zombie" mode. It can be enabled
>> by setting mount option "zombie=1", and results with defering function
>> closure to the moment of reopening ep0 file or filesystem umount.
>>
>> When ffs->state == FFS_ZOMBIE:
>> - function is still binded and visible to host,
>> - setup requests are automatically stalled,
>> - all another transfers are refused,
>> - opening ep0 causes function close, and then FunctionFS is ready for
>>   descriptors and string write,
>> - umount of functionfs cause function close.
>>
>> Signed-off-by: Robert Baldyga <r.baldyga@samsung.com>
> 
> Can you further explain how do you trigger this ? Do I understand
> correctly that you composed a gadget using configfs and that gadget has
> functionfs + another gadget ?
> 

Yes, I compose configfs gadget from functionfs + another gadget, and
when functionfs daemon closes ep files, entire gadget get disconnected
from host. FFS function is userspace code so there is no way to know
when it will close files (it doesn't matter what is the reason of this
situation, it can be daemon logic, program breakage, process kill or any
other). So when we have another function in gadget which, for example,
sends some amount of data, does some software update or implements some
real-time functionality, we may want to keep the gadget connected
despite FFS function is no longer functional. We can't just remove one
of functions from gadget since it has been enumerated, so the only way
we can do that is to make broken FFS function "zombie". It will be still
visible to host but it will no longer implement it's functionality.

> Then what do you need to do the trigger the issue, and what really _is_
> the issue ? Is gadget disconnecting from host too early ? Do you see a
> crash ? Memory leak ? Any logs available ? Any steps to reproduce ?
> 

You simply compose gadget from, for example, ethernet and functionfs.
You try to send some huge file through ftp, and in meantime FFS function
breaks. If FFS hasn't enabled "zombie" mode, entire gadget will be
disconnected and data transmission will fail. If "zombie" mode is
enabled, then FFS function after daemon breakage will become "zombie"
and will be nonfunctional, but ethernet gadget will be still active and
data transfer will be completed.

> Quite frankly, I don't really like this "zombie" mode. <joke> I know
> there's a "The Walking Dead" hype right now, but this is too much.
> </joke>
>

I see, but after all I couldn't find more descriptive name for this feature.

> Anyway, please giver me further details of how to get this done.
> 

Best regards,
Robert Baldyga

Re: [PATCH] usb: gadget: f_fs: add "zombie" mode

[Felipe Balbi]

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

Hi,

On Tue, Oct 07, 2014 at 08:33:16AM +0200, Robert Baldyga wrote:
> On 10/07/2014 04:28 AM, Felipe Balbi wrote:
> > Hi,
> > 
> > On Mon, Oct 06, 2014 at 01:25:14PM +0200, Robert Baldyga wrote:
> >> Since we can compose gadgets from many functions, there is the problem
> >> related to gadget breakage while FunctionFS daemon being closed. In some
> >> cases it's strongly desired to keep gadget alive for a while, despite
> >> FunctionFS files are closed, to allow another functions to complete
> >> some presumably critical operations.
> >>
> >> For this purpose this patch introduces "zombie" mode. It can be enabled
> >> by setting mount option "zombie=1", and results with defering function
> >> closure to the moment of reopening ep0 file or filesystem umount.
> >>
> >> When ffs->state == FFS_ZOMBIE:
> >> - function is still binded and visible to host,
> >> - setup requests are automatically stalled,
> >> - all another transfers are refused,
> >> - opening ep0 causes function close, and then FunctionFS is ready for
> >>   descriptors and string write,
> >> - umount of functionfs cause function close.
> >>
> >> Signed-off-by: Robert Baldyga <r.baldyga@samsung.com>
> > 
> > Can you further explain how do you trigger this ? Do I understand
> > correctly that you composed a gadget using configfs and that gadget has
> > functionfs + another gadget ?
> > 
> 
> Yes, I compose configfs gadget from functionfs + another gadget, and
> when functionfs daemon closes ep files, entire gadget get disconnected
> from host. FFS function is userspace code so there is no way to know
> when it will close files (it doesn't matter what is the reason of this
> situation, it can be daemon logic, program breakage, process kill or any
> other). So when we have another function in gadget which, for example,
> sends some amount of data, does some software update or implements some
> real-time functionality, we may want to keep the gadget connected
> despite FFS function is no longer functional. We can't just remove one
> of functions from gadget since it has been enumerated, so the only way
> we can do that is to make broken FFS function "zombie". It will be still
> visible to host but it will no longer implement it's functionality.

now that's an explanation. Can you update commit log with some of this
info (once we agree on how to go about fixing this) ?

I'm not sure we should try to fix this. The only case where this could
trigger is if ffs daemon crashes and dies or somebody sends a bogus
signal to kill it.

A function cannot communicate with the host if it isn't functional
and ffs depends on its userland daemon. If daemon is crashing, why not
print a big WARN("closed %s while connected to host\n") ? That seems
like it's as much as we can do from the kernel. Userland should know
that they can't have a buggy ffs daemon.

> > Then what do you need to do the trigger the issue, and what really _is_
> > the issue ? Is gadget disconnecting from host too early ? Do you see a
> > crash ? Memory leak ? Any logs available ? Any steps to reproduce ?
> > 
> 
> You simply compose gadget from, for example, ethernet and functionfs.
> You try to send some huge file through ftp, and in meantime FFS function
> breaks. If FFS hasn't enabled "zombie" mode, entire gadget will be
> disconnected and data transmission will fail. If "zombie" mode is
> enabled, then FFS function after daemon breakage will become "zombie"
> and will be nonfunctional, but ethernet gadget will be still active and
> data transfer will be completed.

yeah, this is the problem I have with the feature. We can't expose a
non-working interface to USB host. If daemon crashes, we have to
disconnect.

> > Quite frankly, I don't really like this "zombie" mode. <joke> I know
> > there's a "The Walking Dead" hype right now, but this is too much.
> > </joke>
> >
> 
> I see, but after all I couldn't find more descriptive name for this feature.

That was a joke :)

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

RE: [PATCH] usb: gadget: f_fs: add "zombie" mode

[Krzysztof Opasiak]

Hi,

> -----Original Message-----
> From: Felipe Balbi [mailto:balbi@ti.com]
> Sent: Tuesday, October 07, 2014 4:07 PM
> To: Robert Baldyga
> Cc: balbi@ti.com; gregkh@linuxfoundation.org; linux-
> usb@vger.kernel.org; linux-kernel@vger.kernel.org;
> mina86@mina86.com; andrzej.p@samsung.com; k.opasiak@samsung.com
> Subject: Re: [PATCH] usb: gadget: f_fs: add "zombie" mode
> 
> Hi,
> 
> On Tue, Oct 07, 2014 at 08:33:16AM +0200, Robert Baldyga wrote:
> > On 10/07/2014 04:28 AM, Felipe Balbi wrote:
> > > Hi,
> > >
> > > On Mon, Oct 06, 2014 at 01:25:14PM +0200, Robert Baldyga wrote:
> > >> Since we can compose gadgets from many functions, there is the
> > >> problem related to gadget breakage while FunctionFS daemon
> being
> > >> closed. In some cases it's strongly desired to keep gadget
> alive
> > >> for a while, despite FunctionFS files are closed, to allow
> another
> > >> functions to complete some presumably critical operations.
> > >>
> > >> For this purpose this patch introduces "zombie" mode. It can
> be
> > >> enabled by setting mount option "zombie=1", and results with
> > >> defering function closure to the moment of reopening ep0 file
> or filesystem umount.
> > >>
> > >> When ffs->state == FFS_ZOMBIE:
> > >> - function is still binded and visible to host,
> > >> - setup requests are automatically stalled,
> > >> - all another transfers are refused,
> > >> - opening ep0 causes function close, and then FunctionFS is
> ready for
> > >>   descriptors and string write,
> > >> - umount of functionfs cause function close.
> > >>
> > >> Signed-off-by: Robert Baldyga <r.baldyga@samsung.com>
> > >
> > > Can you further explain how do you trigger this ? Do I
> understand
> > > correctly that you composed a gadget using configfs and that
> gadget
> > > has functionfs + another gadget ?
> > >
> >
> > Yes, I compose configfs gadget from functionfs + another gadget,
> and
> > when functionfs daemon closes ep files, entire gadget get
> disconnected
> > from host. FFS function is userspace code so there is no way to
> know
> > when it will close files (it doesn't matter what is the reason of
> this
> > situation, it can be daemon logic, program breakage, process kill
> or
> > any other). So when we have another function in gadget which, for
> > example, sends some amount of data, does some software update or
> > implements some real-time functionality, we may want to keep the
> > gadget connected despite FFS function is no longer functional. We
> > can't just remove one of functions from gadget since it has been
> > enumerated, so the only way we can do that is to make broken FFS
> > function "zombie". It will be still visible to host but it will
> no longer implement it's functionality.
> 
> now that's an explanation. Can you update commit log with some of
> this info (once we agree on how to go about fixing this) ?
> 
> I'm not sure we should try to fix this. The only case where this
> could trigger is if ffs daemon crashes and dies or somebody sends a
> bogus signal to kill it.
> 
> A function cannot communicate with the host if it isn't functional
> and ffs depends on its userland daemon. If daemon is crashing, why
> not print a big WARN("closed %s while connected to host\n") ? That
> seems like it's as much as we can do from the kernel. Userland
> should know that they can't have a buggy ffs daemon.

It's not a problem of buggy ffs daemon. The problem is that there are
some non deterministic mechanisms in userspace like OOM killer. FFS
daemon can be written very well but if we are out of memory it may
become a victim. In this case reliability of whole gadget hurts a lot.

If it's going about WARN(). I'm not enthusiastic about it. Userspace
process dies all the time, that's quite normal;) I don't think that it
is good idea to generate a warning on kernel level when some process
dies. Kernel should be resistant for such situations and know how to
deal with them (maybe user could select exact behavior, but it should be
done on kernel site)

-- 
Krzysztof Opasiak
Samsung R&D Institute Poland
Samsung Electronics
k.opasiak@samsung.com

Re: [PATCH] usb: gadget: f_fs: add "zombie" mode

[Felipe Balbi]

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

Hi,

On Tue, Oct 07, 2014 at 05:01:15PM +0200, Krzysztof Opasiak wrote:
> > > > Hi,
> > > >
> > > > On Mon, Oct 06, 2014 at 01:25:14PM +0200, Robert Baldyga wrote:
> > > >> Since we can compose gadgets from many functions, there is the
> > > >> problem related to gadget breakage while FunctionFS daemon
> > being
> > > >> closed. In some cases it's strongly desired to keep gadget
> > alive
> > > >> for a while, despite FunctionFS files are closed, to allow
> > another
> > > >> functions to complete some presumably critical operations.
> > > >>
> > > >> For this purpose this patch introduces "zombie" mode. It can
> > be
> > > >> enabled by setting mount option "zombie=1", and results with
> > > >> defering function closure to the moment of reopening ep0 file
> > or filesystem umount.
> > > >>
> > > >> When ffs->state == FFS_ZOMBIE:
> > > >> - function is still binded and visible to host,
> > > >> - setup requests are automatically stalled,
> > > >> - all another transfers are refused,
> > > >> - opening ep0 causes function close, and then FunctionFS is
> > ready for
> > > >>   descriptors and string write,
> > > >> - umount of functionfs cause function close.
> > > >>
> > > >> Signed-off-by: Robert Baldyga <r.baldyga@samsung.com>
> > > >
> > > > Can you further explain how do you trigger this ? Do I
> > understand
> > > > correctly that you composed a gadget using configfs and that
> > gadget
> > > > has functionfs + another gadget ?
> > > >
> > >
> > > Yes, I compose configfs gadget from functionfs + another gadget,
> > and
> > > when functionfs daemon closes ep files, entire gadget get
> > disconnected
> > > from host. FFS function is userspace code so there is no way to
> > know
> > > when it will close files (it doesn't matter what is the reason of
> > this
> > > situation, it can be daemon logic, program breakage, process kill
> > or
> > > any other). So when we have another function in gadget which, for
> > > example, sends some amount of data, does some software update or
> > > implements some real-time functionality, we may want to keep the
> > > gadget connected despite FFS function is no longer functional. We
> > > can't just remove one of functions from gadget since it has been
> > > enumerated, so the only way we can do that is to make broken FFS
> > > function "zombie". It will be still visible to host but it will
> > no longer implement it's functionality.
> > 
> > now that's an explanation. Can you update commit log with some of
> > this info (once we agree on how to go about fixing this) ?
> > 
> > I'm not sure we should try to fix this. The only case where this
> > could trigger is if ffs daemon crashes and dies or somebody sends a
> > bogus signal to kill it.
> > 
> > A function cannot communicate with the host if it isn't functional
> > and ffs depends on its userland daemon. If daemon is crashing, why
> > not print a big WARN("closed %s while connected to host\n") ? That
> > seems like it's as much as we can do from the kernel. Userland
> > should know that they can't have a buggy ffs daemon.
> 
> It's not a problem of buggy ffs daemon. The problem is that there are
> some non deterministic mechanisms in userspace like OOM killer. FFS
> daemon can be written very well but if we are out of memory it may
> become a victim. In this case reliability of whole gadget hurts a lot.
> 
> If it's going about WARN(). I'm not enthusiastic about it. Userspace
> process dies all the time, that's quite normal;) I don't think that it
> is good idea to generate a warning on kernel level when some process
> dies. Kernel should be resistant for such situations and know how to
> deal with them (maybe user could select exact behavior, but it should be
> done on kernel site)

yeah, and the way to deal with that is disconnecting from the host
because that USB function, can't be functional anymore. I mean, imagine
you try to e.g. unload pictures from your nice DSLR and that DSLR runs
Linux and implements MTP or PTP using FFS. Then ptpd dies and you're
still connected to the host so you can't know that something went wrong,
the camera just stoped sending you data. So you figure: well, it must
just be slow, I'll leave it here and go have a nap. Hours later and
nothing has changed, because ptpd is still missing.

If you disconnect from the host, however, user knows instantaneously
that something went wrong.

I don't think maintaining a "zombie" function is very nice. In fact, the
very reason for adding usb_function_activate/deactivate was exactly to
prevent us from ever connecting to a host with a non-working function.

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

RE: [PATCH] usb: gadget: f_fs: add "zombie" mode

[Krzysztof Opasiak]

Hi

> -----Original Message-----
> From: Felipe Balbi [mailto:balbi@ti.com]
> Sent: Tuesday, October 07, 2014 5:28 PM
> To: Krzysztof Opasiak
> Cc: balbi@ti.com; 'Robert Baldyga'; gregkh@linuxfoundation.org;
> linux-usb@vger.kernel.org; linux-kernel@vger.kernel.org;
> mina86@mina86.com; andrzej.p@samsung.com
> Subject: Re: [PATCH] usb: gadget: f_fs: add "zombie" mode
> 
> Hi,
> 
> On Tue, Oct 07, 2014 at 05:01:15PM +0200, Krzysztof Opasiak wrote:
> > > > > Hi,
> > > > >
> > > > > On Mon, Oct 06, 2014 at 01:25:14PM +0200, Robert Baldyga
> wrote:
> > > > >> Since we can compose gadgets from many functions, there is
> the
> > > > >> problem related to gadget breakage while FunctionFS daemon
> > > being
> > > > >> closed. In some cases it's strongly desired to keep gadget
> > > alive
> > > > >> for a while, despite FunctionFS files are closed, to allow
> > > another
> > > > >> functions to complete some presumably critical operations.
> > > > >>
> > > > >> For this purpose this patch introduces "zombie" mode. It
> can
> > > be
> > > > >> enabled by setting mount option "zombie=1", and results
> with
> > > > >> defering function closure to the moment of reopening ep0
> file
> > > or filesystem umount.
> > > > >>
> > > > >> When ffs->state == FFS_ZOMBIE:
> > > > >> - function is still binded and visible to host,
> > > > >> - setup requests are automatically stalled,
> > > > >> - all another transfers are refused,
> > > > >> - opening ep0 causes function close, and then FunctionFS
> is
> > > ready for
> > > > >>   descriptors and string write,
> > > > >> - umount of functionfs cause function close.
> > > > >>
> > > > >> Signed-off-by: Robert Baldyga <r.baldyga@samsung.com>
> > > > >
> > > > > Can you further explain how do you trigger this ? Do I
> > > understand
> > > > > correctly that you composed a gadget using configfs and
> that
> > > gadget
> > > > > has functionfs + another gadget ?
> > > > >
> > > >
> > > > Yes, I compose configfs gadget from functionfs + another
> gadget,
> > > and
> > > > when functionfs daemon closes ep files, entire gadget get
> > > disconnected
> > > > from host. FFS function is userspace code so there is no way
> to
> > > know
> > > > when it will close files (it doesn't matter what is the
> reason of
> > > this
> > > > situation, it can be daemon logic, program breakage, process
> kill
> > > or
> > > > any other). So when we have another function in gadget which,
> for
> > > > example, sends some amount of data, does some software update
> or
> > > > implements some real-time functionality, we may want to keep
> the
> > > > gadget connected despite FFS function is no longer
> functional. We
> > > > can't just remove one of functions from gadget since it has
> been
> > > > enumerated, so the only way we can do that is to make broken
> FFS
> > > > function "zombie". It will be still visible to host but it
> will
> > > no longer implement it's functionality.
> > >
> > > now that's an explanation. Can you update commit log with some
> of
> > > this info (once we agree on how to go about fixing this) ?
> > >
> > > I'm not sure we should try to fix this. The only case where
> this
> > > could trigger is if ffs daemon crashes and dies or somebody
> sends a
> > > bogus signal to kill it.
> > >
> > > A function cannot communicate with the host if it isn't
> functional
> > > and ffs depends on its userland daemon. If daemon is crashing,
> why
> > > not print a big WARN("closed %s while connected to host\n") ?
> That
> > > seems like it's as much as we can do from the kernel. Userland
> > > should know that they can't have a buggy ffs daemon.
> >
> > It's not a problem of buggy ffs daemon. The problem is that there
> are
> > some non deterministic mechanisms in userspace like OOM killer.
> FFS
> > daemon can be written very well but if we are out of memory it
> may
> > become a victim. In this case reliability of whole gadget hurts a
> lot.
> >
> > If it's going about WARN(). I'm not enthusiastic about it.
> Userspace
> > process dies all the time, that's quite normal;) I don't think
> that it
> > is good idea to generate a warning on kernel level when some
> process
> > dies. Kernel should be resistant for such situations and know how
> to
> > deal with them (maybe user could select exact behavior, but it
> should
> > be done on kernel site)
> 
> yeah, and the way to deal with that is disconnecting from the host
> because that USB function, can't be functional anymore. I mean,
> imagine you try to e.g. unload pictures from your nice DSLR and
> that DSLR runs Linux and implements MTP or PTP using FFS. Then ptpd
> dies and you're still connected to the host so you can't know that
> something went wrong, the camera just stoped sending you data. So
> you figure: well, it must just be slow, I'll leave it here and go
> have a nap. Hours later and nothing has changed, because ptpd is
> still missing.
> 
> If you disconnect from the host, however, user knows
> instantaneously that something went wrong.

Please believe me that I totally agree with you, but I also see Robert's
point. Let's take ADB as example. Before ADB has been ported to
FunctionFS it communicated with kernel using dev node provided by ADB
function driver. With that infrastructure death of daemon didn't cause
gadget unbind because kernel driver of that function was just stalling
the endpoints. This allows user to use all other functions of this
gadget. In current design ADB uses FunctionFS and for example if daemon
will be killed by OOM whole gadget get's unbind and user cannot use any
other function. Don't you think that's a bit of regression?

I see and understand yours and Roberts point of view. Personally I'm not
too enthusiastic about using this solution but I see some rationales for
this and use cases. Summing it up, this patch may be useful in some
case. Let's allow end user to decide whether to use this mode or not. I
think that a few people will find this useful.

> 
> I don't think maintaining a "zombie" function is very nice. In
> fact, the very reason for adding usb_function_activate/deactivate
> was exactly to prevent us from ever connecting to a host with a
> non-working function.

Here also I agree. Zombie mode should "mock" the function until first
next enumeration or unbind. It should not be possible to bind gadget
with function in zombie mode to UDC. Zombie mode should "pretend" only
as long as gadget is bound and enumerated.


-- 
Krzysztof Opasiak
Samsung R&D Institute Poland
Samsung Electronics
k.opasiak@samsung.com

Re: [PATCH] usb: gadget: f_fs: add "zombie" mode

[Felipe Balbi]

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

Hi,

On Tue, Oct 07, 2014 at 06:37:26PM +0200, Krzysztof Opasiak wrote:

[snip]

> > yeah, and the way to deal with that is disconnecting from the host
> > because that USB function, can't be functional anymore. I mean,
> > imagine you try to e.g. unload pictures from your nice DSLR and
> > that DSLR runs Linux and implements MTP or PTP using FFS. Then ptpd
> > dies and you're still connected to the host so you can't know that
> > something went wrong, the camera just stoped sending you data. So
> > you figure: well, it must just be slow, I'll leave it here and go
> > have a nap. Hours later and nothing has changed, because ptpd is
> > still missing.
> > 
> > If you disconnect from the host, however, user knows
> > instantaneously that something went wrong.
> 
> Please believe me that I totally agree with you, but I also see Robert's
> point. Let's take ADB as example. Before ADB has been ported to
> FunctionFS it communicated with kernel using dev node provided by ADB
> function driver. With that infrastructure death of daemon didn't cause
> gadget unbind because kernel driver of that function was just stalling
> the endpoints. This allows user to use all other functions of this

I really mixed feelings about that. It's really counter-intuitive to
allow a non-working function to be exposed to the host.

> gadget. In current design ADB uses FunctionFS and for example if daemon
> will be killed by OOM whole gadget get's unbind and user cannot use any
> other function. Don't you think that's a bit of regression?

Why ? Why would you event want to keep the other functions working ?
Since you're running out of memory anyway, you'd just delay the
inevitable. Soon enough you won't be able to transfer files through
mtp/ptp, or enable usb tethering, or any of that.

> I see and understand yours and Roberts point of view. Personally I'm not
> too enthusiastic about using this solution but I see some rationales for
> this and use cases. Summing it up, this patch may be useful in some
> case. Let's allow end user to decide whether to use this mode or not. I
> think that a few people will find this useful.

It can't be only end user, we have to also consider USB certification.
If we go to certification with a non-working function (say adbd crashed
during init or whatever), then we won't pass certification.

I would rather cause the gadget to disconnect so any crashes on adbd can
be fixed and we pass certification, then exposing that non-working
function to the host.

OOM is another situation which we don't really have control over.
There's nothing we can do to prevent an application from malloc(1 << 30)
and cause OOM to go crazy, however arguably that application is wrong
for allocating 1GiB of memory, in any case, you get the point :-)

> > I don't think maintaining a "zombie" function is very nice. In
> > fact, the very reason for adding usb_function_activate/deactivate
> > was exactly to prevent us from ever connecting to a host with a
> > non-working function.
> 
> Here also I agree. Zombie mode should "mock" the function until first
> next enumeration or unbind. It should not be possible to bind gadget
> with function in zombie mode to UDC. Zombie mode should "pretend" only
> as long as gadget is bound and enumerated.

Not really. We shouldn't even coonect to host until adbd is running.
Now, when adbd crashes we fix adbd. If it gets killed due to OOM we
can't even say "ok, we'll buffer USB requests until adbd is restarted"
because, well, we're running out of memory.

So, OOM we can't fix. Soon enough, another daemon (mtpd, ptpd, whatever)
will be killed and another function will be left unusable.

As for adbd/mtpd/ptpd crashes, those need to fixed and kernel should not
have to deal with them in any way.

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH] usb: gadget: f_fs: add "zombie" mode

[Alan Stern]

On Tue, 7 Oct 2014, Felipe Balbi wrote:

> > Please believe me that I totally agree with you, but I also see Robert's
> > point. Let's take ADB as example. Before ADB has been ported to
> > FunctionFS it communicated with kernel using dev node provided by ADB
> > function driver. With that infrastructure death of daemon didn't cause
> > gadget unbind because kernel driver of that function was just stalling
> > the endpoints. This allows user to use all other functions of this
> 
> I really mixed feelings about that. It's really counter-intuitive to
> allow a non-working function to be exposed to the host.

...

> > Here also I agree. Zombie mode should "mock" the function until first
> > next enumeration or unbind. It should not be possible to bind gadget
> > with function in zombie mode to UDC. Zombie mode should "pretend" only
> > as long as gadget is bound and enumerated.
> 
> Not really. We shouldn't even coonect to host until adbd is running.
> Now, when adbd crashes we fix adbd. If it gets killed due to OOM we
> can't even say "ok, we'll buffer USB requests until adbd is restarted"
> because, well, we're running out of memory.
> 
> So, OOM we can't fix. Soon enough, another daemon (mtpd, ptpd, whatever)
> will be killed and another function will be left unusable.
> 
> As for adbd/mtpd/ptpd crashes, those need to fixed and kernel should not
> have to deal with them in any way.

It seems to me that we should imitate what an ordinary USB device would
do.  If part of the firmware crashes, generally you would expect none
of the endpoints associated with that function to work.  Either they
refuse to accept output from the host or they stall everything.  But
endpoints associated with other parts of the firmware might very well
continue to work okay.

Don't buffer requests.  Either allow the internal FIFOs to fill up or
else reject everything.  Any reasonable host will start getting timeout
expirations and will realize that something is wrong.

Alan Stern

Re: [PATCH] usb: gadget: f_fs: add "zombie" mode

[Felipe Balbi]

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

Hi,

On Tue, Oct 07, 2014 at 01:15:32PM -0400, Alan Stern wrote:
> > > Here also I agree. Zombie mode should "mock" the function until first
> > > next enumeration or unbind. It should not be possible to bind gadget
> > > with function in zombie mode to UDC. Zombie mode should "pretend" only
> > > as long as gadget is bound and enumerated.
> > 
> > Not really. We shouldn't even coonect to host until adbd is running.
> > Now, when adbd crashes we fix adbd. If it gets killed due to OOM we
> > can't even say "ok, we'll buffer USB requests until adbd is restarted"
> > because, well, we're running out of memory.
> > 
> > So, OOM we can't fix. Soon enough, another daemon (mtpd, ptpd, whatever)
> > will be killed and another function will be left unusable.
> > 
> > As for adbd/mtpd/ptpd crashes, those need to fixed and kernel should not
> > have to deal with them in any way.
> 
> It seems to me that we should imitate what an ordinary USB device would
> do.  If part of the firmware crashes, generally you would expect none
> of the endpoints associated with that function to work.  Either they
> refuse to accept output from the host or they stall everything.  But
> endpoints associated with other parts of the firmware might very well
> continue to work okay.

dunno, I have never seen a USB device firmware crash and I don't think
anybody deliberately does anything to make sure other parts of the
device work. If it _does_ work, I'd assume it's really by chance.

> Don't buffer requests.  Either allow the internal FIFOs to fill up or
> else reject everything.  Any reasonable host will start getting timeout
> expirations and will realize that something is wrong.

Right, but if we allow this, I can already see folks abusing to connect
to the host early and only when necessary do some trickery to e.g. start
adbd (not saying Android will do this, just using it as an easy
example).

Sure, we can deactivate and only activate when files are opened but is
there any guarantee that when a process receives segfault that we will
have, from FFS point of view, any information to know that the thing
crashed ? I mean, a userland application can register its own handler
for SIGSEGV/SIGKILL, right ? And that handler could very well just call
close() on all file descriptors. Then how do we differentiate a normal
close() from a "oh-crap-I-died" close() ?

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH] usb: gadget: f_fs: add "zombie" mode

[Alan Stern]

On Tue, 7 Oct 2014, Felipe Balbi wrote:

> > It seems to me that we should imitate what an ordinary USB device would
> > do.  If part of the firmware crashes, generally you would expect none
> > of the endpoints associated with that function to work.  Either they
> > refuse to accept output from the host or they stall everything.  But
> > endpoints associated with other parts of the firmware might very well
> > continue to work okay.
> 
> dunno, I have never seen a USB device firmware crash and I don't think
> anybody deliberately does anything to make sure other parts of the
> device work. If it _does_ work, I'd assume it's really by chance.

I've seen it happen lots of times, but only on single-function devices.  
When it somes to multi-function devices, who knows?

Still, with the single-function devices, firmware crashes generally 
don't lead to disconnections.  Sometimes they do, but usually they 
don't.

> > Don't buffer requests.  Either allow the internal FIFOs to fill up or
> > else reject everything.  Any reasonable host will start getting timeout
> > expirations and will realize that something is wrong.
> 
> Right, but if we allow this, I can already see folks abusing to connect
> to the host early and only when necessary do some trickery to e.g. start
> adbd (not saying Android will do this, just using it as an easy
> example).

We can still keep the pullup turned off until all the functions are
ready.  That's a part of normal behavior -- unlike what happens when a
userspace component crashes or is killed.

> Sure, we can deactivate and only activate when files are opened but is
> there any guarantee that when a process receives segfault that we will
> have, from FFS point of view, any information to know that the thing
> crashed ? I mean, a userland application can register its own handler
> for SIGSEGV/SIGKILL, right ? And that handler could very well just call
> close() on all file descriptors. Then how do we differentiate a normal
> close() from a "oh-crap-I-died" close() ?

We can't, so why worry about it?

If a file handle was closed for normal reasons then userspace probably 
in the middle of shutting down the gadget anyway.  If not then the 
user will get what they deserve.

If the file handle was closed for abnormal reasons, we can behave like 
crashed firmware.  Which means, in the end, doing the same thing as in 
the normal-reason case -- i.e., do nothing.  In particular, don't 
disconnect.

If you want to allow for the possibility of orderly shutdown (and maybe 
even possible restart) of a userspace handler, the function library 
should first tell the kernel explicitly to disconnect.  Then function 
components can be changed around completely, and when everything is 
ready, userspace can tell the kernel to connect again.

Alan Stern

Re: [PATCH] usb: gadget: f_fs: add "zombie" mode

[Felipe Balbi]

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

Hi,

On Tue, Oct 07, 2014 at 02:42:33PM -0400, Alan Stern wrote:
> > > It seems to me that we should imitate what an ordinary USB device would
> > > do.  If part of the firmware crashes, generally you would expect none
> > > of the endpoints associated with that function to work.  Either they
> > > refuse to accept output from the host or they stall everything.  But
> > > endpoints associated with other parts of the firmware might very well
> > > continue to work okay.
> > 
> > dunno, I have never seen a USB device firmware crash and I don't think
> > anybody deliberately does anything to make sure other parts of the
> > device work. If it _does_ work, I'd assume it's really by chance.
> 
> I've seen it happen lots of times, but only on single-function devices.  
> When it somes to multi-function devices, who knows?
> 
> Still, with the single-function devices, firmware crashes generally 
> don't lead to disconnections.  Sometimes they do, but usually they 
> don't.
> 
> > > Don't buffer requests.  Either allow the internal FIFOs to fill up or
> > > else reject everything.  Any reasonable host will start getting timeout
> > > expirations and will realize that something is wrong.
> > 
> > Right, but if we allow this, I can already see folks abusing to connect
> > to the host early and only when necessary do some trickery to e.g. start
> > adbd (not saying Android will do this, just using it as an easy
> > example).
> 
> We can still keep the pullup turned off until all the functions are
> ready.  That's a part of normal behavior -- unlike what happens when a
> userspace component crashes or is killed.
> 
> > Sure, we can deactivate and only activate when files are opened but is
> > there any guarantee that when a process receives segfault that we will
> > have, from FFS point of view, any information to know that the thing
> > crashed ? I mean, a userland application can register its own handler
> > for SIGSEGV/SIGKILL, right ? And that handler could very well just call
> > close() on all file descriptors. Then how do we differentiate a normal
> > close() from a "oh-crap-I-died" close() ?
> 
> We can't, so why worry about it?

because on close(), I want to disconnect data pullups :-) Everything has
been tore down and there's nothing else to do.

> If a file handle was closed for normal reasons then userspace probably 
> in the middle of shutting down the gadget anyway.  If not then the 
> user will get what they deserve.

yeah, I think the same way about a crashing functionfs daemon :-)

> If the file handle was closed for abnormal reasons, we can behave like 
> crashed firmware.  Which means, in the end, doing the same thing as in 
> the normal-reason case -- i.e., do nothing.  In particular, don't 
> disconnect.
> 
> If you want to allow for the possibility of orderly shutdown (and maybe 
> even possible restart) of a userspace handler, the function library 
> should first tell the kernel explicitly to disconnect.  Then function 
> components can be changed around completely, and when everything is 
> ready, userspace can tell the kernel to connect again.

I still feel iffy about it, but I must say I understand where you're
coming from. It's weird to force a disconnect, sure. I guess we could
accept this with a new option (just not 'zombie', perhaps no_disconnect
:-) but only if we still have the same "delay pullups until daemon is
running" requirement.

/me hides

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH] usb: gadget: f_fs: add "zombie" mode

[Alan Stern]

On Tue, 7 Oct 2014, Felipe Balbi wrote:

> > If the file handle was closed for abnormal reasons, we can behave like 
> > crashed firmware.  Which means, in the end, doing the same thing as in 
> > the normal-reason case -- i.e., do nothing.  In particular, don't 
> > disconnect.
> > 
> > If you want to allow for the possibility of orderly shutdown (and maybe 
> > even possible restart) of a userspace handler, the function library 
> > should first tell the kernel explicitly to disconnect.  Then function 
> > components can be changed around completely, and when everything is 
> > ready, userspace can tell the kernel to connect again.
> 
> I still feel iffy about it, but I must say I understand where you're
> coming from. It's weird to force a disconnect, sure. I guess we could

Well, it's not all that weird.  Devices disconnect automatically when
they receive a firmware update, so that they can reconnect with new
descriptors.  Much the same thing should happen if the user wants to
replace one function driver with a different one.

I guess the real idea is to give the user a choice of disconnecting or 
not.  Don't always force the whole device to disconnect when one of the 
function drivers goes away.

> accept this with a new option (just not 'zombie', perhaps no_disconnect
> :-) but only if we still have the same "delay pullups until daemon is
> running" requirement.

Seems reasonable to me.  Or something that can be adjusted while the 
library is running, as opposed to setting an option once when the 
library starts.

> /me hides

I'm out of further ideas.  What do the library designers think?

Alan Stern

Re: [PATCH] usb: gadget: f_fs: add "zombie" mode

[Michal Nazarewicz]

> On Tue, 7 Oct 2014, Felipe Balbi wrote:
>> Right, but if we allow this, I can already see folks abusing to
>> connect to the host early and only when necessary do some trickery to
>> e.g. start adbd (not saying Android will do this, just using it as an
>> easy example).

I don't really see that happening.  For the gadget to start all
descriptors need to be known.  Functionfs  will know the descriptors
only once the user space daemon provides them.  Therefore, with the
current features (or even with addition of Robert's feature) there is no
way to let the gadget start without having the daemon running.

On Tue, Oct 07 2014, Alan Stern <stern@rowland.harvard.edu> wrote:
> We can still keep the pullup turned off until all the functions are
> ready.  That's a part of normal behavior -- unlike what happens when a
> userspace component crashes or is killed.

>> Then how do we differentiate a normal close() from a "oh-crap-I-died"
>> close() ?

> We can't, so why worry about it?

We actually might be able to distinguish those cases with another ioctl
which daemon must issue on the ep0 prior to closing it.  I'm not saying
that we should implement that though.

> If a file handle was closed for normal reasons then userspace probably 
> in the middle of shutting down the gadget anyway.  If not then the 
> user will get what they deserve.
>
> If the file handle was closed for abnormal reasons, we can behave like 
> crashed firmware.  Which means, in the end, doing the same thing as in 
> the normal-reason case -- i.e., do nothing.  In particular, don't 
> disconnect.
>
> If you want to allow for the possibility of orderly shutdown (and maybe 
> even possible restart) of a userspace handler, the function library 
> should first tell the kernel explicitly to disconnect.  Then function 
> components can be changed around completely, and when everything is 
> ready, userspace can tell the kernel to connect again.

I'm wondering if it would be possible to support user-space daemon
restarts with O_APPEND flag.  This is probably looking too far to the
future though.

-- 
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: f_fs: add "zombie" mode

[Krzysztof Opasiak]

Hi,

> -----Original Message-----
> From: Mike Nazarewicz [mailto:mpn@google.com] On Behalf Of Michal
> Nazarewicz
> Sent: Tuesday, October 07, 2014 10:08 PM
> To: Alan Stern; Felipe Balbi
> Cc: Krzysztof Opasiak; 'Robert Baldyga';
> gregkh@linuxfoundation.org; linux-usb@vger.kernel.org; linux-
> kernel@vger.kernel.org; andrzej.p@samsung.com
> Subject: Re: [PATCH] usb: gadget: f_fs: add "zombie" mode
> 
> > On Tue, 7 Oct 2014, Felipe Balbi wrote:
> >> Right, but if we allow this, I can already see folks abusing to
> >> connect to the host early and only when necessary do some
> trickery to
> >> e.g. start adbd (not saying Android will do this, just using it
> as an
> >> easy example).
> 
> I don't really see that happening.  For the gadget to start all
> descriptors need to be known.  Functionfs  will know the
> descriptors
> only once the user space daemon provides them.  Therefore, with the
> current features (or even with addition of Robert's feature) there
> is no
> way to let the gadget start without having the daemon running.

Well, to be honest we do some lazy daemon startup in gadgetd. The idea is to provide functionality quite similar to inet. So we have divided functionfs services into two parts:
- Descriptors - provided in configuration file
- function implementation - provided in binary

Now user can create ffs function using gadgetd without worrying about mounting the file system, running daemon and many other stuff. Gadgetd is system-wide usb gadget manager which provides abstraction layer for kernel functions and ffs-based functions.

Example:
User would like to create gadget which contains MTP in first configuration and ADB in second. When gadgetd receives such request via DBUS it creates suitable functions on configfs, find suitable configuration files, mount two instances of ffs, write descriptors from config files and run poll() on both ep0. Please notice here that any other daemon has not been run but the whole gadget can be bound to UDC.

When usb device is connected to host then host will select one of available configurations. All functions in that configuration receive ENABLE event. When gadgetd receives such event from one of ep0 then fork() is executed and desired service is being run with all file descriptors opened and ready to use. Please also notice here that if  host select first configuration, only one of those daemon is going to be run.

(...)

-- 
Krzysztof Opasiak
Samsung R&D Institute Poland
Samsung Electronics
k.opasiak@samsung.com

Re: [PATCH] usb: gadget: f_fs: add "zombie" mode

[Michal Nazarewicz]

>> -----Original Message-----
>> From: Mike Nazarewicz [mailto:mpn@google.com]
>> I don't really see that happening.  For the gadget to start all
>> descriptors need to be known.  Functionfs will know the descriptors
>> only once the user space daemon provides them.  Therefore, with the
>> current features (or even with addition of Robert's feature) there is
>> no way to let the gadget start without having the daemon running.

On Wed, Oct 08 2014, Krzysztof Opasiak <k.opasiak@samsung.com> wrote:
> Well, to be honest we do some lazy daemon startup in gadgetd. The idea
> is to provide functionality quite similar to inet. So we have divided
> functionfs services into two parts:
> - Descriptors - provided in configuration file
> - function implementation - provided in binary

Sure, and I'm not surprised to hear that has been implemented, but from
kernel point of view, the daemon is there and running.  Furthermore,
such behaviour is possible with or without the zombie feature, and in
fact kernel isn't able to prevent it, so it's immaterial to discussion
of the zombie feature.

-- 
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: f_fs: add "zombie" mode

[Alan Stern]

On Tue, 7 Oct 2014, Michal Nazarewicz wrote:

> > On Tue, 7 Oct 2014, Felipe Balbi wrote:
> >> Right, but if we allow this, I can already see folks abusing to
> >> connect to the host early and only when necessary do some trickery to
> >> e.g. start adbd (not saying Android will do this, just using it as an
> >> easy example).
> 
> I don't really see that happening.  For the gadget to start all
> descriptors need to be known.  Functionfs  will know the descriptors
> only once the user space daemon provides them.  Therefore, with the
> current features (or even with addition of Robert's feature) there is no
> way to let the gadget start without having the daemon running.
> 
> On Tue, Oct 07 2014, Alan Stern <stern@rowland.harvard.edu> wrote:
> > We can still keep the pullup turned off until all the functions are
> > ready.  That's a part of normal behavior -- unlike what happens when a
> > userspace component crashes or is killed.
> 
> >> Then how do we differentiate a normal close() from a "oh-crap-I-died"
> >> close() ?
> 
> > We can't, so why worry about it?
> 
> We actually might be able to distinguish those cases with another ioctl
> which daemon must issue on the ep0 prior to closing it.  I'm not saying
> that we should implement that though.
> 
> > If a file handle was closed for normal reasons then userspace probably 
> > in the middle of shutting down the gadget anyway.  If not then the 
> > user will get what they deserve.
> >
> > If the file handle was closed for abnormal reasons, we can behave like 
> > crashed firmware.  Which means, in the end, doing the same thing as in 
> > the normal-reason case -- i.e., do nothing.  In particular, don't 
> > disconnect.
> >
> > If you want to allow for the possibility of orderly shutdown (and maybe 
> > even possible restart) of a userspace handler, the function library 
> > should first tell the kernel explicitly to disconnect.  Then function 
> > components can be changed around completely, and when everything is 
> > ready, userspace can tell the kernel to connect again.
> 
> I'm wondering if it would be possible to support user-space daemon
> restarts with O_APPEND flag.  This is probably looking too far to the
> future though.

Actually, we shouldn't need to consider the case where the descriptors
change.  That _always_ requires a disconnect, and the user can cause a
disconnect simply by killing the daemon and starting it again.  No
separate restart capability is needed.

Alan Stern

Re: [PATCH] usb: gadget: f_fs: add "zombie" mode

[Michal Nazarewicz]

>> On Tue, Oct 07 2014, Alan Stern <stern@rowland.harvard.edu> wrote:
>>> If you want to allow for the possibility of orderly shutdown (and maybe 
>>> even possible restart) of a userspace handler, the function library 
>>> should first tell the kernel explicitly to disconnect.

> On Tue, 7 Oct 2014, Michal Nazarewicz wrote:
>> I'm wondering if it would be possible to support user-space daemon
>> restarts with O_APPEND flag.  This is probably looking too far to the
>> future though.

On Wed, Oct 08 2014, Alan Stern <stern@rowland.harvard.edu> wrote:
> Actually, we shouldn't need to consider the case where the descriptors
> change.  That _always_ requires a disconnect, and the user can cause
> a disconnect simply by killing the daemon and starting it again.  No
> separate restart capability is needed.

Correct.  This may be going a bit off-topic, but I was thinking of
a possible feature that would allow the daemon to indicate to kernel it
is ready to pick up the pieces after its previous instance crashed.
This would require the zombie mode to be implemented.

* Currently, once the daemon finishes or crashes, USB disconnect
  happens.

* In zombie mode, I could imagine the following scenarios:
  - daemon crashes, but the gadget still works, no disconnect happens;
  - daemon opens ep0 with O_APPEND, no disconnect happens;
  - daemon sends *the same* descriptors as before;
  - kernel recreates all the ep# files and let the daemon continue
    handling USB requests with host possibly never noticing.

Opening ep0 w/o O_APPEND or sending different descriptors would cause
a disconnect.  With the above, user-space would be able to force gadget
to disconnect by killing the daemon and then doing
    printf '' >/dev/functionfs/ep0
  
So I guess my point is that with zombie mode, user space could tell the
kernel to not-disconnect (rather than having an explicit disconnect
request) if it was written in a way that supports crash recovery.

This is a wishful thinking at this stage I guess, but perhaps it's worth
considering when deciding how the zombie interface should look like.
For example, I have some concerns if it should be enabled by an fs mount
option.

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