On 11/08/2012 03:53 PM, Michal Nazarewicz wrote:
The code looks promising, and definitely a step in right direction, but
I don't think this configuration structure is good solution for
ConfigFS.
Sure, many (most) functions have same static unstructured configuration
arguments which can be expressed in a flat array, but if you look at
mass storage, this is not really a good solution.
At the moment, mass storage is using a static and flat list of arguments
because it uses module parameters, but the “file” argument is very
ugly... It accepts a list of file names to use for each LUN.
With ConfigFS I believe we can do much better than to use inflexible
list of arguments. In particular, the way I would see mass storage
function be configured from ConfigFS is:
$ cd /path/to/mass/storage/function/configfs/directory
$ ls
nluns
$ echo 2>nluns
$ ls
nluns lun0 lun1
$ echo 0>lun0/ro
$ echo 0>lun0/cdrom
$ echo 0>lun0/nofua
$ echo 0>lun0/removable
$ echo /path/to/some/file>lun0/file
$ echo 1>lun0/ro
$ echo 1>lun0/cdrom
$ echo 0>lun0/nofua
$ echo 1>lun0/removable
This is actually what the tcm gadget does today. So let me then just
drop the usbf_option thingy then. So we keep the configuration we have
right now and we add configfs specific code to make something like that
possible.
diff --git a/drivers/usb/gadget/composite.c b/drivers/usb/gadget/composite.c
index 4eb07c7..c46ae24 100644
--- a/drivers/usb/gadget/composite.c
+++ b/drivers/usb/gadget/composite.c
diff --git a/drivers/usb/gadget/functions.c b/drivers/usb/gadget/functions.c
new file mode 100644
index 0000000..79f9472f3
--- /dev/null
+++ b/drivers/usb/gadget/functions.c
@@ -0,0 +1,97 @@
+#include<linux/kernel.h>
+#include<linux/slab.h>
+#include<linux/module.h>
+#include<linux/err.h>
+
+#include<linux/usb/composite.h>
+
+static LIST_HEAD(func_list);
+static DEFINE_MUTEX(func_lock);
+
+static struct usb_function *try_get_usb_function(const char *name)
+{
+ struct usb_d_function *f;
+ struct usb_function *uf;
+
+ mutex_lock(&func_lock);
+ list_for_each_entry(f,&func_list, list) {
+ if (!strcmp(name, f->name)) {
Alternatively
+ if (strcmp(name, f->name))
+ continue;
which will make the rest one indention less, but feel free to stick to
your preference here.
that is always a good one :)
+ bool ok;
+ ok = try_module_get(f->mod);
+ uf = ERR_PTR(-EBUSY);
+ if (!ok)
+ goto out;
This temporary variable looks strange to me. How about:
+ if (!try_module_get(f->mod)) {
+ uf = ERR_PTR(-EBUSY);
+ goto out;
+ }
okay.
+ uf = f->alloc();
+ if (uf) {
+ uf->mod = f->mod;
+ } else {
+ module_put(f->mod);
+ uf = ERR_PTR(-ENOMEM);
+ }
+ mutex_unlock(&func_lock);
+ return uf;
“goto out;” instead of those two lines perhaps? This will keep a single
mutex_unlock() location.
+ }
+ }
+ uf = ERR_PTR(-ENOENT);
+out:
Actually, if you initialise uf with ERR_PTR(-ENOENT) at the beginning,
and apply my change with the “bool ok” variable, you can get rid of the
out label and use “break” instead of “goto”.
Patch updated. Thanks.
+ mutex_unlock(&func_lock);
+ return uf;
+}
+
diff --git a/include/linux/usb/composite.h b/include/linux/usb/composite.h
index b09c37e..ced2910 100644
--- a/include/linux/usb/composite.h
+++ b/include/linux/usb/composite.h
@@ -116,6 +116,22 @@ struct usb_configuration;
* two or more distinct instances within the same configuration, providing
* several independent logical data links to a USB host.
*/
+
+enum USBF_OPTION_TYPE {
+ USBF_OPTION_INVALID,
+ USBF_OPTION_STRING,
+ USBF_OPTION_INT,
+};
+
+struct usbf_option {
+ enum USBF_OPTION_TYPE type;
+ char *name;
+ union {
+ char *o_charp;
+ int o_int;
+ } val;
+};
+
struct usb_function {
const char *name;
struct usb_gadget_strings **strings;
@@ -136,6 +152,13 @@ struct usb_function {
struct usb_function *);
void (*unbind)(struct usb_configuration *,
struct usb_function *);
+ /* */
+ void (*free_func)(struct usb_function *f);
+ const struct usbf_option *avail_options;
+ unsigned avail_options_num;
Why not make the avail_options array be terminated with a NULL-named
option or something like that? I think that's simpler than having to
specify the number of arguments explicitly.
As mentioned before, I will try to drop this provide something configfs
specific once we have it.
Sebastian
--
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
