[U-Boot] [PATCH] ARM: cfi_flash: Fix unaligned accesses to cfi_qry structure

[U-Boot] [PATCH] ARM: cfi_flash: Fix unaligned accesses to cfi_qry structure

[Andrew Gabbasov]

Packed structure cfi_qry contains unaligned 16- and 32-bits members,
accessing which causes problems when cfi_flash driver is compiled with
-munaligned-access option: flash initialization hangs, probably
due to data error.

This fix converts 16- and 32-bit members to byte arrays and uses special
macros to access such fields. It removes possible unaligned accesses
in cfi_flash driver.

Signed-off-by: Andrew Gabbasov <andrew_gabbasov@mentor.com>
---
 drivers/mtd/cfi_flash.c |   23 +++++++++++++----------
 include/mtd/cfi_flash.h |   18 +++++++++++-------
 2 files changed, 24 insertions(+), 17 deletions(-)

diff --git a/drivers/mtd/cfi_flash.c b/drivers/mtd/cfi_flash.c
index 60dbb78..2112ffc 100644
--- a/drivers/mtd/cfi_flash.c
+++ b/drivers/mtd/cfi_flash.c
@@ -1636,13 +1636,16 @@ void flash_read_factory_serial (flash_info_t * info, void *buffer, int offset,
  */
 static void cfi_reverse_geometry(struct cfi_qry *qry)
 {
-	unsigned int i, j;
-	u32 tmp;
+	unsigned int i, j, k;
+	u8 tmp;
 
 	for (i = 0, j = qry->num_erase_regions - 1; i < j; i++, j--) {
-		tmp = qry->erase_region_info[i];
-		qry->erase_region_info[i] = qry->erase_region_info[j];
-		qry->erase_region_info[j] = tmp;
+		for (k = 0; k < 4; k++) {
+			tmp = qry->erase_region_info[k][i];
+			qry->erase_region_info[k][i] =
+				qry->erase_region_info[k][j];
+			qry->erase_region_info[k][j] = tmp;
+		}
 	}
 }
 
@@ -1891,7 +1894,7 @@ static int __flash_detect_cfi (flash_info_t * info, struct cfi_qry *qry)
 		    && flash_isequal (info, 0, FLASH_OFFSET_CFI_RESP + 2, 'Y')) {
 			flash_read_cfi(info, qry, FLASH_OFFSET_CFI_RESP,
 					sizeof(struct cfi_qry));
-			info->interface	= le16_to_cpu(qry->interface_desc);
+			info->interface	= cfiqry_get16(qry->interface_desc);
 
 			info->cfi_offset = flash_offset_cfi[cfi_offset];
 			debug ("device interface is %d\n",
@@ -2053,8 +2056,8 @@ ulong flash_get_size (phys_addr_t base, int banknum)
 	info->start[0] = (ulong)map_physmem(base, info->portwidth, MAP_NOCACHE);
 
 	if (flash_detect_cfi (info, &qry)) {
-		info->vendor = le16_to_cpu(qry.p_id);
-		info->ext_addr = le16_to_cpu(qry.p_adr);
+		info->vendor = cfiqry_get16(qry.p_id);
+		info->ext_addr = cfiqry_get16(qry.p_adr);
 		num_erase_regions = qry.num_erase_regions;
 
 		if (info->ext_addr) {
@@ -2140,7 +2143,7 @@ ulong flash_get_size (phys_addr_t base, int banknum)
 				break;
 			}
 
-			tmp = le32_to_cpu(qry.erase_region_info[i]);
+			tmp = cfiqry_get32(qry.erase_region_info[i]);
 			debug("erase region %u: 0x%08lx\n", i, tmp);
 
 			erase_region_count = (tmp & 0xffff) + 1;
@@ -2213,7 +2216,7 @@ ulong flash_get_size (phys_addr_t base, int banknum)
 		}
 
 		info->sector_count = sect_cnt;
-		info->buffer_size = 1 << le16_to_cpu(qry.max_buf_write_size);
+		info->buffer_size = 1 << cfiqry_get16(qry.max_buf_write_size);
 		tmp = 1 << qry.block_erase_timeout_typ;
 		info->erase_blk_tout = tmp *
 			(1 << qry.block_erase_timeout_max);
diff --git a/include/mtd/cfi_flash.h b/include/mtd/cfi_flash.h
index 966b5e0..30d6896 100644
--- a/include/mtd/cfi_flash.h
+++ b/include/mtd/cfi_flash.h
@@ -131,10 +131,10 @@ typedef union {
 /* CFI standard query structure */
 struct cfi_qry {
 	u8	qry[3];
-	u16	p_id;
-	u16	p_adr;
-	u16	a_id;
-	u16	a_adr;
+	u8	p_id[2];
+	u8	p_adr[2];
+	u8	a_id[2];
+	u8	a_adr[2];
 	u8	vcc_min;
 	u8	vcc_max;
 	u8	vpp_min;
@@ -148,10 +148,10 @@ struct cfi_qry {
 	u8	block_erase_timeout_max;
 	u8	chip_erase_timeout_max;
 	u8	dev_size;
-	u16	interface_desc;
-	u16	max_buf_write_size;
+	u8	interface_desc[2];
+	u8	max_buf_write_size[2];
 	u8	num_erase_regions;
-	u32	erase_region_info[NUM_ERASE_REGIONS];
+	u8	erase_region_info[4][NUM_ERASE_REGIONS];
 } __attribute__((packed));
 
 struct cfi_pri_hdr {
@@ -160,6 +160,10 @@ struct cfi_pri_hdr {
 	u8	minor_version;
 } __attribute__((packed));
 
+#define cfiqry_get16(mp) ((u16)((mp)[0]) | ((u16)((mp)[1]) << 8))
+#define cfiqry_get32(mp) ((u32)((mp)[0]) | ((u32)((mp)[1]) << 8) | \
+			  ((u32)((mp)[2]) << 16) | ((u32)((mp)[3]) << 24))
+
 #ifndef CONFIG_SYS_FLASH_BANKS_LIST
 #define CONFIG_SYS_FLASH_BANKS_LIST { CONFIG_SYS_FLASH_BASE }
 #endif
-- 
1.7.10.4

[U-Boot] [PATCH] ARM: cfi_flash: Fix unaligned accesses to cfi_qry structure

[Gabbasov, Andrew]

> From: Gabbasov, Andrew
> Sent: Monday, April 29, 2013 13:34
> To: u-boot at lists.denx.de
> Subject: [U-Boot][PATCH] ARM: cfi_flash: Fix unaligned accesses to cfi_qry structure
> 
> Packed structure cfi_qry contains unaligned 16- and 32-bits members,
> accessing which causes problems when cfi_flash driver is compiled with
> -munaligned-access option: flash initialization hangs, probably
> due to data error.
> 
> This fix converts 16- and 32-bit members to byte arrays and uses special
> macros to access such fields. It removes possible unaligned accesses
> in cfi_flash driver.
> 
> Signed-off-by: Andrew Gabbasov <andrew_gabbasov@mentor.com>
> ---
>  drivers/mtd/cfi_flash.c |   23 +++++++++++++----------
>  include/mtd/cfi_flash.h |   18 +++++++++++-------
>  2 files changed, 24 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/mtd/cfi_flash.c b/drivers/mtd/cfi_flash.c
> index 60dbb78..2112ffc 100644
> --- a/drivers/mtd/cfi_flash.c
> +++ b/drivers/mtd/cfi_flash.c
> @@ -1636,13 +1636,16 @@ void flash_read_factory_serial (flash_info_t * info, void *buffer, int offset,
>   */
>  static void cfi_reverse_geometry(struct cfi_qry *qry)
>  {
> -       unsigned int i, j;
> -       u32 tmp;
> +       unsigned int i, j, k;
> +       u8 tmp;
> 
>         for (i = 0, j = qry->num_erase_regions - 1; i < j; i++, j--) {
> -               tmp = qry->erase_region_info[i];
> -               qry->erase_region_info[i] = qry->erase_region_info[j];
> -               qry->erase_region_info[j] = tmp;
> +               for (k = 0; k < 4; k++) {
> +                       tmp = qry->erase_region_info[k][i];
> +                       qry->erase_region_info[k][i] =
> +                               qry->erase_region_info[k][j];
> +                       qry->erase_region_info[k][j] = tmp;
> +               }
>         }
>  }
> 
> @@ -1891,7 +1894,7 @@ static int __flash_detect_cfi (flash_info_t * info, struct cfi_qry *qry)
>                     && flash_isequal (info, 0, FLASH_OFFSET_CFI_RESP + 2, 'Y')) {
>                         flash_read_cfi(info, qry, FLASH_OFFSET_CFI_RESP,
>                                         sizeof(struct cfi_qry));
> -                       info->interface = le16_to_cpu(qry->interface_desc);
> +                       info->interface = cfiqry_get16(qry->interface_desc);
> 
>                         info->cfi_offset = flash_offset_cfi[cfi_offset];
>                         debug ("device interface is %d\n",
> @@ -2053,8 +2056,8 @@ ulong flash_get_size (phys_addr_t base, int banknum)
>         info->start[0] = (ulong)map_physmem(base, info->portwidth, MAP_NOCACHE);
> 
>         if (flash_detect_cfi (info, &qry)) {
> -               info->vendor = le16_to_cpu(qry.p_id);
> -               info->ext_addr = le16_to_cpu(qry.p_adr);
> +               info->vendor = cfiqry_get16(qry.p_id);
> +               info->ext_addr = cfiqry_get16(qry.p_adr);
>                 num_erase_regions = qry.num_erase_regions;
> 
>                 if (info->ext_addr) {
> @@ -2140,7 +2143,7 @@ ulong flash_get_size (phys_addr_t base, int banknum)
>                                 break;
>                         }
> 
> -                       tmp = le32_to_cpu(qry.erase_region_info[i]);
> +                       tmp = cfiqry_get32(qry.erase_region_info[i]);
>                         debug("erase region %u: 0x%08lx\n", i, tmp);
> 
>                         erase_region_count = (tmp & 0xffff) + 1;
> @@ -2213,7 +2216,7 @@ ulong flash_get_size (phys_addr_t base, int banknum)
>                 }
> 
>                 info->sector_count = sect_cnt;
> -               info->buffer_size = 1 << le16_to_cpu(qry.max_buf_write_size);
> +               info->buffer_size = 1 << cfiqry_get16(qry.max_buf_write_size);
>                 tmp = 1 << qry.block_erase_timeout_typ;
>                 info->erase_blk_tout = tmp *
>                         (1 << qry.block_erase_timeout_max);
> diff --git a/include/mtd/cfi_flash.h b/include/mtd/cfi_flash.h
> index 966b5e0..30d6896 100644
> --- a/include/mtd/cfi_flash.h
> +++ b/include/mtd/cfi_flash.h
> @@ -131,10 +131,10 @@ typedef union {
>  /* CFI standard query structure */
>  struct cfi_qry {
>         u8      qry[3];
> -       u16     p_id;
> -       u16     p_adr;
> -       u16     a_id;
> -       u16     a_adr;
> +       u8      p_id[2];
> +       u8      p_adr[2];
> +       u8      a_id[2];
> +       u8      a_adr[2];
>         u8      vcc_min;
>         u8      vcc_max;
>         u8      vpp_min;
> @@ -148,10 +148,10 @@ struct cfi_qry {
>         u8      block_erase_timeout_max;
>         u8      chip_erase_timeout_max;
>         u8      dev_size;
> -       u16     interface_desc;
> -       u16     max_buf_write_size;
> +       u8      interface_desc[2];
> +       u8      max_buf_write_size[2];
>         u8      num_erase_regions;
> -       u32     erase_region_info[NUM_ERASE_REGIONS];
> +       u8      erase_region_info[4][NUM_ERASE_REGIONS];
>  } __attribute__((packed));
> 
>  struct cfi_pri_hdr {
> @@ -160,6 +160,10 @@ struct cfi_pri_hdr {
>         u8      minor_version;
>  } __attribute__((packed));
> 
> +#define cfiqry_get16(mp) ((u16)((mp)[0]) | ((u16)((mp)[1]) << 8))
> +#define cfiqry_get32(mp) ((u32)((mp)[0]) | ((u32)((mp)[1]) << 8) | \
> +                         ((u32)((mp)[2]) << 16) | ((u32)((mp)[3]) << 24))
> +
>  #ifndef CONFIG_SYS_FLASH_BANKS_LIST
>  #define CONFIG_SYS_FLASH_BANKS_LIST { CONFIG_SYS_FLASH_BASE }
>  #endif
> --
> 1.7.10.4

Unfortunately, I can't verify this fix for the boards with flash configuration,
requiring reversing geometry. So, if anybody could test it on such board,
I would greatly appreciate it.

Thanks.

Best regards,
Andrew

[U-Boot] [PATCH] ARM: cfi_flash: Fix unaligned accesses to cfi_qry structure

[Marek Vasut]

Dear Andrew Gabbasov,

> Packed structure cfi_qry contains unaligned 16- and 32-bits members,
> accessing which causes problems when cfi_flash driver is compiled with
> -munaligned-access option: flash initialization hangs, probably
> due to data error.
> 
> This fix converts 16- and 32-bit members to byte arrays and uses special
> macros to access such fields. It removes possible unaligned accesses
> in cfi_flash driver.
> 
> Signed-off-by: Andrew Gabbasov <andrew_gabbasov@mentor.com>

It seems OK. I just wonder if the le16_to_cpu you removed can have no impact now 
on obscure-endian architectures, that's my only concern.

Best regards,
Marek Vasut

[U-Boot] [PATCH] ARM: cfi_flash: Fix unaligned accesses to cfi_qry structure

[Gabbasov, Andrew]

Hi Marek,

> From: Marek Vasut [marex at denx.de]
> Sent: Wednesday, May 08, 2013 21:26
> To: u-boot at lists.denx.de
> Cc: Gabbasov, Andrew; Tom Rini; Albert Aribaud
> Subject: Re: [U-Boot] [PATCH] ARM: cfi_flash: Fix unaligned accesses to cfi_qry structure
> 
> Dear Andrew Gabbasov,
> 
> > Packed structure cfi_qry contains unaligned 16- and 32-bits members,
> > accessing which causes problems when cfi_flash driver is compiled with
> > -munaligned-access option: flash initialization hangs, probably
> > due to data error.
> >
> > This fix converts 16- and 32-bit members to byte arrays and uses special
> > macros to access such fields. It removes possible unaligned accesses
> > in cfi_flash driver.
> >
> > Signed-off-by: Andrew Gabbasov <andrew_gabbasov@mentor.com>
> 
> It seems OK. I just wonder if the le16_to_cpu you removed can have no impact now
> on obscure-endian architectures, that's my only concern.
> 
> Best regards,
> Marek Vasut

Thank you for your feedback.

Using le16_to_cpu macros assumed that the data, read from flash, are always in little-endian
order, that is the LSB byte comes first in those 2-bytes unaligned fields. This is exactly how
the data is read by new macros. So, it looks like new macros should work on any host
endianness.

Thanks.

Best regards,
Andrew

[U-Boot] ARM: cfi_flash: Fix unaligned accesses to cfi_qry structure

[Masahiro Yamada]

Dear Andrew Gabbasov,

Your patch seems to newly generate the following warning and error,
because your patch is changing the type of 'qry->max_buf_write_size' from u16 to u8[2].

> cfi_flash.c:2038:33: warning: comparison between pointer and integer [enabled by default]
> cfi_flash.c:2046:27: error: incompatible types when assigning to type 'u8[2]' from type 'int'


By the way, I also had this unalignment access problem for my board.
Before finding your patch, I was thinking another way to fix this problem.

My idea is to just use 'get_unaligned' and 'put_unaligned' functions
instead of introducing special macros.
With this way, we don't need to change the fields of struct cfi_qry.

If you fix the above problem, I think your patch will be fine.
But I am wondering which way is more smart.

Best regards,
Masahiro Yamada

[U-Boot] ARM: cfi_flash: Fix unaligned accesses to cfi_qry structure

[Marek Vasut]

Hello Masahiro-san,

> Dear Andrew Gabbasov,

This way of starting emails seems to be dangerously widely adopted ;-D

> Your patch seems to newly generate the following warning and error,
> because your patch is changing the type of 'qry->max_buf_write_size' from
> u16 to u8[2].
> 
> > cfi_flash.c:2038:33: warning: comparison between pointer and integer
> > [enabled by default] cfi_flash.c:2046:27: error: incompatible types when
> > assigning to type 'u8[2]' from type 'int'

Good find.

> By the way, I also had this unalignment access problem for my board.
> Before finding your patch, I was thinking another way to fix this problem.
> 
> My idea is to just use 'get_unaligned' and 'put_unaligned' functions
> instead of introducing special macros.
> With this way, we don't need to change the fields of struct cfi_qry.

I think we should make sure to use natural alignment as much as possible, 
really. I'm keeping Albert in CC because this is his turf.

> If you fix the above problem, I think your patch will be fine.
> But I am wondering which way is more smart.
> 
> Best regards,
> Masahiro Yamada

[...]

Best regards,
Marek Vasut

[U-Boot] ARM: cfi_flash: Fix unaligned accesses to cfi_qry structure

[Masahiro Yamada]

Hi, Marek Vasut

> Hello Masahiro-san,
> 
> > Dear Andrew Gabbasov,
> 
> This way of starting emails seems to be dangerously widely adopted ;-D

Thank you for your respond, but I could not understand what you mean.
Do you mean that starting emails with "Dear" is something strange?
Starting with "Hi" or "Hello" is more natural?
I'm not very good at English, so I don't understand
English nuances and customs.
If there is something strange, please let me know. I'll appreciate it.
(You called me with  "-san". You're right. I'm Japanese.)


> I think we should make sure to use natural alignment as much as possible, 

I understand.

With all members of 'struct cfi_qry' having u8 type,
I think '__attribute__((packed))' can be omitted.


> really. I'm keeping Albert in CC because this is his turf.

Sorry for inconvenience in my previous mail, but it was not malicious.

I am new to this mailing list.
(I subscribed this U-Boot mailing list at May, 8.)

At first, I subscribed as a digested user.
When I tried to post a reply to this thread,
I recognized digest mails are not useful for replying.
I had no separated mails in my mail box and I could not simply reply
with my mail agent.

In order to post to this thread, I filled 'In-Reply-To:', 'Reference:'
fields etc by a unusual way.
And I dropped 'CC:' field accidentally.

Thanks for adding CC again.

Best regards,
Masahiro

[U-Boot] ARM: cfi_flash: Fix unaligned accesses to cfi_qry structure

[Marek Vasut]

Hello Masahiro-san

> Hi, Marek Vasut
> 
> > Hello Masahiro-san,
> > 
> > > Dear Andrew Gabbasov,
> > 
> > This way of starting emails seems to be dangerously widely adopted ;-D
> 
> Thank you for your respond, but I could not understand what you mean.
> Do you mean that starting emails with "Dear" is something strange?

No no, don't worry, I was just laughing about it :-)

> Starting with "Hi" or "Hello" is more natural?
> I'm not very good at English, so I don't understand
> English nuances and customs.
> If there is something strange, please let me know. I'll appreciate it.
> (You called me with  "-san". You're right. I'm Japanese.)

Your english is better than my japaneese :-)

> > I think we should make sure to use natural alignment as much as possible,
> 
> I understand.
> 
> With all members of 'struct cfi_qry' having u8 type,
> I think '__attribute__((packed))' can be omitted.

Yes, no padding should happen now so it'd be ok to drop this ... unless we 
compile for 64-bit architecture. Tom?

> > really. I'm keeping Albert in CC because this is his turf.
> 
> Sorry for inconvenience in my previous mail, but it was not malicious.

No no, don't worry.

> I am new to this mailing list.
> (I subscribed this U-Boot mailing list at May, 8.)
> 
> At first, I subscribed as a digested user.
> When I tried to post a reply to this thread,
> I recognized digest mails are not useful for replying.
> I had no separated mails in my mail box and I could not simply reply
> with my mail agent.
> 
> In order to post to this thread, I filled 'In-Reply-To:', 'Reference:'
> fields etc by a unusual way.
> And I dropped 'CC:' field accidentally.
> 
> Thanks for adding CC again.

It found it's way to us so it's ok.

Best regards,
Marek Vasut

[U-Boot] ARM: cfi_flash: Fix unaligned accesses to cfi_qry structure

[Albert ARIBAUD]

Hi Marek,

On Fri, 10 May 2013 14:36:00 +0200, Marek Vasut <marex@denx.de> wrote:

> Hello Masahiro-san,

> > By the way, I also had this unalignment access problem for my board.
> > Before finding your patch, I was thinking another way to fix this problem.
> > 
> > My idea is to just use 'get_unaligned' and 'put_unaligned' functions
> > instead of introducing special macros.
> > With this way, we don't need to change the fields of struct cfi_qry.
> 
> I think we should make sure to use natural alignment as much as possible, 
> really. I'm keeping Albert in CC because this is his turf.

Marek, you invoked me; next time be careful what you wish for. :)

My rules (not 'of thumb', as they also apply to ARM) regarding
alignment are as follows (yes, it's a more general answer than your
question called for, but the last rules are more directly related):

0) Never assume that U-Boot can use native unaligned accesses. Yes,
ARMv7+ can do native unaligned accesses with almost no performance
cost, but U-Boot runs on all sorts of targets (not only ARM), some
allowing unaligned access but with a penalty, and some unable to
perform unaligned access at all. We always run the risk that some code
in U-Boot ends up running on target which will die horribly on some
unaligned access, so to avoid this we must assume and enforce strict
alignment, even for architectures which do not require it, such as
ARMv7+.

1) As per rule 0, always enable alignment check -- again, even on
targets which could do without it. This allows catching any unaligned
access, be they accidental (bad pointer arithmetic) or by design
(misaligned field in an otherwise aligned struct).

2) Despite rules 0 and 1, always enable native unaligned accesses (IOW,
always use option -munaligned-accesses). That is because without this
option (or more precisely, when -mno-unaligned-accesses is in effect),
the ARM compiler may silently detect misaligned accesses and fix them
using smaller aligned accesses, thus hiding a potential alignment
issue until it bites on some later and unrelated target run.

3) always size fields in a structure to their natural size, i.e., if a
field is 16-bits, make it u16 or s16.

4) always align fields in a structure to their natural boundaries,
i.e., if a field is 16-bits, align it to an even address.

5) if a field absolutely has to be unaligned because of hardware or
standard, then a) document that! and b) access it with explicit
unaligned access macros.

