[PATCH 0/2] wifi: ath10k: Fixed locking bug when setting bitrates.

[PATCH 0/2] wifi: ath10k: Fixed locking bug when setting bitrates.

[Rory Little]

We have noticed our kernel locking up when attempting to set bitrates in the
ath10k. The fault appears to be the sleep calls from sending a wmi command
while an RCU read lock is held. The RCU lock seems unnecessary in this case, as
we are already holding the wiphy mutex at that point.

We have introduced an iterator for this event - when the wiphy mutex is held and
an RCU lock is unnecessary - and used this iterator in the bitrate mask clearing
logic.

Rory Little (2):
  wifi: mac80211: Add non-atomic station iterator.
  wifi: ath10k: Removed atomic iteration in bitrate mask clear.

 drivers/net/wireless/ath/ath10k/mac.c |  6 +++---
 include/net/mac80211.h                | 18 ++++++++++++++++
 net/mac80211/util.c                   | 30 +++++++++++++++++++++++++++
 3 files changed, 51 insertions(+), 3 deletions(-)

-- 
2.34.1

[PATCH 1/2] wifi: mac80211: Add non-atomic station iterator.

[Rory Little]

Drivers may at times want to iterate their stations with a function
which requires some non-atomic operations.

ieee80211_iterate_stations_mtx() introduces an API to iterate stations
while holding that wiphy's mutex. This allows the iterating function to
do non-atomic operations safely.

Signed-off-by: Rory Little <rory@candelatech.com>
---
 include/net/mac80211.h | 18 ++++++++++++++++++
 net/mac80211/util.c    | 30 ++++++++++++++++++++++++++++++
 2 files changed, 48 insertions(+)

diff --git a/include/net/mac80211.h b/include/net/mac80211.h
index e1580360e099..6c8a2dd53243 100644
--- a/include/net/mac80211.h
+++ b/include/net/mac80211.h
@@ -6313,6 +6313,24 @@ void ieee80211_iterate_stations_atomic(struct ieee80211_hw *hw,
 				       void (*iterator)(void *data,
 						struct ieee80211_sta *sta),
 				       void *data);
