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/[email protected]/T/
[3] https://lore.kernel.org/netdev/[email protected]/
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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
Hi,
> -----Original Message-----
> From: Martin Blumenstingl <[email protected]>
> Sent: Saturday, January 8, 2022 8:55 AM
> To: [email protected]
> Cc: [email protected]; [email protected]; [email protected]; [email protected];
> [email protected]; Neo Jou <[email protected]>; Jernej Skrabec <[email protected]>;
> Pkshih <[email protected]>; Ed Swierk <[email protected]>; Martin Blumenstingl
> <[email protected]>
> 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
> -----Original Message-----
> From: Pkshih
> Sent: Wednesday, January 19, 2022 5:38 PM
> To: 'Martin Blumenstingl' <[email protected]>; [email protected]
> Cc: [email protected]; [email protected]; [email protected]; [email protected];
> [email protected]; Neo Jou <[email protected]>; Jernej Skrabec <[email protected]>; Ed
> Swierk <[email protected]>
> Subject: RE: [PATCH v3 0/8] rtw88: prepare locking for SDIO support
>
> Hi,
>
> > -----Original Message-----
> > From: Martin Blumenstingl <[email protected]>
> > Sent: Saturday, January 8, 2022 8:55 AM
> > To: [email protected]
> > Cc: [email protected]; [email protected]; [email protected]; [email protected];
> > [email protected]; Neo Jou <[email protected]>; Jernej Skrabec <[email protected]>;
> > Pkshih <[email protected]>; Ed Swierk <[email protected]>; Martin Blumenstingl
> > <[email protected]>
> > 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
Hi Ping-Ke,
On Mon, Jan 24, 2022 at 3:59 AM Pkshih <[email protected]> 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
Hi,
> -----Original Message-----
> From: Martin Blumenstingl <[email protected]>
> Sent: Friday, January 28, 2022 5:53 AM
> To: Pkshih <[email protected]>
> Cc: [email protected]; [email protected]; [email protected];
> [email protected]; [email protected]; [email protected]; Neo Jou
> <[email protected]>; Jernej Skrabec <[email protected]>; Ed Swierk <[email protected]>
> 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 <[email protected]> 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
Hi Ping-Ke,
On Fri, Jan 28, 2022 at 1:51 AM Pkshih <[email protected]> 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
Hi,
On Sun, 2022-01-30 at 22:40 +0100, Martin Blumenstingl wrote:
>
> On Fri, Jan 28, 2022 at 1:51 AM Pkshih <[email protected]> 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