[U-Boot] [PATCH] fat: fix unaligned errors

[U-Boot] [PATCH] fat: fix unaligned errors

[Mike Frysinger]

From: Bryan Wu <bryan.wu@analog.com>

A couple of buffers in the fat code are declared as an array of bytes.  But
it is then cast up to a structure with 16bit and 32bit members.  Since GCC
assumes structure alignment here, we have to force the buffers to be
aligned according to the structure usage.

Signed-off-by: Bryan Wu <bryan.wu@analog.com>
Signed-off-by: Mike Frysinger <vapier@gentoo.org>
---
 fs/fat/fat.c |   19 +++++++++++++++----
 1 files changed, 15 insertions(+), 4 deletions(-)

diff --git a/fs/fat/fat.c b/fs/fat/fat.c
index a9dde7d..06736d9 100644
--- a/fs/fat/fat.c
+++ b/fs/fat/fat.c
@@ -429,12 +429,16 @@ slot2str(dir_slot *slotptr, char *l_name, int *idx)
 
 
 /*
+ * We need align this buffer to 16bit, cause it will be cast up to
+ * a dir_slot structure
+ */
+__u8 get_vfatname_block[MAX_CLUSTSIZE] __attribute__ ((aligned(sizeof(__u16))));
+/*
  * Extract the full long filename starting at 'retdent' (which is really
  * a slot) into 'l_name'. If successful also copy the real directory entry
  * into 'retdent'
  * Return 0 on success, -1 otherwise.
  */
-__u8	 get_vfatname_block[MAX_CLUSTSIZE];
 static int
 get_vfatname(fsdata *mydata, int curclust, __u8 *cluster,
 	     dir_entry *retdent, char *l_name)
@@ -517,10 +521,14 @@ mkcksum(const char *str)
 
 
 /*
+ * We need align this buffer to 32bit, cause it will be cast up to
+ * a dir_entry structure
+ */
+__u8 get_dentfromdir_block[MAX_CLUSTSIZE] __attribute__ ((aligned(sizeof(__u32))));
+/*
  * Get the directory entry associated with 'filename' from the directory
  * starting at 'startsect'
  */
-__u8 get_dentfromdir_block[MAX_CLUSTSIZE];
 static dir_entry *get_dentfromdir (fsdata * mydata, int startsect,
 				   char *filename, dir_entry * retdent,
 				   int dols)
@@ -725,8 +733,11 @@ read_bootsectandvi(boot_sector *bs, volume_info *volinfo, int *fatsize)
 	return -1;
 }
 
-
-__u8 do_fat_read_block[MAX_CLUSTSIZE];  /* Block buffer */
+/*
+ * We need align this buffer to 32bit, cause it will be cast up to
+ * a dir_entry structure
+ */
+__u8 do_fat_read_block[MAX_CLUSTSIZE] __attribute__ ((aligned(sizeof(__u32))));
 long
 do_fat_read (const char *filename, void *buffer, unsigned long maxsize,
 	     int dols)
-- 
1.6.0.6

[U-Boot] [PATCH] fat: fix unaligned errors

[Remy Bohmer]

Hello Mike,

2009/1/2 Mike Frysinger <vapier@gentoo.org>:
> From: Bryan Wu <bryan.wu@analog.com>
> A couple of buffers in the fat code are declared as an array of bytes.  But
> it is then cast up to a structure with 16bit and 32bit members.  Since GCC
> assumes structure alignment here, we have to force the buffers to be
> aligned according to the structure usage.

Just curious (because before the Christmas holidays I was also
debugging some FAT problems, and maybe this is related)
What problem/symptoms does this patch fix? How did the bug show itself?

Kind Regards,

Remy

