diff mbox series

[05/10] rtw88: Do not access registers while atomic

Message ID 20220518082318.3898514-6-s.hauer@pengutronix.de
State New
Headers show
Series RTW88: Add support for USB variants | expand

Commit Message

Sascha Hauer May 18, 2022, 8:23 a.m. UTC
The driver uses ieee80211_iterate_active_interfaces_atomic()
and ieee80211_iterate_stations_atomic() in several places and does
register accesses in the iterators. This doesn't cope with upcoming
USB support as registers can only be accessed non-atomically.

Split these into a two stage process: First use the atomic iterator
functions to collect all active interfaces or stations on a list, then
iterate over the list non-atomically and call the iterator on each
entry.

Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
Suggested-by: Pkshih <pkshih@realtek.com>
---
 drivers/net/wireless/realtek/rtw88/phy.c  |  6 +-
 drivers/net/wireless/realtek/rtw88/ps.c   |  2 +-
 drivers/net/wireless/realtek/rtw88/util.c | 92 +++++++++++++++++++++++
 drivers/net/wireless/realtek/rtw88/util.h | 12 ++-
 4 files changed, 105 insertions(+), 7 deletions(-)

Comments

Ping-Ke Shih May 20, 2022, 6:06 a.m. UTC | #1
On Wed, 2022-05-18 at 10:23 +0200, Sascha Hauer wrote:
> The driver uses ieee80211_iterate_active_interfaces_atomic()
> and ieee80211_iterate_stations_atomic() in several places and does
> register accesses in the iterators. This doesn't cope with upcoming
                                           ^^^^^^^ does?
> USB support as registers can only be accessed non-atomically.
> 
> Split these into a two stage process: First use the atomic iterator
> functions to collect all active interfaces or stations on a list, then
> iterate over the list non-atomically and call the iterator on each
> entry.

I think the subject could be "iterate over vif/sta list non-atomically"

> 
> Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
> Suggested-by: Pkshih <pkshih@realtek.com>
> ---
>  drivers/net/wireless/realtek/rtw88/phy.c  |  6 +-
>  drivers/net/wireless/realtek/rtw88/ps.c   |  2 +-
>  drivers/net/wireless/realtek/rtw88/util.c | 92 +++++++++++++++++++++++
>  drivers/net/wireless/realtek/rtw88/util.h | 12 ++-
>  4 files changed, 105 insertions(+), 7 deletions(-)
> 
> 

[...]

>  
> diff --git a/drivers/net/wireless/realtek/rtw88/util.c b/drivers/net/wireless/realtek/rtw88/util.c
> index 2c515af214e76..db55dbd5c533e 100644
> --- a/drivers/net/wireless/realtek/rtw88/util.c
> +++ b/drivers/net/wireless/realtek/rtw88/util.c
> @@ -105,3 +105,95 @@ void rtw_desc_to_mcsrate(u16 rate, u8 *mcs, u8 *nss)
>  		*mcs = rate - DESC_RATEMCS0;
>  	}
>  }
> 

[...]

> +
> +void rtw_iterate_stas(struct rtw_dev *rtwdev,
> +		      void (*iterator)(void *data,
> +				       struct ieee80211_sta *sta),
> +				       void *data)
> +{
> +	struct rtw_iter_stas_data iter_data;
> +	struct rtw_stas_entry *sta_entry, *tmp;

lockdep_assert_held(&rtwdev->mutex);

> +
> +	iter_data.rtwdev = rtwdev;
> +	INIT_LIST_HEAD(&iter_data.list);
> +
> +	ieee80211_iterate_stations_atomic(rtwdev->hw, rtw_collect_sta_iter,
> +					  &iter_data);
> +
> +	list_for_each_entry_safe(sta_entry, tmp, &iter_data.list,
> +				 list) {
> +		list_del_init(&sta_entry->list);
> +		iterator(data, sta_entry->sta);
> +		kfree(sta_entry);
> +	}
> +}
> +

[...]

