Re: [PATCH] usb: gadget: f_fs: simplify ffs_dev name handling

[Felipe Balbi <felipe.balbi@linux.intel.com>]

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


Hi,

Michal Nazarewicz <mina86@mina86.com> writes:
> Currently ffs_dev::name can be either allocated by the client of
> the ffs_dev structure or by the f_fs.c core itself.  The former
> is used by g_ffs while the latter happens with configfs.
>
> Historically, g_ffs did not need to allocate separate buffer for
> the name so what is now f_fs.c core never cared about freeing
> that space.  With configfs the name needs to be copied since the
> memory is not guaranteed to be availeble after ffs_set_inst_name
> finishes.
>
> The complication is therefore here to avoid allocations in the
> g_ffs case but it complicates the code inproportinally to
> benefits it provides.  In particular, g_ffs is considered
> ‘legacy’ so optimising for its sake is unlikely to be worth the
> effort.
>
> With that observation in mind, simplify the code by unifying the
> code paths in g_ffs and configfs paths.  Furthermore, instead of
> allocating a new buffer for the name, simply embed it in the
> ffs_dev structure.  This further makes the memory management
> less convoluted and error-prone.
>
> The configfs interface for functionfs imposed a limit of 40
> characters for the name so this results in a 41-byte buffer
> added to the structure.  (For short names this may lead to
> wasted memory but the actual amount is not immediately obvious
> and depends on pointer size and which slab buckets the structure
> and name would fall into).
>
> Signed-off-by: Michal Nazarewicz <mina86@mina86.com>
> ---
>  drivers/usb/gadget/function/f_fs.c | 79 ++++++++------------------------------
>  drivers/usb/gadget/function/u_fs.h | 11 +++---
>  2 files changed, 22 insertions(+), 68 deletions(-)
>
> diff --git a/drivers/usb/gadget/function/f_fs.c b/drivers/usb/gadget/function/f_fs.c
> index f5d6bf527aac..6ed4da6e4474 100644
> --- a/drivers/usb/gadget/function/f_fs.c
> +++ b/drivers/usb/gadget/function/f_fs.c
> @@ -245,7 +245,6 @@ EXPORT_SYMBOL_GPL(ffs_lock);
>  
>  static struct ffs_dev *_ffs_find_dev(const char *name);
>  static struct ffs_dev *_ffs_alloc_dev(void);
> -static int _ffs_name_dev(struct ffs_dev *dev, const char *name);
>  static void _ffs_free_dev(struct ffs_dev *dev);
>  static void *ffs_acquire_dev(const char *dev_name);
>  static void ffs_release_dev(struct ffs_data *ffs_data);
> @@ -3277,13 +3276,13 @@ static LIST_HEAD(ffs_devices);
>  
>  static struct ffs_dev *_ffs_do_find_dev(const char *name)
>  {
> -	struct ffs_dev *dev;
> +	if (name) {

how about avoiding the extra indentation level?

	if (!name)
        	return NULL;

	[...]

-- 
balbi

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]