>
> Signed-off-by: Bryan Wu <bryan.wu@analog.com>
> Signed-off-by: Mike Frysinger <vapier@gentoo.org>
> ---
>  fs/fat/fat.c |   19 +++++++++++++++----
>  1 files changed, 15 insertions(+), 4 deletions(-)
>
> diff --git a/fs/fat/fat.c b/fs/fat/fat.c
> index a9dde7d..06736d9 100644
> --- a/fs/fat/fat.c
> +++ b/fs/fat/fat.c
> @@ -429,12 +429,16 @@ slot2str(dir_slot *slotptr, char *l_name, int *idx)
>
>
>  /*
> + * We need align this buffer to 16bit, cause it will be cast up to
> + * a dir_slot structure
> + */
> +__u8 get_vfatname_block[MAX_CLUSTSIZE] __attribute__ ((aligned(sizeof(__u16))));
> +/*
>  * Extract the full long filename starting at 'retdent' (which is really
>  * a slot) into 'l_name'. If successful also copy the real directory entry
>  * into 'retdent'
>  * Return 0 on success, -1 otherwise.
>  */
> -__u8    get_vfatname_block[MAX_CLUSTSIZE];
>  static int
>  get_vfatname(fsdata *mydata, int curclust, __u8 *cluster,
>             dir_entry *retdent, char *l_name)
> @@ -517,10 +521,14 @@ mkcksum(const char *str)
>
>
>  /*
> + * We need align this buffer to 32bit, cause it will be cast up to
> + * a dir_entry structure
> + */
> +__u8 get_dentfromdir_block[MAX_CLUSTSIZE] __attribute__ ((aligned(sizeof(__u32))));
> +/*
>  * Get the directory entry associated with 'filename' from the directory
>  * starting at 'startsect'
>  */
> -__u8 get_dentfromdir_block[MAX_CLUSTSIZE];
>  static dir_entry *get_dentfromdir (fsdata * mydata, int startsect,
>                                   char *filename, dir_entry * retdent,
>                                   int dols)
> @@ -725,8 +733,11 @@ read_bootsectandvi(boot_sector *bs, volume_info *volinfo, int *fatsize)
>        return -1;
>  }
>
> -
> -__u8 do_fat_read_block[MAX_CLUSTSIZE];  /* Block buffer */
> +/*
> + * We need align this buffer to 32bit, cause it will be cast up to
> + * a dir_entry structure
> + */
> +__u8 do_fat_read_block[MAX_CLUSTSIZE] __attribute__ ((aligned(sizeof(__u32))));
>  long
>  do_fat_read (const char *filename, void *buffer, unsigned long maxsize,
>             int dols)
> --
> 1.6.0.6
>
> _______________________________________________
> U-Boot mailing list
> U-Boot at lists.denx.de
> http://lists.denx.de/mailman/listinfo/u-boot
>

[U-Boot] [PATCH] fat: fix unaligned errors

[Mike Frysinger]

On Friday 02 January 2009 04:32:39 Remy Bohmer wrote:
> 2009/1/2 Mike Frysinger <vapier@gentoo.org>:
> > From: Bryan Wu <bryan.wu@analog.com>
> > A couple of buffers in the fat code are declared as an array of bytes. 
> > But it is then cast up to a structure with 16bit and 32bit members. 
> > Since GCC assumes structure alignment here, we have to force the buffers
> > to be aligned according to the structure usage.
>
> Just curious (because before the Christmas holidays I was also
> debugging some FAT problems, and maybe this is related)
> What problem/symptoms does this patch fix? How did the bug show itself?

i think should cover all your questions ...
http://blackfin.uclinux.org/gf/tracker/4765
-mike
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 835 bytes
Desc: This is a digitally signed message part.
Url : http://lists.denx.de/pipermail/u-boot/attachments/20090102/19a9146d/attachment.pgp 

[U-Boot] [PATCH] fat: fix unaligned errors

[Wolfgang Denk]

Dear Mike Frysinger,

