LinuxLists.cc - [PATCH 0/4] rtw88: Four fixes found while working on SDIO support

[permalink] [raw]

Subject: [PATCH 2/4] rtw88: Configure the registers from rtw_bf_assoc() outside the RCU lock

USB and (upcoming) SDIO support may sleep in the read/write handlers.
Shrink the RCU critical section so it only cover the call to
ieee80211_find_sta() and finding the ic_vht_cap/vht_cap based on the
found station. This moves the chip's BFEE configuration outside the
rcu_read_lock section and thus prevent "scheduling while atomic" or
"Voluntary context switch within RCU read-side critical section!"
warnings when accessing the registers using an SDIO card (which is
where this issue has been spotted in the real world - but it also
affects USB cards).

Signed-off-by: Martin Blumenstingl <[email protected]>
---
drivers/net/wireless/realtek/rtw88/bf.c | 13 +++++++------
1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/drivers/net/wireless/realtek/rtw88/bf.c b/drivers/net/wireless/realtek/rtw88/bf.c
index 038a30b170ef..c827c4a2814b 100644
--- a/drivers/net/wireless/realtek/rtw88/bf.c
+++ b/drivers/net/wireless/realtek/rtw88/bf.c
@@ -49,19 +49,23 @@ void rtw_bf_assoc(struct rtw_dev *rtwdev, struct ieee80211_vif *vif,

sta = ieee80211_find_sta(vif, bssid);
if (!sta) {
+ rcu_read_unlock();
+
rtw_warn(rtwdev, "failed to find station entry for bss %pM\n",
bssid);
- goto out_unlock;
+ return;
}

ic_vht_cap = &hw->wiphy->bands[NL80211_BAND_5GHZ]->vht_cap;
vht_cap = &sta->deflink.vht_cap;

+ rcu_read_unlock();
+
if ((ic_vht_cap->cap & IEEE80211_VHT_CAP_MU_BEAMFORMEE_CAPABLE) &&
(vht_cap->cap & IEEE80211_VHT_CAP_MU_BEAMFORMER_CAPABLE)) {
if (bfinfo->bfer_mu_cnt >= chip->bfer_mu_max_num) {
rtw_dbg(rtwdev, RTW_DBG_BF, "mu bfer number over limit\n");
- goto out_unlock;
+ return;
}

ether_addr_copy(bfee->mac_addr, bssid);
@@ -75,7 +79,7 @@ void rtw_bf_assoc(struct rtw_dev *rtwdev, struct ieee80211_vif *vif,
(vht_cap->cap & IEEE80211_VHT_CAP_SU_BEAMFORMER_CAPABLE)) {
if (bfinfo->bfer_su_cnt >= chip->bfer_su_max_num) {
rtw_dbg(rtwdev, RTW_DBG_BF, "su bfer number over limit\n");
- goto out_unlock;
+ return;
}

sound_dim = vht_cap->cap &
@@ -98,9 +102,6 @@ void rtw_bf_assoc(struct rtw_dev *rtwdev, struct ieee80211_vif *vif,

rtw_chip_config_bfee(rtwdev, rtwvif, bfee, true);
}
-
-out_unlock:
- rcu_read_unlock();
}

void rtw_bf_init_bfer_entry_mu(struct rtw_dev *rtwdev,
--
2.39.0

2022-12-28 13:37:35

[permalink] [raw]

Subject: [PATCH 3/4] rtw88: Use rtw_iterate_vifs() for rtw_vif_watch_dog_iter()

USB and (upcoming) SDIO support may sleep in the read/write handlers.
Make rtw_watch_dog_work() use rtw_iterate_vifs() to prevent "scheduling
while atomic" or "Voluntary context switch within RCU read-side
critical section!" warnings when accessing the registers using an SDIO
card (which is where this issue has been spotted in the real world but
it also affects USB cards).

Fixes: 78d5bf925f30 ("wifi: rtw88: iterate over vif/sta list non-atomically")
Suggested-by: Ping-Ke Shih <[email protected]>
Signed-off-by: Martin Blumenstingl <[email protected]>
---
drivers/net/wireless/realtek/rtw88/main.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/net/wireless/realtek/rtw88/main.c b/drivers/net/wireless/realtek/rtw88/main.c
index 888427cf3bdf..b2e78737bd5d 100644
--- a/drivers/net/wireless/realtek/rtw88/main.c
+++ b/drivers/net/wireless/realtek/rtw88/main.c
@@ -241,8 +241,10 @@ static void rtw_watch_dog_work(struct work_struct *work)
rtw_phy_dynamic_mechanism(rtwdev);

data.rtwdev = rtwdev;
- /* use atomic version to avoid taking local->iflist_mtx mutex */
- rtw_iterate_vifs_atomic(rtwdev, rtw_vif_watch_dog_iter, &data);
+ /* rtw_iterate_vifs internally uses an atomic iterator which is needed
+ * to avoid taking local->iflist_mtx mutex
+ */
+ rtw_iterate_vifs(rtwdev, rtw_vif_watch_dog_iter, &data);

/* fw supports only one station associated to enter lps, if there are
* more than two stations associated to the AP, then we can not enter
--
2.39.0

2022-12-28 13:37:46

[permalink] [raw]

Subject: [PATCH 4/4] rtw88: Use non-atomic rtw_iterate_stas() in rtw_ra_mask_info_update()

USB and (upcoming) SDIO support may sleep in the read/write handlers.
Use non-atomic rtw_iterate_stas() in rtw_ra_mask_info_update() because
the iterator function rtw_ra_mask_info_update_iter() needs to read and
write registers from within rtw_update_sta_info(). Using the non-atomic
iterator ensures that we can sleep during USB and SDIO register reads
and writes. This fixes "scheduling while atomic" or "Voluntary context
switch within RCU read-side critical section!" warnings as seen by SDIO
card users (but it also affects USB cards).

Fixes: 78d5bf925f30 ("wifi: rtw88: iterate over vif/sta list non-atomically")
Suggested-by: Ping-Ke Shih <[email protected]>
Signed-off-by: Martin Blumenstingl <[email protected]>
---
drivers/net/wireless/realtek/rtw88/mac80211.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/net/wireless/realtek/rtw88/mac80211.c b/drivers/net/wireless/realtek/rtw88/mac80211.c
index 776a9a9884b5..3b92ac611d3f 100644
--- a/drivers/net/wireless/realtek/rtw88/mac80211.c
+++ b/drivers/net/wireless/realtek/rtw88/mac80211.c
@@ -737,7 +737,7 @@ static void rtw_ra_mask_info_update(struct rtw_dev *rtwdev,
br_data.rtwdev = rtwdev;
br_data.vif = vif;
br_data.mask = mask;
- rtw_iterate_stas_atomic(rtwdev, rtw_ra_mask_info_update_iter, &br_data);
+ rtw_iterate_stas(rtwdev, rtw_ra_mask_info_update_iter, &br_data);
}

static int rtw_ops_set_bitrate_mask(struct ieee80211_hw *hw,
@@ -746,7 +746,9 @@ static int rtw_ops_set_bitrate_mask(struct ieee80211_hw *hw,
{
struct rtw_dev *rtwdev = hw->priv;

+ mutex_lock(&rtwdev->mutex);
rtw_ra_mask_info_update(rtwdev, vif, mask);
+ mutex_unlock(&rtwdev->mutex);

return 0;
}
--
2.39.0

2022-12-28 13:38:03

[permalink] [raw]

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 <[email protected]>
---
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(-)

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,
--
2.39.0

2022-12-29 09:25:47

[permalink] [raw]

Subject: RE: [PATCH 1/4] rtw88: Add packed attribute to the eFuse structs

> -----Original Message-----
> From: Martin Blumenstingl <[email protected]>
> Sent: Wednesday, December 28, 2022 9:36 PM
> To: [email protected]
> Cc: [email protected]; [email protected]; Ping-Ke Shih <[email protected]>; [email protected];
> [email protected]; [email protected]; [email protected]; Martin Blumenstingl
> <[email protected]>
> 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 <[email protected]>
> ---
> 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.

[...]

2022-12-29 09:27:16

[permalink] [raw]

Subject: RE: [PATCH 0/4] rtw88: Four fixes found while working on SDIO support

> -----Original Message-----
> From: Martin Blumenstingl <[email protected]>
> Sent: Wednesday, December 28, 2022 9:36 PM
> To: [email protected]
> Cc: [email protected]; [email protected]; Ping-Ke Shih <[email protected]>; [email protected];
> [email protected]; [email protected]; [email protected]; Martin Blumenstingl
> <[email protected]>
> Subject: [PATCH 0/4] rtw88: Four fixes found while working on SDIO support
>
> This series consists of three patches which are fixing existing
> behavior (meaning: it either affects PCIe or USB or both) in the rtw88
> driver.
>
> The first change adds the packed attribute to the eFuse structs. This
> was spotted by Ping-Ke while reviewing the SDIO support patches from
> [0].
>
> The remaining three changes relate to locking (barrier hold) problems.
> We previously had discussed patches for this for SDIO support, but the
> problem never ocurred while testing USB cards. It turns out that these
> are still needed and I think that they also fix the same problems for
> USB users (it's not clear how often it happens there though).
>
> The issue fixed by the second and third patches have been spotted by a
> user who tested rtw88 SDIO support. Everything is working fine for him
> but there are warnings [1] and [2] in the kernel log stating "Voluntary
> context switch within RCU read-side critical section!".
>
> The solution in the third and fourth patch was actually suggested by
> Ping-Ke in [3]. Thanks again!
>
>
> [0] https://lore.kernel.org/linux-wireless/[email protected]/
> [1] https://github.com/LibreELEC/LibreELEC.tv/pull/7301#issuecomment-1366421445
> [2] https://github.com/LibreELEC/LibreELEC.tv/pull/7301#issuecomment-1366610249
> [3] https://lore.kernel.org/lkml/[email protected]/
>
>
> Martin Blumenstingl (4):
> rtw88: Add packed attribute to the eFuse structs

I think this patch depends on another patchset or oppositely.
Please point that out for reviewers.

> rtw88: Configure the registers from rtw_bf_assoc() outside the RCU
> lock
> rtw88: Use rtw_iterate_vifs() for rtw_vif_watch_dog_iter()
> rtw88: Use non-atomic rtw_iterate_stas() in rtw_ra_mask_info_update()
>
> drivers/net/wireless/realtek/rtw88/bf.c | 13 ++++++------
> drivers/net/wireless/realtek/rtw88/mac80211.c | 4 +++-
> drivers/net/wireless/realtek/rtw88/main.c | 6 ++++--
> 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 +++++++++----------
> 8 files changed, 50 insertions(+), 45 deletions(-)
>
> --
> 2.39.0

2022-12-29 09:50:48

[permalink] [raw]

Subject: RE: [PATCH 2/4] rtw88: Configure the registers from rtw_bf_assoc() outside the RCU lock

> -----Original Message-----
> From: Martin Blumenstingl <[email protected]>
> Sent: Wednesday, December 28, 2022 9:36 PM
> To: [email protected]
> Cc: [email protected]; [email protected]; Ping-Ke Shih <[email protected]>; [email protected];
> [email protected]; [email protected]; [email protected]; Martin Blumenstingl
> <[email protected]>
> Subject: [PATCH 2/4] rtw88: Configure the registers from rtw_bf_assoc() outside the RCU lock
>
> USB and (upcoming) SDIO support may sleep in the read/write handlers.
> Shrink the RCU critical section so it only cover the call to
> ieee80211_find_sta() and finding the ic_vht_cap/vht_cap based on the
> found station. This moves the chip's BFEE configuration outside the
> rcu_read_lock section and thus prevent "scheduling while atomic" or
> "Voluntary context switch within RCU read-side critical section!"
> warnings when accessing the registers using an SDIO card (which is
> where this issue has been spotted in the real world - but it also
> affects USB cards).
>
> Signed-off-by: Martin Blumenstingl <[email protected]>

Reviewed-by: Ping-Ke Shih <[email protected]>

> ---
> drivers/net/wireless/realtek/rtw88/bf.c | 13 +++++++------
> 1 file changed, 7 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/net/wireless/realtek/rtw88/bf.c b/drivers/net/wireless/realtek/rtw88/bf.c
> index 038a30b170ef..c827c4a2814b 100644
> --- a/drivers/net/wireless/realtek/rtw88/bf.c
> +++ b/drivers/net/wireless/realtek/rtw88/bf.c
> @@ -49,19 +49,23 @@ void rtw_bf_assoc(struct rtw_dev *rtwdev, struct ieee80211_vif *vif,
>
> sta = ieee80211_find_sta(vif, bssid);
> if (!sta) {
> + rcu_read_unlock();
> +
> rtw_warn(rtwdev, "failed to find station entry for bss %pM\n",
> bssid);
> - goto out_unlock;
> + return;
> }
>
> ic_vht_cap = &hw->wiphy->bands[NL80211_BAND_5GHZ]->vht_cap;
> vht_cap = &sta->deflink.vht_cap;
>
> + rcu_read_unlock();
> +
> if ((ic_vht_cap->cap & IEEE80211_VHT_CAP_MU_BEAMFORMEE_CAPABLE) &&
> (vht_cap->cap & IEEE80211_VHT_CAP_MU_BEAMFORMER_CAPABLE)) {
> if (bfinfo->bfer_mu_cnt >= chip->bfer_mu_max_num) {
> rtw_dbg(rtwdev, RTW_DBG_BF, "mu bfer number over limit\n");
> - goto out_unlock;
> + return;
> }
>
> ether_addr_copy(bfee->mac_addr, bssid);
> @@ -75,7 +79,7 @@ void rtw_bf_assoc(struct rtw_dev *rtwdev, struct ieee80211_vif *vif,
> (vht_cap->cap & IEEE80211_VHT_CAP_SU_BEAMFORMER_CAPABLE)) {
> if (bfinfo->bfer_su_cnt >= chip->bfer_su_max_num) {
> rtw_dbg(rtwdev, RTW_DBG_BF, "su bfer number over limit\n");
> - goto out_unlock;
> + return;
> }
>
> sound_dim = vht_cap->cap &
> @@ -98,9 +102,6 @@ void rtw_bf_assoc(struct rtw_dev *rtwdev, struct ieee80211_vif *vif,
>
> rtw_chip_config_bfee(rtwdev, rtwvif, bfee, true);
> }
> -
> -out_unlock:
> - rcu_read_unlock();
> }
>
> void rtw_bf_init_bfer_entry_mu(struct rtw_dev *rtwdev,
> --
> 2.39.0

2022-12-29 09:51:37

[permalink] [raw]

Subject: RE: [PATCH 4/4] rtw88: Use non-atomic rtw_iterate_stas() in rtw_ra_mask_info_update()

> -----Original Message-----
> From: Martin Blumenstingl <[email protected]>
> Sent: Wednesday, December 28, 2022 9:36 PM
> To: [email protected]
> Cc: [email protected]; [email protected]; Ping-Ke Shih <[email protected]>; [email protected];
> [email protected]; [email protected]; [email protected]; Martin Blumenstingl
> <[email protected]>
> Subject: [PATCH 4/4] rtw88: Use non-atomic rtw_iterate_stas() in rtw_ra_mask_info_update()
>
> USB and (upcoming) SDIO support may sleep in the read/write handlers.
> Use non-atomic rtw_iterate_stas() in rtw_ra_mask_info_update() because
> the iterator function rtw_ra_mask_info_update_iter() needs to read and
> write registers from within rtw_update_sta_info(). Using the non-atomic
> iterator ensures that we can sleep during USB and SDIO register reads
> and writes. This fixes "scheduling while atomic" or "Voluntary context
> switch within RCU read-side critical section!" warnings as seen by SDIO
> card users (but it also affects USB cards).
>
> Fixes: 78d5bf925f30 ("wifi: rtw88: iterate over vif/sta list non-atomically")
> Suggested-by: Ping-Ke Shih <[email protected]>
> Signed-off-by: Martin Blumenstingl <[email protected]>

Reviewed-by: Ping-Ke Shih <[email protected]>

> ---
> drivers/net/wireless/realtek/rtw88/mac80211.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/net/wireless/realtek/rtw88/mac80211.c
> b/drivers/net/wireless/realtek/rtw88/mac80211.c
> index 776a9a9884b5..3b92ac611d3f 100644
> --- a/drivers/net/wireless/realtek/rtw88/mac80211.c
> +++ b/drivers/net/wireless/realtek/rtw88/mac80211.c
> @@ -737,7 +737,7 @@ static void rtw_ra_mask_info_update(struct rtw_dev *rtwdev,
> br_data.rtwdev = rtwdev;
> br_data.vif = vif;
> br_data.mask = mask;
> - rtw_iterate_stas_atomic(rtwdev, rtw_ra_mask_info_update_iter, &br_data);
> + rtw_iterate_stas(rtwdev, rtw_ra_mask_info_update_iter, &br_data);
> }
>
> static int rtw_ops_set_bitrate_mask(struct ieee80211_hw *hw,
> @@ -746,7 +746,9 @@ static int rtw_ops_set_bitrate_mask(struct ieee80211_hw *hw,
> {
> struct rtw_dev *rtwdev = hw->priv;
>
> + mutex_lock(&rtwdev->mutex);
> rtw_ra_mask_info_update(rtwdev, vif, mask);
> + mutex_unlock(&rtwdev->mutex);
>
> return 0;
> }
> --
> 2.39.0

2022-12-29 09:51:54

[permalink] [raw]

Subject: RE: [PATCH 3/4] rtw88: Use rtw_iterate_vifs() for rtw_vif_watch_dog_iter()

> -----Original Message-----
> From: Martin Blumenstingl <[email protected]>
> Sent: Wednesday, December 28, 2022 9:36 PM
> To: [email protected]
> Cc: [email protected]; [email protected]; Ping-Ke Shih <[email protected]>; [email protected];
> [email protected]; [email protected]; [email protected]; Martin Blumenstingl
> <[email protected]>
> Subject: [PATCH 3/4] rtw88: Use rtw_iterate_vifs() for rtw_vif_watch_dog_iter()
>
> USB and (upcoming) SDIO support may sleep in the read/write handlers.
> Make rtw_watch_dog_work() use rtw_iterate_vifs() to prevent "scheduling
> while atomic" or "Voluntary context switch within RCU read-side
> critical section!" warnings when accessing the registers using an SDIO
> card (which is where this issue has been spotted in the real world but
> it also affects USB cards).
>
> Fixes: 78d5bf925f30 ("wifi: rtw88: iterate over vif/sta list non-atomically")
> Suggested-by: Ping-Ke Shih <[email protected]>
> Signed-off-by: Martin Blumenstingl <[email protected]>

Reviewed-by: Ping-Ke Shih <[email protected]>

> ---
> drivers/net/wireless/realtek/rtw88/main.c | 6 ++++--
> 1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/net/wireless/realtek/rtw88/main.c b/drivers/net/wireless/realtek/rtw88/main.c
> index 888427cf3bdf..b2e78737bd5d 100644
> --- a/drivers/net/wireless/realtek/rtw88/main.c
> +++ b/drivers/net/wireless/realtek/rtw88/main.c
> @@ -241,8 +241,10 @@ static void rtw_watch_dog_work(struct work_struct *work)
> rtw_phy_dynamic_mechanism(rtwdev);
>
> data.rtwdev = rtwdev;
> - /* use atomic version to avoid taking local->iflist_mtx mutex */
> - rtw_iterate_vifs_atomic(rtwdev, rtw_vif_watch_dog_iter, &data);
> + /* rtw_iterate_vifs internally uses an atomic iterator which is needed
> + * to avoid taking local->iflist_mtx mutex
> + */
> + rtw_iterate_vifs(rtwdev, rtw_vif_watch_dog_iter, &data);
>
> /* fw supports only one station associated to enter lps, if there are
> * more than two stations associated to the AP, then we can not enter
> --
> 2.39.0

2022-12-29 10:38:27

[permalink] [raw]

Subject: Re: [PATCH 1/4] rtw88: Add packed attribute to the eFuse structs

Hi Ping-Ke,

On Thu, Dec 29, 2022 at 10:25 AM Ping-Ke Shih <[email protected]> wrote:
[...]
>
> > @@ -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.
My understanding is that it should look like this (replacing all of res0..res3):
__le16 some_field_name; /* 0xf4 */
How to call that single __le16 field then?

I also tried using bit-fields for an __le16 (so basically the same as
my patch but using __le16 instead of u16) but that makes sparse
complain:
error: invalid bitfield specifier for type restricted __le16

Best regards,
Martin

2022-12-29 10:47:49

[permalink] [raw]

Subject: Re: [PATCH 0/4] rtw88: Four fixes found while working on SDIO support

Hi Ping-Ke,

On Thu, Dec 29, 2022 at 10:26 AM Ping-Ke Shih <[email protected]> wrote:
[...]
> > Martin Blumenstingl (4):
> > rtw88: Add packed attribute to the eFuse structs
>
> I think this patch depends on another patchset or oppositely.
> Please point that out for reviewers.
There are no dependencies for this smaller individual series other
than Linux 6.2-rc1 (as this has USB support). I made sure to not
include any of the SDIO changes in this series.
The idea is that it can be applied individually and make it either
into 6.2-rc2 (or newer) or -next (6.3).

Best regards,
Martin

2022-12-29 11:36:30

[permalink] [raw]

Subject: Re: [PATCH 1/4] rtw88: Add packed attribute to the eFuse structs

On Thu, 2022-12-29 at 11:37 +0100, Martin Blumenstingl wrote:
> Hi Ping-Ke,
>
> On Thu, Dec 29, 2022 at 10:25 AM Ping-Ke Shih <[email protected]> wrote:
> [...]
> > > @@ -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.
> My understanding is that it should look like this (replacing all of res0..res3):
> __le16 some_field_name; /* 0xf4 */
> How to call that single __le16 field then?

You are right. Maybe, we can name it 'pcie_cap'.
But, we don't use them for now, so it is harmless to preserve them as is.

>
> I also tried using bit-fields for an __le16 (so basically the same as
> my patch but using __le16 instead of u16) but that makes sparse
> complain:
> error: invalid bitfield specifier for type restricted __le16
>
>

We can fix it by:

u8 res0:2; /* 0xf4 */
u8 ltr_en:1;
u8 res1:2;
u8 obff:2;
- u8 res2:3;
+ u8 res2_1:1;
+ u8 res2_2:2;
u8 obff_cap:2;
u8 res3:4;

I'm not sure why people merge bit fields res2_1:1 and res2_2:2 that
should be in different u8. I have confirmed this with internal data.

--
Ping-Ke

2022-12-29 11:52:36

[permalink] [raw]

Subject: Re: [PATCH 0/4] rtw88: Four fixes found while working on SDIO support

On Thu, 2022-12-29 at 11:40 +0100, Martin Blumenstingl wrote:
> Hi Ping-Ke,
>
> On Thu, Dec 29, 2022 at 10:26 AM Ping-Ke Shih <[email protected]> wrote:
> [...]
> > > Martin Blumenstingl (4):
> > > rtw88: Add packed attribute to the eFuse structs
> >
> > I think this patch depends on another patchset or oppositely.
> > Please point that out for reviewers.
> There are no dependencies for this smaller individual series other
> than Linux 6.2-rc1 (as this has USB support). I made sure to not
> include any of the SDIO changes in this series.
> The idea is that it can be applied individually and make it either
> into 6.2-rc2 (or newer) or -next (6.3).
>

I thought this could depend on SDIO patchset, because you add
struct for efuse layout nearby, so there may be merge conflicts.
Please ignore this comment, then.

Ping-Ke

2022-12-31 17:08:10

[permalink] [raw]

Subject: RE: [PATCH 1/4] rtw88: Add packed attribute to the eFuse structs

From: Ping-Ke Shih
> Sent: 29 December 2022 09:25
>
> > -----Original Message-----
> > From: Martin Blumenstingl <[email protected]>
> > Sent: Wednesday, December 28, 2022 9:36 PM
> > To: [email protected]
> > Cc: [email protected]; [email protected]; Ping-Ke Shih <[email protected]>;
> [email protected];
> > [email protected]; [email protected]; [email protected]; Martin Blumenstingl
> > <[email protected]>
> > 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 <[email protected]>
> > ---
> > 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)

2023-01-01 11:48:47

[permalink] [raw]

Subject: Re: [PATCH 1/4] rtw88: Add packed attribute to the eFuse structs

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 <[email protected]>
> > > Sent: Wednesday, December 28, 2022 9:36 PM
> > > To: [email protected]
> > > Cc: [email protected]; [email protected]; Ping-Ke Shih <[email protected]>;
> > [email protected];
> > > [email protected]; [email protected]; [email protected]; Martin
> > > Blumenstingl
> > > <[email protected]>
> > > 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 <[email protected]>
> > > ---
> > > 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

2023-01-01 11:55:31

[permalink] [raw]

Subject: RE: [PATCH 1/4] rtw88: Add packed attribute to the eFuse structs

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 <[email protected]>
> > > > Sent: Wednesday, December 28, 2022 9:36 PM
> > > > To: [email protected]
> > > > Cc: [email protected]; [email protected]; Ping-Ke Shih <[email protected]>;
> > > [email protected];
> > > > [email protected]; [email protected]; [email protected]; Martin
> > > > Blumenstingl
> > > > <[email protected]>
> > > > 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 <[email protected]>
> > > > ---
> > > > 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)

2023-01-01 13:15:45

[permalink] [raw]

Subject: Re: [PATCH 1/4] rtw88: Add packed attribute to the eFuse structs

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 <[email protected]>
> > > > > Sent: Wednesday, December 28, 2022 9:36 PM
> > > > > To: [email protected]
> > > > > Cc: [email protected]; [email protected]; Ping-Ke Shih <[email protected]>;
> > > > [email protected];
> > > > > [email protected]; [email protected]; [email protected]; Martin
> > > > > Blumenstingl
> > > > > <[email protected]>
> > > > > 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 <[email protected]>
> > > > > ---
> > > > > 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

2023-01-04 15:35:07

[permalink] [raw]

Subject: Re: [PATCH 1/4] rtw88: Add packed attribute to the eFuse structs

Hi Ping-Ke, Hi David,

On Sun, Jan 1, 2023 at 2:09 PM Ping-Ke Shih <[email protected]> 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/[email protected]/

2023-01-04 15:55:48

[permalink] [raw]

Subject: RE: [PATCH 1/4] rtw88: Add packed attribute to the eFuse structs

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 <[email protected]> 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)

