diff mbox series

[2/3] wifi: ath12k: Refactor hardware recovery synchronous

Message ID 20240130060838.3895599-3-quic_periyasa@quicinc.com (mailing list archive)
State Changes Requested
Delegated to: Kalle Valo
Headers show
Series wifi: ath12k: Refactor the hardware recovery procedures | expand

Commit Message

Karthikeyan Periyasamy Jan. 30, 2024, 6:08 a.m. UTC
Currently, in multi wiphy models, radio/link level synchronization is
sufficient for MAC hardware (ieee80211_hw) reconfigure/recovery procedures
since each radio/link is exposed as a MAC hardware (ieee80211_hw). However,
in single wiphy models, multiple radio/links is exposed as a MAC hardware
(ieee80211_hw) through the driver hardware abstraction (ath12k_hw) layer.
In such scenario, we need synchronization between the reconfigure/recovery
callback operations (i.e., ath12k_core_post_reconfigure_recovery(),
ieee80211_ops->start(), ieee80211_ops->reconfig_complete(),
ieee80211_ops->stop()) at the driver hardware abstraction (ath12k_hw) layer
instead of radio/link (ath12k) layer. Therefore, introduce a new mutex in
the ath12k_hw layer. This approach ensures compatibility of the hardware
recovery procedure with both multi wiphy models and future single wiphy
models.

Tested-on: QCN9274 hw2.0 PCI WLAN.WBE.1.0.1-00029-QCAHKSWPL_SILICONZ-1

Signed-off-by: Karthikeyan Periyasamy <quic_periyasa@quicinc.com>
---
 drivers/net/wireless/ath/ath12k/core.c |  4 ++++
 drivers/net/wireless/ath/ath12k/core.h |  5 +++++
 drivers/net/wireless/ath/ath12k/mac.c  | 26 ++++++++++++++++++++++----
 3 files changed, 31 insertions(+), 4 deletions(-)

Comments

Jeff Johnson Jan. 30, 2024, 7:39 p.m. UTC | #1
On 1/29/2024 10:08 PM, Karthikeyan Periyasamy wrote:
> Currently, in multi wiphy models, radio/link level synchronization is
> sufficient for MAC hardware (ieee80211_hw) reconfigure/recovery procedures
> since each radio/link is exposed as a MAC hardware (ieee80211_hw). However,
> in single wiphy models, multiple radio/links is exposed as a MAC hardware
> (ieee80211_hw) through the driver hardware abstraction (ath12k_hw) layer.
> In such scenario, we need synchronization between the reconfigure/recovery
> callback operations (i.e., ath12k_core_post_reconfigure_recovery(),
> ieee80211_ops->start(), ieee80211_ops->reconfig_complete(),
> ieee80211_ops->stop()) at the driver hardware abstraction (ath12k_hw) layer
> instead of radio/link (ath12k) layer. Therefore, introduce a new mutex in
> the ath12k_hw layer. This approach ensures compatibility of the hardware
> recovery procedure with both multi wiphy models and future single wiphy
> models.
> 
> Tested-on: QCN9274 hw2.0 PCI WLAN.WBE.1.0.1-00029-QCAHKSWPL_SILICONZ-1
> 
> Signed-off-by: Karthikeyan Periyasamy <quic_periyasa@quicinc.com>
Acked-by: Jeff Johnson <quic_jjohnson@quicinc.com>
Kalle Valo Feb. 9, 2024, 10:12 a.m. UTC | #2
Karthikeyan Periyasamy <quic_periyasa@quicinc.com> writes:

> Currently, in multi wiphy models, radio/link level synchronization is
> sufficient for MAC hardware (ieee80211_hw) reconfigure/recovery procedures
> since each radio/link is exposed as a MAC hardware (ieee80211_hw). However,
> in single wiphy models, multiple radio/links is exposed as a MAC hardware
> (ieee80211_hw) through the driver hardware abstraction (ath12k_hw) layer.
> In such scenario, we need synchronization between the reconfigure/recovery
> callback operations (i.e., ath12k_core_post_reconfigure_recovery(),
> ieee80211_ops->start(), ieee80211_ops->reconfig_complete(),
> ieee80211_ops->stop()) at the driver hardware abstraction (ath12k_hw) layer
> instead of radio/link (ath12k) layer. Therefore, introduce a new mutex in
> the ath12k_hw layer. This approach ensures compatibility of the hardware
> recovery procedure with both multi wiphy models and future single wiphy
> models.
>
> Tested-on: QCN9274 hw2.0 PCI WLAN.WBE.1.0.1-00029-QCAHKSWPL_SILICONZ-1
>
> Signed-off-by: Karthikeyan Periyasamy <quic_periyasa@quicinc.com>

[...]