In message <1230857672-11798-1-git-send-email-vapier@gentoo.org> you wrote:
> From: Bryan Wu <bryan.wu@analog.com>
> 
> A couple of buffers in the fat code are declared as an array of bytes.  But
> it is then cast up to a structure with 16bit and 32bit members.  Since GCC
> assumes structure alignment here, we have to force the buffers to be
> aligned according to the structure usage.
...
> +__u8 get_vfatname_block[MAX_CLUSTSIZE] __attribute__ ((aligned(sizeof(__u16))));
...
> +__u8 get_dentfromdir_block[MAX_CLUSTSIZE] __attribute__ ((aligned(sizeof(__u32))));
...
> +__u8 do_fat_read_block[MAX_CLUSTSIZE] __attribute__ ((aligned(sizeof(__u32))));

What makes you sure that a 16 resp. 32 bit alignment is sufficient,
and that gcc does not decide to align such structures even stricter?

Wouldn't it make more sense to use "__alignof__ ()" here to be on the
safe side?

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
One friend in a lifetime is much; two  are  many;  three  are  hardly
possible. Friendship needs a certain parallelism of life, a community
of thought, a rivalry of aim.                     - Henry Brook Adams

[U-Boot] [PATCH] fat: fix unaligned errors

[Mike Frysinger]

On Friday 02 January 2009 17:20:47 Wolfgang Denk wrote:
> In message <vapier@gentoo.org> you wrote:
> > From: Bryan Wu <bryan.wu@analog.com>
> >
> > A couple of buffers in the fat code are declared as an array of bytes. 
> > But it is then cast up to a structure with 16bit and 32bit members. 
> > Since GCC assumes structure alignment here, we have to force the buffers
> > to be aligned according to the structure usage.
>
> ...
>
> > +__u8 get_vfatname_block[MAX_CLUSTSIZE] __attribute__
> > ((aligned(sizeof(__u16))));
>
> ...
>
> > +__u8 get_dentfromdir_block[MAX_CLUSTSIZE] __attribute__
> > ((aligned(sizeof(__u32))));
>
> ...
>
> > +__u8 do_fat_read_block[MAX_CLUSTSIZE] __attribute__
> > ((aligned(sizeof(__u32))));
>
> What makes you sure that a 16 resp. 32 bit alignment is sufficient,
> and that gcc does not decide to align such structures even stricter?

read the structures in question.  the largest member is 16bit/32bit.  i really 
dont know what other alignment gcc could randomly generate.

> Wouldn't it make more sense to use "__alignof__ ()" here to be on the
> safe side?

i found no such alignment attribute in the gcc docs which is why i suggested 
Bryan use aligned(sizeof(...)).
http://gcc.gnu.org/onlinedocs/gcc-4.3.2/gcc/Function-Attributes.html
http://gcc.gnu.org/onlinedocs/gcc-4.3.2/gcc/Variable-Attributes.html
http://gcc.gnu.org/onlinedocs/gcc-4.3.2/gcc/Type-Attributes.html
-mike
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 835 bytes
Desc: This is a digitally signed message part.
Url : http://lists.denx.de/pipermail/u-boot/attachments/20090102/1ce92c0d/attachment.pgp 

[U-Boot] [PATCH] fat: fix unaligned errors

[Wolfgang Denk]

Dear Mike,

In message <200901021742.05363.vapier@gentoo.org> you wrote:
>
> > > +__u8 get_vfatname_block[MAX_CLUSTSIZE] __attribute__
> > > ((aligned(sizeof(__u16))));
> > ...
> > > +__u8 get_dentfromdir_block[MAX_CLUSTSIZE] __attribute__
> > > ((aligned(sizeof(__u32))));
> > ...
> > > +__u8 do_fat_read_block[MAX_CLUSTSIZE] __attribute__
> > > ((aligned(sizeof(__u32))));
> >
> > What makes you sure that a 16 resp. 32 bit alignment is sufficient,
> > and that gcc does not decide to align such structures even stricter?
> 
> read the structures in question.  the largest member is 16bit/32bit.  i really 
> dont know what other alignment gcc could randomly generate.