2023-01-04 16:08:13

[permalink] [raw]

Subject: Re: [PATCH 1/4] rtw88: Add packed attribute to the eFuse structs

On Wed, Jan 4, 2023 at 4:53 PM David Laight <[email protected]> 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 <[email protected]> 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

2023-01-04 16:36:23

[permalink] [raw]

Subject: RE: [PATCH 1/4] rtw88: Add packed attribute to the eFuse structs

From: Martin Blumenstingl
> Sent: 04 January 2023 16:08
>
> On Wed, Jan 4, 2023 at 4:53 PM David Laight <[email protected]> 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 <[email protected]> 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)

2023-01-04 17:50:37

[permalink] [raw]

Subject: Re: [PATCH 1/4] rtw88: Add packed attribute to the eFuse structs

On Wed, Jan 4, 2023 at 5:31 PM David Laight <[email protected]> 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

Attachments:

add-build-bug-on.patch (1.94 kB)

2023-01-05 01:16:07

[permalink] [raw]

Subject: RE: [PATCH 1/4] rtw88: Add packed attribute to the eFuse structs

> -----Original Message-----
> From: Martin Blumenstingl <[email protected]>
> Sent: Thursday, January 5, 2023 1:49 AM
> To: David Laight <[email protected]>
> Cc: Ping-Ke Shih <[email protected]>; [email protected]; [email protected];
> [email protected]; [email protected]; [email protected]; [email protected];
> [email protected]
> Subject: Re: [PATCH 1/4] rtw88: Add packed attribute to the eFuse structs
>
> On Wed, Jan 4, 2023 at 5:31 PM David Laight <[email protected]> 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

