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
--
Best regards,
Lukasz Majewski
Samsung Poland R&D Center | Linux Platform Group
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