It is not (only) the size of the "largest" member in a struct which
determins global alignment, but also the size of the struct itself.

See for example
http://gcc.gnu.org/onlinedocs/gcc-4.3.2/gcc/Type-Attributes.html

Read especially this part:

	In the example above, if the size of each short is 2 bytes,
	then the size of the entire struct S type is 6 bytes. The
	smallest power of two which is greater than or equal to that
	is 8, so the compiler sets the alignment for the entire struct
	S type to 8 bytes. 

> > Wouldn't it make more sense to use "__alignof__ ()" here to be on the
> > safe side?
>
> i found no such alignment attribute in the gcc docs which is why i suggested 
> Bryan use aligned(sizeof(...)).
> http://gcc.gnu.org/onlinedocs/gcc-4.3.2/gcc/Function-Attributes.html
> http://gcc.gnu.org/onlinedocs/gcc-4.3.2/gcc/Variable-Attributes.html
> http://gcc.gnu.org/onlinedocs/gcc-4.3.2/gcc/Type-Attributes.html

Maybe you should have searched for the (IMHO pretty obvious) term
"Alignment" as well, i.e. look at
http://gcc.gnu.org/onlinedocs/gcc-4.3.2/gcc/Alignment.html#Alignment

Quote:

	The keyword __alignof__ allows you to inquire about how an
	object is aligned, or the minimum alignment usually required
	by a type. Its syntax is just like sizeof. 


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 more sins you confess, the more books you will sell.

[U-Boot] [PATCH] fat: fix unaligned errors

[Mike Frysinger]

On Friday 02 January 2009 18:48:50 Wolfgang Denk wrote:
> In message <200901021742.05363.vapier@gentoo.org> you wrote:
> > > > +__u8 get_vfatname_block[MAX_CLUSTSIZE] __attribute__
> > > > ((aligned(sizeof(__u16))));
> > >
> > > ...
> > >
> > > > +__u8 get_dentfromdir_block[MAX_CLUSTSIZE] __attribute__
> > > > ((aligned(sizeof(__u32))));
> > >
> > > ...
> > >
> > > > +__u8 do_fat_read_block[MAX_CLUSTSIZE] __attribute__
> > > > ((aligned(sizeof(__u32))));
> > >
> > > What makes you sure that a 16 resp. 32 bit alignment is sufficient,
> > > and that gcc does not decide to align such structures even stricter?
> >
> > read the structures in question.  the largest member is 16bit/32bit.  i
> > really dont know what other alignment gcc could randomly generate.
>
> It is not (only) the size of the "largest" member in a struct which
> determins global alignment, but also the size of the struct itself.
>
> See for example
> http://gcc.gnu.org/onlinedocs/gcc-4.3.2/gcc/Type-Attributes.html
>
> Read especially this part:
>
> 	In the example above, if the size of each short is 2 bytes,
> 	then the size of the entire struct S type is 6 bytes. The
> 	smallest power of two which is greater than or equal to that
> 	is 8, so the compiler sets the alignment for the entire struct
> 	S type to 8 bytes.

i was not aware of this, thanks for the highlight

> > > Wouldn't it make more sense to use "__alignof__ ()" here to be on the
> > > safe side?
> >
> > i found no such alignment attribute in the gcc docs which is why i
> > suggested Bryan use aligned(sizeof(...)).
> > http://gcc.gnu.org/onlinedocs/gcc-4.3.2/gcc/Function-Attributes.html
> > http://gcc.gnu.org/onlinedocs/gcc-4.3.2/gcc/Variable-Attributes.html
> > http://gcc.gnu.org/onlinedocs/gcc-4.3.2/gcc/Type-Attributes.html
>
> Maybe you should have searched for the (IMHO pretty obvious) term
> "Alignment" as well, i.e. look at
> http://gcc.gnu.org/onlinedocs/gcc-4.3.2/gcc/Alignment.html#Alignment
>
> Quote:
>
> 	The keyword __alignof__ allows you to inquire about how an
> 	object is aligned, or the minimum alignment usually required
> 	by a type. Its syntax is just like sizeof.