> +void rtw_iterate_vifs(struct rtw_dev *rtwdev,
> +		      void (*iterator)(void *data, u8 *mac,
> +				       struct ieee80211_vif *vif),
> +		      void *data)
> +{
> +	struct rtw_iter_vifs_data iter_data;
> +	struct rtw_vifs_entry *vif_entry, *tmp;

lockdep_assert_held(&rtwdev->mutex);

> +
> +	iter_data.rtwdev = rtwdev;
> +	INIT_LIST_HEAD(&iter_data.list);
> +
> +	ieee80211_iterate_active_interfaces_atomic(rtwdev->hw,
> +			IEEE80211_IFACE_ITER_NORMAL, rtw_collect_vif_iter, &iter_data);
> +
> +	list_for_each_entry_safe(vif_entry, tmp, &iter_data.list,
> +				 list) {
> +		list_del_init(&vif_entry->list);
> +		iterator(data, vif_entry->mac, vif_entry->vif);
> +		kfree(vif_entry);
> +	}
> +}
> 

[...]

--
Ping-Ke
Sascha Hauer May 20, 2022, 7:07 a.m. UTC | #2
On Fri, May 20, 2022 at 06:06:05AM +0000, Pkshih wrote:
> On Wed, 2022-05-18 at 10:23 +0200, Sascha Hauer wrote:
> > The driver uses ieee80211_iterate_active_interfaces_atomic()
> > and ieee80211_iterate_stations_atomic() in several places and does
> > register accesses in the iterators. This doesn't cope with upcoming
>                                            ^^^^^^^ does?
> > USB support as registers can only be accessed non-atomically.
> > 
> > Split these into a two stage process: First use the atomic iterator
> > functions to collect all active interfaces or stations on a list, then
> > iterate over the list non-atomically and call the iterator on each
> > entry.
> 
> I think the subject could be "iterate over vif/sta list non-atomically"

Ok.