> --- a/drivers/net/wireless/ath/ath12k/core.h
> +++ b/drivers/net/wireless/ath/ath12k/core.h
> @@ -600,7 +600,12 @@ struct ath12k {
>  struct ath12k_hw {
>  	struct ieee80211_hw *hw;
>  
> +	/* To synchronize hardware restart operation */
> +	struct mutex conf_mutex;

As we discussed already offline, there's a high bar for adding new
mutexes. I would rather remove the existing conf_mutex than add a new
one.

Also having two mutexes named 'conf_mutex' in the same driver is risky,
it would be very easy to confuse the two.
diff mbox series

Patch

diff --git a/drivers/net/wireless/ath/ath12k/core.c b/drivers/net/wireless/ath/ath12k/core.c
index feca204b82b9..4ea5eacc1342 100644
--- a/drivers/net/wireless/ath/ath12k/core.c
+++ b/drivers/net/wireless/ath/ath12k/core.c
@@ -978,6 +978,8 @@  static void ath12k_core_post_reconfigure_recovery(struct ath12k_base *ab)
 		if (!ah)
 			continue;
 
+		mutex_lock(&ah->conf_mutex);
+
 		for (j = 0, restart_count = 0; j < ah->num_radio; j++) {
 			ar = &ah->radio[j];
 			if (ar->state == ATH12K_STATE_OFF)
@@ -1012,6 +1014,8 @@  static void ath12k_core_post_reconfigure_recovery(struct ath12k_base *ab)
 		/* Restart after all the link/radio got restart */
 		if (restart_count == ah->num_radio)
 			ieee80211_restart_hw(ah->hw);
+
+		mutex_unlock(&ah->conf_mutex);
 	}
 
 	complete(&ab->driver_recovery);
diff --git a/drivers/net/wireless/ath/ath12k/core.h b/drivers/net/wireless/ath/ath12k/core.h
index 5c6c1e2eddb6..660c1b260201 100644
--- a/drivers/net/wireless/ath/ath12k/core.h
+++ b/drivers/net/wireless/ath/ath12k/core.h
@@ -600,7 +600,12 @@  struct ath12k {
 struct ath12k_hw {
 	struct ieee80211_hw *hw;
 
+	/* To synchronize hardware restart operation */
+	struct mutex conf_mutex;
+
 	u8 num_radio;
+
+	/* Keep last */
 	struct ath12k radio[] __aligned(sizeof(void *));
 };
 
diff --git a/drivers/net/wireless/ath/ath12k/mac.c b/drivers/net/wireless/ath/ath12k/mac.c
index 1b465f7fed7f..4c9a591778a2 100644
--- a/drivers/net/wireless/ath/ath12k/mac.c
+++ b/drivers/net/wireless/ath/ath12k/mac.c
@@ -5120,10 +5120,13 @@  static void ath12k_mac_wait_reconfigure(struct ath12k_base *ab)
 
 static int ath12k_mac_start(struct ath12k *ar)
 {
+	struct ath12k_hw *ah = ar->ah;
 	struct ath12k_base *ab = ar->ab;
 	struct ath12k_pdev *pdev = ar->pdev;
 	int ret;
 
+	lockdep_assert_held(&ah->conf_mutex);
+
 	mutex_lock(&ar->conf_mutex);
 
 	switch (ar->state) {
@@ -5247,14 +5250,16 @@  static int ath12k_mac_op_start(struct ieee80211_hw *hw)
 
 	ath12k_mac_drain_tx(ar);
 
+	mutex_lock(&ah->conf_mutex);
+
 	ret = ath12k_mac_start(ar);
-	if (ret) {
+	if (ret)
 		ath12k_err(ab, "fail to start mac operations in pdev idx %d ret %d\n",
 			   ar->pdev_idx, ret);
-		return ret;
-	}
 
-	return 0;
+	mutex_unlock(&ah->conf_mutex);
+
+	return ret;
 }
 
 int ath12k_mac_rfkill_config(struct ath12k *ar)
@@ -5316,9 +5321,12 @@  int ath12k_mac_rfkill_enable_radio(struct ath12k *ar, bool enable)
 
 static void ath12k_mac_stop(struct ath12k *ar)
 {
+	struct ath12k_hw *ah = ar->ah;
 	struct htt_ppdu_stats_info *ppdu_stats, *tmp;
 	int ret;
 
+	lockdep_assert_held(&ah->conf_mutex);
+
 	mutex_lock(&ar->conf_mutex);
 	ret = ath12k_mac_config_mon_status_default(ar, false);
 	if (ret && (ret != -EOPNOTSUPP))
@@ -5354,7 +5362,11 @@  static void ath12k_mac_op_stop(struct ieee80211_hw *hw)
 
 	ath12k_mac_drain_tx(ar);
 
+	mutex_lock(&ah->conf_mutex);
+
 	ath12k_mac_stop(ar);
+
+	mutex_unlock(&ah->conf_mutex);
 }
 
 static u8
@@ -7119,6 +7131,8 @@  ath12k_mac_op_reconfig_complete(struct ieee80211_hw *hw,
 	if (reconfig_type != IEEE80211_RECONFIG_TYPE_RESTART)
 		return;
 
+	mutex_lock(&ah->conf_mutex);
+
 	for (i = 0; i < ah->num_radio; i++) {
 		ar = &ah->radio[i];
 
@@ -7179,6 +7193,8 @@  ath12k_mac_op_reconfig_complete(struct ieee80211_hw *hw,
 
 		mutex_unlock(&ar->conf_mutex);
 	}
+
+	mutex_unlock(&ah->conf_mutex);
 }
 
 static void
@@ -7925,6 +7941,8 @@  static struct ath12k_hw *ath12k_mac_hw_allocate(struct ath12k_base *ab,
 	ah->hw = hw;
 	ah->num_radio = num_pdev_map;
 
+	mutex_init(&ah->conf_mutex);
+
 	for (i = 0; i < num_pdev_map; i++) {
 		ab = pdev_map[i].ab;
 		pdev_idx = pdev_map[i].pdev_idx;