[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
From: Gabbasov, Andrew Sent: Monday, April 29, 2013 13:34 To: u-boot@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
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
Hi Marek,
From: Marek Vasut [marex@denx.de] Sent: Wednesday, May 08, 2013 21:26 To: u-boot@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
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
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
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
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
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,
Hello Albert,
From: Albert ARIBAUD [albert.u.boot@aribaud.net] Sent: Monday, May 13, 2013 10:48 To: Marek Vasut Cc: u-boot@lists.denx.de; Masahiro Yamada; trini@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):
- 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+.
- 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).
- 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.
- always size fields in a structure to their natural size, i.e., if a
field is 16-bits, make it u16 or s16.
- always align fields in a structure to their natural boundaries,
i.e., if a field is 16-bits, align it to an even address.
- 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
From: Gabbasov, Andrew Sent: Tuesday, May 14, 2013 21:27 To: Albert ARIBAUD; Marek Vasut Cc: u-boot@lists.denx.de; Masahiro Yamada; trini@ti.com Subject: RE: [U-Boot] ARM: cfi_flash: Fix unaligned accesses to cfi_qry structure
Hello Albert,
From: Albert ARIBAUD [albert.u.boot@aribaud.net] Sent: Monday, May 13, 2013 10:48 To: Marek Vasut Cc: u-boot@lists.denx.de; Masahiro Yamada; trini@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):
- 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+.
- 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).
- 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.
- always size fields in a structure to their natural size, i.e., if a
field is 16-bits, make it u16 or s16.
- always align fields in a structure to their natural boundaries,
i.e., if a field is 16-bits, align it to an even address.
- 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
participants (5)
-
Albert ARIBAUD -
Andrew Gabbasov -
Gabbasov, Andrew
-
Marek Vasut -
Masahiro Yamada