Dear Mike Frysinger,
Thank you for thorough review.
On Tuesday 03 July 2012 05:38:05 Lukasz Majewski wrote:
--- /dev/null +++ b/drivers/usb/gadget/g_dnl.c
+static const char shortname[] = "usb_dnl_";
shortname -> gadget_name_prefix
This might be only matter of taste, but in my opinion this name is more readable.
+static void g_dnl_suspend(struct usb_composite_dev *cdev) +{
- if (cdev->gadget->speed == USB_SPEED_UNKNOWN)
return;- debug("suspend\n");
+}
+static void g_dnl_resume(struct usb_composite_dev *cdev) +{
- debug("resume\n");
+}
do suspend/resume funcs make any sense in u-boot ?
You have reviewed the v1 of this patch series. Marek Vasut has already pointed out this problem and it has been resoled with v2.
+int g_dnl_init(char *s)
const char
Ok.
+{
- int ret;
- static char str[16];
- memset(str, '\0', sizeof(str));
- strncpy(str, shortname, sizeof(shortname));
no need for the memset.
The gadget can be called from many separate commands (e.g. command "dfu" and command "ums") and those commands can be executed without power cycle. Thereof I need to be sure, that str is not polluted by previous name.
this strncpy looks broken -- the 3rd arg is for how many bytes are available in the *dest* buffer, not how long the source is.
After looking deeply into the source I admit that providing the upper bound on the dest is more safe.
- if (!strncmp(s, "dfu", sizeof(s))) {
sizeof() here is wrong -- that gives you back 4 (the size of a pointer) on 32bit systems
... and it works only because the "dfu\0" and "ums\0" is 4 lenght. :/
strncat(str, s, sizeof(str));this is also incorrect. the length given to strncat is how many bytes are left, not the total length.
I cannot agree. sizeof(str) return 16, which is the destination buffer size.
since this string parsing logic is all just completely broken, i'd suggest replacing it all with:
{ int ret; /* We only allow "dfu" atm, so 3 should be enough */ static char name[sizeof(shortname) + 3];
if (strcmp(s, "dfu")) { printf("%s: unknown command: %s\n", __func__, s); return -EINVAL; }
strcpy(name, shortname); strcat(name, s);
This is a very neat design, but it assumes that there will be only one function ("dfu" in this case). For this particular function +3 applies, but what if another function (like "usb_storage") will be defined? I'm now working on another function - the USB Mass Storage (named "ums" ;-) ).
Another issue is omitting the strncmp/strncpy functions and depending on the: static char name[sizeof(shortname) + 3]; definition to prevent buffer overflow.
The +3 magic number worries me a bit...
- if (ret) {
printf("%s: failed!, error:%d ", __func__, ret);needs a space between "error:" and "%d"
Will be fixed
--- /dev/null +++ b/include/g_dnl.h
+#include <linux/usb/ch9.h> +#include <usbdescriptors.h> +#include <linux/usb/gadget.h>
unused -> delete
I will remove those includes from g_dnl.c file
+int g_dnl_init(char *s);
int g_dnl_register(const char *type);
this also needs documentation in the header explaining how to use this func
I will provide the working example of this gadget. I will not insist on the function name. It can be g_dnl_register().
+void g_dnl_cleanup(void);
void g_dnl_unregister(void);
Can be g_dnl_unregister() as well.
+/* USB initialization declaration - board specific*/ +void board_usb_init(void);
not used in these files -> delete
But it is used at e.g. samsung/trats/trats.c I think that g_dnl.h is a good place for this.
-mike