> > +void rtw_iterate_stas(struct rtw_dev *rtwdev,
> > +		      void (*iterator)(void *data,
> > +				       struct ieee80211_sta *sta),
> > +				       void *data)
> > +{
> > +	struct rtw_iter_stas_data iter_data;
> > +	struct rtw_stas_entry *sta_entry, *tmp;
> 
> lockdep_assert_held(&rtwdev->mutex);
> 
> > +
> > +	iter_data.rtwdev = rtwdev;
> > +	INIT_LIST_HEAD(&iter_data.list);
> > +
> > +	ieee80211_iterate_stations_atomic(rtwdev->hw, rtw_collect_sta_iter,
> > +					  &iter_data);
> > +
> > +	list_for_each_entry_safe(sta_entry, tmp, &iter_data.list,
> > +				 list) {
> > +		list_del_init(&sta_entry->list);
> > +		iterator(data, sta_entry->sta);
> > +		kfree(sta_entry);
> > +	}
> > +}
> > +
> 
> [...]
> 
> > +void rtw_iterate_vifs(struct rtw_dev *rtwdev,
> > +		      void (*iterator)(void *data, u8 *mac,
> > +				       struct ieee80211_vif *vif),
> > +		      void *data)
> > +{
> > +	struct rtw_iter_vifs_data iter_data;
> > +	struct rtw_vifs_entry *vif_entry, *tmp;
> 
> lockdep_assert_held(&rtwdev->mutex);

Ok, will add these. For what it's worth they didn't trigger in a short
test.

Sascha
Larry Finger May 20, 2022, 5:32 p.m. UTC | #3
On 5/18/22 03:23, Sascha Hauer wrote:
> The driver uses ieee80211_iterate_active_interfaces_atomic()
> and ieee80211_iterate_stations_atomic() in several places and does
> register accesses in the iterators. This doesn't cope with upcoming
> USB support as registers can only be accessed non-atomically.
> 
> Split these into a two stage process: First use the atomic iterator
> functions to collect all active interfaces or stations on a list, then
> iterate over the list non-atomically and call the iterator on each
> entry.
> 
> Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
> Suggested-by: Pkshih <pkshih@realtek.com>
> ---
>   drivers/net/wireless/realtek/rtw88/phy.c  |  6 +-
>   drivers/net/wireless/realtek/rtw88/ps.c   |  2 +-
>   drivers/net/wireless/realtek/rtw88/util.c | 92 +++++++++++++++++++++++
>   drivers/net/wireless/realtek/rtw88/util.h | 12 ++-
>   4 files changed, 105 insertions(+), 7 deletions(-)
> 

...

> diff --git a/drivers/net/wireless/realtek/rtw88/util.c b/drivers/net/wireless/realtek/rtw88/util.c
> index 2c515af214e76..db55dbd5c533e 100644
> --- a/drivers/net/wireless/realtek/rtw88/util.c
> +++ b/drivers/net/wireless/realtek/rtw88/util.c
> @@ -105,3 +105,95 @@ void rtw_desc_to_mcsrate(u16 rate, u8 *mcs, u8 *nss)
>   		*mcs = rate - DESC_RATEMCS0;
>   	}
>   }
> +
> +struct rtw_stas_entry {
> +	struct list_head list;
> +	struct ieee80211_sta *sta;
> +};
> +
> +struct rtw_iter_stas_data {
> +	struct rtw_dev *rtwdev;
> +	struct list_head list;
> +};
> +
> +void rtw_collect_sta_iter(void *data, struct ieee80211_sta *sta)
> +{
> +	struct rtw_iter_stas_data *iter_stas = data;
> +	struct rtw_stas_entry *stas_entry;
> +
> +	stas_entry = kmalloc(sizeof(*stas_entry), GFP_ATOMIC);
> +	if (!stas_entry)
> +		return;
> +
> +	stas_entry->sta = sta;
> +	list_add_tail(&stas_entry->list, &iter_stas->list);
> +}
> +
> +void rtw_iterate_stas(struct rtw_dev *rtwdev,
> +		      void (*iterator)(void *data,
> +				       struct ieee80211_sta *sta),
> +				       void *data)
> +{
> +	struct rtw_iter_stas_data iter_data;
> +	struct rtw_stas_entry *sta_entry, *tmp;
> +
> +	iter_data.rtwdev = rtwdev;
> +	INIT_LIST_HEAD(&iter_data.list);
> +
> +	ieee80211_iterate_stations_atomic(rtwdev->hw, rtw_collect_sta_iter,
> +					  &iter_data);
> +
> +	list_for_each_entry_safe(sta_entry, tmp, &iter_data.list,
> +				 list) {
> +		list_del_init(&sta_entry->list);
> +		iterator(data, sta_entry->sta);
> +		kfree(sta_entry);
> +	}
> +}
> +
> +struct rtw_vifs_entry {
> +	struct list_head list;
> +	struct ieee80211_vif *vif;
> +	u8 mac[ETH_ALEN];
> +};
> +
> +struct rtw_iter_vifs_data {
> +	struct rtw_dev *rtwdev;
> +	struct list_head list;
> +};
> +
> +void rtw_collect_vif_iter(void *data, u8 *mac, struct ieee80211_vif *vif)
> +{
> +	struct rtw_iter_vifs_data *iter_stas = data;
> +	struct rtw_vifs_entry *vifs_entry;
> +
> +	vifs_entry = kmalloc(sizeof(*vifs_entry), GFP_ATOMIC);
> +	if (!vifs_entry)
> +		return;
> +
> +	vifs_entry->vif = vif;
> +	ether_addr_copy(vifs_entry->mac, mac);
> +	list_add_tail(&vifs_entry->list, &iter_stas->list);
> +}
> +
> +void rtw_iterate_vifs(struct rtw_dev *rtwdev,
> +		      void (*iterator)(void *data, u8 *mac,
> +				       struct ieee80211_vif *vif),
> +		      void *data)
> +{
> +	struct rtw_iter_vifs_data iter_data;
> +	struct rtw_vifs_entry *vif_entry, *tmp;
> +
> +	iter_data.rtwdev = rtwdev;
> +	INIT_LIST_HEAD(&iter_data.list);
> +
> +	ieee80211_iterate_active_interfaces_atomic(rtwdev->hw,
> +			IEEE80211_IFACE_ITER_NORMAL, rtw_collect_vif_iter, &iter_data);
> +
> +	list_for_each_entry_safe(vif_entry, tmp, &iter_data.list,
> +				 list) {
> +		list_del_init(&vif_entry->list);
> +		iterator(data, vif_entry->mac, vif_entry->vif);
> +		kfree(vif_entry);
> +	}
> +}

