[PATCH RFC v1 0/7] rtw88: prepare locking for SDIO support

[PATCH RFC v1 0/7] 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.
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

[PATCH RFC v1 1/7] mac80211: Add stations iterator where the iterator function may sleep

[Martin Blumenstingl]

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 <martin.blumenstingl@googlemail.com>
---
 include/net/mac80211.h | 18 ++++++++++++++++++
 net/mac80211/util.c    | 13 +++++++++++++
 2 files changed, 31 insertions(+)

diff --git a/include/net/mac80211.h b/include/net/mac80211.h
index d8a1d09a2141..77de89690008 100644
--- a/include/net/mac80211.h
+++ b/include/net/mac80211.h
@@ -5575,6 +5575,24 @@ void ieee80211_iterate_active_interfaces_mtx(struct ieee80211_hw *hw,
 						struct ieee80211_vif *vif),
 					     void *data);
 
+/**
+ * ieee80211_iterate_stations_atomic - 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 05e96212b104..c6984d0464f2 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.32.0

RE: [PATCH RFC v1 1/7] mac80211: Add stations iterator where the iterator function may sleep

[Pkshih]

> -----Original Message-----
> From: Martin Blumenstingl [mailto:martin.blumenstingl@googlemail.com]
> Sent: Sunday, July 18, 2021 4:41 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; Jernej Skrabec; Martin Blumenstingl
> Subject: [PATCH RFC v1 1/7] mac80211: Add stations iterator where the iterator function may sleep
> 
> 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 <martin.blumenstingl@googlemail.com>
> ---
>  include/net/mac80211.h | 18 ++++++++++++++++++
>  net/mac80211/util.c    | 13 +++++++++++++
>  2 files changed, 31 insertions(+)
> 
> diff --git a/include/net/mac80211.h b/include/net/mac80211.h
> index d8a1d09a2141..77de89690008 100644
> --- a/include/net/mac80211.h
> +++ b/include/net/mac80211.h
> @@ -5575,6 +5575,24 @@ void ieee80211_iterate_active_interfaces_mtx(struct ieee80211_hw *hw,
>  						struct ieee80211_vif *vif),
>  					     void *data);
> 
> +/**
> + * ieee80211_iterate_stations_atomic - iterate stations

ieee80211_iterate_stations - ...

[...]

--
Ping-Ke

Re: [PATCH RFC v1 1/7] mac80211: Add stations iterator where the iterator function may sleep

[Johannes Berg]

> 
> +/**
> + * ieee80211_iterate_stations_atomic - iterate stations

Copy/paste issue, as PK pointed out too.

> + *
> + * 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.
> 

I have no real objections to this, but I think you should carefully
document something like "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" or something like
that, because both of those things are going to cause deadlocks.

johannes

[PATCH RFC v1 2/7] 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 | 6 +++---
 drivers/net/wireless/realtek/rtw88/ps.c   | 2 +-
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/net/wireless/realtek/rtw88/main.c b/drivers/net/wireless/realtek/rtw88/main.c
index c6364837e83b..207161a8f5bd 100644
--- a/drivers/net/wireless/realtek/rtw88/main.c
+++ b/drivers/net/wireless/realtek/rtw88/main.c
@@ -229,8 +229,8 @@ static void rtw_watch_dog_work(struct work_struct *work)
 	rtw_phy_dynamic_mechanism(rtwdev);
 
 	data.rtwdev = rtwdev;
-	/* use atomic version to avoid taking local->iflist_mtx mutex */
-	rtw_iterate_vifs_atomic(rtwdev, rtw_vif_watch_dog_iter, &data);
+
+	rtw_iterate_vifs(rtwdev, rtw_vif_watch_dog_iter, &data);
 
 	/* fw supports only one station associated to enter lps, if there are
 	 * more than two stations associated to the AP, then we can not enter
@@ -578,7 +578,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 3f0ac33156d6..95f9060b083f 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.32.0

RE: [PATCH RFC v1 2/7] rtw88: Use rtw_iterate_vifs where the iterator reads or writes registers

[Pkshih]

> -----Original Message-----
> From: Martin Blumenstingl [mailto:martin.blumenstingl@googlemail.com]
> Sent: Sunday, July 18, 2021 4:41 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; Jernej Skrabec; Martin Blumenstingl
> Subject: [PATCH RFC v1 2/7] rtw88: Use rtw_iterate_vifs where the iterator reads or writes registers
> 
> 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 | 6 +++---
>  drivers/net/wireless/realtek/rtw88/ps.c   | 2 +-
>  2 files changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/net/wireless/realtek/rtw88/main.c b/drivers/net/wireless/realtek/rtw88/main.c
> index c6364837e83b..207161a8f5bd 100644
> --- a/drivers/net/wireless/realtek/rtw88/main.c
> +++ b/drivers/net/wireless/realtek/rtw88/main.c
> @@ -229,8 +229,8 @@ static void rtw_watch_dog_work(struct work_struct *work)
>  	rtw_phy_dynamic_mechanism(rtwdev);
> 
>  	data.rtwdev = rtwdev;
> -	/* use atomic version to avoid taking local->iflist_mtx mutex */
> -	rtw_iterate_vifs_atomic(rtwdev, rtw_vif_watch_dog_iter, &data);
> +
> +	rtw_iterate_vifs(rtwdev, rtw_vif_watch_dog_iter, &data);

