[U-Boot] [PATCH 1/7] dfu:usb: Support for g_dnl composite download gadget.

[Lukasz Majewski <l.majewski@samsung.com>]

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