[PATCH v3 0/8] rtw88: prepare locking for SDIO support

[PATCH v3 0/8] rtw88: prepare locking for SDIO support

[Martin Blumenstingl]

Hello rtw88 and mac80211 maintainers/contributors,

there is an ongoing effort where Jernej and I are working on adding
SDIO support to the rtw88 driver [0].
The hardware we use at the moment is RTL8822BS and RTL8822CS.
We are at a point where scanning, assoc etc. works. Performance is
better in the latest version:
- RX throughput is between 20Mbit/s and 25Mbit/s on RTL8822CS
- TX throughput varies a lot, typically it's between 25Mbit/s and
  50Mbit/s

This series contains some preparation work for adding SDIO support.
While testing our changes we found that there are some "scheduling
while atomic" errors in the kernel log. These are due to the fact
that the SDIO accessors (sdio_readb, sdio_writeb and friends) may
sleep internally.

Some background on why SDIO access (for example: sdio_writeb) cannot
be done with a spinlock held (this is a copy from my previous mail,
see [1]):
- when using for example sdio_writeb the MMC subsystem in Linux
  prepares a so-called MMC request
- this request is submitted to the MMC host controller hardware
- the host controller hardware forwards the MMC request to the card
- the card signals when it's done processing the request
- the MMC subsystem in Linux waits for the card to signal that it's
done processing the request in mmc_wait_for_req_done() -> this uses
wait_for_completion() internally, which might sleep (which is not
allowed while a spinlock is held)

Based on Ping-Ke's suggestion I came up with the code in this series.
The goal is to use non-atomic locking for all register access in the
rtw88 driver. One patch adds a new function to mac80211 which did not
have a "non-atomic" version of it's "atomic" counterpart yet.

As mentioned before I don't have any rtw88 PCIe device so I am unable
to test on that hardware.
I am sending this as an RFC series since I am new to the mac80211
subsystem as well as the rtw88 driver. So any kind of feedback is
very welcome!
The actual changes for adding SDIO support will be sent separately in
the future.


Changes since v2 at [3]:
- patch #1: dropped "mac80211: Add stations iterator where the iterator
  function may sleep" which was already applied to mac80211-next by
  Johannes (thanks!)
- patch #2 (previously #3): move locking to the (only) caller of
  rtw_ra_mask_info_update() to make the locking consistent with other
  functions. Thank you Ping-Ke for this suggestion!
- cover-letter: update link to the current SDIO work-in-progress code
  at [0]

Changes since v1 at [2] (which I sent back in summer):
- patch #1: fixed kernel doc copy & paste (remove _atomic) as suggested
  by Ping-Ke and Johannes
- patch #1: added paragraph about driver authors having to be careful
  where they use this new function as suggested by Johannes
