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.
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 [0]):
- 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.
[0] https://lore.kernel.org/linux-wireless/CAFBinCDMPPJ7qW7xTkep1Trg+zP0B9Jxei6sgjqmF4NDA1JAhQ@mail.gmail.com/
Martin Blumenstingl (7):
mac80211: Add stations iterator where the iterator function may sleep
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 | 8 ++++++--
drivers/net/wireless/realtek/rtw88/fw.c | 14 +++++++-------
drivers/net/wireless/realtek/rtw88/hci.h | 11 ++++-------
drivers/net/wireless/realtek/rtw88/mac80211.c | 2 +-
drivers/net/wireless/realtek/rtw88/main.c | 16 +++++++---------
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 | 18 ++++++++++++++++++
net/mac80211/util.c | 13 +++++++++++++
12 files changed, 64 insertions(+), 34 deletions(-)
--
2.32.0
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 3bfa5ecc0053..5acc798299e5 100644
--- a/drivers/net/wireless/realtek/rtw88/fw.c
+++ b/drivers/net/wireless/realtek/rtw88/fw.c
@@ -285,7 +285,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) {
@@ -310,9 +310,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");
@@ -328,7 +328,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)
@@ -340,7 +340,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);
@@ -348,7 +348,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 5ebc4c0b4ccc..34e5bc97d9f4 100644
--- a/drivers/net/wireless/realtek/rtw88/main.c
+++ b/drivers/net/wireless/realtek/rtw88/main.c
@@ -1834,12 +1834,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 fd213252fbe2..1788fc339afb 100644
--- a/drivers/net/wireless/realtek/rtw88/main.h
+++ b/drivers/net/wireless/realtek/rtw88/main.h
@@ -1868,7 +1868,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.32.0
Upcoming SDIO support may sleep in the read/write handlers. Configure
the chip's BFEE configuration set from rtw_bf_assoc() outside the
rcu_read_lock section to prevent a "scheduling while atomic" issue.
Signed-off-by: Martin Blumenstingl <[email protected]>
---
drivers/net/wireless/realtek/rtw88/bf.c | 8 ++++++--
1 file changed, 6 insertions(+), 2 deletions(-)
diff --git a/drivers/net/wireless/realtek/rtw88/bf.c b/drivers/net/wireless/realtek/rtw88/bf.c
index aff70e4ae028..06034d5d6f6c 100644
--- a/drivers/net/wireless/realtek/rtw88/bf.c
+++ b/drivers/net/wireless/realtek/rtw88/bf.c
@@ -39,6 +39,7 @@ void rtw_bf_assoc(struct rtw_dev *rtwdev, struct ieee80211_vif *vif,
struct ieee80211_sta_vht_cap *vht_cap;
struct ieee80211_sta_vht_cap *ic_vht_cap;
const u8 *bssid = bss_conf->bssid;
+ bool config_bfee = false;
u32 sound_dim;
u8 i;
@@ -70,7 +71,7 @@ void rtw_bf_assoc(struct rtw_dev *rtwdev, struct ieee80211_vif *vif,
bfee->aid = bss_conf->aid;
bfinfo->bfer_mu_cnt++;
- rtw_chip_config_bfee(rtwdev, rtwvif, bfee, true);
+ config_bfee = true;
} else if ((ic_vht_cap->cap & IEEE80211_VHT_CAP_SU_BEAMFORMEE_CAPABLE) &&
(vht_cap->cap & IEEE80211_VHT_CAP_SU_BEAMFORMER_CAPABLE)) {
if (bfinfo->bfer_su_cnt >= chip->bfer_su_max_num) {
@@ -96,11 +97,14 @@ void rtw_bf_assoc(struct rtw_dev *rtwdev, struct ieee80211_vif *vif,
}
}
- rtw_chip_config_bfee(rtwdev, rtwvif, bfee, true);
+ config_bfee = true;
}
out_unlock:
rcu_read_unlock();
+
+ if (config_bfee)
+ rtw_chip_config_bfee(rtwdev, rtwvif, bfee, true);
}
void rtw_bf_init_bfer_entry_mu(struct rtw_dev *rtwdev,
--
2.32.0
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 e40432b1dcee..5ebc4c0b4ccc 100644
--- a/drivers/net/wireless/realtek/rtw88/main.c
+++ b/drivers/net/wireless/realtek/rtw88/main.c
@@ -1834,12 +1834,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 e5af375b3dd0..fd213252fbe2 100644
--- a/drivers/net/wireless/realtek/rtw88/main.h
+++ b/drivers/net/wireless/realtek/rtw88/main.h
@@ -1842,7 +1842,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.32.0
> -----Original Message-----
> From: Martin Blumenstingl [mailto:[email protected]]
> Sent: Sunday, July 18, 2021 4:41 AM
> To: [email protected]
> Cc: [email protected]; [email protected]; [email protected]; [email protected];
> [email protected]; Neo Jou; Jernej Skrabec; Martin Blumenstingl
> Subject: [PATCH RFC v1 5/7] rtw88: Configure the registers from rtw_bf_assoc() outside the RCU lock
>
> Upcoming SDIO support may sleep in the read/write handlers. Configure
> the chip's BFEE configuration set from rtw_bf_assoc() outside the
> rcu_read_lock section to prevent a "scheduling while atomic" issue.
>
> Signed-off-by: Martin Blumenstingl <[email protected]>
> ---
> drivers/net/wireless/realtek/rtw88/bf.c | 8 ++++++--
> 1 file changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/net/wireless/realtek/rtw88/bf.c b/drivers/net/wireless/realtek/rtw88/bf.c
> index aff70e4ae028..06034d5d6f6c 100644
> --- a/drivers/net/wireless/realtek/rtw88/bf.c
> +++ b/drivers/net/wireless/realtek/rtw88/bf.c
> @@ -39,6 +39,7 @@ void rtw_bf_assoc(struct rtw_dev *rtwdev, struct ieee80211_vif *vif,
> struct ieee80211_sta_vht_cap *vht_cap;
> struct ieee80211_sta_vht_cap *ic_vht_cap;
> const u8 *bssid = bss_conf->bssid;
> + bool config_bfee = false;
> u32 sound_dim;
> u8 i;
>
The rcu_read_lock() in this function is used to access ieee80211_find_sta() and protect 'sta'.
A simple way is to shrink the critical section, like:
rcu_read_lock();
sta = ieee80211_find_sta(vif, bssid);
if (!sta) {
rtw_warn(rtwdev, "failed to find station entry for bss %pM\n",
bssid);
rcu_read_unlock();
}
vht_cap = &sta->vht_cap;
rcu_read_unlock();
--
Ping-Ke
> -----Original Message-----
> From: Martin Blumenstingl [mailto:[email protected]]
> Sent: Sunday, July 18, 2021 4:41 AM
> To: [email protected]
> Cc: [email protected]; [email protected]; [email protected]; [email protected];
> [email protected]; Neo Jou; Jernej Skrabec; Martin Blumenstingl
> Subject: [PATCH RFC v1 0/7] rtw88: prepare locking for SDIO support
>
[...]
I have reviewed patchset v1. But, please wait a moment before sending v2 to see
if other experts have better suggestions.
--
Ping-Ke
Hi Ping-Ke,
On Mon, Jul 19, 2021 at 7:47 AM Pkshih <[email protected]> wrote:
[...]
> The rcu_read_lock() in this function is used to access ieee80211_find_sta() and protect 'sta'.
> A simple way is to shrink the critical section, like:
>
> rcu_read_lock();
>
> sta = ieee80211_find_sta(vif, bssid);
> if (!sta) {
> rtw_warn(rtwdev, "failed to find station entry for bss %pM\n",
> bssid);
> rcu_read_unlock();
> }
>
> vht_cap = &sta->vht_cap;
>
> rcu_read_unlock();
I agree that reducing the amount of code under the lock will help my
use-case as well
in your code-example I am wondering if we should change
struct ieee80211_sta_vht_cap *vht_cap;
vht_cap = &sta->vht_cap;
to
struct ieee80211_sta_vht_cap vht_cap;
vht_cap = sta->vht_cap;
My thinking is that ieee80211_sta may be freed in parallel to this code running.
If that cannot happen then your code will be fine.
So I am hoping that you can also share your thoughts on this one.
Thank you and best regards,
Martin
> -----Original Message-----
> From: Martin Blumenstingl [mailto:[email protected]]
> Sent: Monday, July 26, 2021 5:36 AM
> To: Pkshih
> Cc: [email protected]; [email protected]; [email protected];
> [email protected]; [email protected]; [email protected]; Neo Jou; Jernej
> Skrabec
> Subject: Re: [PATCH RFC v1 5/7] rtw88: Configure the registers from rtw_bf_assoc() outside the RCU lock
>
> Hi Ping-Ke,
>
> On Mon, Jul 19, 2021 at 7:47 AM Pkshih <[email protected]> wrote:
> [...]
> > The rcu_read_lock() in this function is used to access ieee80211_find_sta() and protect 'sta'.
> > A simple way is to shrink the critical section, like:
> >
> > rcu_read_lock();
> >
> > sta = ieee80211_find_sta(vif, bssid);
> > if (!sta) {
> > rtw_warn(rtwdev, "failed to find station entry for bss %pM\n",
> > bssid);
> > rcu_read_unlock();
> > }
> >
> > vht_cap = &sta->vht_cap;
> >
> > rcu_read_unlock();
> I agree that reducing the amount of code under the lock will help my
> use-case as well
> in your code-example I am wondering if we should change
> struct ieee80211_sta_vht_cap *vht_cap;
> vht_cap = &sta->vht_cap;
> to
> struct ieee80211_sta_vht_cap vht_cap;
> vht_cap = sta->vht_cap;
>
> My thinking is that ieee80211_sta may be freed in parallel to this code running.
> If that cannot happen then your code will be fine.
>
> So I am hoping that you can also share your thoughts on this one.
>
When we enter rtw_bf_assoc(), the mutex rtwdev->mutex is held; as well as
rtw_sta_add()/rtw_sta_remove(). So, I think it cannot happen that ieee80211_sta
was freed in parallel.
--
Ping-Ke