2023-01-05 08:43:03

[permalink] [raw]

Subject: RE: [PATCH 1/4] rtw88: Add packed attribute to the eFuse structs

From: Martin Blumenstingl
> Sent: 04 January 2023 17:49
>
> On Wed, Jan 4, 2023 at 5:31 PM David Laight <[email protected]> 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)

2023-01-10 12:04:59

by Kalle Valo

[permalink] [raw]

Subject: Re: [PATCH 1/4] rtw88: Add packed attribute to the eFuse structs

David Laight <[email protected]> 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 <[email protected]> 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.

--
https://patchwork.kernel.org/project/linux-wireless/list/

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

2023-01-10 12:12:27

by Kalle Valo

[permalink] [raw]

Subject: Re: [PATCH 0/4] rtw88: Four fixes found while working on SDIO support

Martin Blumenstingl <[email protected]> writes:

> Hi Ping-Ke,
>
> On Thu, Dec 29, 2022 at 10:26 AM Ping-Ke Shih <[email protected]> wrote:
> [...]
>> > Martin Blumenstingl (4):
>> > rtw88: Add packed attribute to the eFuse structs
>>
>> I think this patch depends on another patchset or oppositely.
>> Please point that out for reviewers.
>
> There are no dependencies for this smaller individual series other
> than Linux 6.2-rc1 (as this has USB support). I made sure to not
> include any of the SDIO changes in this series.
> The idea is that it can be applied individually and make it either
> into 6.2-rc2 (or newer) or -next (6.3).

BTW wireless-next or wireless-testing are the preferred baselines for
wireless patches. Of course you can use other trees if you really want,
but please try to make sure they apply to wireless-next. Conflicts are
always extra churn I would prefer to avoid.

--
https://patchwork.kernel.org/project/linux-wireless/list/

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

2023-01-10 12:47:38