Message ID | 1330005966-1444-5-git-send-email-aneesh@ti.com |
---|---|
State | Accepted |
Commit | 03f69dc6fdc34e5e51f2c09373ff1bea7d16e36b |
Delegated to: | Tom Rini |
Headers | show |
On Thu, Feb 23, 2012 at 7:06 AM, Aneesh V <aneesh@ti.com> wrote: > Avoid using __attribute__ ((__packed__)) unless it's > absolutely necessary. "packed" will remove alignment > requirements for the respective objects and may cause > alignment issues unless alignment is also enforced > using a pragma. > > Here, these packed attributes were causing alignment > faults in Thumb build. > > Signed-off-by: Aneesh V <aneesh@ti.com> Why did we pack these to start with? Otherwise seems fine (and I see the rest of the TI parts don't have this particular packing).
On Thursday 23 February 2012 07:51 PM, Tom Rini wrote: > On Thu, Feb 23, 2012 at 7:06 AM, Aneesh V<aneesh@ti.com> wrote: >> Avoid using __attribute__ ((__packed__)) unless it's >> absolutely necessary. "packed" will remove alignment >> requirements for the respective objects and may cause >> alignment issues unless alignment is also enforced >> using a pragma. >> >> Here, these packed attributes were causing alignment >> faults in Thumb build. >> >> Signed-off-by: Aneesh V<aneesh@ti.com> > > Why did we pack these to start with? Otherwise seems fine (and I see > the rest of the TI parts don't have this particular packing). > I think that was to save some space - to make sure that the compiler didn't pad the structure to have the u16 fields at word boundary. But even without "packed" the complier is not padding it. I checked that today, the size of the mux arrays remain the same even after removing the "packed". So, I guess the "packed" didn't have any impact.
On Thursday 23 February 2012 09:21:00 Tom Rini wrote: > On Thu, Feb 23, 2012 at 7:06 AM, Aneesh V <aneesh@ti.com> wrote: > > Avoid using __attribute__ ((__packed__)) unless it's > > absolutely necessary. "packed" will remove alignment > > requirements for the respective objects and may cause > > alignment issues unless alignment is also enforced > > using a pragma. > > > > Here, these packed attributes were causing alignment > > faults in Thumb build. > > Why did we pack these to start with? Otherwise seems fine (and I see > the rest of the TI parts don't have this particular packing). because these represent hardware register blocks which get used with writew() and typically those blocks get marked packed. if the arch won't introduce padding with the members, then this change should be ok, and looking at the structs which are just 2 16bit members, that should be the case here. if you really want to be pedantic, i think the alternative would be: struct pad_conf_entry {} __packed __aligned(2); -mike
On Thursday 23 February 2012 09:56:24 Aneesh V wrote: > On Thursday 23 February 2012 07:51 PM, Tom Rini wrote: > > On Thu, Feb 23, 2012 at 7:06 AM, Aneesh V<aneesh@ti.com> wrote: > >> Avoid using __attribute__ ((__packed__)) unless it's > >> absolutely necessary. "packed" will remove alignment > >> requirements for the respective objects and may cause > >> alignment issues unless alignment is also enforced > >> using a pragma. > >> > >> Here, these packed attributes were causing alignment > >> faults in Thumb build. > > > > Why did we pack these to start with? Otherwise seems fine (and I see > > the rest of the TI parts don't have this particular packing). > > I think that was to save some space - to make sure that the compiler > didn't pad the structure to have the u16 fields at word boundary. But > even without "packed" the complier is not padding it. I checked that > today, the size of the mux arrays remain the same even after removing > the "packed". So, I guess the "packed" didn't have any impact. i don't think so. see my other reply. -mike
On Thursday 23 February 2012 08:33 PM, Mike Frysinger wrote: > On Thursday 23 February 2012 09:21:00 Tom Rini wrote: >> On Thu, Feb 23, 2012 at 7:06 AM, Aneesh V<aneesh@ti.com> wrote: >>> Avoid using __attribute__ ((__packed__)) unless it's >>> absolutely necessary. "packed" will remove alignment >>> requirements for the respective objects and may cause >>> alignment issues unless alignment is also enforced >>> using a pragma. >>> >>> Here, these packed attributes were causing alignment >>> faults in Thumb build. >> >> Why did we pack these to start with? Otherwise seems fine (and I see >> the rest of the TI parts don't have this particular packing). > > because these represent hardware register blocks which get used with writew() > and typically those blocks get marked packed. No. That's not the case. This is just a table that has the mux data for the board. It's not directly mapped to hardware. The hardware register offset is part of this table. > > if the arch won't introduce padding with the members, then this change should > be ok, and looking at the structs which are just 2 16bit members, that should > be the case here. > > if you really want to be pedantic, i think the alternative would be: > struct pad_conf_entry {} __packed __aligned(2); I had tried that and it had solved the problem too, but the "packed" is not needed at all. That's purely a software table. regards, Aneesh
diff --git a/arch/arm/include/asm/arch-omap4/mux_omap4.h b/arch/arm/include/asm/arch-omap4/mux_omap4.h index 30bfad7..4de7c70 100644 --- a/arch/arm/include/asm/arch-omap4/mux_omap4.h +++ b/arch/arm/include/asm/arch-omap4/mux_omap4.h @@ -34,7 +34,7 @@ struct pad_conf_entry { u16 val; -} __attribute__ ((packed)); +}; #ifdef CONFIG_OFF_PADCONF #define OFF_PD (1 << 12) diff --git a/arch/arm/include/asm/arch-omap5/mux_omap5.h b/arch/arm/include/asm/arch-omap5/mux_omap5.h index b8c2185..af6874f 100644 --- a/arch/arm/include/asm/arch-omap5/mux_omap5.h +++ b/arch/arm/include/asm/arch-omap5/mux_omap5.h @@ -34,7 +34,7 @@ struct pad_conf_entry { u16 val; -} __attribute__ ((__packed__)); +}; #ifdef CONFIG_OFF_PADCONF #define OFF_PD (1 << 12)
Avoid using __attribute__ ((__packed__)) unless it's absolutely necessary. "packed" will remove alignment requirements for the respective objects and may cause alignment issues unless alignment is also enforced using a pragma. Here, these packed attributes were causing alignment faults in Thumb build. Signed-off-by: Aneesh V <aneesh@ti.com> --- arch/arm/include/asm/arch-omap4/mux_omap4.h | 2 +- arch/arm/include/asm/arch-omap5/mux_omap5.h | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-)