Since we can compose gadgets from many functions, there is the problem
related to gadget breakage while FunctionFS daemon being closed. 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 to keep entire gadget working is to make
broken FFS function deactivated but still visible to host. For this
purpose this patch introduces "no_disconnect" mode. It can be enabled
by setting mount option "no_disconnect=1", and results with defering
function disconnect to the moment of reopen ep0 file or filesystem
unmount. After closing all endpoint files, FunctionFS is set to state
FFS_DEACTIVATED.
When ffs->state == FFS_DEACTIVATED:
- function is still bound and visible to host,
- setup requests are automatically stalled,
- transfers on other endpoints are refused,
- epfiles, except ep0, are deleted from the filesystem,
- opening ep0 causes the function to be closes, and then FunctionFS
is ready for descriptors and string write,
- unmounting of the FunctionFS instance causes the function to be closed.
Signed-off-by: Robert Baldyga <[email protected]>
---
Changelog:
v4:
- use ffs_data_reset() instead of ffs_data_clear() to reset ffs data
properly after ffs->ref refcount reach 0 (or under in no_disconnect
mode) in ffs_data_put() function
v3: https://lkml.org/lkml/2014/10/9/170
- change option name to more descriptive and less scary,
- fix cleaning up on unmount (call ffs_data_closed() in ffs_fs_kill_sb(),
and ffs_data_clear() in ffs_data_closed() if ffs->opened is negative).
v2: https://lkml.org/lkml/2014/10/7/109
- delete epfiles, excepting ep0, when FFS is in "zombie" mode,
- add description of FFS_ZOMBIE state,
- minor cleanups.
v1: https://lkml.org/lkml/2014/10/6/128
drivers/usb/gadget/function/f_fs.c | 42 +++++++++++++++++++++++++++++++-------
drivers/usb/gadget/function/u_fs.h | 22 ++++++++++++++++++++
2 files changed, 57 insertions(+), 7 deletions(-)
diff --git a/drivers/usb/gadget/function/f_fs.c b/drivers/usb/gadget/function/f_fs.c
index 12dbdaf..2d47d4a 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_DEACTIVATED:
+ 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 no_disconnect;
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 13:
+ if (!memcmp(opts, "no_disconnect", 13))
+ data->no_disconnect = !!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,
+ .no_disconnect = 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->no_disconnect = data.no_disconnect;
ffs->dev_name = kstrdup(dev_name, GFP_KERNEL);
if (unlikely(!ffs->dev_name)) {
@@ -1333,6 +1344,7 @@ ffs_fs_kill_sb(struct super_block *sb)
kill_litter_super(sb);
if (sb->s_fs_info) {
ffs_release_dev(sb->s_fs_info);
+ ffs_data_closed(sb->s_fs_info);
ffs_data_put(sb->s_fs_info);
}
}
@@ -1389,7 +1401,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_DEACTIVATED)
+ ffs_data_reset(ffs);
}
static void ffs_data_put(struct ffs_data *ffs)
@@ -1411,9 +1425,22 @@ 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->no_disconnect) {
+ ffs->state = FFS_DEACTIVATED;
+ if (ffs->epfiles) {
+ ffs_epfiles_destroy(ffs->epfiles,
+ ffs->eps_count);
+ ffs->epfiles = NULL;
+ }
+ if (ffs->setup_state == FFS_SETUP_PENDING)
+ __ffs_ep0_stall(ffs);
+ } else {
+ ffs->state = FFS_CLOSING;
+ ffs_data_reset(ffs);
+ }
}
+ if (atomic_read(&ffs->opened) < 0)
+ ffs_data_reset(ffs);
ffs_data_put(ffs);
}
@@ -1588,7 +1615,6 @@ static void ffs_epfiles_destroy(struct ffs_epfile *epfiles, unsigned count)
kfree(epfiles);
}
-
static void ffs_func_eps_disable(struct ffs_function *func)
{
struct ffs_ep *ep = func->eps;
@@ -1601,10 +1627,12 @@ static void ffs_func_eps_disable(struct ffs_function *func)
/* pending requests get nuked */
if (likely(ep->ep))
usb_ep_disable(ep->ep);
- epfile->ep = NULL;
-
++ep;
- ++epfile;
+
+ if (epfile) {
+ epfile->ep = NULL;
+ ++epfile;
+ }
} while (--count);
spin_unlock_irqrestore(&func->ffs->eps_lock, flags);
}
diff --git a/drivers/usb/gadget/function/u_fs.h b/drivers/usb/gadget/function/u_fs.h
index cd128e3..7bc0ca2 100644
--- a/drivers/usb/gadget/function/u_fs.h
+++ b/drivers/usb/gadget/function/u_fs.h
@@ -93,6 +93,26 @@ enum ffs_state {
FFS_ACTIVE,
/*
+ * Function is visible to host, but it's not functional. All
+ * setup requests are stalled and transfers on another endpoints
+ * are refused. All epfiles, except ep0, are deleted so there
+ * is no way to perform any operations on them.
+ *
+ * This state is set after closing all functionfs files, when
+ * mount parameter "no_disconnect=1" has been set. Function will
+ * remain in deactivated state until filesystem is umounted or
+ * ep0 is opened again. In the second case functionfs state will
+ * be reset, and it will be ready for descriptors and strings
+ * writing.
+ *
+ * This is useful only when functionfs is composed to gadget
+ * with another function which can perform some critical
+ * operations, and it's strongly desired to have this operations
+ * completed, even after functionfs files closure.
+ */
+ FFS_DEACTIVATED,
+
+ /*
* All endpoints have been closed. This state is also set if
* we encounter an unrecoverable error. The only
* unrecoverable error is situation when after reading strings
@@ -251,6 +271,8 @@ struct ffs_data {
kgid_t gid;
} file_perms;
+ bool no_disconnect;
+
/*
* The endpoint files, filled by ffs_epfiles_create(),
* destroyed by ffs_epfiles_destroy().
--
1.9.1
Hi,
On Thu, Oct 09, 2014 at 03:21:51PM +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. 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 to keep entire gadget working is to make
> broken FFS function deactivated but still visible to host. For this
> purpose this patch introduces "no_disconnect" mode. It can be enabled
> by setting mount option "no_disconnect=1", and results with defering
> function disconnect to the moment of reopen ep0 file or filesystem
> unmount. After closing all endpoint files, FunctionFS is set to state
> FFS_DEACTIVATED.
>
> When ffs->state == FFS_DEACTIVATED:
> - function is still bound and visible to host,
> - setup requests are automatically stalled,
> - transfers on other endpoints are refused,
> - epfiles, except ep0, are deleted from the filesystem,
> - opening ep0 causes the function to be closes, and then FunctionFS
> is ready for descriptors and string write,
> - unmounting of the FunctionFS instance causes the function to be closed.
>
> Signed-off-by: Robert Baldyga <[email protected]>
David, you have been messing with ffs lately, can I get a Tested-by from
you on this patch ?
> ---
>
> Changelog:
>
> v4:
> - use ffs_data_reset() instead of ffs_data_clear() to reset ffs data
> properly after ffs->ref refcount reach 0 (or under in no_disconnect
> mode) in ffs_data_put() function
>
> v3: https://lkml.org/lkml/2014/10/9/170
> - change option name to more descriptive and less scary,
> - fix cleaning up on unmount (call ffs_data_closed() in ffs_fs_kill_sb(),
> and ffs_data_clear() in ffs_data_closed() if ffs->opened is negative).
>
> v2: https://lkml.org/lkml/2014/10/7/109
> - delete epfiles, excepting ep0, when FFS is in "zombie" mode,
> - add description of FFS_ZOMBIE state,
> - minor cleanups.
>
> v1: https://lkml.org/lkml/2014/10/6/128
>
> drivers/usb/gadget/function/f_fs.c | 42 +++++++++++++++++++++++++++++++-------
> drivers/usb/gadget/function/u_fs.h | 22 ++++++++++++++++++++
> 2 files changed, 57 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/usb/gadget/function/f_fs.c b/drivers/usb/gadget/function/f_fs.c
> index 12dbdaf..2d47d4a 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_DEACTIVATED:
> + 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 no_disconnect;
> 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 13:
> + if (!memcmp(opts, "no_disconnect", 13))
> + data->no_disconnect = !!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,
> + .no_disconnect = 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->no_disconnect = data.no_disconnect;
>
> ffs->dev_name = kstrdup(dev_name, GFP_KERNEL);
> if (unlikely(!ffs->dev_name)) {
> @@ -1333,6 +1344,7 @@ ffs_fs_kill_sb(struct super_block *sb)
> kill_litter_super(sb);
> if (sb->s_fs_info) {
> ffs_release_dev(sb->s_fs_info);
> + ffs_data_closed(sb->s_fs_info);
> ffs_data_put(sb->s_fs_info);
> }
> }
> @@ -1389,7 +1401,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_DEACTIVATED)
> + ffs_data_reset(ffs);
> }
>
> static void ffs_data_put(struct ffs_data *ffs)
> @@ -1411,9 +1425,22 @@ 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->no_disconnect) {
> + ffs->state = FFS_DEACTIVATED;
> + if (ffs->epfiles) {
> + ffs_epfiles_destroy(ffs->epfiles,
> + ffs->eps_count);
> + ffs->epfiles = NULL;
> + }
> + if (ffs->setup_state == FFS_SETUP_PENDING)
> + __ffs_ep0_stall(ffs);
> + } else {
> + ffs->state = FFS_CLOSING;
> + ffs_data_reset(ffs);
> + }
> }
> + if (atomic_read(&ffs->opened) < 0)
> + ffs_data_reset(ffs);
>
> ffs_data_put(ffs);
> }
> @@ -1588,7 +1615,6 @@ static void ffs_epfiles_destroy(struct ffs_epfile *epfiles, unsigned count)
> kfree(epfiles);
> }
>
> -
> static void ffs_func_eps_disable(struct ffs_function *func)
> {
> struct ffs_ep *ep = func->eps;
> @@ -1601,10 +1627,12 @@ static void ffs_func_eps_disable(struct ffs_function *func)
> /* pending requests get nuked */
> if (likely(ep->ep))
> usb_ep_disable(ep->ep);
> - epfile->ep = NULL;
> -
> ++ep;
> - ++epfile;
> +
> + if (epfile) {
> + epfile->ep = NULL;
> + ++epfile;
> + }
> } while (--count);
> spin_unlock_irqrestore(&func->ffs->eps_lock, flags);
> }
> diff --git a/drivers/usb/gadget/function/u_fs.h b/drivers/usb/gadget/function/u_fs.h
> index cd128e3..7bc0ca2 100644
> --- a/drivers/usb/gadget/function/u_fs.h
> +++ b/drivers/usb/gadget/function/u_fs.h
> @@ -93,6 +93,26 @@ enum ffs_state {
> FFS_ACTIVE,
>
> /*
> + * Function is visible to host, but it's not functional. All
> + * setup requests are stalled and transfers on another endpoints
> + * are refused. All epfiles, except ep0, are deleted so there
> + * is no way to perform any operations on them.
> + *
> + * This state is set after closing all functionfs files, when
> + * mount parameter "no_disconnect=1" has been set. Function will
> + * remain in deactivated state until filesystem is umounted or
> + * ep0 is opened again. In the second case functionfs state will
> + * be reset, and it will be ready for descriptors and strings
> + * writing.
> + *
> + * This is useful only when functionfs is composed to gadget
> + * with another function which can perform some critical
> + * operations, and it's strongly desired to have this operations
> + * completed, even after functionfs files closure.
> + */
> + FFS_DEACTIVATED,
> +
> + /*
> * All endpoints have been closed. This state is also set if
> * we encounter an unrecoverable error. The only
> * unrecoverable error is situation when after reading strings
> @@ -251,6 +271,8 @@ struct ffs_data {
> kgid_t gid;
> } file_perms;
>
> + bool no_disconnect;
> +
> /*
> * The endpoint files, filled by ffs_epfiles_create(),
> * destroyed by ffs_epfiles_destroy().
> --
> 1.9.1
>
--
balbi
On Mon, Nov 03, 2014 at 10:11:02AM -0600, Felipe Balbi wrote:
> Hi,
>
> On Thu, Oct 09, 2014 at 03:21:51PM +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. 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 to keep entire gadget working is to make
> > broken FFS function deactivated but still visible to host. For this
> > purpose this patch introduces "no_disconnect" mode. It can be enabled
> > by setting mount option "no_disconnect=1", and results with defering
> > function disconnect to the moment of reopen ep0 file or filesystem
> > unmount. After closing all endpoint files, FunctionFS is set to state
> > FFS_DEACTIVATED.
> >
> > When ffs->state == FFS_DEACTIVATED:
> > - function is still bound and visible to host,
> > - setup requests are automatically stalled,
> > - transfers on other endpoints are refused,
> > - epfiles, except ep0, are deleted from the filesystem,
> > - opening ep0 causes the function to be closes, and then FunctionFS
> > is ready for descriptors and string write,
> > - unmounting of the FunctionFS instance causes the function to be closed.
> >
> > Signed-off-by: Robert Baldyga <[email protected]>
>
> David, you have been messing with ffs lately, can I get a Tested-by from
> you on this patch ?
ping. David ? Any chance you can test this one out ?
--
balbi
HI,
On Wed, Nov 05, 2014 at 01:43:21PM -0600, Felipe Balbi wrote:
> On Mon, Nov 03, 2014 at 10:11:02AM -0600, Felipe Balbi wrote:
> > Hi,
> >
> > On Thu, Oct 09, 2014 at 03:21:51PM +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. 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 to keep entire gadget working is to make
> > > broken FFS function deactivated but still visible to host. For this
> > > purpose this patch introduces "no_disconnect" mode. It can be enabled
> > > by setting mount option "no_disconnect=1", and results with defering
> > > function disconnect to the moment of reopen ep0 file or filesystem
> > > unmount. After closing all endpoint files, FunctionFS is set to state
> > > FFS_DEACTIVATED.
> > >
> > > When ffs->state == FFS_DEACTIVATED:
> > > - function is still bound and visible to host,
> > > - setup requests are automatically stalled,
> > > - transfers on other endpoints are refused,
> > > - epfiles, except ep0, are deleted from the filesystem,
> > > - opening ep0 causes the function to be closes, and then FunctionFS
> > > is ready for descriptors and string write,
> > > - unmounting of the FunctionFS instance causes the function to be closed.
> > >
> > > Signed-off-by: Robert Baldyga <[email protected]>
> >
> > David, you have been messing with ffs lately, can I get a Tested-by from
> > you on this patch ?
>
> ping. David ? Any chance you can test this one out ?
a reminder on this request, it's getting close to tree lockdown
(probably another 2 to 3 weeks).
--
balbi
Hello,
> -----Original Message-----
> From: Felipe Balbi [mailto:[email protected]]
> Sent: Thursday, November 13, 2014 3:44 AM
> To: Felipe Balbi
> Cc: Robert Baldyga; David Cohen; [email protected]; linux-
> [email protected]; [email protected];
> [email protected]; [email protected]; [email protected];
> [email protected]
> Subject: Re: [PATCH v4] usb: gadget: f_fs: add "no_disconnect" mode
(...)
> > > David, you have been messing with ffs lately, can I get a
> Tested-by
> > > from you on this patch ?
> >
> > ping. David ? Any chance you can test this one out ?
>
> a reminder on this request, it's getting close to tree lockdown
> (probably another 2 to 3 weeks).
I have tested this patch today but I'm afraid that I can't give
tested-by tag:
When I kill the daemon and then reconnect my device I get on my host:
[776092.877175] usb 1-1.1: can't set config #1, error -32
While on device there is only:
[ 2147.035340] configfs-gadget gadget: high-speed config #1: The only
one
Moreover, I'm not very enthusiastic about the convention in which we
allow
to reconnect the device without any changes.
In my opinion when daemon has been killed we should allow user to finish
communication with other functions and then simply unbind our gadget.
This should happen not only on reconnection but also when host is trying
to enable configuration which contains this function "zombie".
This function is not fully operational so it is very good idea to
allow user to finish his job with other functions but when he reconnects
the
device we should unbind gadget to notify userspace that something is
wrong.
The same when we switch configuration.
What do you think about this Michal and Felipe?
--
Krzysiek
On Thu, Nov 13, 2014 at 05:28:22PM +0100, Krzysztof Opasiak wrote:
> Hello,
>
> > -----Original Message-----
> > From: Felipe Balbi [mailto:[email protected]]
> > Sent: Thursday, November 13, 2014 3:44 AM
> > To: Felipe Balbi
> > Cc: Robert Baldyga; David Cohen; [email protected]; linux-
> > [email protected]; [email protected];
> > [email protected]; [email protected]; [email protected];
> > [email protected]
> > Subject: Re: [PATCH v4] usb: gadget: f_fs: add "no_disconnect" mode
>
> (...)
>
> > > > David, you have been messing with ffs lately, can I get a
> > Tested-by
> > > > from you on this patch ?
> > >
> > > ping. David ? Any chance you can test this one out ?
> >
> > a reminder on this request, it's getting close to tree lockdown
> > (probably another 2 to 3 weeks).
>
> I have tested this patch today but I'm afraid that I can't give
> tested-by tag:
>
> When I kill the daemon and then reconnect my device I get on my host:
>
> [776092.877175] usb 1-1.1: can't set config #1, error -32
>
> While on device there is only:
>
> [ 2147.035340] configfs-gadget gadget: high-speed config #1: The only
> one
>
> Moreover, I'm not very enthusiastic about the convention in which we
> allow
> to reconnect the device without any changes.
>
> In my opinion when daemon has been killed we should allow user to finish
> communication with other functions and then simply unbind our gadget.
> This should happen not only on reconnection but also when host is trying
> to enable configuration which contains this function "zombie".
> This function is not fully operational so it is very good idea to
> allow user to finish his job with other functions but when he reconnects
> the
> device we should unbind gadget to notify userspace that something is
> wrong.
> The same when we switch configuration.
>
> What do you think about this Michal and Felipe?
let's wait another merge window to discuss this one better. If it's
causing confusion/regressions, we better delay it.
--
balbi