So in essence, I am opposed to changing fields from 16-bit to 2 x 8-bit
just 'because unaligned'. Either fix the fields' alignment, if at all
possible; and if not, then apply rule 5: document the issue and fix it
using explicit unaligned access macros.

> Best regards,
> Marek Vasut

Amicalement,
-- 
Albert.

[U-Boot] ARM: cfi_flash: Fix unaligned accesses to cfi_qry structure

[Gabbasov, Andrew]

Hello Albert,

> From: Albert ARIBAUD [albert.u.boot at aribaud.net]
> Sent: Monday, May 13, 2013 10:48
> To: Marek Vasut
> Cc: u-boot at lists.denx.de; Masahiro Yamada; trini at ti.com; Gabbasov, Andrew
> Subject: Re: [U-Boot] ARM: cfi_flash: Fix unaligned accesses to cfi_qry structure
> 
> Hi Marek,
> 
> On Fri, 10 May 2013 14:36:00 +0200, Marek Vasut <marex@denx.de> wrote:
> 
> > Hello Masahiro-san,
> 
> > > By the way, I also had this unalignment access problem for my board.
> > > Before finding your patch, I was thinking another way to fix this problem.
> > >
> > > My idea is to just use 'get_unaligned' and 'put_unaligned' functions
> > > instead of introducing special macros.
> > > With this way, we don't need to change the fields of struct cfi_qry.
> >
> > I think we should make sure to use natural alignment as much as possible,
> > really. I'm keeping Albert in CC because this is his turf.
> 
> Marek, you invoked me; next time be careful what you wish for. :)
> 
> My rules (not 'of thumb', as they also apply to ARM) regarding
> alignment are as follows (yes, it's a more general answer than your
> question called for, but the last rules are more directly related):
> 
> 0) Never assume that U-Boot can use native unaligned accesses. Yes,
> ARMv7+ can do native unaligned accesses with almost no performance
> cost, but U-Boot runs on all sorts of targets (not only ARM), some
> allowing unaligned access but with a penalty, and some unable to
> perform unaligned access at all. We always run the risk that some code
> in U-Boot ends up running on target which will die horribly on some
> unaligned access, so to avoid this we must assume and enforce strict
> alignment, even for architectures which do not require it, such as
> ARMv7+.
> 
> 1) As per rule 0, always enable alignment check -- again, even on
> targets which could do without it. This allows catching any unaligned
> access, be they accidental (bad pointer arithmetic) or by design
> (misaligned field in an otherwise aligned struct).
> 
> 2) Despite rules 0 and 1, always enable native unaligned accesses (IOW,
> always use option -munaligned-accesses). That is because without this
> option (or more precisely, when -mno-unaligned-accesses is in effect),
> the ARM compiler may silently detect misaligned accesses and fix them
> using smaller aligned accesses, thus hiding a potential alignment
> issue until it bites on some later and unrelated target run.
> 
> 3) always size fields in a structure to their natural size, i.e., if a
> field is 16-bits, make it u16 or s16.
> 
> 4) always align fields in a structure to their natural boundaries,
> i.e., if a field is 16-bits, align it to an even address.
> 
> 5) if a field absolutely has to be unaligned because of hardware or
> standard, then a) document that! and b) access it with explicit
> unaligned access macros.
> 
> So in essence, I am opposed to changing fields from 16-bit to 2 x 8-bit
> just 'because unaligned'. Either fix the fields' alignment, if at all
> possible; and if not, then apply rule 5: document the issue and fix it
> using explicit unaligned access macros.
> 
> > Best regards,
> > Marek Vasut
> 
> Amicalement,
> --
> Albert.

