Hi Matt,
Thank you for the library:
http://thread.gmane.org/gmane.linux.kernel/1556137
I'd like to make some comments you might want to consider.
I don't like too much the idea of hardcoding the set of available functions.
When new functions' conversions to use configfs are available (or completely
new functions are added), you have to patch and recompile your library. Why
not discover what the available functions are?
Your library implicitly assumes that kernel modules' dependencies are
properly
calculated and modules are available for modprobe; otherwise mkdir
<function dir> will not cause its corresponding module to load. This is
a sane
assumption. So modules' aliases should be available, too, and functions
using
the new function registration interface do follow a convention for aliases:
$ cat /lib/modules/`uname -r`/modules.alias
alias usbfunc:acm usb_f_acm
alias usbfunc:rndis usb_f_rndis
alias usbfunc:gser usb_f_serial
alias usbfunc:eem usb_f_eem
alias usbfunc:obex usb_f_obex
alias usbfunc:phonet usb_f_phonet
alias usbfunc:ecm usb_f_ecm
alias usbfunc:geth usb_f_ecm_subset
alias usbfunc:ncm usb_f_ncm
Why not parse this file? I know that using strings (which are short, though)
might not be the preferred way of identifying the functions in the program,
so perhaps you could create an array of strings once and then use an index
into it to tell one function from another?
Alternatively, maybe you could use a configuration file which specifies the
available functions? And provide a tool for creating the config file?
If one of the above is in place, it is pretty straightforward to provide
a method of listing the available functions, which is a nice feature for
a gadget tool:
$ gt --list-func
acm
ecm
phonet
obex
...
In gadget_create_gadget(), gadget_create_config() and
gadget_create_function()
you first mkdir() and then allocate some memory. If memory is not allocated,
you don't remove the directories. I guess you should. And even better,
first do all other stuff that can fail, and as the last step try
creating the
directory; that way the filesystem operation does not need to be reverted.
And for function directories there is also a side effect of loading
the function's module upon directory creation, so it is better not to
cause it
to load if the function's creation is unsuccessful.
Writing to attributes might actually fail, so whenever you call
fputs()/fgets(),
perhaps you should take it into account? Propagate the error further? Or
not?
Perhaps consider making gadget_write_string() static inline? It is called
in a number of places while in fact it does nothing apart from delegating
its job to gadget_write_buf().
I have mixed feelings about specialized setters in the section "USB
function-
specific attribute configuration". Whenever new USB functions are available
through configfs, the library must be patched and recompiled, which
might also
affect its users.
Discovering what attributes are available for a given function is possible
only after its directory is created. So I guess the library should provide
a way of setting an attribute of any name and type (number, string) and
fail if
there happens to be no such attribute or the attribute type does not match.
Then, the gadget tool invocation would be something on the lines of:
$ gt {.......} --func=ecm,qmult=1 {......}
If qmult happens to be ecm's attribute and it makes sense to write "1" to it
the gadget is created, otherwise an appropriate error message is returned.
The gt would eventually call gadget_write_buf(path, name, "qmult", "1")
However, I admit there might be cases when e.g. qmult is already available
as a number. Then it would be inconvenient to have to convert it first to
a string, so gadget_write_int/dec/hex() could be made public.
The above is just a bunch of thoughts for you to consider. If there are
better
ideas they are more than welcome.
Thank you,
AP
--
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
