[U-Boot] mmc and fat bug fixes

This patch fixes a number of mmc and fat-related bugs:

> Added a check for blkcnt > 0 in mmc_write_blocks (drivers/mmc.c) to prevent a hangup for further mmc commands.

> 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.

Signed-off-by: Ruud Commandeur <rcommandeur@clb.nl>
Cc: Tom Rini <trini@ti.com>
Cc: Benoît Thébaudeau <benoit.thebaudeau@advansee.com>
Cc: Mats Karrman <Mats.Karrman@tritech.se>

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

On Wed, May 15, 2013 at 9:23 AM, Ruud Commandeur <RCommandeur@clb.nl> wrote:

> This patch fixes a number of mmc and fat-related bugs:
>
> > Added a check for blkcnt > 0 in mmc_write_blocks (drivers/mmc.c) to
> prevent a hangup for further mmc commands.
>


You need more information than that. Why is some code requesting 0-byte
data commands?

As others mentioned, you need to break up patches so each change is one
patch.



> 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
>


Assuming this is necessary, I think it then might be time to reorder this:

if (!blkcnt) <-- possibly at the very start of the function.
  return 0;

if (blkcnt == 1)
  cmd.cmdidx = MMC_CMD_WRITE_SINGLE_BLOCK;
else
  cmd.cmdidx = MMC_CMD_WRITE_MULTIPLE_BLOCK;

While technically correct, checking >1, then >0 creates an odd dissonance
in my mind, and makes me have to think about when that if clause will
evaluate to true, and I hate having to think. :)

Andy

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 05/15/2013 11:37 AM, Benoît Thébaudeau wrote:
> 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:
[snip]
>> @@ -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.

With regards to style, checkpatch.pl wins (so please use it on your
patch, git format-patch -n1 ; ./tools/checkpatch.pl
./0001-what-git-said-it-called-it) and it should be:
if ((one big test) ||
    (second test)) {
<tab>... code ...
}

- -- 
Tom
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.11 (GNU/Linux)
Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/

iQIcBAEBAgAGBQJRlBUZAAoJENk4IS6UOR1WGLUQAK1zwb+enZWBQ0j1HXeQUiMx
mnErR6WKBqg7Tt5ya72abdE5tGmulS6QThrT1uXP98u/qUfiEBEDHb5MRy8cRdL8
LKvplOFVkl9iZXbHqD9/u5FUT9A2t/L0nMEYBG8akCIaxqOPLSgB5U7NbkJohrGE
2jhFN1cLXKFkRujstEjefDw720sIBesWXeLLKIzHG8YujxdRkYGr5bi3iHMNSG90
nOIkf7+umd72QqFVs7Sq2xCmNLzntsx1m76DkX0fJ0ReAlkoBfWEvkTKjmr+ANPz
ksualH7u2PWz53h021iVA+b2SZtWhHj5GgqhShFKoqKyyjUyktn/4J9qan0rDp88
6+p3H5bpYfBhOWGzb9lkX+krc1F7ViJO4CABWNZO/ux4Sg+cdpKsFwyVJfSzh1wd
jViZg0oq+qMT5u+XEFY1QAivP0rcL3wiVQ+YUXOfAqjx1zik3EMl0f6FO/71AGnm
SlYGm39dhTyU5kEruCJBVWk97eEh8ETVLIZ/giaEi6HVsw/uv8yElLq4KtdxC7P0
pnNmR+wGnzifJiCYFDgIRKvpFaUCgqfxb6ac717PL7XDb5d3TMfWdfgR4FKMDK7b
GWTiEnA3hLUWUGCdwYflH1JqzJxzXRQBP3Gc7QOqKtHSEcQ1Hc3tlm4kAKvAfF0k
ys0ayHOizfdTTjE2JlPS
=QJxA
-----END PGP SIGNATURE-----

Dear Ruud Commandeur,

In message <15AE5A936F5E3A42A9144E66875A0A89308F2E@server1-derijp.CLB-Benelux.lokaal> you wrote:
> This patch fixes a number of mmc and fat-related bugs:
> 
> > Added a check for blkcnt > 0 in mmc_write_blocks (drivers/mmc.c) to preve=
> nt a hangup for further mmc commands.
> 
> > Solved a checksum issue in fs/fat/fat.c. The mkcksum has const char argum=
> ents 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 cas=
> e (at least not for the Sourcery CodeBench compiler and probably not accord=
> ing to ANSI C). This causes "long filename checksum errors" for each fat fi=
> le listed or written.