Sasha,

Sparse shows the following warnings:

   CHECK   /home/finger/iwireless-next/drivers/net/wireless/realtek/rtw88/util.c
/home/finger/wireless-next/drivers/net/wireless/realtek/rtw88/util.c:119:6: 
warning: symbol 'rtw_collect_sta_iter' was not declared. Should it be static?
/home/finger/wireless-next/drivers/net/wireless/realtek/rtw88/util.c:165:6: 
warning: symbol 'rtw_collect_vif_iter' was not declared. Should it be static?

Larry
diff mbox series

Patch

diff --git a/drivers/net/wireless/realtek/rtw88/phy.c b/drivers/net/wireless/realtek/rtw88/phy.c
index e505d17f107e4..5521a7c2c1afe 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)
@@ -597,7 +597,7 @@  static void rtw_phy_rrsr_update(struct rtw_dev *rtwdev)
 	struct rtw_dm_info *dm_info = &rtwdev->dm_info;
 
 	dm_info->rrsr_mask_min = RRSR_RATE_ORDER_MAX;
-	rtw_iterate_stas_atomic(rtwdev, rtw_phy_rrsr_mask_min_iter, rtwdev);
+	rtw_iterate_stas(rtwdev, rtw_phy_rrsr_mask_min_iter, rtwdev);
 	rtw_write32(rtwdev, REG_RRSR, dm_info->rrsr_val_init & dm_info->rrsr_mask_min);
 }
 
diff --git a/drivers/net/wireless/realtek/rtw88/ps.c b/drivers/net/wireless/realtek/rtw88/ps.c
index bfa64c038f5f0..a7213ff2c2244 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);
 
diff --git a/drivers/net/wireless/realtek/rtw88/util.c b/drivers/net/wireless/realtek/rtw88/util.c
index 2c515af214e76..db55dbd5c533e 100644
--- a/drivers/net/wireless/realtek/rtw88/util.c
+++ b/drivers/net/wireless/realtek/rtw88/util.c
@@ -105,3 +105,95 @@  void rtw_desc_to_mcsrate(u16 rate, u8 *mcs, u8 *nss)
 		*mcs = rate - DESC_RATEMCS0;
 	}
 }
