[U-Boot] [PATCH v7 4/4] EXYNOS: SMDK5250: Add MMC SPL support

[Mike Frysinger]

On Thursday 02 February 2012 04:11:27 Chander Kashyap wrote:
> --- /dev/null
> +++ b/board/samsung/smdk5250/tools/mkexynos_image.c

tools should be in tools/.  there are already plenty of examples in there (see 
tools/msxboot.c as the first example i found by reading tools/Makefile).

> +int main(int argc, char **argv)
> +{
> ...
> +	unsigned char buffer[BUFSIZE] = {0};

this is an implicit memset() and from what i can see in the code, useless.  
you read() the entire buffer, so there's no need to initialize it.

> +	if (argc != 3) {
> +		printf(" %d Wrong number of arguments\n", argc);

this should tell the user how to use the tool.
	fprintf(stderr, "Usage: %s <infile> <outfile>\n", argv[0]);

> +		if (ifd)
> +			close(ifd);

this if() is wrong (0 is a valid fd) and useless (you already abort if ifd did 
not succeed).  just delete the if statement.

> +	len = lseek(ifd, 0, SEEK_END);
> +	lseek(ifd, 0, SEEK_SET);

lazy man's stat() :P.  just use stat().  and change the type of "len" to 
"off_t".

> +	count = (len < CHECKSUM_OFFSET) ? len : CHECKSUM_OFFSET;
> +
> +	if (read(ifd, buffer, count) != count) {

count should be a ssize_t.  although, this doesn't handle partial interrupted 
reads, so i wonder if this could shouldn't just be changed to use stdio 
fopen/fread.  probably would be simpler that way too.

> +		if (ifd)
> +			close(ifd);
> +		if (ofd)
> +			close(ofd);

these if checks are wrong for the same reason mentioned above

> + unsigned long checksum = 0;
> ...
> +	for (i = 0, checksum = 0; i < CHECKSUM_OFFSET; i++)
> +		checksum += buffer[i];
> +	memcpy(&buffer[CHECKSUM_OFFSET], &checksum, sizeof(checksum));

pretty sure this fails if this tool is run on a big-endian machine, as well as 
64bit vs 32bit.  change the type of "checksum" to uint32_t, then use something 
like:
	put_unaligned_le32(checksum, &buffer[CHECKSUM_OFFSET]);

> +	if (write(ofd, buffer, BUFSIZE) != BUFSIZE) {

same issues as the read() mentioned above

> +		if (ifd)
> +			close(ifd);
> +		if (ofd)
> +			close(ofd);

same bad if() logic

> +	if (ifd)
> +		close(ifd);
> +	if (ofd)
> +		close(ofd);

same bad if() logic
-mike
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: This is a digitally signed message part.
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20120202/4acf154a/attachment.pgp>