| Message ID | 20211228211501.468981-4-martin.blumenstingl@googlemail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Kalle Valo |
| Headers | show |
| Series | rtw88: prepare locking for SDIO support | expand |
> -----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
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 --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,
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(-)