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@xxxxxxxxxx>
---
drivers/usb/gadget/function/f_fs.c | 79 ++++++++------------------------------
drivers/usb/gadget/function/u_fs.h | 11 +++---
2 files changed, 22 insertions(+), 68 deletions(-)
On Tue, Jan 31 2017, Fabian Vogt wrote:
> issue still appears reliably after every use of ffs.
So far I came up with the below even though I don’t believe it fixes the
issue. I probably should finally figure out how to use dummy_hcd so
I can test such things…
diff --git a/drivers/usb/gadget/function/f_fs.c b/drivers/usb/gadget/function/f_fs.c
index 5e746adc8a2d..6333e64f1bc7 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 60139854e0b1..b028aeefe1b6 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);
--
Best regards
ミハウ “𝓶𝓲𝓷𝓪86” ナザレヴイツ
«If at first you don’t succeed, give up skydiving»
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
