[PATCH 1/2] staging: rtl8192e: fix structure alignment

[PATCH 1/2] staging: rtl8192e: fix structure alignment

[Arnd Bergmann]

From: Arnd Bergmann <arnd@arndb.de>

A recent cleanup changed the rtl8192e from using the custom misaligned
rtllib_hdr_3addr structure to the generic ieee80211_hdr_3addr definition
that enforces 16-bit structure alignment in memory.

This causes a gcc warning about conflicting alignment requirements:

drivers/staging/rtl8192e/rtllib.h:645:1: error: alignment 1 of 'struct rtllib_authentication' is less than 2 [-Werror=packed-not-aligned]
  645 | } __packed;
      | ^
rtllib.h:650:1: error: alignment 1 of 'struct rtllib_disauth' is less than 2 [-Werror=packed-not-aligned]
rtllib.h:655:1: error: alignment 1 of 'struct rtllib_disassoc' is less than 2 [-Werror=packed-not-aligned]
rtllib.h:661:1: error: alignment 1 of 'struct rtllib_probe_request' is less than 2 [-Werror=packed-not-aligned]
rtllib.h:672:1: error: alignment 1 of 'struct rtllib_probe_response' is less than 2 [-Werror=packed-not-aligned]
rtllib.h:683:1: error: alignment 1 of 'struct rtllib_assoc_request_frame' is less than 2 [-Werror=packed-not-aligned]
rtllib.h:691:1: error: alignment 1 of 'struct rtllib_assoc_response_frame' is less than 2 [-Werror=packed-not-aligned]

Change all of the structure definitions that include this one to also
use 16-bit alignment. This assumes that the objects are actually aligned
in memory, but that is normally guaranteed by the slab allocator already.

All members of the structure definitions are already 16-bit aligned,
so the layouts do not change. As an added benefit, 16-bit accesses are
generally faster than 8-bit accesses, so architectures without unaligned
load/store instructions can produce better code now by avoiding byte-wise
accesses.

Fixes: 71ddc43ed7c71 ("staging: rtl8192e: Replace struct rtllib_hdr_3addr in structs of rtllib.h")
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
 drivers/staging/rtl8192e/rtllib.h | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/drivers/staging/rtl8192e/rtllib.h b/drivers/staging/rtl8192e/rtllib.h
index 5517b9df65bee..7d26910a0b162 100644
--- a/drivers/staging/rtl8192e/rtllib.h
+++ b/drivers/staging/rtl8192e/rtllib.h
@@ -642,23 +642,23 @@ struct rtllib_authentication {
 	__le16 status;
 	/*challenge*/
 	struct rtllib_info_element info_element[];
-} __packed;
+} __packed __aligned(2);
 
 struct rtllib_disauth {
 	struct ieee80211_hdr_3addr header;
 	__le16 reason;
-} __packed;
+} __packed __aligned(2);
 
 struct rtllib_disassoc {
 	struct ieee80211_hdr_3addr header;
 	__le16 reason;
-} __packed;
+} __packed __aligned(2);
 
 struct rtllib_probe_request {
 	struct ieee80211_hdr_3addr header;
 	/* SSID, supported rates */
 	struct rtllib_info_element info_element[];
-} __packed;
+} __packed __aligned(2);
 
 struct rtllib_probe_response {
 	struct ieee80211_hdr_3addr header;
@@ -669,7 +669,7 @@ struct rtllib_probe_response {
 	 * CF params, IBSS params, TIM (if beacon), RSN
 	 */
 	struct rtllib_info_element info_element[];
-} __packed;
+} __packed __aligned(2);
 
 /* Alias beacon for probe_response */
 #define rtllib_beacon rtllib_probe_response
