[U-Boot] [PATCH] mmc and fat bug fixes

[Benoît Thébaudeau]

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 <rcommandeur at clb.nl>
> Cc: Tom Rini <trini at ti.com>
> Cc: Benoît Thébaudeau <benoit.thebaudeau at advansee.com>
> Cc: Mats Karrman <Mats.Karrman at 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