Please explain.  Under which exact conditions would  sizeof(name) not
be 8, and where is such assumption supported in ANSI C?


I am tempted to NAK the FAT changes, as they make the code much harder
to read and to maintain.

Using this simple test program:

----- snip ----
#include <stdio.h>

int main(void)
{
	const char name[8];
	const char ext[3];

	printf("sizeof(name)=%d, expected 8\n", sizeof(name));
	printf("sizeof(ext) =%d, expected 3\n", sizeof(name));

	return 0;
}
----- snip ----

I get the expected values on all systems and with all compilers I
tested.   For which exact configuration do you get different results?

> > 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.

Please restrict your line length in commit messages to some 70
characters or so.


> Signed-off-by: Ruud Commandeur <rcommandeur@clb.nl>
> Cc: Tom Rini <trini@ti.com>
> Cc: Beno=EEt Th=E9baudeau <benoit.thebaudeau@advansee.com>
> Cc: Mats Karrman <Mats.Karrman@tritech.se>

Please split into three separate patches, one for MMX, and two for
FAT, one for each problem.  And make sure to add the MMC custodian
on Cc:

Best regards,

Wolfgang Denk

Dear Wolfgang Denk,

Thanks for your comments, please see mine below.

> -----Oorspronkelijk bericht-----
> Van: Wolfgang Denk [mailto:wd@denx.de] 
> Verzonden: donderdag 16 mei 2013 7:55
> Aan: Ruud Commandeur
> CC: u-boot@lists.denx.de; Tom Rini; Mats K?rrman
> Onderwerp: Re: [U-Boot] [PATCH] mmc and fat bug fixes
> 
> Dear Ruud Commandeur,
> 
> In message 
> <15AE5A936F5E3A42A9144E66875A0A89308F2E@server1-derijp.CLB-Ben
> elux.lokaal> you wrote:
> > This patch fixes a number of mmc and fat-related bugs:
> > 
> > > Added a check for blkcnt > 0 in mmc_write_blocks 
> (drivers/mmc.c) to preve=
> > nt a hangup for further mmc commands.
> > 
> > > Solved a checksum issue in fs/fat/fat.c. The mkcksum has 
> const char argum=
> > ents 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 cas=
> > e (at least not for the Sourcery CodeBench compiler and 
> probably not accord=
> > ing to ANSI C). This causes "long filename checksum errors" 
> for each fat fi=
> > le listed or written.
> 
> Please explain.  Under which exact conditions would  sizeof(name) not
> be 8, and where is such assumption supported in ANSI C?
> 
> 
> I am tempted to NAK the FAT changes, as they make the code much harder
> to read and to maintain.
> 
> Using this simple test program:
> 
> ----- snip ----
> #include <stdio.h>
> 
> int main(void)
> {
> 	const char name[8];
> 	const char ext[3];
> 
> 	printf("sizeof(name)=%d, expected 8\n", sizeof(name));
> 	printf("sizeof(ext) =%d, expected 3\n", sizeof(name));
> 
> 	return 0;
> }
> ----- snip ----
> 
> I get the expected values on all systems and with all compilers I
> tested.   For which exact configuration do you get different results?
> 

The difference is, that in the original code, the sizeof( ) is used on
function arguments. And in that case the result will be the size of the
pointer, regardless the addition of size indicators of the arguments.

----- snip ----

#include <stdio.h>

const char g_name[8];
const char g_ext[3];

static void mkcksum(const char name[8], const char ext[3])
{
	printf("sizeof(name)=%d\n", sizeof(name));
	printf("sizeof(ext) =%d\n", sizeof(ext));
}

int main(void)
{
	printf("sizeof(g_name)=%d\n", sizeof(g_name));
	printf("sizeof(g_ext) =%d\n", sizeof(g_ext));

	mkcksum(g_name, g_ext);
	return 0;
}

----- snip ----

In this case, the results will be:

sizeof(g_name)=8
sizeof(g_ext) =3
sizeof(name)=4	/* will be machine dependant */
sizeof(ext) =4		/* will be machine dependant */

> > > 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.
> 
> Please restrict your line length in commit messages to some 70
> characters or so.
> 