@@ -680,7 +680,7 @@ struct rtllib_assoc_request_frame {
 	__le16 listen_interval;
 	/* SSID, supported rates, RSN */
 	struct rtllib_info_element info_element[];
-} __packed;
+} __packed __aligned(2);
 
 struct rtllib_assoc_response_frame {
 	struct ieee80211_hdr_3addr header;
@@ -688,7 +688,7 @@ struct rtllib_assoc_response_frame {
 	__le16 status;
 	__le16 aid;
 	struct rtllib_info_element info_element[]; /* supported rates */
-} __packed;
+} __packed __aligned(2);
 
 struct rtllib_txb {
 	u8 nr_frags;
-- 
2.39.2

[PATCH 2/2] staging: rtl8192e: remove bogus __packed annotations

[Arnd Bergmann]

From: Arnd Bergmann <arnd@arndb.de>

The rtllib_rxb structure contains a pointer, so this is not a hardware
structure but could benefit from loading the pointer in a single
instruction rather than assembling it from four or eight individual
bytes.

Both structures are allocated as part of larger structure that
already enforce at least a 4-byte alignment, so there is no
reason to ever have to deal with misaligned definitions.

Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
 drivers/staging/rtl8192e/rtllib.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/staging/rtl8192e/rtllib.h b/drivers/staging/rtl8192e/rtllib.h
index 7d26910a0b162..fe7b58ae33995 100644
--- a/drivers/staging/rtl8192e/rtllib.h
+++ b/drivers/staging/rtl8192e/rtllib.h
@@ -172,7 +172,7 @@ struct sw_chnl_cmd {
 	u32			Para1;
 	u32			Para2;
 	u32			msDelay;
-} __packed;
+};
 
 /*--------------------------Define -------------------------------------------*/
 #define MGN_1M		  0x02
@@ -707,7 +707,7 @@ struct rtllib_rxb {
 	struct sk_buff *subframes[MAX_SUBFRAME_COUNT];
 	u8 dst[ETH_ALEN];
 	u8 src[ETH_ALEN];
-} __packed;
+};
 
 union frameqos {
 	u16 shortdata;
-- 
2.39.2

Re: [PATCH 1/2] staging: rtl8192e: fix structure alignment

[Philipp Hortmann]

On 9/25/23 17:54, Arnd Bergmann wrote:
> From: Arnd Bergmann<arnd@arndb.de>
> 
> A recent cleanup changed the rtl8192e from using the custom misaligned
> rtllib_hdr_3addr structure to the generic ieee80211_hdr_3addr definition
> that enforces 16-bit structure alignment in memory.
> 
> This causes a gcc warning about conflicting alignment requirements:
> 
> drivers/staging/rtl8192e/rtllib.h:645:1: error: alignment 1 of 'struct rtllib_authentication' is less than 2 [-Werror=packed-not-aligned]
>    645 | } __packed;
>        | ^
> rtllib.h:650:1: error: alignment 1 of 'struct rtllib_disauth' is less than 2 [-Werror=packed-not-aligned]
> rtllib.h:655:1: error: alignment 1 of 'struct rtllib_disassoc' is less than 2 [-Werror=packed-not-aligned]
> rtllib.h:661:1: error: alignment 1 of 'struct rtllib_probe_request' is less than 2 [-Werror=packed-not-aligned]
> rtllib.h:672:1: error: alignment 1 of 'struct rtllib_probe_response' is less than 2 [-Werror=packed-not-aligned]
> rtllib.h:683:1: error: alignment 1 of 'struct rtllib_assoc_request_frame' is less than 2 [-Werror=packed-not-aligned]
> rtllib.h:691:1: error: alignment 1 of 'struct rtllib_assoc_response_frame' is less than 2 [-Werror=packed-not-aligned]
> 
> Change all of the structure definitions that include this one to also
> use 16-bit alignment. This assumes that the objects are actually aligned
> in memory, but that is normally guaranteed by the slab allocator already.
> 
> All members of the structure definitions are already 16-bit aligned,
> so the layouts do not change. As an added benefit, 16-bit accesses are
> generally faster than 8-bit accesses, so architectures without unaligned
> load/store instructions can produce better code now by avoiding byte-wise
> accesses.
> 
> Fixes: 71ddc43ed7c71 ("staging: rtl8192e: Replace struct rtllib_hdr_3addr in structs of rtllib.h")
> Signed-off-by: Arnd Bergmann<arnd@arndb.de>

