Hi Ruud,
On Wednesday, May 15, 2013 4:23:51 PM, Ruud Commandeur wrote:
> This patch fixes a number of mmc and fat-related bugs:
There should be only one logical change per patch.
>
> > Added a check for blkcnt > 0 in mmc_write_blocks (drivers/mmc.c) to prevent
> > a hangup for further mmc commands.
Adding Andy, the MMC custodian, to Cc.
>
> > Solved a checksum issue in fs/fat/fat.c. The mkcksum has const char
> > arguments with a size specifier, like "const char name[8]". In the
> > function, it is assumed that sizeof(name) will have the value 8, but this
> > is not the case (at least not for the Sourcery CodeBench compiler and
> > probably not according to ANSI C). This causes "long filename checksum
> > errors" for each fat file listed or written.
>
> > Made some changes to fs/fat/fat_write.c. Fixed testing fat_val for
> > 0xffff/0xfff8 and 0xfffffff/0xffffff8 by adding the corresponding fatsize
> > in the test (as read in earlier posts) and some changes in debug output.
Expressions like "as read in earlier posts" should not be present in a patch
description since it is unclear what it refers to once the patch has been
applied.
Line lengths should be at most 80 characters, including in the patch
description.
>
> Signed-off-by: Ruud Commandeur <rcommand...@clb.nl>
> Cc: Tom Rini <tr...@ti.com>
> Cc: Benoît Thébaudeau <benoit.thebaud...@advansee.com>
> Cc: Mats Karrman <mats.karr...@tritech.se>
>
> Index: drivers/mmc/mmc.c
> ===================================================================
> --- drivers/mmc/mmc.c (revision 9)
> +++ drivers/mmc/mmc.c (working copy)
> @@ -282,8 +282,9 @@
>
> if (blkcnt > 1)
> cmd.cmdidx = MMC_CMD_WRITE_MULTIPLE_BLOCK;
> - else
> + else if (blkcnt > 0)
> cmd.cmdidx = MMC_CMD_WRITE_SINGLE_BLOCK;
> + else return 0; //Called with blkcnt = 0
The comment above is useless. Also, the comment style in U-Boot is /**/, not //.
>
> if (mmc->high_capacity)
> cmd.cmdarg = start;
> Index: fs/fat/fat.c
> ===================================================================
> --- fs/fat/fat.c (revision 9)
> +++ fs/fat/fat.c (working copy)
> @@ -569,10 +569,12 @@
>
> __u8 ret = 0;
>
> - for (i = 0; i < sizeof(name); i++)
> + for (i = 0; i < 8; i++) {
> ret = (((ret & 1) << 7) | ((ret & 0xfe) >> 1)) + name[i];
> - for (i = 0; i < sizeof(ext); i++)
> + }
> + for (i = 0; i < 3; i++) {
> ret = (((ret & 1) << 7) | ((ret & 0xfe) >> 1)) + ext[i];
> + }
>
> return ret;
> }
There should not be curly braces for single-line blocks.
Anyway, this has already been done by Marek in commit 6ad77d8, so this hunk
should be dropped.
> Index: fs/fat/fat_write.c
> ===================================================================
> --- fs/fat/fat_write.c (revision 9)
> +++ fs/fat/fat_write.c (working copy)
> @@ -41,6 +41,7 @@
> }
>
> static int total_sector;
> +
> static int disk_write(__u32 block, __u32 nr_blocks, void *buf)
> {
> if (!cur_dev || !cur_dev->block_write)
This hunk is not very useful.
> @@ -104,8 +105,7 @@
> } else
> memcpy(dirent->ext, s_name + period_location + 1, 3);
>
> - debug("name : %s\n", dirent->name);
> - debug("ext : %s\n", dirent->ext);
> + debug("name.ext : %.8s.%.3s\n", dirent->name, dirent->ext);
Correct. You could just remove the space before the colon since this is the
standard English formatting.
> }
>
> static __u8 num_of_fats;
> @@ -264,6 +264,7 @@
> goto name0_4;
> }
> slotptr->name0_4[j] = name[*idx];
> + slotptr->name0_4[j + 1] = 0x00;
> (*idx)++;
> end_idx++;
> }
> @@ -275,6 +276,7 @@
> goto name5_10;
> }
> slotptr->name5_10[j] = name[*idx];
> + slotptr->name5_10[j + 1] = 0x00;
> (*idx)++;
> end_idx++;
> }
> @@ -286,6 +288,7 @@
> goto name11_12;
> }
> slotptr->name11_12[j] = name[*idx];
> + slotptr->name11_12[j + 1] = 0x00;
> (*idx)++;
> end_idx++;
> }
These 3 hunks are correct, but they should be mentioned in the patch
description.
> @@ -569,7 +572,7 @@
> else
> startsect = mydata->rootdir_sect;
>
> - debug("clustnum: %d, startsect: %d\n", clustnum, startsect);
> + debug("clustnum: %d, startsect: %d, size %lu\n", clustnum, startsect,
> size);
Line too long: max 80 chars.
>
> if (disk_write(startsect, size / mydata->sect_size, buffer) < 0) {
> debug("Error writing data\n");
> @@ -587,10 +590,7 @@
> debug("Error writing data\n");
> return -1;
> }
> -
> - return 0;
OK.
> }
> -
This empty line would be better kept for code legibility.
> return 0;
> }
>
> @@ -656,7 +656,8 @@
> else
> break;
>
> - if (fat_val == 0xfffffff || fat_val == 0xffff)
> + if (((fat_val == 0xfffffff) && (mydata->fatsize == 32)) ||
> + ((fat_val == 0xffff) && (mydata->fatsize == 16)))
> break;
>
> entry = fat_val;
> @@ -901,7 +902,8 @@
> }
>
> curclust = get_fatent_value(mydata, dir_curclust);
> - if ((curclust >= 0xffffff8) || (curclust >= 0xfff8)) {
> + if (((curclust >= 0xffffff8) && (mydata->fatsize == 32)) ||
> + ((curclust >= 0xfff8) && (mydata->fatsize == 16))) {
> empty_dentptr = dentptr;
> return NULL;
> }
>
These 2 hunks are correct, but please remove the parentheses around the "=="
expressions: They make the code less readable. And add another tab to indent the
2nd line of the if-s so that it is more indented than the if block contents.
Best regards,
Benoît
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