+
+struct rtw_stas_entry {
+	struct list_head list;
+	struct ieee80211_sta *sta;
+};
+
+struct rtw_iter_stas_data {
+	struct rtw_dev *rtwdev;
+	struct list_head list;
+};
+
+void rtw_collect_sta_iter(void *data, struct ieee80211_sta *sta)
+{
+	struct rtw_iter_stas_data *iter_stas = data;
+	struct rtw_stas_entry *stas_entry;
+
+	stas_entry = kmalloc(sizeof(*stas_entry), GFP_ATOMIC);
+	if (!stas_entry)
+		return;
+
+	stas_entry->sta = sta;
+	list_add_tail(&stas_entry->list, &iter_stas->list);
+}
+
+void rtw_iterate_stas(struct rtw_dev *rtwdev,
+		      void (*iterator)(void *data,
+				       struct ieee80211_sta *sta),
+				       void *data)
+{
+	struct rtw_iter_stas_data iter_data;
+	struct rtw_stas_entry *sta_entry, *tmp;
+
+	iter_data.rtwdev = rtwdev;
+	INIT_LIST_HEAD(&iter_data.list);
+
+	ieee80211_iterate_stations_atomic(rtwdev->hw, rtw_collect_sta_iter,
+					  &iter_data);
+
+	list_for_each_entry_safe(sta_entry, tmp, &iter_data.list,
+				 list) {
+		list_del_init(&sta_entry->list);
+		iterator(data, sta_entry->sta);
+		kfree(sta_entry);
+	}
+}
+
+struct rtw_vifs_entry {
+	struct list_head list;
+	struct ieee80211_vif *vif;
+	u8 mac[ETH_ALEN];
+};
+
+struct rtw_iter_vifs_data {
+	struct rtw_dev *rtwdev;
+	struct list_head list;
+};
+
+void rtw_collect_vif_iter(void *data, u8 *mac, struct ieee80211_vif *vif)
+{
+	struct rtw_iter_vifs_data *iter_stas = data;
+	struct rtw_vifs_entry *vifs_entry;
+
+	vifs_entry = kmalloc(sizeof(*vifs_entry), GFP_ATOMIC);
+	if (!vifs_entry)
+		return;
+
+	vifs_entry->vif = vif;
+	ether_addr_copy(vifs_entry->mac, mac);
+	list_add_tail(&vifs_entry->list, &iter_stas->list);
+}
+
+void rtw_iterate_vifs(struct rtw_dev *rtwdev,
+		      void (*iterator)(void *data, u8 *mac,
+				       struct ieee80211_vif *vif),
+		      void *data)
+{
+	struct rtw_iter_vifs_data iter_data;
+	struct rtw_vifs_entry *vif_entry, *tmp;
+
+	iter_data.rtwdev = rtwdev;
+	INIT_LIST_HEAD(&iter_data.list);
+
+	ieee80211_iterate_active_interfaces_atomic(rtwdev->hw,
+			IEEE80211_IFACE_ITER_NORMAL, rtw_collect_vif_iter, &iter_data);
+
+	list_for_each_entry_safe(vif_entry, tmp, &iter_data.list,
+				 list) {
+		list_del_init(&vif_entry->list);
+		iterator(data, vif_entry->mac, vif_entry->vif);
+		kfree(vif_entry);
+	}
+}
diff --git a/drivers/net/wireless/realtek/rtw88/util.h b/drivers/net/wireless/realtek/rtw88/util.h
index 0c23b5069be0b..dc89655254002 100644
--- a/drivers/net/wireless/realtek/rtw88/util.h
+++ b/drivers/net/wireless/realtek/rtw88/util.h
@@ -7,9 +7,6 @@ 
 
 struct rtw_dev;
 
-#define rtw_iterate_vifs(rtwdev, iterator, data)                               \
-	ieee80211_iterate_active_interfaces(rtwdev->hw,                        \
-			IEEE80211_IFACE_ITER_NORMAL, iterator, data)
 #define rtw_iterate_vifs_atomic(rtwdev, iterator, data)                        \
 	ieee80211_iterate_active_interfaces_atomic(rtwdev->hw,                 \
 			IEEE80211_IFACE_ITER_NORMAL, iterator, data)
@@ -20,6 +17,15 @@  struct rtw_dev;
 #define rtw_iterate_keys_rcu(rtwdev, vif, iterator, data)		       \
 	ieee80211_iter_keys_rcu((rtwdev)->hw, vif, iterator, data)
 
+void rtw_iterate_vifs(struct rtw_dev *rtwdev,
+		      void (*iterator)(void *data, u8 *mac,
+				       struct ieee80211_vif *vif),
+		      void *data);
+void rtw_iterate_stas(struct rtw_dev *rtwdev,
+		      void (*iterator)(void *data,
+				       struct ieee80211_sta *sta),
+				       void *data);
+
 static inline u8 *get_hdr_bssid(struct ieee80211_hdr *hdr)
 {
 	__le16 fc = hdr->frame_control;