Thank you for your review and the very useful list of rules.

Although theoretically the cfi_qry structure can be made non-packed and
with properly aligned members (and filled in by individual members assignments
when reading, rather than just copying byte-to-byte, as it is now),
it may make more sense to keep it packed and replicating the actual flash layout
as much as possible. This also makes it more similar to what is used
in Linux kernel (although it seems to rely on native unaligned access ;-)).

So, it seems reasonable to choose to apply rule 5: add comments to cfi_qry strcuture
definition and use get_unaligned/put_unaligned for the necessary fields (only for really
unaligned members, since some 16-bit fields are properly aligned).

I'm sending the version 2 of the patch with this change as a separate message with
Subject: [U-Boot][PATCH v2] cfi_flash: Fix unaligned accesses to cfi_qry structure

Please, let me know if somebody still prefers making non-packed aligned structure,
as described above.

Thanks.

Best regards,
Andrew

[U-Boot] ARM: cfi_flash: Fix unaligned accesses to cfi_qry structure

[Gabbasov, Andrew]

> From: Gabbasov, Andrew
> Sent: Tuesday, May 14, 2013 21:27
> To: Albert ARIBAUD; Marek Vasut
> Cc: u-boot at lists.denx.de; Masahiro Yamada; trini at ti.com
> Subject: RE: [U-Boot] ARM: cfi_flash: Fix unaligned accesses to cfi_qry structure
> 
> Hello Albert,
> 
> > From: Albert ARIBAUD [albert.u.boot at aribaud.net]
> > Sent: Monday, May 13, 2013 10:48
> > To: Marek Vasut
> > Cc: u-boot at lists.denx.de; Masahiro Yamada; trini at ti.com; Gabbasov, Andrew
> > Subject: Re: [U-Boot] ARM: cfi_flash: Fix unaligned accesses to cfi_qry structure
> >
> > Hi Marek,
> >
> > On Fri, 10 May 2013 14:36:00 +0200, Marek Vasut <marex@denx.de> wrote:
> >
> > > Hello Masahiro-san,
> >
> > > > By the way, I also had this unalignment access problem for my board.
> > > > Before finding your patch, I was thinking another way to fix this problem.
> > > >
> > > > My idea is to just use 'get_unaligned' and 'put_unaligned' functions
> > > > instead of introducing special macros.
> > > > With this way, we don't need to change the fields of struct cfi_qry.
> > >
> > > I think we should make sure to use natural alignment as much as possible,
> > > really. I'm keeping Albert in CC because this is his turf.
> >
> > Marek, you invoked me; next time be careful what you wish for. :)
> >
> > My rules (not 'of thumb', as they also apply to ARM) regarding
> > alignment are as follows (yes, it's a more general answer than your
> > question called for, but the last rules are more directly related):
> >
> > 0) Never assume that U-Boot can use native unaligned accesses. Yes,
> > ARMv7+ can do native unaligned accesses with almost no performance
> > cost, but U-Boot runs on all sorts of targets (not only ARM), some
> > allowing unaligned access but with a penalty, and some unable to
> > perform unaligned access at all. We always run the risk that some code
> > in U-Boot ends up running on target which will die horribly on some
> > unaligned access, so to avoid this we must assume and enforce strict
> > alignment, even for architectures which do not require it, such as
> > ARMv7+.
> >
> > 1) As per rule 0, always enable alignment check -- again, even on
> > targets which could do without it. This allows catching any unaligned
> > access, be they accidental (bad pointer arithmetic) or by design
> > (misaligned field in an otherwise aligned struct).
> >
> > 2) Despite rules 0 and 1, always enable native unaligned accesses (IOW,
> > always use option -munaligned-accesses). That is because without this
> > option (or more precisely, when -mno-unaligned-accesses is in effect),
> > the ARM compiler may silently detect misaligned accesses and fix them
> > using smaller aligned accesses, thus hiding a potential alignment
> > issue until it bites on some later and unrelated target run.
> >
> > 3) always size fields in a structure to their natural size, i.e., if a
> > field is 16-bits, make it u16 or s16.
> >
> > 4) always align fields in a structure to their natural boundaries,
> > i.e., if a field is 16-bits, align it to an even address.
> >
> > 5) if a field absolutely has to be unaligned because of hardware or
> > standard, then a) document that! and b) access it with explicit
> > unaligned access macros.
> >
> > So in essence, I am opposed to changing fields from 16-bit to 2 x 8-bit
> > just 'because unaligned'. Either fix the fields' alignment, if at all
> > possible; and if not, then apply rule 5: document the issue and fix it
> > using explicit unaligned access macros.
> >
> > > Best regards,
> > > Marek Vasut
> >
> > Amicalement,
> > --
> > Albert.
> 
> Thank you for your review and the very useful list of rules.
> 
> Although theoretically the cfi_qry structure can be made non-packed and
> with properly aligned members (and filled in by individual members assignments
> when reading, rather than just copying byte-to-byte, as it is now),
> it may make more sense to keep it packed and replicating the actual flash layout
> as much as possible. This also makes it more similar to what is used
> in Linux kernel (although it seems to rely on native unaligned access ;-)).
> 
> So, it seems reasonable to choose to apply rule 5: add comments to cfi_qry strcuture
> definition and use get_unaligned/put_unaligned for the necessary fields (only for really
> unaligned members, since some 16-bit fields are properly aligned).
> 
> I'm sending the version 2 of the patch with this change as a separate message with
> Subject: [U-Boot][PATCH v2] cfi_flash: Fix unaligned accesses to cfi_qry structure
> 
> Please, let me know if somebody still prefers making non-packed aligned structure,
> as described above.
> 
> Thanks.
> 
> Best regards,
> Andrew

Does anybody have any comments on the mentioned updated patch?

Thanks.

Best regards,
Andrew