You revert the fix of [1].

I think we can move out rtw_chip_cfg_csi_rate() from rtw_dynamic_csi_rate(), and
add/set a field cfg_csi_rate to itera data. Then, we do rtw_chip_cfg_csi_rate()
outside iterate function. Therefore, we can keep the atomic version of iterate_vifs.


[1] https://lore.kernel.org/linux-wireless/1556886547-23632-1-git-send-email-sgruszka@redhat.com/

--
Ping-Ke

Re: [PATCH RFC v1 2/7] rtw88: Use rtw_iterate_vifs where the iterator reads or writes registers

[Martin Blumenstingl]

Hello Ping-Ke,

On Mon, Jul 19, 2021 at 7:47 AM Pkshih <pkshih@realtek.com> wrote:
>
>
>
> > -----Original Message-----
> > From: Martin Blumenstingl [mailto:martin.blumenstingl@googlemail.com]
> > Sent: Sunday, July 18, 2021 4:41 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; Jernej Skrabec; Martin Blumenstingl
> > Subject: [PATCH RFC v1 2/7] rtw88: Use rtw_iterate_vifs where the iterator reads or writes registers
> >
> > 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 | 6 +++---
> >  drivers/net/wireless/realtek/rtw88/ps.c   | 2 +-
> >  2 files changed, 4 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/net/wireless/realtek/rtw88/main.c b/drivers/net/wireless/realtek/rtw88/main.c
> > index c6364837e83b..207161a8f5bd 100644
> > --- a/drivers/net/wireless/realtek/rtw88/main.c
> > +++ b/drivers/net/wireless/realtek/rtw88/main.c
> > @@ -229,8 +229,8 @@ static void rtw_watch_dog_work(struct work_struct *work)
> >       rtw_phy_dynamic_mechanism(rtwdev);
> >
> >       data.rtwdev = rtwdev;
> > -     /* use atomic version to avoid taking local->iflist_mtx mutex */
> > -     rtw_iterate_vifs_atomic(rtwdev, rtw_vif_watch_dog_iter, &data);
> > +
> > +     rtw_iterate_vifs(rtwdev, rtw_vif_watch_dog_iter, &data);
>
> You revert the fix of [1].
Thanks for bringing this to my attention!

> I think we can move out rtw_chip_cfg_csi_rate() from rtw_dynamic_csi_rate(), and
> add/set a field cfg_csi_rate to itera data. Then, we do rtw_chip_cfg_csi_rate()
> outside iterate function. Therefore, we can keep the atomic version of iterate_vifs.
just to make sure that I understand this correctly:
rtw_iterate_vifs_atomic can be the iterator as it was before
inside the iterator func I use something like:
    iter_data->cfg_csi_rate = rtwvif->bfee.role == RTW_BFEE_SU ||
rtwvif->bfee.role == RTW_BFEE_MU || iter_data->cfg_csi_rate;
(the last iter_data->cfg_csi_rate may read a bit strange, but I think
it's needed because there can be multiple interfaces and if any of
them has cfg_csi_rate true then we need to remember that)
then move the rtw_chip_cfg_csi_rate outside the iterator function,
taking iter_data->cfg_csi_rate to decide whether it needs to be called


