Message-Id: <20170301143005.23396-1-mina86@mina86.com>
Date:   Wed,  1 Mar 2017 15:30:05 +0100
From:   Michal Nazarewicz <mina86@...a86.com>
To:     Felipe Balbi <felipe.balbi@...ux.intel.com>
Cc:     linux-kernel@...r.kernel.org, linux-usb@...r.kernel.org,
        Michal Nazarewicz <mina86@...a86.com>
Subject: [PATCH] usb: gadget: f_fs: simplify ffs_dev name handling

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@...a86.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) {
+		struct ffs_dev *dev;
 
-	list_for_each_entry(dev, &ffs_devices, entry) {
-		if (!dev->name || !name)
-			continue;
-		if (strcmp(dev->name, name) == 0)
-			return dev;
+		list_for_each_entry(dev, &ffs_devices, entry) {
+			if (strcmp(dev->name, name) == 0)
+				return dev;
+		}
 	}
 
 	return NULL;
@@ -3357,42 +3356,11 @@ static void ffs_free_inst(struct usb_function_instance *f)
 	kfree(opts);
 }
 
-#define MAX_INST_NAME_LEN	40
-
 static int ffs_set_inst_name(struct usb_function_instance *fi, const char *name)
 {
-	struct f_fs_opts *opts;
-	char *ptr;
-	const char *tmp;
-	int name_len, ret;
-
-	name_len = strlen(name) + 1;
-	if (name_len > MAX_INST_NAME_LEN)
+	if (strlen(name) >= FIELD_SIZEOF(struct ffs_dev, name))
 		return -ENAMETOOLONG;
-
-	ptr = kstrndup(name, name_len, GFP_KERNEL);
-	if (!ptr)
-		return -ENOMEM;
-
-	opts = to_f_fs_opts(fi);
-	tmp = NULL;
-
-	ffs_dev_lock();
-
-	tmp = opts->dev->name_allocated ? opts->dev->name : NULL;
-	ret = _ffs_name_dev(opts->dev, ptr);
-	if (ret) {
-		kfree(ptr);
-		ffs_dev_unlock();
-		return ret;
-	}
-	opts->dev->name_allocated = true;
-
-	ffs_dev_unlock();
-
-	kfree(tmp);
-
-	return 0;
+	return ffs_name_dev(to_f_fs_opts(fi)->dev, name);
 }
 
 static struct usb_function_instance *ffs_alloc_inst(void)
@@ -3522,32 +3490,19 @@ static struct ffs_dev *_ffs_alloc_dev(void)
 	return dev;
 }
 
-/*
- * ffs_lock must be taken by the caller of this function
- * The caller is responsible for "name" being available whenever f_fs needs it
- */
-static int _ffs_name_dev(struct ffs_dev *dev, const char *name)
+int ffs_name_dev(struct ffs_dev *dev, const char *name)
 {
 	struct ffs_dev *existing;
+	int ret = 0;
 
-	existing = _ffs_do_find_dev(name);
-	if (existing)
-		return -EBUSY;
-
-	dev->name = name;
-
-	return 0;
-}
+	ffs_dev_lock();
 
-/*
- * The caller is responsible for "name" being available whenever f_fs needs it
- */
-int ffs_name_dev(struct ffs_dev *dev, const char *name)
-{
-	int ret;
+	existing = _ffs_do_find_dev(name);
+	if (!existing)
+		strlcpy(dev->name, name, ARRAY_SIZE(dev->name));
+	else if (existing != dev)
+		ret = -EBUSY;
 
-	ffs_dev_lock();
-	ret = _ffs_name_dev(dev, name);
 	ffs_dev_unlock();
 
 	return ret;
@@ -3577,8 +3532,6 @@ EXPORT_SYMBOL_GPL(ffs_single_dev);
 static void _ffs_free_dev(struct ffs_dev *dev)
 {
 	list_del(&dev->entry);
-	if (dev->name_allocated)
-		kfree(dev->name);
 
 	/* Clear the private_data pointer to stop incorrect dev access */
 	if (dev->ffs_data)
diff --git a/drivers/usb/gadget/function/u_fs.h b/drivers/usb/gadget/function/u_fs.h
index 4b6969451cdc..7ff485af3bee 100644
--- a/drivers/usb/gadget/function/u_fs.h
+++ b/drivers/usb/gadget/function/u_fs.h
@@ -39,15 +39,16 @@
 struct f_fs_opts;
 
 struct ffs_dev {
-	const char *name;
-	bool name_allocated;
-	bool mounted;
-	bool desc_ready;
-	bool single;
 	struct ffs_data *ffs_data;
 	struct f_fs_opts *opts;
 	struct list_head entry;
 
+	char name[41];
+
+	bool mounted;
+	bool desc_ready;
+	bool single;
+
 	int (*ffs_ready_callback)(struct ffs_data *ffs);
 	void (*ffs_closed_callback)(struct ffs_data *ffs);
 	void *(*ffs_acquire_dev_callback)(struct ffs_dev *dev);
-- 
2.12.0.rc1.440.g5b76565f74-goog

Powered by blists - more mailing lists