+
+/**
+ * ieee80211_iterate_stations_mtx - 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 version can only be used while holding the wiphy
+ * mutex.
+ *
+ * @hw: the hardware struct of which the interfaces should be iterated over
+ * @iterator: the iterator function to call
+ * @data: first argument of the iterator function
+ */
+void ieee80211_iterate_stations_mtx(struct ieee80211_hw *hw,
+				    void (*iterator)(void *data,
+						     struct ieee80211_sta *sta),
+				    void *data);
+
 /**
  * ieee80211_queue_work - add work onto the mac80211 workqueue
  *
diff --git a/net/mac80211/util.c b/net/mac80211/util.c
index ef7133ac13f0..e12c871f0477 100644
--- a/net/mac80211/util.c
+++ b/net/mac80211/util.c
@@ -940,6 +940,23 @@ static void __iterate_stations(struct ieee80211_local *local,
 	}
 }
 
+static void __iterate_stations_safe(struct ieee80211_local *local,
+				    void (*iterator)(void *data,
+						     struct ieee80211_sta *sta),
+				    void *data)
+{
+	struct sta_info *sta, *sta_temp;
+
+	lockdep_assert_wiphy(local->hw.wiphy);
+
+	list_for_each_entry_safe(sta, sta_temp, &local->sta_list, list) {
+		if (!sta->uploaded)
+			continue;
+
+		iterator(data, &sta->sta);
+	}
+}
+
 void ieee80211_iterate_stations_atomic(struct ieee80211_hw *hw,
 			void (*iterator)(void *data,
 					 struct ieee80211_sta *sta),
@@ -953,6 +970,19 @@ void ieee80211_iterate_stations_atomic(struct ieee80211_hw *hw,
 }
 EXPORT_SYMBOL_GPL(ieee80211_iterate_stations_atomic);
 
+void ieee80211_iterate_stations_mtx(struct ieee80211_hw *hw,
+				    void (*iterator)(void *data,
+						     struct ieee80211_sta *sta),
+				    void *data)
+{
+	struct ieee80211_local *local = hw_to_local(hw);
+
+	lockdep_assert_wiphy(local->hw.wiphy);
+
+	__iterate_stations_safe(local, iterator, data);
+}
+EXPORT_SYMBOL_GPL(ieee80211_iterate_stations_mtx);
+
 struct ieee80211_vif *wdev_to_ieee80211_vif(struct wireless_dev *wdev)
 {
 	struct ieee80211_sub_if_data *sdata = IEEE80211_WDEV_TO_SUB_IF(wdev);
-- 
2.34.1

[PATCH 2/2] wifi: ath10k: Removed atomic iteration in bitrate mask clear.

[Rory Little]

This operation requires some blocking calls, which causes issues when
attempting to guard this iteration's critical section with an RCU lock.
Instead, we will take advantage of the held wiphy mutex to protect this
operation.

Signed-off-by: Rory Little <rory@candelatech.com>
---
 drivers/net/wireless/ath/ath10k/mac.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/net/wireless/ath/ath10k/mac.c b/drivers/net/wireless/ath/ath10k/mac.c
index 7579a1cd7d15..a1a13b9ad465 100644
--- a/drivers/net/wireless/ath/ath10k/mac.c
+++ b/drivers/net/wireless/ath/ath10k/mac.c
@@ -9502,9 +9502,9 @@ static int ath10k_mac_op_set_bitrate_mask(struct ieee80211_hw *hw,
 			     ar->normal_mode_fw.fw_file.fw_features);
 	if (allow_pfr) {
 		mutex_lock(&ar->conf_mutex);
-		ieee80211_iterate_stations_atomic(ar->hw,
-						  ath10k_mac_clr_bitrate_mask_iter,
-						  arvif);
+		ieee80211_iterate_stations_mtx(ar->hw,
+					       ath10k_mac_clr_bitrate_mask_iter,
+					       arvif);
 		mutex_unlock(&ar->conf_mutex);
 	}
 
-- 
2.34.1

Re: [PATCH 2/2] wifi: ath10k: Removed atomic iteration in bitrate mask clear.

[Jeff Johnson]

On 8/5/24 17:40, Rory Little wrote:
> This operation requires some blocking calls, which causes issues when
> attempting to guard this iteration's critical section with an RCU lock.
> Instead, we will take advantage of the held wiphy mutex to protect this
> operation.
> 
> Signed-off-by: Rory Little <rory@candelatech.com>
> ---
>  drivers/net/wireless/ath/ath10k/mac.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/net/wireless/ath/ath10k/mac.c b/drivers/net/wireless/ath/ath10k/mac.c
> index 7579a1cd7d15..a1a13b9ad465 100644
> --- a/drivers/net/wireless/ath/ath10k/mac.c
> +++ b/drivers/net/wireless/ath/ath10k/mac.c
> @@ -9502,9 +9502,9 @@ static int ath10k_mac_op_set_bitrate_mask(struct ieee80211_hw *hw,
>  			     ar->normal_mode_fw.fw_file.fw_features);
>  	if (allow_pfr) {
>  		mutex_lock(&ar->conf_mutex);
> -		ieee80211_iterate_stations_atomic(ar->hw,
> -						  ath10k_mac_clr_bitrate_mask_iter,
> -						  arvif);
> +		ieee80211_iterate_stations_mtx(ar->hw,
> +					       ath10k_mac_clr_bitrate_mask_iter,
> +					       arvif);
>  		mutex_unlock(&ar->conf_mutex);
>  	}
>  

+ ath10k list

This was deferred back when Kalle was maintainer, and I'm now revisiting the backlog.
Is this still needed? And if so, is there a reason why the other instance of
ieee80211_iterate_stations_atomic() (which sets the mask) does not need to be
modified?

/jeff