diff mbox series

[3/9] rtw88: Move rtw_update_sta_info() out of rtw_ra_mask_info_update_iter()

Message ID 20211228211501.468981-4-martin.blumenstingl@googlemail.com
State New
Headers show
Series rtw88: prepare locking for SDIO support | expand

Commit Message

Martin Blumenstingl Dec. 28, 2021, 9:14 p.m. UTC
rtw_update_sta_info() internally access some registers while being
called unter an atomic lock acquired by rtw_iterate_vifs_atomic(). Move
rtw_update_sta_info() call out of (rtw_ra_mask_info_update_iter) in
preparation for SDIO support where register access may sleep.

Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
---
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(-)

Comments

Ping-Ke Shih Jan. 7, 2022, 8:42 a.m. UTC | #1
> -----Original Message-----
> From: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
> Sent: Wednesday, December 29, 2021 5:15 AM
> To: linux-wireless@vger.kernel.org
> Cc: tony0620emma@gmail.com; kvalo@codeaurora.org; johannes@sipsolutions.net; netdev@vger.kernel.org;
> linux-kernel@vger.kernel.org; Neo Jou <neojou@gmail.com>; Jernej Skrabec <jernej.skrabec@gmail.com>;
> Pkshih <pkshih@realtek.com>; Martin Blumenstingl <martin.blumenstingl@googlemail.com>
> 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 <martin.blumenstingl@googlemail.com>
> ---
> 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
Martin Blumenstingl Jan. 7, 2022, 9:44 p.m. UTC | #2
Hi Ping-Ke,

On Fri, Jan 7, 2022 at 9:42 AM Pkshih <pkshih@realtek.com> 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
diff mbox series

Patch

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,