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 (though it's not
fast yet, in my tests I got ~6Mbit/s in either direction).
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 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-sdio-locking-prep-linux-next-20211226
[1] https://lore.kernel.org/linux-wireless/CAFBinCDMPPJ7qW7xTkep1Trg+zP0B9Jxei6sgjqmF4NDA1JAhQ@mail.gmail.com/
[2] https://lore.kernel.org/netdev/[email protected]/T/
Martin Blumenstingl (9):
mac80211: Add stations iterator where the iterator function may sleep
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 | 14 +++++-
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 +-
include/net/mac80211.h | 21 +++++++++
net/mac80211/util.c | 13 +++++
12 files changed, 94 insertions(+), 55 deletions(-)
--
2.34.1
ieee80211_iterate_active_interfaces() and
ieee80211_iterate_active_interfaces_atomic() already exist, where the
former allows the iterator function to sleep. Add
ieee80211_iterate_stations() which is similar to
ieee80211_iterate_stations_atomic() but allows the iterator to sleep.
This is needed for adding SDIO support to the rtw88 driver. Some
interators there are reading or writing registers. With the SDIO ops
(sdio_readb, sdio_writeb and friends) this means that the iterator
function may sleep.
Signed-off-by: Martin Blumenstingl <[email protected]>
---
v1 -> v2:
- fixed kernel doc copy & paste (remove _atomic) as suggested by Ping-Ke
and Johannes
- added paragraph about driver authors having to be careful where they
use this new function as suggested by Johannes
include/net/mac80211.h | 21 +++++++++++++++++++++
net/mac80211/util.c | 13 +++++++++++++
2 files changed, 34 insertions(+)
diff --git a/include/net/mac80211.h b/include/net/mac80211.h
index 8907338d52b5..c50221d7e82c 100644
--- a/include/net/mac80211.h
+++ b/include/net/mac80211.h
@@ -5614,6 +5614,9 @@ void ieee80211_iterate_active_interfaces_atomic(struct ieee80211_hw *hw,
* This function iterates over the interfaces associated with a given
* hardware that are currently active and calls the callback for them.
* This version can only be used while holding the wiphy mutex.
+ * The driver must not call this with a lock held that it can also take in
+ * response to callbacks from mac80211, and it must not call this within
+ * callbacks made by mac80211 - both would result in deadlocks.
*
* @hw: the hardware struct of which the interfaces should be iterated over
* @iter_flags: iteration flags, see &enum ieee80211_interface_iteration_flags
@@ -5627,6 +5630,24 @@ void ieee80211_iterate_active_interfaces_mtx(struct ieee80211_hw *hw,
struct ieee80211_vif *vif),
void *data);
+/**
+ * ieee80211_iterate_stations - iterate stations
+ *
+ * This function iterates over all stations associated with a given
+ * hardware that are currently uploaded to the driver and calls the callback
+ * function for them.
+ * This function allows the iterator function to sleep, when the iterator
+ * function is atomic @ieee80211_iterate_stations_atomic can be used.
+ *
+ * @hw: the hardware struct of which the interfaces should be iterated over
+ * @iterator: the iterator function to call, cannot sleep
+ * @data: first argument of the iterator function
+ */
+void ieee80211_iterate_stations(struct ieee80211_hw *hw,
+ void (*iterator)(void *data,
+ struct ieee80211_sta *sta),
+ void *data);
+
/**
* ieee80211_iterate_stations_atomic - iterate stations
*
diff --git a/net/mac80211/util.c b/net/mac80211/util.c
index 0e4e1956bcea..f71b042a5c8b 100644
--- a/net/mac80211/util.c
+++ b/net/mac80211/util.c
@@ -862,6 +862,19 @@ static void __iterate_stations(struct ieee80211_local *local,
}
}
+void ieee80211_iterate_stations(struct ieee80211_hw *hw,
+ void (*iterator)(void *data,
+ struct ieee80211_sta *sta),
+ void *data)
+{
+ struct ieee80211_local *local = hw_to_local(hw);
+
+ mutex_lock(&local->sta_mtx);
+ __iterate_stations(local, iterator, data);
+ mutex_unlock(&local->sta_mtx);
+}
+EXPORT_SYMBOL_GPL(ieee80211_iterate_stations);
+
void ieee80211_iterate_stations_atomic(struct ieee80211_hw *hw,
void (*iterator)(void *data,
struct ieee80211_sta *sta),
--
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]>
---
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]>
---
v1 -> v2:
- this patch is new in 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. 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
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
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. 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. 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
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]>
---
v1 -> v2:
- this patch is new in 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 | 14 +++++++++++++-
1 file changed, 13 insertions(+), 1 deletion(-)
diff --git a/drivers/net/wireless/realtek/rtw88/mac80211.c b/drivers/net/wireless/realtek/rtw88/mac80211.c
index ae7d97de5fdf..3bd12354a8a1 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,20 @@ 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;
+
+ mutex_lock(&rtwdev->mutex);
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]);
+
+ mutex_unlock(&rtwdev->mutex);
}
static int rtw_ops_set_bitrate_mask(struct ieee80211_hw *hw,
--
2.34.1
> -----Original Message-----
> From: Martin Blumenstingl <[email protected]>
> Sent: Wednesday, December 29, 2021 5:15 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]>; Martin Blumenstingl <[email protected]>
> Subject: [PATCH 3/9] rtw88: Move rtw_update_sta_info() out of rtw_ra_mask_info_update_iter()
>
> 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]>
> ---
> v1 -> v2:
> - this patch is new in 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 | 14 +++++++++++++-
> 1 file changed, 13 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/net/wireless/realtek/rtw88/mac80211.c
> b/drivers/net/wireless/realtek/rtw88/mac80211.c
> index ae7d97de5fdf..3bd12354a8a1 100644
> --- a/drivers/net/wireless/realtek/rtw88/mac80211.c
> +++ b/drivers/net/wireless/realtek/rtw88/mac80211.c
[...]
> @@ -699,11 +702,20 @@ 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;
> +
> + mutex_lock(&rtwdev->mutex);
I think this lock is used to protect br_data.si[i], right?
And, I prefer to move mutex lock to caller, like:
@@ -734,7 +734,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;
}
>
> 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]);
> +
> + mutex_unlock(&rtwdev->mutex);
> }
>
--
Ping-Ke
> -----Original Message-----
> From: Martin Blumenstingl <[email protected]>
> Sent: Wednesday, December 29, 2021 5:15 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]>; Martin Blumenstingl <[email protected]>
> Subject: [PATCH 0/9] rtw88: prepare locking for SDIO support
>
> 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 (though it's not
> fast yet, in my tests I got ~6Mbit/s in either direction).
Could I know if you have improvement of this throughput issue?
I have done simple test of this patchset on RTL8822CE, and it works
well. But, I think I don't test all flows yet, so I will do more
test that will take a while. After that, I can give a Tested-by tag.
Thank you
Ping-Ke
Hi Ping-Ke,
On Fri, Jan 7, 2022 at 9:42 AM Pkshih <[email protected]> wrote:
[...]
>
> > @@ -699,11 +702,20 @@ 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;
> > +
> > + mutex_lock(&rtwdev->mutex);
>
> I think this lock is used to protect br_data.si[i], right?
Correct, I chose this lock because it's also used in
rtw_ops_sta_remove() and rtw_ops_sta_add() (which could modify the
data in br_data.si[i]).
> And, I prefer to move mutex lock to caller, like:
>
> @@ -734,7 +734,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;
> }
Thank you for this hint - if I do it like you suggest then the locking
will be consistent with other functions.
I'll send a v3 with this fixed.
Best regards,
Martin
Hi Ping-Ke,
On Fri, Jan 7, 2022 at 10:19 AM Pkshih <[email protected]> wrote:
>
>
> > -----Original Message-----
> > From: Martin Blumenstingl <[email protected]>
> > Sent: Wednesday, December 29, 2021 5:15 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]>; Martin Blumenstingl <[email protected]>
> > Subject: [PATCH 0/9] rtw88: prepare locking for SDIO support
> >
> > 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 (though it's not
> > fast yet, in my tests I got ~6Mbit/s in either direction).
>
> Could I know if you have improvement of this throughput issue?
Yes, in the meantime we have made some performance improvements.
Currently the throughput numbers are approx.:
TX: 30Mbit/s
RX: 20Mbit/s
I have seen RX and TX throughputs of up to 50Mbit/s on my RTL8822CS,
but I cannot reliably reproduce this (meaning: if I don't touch my
board and run the same iperf3 test again then in one run it may
achieve 50Mbit/s, but in the next run only 25Mbit/s).
In other words: throughput is much better than what we started with in
summer, but I think it can be improved further.
> I have done simple test of this patchset on RTL8822CE, and it works
> well. But, I think I don't test all flows yet, so I will do more
> test that will take a while. After that, I can give a Tested-by tag.
I also got feedback off-list from a user who used the patches from
this series on top of the out-of-tree rtw88-usb driver. These patches
fix one "scheduling while atomic" issue for him as well.
Maybe you can do your extensive tests after I sent v3 of this series?
Also thanks for offering to test this, I don't have any Realtek PCIe
wifi, so I am unable to verify I broke anything myself.
Best regards,
Martin