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

[PATCH v2] 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,
- epfiles, excepting ep0, are deleted from filesystem,
- 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>
---

Changelog:

v2:
- 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 | 38 ++++++++++++++++++++++++++++++++++----
 drivers/usb/gadget/function/u_fs.h | 22 ++++++++++++++++++++++
 2 files changed, 56 insertions(+), 4 deletions(-)

diff --git a/drivers/usb/gadget/function/f_fs.c b/drivers/usb/gadget/function/f_fs.c
index 7c6771d..b368b0a 100644
--- a/drivers/usb/gadget/function/f_fs.c
+++ b/drivers/usb/gadget/function/f_fs.c
@@ -162,6 +162,7 @@ struct ffs_desc_helper {
 };
 
 static int  __must_check ffs_epfiles_create(struct ffs_data *ffs);
+static void ffs_epfiles_delete(struct ffs_epfile *epfiles, unsigned count);
 static void ffs_epfiles_destroy(struct ffs_epfile *epfiles, unsigned count);
 
 static struct dentry *
@@ -606,6 +607,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 +1155,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 +1226,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 +1296,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 +1313,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 +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_ZOMBIE)
+			ffs_data_reset(ffs);
 }
 
 static void ffs_data_put(struct ffs_data *ffs)
@@ -1411,8 +1425,17 @@ 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->epfiles)
+				ffs_epfiles_delete(ffs->epfiles,
+						   ffs->eps_count);
+			if (ffs->setup_state == FFS_SETUP_PENDING)
+				__ffs_ep0_stall(ffs);
+		} else {
+			ffs->state = FFS_CLOSING;
+			ffs_data_reset(ffs);
+		}
 	}
 
 	ffs_data_put(ffs);
@@ -1569,7 +1592,7 @@ static int ffs_epfiles_create(struct ffs_data *ffs)
 	return 0;
 }
 
-static void ffs_epfiles_destroy(struct ffs_epfile *epfiles, unsigned count)
+static void ffs_epfiles_delete(struct ffs_epfile *epfiles, unsigned count)
 {
 	struct ffs_epfile *epfile = epfiles;
 
@@ -1584,6 +1607,13 @@ static void ffs_epfiles_destroy(struct ffs_epfile *epfiles, unsigned count)
 			epfile->dentry = NULL;
 		}
 	}
+}
+
+static void ffs_epfiles_destroy(struct ffs_epfile *epfiles, unsigned count)
+{
+	ENTER();
+
+	ffs_epfiles_delete(epfiles, count);
 
 	kfree(epfiles);
 }
diff --git a/drivers/usb/gadget/function/u_fs.h b/drivers/usb/gadget/function/u_fs.h
index cd128e3..bc16a02 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 another transfers are refused.
+	 * All epfiles, excepting 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 "zombie=1" has been set. Function will remain
+	 * in zombie state until filesystem will be umounted or ep0 will
+	 * be opened again. In the second case functionfs state will be
+	 * reseted, 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_ZOMBIE,
+
+	/*
 	 * 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 zombie_mode;
+
 	/*
 	 * The endpoint files, filled by ffs_epfiles_create(),
 	 * destroyed by ffs_epfiles_destroy().
-- 
1.9.1

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

[Michal Nazarewicz]

On Tue, Oct 07 2014, Robert Baldyga <r.baldyga@samsung.com> wrote:
> @@ -1411,8 +1425,17 @@ 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->epfiles)
> +				ffs_epfiles_delete(ffs->epfiles,
> +						   ffs->eps_count);

This looks suspicious.  Why isn't it:

+			if (ffs->epfiles) {
+				ffs_epfiles_destroy(ffs->epfiles,
+						   ffs->eps_count);
+				ffs->epfiles = NULL;
+			}

If ffs->epfiles is not NULLed, call to ffs_data_reset in ffs_data_open
will call ffs_epfiles_destroy which we don't want, do we?

> +			if (ffs->setup_state == FFS_SETUP_PENDING)
> +				__ffs_ep0_stall(ffs);
> +		} else {
> +			ffs->state = FFS_CLOSING;
> +			ffs_data_reset(ffs);
> +		}
>  	}
>  
>  	ffs_data_put(ffs);

> @@ -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 another transfers are refused.

“and transfers on other endpoints are refused.”

> +	 * All epfiles, excepting ep0, are deleted so there is no way

s/excepting/except/

> +	 * to perform any operations on them.
> +	 *
> +	 * This state is set after closing all functionfs files, when
> +	 * mount parameter "zombie=1" has been set. Function will remain
> +	 * in zombie state until filesystem will be umounted or ep0 will

s/will be umounted/is unmounted/
s/ep0 will be/ep0 is/

> +	 * be opened again. In the second case functionfs state will be
> +	 * reseted, and it will be ready for descriptors and strings

s/reseted/reset/

> +	 * 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_ZOMBIE,
> +
> +	/*
>  	 * 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

-- 
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 v2] usb: gadget: f_fs: add "zombie" mode

[Robert Baldyga]

On 10/07/2014 12:07 PM, Michal Nazarewicz wrote:
> On Tue, Oct 07 2014, Robert Baldyga <r.baldyga@samsung.com> wrote:
>> @@ -1411,8 +1425,17 @@ 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->epfiles)
>> +				ffs_epfiles_delete(ffs->epfiles,
>> +						   ffs->eps_count);
> 
> This looks suspicious.  Why isn't it:
> 
> +			if (ffs->epfiles) {
> +				ffs_epfiles_destroy(ffs->epfiles,
> +						   ffs->eps_count);
> +				ffs->epfiles = NULL;
> +			}
> 
> If ffs->epfiles is not NULLed, call to ffs_data_reset in ffs_data_open
> will call ffs_epfiles_destroy which we don't want, do we?
> 

We do. When epfile->dentry == NULL, ffs_epfiles_destroy() will do only
kfree(epfiles). We want to do it.

We don't want to have ffs->epfiles being equal NULL before calling
ffs_func_eps_disable(), which iterates on this table.

Hmm, we could also change ffs_func_eps_disable() to not touch
ffs->epfiles if it's NULL. It seems to be better solution.

>> +			if (ffs->setup_state == FFS_SETUP_PENDING)
>> +				__ffs_ep0_stall(ffs);
>> +		} else {
>> +			ffs->state = FFS_CLOSING;
>> +			ffs_data_reset(ffs);
>> +		}
>>  	}
>>  
>>  	ffs_data_put(ffs);
> 
>> @@ -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 another transfers are refused.
> 
> “and transfers on other endpoints are refused.”
> 
>> +	 * All epfiles, excepting ep0, are deleted so there is no way
> 
> s/excepting/except/
> 
>> +	 * to perform any operations on them.
>> +	 *
>> +	 * This state is set after closing all functionfs files, when
>> +	 * mount parameter "zombie=1" has been set. Function will remain
>> +	 * in zombie state until filesystem will be umounted or ep0 will
> 
> s/will be umounted/is unmounted/
> s/ep0 will be/ep0 is/
> 
>> +	 * be opened again. In the second case functionfs state will be
>> +	 * reseted, and it will be ready for descriptors and strings
> 
> s/reseted/reset/
> 
>> +	 * 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_ZOMBIE,
>> +
>> +	/*
>>  	 * 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
> 

Thanks,
Robert Baldyga