I will try...

> 
> > Signed-off-by: Ruud Commandeur <rcommandeur@clb.nl>
> > Cc: Tom Rini <trini@ti.com>
> > Cc: Beno=EEt Th=E9baudeau <benoit.thebaudeau@advansee.com>
> > Cc: Mats Karrman <Mats.Karrman@tritech.se>
> 
> Please split into three separate patches, one for MMX, and two for
> FAT, one for each problem.  And make sure to add the MMC custodian
> on Cc:
> 

I will, and I will take notice of all other comments (will not answer
them all
individually). And I guess that I could best post these as new patches
(and forget about for this one)?

And just in case you haven't noticed: This was my first patch posted...
:-)

> Best regards,
> 
> Wolfgang Denk
> 
> -- 
> DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
> HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
> Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd@denx.de
> The existence of god implies a violation of causality.
>

> -----Oorspronkelijk bericht-----
> Van: Andy Fleming [mailto:afleming@gmail.com] 
> Verzonden: donderdag 16 mei 2013 0:15
> Aan: Ruud Commandeur
> CC: U-Boot list; Tom Rini; Mats K?rrman
> Onderwerp: Re: [U-Boot] [PATCH] mmc and fat bug fixes
> 
> 
> 
> 
> On Wed, May 15, 2013 at 9:23 AM, Ruud Commandeur 
> <RCommandeur@clb.nl> wrote:
> 
> 
> 	This patch fixes a number of mmc and fat-related bugs:
> 	
> 	> Added a check for blkcnt > 0 in mmc_write_blocks 
> (drivers/mmc.c) to prevent a hangup for further mmc commands.
> 	
> 
> 
> 
> You need more information than that. Why is some code 
> requesting 0-byte data commands?
> 

I have discussed this issue in an earlier thread. But I agree that it
would make
sense to add some of this earlier comments here.

> As others mentioned, you need to break up patches so each 
> change is one patch.
> 

Yep :-)

>  
> 
> 	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
> 	
> 
> 
> 
> Assuming this is necessary, I think it then might be time to 
> reorder this:
> 
> if (!blkcnt) <-- possibly at the very start of the function.
>   return 0;
> 
> if (blkcnt == 1)
>   cmd.cmdidx = MMC_CMD_WRITE_SINGLE_BLOCK;
> else
>   cmd.cmdidx = MMC_CMD_WRITE_MULTIPLE_BLOCK;
> 
> 
> While technically correct, checking >1, then >0 creates an 
> odd dissonance in my mind, and makes me have to think about 
> when that if clause will evaluate to true, and I hate having 
> to think. :)
> 

You're right. That was the reason for adding my (wrong styled) comments.
So I can either reorder his to:

	if (blkcnt == 0)
		return 0;
	else if (blkcnt == 1)
		cmd.cmdidx = MMC_CMD_WRITE_SINGLE_BLOCK;
	else
		cmd.cmdidx = MMC_CMD_WRITE_MULTIPLE_BLOCK;

Or add the test for 0 at the very beginning as you suggested.
Any preference?

> Andy
>

On Thu, May 16, 2013 at 4:26 AM, Ruud Commandeur <RCommandeur@clb.nl> wrote:


> > Assuming this is necessary, I think it then might be time to
> > reorder this:
> >
> > if (!blkcnt) <-- possibly at the very start of the function.
> >   return 0;
> >
> > if (blkcnt == 1)
> >   cmd.cmdidx = MMC_CMD_WRITE_SINGLE_BLOCK;
> > else
> >   cmd.cmdidx = MMC_CMD_WRITE_MULTIPLE_BLOCK;
> >
> >
> > While technically correct, checking >1, then >0 creates an
> > odd dissonance in my mind, and makes me have to think about
> > when that if clause will evaluate to true, and I hate having
> > to think. :)
> >
>
> You're right. That was the reason for adding my (wrong styled) comments.
> So I can either reorder his to:
>
>         if (blkcnt == 0)
>                 return 0;
>         else if (blkcnt == 1)
>                 cmd.cmdidx = MMC_CMD_WRITE_SINGLE_BLOCK;
>         else
>                 cmd.cmdidx = MMC_CMD_WRITE_MULTIPLE_BLOCK;
>
> Or add the test for 0 at the very beginning as you suggested.
> Any preference?
>
>
This looks fine to me.