Best regards,
Martin

RE: [PATCH RFC v1 2/7] rtw88: Use rtw_iterate_vifs where the iterator reads or writes registers

[Pkshih]

> -----Original Message-----
> From: Martin Blumenstingl [mailto:martin.blumenstingl@googlemail.com]
> Sent: Monday, July 26, 2021 5:31 AM
> To: Pkshih
> 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; Jernej
> Skrabec
> Subject: Re: [PATCH RFC v1 2/7] rtw88: Use rtw_iterate_vifs where the iterator reads or writes registers
> 
> Hello Ping-Ke,
> 
> On Mon, Jul 19, 2021 at 7:47 AM Pkshih <pkshih@realtek.com> wrote:
> >
> >
> >
> > > -----Original Message-----
> > > From: Martin Blumenstingl [mailto:martin.blumenstingl@googlemail.com]
> > > Sent: Sunday, July 18, 2021 4:41 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; Jernej Skrabec; Martin Blumenstingl
> > > Subject: [PATCH RFC v1 2/7] rtw88: Use rtw_iterate_vifs where the iterator reads or writes registers
> > >
> > > 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 | 6 +++---
> > >  drivers/net/wireless/realtek/rtw88/ps.c   | 2 +-
> > >  2 files changed, 4 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/drivers/net/wireless/realtek/rtw88/main.c
> b/drivers/net/wireless/realtek/rtw88/main.c
> > > index c6364837e83b..207161a8f5bd 100644
> > > --- a/drivers/net/wireless/realtek/rtw88/main.c
> > > +++ b/drivers/net/wireless/realtek/rtw88/main.c
> > > @@ -229,8 +229,8 @@ static void rtw_watch_dog_work(struct work_struct *work)
> > >       rtw_phy_dynamic_mechanism(rtwdev);
> > >
> > >       data.rtwdev = rtwdev;
> > > -     /* use atomic version to avoid taking local->iflist_mtx mutex */
> > > -     rtw_iterate_vifs_atomic(rtwdev, rtw_vif_watch_dog_iter, &data);
> > > +
> > > +     rtw_iterate_vifs(rtwdev, rtw_vif_watch_dog_iter, &data);
> >
> > You revert the fix of [1].
> Thanks for bringing this to my attention!
> 
> > I think we can move out rtw_chip_cfg_csi_rate() from rtw_dynamic_csi_rate(), and
> > add/set a field cfg_csi_rate to itera data. Then, we do rtw_chip_cfg_csi_rate()
> > outside iterate function. Therefore, we can keep the atomic version of iterate_vifs.
> just to make sure that I understand this correctly:
> rtw_iterate_vifs_atomic can be the iterator as it was before
> inside the iterator func I use something like:
>     iter_data->cfg_csi_rate = rtwvif->bfee.role == RTW_BFEE_SU ||
> rtwvif->bfee.role == RTW_BFEE_MU || iter_data->cfg_csi_rate;
> (the last iter_data->cfg_csi_rate may read a bit strange, but I think
> it's needed because there can be multiple interfaces and if any of
> them has cfg_csi_rate true then we need to remember that)
> then move the rtw_chip_cfg_csi_rate outside the iterator function,
> taking iter_data->cfg_csi_rate to decide whether it needs to be called
> 

Yes, you understand correctly.

For the strange part that you mentioned, how about this?
iter_data->cfg_csi_rate |= rtwvif->bfee.role == RTW_BFEE_SU ||
                           rtwvif->bfee.role == RTW_BFEE_MU;

--
Ping-Ke

[PATCH RFC v1 3/7] 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/mac80211.c | 2 +-
 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 +-
 5 files changed, 7 insertions(+), 5 deletions(-)

