[PATCH] carl9170: fix struct alignment conflict

[PATCH] carl9170: fix struct alignment conflict

[Arnd Bergmann]

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>
---
 drivers/net/wireless/ath/carl9170/fwcmd.h |  2 +-
 drivers/net/wireless/ath/carl9170/wlan.h  | 20 ++++++++++----------
 2 files changed, 11 insertions(+), 11 deletions(-)

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)
 {
-- 
2.29.2

Re: [PATCH] carl9170: fix struct alignment conflict

[Christian Lamparter]

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

RE: [PATCH] carl9170: fix struct alignment conflict

[David Laight]

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)

Re: [PATCH] carl9170: fix struct alignment conflict

[Kalle Valo]

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

-- 
https://patchwork.kernel.org/project/linux-wireless/patch/20210204162926.3262598-1-arnd@kernel.org/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches