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

[Chander Kashyap]

Hi Mike,

On 3 February 2012 02:51, Mike Frysinger <vapier at gentoo.org> wrote:
> 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]);
Yes That's  better.
>
>> +             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.
Ah yes. I will fix it.
>
>> +     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]);
>
Will take care of it.
>> +     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



-- 
with warm regards,
Chander Kashyap