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

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

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