| Message ID | 20221228133547.633797-2-martin.blumenstingl@googlemail.com |
|---|---|
| State | New |
| Headers | show |
| Series | rtw88: Four fixes found while working on SDIO support | expand |
From: Ping-Ke Shih > Sent: 29 December 2022 09:25 > > > -----Original Message----- > > From: Martin Blumenstingl <martin.blumenstingl@googlemail.com> > > Sent: Wednesday, December 28, 2022 9:36 PM > > To: linux-wireless@vger.kernel.org > > Cc: tony0620emma@gmail.com; kvalo@kernel.org; Ping-Ke Shih <pkshih@realtek.com>; > tehuang@realtek.com; > > s.hauer@pengutronix.de; netdev@vger.kernel.org; linux-kernel@vger.kernel.org; Martin Blumenstingl > > <martin.blumenstingl@googlemail.com> > > Subject: [PATCH 1/4] rtw88: Add packed attribute to the eFuse structs > > > > The eFuse definitions in the rtw88 are using structs to describe the > > eFuse contents. Add the packed attribute to all structs used for the > > eFuse description so the compiler doesn't add gaps or re-order > > attributes. > > > > Also change the type of the res2..res3 eFuse fields to u16 to avoid the > > following warning, now that their surrounding struct has the packed > > attribute: > > note: offset of packed bit-field 'res2' has changed in GCC 4.4 > > > > Fixes: e3037485c68e ("rtw88: new Realtek 802.11ac driver") > > Fixes: ab0a031ecf29 ("rtw88: 8723d: Add read_efuse to recognize efuse info from map") > > Fixes: 769a29ce2af4 ("rtw88: 8821c: add basic functions") > > Fixes: 87caeef032fc ("wifi: rtw88: Add rtw8723du chipset support") > > Fixes: aff5ffd718de ("wifi: rtw88: Add rtw8821cu chipset support") > > Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com> > > --- > > drivers/net/wireless/realtek/rtw88/main.h | 6 +++--- > > drivers/net/wireless/realtek/rtw88/rtw8723d.h | 6 +++--- > > drivers/net/wireless/realtek/rtw88/rtw8821c.h | 20 +++++++++---------- > > drivers/net/wireless/realtek/rtw88/rtw8822b.h | 20 +++++++++---------- > > drivers/net/wireless/realtek/rtw88/rtw8822c.h | 20 +++++++++---------- > > 5 files changed, 36 insertions(+), 36 deletions(-) > > > > [...] > > > @@ -43,13 +43,13 @@ struct rtw8821ce_efuse { > > u8 link_cap[4]; > > u8 link_control[2]; > > u8 serial_number[8]; > > - u8 res0:2; /* 0xf4 */ > > - u8 ltr_en:1; > > - u8 res1:2; > > - u8 obff:2; > > - u8 res2:3; > > - u8 obff_cap:2; > > - u8 res3:4; > > + u16 res0:2; /* 0xf4 */ > > + u16 ltr_en:1; > > + u16 res1:2; > > + u16 obff:2; > > + u16 res2:3; > > + u16 obff_cap:2; > > + u16 res3:4; > > These should be __le16. Though bit fields are suitable to efuse layout, > we don't access these fields for now. It would be well. IIRC the assignment of actual bits to bit-fields is (at best) architecturally defined - so isn't really suitable for anything where the bits have to match a portable memory buffer. The bit allocation isn't tied to the byte endianness. To get an explicit layout you have to do explicit masking. You also don't need __packed unless the 'natural' alignment of fields would need gaps or the actual structure itself might be misaligned in memory. While C compilers are allowed to add arbitrary padding the Linux kernel requires that they don't. I'm also pretty sure that compilers are not allowed to reorder fields. Specifying __packed can add considerable run-time (and code size) overhead on some architectures - it should only be used if actually needed. David - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
On Sat, 2022-12-31 at 16:57 +0000, David Laight wrote: > From: Ping-Ke Shih > > Sent: 29 December 2022 09:25 > > > > > -----Original Message----- > > > From: Martin Blumenstingl <martin.blumenstingl@googlemail.com> > > > Sent: Wednesday, December 28, 2022 9:36 PM > > > To: linux-wireless@vger.kernel.org > > > Cc: tony0620emma@gmail.com; kvalo@kernel.org; Ping-Ke Shih <pkshih@realtek.com>; > > tehuang@realtek.com; > > > s.hauer@pengutronix.de; netdev@vger.kernel.org; linux-kernel@vger.kernel.org; Martin > > > Blumenstingl > > > <martin.blumenstingl@googlemail.com> > > > Subject: [PATCH 1/4] rtw88: Add packed attribute to the eFuse structs > > > > > > The eFuse definitions in the rtw88 are using structs to describe the > > > eFuse contents. Add the packed attribute to all structs used for the > > > eFuse description so the compiler doesn't add gaps or re-order > > > attributes. > > > > > > Also change the type of the res2..res3 eFuse fields to u16 to avoid the > > > following warning, now that their surrounding struct has the packed > > > attribute: > > > note: offset of packed bit-field 'res2' has changed in GCC 4.4 > > > > > > Fixes: e3037485c68e ("rtw88: new Realtek 802.11ac driver") > > > Fixes: ab0a031ecf29 ("rtw88: 8723d: Add read_efuse to recognize efuse info from map") > > > Fixes: 769a29ce2af4 ("rtw88: 8821c: add basic functions") > > > Fixes: 87caeef032fc ("wifi: rtw88: Add rtw8723du chipset support") > > > Fixes: aff5ffd718de ("wifi: rtw88: Add rtw8821cu chipset support") > > > Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com> > > > --- > > > drivers/net/wireless/realtek/rtw88/main.h | 6 +++--- > > > drivers/net/wireless/realtek/rtw88/rtw8723d.h | 6 +++--- > > > drivers/net/wireless/realtek/rtw88/rtw8821c.h | 20 +++++++++---------- > > > drivers/net/wireless/realtek/rtw88/rtw8822b.h | 20 +++++++++---------- > > > drivers/net/wireless/realtek/rtw88/rtw8822c.h | 20 +++++++++---------- > > > 5 files changed, 36 insertions(+), 36 deletions(-) > > > > > > > [...] > > > > > @@ -43,13 +43,13 @@ struct rtw8821ce_efuse { > > > u8 link_cap[4]; > > > u8 link_control[2]; > > > u8 serial_number[8]; > > > - u8 res0:2; /* 0xf4 */ > > > - u8 ltr_en:1; > > > - u8 res1:2; > > > - u8 obff:2; > > > - u8 res2:3; > > > - u8 obff_cap:2; > > > - u8 res3:4; > > > + u16 res0:2; /* 0xf4 */ > > > + u16 ltr_en:1; > > > + u16 res1:2; > > > + u16 obff:2; > > > + u16 res2:3; > > > + u16 obff_cap:2; > > > + u16 res3:4; > > > > These should be __le16. Though bit fields are suitable to efuse layout, > > we don't access these fields for now. It would be well. Uh. I typo the sentence. Originally, I would like to type "...bit fields are NOT suitable...". In this driver, efuse is read into a u8 array, and cast to this struct pointer to access the field. > > IIRC the assignment of actual bits to bit-fields is (at best) > architecturally defined - so isn't really suitable for anything > where the bits have to match a portable memory buffer. > The bit allocation isn't tied to the byte endianness. Yes, this kind of struct has endian problem. Fortunately, we don't actually access values via bit fields. > > To get an explicit layout you have to do explicit masking. If we actually want to access these values, we will define masks and use {u8,_le16,le32}_get_bits() or bare '&' bit operation to access them. > > You also don't need __packed unless the 'natural' alignment > of fields would need gaps or the actual structure itself might > be misaligned in memory. > While C compilers are allowed to add arbitrary padding the Linux kernel > requires that they don't. > I'm also pretty sure that compilers are not allowed to reorder fields. > > Specifying __packed can add considerable run-time (and code size) > overhead on some architectures - it should only be used if actually > needed. > Understood. We only add __packed to the struct which is used to access predefined format, like efuse content defined by vendor. Ping-Ke
From: Ping-Ke Shih > Sent: 01 January 2023 11:42 > > On Sat, 2022-12-31 at 16:57 +0000, David Laight wrote: > > From: Ping-Ke Shih > > > Sent: 29 December 2022 09:25 > > > > > > > -----Original Message----- > > > > From: Martin Blumenstingl <martin.blumenstingl@googlemail.com> > > > > Sent: Wednesday, December 28, 2022 9:36 PM > > > > To: linux-wireless@vger.kernel.org > > > > Cc: tony0620emma@gmail.com; kvalo@kernel.org; Ping-Ke Shih <pkshih@realtek.com>; > > > tehuang@realtek.com; > > > > s.hauer@pengutronix.de; netdev@vger.kernel.org; linux-kernel@vger.kernel.org; Martin > > > > Blumenstingl > > > > <martin.blumenstingl@googlemail.com> > > > > Subject: [PATCH 1/4] rtw88: Add packed attribute to the eFuse structs > > > > > > > > The eFuse definitions in the rtw88 are using structs to describe the > > > > eFuse contents. Add the packed attribute to all structs used for the > > > > eFuse description so the compiler doesn't add gaps or re-order > > > > attributes. > > > > > > > > Also change the type of the res2..res3 eFuse fields to u16 to avoid the > > > > following warning, now that their surrounding struct has the packed > > > > attribute: > > > > note: offset of packed bit-field 'res2' has changed in GCC 4.4 > > > > > > > > Fixes: e3037485c68e ("rtw88: new Realtek 802.11ac driver") > > > > Fixes: ab0a031ecf29 ("rtw88: 8723d: Add read_efuse to recognize efuse info from map") > > > > Fixes: 769a29ce2af4 ("rtw88: 8821c: add basic functions") > > > > Fixes: 87caeef032fc ("wifi: rtw88: Add rtw8723du chipset support") > > > > Fixes: aff5ffd718de ("wifi: rtw88: Add rtw8821cu chipset support") > > > > Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com> > > > > --- > > > > drivers/net/wireless/realtek/rtw88/main.h | 6 +++--- > > > > drivers/net/wireless/realtek/rtw88/rtw8723d.h | 6 +++--- > > > > drivers/net/wireless/realtek/rtw88/rtw8821c.h | 20 +++++++++---------- > > > > drivers/net/wireless/realtek/rtw88/rtw8822b.h | 20 +++++++++---------- > > > > drivers/net/wireless/realtek/rtw88/rtw8822c.h | 20 +++++++++---------- > > > > 5 files changed, 36 insertions(+), 36 deletions(-) > > > > > > > > > > [...] > > > > > > > @@ -43,13 +43,13 @@ struct rtw8821ce_efuse { > > > > u8 link_cap[4]; > > > > u8 link_control[2]; > > > > u8 serial_number[8]; > > > > - u8 res0:2; /* 0xf4 */ > > > > - u8 ltr_en:1; > > > > - u8 res1:2; > > > > - u8 obff:2; > > > > - u8 res2:3; > > > > - u8 obff_cap:2; > > > > - u8 res3:4; > > > > + u16 res0:2; /* 0xf4 */ > > > > + u16 ltr_en:1; > > > > + u16 res1:2; > > > > + u16 obff:2; > > > > + u16 res2:3; > > > > + u16 obff_cap:2; > > > > + u16 res3:4; > > > > > > These should be __le16. Though bit fields are suitable to efuse layout, > > > we don't access these fields for now. It would be well. > > Uh. I typo the sentence. Originally, I would like to type > "...bit fields are NOT suitable...". > > In this driver, efuse is read into a u8 array, and cast to this struct > pointer to access the field. Then define it as such. The 16bit endianness and bit-order dependant bitfields serve no purpose. > > IIRC the assignment of actual bits to bit-fields is (at best) > > architecturally defined - so isn't really suitable for anything > > where the bits have to match a portable memory buffer. > > The bit allocation isn't tied to the byte endianness. > > Yes, this kind of struct has endian problem. Fortunately, we don't > actually access values via bit fields. > > > > > To get an explicit layout you have to do explicit masking. > > If we actually want to access these values, we will define masks > and use {u8,_le16,le32}_get_bits() or bare '&' bit operation to access > them. But you can't take the address of bitfield members. Define the data properly. > > > > You also don't need __packed unless the 'natural' alignment > > of fields would need gaps or the actual structure itself might > > be misaligned in memory. > > While C compilers are allowed to add arbitrary padding the Linux kernel > > requires that they don't. > > I'm also pretty sure that compilers are not allowed to reorder fields. > > > > Specifying __packed can add considerable run-time (and code size) > > overhead on some architectures - it should only be used if actually > > needed. > > > > Understood. We only add __packed to the struct which is used to > access predefined format, like efuse content defined by vendor. No - that doesn't mean you need to use __packed. It does mean that you shouldn't use bitfields. Look at all the hardware drivers, they use structs to map device registers and absolutely require the compile not add padding. David - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
On Sun, 2023-01-01 at 11:54 +0000, David Laight wrote: > From: Ping-Ke Shih > > Sent: 01 January 2023 11:42 > > > > On Sat, 2022-12-31 at 16:57 +0000, David Laight wrote: > > > From: Ping-Ke Shih > > > > Sent: 29 December 2022 09:25 > > > > > > > > > -----Original Message----- > > > > > From: Martin Blumenstingl <martin.blumenstingl@googlemail.com> > > > > > Sent: Wednesday, December 28, 2022 9:36 PM > > > > > To: linux-wireless@vger.kernel.org > > > > > Cc: tony0620emma@gmail.com; kvalo@kernel.org; Ping-Ke Shih <pkshih@realtek.com>; > > > > tehuang@realtek.com; > > > > > s.hauer@pengutronix.de; netdev@vger.kernel.org; linux-kernel@vger.kernel.org; Martin > > > > > Blumenstingl > > > > > <martin.blumenstingl@googlemail.com> > > > > > Subject: [PATCH 1/4] rtw88: Add packed attribute to the eFuse structs > > > > > > > > > > The eFuse definitions in the rtw88 are using structs to describe the > > > > > eFuse contents. Add the packed attribute to all structs used for the > > > > > eFuse description so the compiler doesn't add gaps or re-order > > > > > attributes. > > > > > > > > > > Also change the type of the res2..res3 eFuse fields to u16 to avoid the > > > > > following warning, now that their surrounding struct has the packed > > > > > attribute: > > > > > note: offset of packed bit-field 'res2' has changed in GCC 4.4 > > > > > > > > > > Fixes: e3037485c68e ("rtw88: new Realtek 802.11ac driver") > > > > > Fixes: ab0a031ecf29 ("rtw88: 8723d: Add read_efuse to recognize efuse info from map") > > > > > Fixes: 769a29ce2af4 ("rtw88: 8821c: add basic functions") > > > > > Fixes: 87caeef032fc ("wifi: rtw88: Add rtw8723du chipset support") > > > > > Fixes: aff5ffd718de ("wifi: rtw88: Add rtw8821cu chipset support") > > > > > Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com> > > > > > --- > > > > > drivers/net/wireless/realtek/rtw88/main.h | 6 +++--- > > > > > drivers/net/wireless/realtek/rtw88/rtw8723d.h | 6 +++--- > > > > > drivers/net/wireless/realtek/rtw88/rtw8821c.h | 20 +++++++++---------- > > > > > drivers/net/wireless/realtek/rtw88/rtw8822b.h | 20 +++++++++---------- > > > > > drivers/net/wireless/realtek/rtw88/rtw8822c.h | 20 +++++++++---------- > > > > > 5 files changed, 36 insertions(+), 36 deletions(-) > > > > > > > > > > > > > [...] > > > > > > > > > @@ -43,13 +43,13 @@ struct rtw8821ce_efuse { > > > > > u8 link_cap[4]; > > > > > u8 link_control[2]; > > > > > u8 serial_number[8]; > > > > > - u8 res0:2; /* 0xf4 */ > > > > > - u8 ltr_en:1; > > > > > - u8 res1:2; > > > > > - u8 obff:2; > > > > > - u8 res2:3; > > > > > - u8 obff_cap:2; > > > > > - u8 res3:4; > > > > > + u16 res0:2; /* 0xf4 */ > > > > > + u16 ltr_en:1; > > > > > + u16 res1:2; > > > > > + u16 obff:2; > > > > > + u16 res2:3; > > > > > + u16 obff_cap:2; > > > > > + u16 res3:4; > > > > > > > > These should be __le16. Though bit fields are suitable to efuse layout, > > > > we don't access these fields for now. It would be well. > > > > Uh. I typo the sentence. Originally, I would like to type > > "...bit fields are NOT suitable...". > > > > In this driver, efuse is read into a u8 array, and cast to this struct > > pointer to access the field. > > Then define it as such. > The 16bit endianness and bit-order dependant bitfields serve > no purpose. > > > > IIRC the assignment of actual bits to bit-fields is (at best) > > > architecturally defined - so isn't really suitable for anything > > > where the bits have to match a portable memory buffer. > > > The bit allocation isn't tied to the byte endianness. > > > > Yes, this kind of struct has endian problem. Fortunately, we don't > > actually access values via bit fields. > > > > > To get an explicit layout you have to do explicit masking. > > > > If we actually want to access these values, we will define masks > > and use {u8,_le16,le32}_get_bits() or bare '&' bit operation to access > > them. > > But you can't take the address of bitfield members. > Define the data properly. Yes, it should not use bit filed. Instead, use a __le16 for all fields, such as struct rtw8821ce_efuse { ... __le16 caps; ... } #define CAPS_RES0 GENMASK(1, 0) #define CAPS_LTR_EN BIT(2) #define CAPS_RES1 GENMASK(4, 3) #define CAPS_OBFF GENMASK(6, 5) ... Assume the pointer of efuse content is 'const u8 *efuse_raw;' const struct rtw8821ce_efuse *efuse = (const struct rtw8821ce_efuse *)efuse_raw; Then, get ltr_en ltr_en = le16_get_bits(efuse->caps, CAPS_LTR_EN); > > > > You also don't need __packed unless the 'natural' alignment > > > of fields would need gaps or the actual structure itself might > > > be misaligned in memory. > > > While C compilers are allowed to add arbitrary padding the Linux kernel > > > requires that they don't. > > > I'm also pretty sure that compilers are not allowed to reorder fields. > > > > > > Specifying __packed can add considerable run-time (and code size) > > > overhead on some architectures - it should only be used if actually > > > needed. > > > > > > > Understood. We only add __packed to the struct which is used to > > access predefined format, like efuse content defined by vendor. > > No - that doesn't mean you need to use __packed. > It does mean that you shouldn't use bitfields. > Look at all the hardware drivers, they use structs to map device > registers and absolutely require the compile not add padding. > I think the original struct has two problem -- endian and __packed. I mentioned endian part above. Taking below as example to explain why I need __packed, struct rtw8821ce_efuse { ... u8 data1; // offset 0x100 __le16 data2; // offset 0x101-0x102 ... } __packed; Without __packed, compiler could has pad between data1 and data2, and then get wrong result. In this patch, struct isn't to map registers. Instead it is used to access efuse content of a u8 array existing in memory. Ping-Ke
Hi Ping-Ke, Hi David, On Sun, Jan 1, 2023 at 2:09 PM Ping-Ke Shih <pkshih@realtek.com> wrote: [...] > Yes, it should not use bit filed. Instead, use a __le16 for all fields, such as I think this can be done in a separate patch. My v2 of this patch has reduced these changes to a minimum, see [0] [...] > struct rtw8821ce_efuse { > ... > u8 data1; // offset 0x100 > __le16 data2; // offset 0x101-0x102 > ... > } __packed; > > Without __packed, compiler could has pad between data1 and data2, > and then get wrong result. My understanding is that this is the reason why we need __packed. So my idea for the next steps is: - I will send a v3 of my series but change the wording in the commit description so it only mentions padding (but dropping the re-ordering part) - maybe Ping-Ke or his team can send a patch to fix the endian/bit field problem in the PCIe eFuse structs - (I'll keep working on SDIO support) Does this make sense to both of you? Best regards, Martin [0] https://lore.kernel.org/linux-wireless/20221229124845.1155429-2-martin.blumenstingl@googlemail.com/
From: Martin Blumenstingl > Sent: 04 January 2023 15:30 > > Hi Ping-Ke, Hi David, > > On Sun, Jan 1, 2023 at 2:09 PM Ping-Ke Shih <pkshih@realtek.com> wrote: > [...] > > Yes, it should not use bit filed. Instead, use a __le16 for all fields, such as > I think this can be done in a separate patch. > My v2 of this patch has reduced these changes to a minimum, see [0] > > [...] > > struct rtw8821ce_efuse { > > ... > > u8 data1; // offset 0x100 > > __le16 data2; // offset 0x101-0x102 > > ... > > } __packed; > > > > Without __packed, compiler could has pad between data1 and data2, > > and then get wrong result. > My understanding is that this is the reason why we need __packed. True, but does it really have to look like that? I can't find that version (I don't have a net_next tree). Possibly it should be 'u8 data2[2];' Most hardware definitions align everything. What you may want to do is add compile-time asserts for the sizes of the structures. Remember that if you have 16/32 bit fields in packed structures on some architectures the compile has to generate code that does byte loads and shifts. The 'misaligned' property is lost when you take the address - so you can easily generate a fault. Adding __packed to a struct is a sledgehammer you really shouldn't need. David - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
On Wed, Jan 4, 2023 at 4:53 PM David Laight <David.Laight@aculab.com> wrote: > > From: Martin Blumenstingl > > Sent: 04 January 2023 15:30 > > > > Hi Ping-Ke, Hi David, > > > > On Sun, Jan 1, 2023 at 2:09 PM Ping-Ke Shih <pkshih@realtek.com> wrote: > > [...] > > > Yes, it should not use bit filed. Instead, use a __le16 for all fields, such as > > I think this can be done in a separate patch. > > My v2 of this patch has reduced these changes to a minimum, see [0] > > > > [...] > > > struct rtw8821ce_efuse { > > > ... > > > u8 data1; // offset 0x100 > > > __le16 data2; // offset 0x101-0x102 > > > ... > > > } __packed; > > > > > > Without __packed, compiler could has pad between data1 and data2, > > > and then get wrong result. > > My understanding is that this is the reason why we need __packed. > > True, but does it really have to look like that? > I can't find that version (I don't have a net_next tree). My understanding is that there's one actual and one potential use-case. Let's start with the actual one in drivers/net/wireless/realtek/rtw88/rtw8821c.h: struct rtw8821c_efuse { __le16 rtl_id; u8 res0[0x0e]; ... The second one is a potential one, also in drivers/net/wireless/realtek/rtw88/rtw8821c.h if we replace the bitfields by an __le16 (which is my understanding how the data is modeled in the eFuse): struct rtw8821ce_efuse { ... u8 serial_number[8]; __le16 cap_data; /* 0xf4 */ ... (I'm not sure about the "cap_data" name, but I think you get the point) > Possibly it should be 'u8 data2[2];' So you're saying we should replace the __le16 with u8 some_name[2]; instead, then we don't need the __packed attribute. > What you may want to do is add compile-time asserts for the > sizes of the structures. Do I get you right that something like: BUILD_BUG_ON(sizeof(rtw8821c_efuse) != 256); is what you have in mind? Best regards, Martin
From: Martin Blumenstingl > Sent: 04 January 2023 16:08 > > On Wed, Jan 4, 2023 at 4:53 PM David Laight <David.Laight@aculab.com> wrote: > > > > From: Martin Blumenstingl > > > Sent: 04 January 2023 15:30 > > > > > > Hi Ping-Ke, Hi David, > > > > > > On Sun, Jan 1, 2023 at 2:09 PM Ping-Ke Shih <pkshih@realtek.com> wrote: > > > [...] > > > > Yes, it should not use bit filed. Instead, use a __le16 for all fields, such as > > > I think this can be done in a separate patch. > > > My v2 of this patch has reduced these changes to a minimum, see [0] > > > > > > [...] > > > > struct rtw8821ce_efuse { > > > > ... > > > > u8 data1; // offset 0x100 > > > > __le16 data2; // offset 0x101-0x102 > > > > ... > > > > } __packed; > > > > > > > > Without __packed, compiler could has pad between data1 and data2, > > > > and then get wrong result. > > > My understanding is that this is the reason why we need __packed. > > > > True, but does it really have to look like that? > > I can't find that version (I don't have a net_next tree). > My understanding is that there's one actual and one potential use-case. > Let's start with the actual one in > drivers/net/wireless/realtek/rtw88/rtw8821c.h: > struct rtw8821c_efuse { > __le16 rtl_id; > u8 res0[0x0e]; > ... > > The second one is a potential one, also in > drivers/net/wireless/realtek/rtw88/rtw8821c.h if we replace the > bitfields by an __le16 (which is my understanding how the data is > modeled in the eFuse): > struct rtw8821ce_efuse { > ... > u8 serial_number[8]; > __le16 cap_data; /* 0xf4 */ > ... > (I'm not sure about the "cap_data" name, but I think you get the point) Both those seem to be aligned - provided the structure is aligned. > > Possibly it should be 'u8 data2[2];' > So you're saying we should replace the __le16 with u8 some_name[2]; > instead, then we don't need the __packed attribute. But maybe you should look at defining the bitfields differently. Change to __le16 is probably making it hard for yourself. Perhaps you could #define a constant for each bitfield so you can write an access function like: #define bitval(field, n) (field[n >> 16] >> ((n >> 8) & 7)) & (n & 0xff)) If 'n' is always a compile time constant the code will be fine. Then add another define to create the 'n' based on values from the spec. (Which could be offsets onto 16bit items on odd boundaries.) Provided nothing crosses byte boundaries it should be fine and the source code will be reasonably readable. > > What you may want to do is add compile-time asserts for the > > sizes of the structures. > Do I get you right that something like: > BUILD_BUG_ON(sizeof(rtw8821c_efuse) != 256); > is what you have in mind? That looks like the one... David - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
On Wed, Jan 4, 2023 at 5:31 PM David Laight <David.Laight@aculab.com> wrote: [...] > > > What you may want to do is add compile-time asserts for the > > > sizes of the structures. > > Do I get you right that something like: > > BUILD_BUG_ON(sizeof(rtw8821c_efuse) != 256); > > is what you have in mind? > > That looks like the one... I tried this (see the attached patch - it's just meant to show what I did, it's not meant to be applied upstream). With the attached patch but no other patches this makes the rtw88 driver compile fine on 6.2-rc2. Adding __packed to struct rtw8723d_efuse changes the size of that struct for me (I'm compiling for AArch64 / ARM64). With the packed attribute it has 267 bytes, without 268 bytes. Do you have any ideas as to why that is? Best regards, Martin
> -----Original Message----- > From: Martin Blumenstingl <martin.blumenstingl@googlemail.com> > Sent: Thursday, January 5, 2023 1:49 AM > To: David Laight <David.Laight@aculab.com> > Cc: Ping-Ke Shih <pkshih@realtek.com>; linux-wireless@vger.kernel.org; kvalo@kernel.org; > tehuang@realtek.com; s.hauer@pengutronix.de; tony0620emma@gmail.com; netdev@vger.kernel.org; > linux-kernel@vger.kernel.org > Subject: Re: [PATCH 1/4] rtw88: Add packed attribute to the eFuse structs > > On Wed, Jan 4, 2023 at 5:31 PM David Laight <David.Laight@aculab.com> wrote: > [...] > > > > What you may want to do is add compile-time asserts for the > > > > sizes of the structures. > > > Do I get you right that something like: > > > BUILD_BUG_ON(sizeof(rtw8821c_efuse) != 256); > > > is what you have in mind? > > > > That looks like the one... > I tried this (see the attached patch - it's just meant to show what I > did, it's not meant to be applied upstream). > With the attached patch but no other patches this makes the rtw88 > driver compile fine on 6.2-rc2. I prefer to use static_assert() below the struct if we really need it. In fact, we only list fields of efuse map we need in the struct, not full map. > > Adding __packed to struct rtw8723d_efuse changes the size of that > struct for me (I'm compiling for AArch64 / ARM64). > With the packed attribute it has 267 bytes, without 268 bytes. Try static_assert(offsetof(struct rtw8723d_efuse, rf_antenna_option) == 0xc9); and other fields to bisect which field gets wrong. Ping-Ke
From: Martin Blumenstingl > Sent: 04 January 2023 17:49 > > On Wed, Jan 4, 2023 at 5:31 PM David Laight <David.Laight@aculab.com> wrote: > [...] > > > > What you may want to do is add compile-time asserts for the > > > > sizes of the structures. > > > Do I get you right that something like: > > > BUILD_BUG_ON(sizeof(rtw8821c_efuse) != 256); > > > is what you have in mind? > > > > That looks like the one... > I tried this (see the attached patch - it's just meant to show what I > did, it's not meant to be applied upstream). > With the attached patch but no other patches this makes the rtw88 > driver compile fine on 6.2-rc2. > > Adding __packed to struct rtw8723d_efuse changes the size of that > struct for me (I'm compiling for AArch64 / ARM64). > With the packed attribute it has 267 bytes, without 268 bytes. > > Do you have any ideas as to why that is? Tail padding - you won't get an odd length for a structure that contains a 16bit item. OTOH I doubt you care about the size of that structure, just the offset of the union and the sizes of the union members. David - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
David Laight <David.Laight@ACULAB.COM> writes: > From: Martin Blumenstingl >> Sent: 04 January 2023 15:30 >> >> Hi Ping-Ke, Hi David, >> >> On Sun, Jan 1, 2023 at 2:09 PM Ping-Ke Shih <pkshih@realtek.com> wrote: >> [...] >> > Yes, it should not use bit filed. Instead, use a __le16 for all fields, such as >> I think this can be done in a separate patch. >> My v2 of this patch has reduced these changes to a minimum, see [0] >> >> [...] >> > struct rtw8821ce_efuse { >> > ... >> > u8 data1; // offset 0x100 >> > __le16 data2; // offset 0x101-0x102 >> > ... >> > } __packed; >> > >> > Without __packed, compiler could has pad between data1 and data2, >> > and then get wrong result. >> My understanding is that this is the reason why we need __packed. > > True, but does it really have to look like that? > I can't find that version (I don't have a net_next tree). > Possibly it should be 'u8 data2[2];' > > Most hardware definitions align everything. > > What you may want to do is add compile-time asserts for the > sizes of the structures. > > Remember that if you have 16/32 bit fields in packed structures > on some architectures the compile has to generate code that does > byte loads and shifts. > > The 'misaligned' property is lost when you take the address - so > you can easily generate a fault. > > Adding __packed to a struct is a sledgehammer you really shouldn't need. Avoiding use of __packed is news to me, but is this really a safe rule? Most of the wireless engineers are no compiler experts (myself included) so I'm worried. For example, in ath10k and ath11k I try to use __packed for all structs which are accessing hardware or firmware just to make sure that the compiler is not changing anything.
From: Kalle Valo > Sent: 10 January 2023 12:03 ... > > Most hardware definitions align everything. > > > > What you may want to do is add compile-time asserts for the > > sizes of the structures. > > > > Remember that if you have 16/32 bit fields in packed structures > > on some architectures the compile has to generate code that does > > byte loads and shifts. > > > > The 'misaligned' property is lost when you take the address - so > > you can easily generate a fault. > > > > Adding __packed to a struct is a sledgehammer you really shouldn't need. > > Avoiding use of __packed is news to me, but is this really a safe rule? > Most of the wireless engineers are no compiler experts (myself included) > so I'm worried. For example, in ath10k and ath11k I try to use __packed > for all structs which are accessing hardware or firmware just to make > sure that the compiler is not changing anything. What may wish to do is get the compiler to generate an error if it would add any padding - but that isn't what __packed is for or what it does. The compiler will only ever add padding to ensure that fields are correctly aligned (usually a multiple of their size). There can also be padding at the end of a structure so that arrays are aligned. There are some unusual ABI that align all structures on 4 byte boundaries - but i don't think Linux has any of them. In any case this rarely matters. All structures that hardware/firmware access are very likely to have everything on its natural alignment unless you have a very old structure hat might have a 16bit aligned 32bit value that was assumed to be two words. Now if you have: struct { char a[4]; int b; } __packed foo; whenever you access foo.b the compiler might have to generate 4 separate byte memory accesses and a load of shift/and/or instructions in order to avoid a misaligned address trap. So you don't want to use __packed unless the field is actually expected to be misaligned. For most hardware/firmware structures this isn't true. David - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
diff --git a/drivers/net/wireless/realtek/rtw88/main.h b/drivers/net/wireless/realtek/rtw88/main.h index 165f299e8e1f..8441c26680ad 100644 --- a/drivers/net/wireless/realtek/rtw88/main.h +++ b/drivers/net/wireless/realtek/rtw88/main.h @@ -438,7 +438,7 @@ struct rtw_2g_txpwr_idx { struct rtw_2g_ns_pwr_idx_diff ht_2s_diff; struct rtw_2g_ns_pwr_idx_diff ht_3s_diff; struct rtw_2g_ns_pwr_idx_diff ht_4s_diff; -}; +} __packed; struct rtw_5g_ht_1s_pwr_idx_diff { #ifdef __LITTLE_ENDIAN @@ -495,12 +495,12 @@ struct rtw_5g_txpwr_idx { struct rtw_5g_vht_ns_pwr_idx_diff vht_2s_diff; struct rtw_5g_vht_ns_pwr_idx_diff vht_3s_diff; struct rtw_5g_vht_ns_pwr_idx_diff vht_4s_diff; -}; +} __packed; struct rtw_txpwr_idx { struct rtw_2g_txpwr_idx pwr_idx_2g; struct rtw_5g_txpwr_idx pwr_idx_5g; -}; +} __packed; struct rtw_timer_list { struct timer_list timer; diff --git a/drivers/net/wireless/realtek/rtw88/rtw8723d.h b/drivers/net/wireless/realtek/rtw88/rtw8723d.h index a356318a5c15..8160c4782457 100644 --- a/drivers/net/wireless/realtek/rtw88/rtw8723d.h +++ b/drivers/net/wireless/realtek/rtw88/rtw8723d.h @@ -39,7 +39,7 @@ struct rtw8723de_efuse { u8 device_id[2]; u8 sub_vender_id[2]; u8 sub_device_id[2]; -}; +} __packed; struct rtw8723du_efuse { u8 res4[48]; /* 0xd0 */ @@ -47,7 +47,7 @@ struct rtw8723du_efuse { u8 product_id[2]; /* 0x102 */ u8 usb_option; /* 0x104 */ u8 mac_addr[ETH_ALEN]; /* 0x107 */ -}; +} __packed; struct rtw8723d_efuse { __le16 rtl_id; @@ -81,7 +81,7 @@ struct rtw8723d_efuse { struct rtw8723de_efuse e; struct rtw8723du_efuse u; }; -}; +} __packed; extern const struct rtw_chip_info rtw8723d_hw_spec; diff --git a/drivers/net/wireless/realtek/rtw88/rtw8821c.h b/drivers/net/wireless/realtek/rtw88/rtw8821c.h index 1c81260f3a54..6ba0d4ee92bd 100644 --- a/drivers/net/wireless/realtek/rtw88/rtw8821c.h +++ b/drivers/net/wireless/realtek/rtw88/rtw8821c.h @@ -27,7 +27,7 @@ struct rtw8821cu_efuse { u8 res11[0xcf]; u8 package_type; /* 0x1fb */ u8 res12[0x4]; -}; +} __packed; struct rtw8821ce_efuse { u8 mac_addr[ETH_ALEN]; /* 0xd0 */ @@ -43,13 +43,13 @@ struct rtw8821ce_efuse { u8 link_cap[4]; u8 link_control[2]; u8 serial_number[8]; - u8 res0:2; /* 0xf4 */ - u8 ltr_en:1; - u8 res1:2; - u8 obff:2; - u8 res2:3; - u8 obff_cap:2; - u8 res3:4; + u16 res0:2; /* 0xf4 */ + u16 ltr_en:1; + u16 res1:2; + u16 obff:2; + u16 res2:3; + u16 obff_cap:2; + u16 res3:4; u8 res4[3]; u8 class_code[3]; u8 pci_pm_L1_2_supp:1; @@ -63,7 +63,7 @@ struct rtw8821ce_efuse { u8 res6:1; u8 port_t_power_on_value:5; u8 res7; -}; +} __packed; struct rtw8821c_efuse { __le16 rtl_id; @@ -95,7 +95,7 @@ struct rtw8821c_efuse { struct rtw8821ce_efuse e; struct rtw8821cu_efuse u; }; -}; +} __packed; static inline void _rtw_write32s_mask(struct rtw_dev *rtwdev, u32 addr, u32 mask, u32 data) diff --git a/drivers/net/wireless/realtek/rtw88/rtw8822b.h b/drivers/net/wireless/realtek/rtw88/rtw8822b.h index 01d3644e0c94..12a123436741 100644 --- a/drivers/net/wireless/realtek/rtw88/rtw8822b.h +++ b/drivers/net/wireless/realtek/rtw88/rtw8822b.h @@ -27,7 +27,7 @@ struct rtw8822bu_efuse { u8 res11[0xcf]; u8 package_type; /* 0x1fb */ u8 res12[0x4]; -}; +} __packed; struct rtw8822be_efuse { u8 mac_addr[ETH_ALEN]; /* 0xd0 */ @@ -43,13 +43,13 @@ struct rtw8822be_efuse { u8 link_cap[4]; u8 link_control[2]; u8 serial_number[8]; - u8 res0:2; /* 0xf4 */ - u8 ltr_en:1; - u8 res1:2; - u8 obff:2; - u8 res2:3; - u8 obff_cap:2; - u8 res3:4; + u16 res0:2; /* 0xf4 */ + u16 ltr_en:1; + u16 res1:2; + u16 obff:2; + u16 res2:3; + u16 obff_cap:2; + u16 res3:4; u8 res4[3]; u8 class_code[3]; u8 pci_pm_L1_2_supp:1; @@ -63,7 +63,7 @@ struct rtw8822be_efuse { u8 res6:1; u8 port_t_power_on_value:5; u8 res7; -}; +} __packed; struct rtw8822b_efuse { __le16 rtl_id; @@ -95,7 +95,7 @@ struct rtw8822b_efuse { struct rtw8822bu_efuse u; struct rtw8822be_efuse e; }; -}; +} __packed; static inline void _rtw_write32s_mask(struct rtw_dev *rtwdev, u32 addr, u32 mask, u32 data) diff --git a/drivers/net/wireless/realtek/rtw88/rtw8822c.h b/drivers/net/wireless/realtek/rtw88/rtw8822c.h index 479d5d769c52..4ca7874fdc35 100644 --- a/drivers/net/wireless/realtek/rtw88/rtw8822c.h +++ b/drivers/net/wireless/realtek/rtw88/rtw8822c.h @@ -14,7 +14,7 @@ struct rtw8822cu_efuse { u8 res1[3]; u8 mac_addr[ETH_ALEN]; /* 0x157 */ u8 res2[0x3d]; -}; +} __packed; struct rtw8822ce_efuse { u8 mac_addr[ETH_ALEN]; /* 0x120 */ @@ -30,13 +30,13 @@ struct rtw8822ce_efuse { u8 link_cap[4]; u8 link_control[2]; u8 serial_number[8]; - u8 res0:2; /* 0x144 */ - u8 ltr_en:1; - u8 res1:2; - u8 obff:2; - u8 res2:3; - u8 obff_cap:2; - u8 res3:4; + u16 res0:2; /* 0x144 */ + u16 ltr_en:1; + u16 res1:2; + u16 obff:2; + u16 res2:3; + u16 obff_cap:2; + u16 res3:4; u8 class_code[3]; u8 res4; u8 pci_pm_L1_2_supp:1; @@ -50,7 +50,7 @@ struct rtw8822ce_efuse { u8 res6:1; u8 port_t_power_on_value:5; u8 res7; -}; +} __packed; struct rtw8822c_efuse { __le16 rtl_id; @@ -94,7 +94,7 @@ struct rtw8822c_efuse { struct rtw8822cu_efuse u; struct rtw8822ce_efuse e; }; -}; +} __packed; enum rtw8822c_dpk_agc_phase { RTW_DPK_GAIN_CHECK,
The eFuse definitions in the rtw88 are using structs to describe the eFuse contents. Add the packed attribute to all structs used for the eFuse description so the compiler doesn't add gaps or re-order attributes. Also change the type of the res2..res3 eFuse fields to u16 to avoid the following warning, now that their surrounding struct has the packed attribute: note: offset of packed bit-field 'res2' has changed in GCC 4.4 Fixes: e3037485c68e ("rtw88: new Realtek 802.11ac driver") Fixes: ab0a031ecf29 ("rtw88: 8723d: Add read_efuse to recognize efuse info from map") Fixes: 769a29ce2af4 ("rtw88: 8821c: add basic functions") Fixes: 87caeef032fc ("wifi: rtw88: Add rtw8723du chipset support") Fixes: aff5ffd718de ("wifi: rtw88: Add rtw8821cu chipset support") Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com> --- drivers/net/wireless/realtek/rtw88/main.h | 6 +++--- drivers/net/wireless/realtek/rtw88/rtw8723d.h | 6 +++--- drivers/net/wireless/realtek/rtw88/rtw8821c.h | 20 +++++++++---------- drivers/net/wireless/realtek/rtw88/rtw8822b.h | 20 +++++++++---------- drivers/net/wireless/realtek/rtw88/rtw8822c.h | 20 +++++++++---------- 5 files changed, 36 insertions(+), 36 deletions(-)