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

[Ruud Commandeur]

Dear Wolfgang Denk,

Thanks for your comments, please see mine below.

> -----Oorspronkelijk bericht-----
> Van: Wolfgang Denk [mailto:wd at denx.de] 
> Verzonden: donderdag 16 mei 2013 7:55
> Aan: Ruud Commandeur
> CC: u-boot at lists.denx.de; Tom Rini; Mats K?rrman
> Onderwerp: Re: [U-Boot] [PATCH] mmc and fat bug fixes
> 
> Dear Ruud Commandeur,
> 
> In message 
> <15AE5A936F5E3A42A9144E66875A0A89308F2E at 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 at clb.nl>
> > Cc: Tom Rini <trini at ti.com>
> > Cc: Beno=EEt Th=E9baudeau <benoit.thebaudeau at advansee.com>
> > Cc: Mats Karrman <Mats.Karrman at 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 at denx.de
> The existence of god implies a violation of causality.
>