| Message ID | 20210204162926.3262598-1-arnd@kernel.org (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | ca9ad549e4042089d55a68f0312647fca4cc6e87 |
| Delegated to: | Kalle Valo |
| Headers | show |
| Series | carl9170: fix struct alignment conflict | expand |
On 04/02/2021 17:29, Arnd Bergmann wrote: > From: Arnd Bergmann <arnd@arndb.de> > > Multiple structures in the carl9170 driver have alignment > impossible alignment constraints that gcc warns about when > building with 'make W=1': > > drivers/net/wireless/ath/carl9170/fwcmd.h:243:2: warning: alignment 1 of 'union <anonymous>' is less than 4 [-Wpacked-not-aligned] > drivers/net/wireless/ath/carl9170/wlan.h:373:1: warning: alignment 1 of 'struct ar9170_rx_frame_single' is less than 2 [-Wpacked-not-aligned] > > In the carl9170_cmd structure, multiple members that have an explicit > alignment requirement of four bytes are added into a union with explicit > byte alignment, but this in turn is part of a structure that also has > four-byte alignment. > > In the wlan.h header, multiple structures contain a ieee80211_hdr member > that is required to be two-byte aligned to avoid alignmnet faults when > processing network headers, but all members are forced to be byte-aligned > using the __packed tag at the end of the struct definition. > > In both cases, leaving out the packing does not change the internal > layout of the structure but changes the alignment constraint of the > structure itself. > > Change all affected structures to only apply packing where it does > not violate the alignment requirement of the contained structure. > > Signed-off-by: Arnd Bergmann <arnd@arndb.de> Acked-by: Christian Lamparter <chunkeey@gmail.com> I've also applied this patch and a previous one dealing with VLAs to the firmware source at <https://github.com/chunkeey/carl9170fw>. I did a quick rebuilt and the same binary firmware was produced, so I think what's left is to update those shared firmware/driver headers some day... Cheers, Christian
From: Arnd Bergmann > Sent: 04 February 2021 16:29 > > Multiple structures in the carl9170 driver have alignment > impossible alignment constraints that gcc warns about when > building with 'make W=1': > > drivers/net/wireless/ath/carl9170/fwcmd.h:243:2: warning: alignment 1 of 'union <anonymous>' is less > than 4 [-Wpacked-not-aligned] > drivers/net/wireless/ath/carl9170/wlan.h:373:1: warning: alignment 1 of 'struct > ar9170_rx_frame_single' is less than 2 [-Wpacked-not-aligned] > > In the carl9170_cmd structure, multiple members that have an explicit > alignment requirement of four bytes are added into a union with explicit > byte alignment, but this in turn is part of a structure that also has > four-byte alignment. > > In the wlan.h header, multiple structures contain a ieee80211_hdr member > that is required to be two-byte aligned to avoid alignmnet faults when > processing network headers, but all members are forced to be byte-aligned > using the __packed tag at the end of the struct definition. > > In both cases, leaving out the packing does not change the internal > layout of the structure but changes the alignment constraint of the > structure itself. > > Change all affected structures to only apply packing where it does > not violate the alignment requirement of the contained structure. I think I'd add compile-time assert that some of these structures are exactly the expected size. Then look at removing the outer packed/aligned attributes and just putting the attribute on the 16/32 bit member(s) that themselves might be misaligned. Much the way that the 32bit aligned 64bit values are handled in the x86 compat code in x86-64. David - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
Arnd Bergmann <arnd@kernel.org> wrote: > Multiple structures in the carl9170 driver have alignment > impossible alignment constraints that gcc warns about when > building with 'make W=1': > > drivers/net/wireless/ath/carl9170/fwcmd.h:243:2: warning: alignment 1 of 'union <anonymous>' is less than 4 [-Wpacked-not-aligned] > drivers/net/wireless/ath/carl9170/wlan.h:373:1: warning: alignment 1 of 'struct ar9170_rx_frame_single' is less than 2 [-Wpacked-not-aligned] > > In the carl9170_cmd structure, multiple members that have an explicit > alignment requirement of four bytes are added into a union with explicit > byte alignment, but this in turn is part of a structure that also has > four-byte alignment. > > In the wlan.h header, multiple structures contain a ieee80211_hdr member > that is required to be two-byte aligned to avoid alignmnet faults when > processing network headers, but all members are forced to be byte-aligned > using the __packed tag at the end of the struct definition. > > In both cases, leaving out the packing does not change the internal > layout of the structure but changes the alignment constraint of the > structure itself. > > Change all affected structures to only apply packing where it does > not violate the alignment requirement of the contained structure. > > Signed-off-by: Arnd Bergmann <arnd@arndb.de> > Acked-by: Christian Lamparter <chunkeey@gmail.com> > Signed-off-by: Kalle Valo <kvalo@codeaurora.org> Patch applied to ath-next branch of ath.git, thanks. ca9ad549e404 carl9170: fix struct alignment conflict
diff --git a/drivers/net/wireless/ath/carl9170/fwcmd.h b/drivers/net/wireless/ath/carl9170/fwcmd.h index 56999a3b9d3b..4a500095555c 100644 --- a/drivers/net/wireless/ath/carl9170/fwcmd.h +++ b/drivers/net/wireless/ath/carl9170/fwcmd.h @@ -240,7 +240,7 @@ struct carl9170_cmd { struct carl9170_bcn_ctrl_cmd bcn_ctrl; struct carl9170_rx_filter_cmd rx_filter; u8 data[CARL9170_MAX_CMD_PAYLOAD_LEN]; - } __packed; + } __packed __aligned(4); } __packed __aligned(4); #define CARL9170_TX_STATUS_QUEUE 3 diff --git a/drivers/net/wireless/ath/carl9170/wlan.h b/drivers/net/wireless/ath/carl9170/wlan.h index ea17995b32f4..bb73553fd7c2 100644 --- a/drivers/net/wireless/ath/carl9170/wlan.h +++ b/drivers/net/wireless/ath/carl9170/wlan.h @@ -367,27 +367,27 @@ struct ar9170_rx_macstatus { struct ar9170_rx_frame_single { struct ar9170_rx_head phy_head; - struct ieee80211_hdr i3e; + struct ieee80211_hdr i3e __packed __aligned(2); struct ar9170_rx_phystatus phy_tail; struct ar9170_rx_macstatus macstatus; -} __packed; +}; struct ar9170_rx_frame_head { struct ar9170_rx_head phy_head; - struct ieee80211_hdr i3e; + struct ieee80211_hdr i3e __packed __aligned(2); struct ar9170_rx_macstatus macstatus; -} __packed; +}; struct ar9170_rx_frame_middle { - struct ieee80211_hdr i3e; + struct ieee80211_hdr i3e __packed __aligned(2); struct ar9170_rx_macstatus macstatus; -} __packed; +}; struct ar9170_rx_frame_tail { - struct ieee80211_hdr i3e; + struct ieee80211_hdr i3e __packed __aligned(2); struct ar9170_rx_phystatus phy_tail; struct ar9170_rx_macstatus macstatus; -} __packed; +}; struct ar9170_rx_frame { union { @@ -395,8 +395,8 @@ struct ar9170_rx_frame { struct ar9170_rx_frame_head head; struct ar9170_rx_frame_middle middle; struct ar9170_rx_frame_tail tail; - } __packed; -} __packed; + }; +}; static inline u8 ar9170_get_decrypt_type(struct ar9170_rx_macstatus *t) {