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