diff --git a/drivers/net/wireless/realtek/rtw88/mac80211.c b/drivers/net/wireless/realtek/rtw88/mac80211.c
index 6f5629852416..7650a1ca0e9e 100644
--- a/drivers/net/wireless/realtek/rtw88/mac80211.c
+++ b/drivers/net/wireless/realtek/rtw88/mac80211.c
@@ -721,7 +721,7 @@ static void rtw_ra_mask_info_update(struct rtw_dev *rtwdev,
 	br_data.rtwdev = rtwdev;
 	br_data.vif = vif;
 	br_data.mask = mask;
-	rtw_iterate_stas_atomic(rtwdev, rtw_ra_mask_info_update_iter, &br_data);
+	rtw_iterate_stas(rtwdev, rtw_ra_mask_info_update_iter, &br_data);
 }
 
 static int rtw_ops_set_bitrate_mask(struct ieee80211_hw *hw,
diff --git a/drivers/net/wireless/realtek/rtw88/main.c b/drivers/net/wireless/realtek/rtw88/main.c
index 207161a8f5bd..6e0d25f0afe3 100644
--- a/drivers/net/wireless/realtek/rtw88/main.c
+++ b/drivers/net/wireless/realtek/rtw88/main.c
@@ -577,7 +577,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 569dd3cfde35..8f2827ecb514 100644
--- a/drivers/net/wireless/realtek/rtw88/phy.c
+++ b/drivers/net/wireless/realtek/rtw88/phy.c
@@ -240,7 +240,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;
@@ -484,7 +484,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 fc9544f4e5e4..9c4050d4c6e2 100644
--- a/drivers/net/wireless/realtek/rtw88/wow.c
+++ b/drivers/net/wireless/realtek/rtw88/wow.c
@@ -429,7 +429,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 void rtw_wow_config_pno_rsvd_page(struct rtw_dev *rtwdev,
-- 
2.32.0

Re: [PATCH RFC v1 3/7] rtw88: Use rtw_iterate_stas where the iterator reads or writes registers

[Johannes Berg]

On Sat, 2021-07-17 at 22:40 +0200, Martin Blumenstingl wrote:
> 
> --- a/drivers/net/wireless/realtek/rtw88/mac80211.c
> +++ b/drivers/net/wireless/realtek/rtw88/mac80211.c
> @@ -721,7 +721,7 @@ static void rtw_ra_mask_info_update(struct rtw_dev *rtwdev,
>  	br_data.rtwdev = rtwdev;
>  	br_data.vif = vif;
>  	br_data.mask = mask;
> -	rtw_iterate_stas_atomic(rtwdev, rtw_ra_mask_info_update_iter, &br_data);
> +	rtw_iterate_stas(rtwdev, rtw_ra_mask_info_update_iter, &br_data);

And then you pretty much immediately break that invariant here, namely
that you're calling this within the set_bitrate_mask() method called by
mac80211.

That's not actually fundamentally broken today, but it does *severely*
restrict what we can do in mac80211 wrt. locking, and I really don't
want to keep the dozen or so locks forever, this needs simplification
because clearly we don't even know what should be under what lock.

So like I said on the other patch, I don't have a fundamental objection
to taking such a patch, but the locking mess that this gets us into is
something I'd rather not have.

Maybe just don't support set_bitrate_mask for SDIO drivers for now?

The other cases look OK, it's being called from outside contexts
(wowlan, etc.)

johannes

Re: [PATCH RFC v1 3/7] rtw88: Use rtw_iterate_stas where the iterator reads or writes registers

[Martin Blumenstingl]

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

Hi Johannes, Hi Ping-Ke,

On Mon, Jul 19, 2021 at 8:36 AM Johannes Berg <johannes@sipsolutions.net> wrote:
>
> On Sat, 2021-07-17 at 22:40 +0200, Martin Blumenstingl wrote:
> >
> > --- a/drivers/net/wireless/realtek/rtw88/mac80211.c
> > +++ b/drivers/net/wireless/realtek/rtw88/mac80211.c
> > @@ -721,7 +721,7 @@ static void rtw_ra_mask_info_update(struct rtw_dev *rtwdev,
> >       br_data.rtwdev = rtwdev;
> >       br_data.vif = vif;
> >       br_data.mask = mask;
> > -     rtw_iterate_stas_atomic(rtwdev, rtw_ra_mask_info_update_iter, &br_data);
> > +     rtw_iterate_stas(rtwdev, rtw_ra_mask_info_update_iter, &br_data);
>
> And then you pretty much immediately break that invariant here, namely
> that you're calling this within the set_bitrate_mask() method called by
> mac80211.
you are right, I was not aware of this

> That's not actually fundamentally broken today, but it does *severely*
> restrict what we can do in mac80211 wrt. locking, and I really don't
> want to keep the dozen or so locks forever, this needs simplification
> because clearly we don't even know what should be under what lock.
To me it's also not clear what the goal of the whole locking is.
The lock in ieee80211_iterate_stations_atomic is obviously for the
mac80211-internal state-machine
But I *believe* that there's a second purpose (rtw88 specific) -
here's my understanding of that part:
- rtw_sta_info contains a "mac_id" which is an identifier for a
specific station used by the rtw88 driver and is shared with the
firmware
- rtw_ops_sta_{add,remove} uses rtwdev->mutex to protect the rtw88
side of this "mac_id" identifier
- (for some reason rtw_update_sta_info doesn't use rtwdev->mutex)

So now I am wondering if the ieee80211_iterate_stations_atomic lock is
also used to protect any modifications to rtw_sta_info.
Ping-Ke, I am wondering if the attached patch (untested - to better
demonstrate what I want to say) would:
- allow us to move the register write outside of
ieee80211_iterate_stations_atomic
- mean we can keep ieee80211_iterate_stations_atomic (instead of the
non-atomic variant)
- protect the code managing the "mac_id" with rtwdev->mutex consistently

> The other cases look OK, it's being called from outside contexts
> (wowlan, etc.)
Thanks for reviewing this Johannes!


Best regards,
Martin

[-- Attachment #2: rtw_update_sta_info-outside-rtw_iterate_stas_atomic.patch --]
[-- Type: text/x-patch, Size: 1505 bytes --]

diff --git a/drivers/net/wireless/realtek/rtw88/mac80211.c b/drivers/net/wireless/realtek/rtw88/mac80211.c
index 7650a1ca0e9e..be39c6d0ee31 100644
--- a/drivers/net/wireless/realtek/rtw88/mac80211.c
+++ b/drivers/net/wireless/realtek/rtw88/mac80211.c
@@ -689,6 +689,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)
@@ -709,7 +711,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,
@@ -717,11 +720,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;
-	rtw_iterate_stas(rtwdev, rtw_ra_mask_info_update_iter, &br_data);
+	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,

RE: [PATCH RFC v1 3/7] rtw88: Use rtw_iterate_stas where the iterator reads or writes registers

[Pkshih]

> -----Original Message-----
> From: Martin Blumenstingl [mailto:martin.blumenstingl@googlemail.com]
> Sent: Monday, July 26, 2021 5:51 AM
> To: Johannes Berg; Pkshih
> Cc: linux-wireless@vger.kernel.org; tony0620emma@gmail.com; kvalo@codeaurora.org;
> netdev@vger.kernel.org; linux-kernel@vger.kernel.org; Neo Jou; Jernej Skrabec
> Subject: Re: [PATCH RFC v1 3/7] rtw88: Use rtw_iterate_stas where the iterator reads or writes registers
> 
> Hi Johannes, Hi Ping-Ke,
> 
> On Mon, Jul 19, 2021 at 8:36 AM Johannes Berg <johannes@sipsolutions.net> wrote:
> >
> > On Sat, 2021-07-17 at 22:40 +0200, Martin Blumenstingl wrote:
> > >
> > > --- a/drivers/net/wireless/realtek/rtw88/mac80211.c
> > > +++ b/drivers/net/wireless/realtek/rtw88/mac80211.c
> > > @@ -721,7 +721,7 @@ static void rtw_ra_mask_info_update(struct rtw_dev *rtwdev,
> > >       br_data.rtwdev = rtwdev;
> > >       br_data.vif = vif;
> > >       br_data.mask = mask;
> > > -     rtw_iterate_stas_atomic(rtwdev, rtw_ra_mask_info_update_iter, &br_data);
> > > +     rtw_iterate_stas(rtwdev, rtw_ra_mask_info_update_iter, &br_data);
> >
> > And then you pretty much immediately break that invariant here, namely
> > that you're calling this within the set_bitrate_mask() method called by
> > mac80211.
> you are right, I was not aware of this
> 
> > That's not actually fundamentally broken today, but it does *severely*
> > restrict what we can do in mac80211 wrt. locking, and I really don't
> > want to keep the dozen or so locks forever, this needs simplification
> > because clearly we don't even know what should be under what lock.
> To me it's also not clear what the goal of the whole locking is.
> The lock in ieee80211_iterate_stations_atomic is obviously for the
> mac80211-internal state-machine
> But I *believe* that there's a second purpose (rtw88 specific) -
> here's my understanding of that part:
> - rtw_sta_info contains a "mac_id" which is an identifier for a
> specific station used by the rtw88 driver and is shared with the
> firmware
> - rtw_ops_sta_{add,remove} uses rtwdev->mutex to protect the rtw88
> side of this "mac_id" identifier
> - (for some reason rtw_update_sta_info doesn't use rtwdev->mutex)

I am thinking rtw88 needs to maintain sta and vif lists itself, and
these lists are also protected by rtwdev->mutex. When rtw88 wants to
iterate all sta/vif, it holds rtwdev->mutex to do list_for_each_entry.
No need to hold mac80211 locks.

> 
> So now I am wondering if the ieee80211_iterate_stations_atomic lock is
> also used to protect any modifications to rtw_sta_info.
> Ping-Ke, I am wondering if the attached patch (untested - to better
> demonstrate what I want to say) would:
> - allow us to move the register write outside of
> ieee80211_iterate_stations_atomic
> - mean we can keep ieee80211_iterate_stations_atomic (instead of the
> non-atomic variant)
> - protect the code managing the "mac_id" with rtwdev->mutex consistently

I think your attached patch can work well.

> 
> > The other cases look OK, it's being called from outside contexts
> > (wowlan, etc.)
> Thanks for reviewing this Johannes!
> 

--
Ping-Ke

Re: [PATCH RFC v1 3/7] rtw88: Use rtw_iterate_stas where the iterator reads or writes registers

[Johannes Berg]

> 
> I am thinking rtw88 needs to maintain sta and vif lists itself, 

I would tend to prefer drivers do not maintain separate lists - that's
just duplicated book-keeping and prone to state mismatch errors?

But OTOH the locking does make things complex.

johannes

[PATCH RFC v1 4/7] 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 6e0d25f0afe3..e40432b1dcee 100644
--- a/drivers/net/wireless/realtek/rtw88/main.c
+++ b/drivers/net/wireless/realtek/rtw88/main.c
@@ -574,9 +574,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.32.0

[PATCH RFC v1 5/7] rtw88: Configure the registers from rtw_bf_assoc() outside the RCU lock

[Martin Blumenstingl]

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 <martin.blumenstingl@googlemail.com>
---
 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

RE: [PATCH RFC v1 5/7] rtw88: Configure the registers from rtw_bf_assoc() outside the RCU lock

[Pkshih]

> -----Original Message-----
> From: Martin Blumenstingl [mailto:martin.blumenstingl@googlemail.com]
> Sent: Sunday, July 18, 2021 4:41 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; 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 <martin.blumenstingl@googlemail.com>
> ---
>  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

Re: [PATCH RFC v1 5/7] rtw88: Configure the registers from rtw_bf_assoc() outside the RCU lock

[Martin Blumenstingl]

Hi Ping-Ke,

On Mon, Jul 19, 2021 at 7:47 AM Pkshih <pkshih@realtek.com> 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

RE: [PATCH RFC v1 5/7] rtw88: Configure the registers from rtw_bf_assoc() outside the RCU lock

[Pkshih]

> -----Original Message-----
> From: Martin Blumenstingl [mailto:martin.blumenstingl@googlemail.com]
> Sent: Monday, July 26, 2021 5:36 AM
> To: Pkshih
> 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; 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 <pkshih@realtek.com> 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

[PATCH RFC v1 6/7] 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 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

[PATCH RFC v1 7/7] 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 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

RE: [PATCH RFC v1 0/7] rtw88: prepare locking for SDIO support

[Pkshih]

> -----Original Message-----
> From: Martin Blumenstingl [mailto:martin.blumenstingl@googlemail.com]
> Sent: Sunday, July 18, 2021 4:41 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; 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