[U-Boot] [U-Boot, PATCH v3 2/2] fastboot: Flash command support

[Wolfgang Denk]

Dear Dileep Katta,

In message <1410417650-16522-2-git-send-email-dileep.katta at linaro.org> you wrote:
> Flash command internally uses DFU, and Fastboot command initialization is modified to add DFU and partition initialization
> Added oem format functionality for GPT table creation
> partitioning code is added as disk/part_fastboot.c for better usability
> 
> Fastboot flash command code is enabled and being tested on BeagleBone Black.
> OMAP3 Beagle configuration is modified to fix the build errors, but this configuration needs to be updated as per the flash functionality.

Please fix the line length in the commit message.

Also, checkpatch says:

	WARNING: do not add new typedefs
	#1073: FILE: include/usb/fastboot.h:98:
	+typedef struct fastboot_ptentry fastboot_ptentry;

> +int handle_flash(char *part_name, char *response, char *transfer_buffer)
> +{
> +	int status = 0;
> +	printf("download_bytes: 0x%x\n", download_bytes);
> +	if (download_bytes) {
> +		struct fastboot_ptentry *ptn;
> +
> +		/* Next is the partition name */
> +		ptn = fastboot_flash_find_ptn(part_name);
> +
> +		if (ptn == 0) {
> +			printf("Partition:[%s] does not exist\n", part_name);
> +			sprintf(response, "FAILpartition does not exist");
> +		} else if ((download_bytes > ptn->length) &&
> +			!(ptn->flags & FASTBOOT_PTENTRY_FLAGS_WRITE_ENV)) {
> +			printf("Image too large for the partition\n");
> +			sprintf(response, "FAILimage too large for partition");
> +		} else if (ptn->flags & FASTBOOT_PTENTRY_FLAGS_WRITE_ENV) {
> +			/* Check if this is not really a flash write,
> +			 * but instead a saveenv
> +			 */
> +			unsigned int i = 0;
> +			/* Env file is expected with a NULL delimeter between
> +			 * env variables So replace New line Feeds(0x0a) with
> +			 * NULL (0x00)
> +			 */

Incorrect multiline comment style.  Please fix globally.

Also this sequence of comment - declaration - comment is highly
confusing.  What is the first comment ("Check if..") actually related
to?

> +			for (i = 0; i < download_bytes; i++) {
> +				if (transfer_buffer[i] == 0x0a)
> +					transfer_buffer[i] = 0x00;
> +			}
> +			memset(env_ptr->data, 0, ENV_SIZE);
> +			memcpy(env_ptr->data, transfer_buffer, download_bytes);
> +			do_env_save(NULL, 0, 1, NULL);
> +			printf("saveenv to '%s' DONE!\n", ptn->name);

I think it is a bad idea to split formatting / reformatting
envrionment data alll over the place.  This should be done only once,
in the environment handling code.  Also, I do not like you using
do_env_save() here - this should remain a static function.  Why do't
you simply use saveenv() here?

Finally, the "saveenv to '%s' DONE!\n" is IMO too verbose.  No news is
Good News - we usually do not report abot the success of operations,
as this is what we expect.  We should only print error messages when
such occur - which points out another problem of that code: there is
no error checking nor handling there...


> +static void end_ptbl(struct ptable *ptbl)
> +{
> +	struct efi_header *hdr = &ptbl->header;
> +	u32 n;
> +
> +	n = crc32(0, 0, 0);

What exactly is this good for?

> +	n = crc32(n, (void *)ptbl->entry, sizeof(ptbl->entry));
> +	hdr->entries_crc32 = n;
> +
> +	n = crc32(0, 0, 0);

What exactly is this good for?

> +	n = crc32(0, (void *)&ptbl->header, sizeof(ptbl->header));
> +	hdr->crc32 = n;
> +}


> +	for (n = 0; n < (128/4); n++) {
> +		/* read partition */
> +		source[0] = '\0';
> +		dest[0] = '\0';
> +		length[0] = '\0';

You overwrite source, dest, and length by the sprintf()s just a few
lines below.  So why are you doing this here?

> +		mmc_read[2] = source;
> +		mmc_read[3] = dest;
> +		mmc_read[4] = length;
> +
> +		sprintf(source, "%p", entry);
> +		sprintf(dest, "0x%x", 0x1+n);
> +		sprintf(length, "0x%x", 1);


> +int board_mmc_fbtptn_init(void)
> +{
> +	char *mmc_init[2] = {"mmc", "rescan",};
> +	char dev[2];
> +	char *mmc_dev[3] = {"mmc", "dev", NULL};
> +
> +	mmc_dev[2] = dev;

Why do you initialize mmc_dev[2] with another value just a line
above?  Why don't you use the correct value right from the beginning?

> +	if (do_mmcops(NULL, 0, 3, mmc_dev)) {
> +		printf("MMC DEV: %d selection FAILED!\n", CONFIG_FB_MMCDEV);
> +		return -1;
> +	}
> +
> +	if (do_mmcops(NULL, 0, 2, mmc_init)) {
> +			printf("FAIL:Init of MMC card\n");
> +			return 1;
> +	}

What exactly are your return codes?  Here you return -1 in case of
error, there +1 - what's the logic behind that?

> +int do_format(void)
> +{
> +	struct ptable *ptbl = &the_ptable;
> +	/* unsigned sector_sz; */

Please remove dead code.

> +	printf("\ndo_format ..!!");

Is this not overly verbose?

> +	/* get mmc info */
> +	struct mmc *mmc = find_mmc_device(CONFIG_FB_MMCDEV);

Please do not mix code and declarations.  Also, always add a blank
line after declarations.

> +	if (mmc_init(mmc)) {
> +		printf("\n mmc init FAILED");
> +		return -1;
> +	} else{

After the return, no else is needed here.

> +		printf("\nmmc capacity is: %llu", mmc->capacity);
> +		printf("\nmmc: number of blocks:0x%lx", mmc->block_dev.lba);
> +		printf("\nmmc: block size:0x%lx", mmc->block_dev.blksz);
> +	}

> +	/* 10/11:modified as per PSP release support */
> +	char *mmc_write[5]  = {"mmc", "write", NULL, NULL, NULL};
> +	char source[32], dest[32], length[32];
> +
> +	char dev[2];
> +	char *mmc_dev[3] = {"mmc", "dev", NULL};
> +	mmc_dev[2] = dev;

See above - please do not mix code and declarations, please use the
right initialization right away.

> +	} else {
> +		printf("Writing mbr is DONE!\n");

Less verbose, please.


Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
At the source of every error which is blamed on the computer you will
find at least two human errors, including the error of blaming it  on
the computer.