Hi,

thanks for your support.

your patches cannot be applied on top of the 24 patches which are in the 
queue. But may be Greg will not accept all of the patches send in.

Will see what happens when Greg sorts them out.

I tried your patches on hardware without the 24 patches send in. All OK

Tested-by: Philipp Hortmann <philipp.g.hortmann@gmail.com>



I use the following command to compile. Why I am not seeing the issue above?
make "KCFLAGS=-pipe -Wpacked-not-aligned" -C . M=drivers/staging /rtl8192e

Thanks

Bye Philipp

Re: [PATCH 1/2] staging: rtl8192e: fix structure alignment

[Greg Kroah-Hartman]

On Mon, Sep 25, 2023 at 09:11:14PM +0200, Philipp Hortmann wrote:
> On 9/25/23 17:54, Arnd Bergmann wrote:
> > From: Arnd Bergmann<arnd@arndb.de>
> > 
> > A recent cleanup changed the rtl8192e from using the custom misaligned
> > rtllib_hdr_3addr structure to the generic ieee80211_hdr_3addr definition
> > that enforces 16-bit structure alignment in memory.
> > 
> > This causes a gcc warning about conflicting alignment requirements:
> > 
> > drivers/staging/rtl8192e/rtllib.h:645:1: error: alignment 1 of 'struct rtllib_authentication' is less than 2 [-Werror=packed-not-aligned]
> >    645 | } __packed;
> >        | ^
> > rtllib.h:650:1: error: alignment 1 of 'struct rtllib_disauth' is less than 2 [-Werror=packed-not-aligned]
> > rtllib.h:655:1: error: alignment 1 of 'struct rtllib_disassoc' is less than 2 [-Werror=packed-not-aligned]
> > rtllib.h:661:1: error: alignment 1 of 'struct rtllib_probe_request' is less than 2 [-Werror=packed-not-aligned]
> > rtllib.h:672:1: error: alignment 1 of 'struct rtllib_probe_response' is less than 2 [-Werror=packed-not-aligned]
> > rtllib.h:683:1: error: alignment 1 of 'struct rtllib_assoc_request_frame' is less than 2 [-Werror=packed-not-aligned]
> > rtllib.h:691:1: error: alignment 1 of 'struct rtllib_assoc_response_frame' is less than 2 [-Werror=packed-not-aligned]
> > 
> > Change all of the structure definitions that include this one to also
> > use 16-bit alignment. This assumes that the objects are actually aligned
> > in memory, but that is normally guaranteed by the slab allocator already.
> > 
> > All members of the structure definitions are already 16-bit aligned,
> > so the layouts do not change. As an added benefit, 16-bit accesses are
> > generally faster than 8-bit accesses, so architectures without unaligned
> > load/store instructions can produce better code now by avoiding byte-wise
> > accesses.
> > 
> > Fixes: 71ddc43ed7c71 ("staging: rtl8192e: Replace struct rtllib_hdr_3addr in structs of rtllib.h")
> > Signed-off-by: Arnd Bergmann<arnd@arndb.de>
> 
> Hi,
> 
> thanks for your support.
> 
> your patches cannot be applied on top of the 24 patches which are in the
> queue. But may be Greg will not accept all of the patches send in.
> 
> Will see what happens when Greg sorts them out.
> 
> I tried your patches on hardware without the 24 patches send in. All OK
> 
> Tested-by: Philipp Hortmann <philipp.g.hortmann@gmail.com>

The first one didn't apply as it was already sent by someone else, but
the second one applied fine, thanks.

greg k-h