Re: [U-Boot] ARM: cfi_flash: Fix unaligned accesses to cfi_qry structure
From: Gabbasov, Andrew Sent: Tuesday, May 14, 2013 21:27 To: Albert ARIBAUD; Marek Vasut Cc: u-boot@lists.denx.de; Masahiro Yamada; tr...@ti.com Subject: RE: [U-Boot] ARM: cfi_flash: Fix unaligned accesses to cfi_qry structure Hello Albert, From: Albert ARIBAUD [albert.u.b...@aribaud.net] Sent: Monday, May 13, 2013 10:48 To: Marek Vasut Cc: u-boot@lists.denx.de; Masahiro Yamada; tr...@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 ma...@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 ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] ARM: cfi_flash: Fix unaligned accesses to cfi_qry structure
Hello Albert, From: Albert ARIBAUD [albert.u.b...@aribaud.net] Sent: Monday, May 13, 2013 10:48 To: Marek Vasut Cc: u-boot@lists.denx.de; Masahiro Yamada; tr...@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 ma...@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 mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
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 ma...@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 mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] ARM: cfi_flash: Fix unaligned accesses to cfi_qry structure
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 mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] ARM: cfi_flash: Fix unaligned accesses to cfi_qry structure
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 mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] ARM: cfi_flash: Fix unaligned accesses to cfi_qry structure
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 mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] ARM: cfi_flash: Fix unaligned accesses to cfi_qry structure
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 mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
