On Mon, 06 Feb 2012 09:24:38 +0100, Andrzej Pietrasiewicz <andrzej.p@xxxxxxxxxxx> wrote:
usb: gadget: FunctionFS: enable multiple USB function instances
Signed-off-by: Andrzej Pietrasiewicz <andrzej.p@xxxxxxxxxxx>
Signed-off-by: Kyungmin Park <kyungmin.park@xxxxxxxxxxx>
This patch enables using multiple instances of USB functions in FunctionFS.
Example usage (*):
On the gadget device:
$ insmod g_ffs.ko idVendor=<ID> iSerialNumber=<string> functions=adb,ptp
mount -t functionfs adb /dev/usbgadget/adb -o uid=2000,gid=2000
mount -t functionfs ptp /dev/usbgadget/ptp -o uid=2000,gid=2000
./adbd &
./ptp `pwd`/images 2>ptp.log
If no "functions" module parameters is supplied, the driver defaults to old
behaviour, i.e. it accepts just one function with any name.
When "functions" module parameter is supplied, only functions with listed names
are accepted.
Come to think of it, functionfs could simply accept any number of functions and
ignore their names all together. Ie. The “functions” parameter could be dropped
and functionfs could stop caring about mount device names.
This, I think, would simplify the code a little.
The gadget is registered only after all the function filesystems have been
mounted and USB descriptors of all functions have been written to their ep0's.
Conversly, the gadget is unregistered after the first USB function closes
its endpoints.
(*) I used a modified adbd daemon which uses FunctionFS as a USB access method.
diff --git a/drivers/usb/gadget/f_fs.c b/drivers/usb/gadget/f_fs.c
index acb3800..2d23aa2 100644
--- a/drivers/usb/gadget/f_fs.c
+++ b/drivers/usb/gadget/f_fs.c
@@ -183,7 +183,7 @@ struct ffs_data {
* Device name, write once when file system is mounted.
* Intended for user to read if she wants.
*/
- const char *dev_name;
+ char *dev_name;
Is that really needed?
/* Private data for our user (ie. gadget). Managed by user. */
void *private_data;
@@ -158,13 +175,36 @@ static struct usb_composite_driver gfs_driver = {
.iProduct = DRIVER_DESC,
};
-static struct ffs_data *gfs_ffs_data;
-static unsigned long gfs_registered;
+static DEFINE_MUTEX(gfs_lock);
+static LIST_HEAD(ffs_list);
+static unsigned int missing_funcs;
+static bool gfs_ether_setup;
+static bool gfs_registered;
+static struct gfs_ffs_obj gfs_default_func;
+static struct gfs_ffs_obj *ffs_tab;
static int gfs_init(void)
{
+ int i;
+
ENTER();
+ if (func_num > 0) {
+ i = 0;
+ ffs_tab = kzalloc(func_num * sizeof *ffs_tab, GFP_KERNEL);
ffs_tab = kcalloc(func_num, sizeof *ffs_tab, GFP_KERNEL);
+ if (!ffs_tab)
+ return -ENOMEM;
+
+ for (i = 0; i < func_num; i++) {
+ ffs_tab[i].name = func_names[i];
+ list_add_tail(&ffs_tab[i].item, &ffs_list);
+ }
+ missing_funcs = func_num;
+ } else {
+ list_add_tail(&gfs_default_func.item, &ffs_list);
Why not allocate it either way?
+ missing_funcs++;
missing_funcs = 1; seems to make more sense.
+ }
+
I'd see it as something as:
missing_funcs = max(func_num, 1u);
ffs_tab = kcalloc(missing_funcs, sizeof *ffs_tab, GFP_KERNEL);
if (!ffs_tab)
return -ENOMEM;
for (i = 0; i < missing_funcs; ++i) {
/* If func_num == 0, func_names[0] == NULL */
ffs_tab[i].name = func_names[i];
list_add_tail(&ffs_tab[i].item, &ffs_list);
}
On top of it all, the list seems redundant since one can iterate over ffs_tab.
But as mentioned earlier, I would consider strongly removing this logic
all together and just adding new functions each time a new mount point
is created.
return functionfs_init();
}
module_init(gfs_init);
@@ -172,63 +212,170 @@ module_init(gfs_init);
static void gfs_exit(void)
{
ENTER();
+ mutex_lock(&gfs_lock);
- if (test_and_clear_bit(0, &gfs_registered))
+ if (gfs_registered)
usb_composite_unregister(&gfs_driver);
+ gfs_registered = false;
Put gfs_registered = false;
functionfs_cleanup();
+
+ kfree(ffs_tab);
+ mutex_unlock(&gfs_lock);
Swap the two above.
}
module_exit(gfs_exit);
+static struct gfs_ffs_obj *gfs_find_dev(const char *dev_name)
+{
+ struct gfs_ffs_obj *i;
The name “i” seems confusing. One usually expects “i” to be an
integer. This applies throughout the file.
+ ENTER();
+
+ if (!func_num)
+ return &gfs_default_func;
+
+ list_for_each_entry(i, &ffs_list, item)
+ if (strcmp(i->name, dev_name) == 0)
+ return i;
+
+ return NULL;
+}
+
static int functionfs_ready_callback(struct ffs_data *ffs)
{
+ struct gfs_ffs_obj *i;
int ret;
ENTER();
- if (WARN_ON(test_and_set_bit(0, &gfs_registered)))
+ mutex_lock(&gfs_lock);
+ i = gfs_find_dev(ffs->dev_name);
+ if (!i) {
+ mutex_unlock(&gfs_lock);
+ return -EINVAL;
It is customary to do:
ret = -EINVAL;
goto done;
where done label points just before mutex_unlock() at the end of
the function. This way, there is only one mutex_lock() which is
nicely paired with mutex_unlock(). It may also lead to smaller
function, as there is only one mutex_unlock() call place.
This comment applies throughout the file.
+ }
+
+ if (WARN_ON(i->desc_rdy)) {
+ mutex_unlock(&gfs_lock);
return -EBUSY;
+ }
+ i->desc_rdy = true;
+ i->ffs_data = ffs;
+
+ if (--missing_funcs) {
+ mutex_unlock(&gfs_lock);
+ return 0;
+ }
+
+ if (gfs_registered) {
+ mutex_unlock(&gfs_lock);
+ return -EBUSY;
+ }
+ gfs_registered = true;
- gfs_ffs_data = ffs;
ret = usb_composite_probe(&gfs_driver, gfs_bind);
if (unlikely(ret < 0))
- clear_bit(0, &gfs_registered);
+ gfs_registered = false;
+ mutex_unlock(&gfs_lock);
+
return ret;
}
--
Best regards, _ _
.o. | Liege of Serenely Enlightened Majesty of o' \,=./ `o
..o | Computer Science, Michał “mina86” Nazarewicz (o o)
ooo +----<email/xmpp: mpn@xxxxxxxxxx>--------------ooO--(_)--Ooo--
--
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