- patch #2 (new): keep rtw_iterate_vifs_atomic() to not undo the fix
  from commit 5b0efb4d670c8 ("rtw88: avoid circular locking between
  local->iflist_mtx and rtwdev->mutex") and instead call
  rtw_bf_cfg_csi_rate() from rtw_watch_dog_work() (outside the atomic
  section) as suggested by Ping-Ke.
- patch #3 (new): keep rtw_iterate_vifs_atomic() to prevent deadlocks
  as Johannes suggested. Keep track of all relevant stations inside
  rtw_ra_mask_info_update_iter() and the iter-data and then call
  rtw_update_sta_info() while held under rtwdev->mutex instead
- patch #7: shrink the critical section as suggested by Ping-Ke


[0] https://github.com/xdarklight/linux/tree/rtw88-test-20220107
[1] https://lore.kernel.org/linux-wireless/CAFBinCDMPPJ7qW7xTkep1Trg+zP0B9Jxei6sgjqmF4NDA1JAhQ@mail.gmail.com/
[2] https://lore.kernel.org/netdev/2170471a1c144adb882d06e08f3c9d1a@realtek.com/T/
[3] https://lore.kernel.org/netdev/20211228211501.468981-1-martin.blumenstingl@googlemail.com/


Martin Blumenstingl (8):
  rtw88: Move rtw_chip_cfg_csi_rate() out of rtw_vif_watch_dog_iter()
  rtw88: Move rtw_update_sta_info() out of
    rtw_ra_mask_info_update_iter()
  rtw88: Use rtw_iterate_vifs where the iterator reads or writes
    registers
  rtw88: Use rtw_iterate_stas where the iterator reads or writes
    registers
  rtw88: Replace usage of rtw_iterate_keys_rcu() with rtw_iterate_keys()
  rtw88: Configure the registers from rtw_bf_assoc() outside the RCU
    lock
  rtw88: hci: Convert rf_lock from a spinlock to a mutex
  rtw88: fw: Convert h2c.lock from a spinlock to a mutex

 drivers/net/wireless/realtek/rtw88/bf.c       | 13 ++---
 drivers/net/wireless/realtek/rtw88/fw.c       | 14 +++---
 drivers/net/wireless/realtek/rtw88/hci.h      | 11 ++---
 drivers/net/wireless/realtek/rtw88/mac80211.c | 12 ++++-
 drivers/net/wireless/realtek/rtw88/main.c     | 47 +++++++++----------
 drivers/net/wireless/realtek/rtw88/main.h     |  4 +-
 drivers/net/wireless/realtek/rtw88/phy.c      |  4 +-
 drivers/net/wireless/realtek/rtw88/ps.c       |  2 +-
 drivers/net/wireless/realtek/rtw88/util.h     |  4 +-
 drivers/net/wireless/realtek/rtw88/wow.c      |  2 +-
 10 files changed, 58 insertions(+), 55 deletions(-)

-- 
2.34.1

[PATCH v3 1/8] rtw88: Move rtw_chip_cfg_csi_rate() out of rtw_vif_watch_dog_iter()

[Martin Blumenstingl]

rtw_bf_cfg_csi_rate() internally access some registers while being
called unter an atomic lock acquired by rtw_iterate_vifs_atomic(). Move
the rtw_bf_cfg_csi_rate() call out of rtw_vif_watch_dog_iter() in
preparation for SDIO support where register access may sleep.

Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
---
v2 -> v3:
- no changes

v1 -> v2:
- keep rtw_iterate_vifs_atomic() to not undo the fix from commit
  5b0efb4d670c8 ("rtw88: avoid circular locking between
  local->iflist_mtx and rtwdev->mutex") and instead call
  rtw_bf_cfg_csi_rate() from rtw_watch_dog_work() (outside the atomic
  section) as suggested by Ping-Ke.

 drivers/net/wireless/realtek/rtw88/main.c | 35 +++++++++++------------
 1 file changed, 16 insertions(+), 19 deletions(-)

diff --git a/drivers/net/wireless/realtek/rtw88/main.c b/drivers/net/wireless/realtek/rtw88/main.c
index 38252113c4a8..fd02c0b0025a 100644
--- a/drivers/net/wireless/realtek/rtw88/main.c
+++ b/drivers/net/wireless/realtek/rtw88/main.c
@@ -144,26 +144,9 @@ static struct ieee80211_supported_band rtw_band_5ghz = {
 struct rtw_watch_dog_iter_data {
 	struct rtw_dev *rtwdev;
 	struct rtw_vif *rtwvif;
+	bool cfg_csi_rate;
 };
 
-static void rtw_dynamic_csi_rate(struct rtw_dev *rtwdev, struct rtw_vif *rtwvif)
-{
-	struct rtw_bf_info *bf_info = &rtwdev->bf_info;
-	u8 fix_rate_enable = 0;
-	u8 new_csi_rate_idx;
-
-	if (rtwvif->bfee.role != RTW_BFEE_SU &&
-	    rtwvif->bfee.role != RTW_BFEE_MU)
-		return;
-
-	rtw_chip_cfg_csi_rate(rtwdev, rtwdev->dm_info.min_rssi,
-			      bf_info->cur_csi_rpt_rate,
-			      fix_rate_enable, &new_csi_rate_idx);
-
-	if (new_csi_rate_idx != bf_info->cur_csi_rpt_rate)
-		bf_info->cur_csi_rpt_rate = new_csi_rate_idx;
-}
-
 static void rtw_vif_watch_dog_iter(void *data, u8 *mac,
 				   struct ieee80211_vif *vif)
 {
@@ -174,7 +157,8 @@ static void rtw_vif_watch_dog_iter(void *data, u8 *mac,
 		if (vif->bss_conf.assoc)
 			iter_data->rtwvif = rtwvif;
 
-	rtw_dynamic_csi_rate(iter_data->rtwdev, rtwvif);
+	iter_data->cfg_csi_rate = rtwvif->bfee.role == RTW_BFEE_SU ||
+				  rtwvif->bfee.role == RTW_BFEE_MU;
 
 	rtwvif->stats.tx_unicast = 0;
 	rtwvif->stats.rx_unicast = 0;
@@ -241,6 +225,19 @@ static void rtw_watch_dog_work(struct work_struct *work)
 	/* use atomic version to avoid taking local->iflist_mtx mutex */
 	rtw_iterate_vifs_atomic(rtwdev, rtw_vif_watch_dog_iter, &data);
 
+	if (data.cfg_csi_rate) {
+		struct rtw_bf_info *bf_info = &rtwdev->bf_info;
+		u8 fix_rate_enable = 0;
+		u8 new_csi_rate_idx;
+
+		rtw_chip_cfg_csi_rate(rtwdev, rtwdev->dm_info.min_rssi,
+				      bf_info->cur_csi_rpt_rate,
+				      fix_rate_enable, &new_csi_rate_idx);
+
+		if (new_csi_rate_idx != bf_info->cur_csi_rpt_rate)
+			bf_info->cur_csi_rpt_rate = new_csi_rate_idx;
+	}
+
 	/* 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
 	 * lps, because fw does not handle the overlapped beacon interval
-- 
2.34.1

[PATCH v3 2/8] rtw88: Move rtw_update_sta_info() out of rtw_ra_mask_info_update_iter()

[Martin Blumenstingl]

rtw_update_sta_info() internally access some registers while being
called unter an atomic lock acquired by rtw_iterate_vifs_atomic(). Move
rtw_update_sta_info() call out of (rtw_ra_mask_info_update_iter) in
preparation for SDIO support where register access may sleep.

Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
---
v2 -> v3:
- Move the mutex lock (to protect the dta inside br_data.si[i]) to
  rtw_ops_set_bitrate_mask() for consistency with other functions in
  the whole driver (and especially in the same file) as suggested by
  Ping-Ke (thank you!)

v1 -> v2:
- keep rtw_iterate_vifs_atomic() to prevent deadlocks as Johannes
  suggested. Keep track of all relevant stations inside
  rtw_ra_mask_info_update_iter() and the iter-data and then call
  rtw_update_sta_info() while held under rtwdev->mutex instead

 drivers/net/wireless/realtek/rtw88/mac80211.c | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/drivers/net/wireless/realtek/rtw88/mac80211.c b/drivers/net/wireless/realtek/rtw88/mac80211.c
index ae7d97de5fdf..78e963fcc6e1 100644
--- a/drivers/net/wireless/realtek/rtw88/mac80211.c
+++ b/drivers/net/wireless/realtek/rtw88/mac80211.c
@@ -671,6 +671,8 @@ struct rtw_iter_bitrate_mask_data {
 	struct rtw_dev *rtwdev;
 	struct ieee80211_vif *vif;
 	const struct cfg80211_bitrate_mask *mask;
+	unsigned int num_si;
+	struct rtw_sta_info *si[RTW_MAX_MAC_ID_NUM];
 };
 
 static void rtw_ra_mask_info_update_iter(void *data, struct ieee80211_sta *sta)
@@ -691,7 +693,8 @@ static void rtw_ra_mask_info_update_iter(void *data, struct ieee80211_sta *sta)
 	}
 
 	si->use_cfg_mask = true;
-	rtw_update_sta_info(br_data->rtwdev, si);
+
+	br_data->si[br_data->num_si++] = si;
 }
 
 static void rtw_ra_mask_info_update(struct rtw_dev *rtwdev,
@@ -699,11 +702,16 @@ static void rtw_ra_mask_info_update(struct rtw_dev *rtwdev,
 				    const struct cfg80211_bitrate_mask *mask)
 {
 	struct rtw_iter_bitrate_mask_data br_data;
+	unsigned int i;
 
 	br_data.rtwdev = rtwdev;
 	br_data.vif = vif;
 	br_data.mask = mask;
+	br_data.num_si = 0;
 	rtw_iterate_stas_atomic(rtwdev, rtw_ra_mask_info_update_iter, &br_data);
+
+	for (i = 0; i < br_data.num_si; i++)
+		rtw_update_sta_info(rtwdev, br_data.si[i]);
 }
 
 static int rtw_ops_set_bitrate_mask(struct ieee80211_hw *hw,
@@ -712,7 +720,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.34.1

[PATCH v3 3/8] rtw88: Use rtw_iterate_vifs where the iterator reads or writes registers

[Martin Blumenstingl]

Upcoming SDIO support may sleep in the read/write handlers. Switch
all users of rtw_iterate_vifs_atomic() which are either reading or
writing a register to rtw_iterate_vifs().

Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
---
 drivers/net/wireless/realtek/rtw88/main.c | 2 +-
 drivers/net/wireless/realtek/rtw88/ps.c   | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/wireless/realtek/rtw88/main.c b/drivers/net/wireless/realtek/rtw88/main.c
index fd02c0b0025a..b0e2ca8ddbe9 100644
--- a/drivers/net/wireless/realtek/rtw88/main.c
+++ b/drivers/net/wireless/realtek/rtw88/main.c
@@ -585,7 +585,7 @@ static void __fw_recovery_work(struct rtw_dev *rtwdev)
 	rtw_iterate_keys_rcu(rtwdev, NULL, rtw_reset_key_iter, rtwdev);
 	rcu_read_unlock();
 	rtw_iterate_stas_atomic(rtwdev, rtw_reset_sta_iter, rtwdev);
-	rtw_iterate_vifs_atomic(rtwdev, rtw_reset_vif_iter, rtwdev);
+	rtw_iterate_vifs(rtwdev, rtw_reset_vif_iter, rtwdev);
 	rtw_enter_ips(rtwdev);
 }
 
diff --git a/drivers/net/wireless/realtek/rtw88/ps.c b/drivers/net/wireless/realtek/rtw88/ps.c
index bfa64c038f5f..a7213ff2c224 100644
--- a/drivers/net/wireless/realtek/rtw88/ps.c
+++ b/drivers/net/wireless/realtek/rtw88/ps.c
@@ -58,7 +58,7 @@ int rtw_leave_ips(struct rtw_dev *rtwdev)
 		return ret;
 	}
 
-	rtw_iterate_vifs_atomic(rtwdev, rtw_restore_port_cfg_iter, rtwdev);
+	rtw_iterate_vifs(rtwdev, rtw_restore_port_cfg_iter, rtwdev);
 
 	rtw_coex_ips_notify(rtwdev, COEX_IPS_LEAVE);
 
-- 
2.34.1

[PATCH v3 4/8] rtw88: Use rtw_iterate_stas where the iterator reads or writes registers

[Martin Blumenstingl]

Upcoming SDIO support may sleep in the read/write handlers. Switch
all users of rtw_iterate_stas_atomic() which are either reading or
writing a register to rtw_iterate_stas().

Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
---
 drivers/net/wireless/realtek/rtw88/main.c | 2 +-
 drivers/net/wireless/realtek/rtw88/phy.c  | 4 ++--
 drivers/net/wireless/realtek/rtw88/util.h | 2 ++
 drivers/net/wireless/realtek/rtw88/wow.c  | 2 +-
 4 files changed, 6 insertions(+), 4 deletions(-)

diff --git a/drivers/net/wireless/realtek/rtw88/main.c b/drivers/net/wireless/realtek/rtw88/main.c
index b0e2ca8ddbe9..4b28c81b3ca0 100644
--- a/drivers/net/wireless/realtek/rtw88/main.c
+++ b/drivers/net/wireless/realtek/rtw88/main.c
@@ -584,7 +584,7 @@ static void __fw_recovery_work(struct rtw_dev *rtwdev)
 	rcu_read_lock();
 	rtw_iterate_keys_rcu(rtwdev, NULL, rtw_reset_key_iter, rtwdev);
 	rcu_read_unlock();
-	rtw_iterate_stas_atomic(rtwdev, rtw_reset_sta_iter, rtwdev);
+	rtw_iterate_stas(rtwdev, rtw_reset_sta_iter, rtwdev);
 	rtw_iterate_vifs(rtwdev, rtw_reset_vif_iter, rtwdev);
 	rtw_enter_ips(rtwdev);
 }
diff --git a/drivers/net/wireless/realtek/rtw88/phy.c b/drivers/net/wireless/realtek/rtw88/phy.c
index e505d17f107e..d8442adc11b1 100644
--- a/drivers/net/wireless/realtek/rtw88/phy.c
+++ b/drivers/net/wireless/realtek/rtw88/phy.c
@@ -300,7 +300,7 @@ static void rtw_phy_stat_rssi(struct rtw_dev *rtwdev)
 
 	data.rtwdev = rtwdev;
 	data.min_rssi = U8_MAX;
-	rtw_iterate_stas_atomic(rtwdev, rtw_phy_stat_rssi_iter, &data);
+	rtw_iterate_stas(rtwdev, rtw_phy_stat_rssi_iter, &data);
 
 	dm_info->pre_min_rssi = dm_info->min_rssi;
 	dm_info->min_rssi = data.min_rssi;
@@ -544,7 +544,7 @@ static void rtw_phy_ra_info_update(struct rtw_dev *rtwdev)
 	if (rtwdev->watch_dog_cnt & 0x3)
 		return;
 
-	rtw_iterate_stas_atomic(rtwdev, rtw_phy_ra_info_update_iter, rtwdev);
+	rtw_iterate_stas(rtwdev, rtw_phy_ra_info_update_iter, rtwdev);
 }
 
 static u32 rtw_phy_get_rrsr_mask(struct rtw_dev *rtwdev, u8 rate_idx)
diff --git a/drivers/net/wireless/realtek/rtw88/util.h b/drivers/net/wireless/realtek/rtw88/util.h
index 0c23b5069be0..b0dfadf8b82a 100644
--- a/drivers/net/wireless/realtek/rtw88/util.h
+++ b/drivers/net/wireless/realtek/rtw88/util.h
@@ -13,6 +13,8 @@ struct rtw_dev;
 #define rtw_iterate_vifs_atomic(rtwdev, iterator, data)                        \
 	ieee80211_iterate_active_interfaces_atomic(rtwdev->hw,                 \
 			IEEE80211_IFACE_ITER_NORMAL, iterator, data)
+#define rtw_iterate_stas(rtwdev, iterator, data)                        \
+	ieee80211_iterate_stations(rtwdev->hw, iterator, data)
 #define rtw_iterate_stas_atomic(rtwdev, iterator, data)                        \
 	ieee80211_iterate_stations_atomic(rtwdev->hw, iterator, data)
 #define rtw_iterate_keys(rtwdev, vif, iterator, data)			       \
diff --git a/drivers/net/wireless/realtek/rtw88/wow.c b/drivers/net/wireless/realtek/rtw88/wow.c
index 89dc595094d5..7ec0731c0346 100644
--- a/drivers/net/wireless/realtek/rtw88/wow.c
+++ b/drivers/net/wireless/realtek/rtw88/wow.c
@@ -468,7 +468,7 @@ static void rtw_wow_fw_media_status(struct rtw_dev *rtwdev, bool connect)
 	data.rtwdev = rtwdev;
 	data.connect = connect;
 
-	rtw_iterate_stas_atomic(rtwdev, rtw_wow_fw_media_status_iter, &data);
+	rtw_iterate_stas(rtwdev, rtw_wow_fw_media_status_iter, &data);
 }
 
 static int rtw_wow_config_wow_fw_rsvd_page(struct rtw_dev *rtwdev)
-- 
2.34.1

[PATCH v3 5/8] rtw88: Replace usage of rtw_iterate_keys_rcu() with rtw_iterate_keys()

[Martin Blumenstingl]

Upcoming SDIO support may sleep in the read/write handlers. The only
occurrence of rtw_iterate_keys_rcu() reads and writes registers from
it's iterator function. Replace it with rtw_iterate_keys() (the non-RCU
version). This will prevent an "scheduling while atomic" issue when
using an SDIO device.

Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
---
 drivers/net/wireless/realtek/rtw88/main.c | 4 +---
 drivers/net/wireless/realtek/rtw88/util.h | 2 --
 2 files changed, 1 insertion(+), 5 deletions(-)

diff --git a/drivers/net/wireless/realtek/rtw88/main.c b/drivers/net/wireless/realtek/rtw88/main.c
index 4b28c81b3ca0..3d4257e0367a 100644
--- a/drivers/net/wireless/realtek/rtw88/main.c
+++ b/drivers/net/wireless/realtek/rtw88/main.c
@@ -581,9 +581,7 @@ static void __fw_recovery_work(struct rtw_dev *rtwdev)
 
 	WARN(1, "firmware crash, start reset and recover\n");
 
-	rcu_read_lock();
-	rtw_iterate_keys_rcu(rtwdev, NULL, rtw_reset_key_iter, rtwdev);
-	rcu_read_unlock();
+	rtw_iterate_keys(rtwdev, NULL, rtw_reset_key_iter, rtwdev);
 	rtw_iterate_stas(rtwdev, rtw_reset_sta_iter, rtwdev);
 	rtw_iterate_vifs(rtwdev, rtw_reset_vif_iter, rtwdev);
 	rtw_enter_ips(rtwdev);
diff --git a/drivers/net/wireless/realtek/rtw88/util.h b/drivers/net/wireless/realtek/rtw88/util.h
index b0dfadf8b82a..06a5b4c4111c 100644
--- a/drivers/net/wireless/realtek/rtw88/util.h
+++ b/drivers/net/wireless/realtek/rtw88/util.h
@@ -19,8 +19,6 @@ struct rtw_dev;
 	ieee80211_iterate_stations_atomic(rtwdev->hw, iterator, data)
 #define rtw_iterate_keys(rtwdev, vif, iterator, data)			       \
 	ieee80211_iter_keys(rtwdev->hw, vif, iterator, data)
-#define rtw_iterate_keys_rcu(rtwdev, vif, iterator, data)		       \
-	ieee80211_iter_keys_rcu((rtwdev)->hw, vif, iterator, data)
 
 static inline u8 *get_hdr_bssid(struct ieee80211_hdr *hdr)
 {
-- 
2.34.1

[PATCH v3 6/8] rtw88: Configure the registers from rtw_bf_assoc() outside the RCU lock

[Martin Blumenstingl]

Upcoming SDIO support may sleep in the read/write handlers. Shrink the
RCU critical section so it only cover the ieee80211_find_sta() call 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 a "scheduling while atomic" issue when accessing the registers
using an SDIO card.

Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
---
v2 -> v3:
- no changes

v1 -> v2:
- shrink the critical section as suggested by Ping-Ke

 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 df750b3a35e9..792eb9930269 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->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.34.1

[PATCH v3 7/8] rtw88: hci: Convert rf_lock from a spinlock to a mutex

[Martin Blumenstingl]

Upcoming SDIO support may sleep in the read/write handlers. Switch
rf_lock from a spinlock to a mutex to allow for this behavior.

Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
---
 drivers/net/wireless/realtek/rtw88/hci.h  | 11 ++++-------
 drivers/net/wireless/realtek/rtw88/main.c |  2 +-
 drivers/net/wireless/realtek/rtw88/main.h |  2 +-
 3 files changed, 6 insertions(+), 9 deletions(-)

diff --git a/drivers/net/wireless/realtek/rtw88/hci.h b/drivers/net/wireless/realtek/rtw88/hci.h
index 4c6fc6fb3f83..3c730b7a94f7 100644
--- a/drivers/net/wireless/realtek/rtw88/hci.h
+++ b/drivers/net/wireless/realtek/rtw88/hci.h
@@ -166,12 +166,11 @@ static inline u32
 rtw_read_rf(struct rtw_dev *rtwdev, enum rtw_rf_path rf_path,
 	    u32 addr, u32 mask)
 {
-	unsigned long flags;
 	u32 val;
 
-	spin_lock_irqsave(&rtwdev->rf_lock, flags);
+	mutex_lock(&rtwdev->rf_lock);
 	val = rtwdev->chip->ops->read_rf(rtwdev, rf_path, addr, mask);
-	spin_unlock_irqrestore(&rtwdev->rf_lock, flags);
+	mutex_unlock(&rtwdev->rf_lock);
 
 	return val;
 }
@@ -180,11 +179,9 @@ static inline void
 rtw_write_rf(struct rtw_dev *rtwdev, enum rtw_rf_path rf_path,
 	     u32 addr, u32 mask, u32 data)
 {
-	unsigned long flags;
-
-	spin_lock_irqsave(&rtwdev->rf_lock, flags);
+	mutex_lock(&rtwdev->rf_lock);
 	rtwdev->chip->ops->write_rf(rtwdev, rf_path, addr, mask, data);
-	spin_unlock_irqrestore(&rtwdev->rf_lock, flags);
+	mutex_unlock(&rtwdev->rf_lock);
 }
 
 static inline u32
diff --git a/drivers/net/wireless/realtek/rtw88/main.c b/drivers/net/wireless/realtek/rtw88/main.c
index 3d4257e0367a..a94678effd77 100644
--- a/drivers/net/wireless/realtek/rtw88/main.c
+++ b/drivers/net/wireless/realtek/rtw88/main.c
@@ -1920,12 +1920,12 @@ int rtw_core_init(struct rtw_dev *rtwdev)
 	skb_queue_head_init(&rtwdev->coex.queue);
 	skb_queue_head_init(&rtwdev->tx_report.queue);
 
-	spin_lock_init(&rtwdev->rf_lock);
 	spin_lock_init(&rtwdev->h2c.lock);
 	spin_lock_init(&rtwdev->txq_lock);
 	spin_lock_init(&rtwdev->tx_report.q_lock);
 
 	mutex_init(&rtwdev->mutex);
+	mutex_init(&rtwdev->rf_lock);
 	mutex_init(&rtwdev->coex.mutex);
 	mutex_init(&rtwdev->hal.tx_power_mutex);
 
diff --git a/drivers/net/wireless/realtek/rtw88/main.h b/drivers/net/wireless/realtek/rtw88/main.h
index dc1cd9bd4b8a..e7a60e6f8596 100644
--- a/drivers/net/wireless/realtek/rtw88/main.h
+++ b/drivers/net/wireless/realtek/rtw88/main.h
@@ -1949,7 +1949,7 @@ struct rtw_dev {
 	struct mutex mutex;
 
 	/* read/write rf register */
-	spinlock_t rf_lock;
+	struct mutex rf_lock;
 
 	/* watch dog every 2 sec */
 	struct delayed_work watch_dog_work;
-- 
2.34.1

[PATCH v3 8/8] rtw88: fw: Convert h2c.lock from a spinlock to a mutex

[Martin Blumenstingl]

Upcoming SDIO support may sleep in the read/write handlers. Switch
the h2c.lock from a spinlock to a mutex to allow for this behavior.

Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
---
 drivers/net/wireless/realtek/rtw88/fw.c   | 14 +++++++-------
 drivers/net/wireless/realtek/rtw88/main.c |  2 +-
 drivers/net/wireless/realtek/rtw88/main.h |  2 +-
 3 files changed, 9 insertions(+), 9 deletions(-)

diff --git a/drivers/net/wireless/realtek/rtw88/fw.c b/drivers/net/wireless/realtek/rtw88/fw.c
index 2f7c036f9022..1bff43c14a05 100644
--- a/drivers/net/wireless/realtek/rtw88/fw.c
+++ b/drivers/net/wireless/realtek/rtw88/fw.c
@@ -317,7 +317,7 @@ static void rtw_fw_send_h2c_command(struct rtw_dev *rtwdev,
 		h2c[3], h2c[2], h2c[1], h2c[0],
 		h2c[7], h2c[6], h2c[5], h2c[4]);
 
-	spin_lock(&rtwdev->h2c.lock);
+	mutex_lock(&rtwdev->h2c.lock);
 
 	box = rtwdev->h2c.last_box_num;
 	switch (box) {
@@ -342,9 +342,9 @@ static void rtw_fw_send_h2c_command(struct rtw_dev *rtwdev,
 		goto out;
 	}
 
-	ret = read_poll_timeout_atomic(rtw_read8, box_state,
-				       !((box_state >> box) & 0x1), 100, 3000,
-				       false, rtwdev, REG_HMETFR);
+	ret = read_poll_timeout(rtw_read8, box_state,
+				!((box_state >> box) & 0x1), 100, 3000, false,
+				rtwdev, REG_HMETFR);
 
 	if (ret) {
 		rtw_err(rtwdev, "failed to send h2c command\n");
@@ -360,7 +360,7 @@ static void rtw_fw_send_h2c_command(struct rtw_dev *rtwdev,
 		rtwdev->h2c.last_box_num = 0;
 
 out:
-	spin_unlock(&rtwdev->h2c.lock);
+	mutex_unlock(&rtwdev->h2c.lock);
 }
 
 void rtw_fw_h2c_cmd_dbg(struct rtw_dev *rtwdev, u8 *h2c)
@@ -372,7 +372,7 @@ static void rtw_fw_send_h2c_packet(struct rtw_dev *rtwdev, u8 *h2c_pkt)
 {
 	int ret;
 
-	spin_lock(&rtwdev->h2c.lock);
+	mutex_lock(&rtwdev->h2c.lock);
 
 	FW_OFFLOAD_H2C_SET_SEQ_NUM(h2c_pkt, rtwdev->h2c.seq);
 	ret = rtw_hci_write_data_h2c(rtwdev, h2c_pkt, H2C_PKT_SIZE);
@@ -380,7 +380,7 @@ static void rtw_fw_send_h2c_packet(struct rtw_dev *rtwdev, u8 *h2c_pkt)
 		rtw_err(rtwdev, "failed to send h2c packet\n");
 	rtwdev->h2c.seq++;
 
-	spin_unlock(&rtwdev->h2c.lock);
+	mutex_unlock(&rtwdev->h2c.lock);
 }
 
 void
diff --git a/drivers/net/wireless/realtek/rtw88/main.c b/drivers/net/wireless/realtek/rtw88/main.c
index a94678effd77..e883f5ecf307 100644
--- a/drivers/net/wireless/realtek/rtw88/main.c
+++ b/drivers/net/wireless/realtek/rtw88/main.c
@@ -1920,12 +1920,12 @@ int rtw_core_init(struct rtw_dev *rtwdev)
 	skb_queue_head_init(&rtwdev->coex.queue);
 	skb_queue_head_init(&rtwdev->tx_report.queue);
 
-	spin_lock_init(&rtwdev->h2c.lock);
 	spin_lock_init(&rtwdev->txq_lock);
 	spin_lock_init(&rtwdev->tx_report.q_lock);
 
 	mutex_init(&rtwdev->mutex);
 	mutex_init(&rtwdev->rf_lock);
+	mutex_init(&rtwdev->h2c.lock);
 	mutex_init(&rtwdev->coex.mutex);
 	mutex_init(&rtwdev->hal.tx_power_mutex);
 
diff --git a/drivers/net/wireless/realtek/rtw88/main.h b/drivers/net/wireless/realtek/rtw88/main.h
index e7a60e6f8596..495a28028ac0 100644
--- a/drivers/net/wireless/realtek/rtw88/main.h
+++ b/drivers/net/wireless/realtek/rtw88/main.h
@@ -1975,7 +1975,7 @@ struct rtw_dev {
 		/* incicate the mail box to use with fw */
 		u8 last_box_num;
 		/* protect to send h2c to fw */
-		spinlock_t lock;
+		struct mutex lock;
 		u32 seq;
 	} h2c;
 
-- 
2.34.1

RE: [PATCH v3 0/8] rtw88: prepare locking for SDIO support

[Pkshih]

Hi,

> -----Original Message-----
> From: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
> Sent: Saturday, January 8, 2022 8:55 AM
> To: linux-wireless@vger.kernel.org
> Cc: tony0620emma@gmail.com; kvalo@codeaurora.org; johannes@sipsolutions.net; netdev@vger.kernel.org;
> linux-kernel@vger.kernel.org; Neo Jou <neojou@gmail.com>; Jernej Skrabec <jernej.skrabec@gmail.com>;
> Pkshih <pkshih@realtek.com>; Ed Swierk <eswierk@gh.st>; Martin Blumenstingl
> <martin.blumenstingl@googlemail.com>
> Subject: [PATCH v3 0/8] rtw88: prepare locking for SDIO support
> 

[...]

I do stressed test of connection and suspend, and it get stuck after about
4 hours but no useful messages. I will re-build my kernel and turn on lockdep debug
to see if it can tell me what is wrong.

--
Ping-Ke

RE: [PATCH v3 0/8] rtw88: prepare locking for SDIO support

[Pkshih]

[-- Attachment #1: Type: text/plain, Size: 2284 bytes --]


> -----Original Message-----
> From: Pkshih
> Sent: Wednesday, January 19, 2022 5:38 PM
> To: 'Martin Blumenstingl' <martin.blumenstingl@googlemail.com>; linux-wireless@vger.kernel.org
> Cc: tony0620emma@gmail.com; kvalo@codeaurora.org; johannes@sipsolutions.net; netdev@vger.kernel.org;
> linux-kernel@vger.kernel.org; Neo Jou <neojou@gmail.com>; Jernej Skrabec <jernej.skrabec@gmail.com>; Ed
> Swierk <eswierk@gh.st>
> Subject: RE: [PATCH v3 0/8] rtw88: prepare locking for SDIO support
> 
> Hi,
> 
> > -----Original Message-----
> > From: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
> > Sent: Saturday, January 8, 2022 8:55 AM
> > To: linux-wireless@vger.kernel.org
> > Cc: tony0620emma@gmail.com; kvalo@codeaurora.org; johannes@sipsolutions.net; netdev@vger.kernel.org;
> > linux-kernel@vger.kernel.org; Neo Jou <neojou@gmail.com>; Jernej Skrabec <jernej.skrabec@gmail.com>;
> > Pkshih <pkshih@realtek.com>; Ed Swierk <eswierk@gh.st>; Martin Blumenstingl
> > <martin.blumenstingl@googlemail.com>
> > Subject: [PATCH v3 0/8] rtw88: prepare locking for SDIO support
> >
> 
> [...]
> 
> I do stressed test of connection and suspend, and it get stuck after about
> 4 hours but no useful messages. I will re-build my kernel and turn on lockdep debug
> to see if it can tell me what is wrong.
> 

I found some deadlock: 

[ 4891.169653]        CPU0                    CPU1
[ 4891.169732]        ----                    ----
[ 4891.169799]   lock(&rtwdev->mutex);
[ 4891.169874]                                lock(&local->sta_mtx);
[ 4891.169948]                                lock(&rtwdev->mutex);
[ 4891.170050]   lock(&local->sta_mtx);


[ 4919.598630]        CPU0                    CPU1
[ 4919.598715]        ----                    ----
[ 4919.598779]   lock(&local->iflist_mtx);
[ 4919.598900]                                lock(&rtwdev->mutex);
[ 4919.598995]                                lock(&local->iflist_mtx);
[ 4919.599092]   lock(&rtwdev->mutex);

So, I add wrappers to iterate rtw_iterate_stas() and rtw_iterate_vifs() that
use _atomic version to collect sta and vif, and use list_for_each() to iterate.
Reference code is attached, and I'm still thinking if we can have better method.

--
Ping-Ke


[-- Attachment #2: 0001-rtw88-use-atomic-to-collect-stas-and-does-iterators.patch --]
[-- Type: application/octet-stream, Size: 4314 bytes --]

From c9539ea5fbbd692030381a42ad31e08490f5b804 Mon Sep 17 00:00:00 2001
From: Ping-Ke Shih <pkshih@realtek.com>
Date: Fri, 21 Jan 2022 11:09:45 +0800
Subject: [PATCH] rtw88: use atomic to collect stas and does iterators

Change-Id: I7665268d0ca859d4e3c3a60b1f15b76f5444f0ab
Signed-off-by: Ping-Ke Shih <pkshih@realtek.com>
---
 util.c | 92 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 util.h | 13 +++++----
 2 files changed, 100 insertions(+), 5 deletions(-)

diff --git a/util.c b/util.c
index 2c515af2..1835968f 100644
--- a/util.c
+++ b/util.c
@@ -105,3 +105,95 @@ void rtw_desc_to_mcsrate(u16 rate, u8 *mcs, u8 *nss)
 		*mcs = rate - DESC_RATEMCS0;
 	}
 }
+
+struct rtw_stas_entry {
+	struct list_head list;
+	struct ieee80211_sta *sta;
+};
+
+struct rtw_iter_stas_data {
+	struct rtw_dev *rtwdev;
+	struct list_head list;
+};
+
+void rtw_collect_sta_iter(void *data, struct ieee80211_sta *sta)
+{
+	struct rtw_iter_stas_data *iter_stas = (struct rtw_iter_stas_data *)data;
+	struct rtw_stas_entry *stas_entry;
+
+	stas_entry = kmalloc(sizeof(*stas_entry), GFP_ATOMIC);
+	if (!stas_entry)
+		return;
+
+	stas_entry->sta = sta;
+	list_add_tail(&stas_entry->list, &iter_stas->list);
+}
+
+void rtw_iterate_stas(struct rtw_dev *rtwdev,
+		      void (*iterator)(void *data,
+				       struct ieee80211_sta *sta),
+				       void *data)
+{
+	struct rtw_iter_stas_data iter_data;
+	struct rtw_stas_entry *sta_entry, *tmp;
+
+	iter_data.rtwdev = rtwdev;
+	INIT_LIST_HEAD(&iter_data.list);
+
+	ieee80211_iterate_stations_atomic(rtwdev->hw, rtw_collect_sta_iter,
+					  &iter_data);
+
+	list_for_each_entry_safe(sta_entry, tmp, &iter_data.list,
+				 list) {
+		list_del_init(&sta_entry->list);
+		iterator(data, sta_entry->sta);
+		kfree(sta_entry);
+	}
+}
+
+struct rtw_vifs_entry {
+	struct list_head list;
+	struct ieee80211_vif *vif;
+	u8 mac[ETH_ALEN];
+};
+
+struct rtw_iter_vifs_data {
+	struct rtw_dev *rtwdev;
+	struct list_head list;
+};
+
+void rtw_collect_vif_iter(void *data, u8 *mac, struct ieee80211_vif *vif)
+{
+	struct rtw_iter_vifs_data *iter_stas = (struct rtw_iter_vifs_data *)data;
+	struct rtw_vifs_entry *vifs_entry;
+
+	vifs_entry = kmalloc(sizeof(*vifs_entry), GFP_ATOMIC);
+	if (!vifs_entry)
+		return;
+
+	vifs_entry->vif = vif;
+	ether_addr_copy(vifs_entry->mac, mac);
+	list_add_tail(&vifs_entry->list, &iter_stas->list);
+}
+
+void rtw_iterate_vifs(struct rtw_dev *rtwdev,
+		      void (*iterator)(void *data, u8 *mac,
+				       struct ieee80211_vif *vif),
+		      void *data)
+{
+	struct rtw_iter_vifs_data iter_data;
+	struct rtw_vifs_entry *vif_entry, *tmp;
+
+	iter_data.rtwdev = rtwdev;
+	INIT_LIST_HEAD(&iter_data.list);
+
+	ieee80211_iterate_active_interfaces_atomic(rtwdev->hw,
+			IEEE80211_IFACE_ITER_NORMAL, rtw_collect_vif_iter, &iter_data);
+
+	list_for_each_entry_safe(vif_entry, tmp, &iter_data.list,
+				 list) {
+		list_del_init(&vif_entry->list);
+		iterator(data, vif_entry->mac, vif_entry->vif);
+		kfree(vif_entry);
+	}
+}
diff --git a/util.h b/util.h
index 06a5b4c4..e4995dba 100644
--- a/util.h
+++ b/util.h
@@ -7,18 +7,21 @@
 
 struct rtw_dev;
 
-#define rtw_iterate_vifs(rtwdev, iterator, data)                               \
-	ieee80211_iterate_active_interfaces(rtwdev->hw,                        \
-			IEEE80211_IFACE_ITER_NORMAL, iterator, data)
 #define rtw_iterate_vifs_atomic(rtwdev, iterator, data)                        \
 	ieee80211_iterate_active_interfaces_atomic(rtwdev->hw,                 \
 			IEEE80211_IFACE_ITER_NORMAL, iterator, data)
-#define rtw_iterate_stas(rtwdev, iterator, data)                        \
-	ieee80211_iterate_stations(rtwdev->hw, iterator, data)
 #define rtw_iterate_stas_atomic(rtwdev, iterator, data)                        \
 	ieee80211_iterate_stations_atomic(rtwdev->hw, iterator, data)
 #define rtw_iterate_keys(rtwdev, vif, iterator, data)			       \
 	ieee80211_iter_keys(rtwdev->hw, vif, iterator, data)
+void rtw_iterate_vifs(struct rtw_dev *rtwdev,
+		      void (*iterator)(void *data, u8 *mac,
+				       struct ieee80211_vif *vif),
+		      void *data);
+void rtw_iterate_stas(struct rtw_dev *rtwdev,
+		      void (*iterator)(void *data,
+				       struct ieee80211_sta *sta),
+				       void *data);
 
 static inline u8 *get_hdr_bssid(struct ieee80211_hdr *hdr)
 {
-- 
2.25.1

Re: [PATCH v3 0/8] rtw88: prepare locking for SDIO support

[Martin Blumenstingl]

Hi Ping-Ke,

On Fri, Jan 21, 2022 at 9:10 AM Pkshih <pkshih@realtek.com> wrote:
[...]
> >
> > I do stressed test of connection and suspend, and it get stuck after about
> > 4 hours but no useful messages. I will re-build my kernel and turn on lockdep debug
> > to see if it can tell me what is wrong.
First of all: thank you so much for testing this and investigating the deadlock!

> I found some deadlock:
>
> [ 4891.169653]        CPU0                    CPU1
> [ 4891.169732]        ----                    ----
> [ 4891.169799]   lock(&rtwdev->mutex);
> [ 4891.169874]                                lock(&local->sta_mtx);
> [ 4891.169948]                                lock(&rtwdev->mutex);
> [ 4891.170050]   lock(&local->sta_mtx);
>
>
> [ 4919.598630]        CPU0                    CPU1
> [ 4919.598715]        ----                    ----
> [ 4919.598779]   lock(&local->iflist_mtx);
> [ 4919.598900]                                lock(&rtwdev->mutex);
> [ 4919.598995]                                lock(&local->iflist_mtx);
> [ 4919.599092]   lock(&rtwdev->mutex);
This looks similar to the problem fixed by 5b0efb4d670c8b ("rtw88:
avoid circular locking between local->iflist_mtx and rtwdev->mutex")
which you have pointed out earlier.
It seems to me that we should avoid using the mutex version of
ieee80211_iterate_*() because it can lead to more of these issues. So
from my point of view the general idea of the code from your attached
patch looks good. That said, I'm still very new to mac80211/cfg80211
so I'm also interested in other's opinions.

> So, I add wrappers to iterate rtw_iterate_stas() and rtw_iterate_vifs() that
> use _atomic version to collect sta and vif, and use list_for_each() to iterate.
> Reference code is attached, and I'm still thinking if we can have better method.
With "better method" do you mean something like in patch #2 from this
series (using unsigned int num_si and struct rtw_sta_info
*si[RTW_MAX_MAC_ID_NUM] inside the iter_data) are you thinking of a
better way in general?


Best regards,
Martin

RE: [PATCH v3 0/8] rtw88: prepare locking for SDIO support

[Pkshih]

Hi, 

> -----Original Message-----
> From: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
> Sent: Monday, January 24, 2022 3:04 AM
> To: Pkshih <pkshih@realtek.com>
> Cc: linux-wireless@vger.kernel.org; tony0620emma@gmail.com; kvalo@codeaurora.org;
> johannes@sipsolutions.net; netdev@vger.kernel.org; linux-kernel@vger.kernel.org; Neo Jou
> <neojou@gmail.com>; Jernej Skrabec <jernej.skrabec@gmail.com>; Ed Swierk <eswierk@gh.st>
> Subject: Re: [PATCH v3 0/8] rtw88: prepare locking for SDIO support
> 
> Hi Ping-Ke,
> 
> On Fri, Jan 21, 2022 at 9:10 AM Pkshih <pkshih@realtek.com> wrote:
> [...]
> > >
> > > I do stressed test of connection and suspend, and it get stuck after about
> > > 4 hours but no useful messages. I will re-build my kernel and turn on lockdep debug
> > > to see if it can tell me what is wrong.
> First of all: thank you so much for testing this and investigating the deadlock!
> 
> > I found some deadlock:
> >
> > [ 4891.169653]        CPU0                    CPU1
> > [ 4891.169732]        ----                    ----
> > [ 4891.169799]   lock(&rtwdev->mutex);
> > [ 4891.169874]                                lock(&local->sta_mtx);
> > [ 4891.169948]                                lock(&rtwdev->mutex);
> > [ 4891.170050]   lock(&local->sta_mtx);
> >
> >
> > [ 4919.598630]        CPU0                    CPU1
> > [ 4919.598715]        ----                    ----
> > [ 4919.598779]   lock(&local->iflist_mtx);
> > [ 4919.598900]                                lock(&rtwdev->mutex);
> > [ 4919.598995]                                lock(&local->iflist_mtx);
> > [ 4919.599092]   lock(&rtwdev->mutex);
> This looks similar to the problem fixed by 5b0efb4d670c8b ("rtw88:
> avoid circular locking between local->iflist_mtx and rtwdev->mutex")
> which you have pointed out earlier.
> It seems to me that we should avoid using the mutex version of
> ieee80211_iterate_*() because it can lead to more of these issues. So
> from my point of view the general idea of the code from your attached
> patch looks good. That said, I'm still very new to mac80211/cfg80211
> so I'm also interested in other's opinions.
> 

The attached patch can work "mostly", because both callers of iterate() and
::remove_interface hold rtwdev->mutex. Theoretically, the exception is a caller
forks another work to iterate() between leaving ::remove_interface and mac80211
doesn't yet free the vif, but the work executes after mac80211 free the vif.
This will lead use-after-free, but I'm not sure if this scenario will happen.
I need time to dig this, or you can help to do this.

To avoid this, we can add a flag to struct rtw_vif, and set this flag
when ::remove_interface. Then, only collect vif without this flag into list
when we use iterate_actiom().

As well as ieee80211_sta can do similar fix.

> > So, I add wrappers to iterate rtw_iterate_stas() and rtw_iterate_vifs() that
> > use _atomic version to collect sta and vif, and use list_for_each() to iterate.
> > Reference code is attached, and I'm still thinking if we can have better method.
> With "better method" do you mean something like in patch #2 from this
> series (using unsigned int num_si and struct rtw_sta_info
> *si[RTW_MAX_MAC_ID_NUM] inside the iter_data) are you thinking of a
> better way in general?
> 

I would like a straight method, for example, we can have another version of
ieee80211_iterate_xxx() and do things in iterator, like original, so we just
need to change the code slightly.

Initially, I have an idea we can hold driver lock, like rtwdev->mutex, in both
places where we use ieee80211_iterate_() and remove sta or vif. Hopefully,
this can ensure it's safe to run iterator without other locks. Then, we can
define another ieee80211_iterate_() version with a drv_lock argument, like

#define ieee80211_iterate_active_interfaces_drv_lock(hw, iter_flags, iterator, data, drv_lock) \
while (0) {	\
	lockdep_assert_wiphy(drv_lock); \
	ieee80211_iterate_active_interfaces_no_lock(hw, iter_flags, iterator, data); \
}

The driv_lock argument can avoid user forgetting to hold a lock, and we need
a helper of no_lock version:

void ieee80211_iterate_active_interfaces_no_lock(
	struct ieee80211_hw *hw, u32 iter_flags,
	void (*iterator)(void *data, u8 *mac,
			 struct ieee80211_vif *vif),
	void *data)
{
	struct ieee80211_local *local = hw_to_local(hw);

	__iterate_interfaces(local, iter_flags | IEEE80211_IFACE_ITER_ACTIVE,
			     iterator, data);
}

However, as I mentioned theoretically it is not safe entirely.

So, I think the easiest way is to maintains the vif/sta lists in driver when
::{add,remove }_interface/::sta_{add,remove}, and hold rtwdev->mutex lock to
access these lists. But, Johannes pointed out this is not a good idea [1].

[1] https://lore.kernel.org/linux-wireless/d61f3947cddec660cbb2a59e2424d2bd8c01346a.camel@sipsolutions.net/
--
Ping-Ke

Re: [PATCH v3 0/8] rtw88: prepare locking for SDIO support

[Martin Blumenstingl]

Hi Ping-Ke,

On Mon, Jan 24, 2022 at 3:59 AM Pkshih <pkshih@realtek.com> wrote:
[...]
> > It seems to me that we should avoid using the mutex version of
> > ieee80211_iterate_*() because it can lead to more of these issues. So
> > from my point of view the general idea of the code from your attached
> > patch looks good. That said, I'm still very new to mac80211/cfg80211
> > so I'm also interested in other's opinions.
> >
>
> The attached patch can work "mostly", because both callers of iterate() and
> ::remove_interface hold rtwdev->mutex. Theoretically, the exception is a caller
> forks another work to iterate() between leaving ::remove_interface and mac80211
> doesn't yet free the vif, but the work executes after mac80211 free the vif.
> This will lead use-after-free, but I'm not sure if this scenario will happen.
> I need time to dig this, or you can help to do this.
>
> To avoid this, we can add a flag to struct rtw_vif, and set this flag
> when ::remove_interface. Then, only collect vif without this flag into list
> when we use iterate_actiom().
>
> As well as ieee80211_sta can do similar fix.
>
> > > So, I add wrappers to iterate rtw_iterate_stas() and rtw_iterate_vifs() that
> > > use _atomic version to collect sta and vif, and use list_for_each() to iterate.
> > > Reference code is attached, and I'm still thinking if we can have better method.
> > With "better method" do you mean something like in patch #2 from this
> > series (using unsigned int num_si and struct rtw_sta_info
> > *si[RTW_MAX_MAC_ID_NUM] inside the iter_data) are you thinking of a
> > better way in general?
> >
>
> I would like a straight method, for example, we can have another version of
> ieee80211_iterate_xxx() and do things in iterator, like original, so we just
> need to change the code slightly.
>
> Initially, I have an idea we can hold driver lock, like rtwdev->mutex, in both
> places where we use ieee80211_iterate_() and remove sta or vif. Hopefully,
> this can ensure it's safe to run iterator without other locks. Then, we can
> define another ieee80211_iterate_() version with a drv_lock argument, like
>
> #define ieee80211_iterate_active_interfaces_drv_lock(hw, iter_flags, iterator, data, drv_lock) \
> while (0) {     \
>         lockdep_assert_wiphy(drv_lock); \
>         ieee80211_iterate_active_interfaces_no_lock(hw, iter_flags, iterator, data); \
> }
>
> The driv_lock argument can avoid user forgetting to hold a lock, and we need
> a helper of no_lock version:
>
> void ieee80211_iterate_active_interfaces_no_lock(
>         struct ieee80211_hw *hw, u32 iter_flags,
>         void (*iterator)(void *data, u8 *mac,
>                          struct ieee80211_vif *vif),
>         void *data)
> {
>         struct ieee80211_local *local = hw_to_local(hw);
>
>         __iterate_interfaces(local, iter_flags | IEEE80211_IFACE_ITER_ACTIVE,
>                              iterator, data);
> }
>
> However, as I mentioned theoretically it is not safe entirely.
>
> So, I think the easiest way is to maintains the vif/sta lists in driver when
> ::{add,remove }_interface/::sta_{add,remove}, and hold rtwdev->mutex lock to
> access these lists. But, Johannes pointed out this is not a good idea [1].
Thank you for this detailed explanation! I appreciate that you took
the time to clearly explain this.

For the sta use-case I thought about adding a dedicated rwlock
(include/linux/rwlock.h) for rtw_dev->mac_id_map.
rtw_sta_{add,remove} would take a write-lock.
rtw_iterate_stas() takes the read-lock (the lock would be acquired
before calling into ieee80211_iterate_...). Additionally
rtw_iterate_stas() needs to check if the station is still valid
according to mac_id_map - if not: skip/ignore it for that iteration.
This could be combined with your
0001-rtw88-use-atomic-to-collect-stas-and-does-iterators.patch.

For the interface use-case it's not clear to me how this works at all.
rtw_ops_add_interface() has (in a simplified view):
    u8 port = 0;
    // the port variable is never changed
    rtwvif->port = port;
    rtwvif->conf = &rtw_vif_port[port];
    rtw_info(rtwdev, "start vif %pM on port %d\n", vif->addr, rtwvif->port);
How do multiple interfaces (vifs) work in rtw88 if the port is always
zero? Is some kind of tracking of the used ports missing (similar to
how we track the used station IDs - also called mac_id - in
rtw_dev->mac_id_map)?


Thank you again and best regards,
Martin

RE: [PATCH v3 0/8] rtw88: prepare locking for SDIO support

[Pkshih]

Hi,

> -----Original Message-----
> From: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
> Sent: Friday, January 28, 2022 5:53 AM
> To: Pkshih <pkshih@realtek.com>
> Cc: linux-wireless@vger.kernel.org; tony0620emma@gmail.com; kvalo@codeaurora.org;
> johannes@sipsolutions.net; netdev@vger.kernel.org; linux-kernel@vger.kernel.org; Neo Jou
> <neojou@gmail.com>; Jernej Skrabec <jernej.skrabec@gmail.com>; Ed Swierk <eswierk@gh.st>
> Subject: Re: [PATCH v3 0/8] rtw88: prepare locking for SDIO support
> 
> Hi Ping-Ke,
> 
> On Mon, Jan 24, 2022 at 3:59 AM Pkshih <pkshih@realtek.com> wrote:

[...]

> >
> > To avoid this, we can add a flag to struct rtw_vif, and set this flag
> > when ::remove_interface. Then, only collect vif without this flag into list
> > when we use iterate_actiom().
> >
> > As well as ieee80211_sta can do similar fix.
> >

I would prefer my method that adds a 'bool disabled' flag to struct rtw_vif/rtw_sta
and set it when ::remove_interface/::sta_remove. Then rtw_iterate_stas() can
check this flag to decide whether does thing or not.

[...]

> 
> For the sta use-case I thought about adding a dedicated rwlock
> (include/linux/rwlock.h) for rtw_dev->mac_id_map.
> rtw_sta_{add,remove} would take a write-lock.
> rtw_iterate_stas() takes the read-lock (the lock would be acquired
> before calling into ieee80211_iterate_...). Additionally
> rtw_iterate_stas() needs to check if the station is still valid
> according to mac_id_map - if not: skip/ignore it for that iteration.
> This could be combined with your
> 0001-rtw88-use-atomic-to-collect-stas-and-does-iterators.patch.

Using a 'disabled' flag within rtw_vif/rtw_sta will be intuitive and
better than bitmap of mac_id_map. Please reference my mention above.

> 
> For the interface use-case it's not clear to me how this works at all.
> rtw_ops_add_interface() has (in a simplified view):
>     u8 port = 0;
>     // the port variable is never changed
>     rtwvif->port = port;
>     rtwvif->conf = &rtw_vif_port[port];
>     rtw_info(rtwdev, "start vif %pM on port %d\n", vif->addr, rtwvif->port);
> How do multiple interfaces (vifs) work in rtw88 if the port is always
> zero? Is some kind of tracking of the used ports missing (similar to
> how we track the used station IDs - also called mac_id - in
> rtw_dev->mac_id_map)?

The port should be allocated dynamically if we support two or more vifs.
We have internal tree that is going to support p2p by second vif.


Ping-Ke

Re: [PATCH v3 0/8] rtw88: prepare locking for SDIO support

[Martin Blumenstingl]

Hi Ping-Ke,

On Fri, Jan 28, 2022 at 1:51 AM Pkshih <pkshih@realtek.com> wrote:
[...]
>
> > >
> > > To avoid this, we can add a flag to struct rtw_vif, and set this flag
> > > when ::remove_interface. Then, only collect vif without this flag into list
> > > when we use iterate_actiom().
> > >
> > > As well as ieee80211_sta can do similar fix.
> > >
>
> I would prefer my method that adds a 'bool disabled' flag to struct rtw_vif/rtw_sta
> and set it when ::remove_interface/::sta_remove. Then rtw_iterate_stas() can
> check this flag to decide whether does thing or not.
That would indeed be a very straight forward approach and easy to read.
In net/mac80211/iface.c there's some cases where after
drv_remove_interface() (which internally calls our .remove_interface
op) will kfree the vif (sdata). Doesn't that then result in a
use-after-free if we rely on a boolean within rtw_vif?

[...]
> > For the interface use-case it's not clear to me how this works at all.
> > rtw_ops_add_interface() has (in a simplified view):
> >     u8 port = 0;
> >     // the port variable is never changed
> >     rtwvif->port = port;
> >     rtwvif->conf = &rtw_vif_port[port];
> >     rtw_info(rtwdev, "start vif %pM on port %d\n", vif->addr, rtwvif->port);
> > How do multiple interfaces (vifs) work in rtw88 if the port is always
> > zero? Is some kind of tracking of the used ports missing (similar to
> > how we track the used station IDs - also called mac_id - in
> > rtw_dev->mac_id_map)?
>
> The port should be allocated dynamically if we support two or more vifs.
> We have internal tree that is going to support p2p by second vif.
I see, thanks for clarifying this!


Best regards,
Martin

Re: [PATCH v3 0/8] rtw88: prepare locking for SDIO support

[Pkshih]

Hi,

On Sun, 2022-01-30 at 22:40 +0100, Martin Blumenstingl wrote:
> 
> On Fri, Jan 28, 2022 at 1:51 AM Pkshih <pkshih@realtek.com> wrote:
> [...]
> > > > To avoid this, we can add a flag to struct rtw_vif, and set this flag
> > > > when ::remove_interface. Then, only collect vif without this flag into list
> > > > when we use iterate_actiom().
> > > > 
> > > > As well as ieee80211_sta can do similar fix.
> > > > 
> > 
> > I would prefer my method that adds a 'bool disabled' flag to struct rtw_vif/rtw_sta
> > and set it when ::remove_interface/::sta_remove. Then rtw_iterate_stas() can
> > check this flag to decide whether does thing or not.
> That would indeed be a very straight forward approach and easy to read.
> In net/mac80211/iface.c there's some cases where after
> drv_remove_interface() (which internally calls our .remove_interface
> op) will kfree the vif (sdata). Doesn't that then result in a
> use-after-free if we rely on a boolean within rtw_vif?

The rtw_vif is drv_priv of ieee80211_vif, and they will be freed at
the same time. We must set 'bool disabled' after holding rtwdev->mutex
lock, and check this flag in iterator of ieee80211_iterate_active_interfaces_atomic()
to contruct a list of vif.

That means we never access this flag out of rtwdev->mutx or iterator.
Does it make sense?

--
Ping-Ke

Re: [PATCH v3 0/8] rtw88: prepare locking for SDIO support

[Johannes Berg]

Hi,

Sorry, I kind of saw this fly by, read over it and wasn't sure I should
take a closer look, and then promptly forgot about it ...

> > So, I think the easiest way is to maintains the vif/sta lists in driver when
> > ::{add,remove }_interface/::sta_{add,remove}, and hold rtwdev->mutex lock to
> > access these lists. But, Johannes pointed out this is not a good idea [1].

> Thank you for this detailed explanation! I appreciate that you took
> the time to clearly explain this.
> 
> For the sta use-case I thought about adding a dedicated rwlock
> (include/linux/rwlock.h) for rtw_dev->mac_id_map.

You can't sleep under an rwlock either though? Wasn't that the point?

> rtw_sta_{add,remove} would take a write-lock.
> rtw_iterate_stas() takes the read-lock (the lock would be acquired
> before calling into ieee80211_iterate_...). Additionally
> rtw_iterate_stas() needs to check if the station is still valid
> according to mac_id_map - if not: skip/ignore it for that iteration.

All of that "still valid" seems fragile though, IMHO. Hard to reason
that it's not racy or prone to use-after-free situations.

IIUC, you're trying to iterate interfaces and stations in a sleepable
context, but you're worried about deadlocks with locking in mac80211, if
ieee80211_iterate_interfaces() or ieee80211_iterate_stations() take
locks that we already hold due to coming into the code from mac80211? Or
about ABBA deadlocks arising from this?

IMHO you should still do it, and just be careful about the locking. I'd
be happy to add APIs for e.g. ieee80211_iterate_stations_locked() when
you know you already hold local->sta_mtx, though whether or not that can
even happen today in any callbacks I don't know, haven't audited it, but
it shouldn't be that hard to audit the path(s) you want to create?
Unless you need this in some very frequently called function ...


Now all of this is potentially also (and just as) error prone as doing
your own iteration machinery, however, my longer-term plan is to unify
the locking more. Now that we're no longer relying on the RTNL so much,
I'm planning to see if we couldn't get rid of most locks in mac80211.
The thing is, that most of the time we're doing all this fine-grained
locking ... under the wdev->mtx, so it's entirely pointless, and in fact
just a bunch of extra work.

So now that from cfg80211 perspective we have everything running under
wdev lock, I think we could in mac80211 just remove all the locks and
take the wdev lock in all the async workers. We already do in many I
guess, or we take local->mtx which is kind of equivalent anyway.

The question is then if we keep wdev->mtx at all, and I'm not really
sure about that yet. There are arguments both ways, and maybe that means
we'd move some things under there rather than under the overall mutex.
One of the strongest arguments for this is probably that mac80211
doesn't really do anything expensive (there aren't really any expensive
calculations in it), so most time spent is in drivers ... and guess
what, drivers mostly just have their own global lock they take for
everything, which makes sense since they need to talk to the device or
firmware.

However we answer that question though, and I'm trending towards
removing the wdev/sdata locks, I (now) think all the mutexes that we
have at the 'local' level in mac80211 are fairly much pointless.

In essence then, with some work in the stack, we could basically have
all calls (apart from TX) into the drivers done with the wiphy->mtx
held, at which point drivers don't even really need their own lock (they
can acquire wiphy_lock() too in workers). Then this whole thing really
all collapses down to being able to do the iteration, just having to
ensure you wiphy_lock() your calls that aren't coming from mac80211 in
the first place.

I really think that's the medium-term strategy, and any help would be
welcome. I'm not sure I can dedicate time to this soon, and the RTNL
rework took I think over a year after I started thinking about it, so I
guess we'll see...

We can do this incrementally btw, like remove local->mtx now and use
wiphy_lock() in its place, then remove local->sta_mtx, local->iface_mtx,
one by one. The RTNL rework was one of the major stepping stones to this
I think, because it ensured a consistent handling of the wiphy_lock() in
cfg80211.

I realize this doesn't help you in the short term, but if you do a lot
of complicated things now, it'll also be hard to get rid of. If you use
the normal iteration functions now it'll be harder to reason about now,
but will sort of automatically become easier once the lock redux work I
outlined above starts. And like I said, any help welcome :)

johannes