well i guess we cant all be "Wolfgang Denk"s
-mike
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 835 bytes
Desc: This is a digitally signed message part.
Url : http://lists.denx.de/pipermail/u-boot/attachments/20090102/b1503012/attachment-0001.pgp 

[U-Boot] [PATCH v2] fat: fix unaligned errors

[Mike Frysinger]

From: Bryan Wu <bryan.wu@analog.com>

A couple of buffers in the fat code are declared as an array of bytes.  But
it is then cast up to a structure with 16bit and 32bit members.  Since GCC
assumes structure alignment here, we have to force the buffers to be
aligned according to the structure usage.

Signed-off-by: Bryan Wu <bryan.wu@analog.com>
Signed-off-by: Mike Frysinger <vapier@gentoo.org>
---
v1
	- use alignof() rather than sizeof()

 fs/fat/fat.c |    8 +++++---
 1 files changed, 5 insertions(+), 3 deletions(-)

diff --git a/fs/fat/fat.c b/fs/fat/fat.c
index a9dde7d..28c7805 100644
--- a/fs/fat/fat.c
+++ b/fs/fat/fat.c
@@ -434,7 +434,8 @@ slot2str(dir_slot *slotptr, char *l_name, int *idx)
  * into 'retdent'
  * Return 0 on success, -1 otherwise.
  */
-__u8	 get_vfatname_block[MAX_CLUSTSIZE];
+__attribute__ ((__aligned__(__alignof__(dir_entry))))
+__u8 get_vfatname_block[MAX_CLUSTSIZE];
 static int
 get_vfatname(fsdata *mydata, int curclust, __u8 *cluster,
 	     dir_entry *retdent, char *l_name)
@@ -520,6 +521,7 @@ mkcksum(const char *str)
  * Get the directory entry associated with 'filename' from the directory
  * starting at 'startsect'
  */
+__attribute__ ((__aligned__(__alignof__(dir_entry))))
 __u8 get_dentfromdir_block[MAX_CLUSTSIZE];
 static dir_entry *get_dentfromdir (fsdata * mydata, int startsect,
 				   char *filename, dir_entry * retdent,
@@ -725,8 +727,8 @@ read_bootsectandvi(boot_sector *bs, volume_info *volinfo, int *fatsize)
 	return -1;
 }
 
-
-__u8 do_fat_read_block[MAX_CLUSTSIZE];  /* Block buffer */
+__attribute__ ((__aligned__(__alignof__(dir_entry))))
+__u8 do_fat_read_block[MAX_CLUSTSIZE];
 long
 do_fat_read (const char *filename, void *buffer, unsigned long maxsize,
 	     int dols)
-- 
1.6.0.6

[U-Boot] [PATCH v2] fat: fix unaligned errors

[Wolfgang Denk]

Dear Mike Frysinger,

In message <1230947265-19412-1-git-send-email-vapier@gentoo.org> you wrote:
> From: Bryan Wu <bryan.wu@analog.com>
> 
> A couple of buffers in the fat code are declared as an array of bytes.  But
> it is then cast up to a structure with 16bit and 32bit members.  Since GCC
> assumes structure alignment here, we have to force the buffers to be
> aligned according to the structure usage.
> 
> Signed-off-by: Bryan Wu <bryan.wu@analog.com>
> Signed-off-by: Mike Frysinger <vapier@gentoo.org>
> ---
> v1
> 	- use alignof() rather than sizeof()
> 
>  fs/fat/fat.c |    8 +++++---
>  1 files changed, 5 insertions(+), 3 deletions(-)

Applied, thanks.

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
"There is such a fine line between genius and stupidity."
- David St. Hubbins, "Spinal Tap"