Hi, > -----Original Message----- > From: Andrzej Pietrasiewicz [mailto:andrzej.p@xxxxxxxxxxx] > Sent: Monday, November 04, 2013 12:52 PM > To: matt.porter@xxxxxxxxxx > Cc: jengelh@xxxxxxx; linux-usb@xxxxxxxxxxxxxxx; Marek Szyprowski; > kyungmin.park@xxxxxxxxxxx; Krzysztof Opasiak > Subject: about libusbg (formerly libgadget) > > 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 > ... > I'm not optimistic about this idea. First of all, modules can be build-in kernel so the file modules.aliases may not contain the whole list of available functions. Secondly, the assumption about calculated modules dependencies is quite good. Providing modules and dependencies is a task of super user. Library should not contain any code related to module loading etc. Program which use this library may not have root permissions. For example root will give chmod 777 /configFS and then all users will be able to manipulate gadget's but they will be able to create only gadget's delivered by root (or other users with such permissions). It's quite good method for gadget administration. Moreover, maybe we can follow the convention of libusb? We could provide a set of dedicated functions for standard gadgets (gadget actually in mainline) and the develop library when new module appeared, but also we should provide a set of functions which will allow to create any gadgets and their parameters from user strings for example gadget_create_raw_function("my_brand_new_function"); gadget_create_raw_function(FUNCTION_ENUM_ACM); > 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(). Good points, this should be changed. > > 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. > As I wrote before, we could provide both side approach - support mainline functions with specialized getters/setters but also provide a set of function for creation and manipulation of functions using raw strings. -- BR's Krzysztof Opasiak -- 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
