[U-Boot] [PATCH v3 4/5] usb/gadget: add the fastboot gadget

[Marek Vasut <marex@denx.de>]

On Thursday, April 10, 2014 at 09:18:06 PM, Rob Herring wrote:
> From: Sebastian Siewior <bigeasy@linutronix.de>
[...]

> +++ b/doc/README.android-fastboot
> @@ -0,0 +1,86 @@
> +Android Fastboot
> +~~~~~~~~~~~~~~~~
> +
> +Overview
> +========
> +The protocol that is used over USB is described in
> +README.android-fastboot-protocol in same folder.

"directory", this is not windows, so no folders here either ...

> +The current implementation does not yet support the flash and erase
> +commands.
> +
> +Client installation
> +===================
> +The counterpart to this gadget is the fastboot client which can
> +be found in Android's platform/system/core repository in the fastboot
> +folder. It runs on Windows, Linux and even OSx. Linux user are lucky since
> +they only need libusb.

OSX is written like so, the X is also capital.

[...]

> +In Action
> +=========
> +Enter into fastboot by executing the fastboot command in u-boot and you
> +should see:
> +|Fastboot entered...
> +
> +The gadget terminates once the is unplugged.

The above sentence makes zero sense to me.

[...]

> +
> +/* e1 */

This is a fine comment, but please make it sensible.

[...]

> +
> +static struct usb_request *ep0_req;
> +
> +struct usb_ep *ep_in;
> +struct usb_request *req_in;
> +
> +struct usb_ep *ep_out;
> +struct usb_request *req_out;

Please define all global variables at the top of the file.

[...]

> diff --git a/drivers/usb/gadget/g_fastboot.h
> b/drivers/usb/gadget/g_fastboot.h new file mode 100644
> index 0000000..733eb38
> --- /dev/null
> +++ b/drivers/usb/gadget/g_fastboot.h
> @@ -0,0 +1,15 @@
> +#ifndef _G_FASTBOOT_H_
> +#define _G_FASTBOOT_H_
> +
> +#define EP_BUFFER_SIZE			4096
> +
> +extern struct usb_ep *ep_in;
> +extern struct usb_request *req_in;
> +extern struct usb_ep *ep_out;
> +extern struct usb_request *req_out;

Ick, I really dislike how you're distributing the variables across the code. Is 
this really necessary ?

> +void rx_handler_command(struct usb_ep *ep, struct usb_request *req);
> +int fastboot_tx_write(const char *buffer, unsigned int buffer_size);
> +const char *fb_find_usb_string(unsigned int id);

Please produce a consistent naming for those function names, something that can 
also be identified that it's coming from the fastboot gadget. Otherwise, these 
function names look rather random and are hard to pair with particular part of 
U-Boot at first glance.

[...]

> +static int strcmp_l1(const char *s1, const char *s2)
> +{
> +	return strncmp(s1, s2, strlen(s1));

I suspect someone will sooner or later call this function with non-null-
terminated string as $s1 . Can't you do some boundary checking here?

I'd hate to have some fastbleed bug here (sorry, bad pun).
[...]

> +static void rx_handler_dl_image(struct usb_ep *ep, struct usb_request
> *req) +{
> +	char response[RESPONSE_LEN];
> +	unsigned int transfer_size = download_size - download_bytes;
> +	const unsigned char *buffer = req->buf;
> +	unsigned int buffer_size = req->actual;
> +
> +	if (req->status != 0) {
> +		printf("Bad status: %d\n", req->status);
> +		return;
> +	}
> +
> +	if (buffer_size < transfer_size)
> +		transfer_size = buffer_size;
> +
> +	memcpy((void *)load_addr + download_bytes, buffer, transfer_size);
> +
> +	download_bytes += transfer_size;
> +
> +	/* Check if transfer is done */
> +	if (download_bytes >= download_size) {
> +		/*
> +		 * Reset global transfer variable, keep download_bytes because
> +		 * it will be used in the next possible flashing command
> +		 */
> +		download_size = 0;
> +		req->complete = rx_handler_command;
> +		req->length = EP_BUFFER_SIZE;
> +
> +		sprintf(response, "OKAY");
> +		fastboot_tx_write_str(response);
> +
> +		printf("\ndownloading of %d bytes finished\n", download_bytes);
> +	} else {
> +		req->length = rx_bytes_expected();
> +	}
> +
> +	if (download_bytes && !(download_bytes % BYTES_PER_DOT)) {
> +		printf(".");

putc()

> +		if (!(download_bytes % (74 * BYTES_PER_DOT)))
> +			printf("\n");

putc()

> +	}
> +	req->actual = 0;
> +	usb_ep_queue(ep, req, 0);
> +}
[...]

> +#define FB_STR_PRODUCT_IDX      1
> +#define FB_STR_SERIAL_IDX       2
> +#define FB_STR_CONFIG_IDX       3
> +#define FB_STR_INTERFACE_IDX    4
> +#define FB_STR_MANUFACTURER_IDX 5
> +#define FB_STR_PROC_REV_IDX     6
> +#define FB_STR_PROC_TYPE_IDX    7

Use enum {} for those for the sake of typechecking ?
[..]