[PATCH 0/3] wifi: ath12k: Refactor the hardware recovery procedures

[PATCH 0/3] wifi: ath12k: Refactor the hardware recovery procedures

[Karthikeyan Periyasamy]

Currently, hardware recovery procedure supports multi wiphy model. However,
to support single wiphy model, we need to refactor the hardware recovery
procedure. This patchset allows the logic to work both multi wiphy models
and future single wiphy models.

Karthikeyan Periyasamy (3):
  wifi: ath12k: Refactor the hardware recovery procedure
  wifi: ath12k: Refactor hardware recovery synchronous
  wifi: ath12k: Refactor the hardware state

 drivers/net/wireless/ath/ath12k/core.c | 97 ++++++++++++++------------
 drivers/net/wireless/ath/ath12k/core.h | 25 +++++--
 drivers/net/wireless/ath/ath12k/mac.c  | 95 +++++++++++++++++--------
 drivers/net/wireless/ath/ath12k/reg.c  |  5 +-
 4 files changed, 137 insertions(+), 85 deletions(-)


base-commit: 17f4b952f067b1f87d14e6df4c8c216fe7a245d1
-- 
2.34.1

[PATCH 1/3] wifi: ath12k: Refactor the hardware recovery procedure

[Karthikeyan Periyasamy]

Currently, in multi-wiphy models, the recovery handler access mac80211
HW from the radio/link structure. This will be incorrect for single wiphy
model, as they will hold multiple link/radio structures. To fix this,
access mac80211 HW based on the number of hardware in the SoC/chip. This
approach makes the recovery handler compatible with both multi wiphy and
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 | 119 +++++++++++++------------
 drivers/net/wireless/ath/ath12k/mac.c  |  25 ++++--
 2 files changed, 82 insertions(+), 62 deletions(-)

diff --git a/drivers/net/wireless/ath/ath12k/core.c b/drivers/net/wireless/ath/ath12k/core.c
index 1baad3302157..feca204b82b9 100644
--- a/drivers/net/wireless/ath/ath12k/core.c
+++ b/drivers/net/wireless/ath/ath12k/core.c
@@ -922,9 +922,8 @@ void ath12k_core_halt(struct ath12k *ar)
 static void ath12k_core_pre_reconfigure_recovery(struct ath12k_base *ab)
 {
 	struct ath12k *ar;
-	struct ath12k_pdev *pdev;
 	struct ath12k_hw *ah;
-	int i;
+	int i, j;
 
 	spin_lock_bh(&ab->base_lock);
 	ab->stats.fw_crash_counter++;
@@ -934,34 +933,33 @@ static void ath12k_core_pre_reconfigure_recovery(struct ath12k_base *ab)
 		set_bit(ATH12K_FLAG_CRASH_FLUSH, &ab->dev_flags);
 
 	for (i = 0; i < ab->num_hw; i++) {
-		if (!ab->ah[i])
+		ah = ab->ah[i];
+		if (!ah)
 			continue;
 
-		ah = ab->ah[i];
 		ieee80211_stop_queues(ah->hw);
-	}
 
-	for (i = 0; i < ab->num_radios; i++) {
-		pdev = &ab->pdevs[i];
-		ar = pdev->ar;
-		if (!ar || ar->state == ATH12K_STATE_OFF)
-			continue;
-
-		ath12k_mac_drain_tx(ar);
-		complete(&ar->scan.started);
-		complete(&ar->scan.completed);
-		complete(&ar->peer_assoc_done);
-		complete(&ar->peer_delete_done);
-		complete(&ar->install_key_done);
-		complete(&ar->vdev_setup_done);
-		complete(&ar->vdev_delete_done);
-		complete(&ar->bss_survey_done);
-
-		wake_up(&ar->dp.tx_empty_waitq);
-		idr_for_each(&ar->txmgmt_idr,
-			     ath12k_mac_tx_mgmt_pending_free, ar);
-		idr_destroy(&ar->txmgmt_idr);
-		wake_up(&ar->txmgmt_empty_waitq);
+		for (j = 0; j < ah->num_radio; j++) {
+			ar = &ah->radio[j];
+			if (!ar || ar->state == ATH12K_STATE_OFF)
+				continue;
+
+			ath12k_mac_drain_tx(ar);
+			complete(&ar->scan.started);
+			complete(&ar->scan.completed);
+			complete(&ar->peer_assoc_done);
+			complete(&ar->peer_delete_done);
+			complete(&ar->install_key_done);
+			complete(&ar->vdev_setup_done);
+			complete(&ar->vdev_delete_done);
+			complete(&ar->bss_survey_done);
+
+			wake_up(&ar->dp.tx_empty_waitq);
+			idr_for_each(&ar->txmgmt_idr,
+				     ath12k_mac_tx_mgmt_pending_free, ar);
+			idr_destroy(&ar->txmgmt_idr);
+			wake_up(&ar->txmgmt_empty_waitq);
+		}
 	}
 
 	wake_up(&ab->wmi_ab.tx_credits_wq);
@@ -970,41 +968,52 @@ static void ath12k_core_pre_reconfigure_recovery(struct ath12k_base *ab)
 
 static void ath12k_core_post_reconfigure_recovery(struct ath12k_base *ab)
 {
+	struct ath12k_hw *ah;
 	struct ath12k *ar;
-	struct ath12k_pdev *pdev;
-	int i;
+	int i, j;
+	u8 restart_count;
 
-	for (i = 0; i < ab->num_radios; i++) {
-		pdev = &ab->pdevs[i];
-		ar = pdev->ar;
-		if (!ar || ar->state == ATH12K_STATE_OFF)
+	for (i = 0; i < ab->num_hw; i++) {
+		ah = ab->ah[i];
+		if (!ah)
 			continue;
 
-		mutex_lock(&ar->conf_mutex);
-
-		switch (ar->state) {
-		case ATH12K_STATE_ON:
-			ar->state = ATH12K_STATE_RESTARTING;
-			ath12k_core_halt(ar);
-			ieee80211_restart_hw(ath12k_ar_to_hw(ar));
-			break;
-		case ATH12K_STATE_OFF:
-			ath12k_warn(ab,
-				    "cannot restart radio %d that hasn't been started\n",
-				    i);
-			break;
-		case ATH12K_STATE_RESTARTING:
-			break;
-		case ATH12K_STATE_RESTARTED:
-			ar->state = ATH12K_STATE_WEDGED;
-			fallthrough;
-		case ATH12K_STATE_WEDGED:
-			ath12k_warn(ab,
-				    "device is wedged, will not restart radio %d\n", i);
-			break;
+		for (j = 0, restart_count = 0; j < ah->num_radio; j++) {
+			ar = &ah->radio[j];
+			if (ar->state == ATH12K_STATE_OFF)
+				continue;
+
+			mutex_lock(&ar->conf_mutex);
+
+			switch (ar->state) {
+			case ATH12K_STATE_ON:
+				ar->state = ATH12K_STATE_RESTARTING;
+				ath12k_core_halt(ar);
+				restart_count++;
+				break;
+			case ATH12K_STATE_OFF:
+				ath12k_warn(ab,
+					    "cannot restart radio %d that hasn't been started\n",
+					    j);
+				break;
+			case ATH12K_STATE_RESTARTING:
+				break;
+			case ATH12K_STATE_RESTARTED:
+				ar->state = ATH12K_STATE_WEDGED;
+				fallthrough;
+			case ATH12K_STATE_WEDGED:
+				ath12k_warn(ab,
+					    "device is wedged, will not restart radio %d\n", j);
+				break;
+			}
+			mutex_unlock(&ar->conf_mutex);
 		}
-		mutex_unlock(&ar->conf_mutex);
+
+		/* Restart after all the link/radio got restart */
+		if (restart_count == ah->num_radio)
+			ieee80211_restart_hw(ah->hw);
 	}
+
 	complete(&ab->driver_recovery);
 }
 
diff --git a/drivers/net/wireless/ath/ath12k/mac.c b/drivers/net/wireless/ath/ath12k/mac.c
index a27480a69b27..1b465f7fed7f 100644
--- a/drivers/net/wireless/ath/ath12k/mac.c
+++ b/drivers/net/wireless/ath/ath12k/mac.c
@@ -7114,26 +7114,35 @@ ath12k_mac_op_reconfig_complete(struct ieee80211_hw *hw,
 	struct ath12k *ar;
 	struct ath12k_base *ab;
 	struct ath12k_vif *arvif;
-	int recovery_count;
+	int recovery_count, i;
 
 	if (reconfig_type != IEEE80211_RECONFIG_TYPE_RESTART)
 		return;
 
-	ar = ath12k_ah_to_ar(ah);
-	ab = ar->ab;
+	for (i = 0; i < ah->num_radio; i++) {
+		ar = &ah->radio[i];
 
-	mutex_lock(&ar->conf_mutex);
+		mutex_lock(&ar->conf_mutex);
+
+		if (ar->state != ATH12K_STATE_RESTARTED) {
+			mutex_unlock(&ar->conf_mutex);
+			continue;
+		}
+
+		ab = ar->ab;
 
-	if (ar->state == ATH12K_STATE_RESTARTED) {
 		ath12k_warn(ar->ab, "pdev %d successfully recovered\n",
 			    ar->pdev->pdev_id);
+
 		ar->state = ATH12K_STATE_ON;
 		ieee80211_wake_queues(hw);
 
 		if (ab->is_reset) {
 			recovery_count = atomic_inc_return(&ab->recovery_count);
+
 			ath12k_dbg(ab, ATH12K_DBG_BOOT, "recovery count %d\n",
 				   recovery_count);
+
 			/* When there are multiple radios in an SOC,
 			 * the recovery has to be done for each radio
 			 */
@@ -7152,6 +7161,7 @@ ath12k_mac_op_reconfig_complete(struct ieee80211_hw *hw,
 				   arvif->key_cipher,
 				   arvif->is_up,
 				   arvif->vdev_type);
+
 			/* After trigger disconnect, then upper layer will
 			 * trigger connect again, then the PN number of
 			 * upper layer will be reset to keep up with AP
@@ -7161,13 +7171,14 @@ ath12k_mac_op_reconfig_complete(struct ieee80211_hw *hw,
 			    arvif->vdev_type == WMI_VDEV_TYPE_STA &&
 			    arvif->vdev_subtype == WMI_VDEV_SUBTYPE_NONE) {
 				ieee80211_hw_restart_disconnect(arvif->vif);
+
 				ath12k_dbg(ab, ATH12K_DBG_BOOT,
 					   "restart disconnect\n");
 			}
 		}
-	}
 
-	mutex_unlock(&ar->conf_mutex);
+		mutex_unlock(&ar->conf_mutex);
+	}
 }
 
 static void
-- 
2.34.1

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

[Karthikeyan Periyasamy]

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(-)

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;
-- 
2.34.1

[PATCH 3/3] wifi: ath12k: Refactor the hardware state

[Karthikeyan Periyasamy]

Currently, in multi wiphy models, the mac80211 hardware state is maintained
within the radio/link structure. However, in single wiphy models, the
mac80211 hardware state is needed at the hardware abstraction layer
(ath12k_hw). Therefore, move the hardware state from the radio/link
structure to the hardware abstraction layer (ath12k_hw). Additionally,
update the naming convention of the state enums to enhance clarity and
consistency.

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 | 58 +++++++++------------
 drivers/net/wireless/ath/ath12k/core.h | 20 ++++---
 drivers/net/wireless/ath/ath12k/mac.c  | 72 ++++++++++++++------------
 drivers/net/wireless/ath/ath12k/reg.c  |  5 +-
 4 files changed, 80 insertions(+), 75 deletions(-)

diff --git a/drivers/net/wireless/ath/ath12k/core.c b/drivers/net/wireless/ath/ath12k/core.c
index 4ea5eacc1342..48134aa1dac7 100644
--- a/drivers/net/wireless/ath/ath12k/core.c
+++ b/drivers/net/wireless/ath/ath12k/core.c
@@ -934,15 +934,13 @@ static void ath12k_core_pre_reconfigure_recovery(struct ath12k_base *ab)
 
 	for (i = 0; i < ab->num_hw; i++) {
 		ah = ab->ah[i];
-		if (!ah)
+		if (!ah || ah->state == ATH12K_HW_STATE_OFF)
 			continue;
 
 		ieee80211_stop_queues(ah->hw);
 
 		for (j = 0; j < ah->num_radio; j++) {
 			ar = &ah->radio[j];
-			if (!ar || ar->state == ATH12K_STATE_OFF)
-				continue;
 
 			ath12k_mac_drain_tx(ar);
 			complete(&ar->scan.started);
@@ -971,49 +969,43 @@ static void ath12k_core_post_reconfigure_recovery(struct ath12k_base *ab)
 	struct ath12k_hw *ah;
 	struct ath12k *ar;
 	int i, j;
-	u8 restart_count;
 
 	for (i = 0; i < ab->num_hw; i++) {
 		ah = ab->ah[i];
-		if (!ah)
+		if (!ah || ah->state == ATH12K_HW_STATE_OFF)
 			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)
-				continue;
+		switch (ah->state) {
+		case ATH12K_HW_STATE_ON:
+			ah->state = ATH12K_HW_STATE_RESTARTING;
 
-			mutex_lock(&ar->conf_mutex);
+			for (j = 0; j < ah->num_radio; j++) {
+				ar = &ah->radio[j];
 
-			switch (ar->state) {
-			case ATH12K_STATE_ON:
-				ar->state = ATH12K_STATE_RESTARTING;
+				mutex_lock(&ar->conf_mutex);
 				ath12k_core_halt(ar);
-				restart_count++;
-				break;
-			case ATH12K_STATE_OFF:
-				ath12k_warn(ab,
-					    "cannot restart radio %d that hasn't been started\n",
-					    j);
-				break;
-			case ATH12K_STATE_RESTARTING:
-				break;
-			case ATH12K_STATE_RESTARTED:
-				ar->state = ATH12K_STATE_WEDGED;
-				fallthrough;
-			case ATH12K_STATE_WEDGED:
-				ath12k_warn(ab,
-					    "device is wedged, will not restart radio %d\n", j);
-				break;
+				mutex_unlock(&ar->conf_mutex);
 			}
-			mutex_unlock(&ar->conf_mutex);
-		}
 
-		/* Restart after all the link/radio got restart */
-		if (restart_count == ah->num_radio)
 			ieee80211_restart_hw(ah->hw);
+			break;
+		case ATH12K_HW_STATE_OFF:
+			ath12k_warn(ab,
+				    "cannot restart hw %d that hasn't been started\n",
+				    i);
+			break;
+		case ATH12K_HW_STATE_RESTARTING:
+			break;
+		case ATH12K_HW_STATE_RESTARTED:
+			ah->state = ATH12K_HW_STATE_WEDGED;
+			fallthrough;
+		case ATH12K_HW_STATE_WEDGED:
+			ath12k_warn(ab,
+				    "device is wedged, will not restart hw %d\n", i);
+			break;
+		}
 
 		mutex_unlock(&ah->conf_mutex);
 	}
diff --git a/drivers/net/wireless/ath/ath12k/core.h b/drivers/net/wireless/ath/ath12k/core.h
index 660c1b260201..64d1bd4898e5 100644
--- a/drivers/net/wireless/ath/ath12k/core.h
+++ b/drivers/net/wireless/ath/ath12k/core.h
@@ -430,12 +430,12 @@ struct ath12k_sta {
 #define ATH12K_NUM_CHANS 100
 #define ATH12K_MAX_5G_CHAN 173
 
-enum ath12k_state {
-	ATH12K_STATE_OFF,
-	ATH12K_STATE_ON,
-	ATH12K_STATE_RESTARTING,
-	ATH12K_STATE_RESTARTED,
-	ATH12K_STATE_WEDGED,
+enum ath12k_hw_state {
+	ATH12K_HW_STATE_OFF,
+	ATH12K_HW_STATE_ON,
+	ATH12K_HW_STATE_RESTARTING,
+	ATH12K_HW_STATE_RESTARTED,
+	ATH12K_HW_STATE_WEDGED,
 	/* Add other states as required */
 };
 
@@ -480,7 +480,6 @@ struct ath12k {
 	u32 ht_cap_info;
 	u32 vht_cap_info;
 	struct ath12k_he ar_he;
-	enum ath12k_state state;
 	bool supports_6ghz;
 	struct {
 		struct completion started;
@@ -599,9 +598,11 @@ struct ath12k {
 
 struct ath12k_hw {
 	struct ieee80211_hw *hw;
+	struct ath12k_base *ab;
 
 	/* To synchronize hardware restart operation */
 	struct mutex conf_mutex;
+	enum ath12k_hw_state state;
 
 	u8 num_radio;
 
@@ -934,6 +935,11 @@ static inline struct ath12k *ath12k_ah_to_ar(struct ath12k_hw *ah)
 	return ah->radio;
 }
 
+static inline struct ath12k_hw *ath12k_ar_to_ah(struct ath12k *ar)
+{
+	return ar->ah;
+}
+
 static inline struct ieee80211_hw *ath12k_ar_to_hw(struct ath12k *ar)
 {
 	return ar->ah->hw;
diff --git a/drivers/net/wireless/ath/ath12k/mac.c b/drivers/net/wireless/ath/ath12k/mac.c
index 4c9a591778a2..de6b1f357fbb 100644
--- a/drivers/net/wireless/ath/ath12k/mac.c
+++ b/drivers/net/wireless/ath/ath12k/mac.c
@@ -4808,6 +4808,7 @@ static void ath12k_mac_setup_sband_iftype_data(struct ath12k *ar,
 
 static int __ath12k_set_antenna(struct ath12k *ar, u32 tx_ant, u32 rx_ant)
 {
+	struct ath12k_hw *ah = ath12k_ar_to_ah(ar);
 	int ret;
 
 	lockdep_assert_held(&ar->conf_mutex);
@@ -4821,8 +4822,8 @@ static int __ath12k_set_antenna(struct ath12k *ar, u32 tx_ant, u32 rx_ant)
 	ar->cfg_tx_chainmask = tx_ant;
 	ar->cfg_rx_chainmask = rx_ant;
 
-	if (ar->state != ATH12K_STATE_ON &&
-	    ar->state != ATH12K_STATE_RESTARTED)
+	if (ah->state != ATH12K_HW_STATE_ON &&
+	    ah->state != ATH12K_HW_STATE_RESTARTED)
 		return 0;
 
 	ret = ath12k_wmi_pdev_set_param(ar, WMI_PDEV_PARAM_TX_CHAIN_MASK,
@@ -5120,7 +5121,7 @@ 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_hw *ah = ath12k_ar_to_ah(ar);
 	struct ath12k_base *ab = ar->ab;
 	struct ath12k_pdev *pdev = ar->pdev;
 	int ret;
@@ -5129,22 +5130,6 @@ static int ath12k_mac_start(struct ath12k *ar)
 
 	mutex_lock(&ar->conf_mutex);
 
-	switch (ar->state) {
-	case ATH12K_STATE_OFF:
-		ar->state = ATH12K_STATE_ON;
-		break;
-	case ATH12K_STATE_RESTARTING:
-		ar->state = ATH12K_STATE_RESTARTED;
-		ath12k_mac_wait_reconfigure(ab);
-		break;
-	case ATH12K_STATE_RESTARTED:
-	case ATH12K_STATE_WEDGED:
-	case ATH12K_STATE_ON:
-		WARN_ON(1);
-		ret = -EINVAL;
-		goto err;
-	}
-
 	ret = ath12k_wmi_pdev_set_param(ar, WMI_PDEV_PARAM_PMF_QOS,
 					1, pdev->pdev_id);
 
@@ -5235,7 +5220,6 @@ static int ath12k_mac_start(struct ath12k *ar)
 
 	return 0;
 err:
-	ar->state = ATH12K_STATE_OFF;
 	mutex_unlock(&ar->conf_mutex);
 
 	return ret;
@@ -5252,11 +5236,33 @@ static int ath12k_mac_op_start(struct ieee80211_hw *hw)
 
 	mutex_lock(&ah->conf_mutex);
 
+	switch (ah->state) {
+	case ATH12K_HW_STATE_OFF:
+		ah->state = ATH12K_HW_STATE_ON;
+		break;
+	case ATH12K_HW_STATE_RESTARTING:
+		ah->state = ATH12K_HW_STATE_RESTARTED;
+		ath12k_mac_wait_reconfigure(ah->ab);
+		break;
+	case ATH12K_HW_STATE_RESTARTED:
+	case ATH12K_HW_STATE_WEDGED:
+	case ATH12K_HW_STATE_ON:
+		ah->state = ATH12K_HW_STATE_OFF;
+
+		WARN_ON(1);
+		ret = -EINVAL;
+		goto err;
+	}
+
 	ret = ath12k_mac_start(ar);
-	if (ret)
+	if (ret) {
+		ah->state = ATH12K_HW_STATE_OFF;
+
 		ath12k_err(ab, "fail to start mac operations in pdev idx %d ret %d\n",
 			   ar->pdev_idx, ret);
+	}
 
+err:
 	mutex_unlock(&ah->conf_mutex);
 
 	return ret;
@@ -5321,7 +5327,7 @@ 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 ath12k_hw *ah = ath12k_ar_to_ah(ar);
 	struct htt_ppdu_stats_info *ppdu_stats, *tmp;
 	int ret;
 
@@ -5334,7 +5340,6 @@ static void ath12k_mac_stop(struct ath12k *ar)
 			   ret);
 
 	clear_bit(ATH12K_CAC_RUNNING, &ar->dev_flags);
-	ar->state = ATH12K_STATE_OFF;
 	mutex_unlock(&ar->conf_mutex);
 
 	cancel_delayed_work_sync(&ar->scan.timeout);
@@ -5364,6 +5369,7 @@ static void ath12k_mac_op_stop(struct ieee80211_hw *hw)
 
 	mutex_lock(&ah->conf_mutex);
 
+	ah->state = ATH12K_HW_STATE_OFF;
 	ath12k_mac_stop(ar);
 
 	mutex_unlock(&ah->conf_mutex);
@@ -7133,24 +7139,23 @@ ath12k_mac_op_reconfig_complete(struct ieee80211_hw *hw,
 
 	mutex_lock(&ah->conf_mutex);
 
+	if (ah->state != ATH12K_HW_STATE_RESTARTED) {
+		mutex_unlock(&ah->conf_mutex);
+		return;
+	}
+
+	ah->state = ATH12K_HW_STATE_ON;
+	ieee80211_wake_queues(hw);
+
 	for (i = 0; i < ah->num_radio; i++) {
 		ar = &ah->radio[i];
+		ab = ar->ab;
 
 		mutex_lock(&ar->conf_mutex);
 
-		if (ar->state != ATH12K_STATE_RESTARTED) {
-			mutex_unlock(&ar->conf_mutex);
-			continue;
-		}
-
-		ab = ar->ab;
-
 		ath12k_warn(ar->ab, "pdev %d successfully recovered\n",
 			    ar->pdev->pdev_id);
 
-		ar->state = ATH12K_STATE_ON;
-		ieee80211_wake_queues(hw);
-
 		if (ab->is_reset) {
 			recovery_count = atomic_inc_return(&ab->recovery_count);
 
@@ -7939,6 +7944,7 @@ static struct ath12k_hw *ath12k_mac_hw_allocate(struct ath12k_base *ab,
 
 	ah = ath12k_hw_to_ah(hw);
 	ah->hw = hw;
+	ah->ab = ab;
 	ah->num_radio = num_pdev_map;
 
 	mutex_init(&ah->conf_mutex);
diff --git a/drivers/net/wireless/ath/ath12k/reg.c b/drivers/net/wireless/ath/ath12k/reg.c
index f308e9a6ed55..4fcd6af3696e 100644
--- a/drivers/net/wireless/ath/ath12k/reg.c
+++ b/drivers/net/wireless/ath/ath12k/reg.c
@@ -200,7 +200,8 @@ static void ath12k_copy_regd(struct ieee80211_regdomain *regd_orig,
 
 int ath12k_regd_update(struct ath12k *ar, bool init)
 {
-	struct ieee80211_hw *hw = ath12k_ar_to_hw(ar);
+	struct ath12k_hw *ah = ath12k_ar_to_ah(ar);
+	struct ieee80211_hw *hw = ah->hw;
 	struct ieee80211_regdomain *regd, *regd_copy = NULL;
 	int ret, regd_len, pdev_id;
 	struct ath12k_base *ab;
@@ -258,7 +259,7 @@ int ath12k_regd_update(struct ath12k *ar, bool init)
 	if (ret)
 		goto err;
 
-	if (ar->state == ATH12K_STATE_ON) {
+	if (ah->state == ATH12K_HW_STATE_ON) {
 		ret = ath12k_reg_update_chan_list(ar);
 		if (ret)
 			goto err;
-- 
2.34.1

Re: [PATCH 1/3] wifi: ath12k: Refactor the hardware recovery procedure

[Jeff Johnson]

On 1/29/2024 10:08 PM, Karthikeyan Periyasamy wrote:
> Currently, in multi-wiphy models, the recovery handler access mac80211
> HW from the radio/link structure. This will be incorrect for single wiphy
> model, as they will hold multiple link/radio structures. To fix this,
> access mac80211 HW based on the number of hardware in the SoC/chip. This
> approach makes the recovery handler compatible with both multi wiphy and
> 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>

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

[Jeff Johnson]

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>

Re: [PATCH 3/3] wifi: ath12k: Refactor the hardware state

[Jeff Johnson]

On 1/29/2024 10:08 PM, Karthikeyan Periyasamy wrote:
> Currently, in multi wiphy models, the mac80211 hardware state is maintained
> within the radio/link structure. However, in single wiphy models, the
> mac80211 hardware state is needed at the hardware abstraction layer
> (ath12k_hw). Therefore, move the hardware state from the radio/link
> structure to the hardware abstraction layer (ath12k_hw). Additionally,
> update the naming convention of the state enums to enhance clarity and
> consistency.
> 
> 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>

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

[Kalle Valo]

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.

-- 
https://patchwork.kernel.org/project/linux-wireless/list/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches

Re: [PATCH 0/3] wifi: ath12k: Refactor the hardware recovery procedures

[Jeff Johnson]

On 1/29/2024 10:08 PM, Karthikeyan Periyasamy wrote:
> Currently, hardware recovery procedure supports multi wiphy model. However,
> to support single wiphy model, we need to refactor the hardware recovery
> procedure. This patchset allows the logic to work both multi wiphy models
> and future single wiphy models.
> 
> Karthikeyan Periyasamy (3):
>   wifi: ath12k: Refactor the hardware recovery procedure
>   wifi: ath12k: Refactor hardware recovery synchronous
>   wifi: ath12k: Refactor the hardware state
> 
>  drivers/net/wireless/ath/ath12k/core.c | 97 ++++++++++++++------------
>  drivers/net/wireless/ath/ath12k/core.h | 25 +++++--
>  drivers/net/wireless/ath/ath12k/mac.c  | 95 +++++++++++++++++--------
>  drivers/net/wireless/ath/ath12k/reg.c  |  5 +-
>  4 files changed, 137 insertions(+), 85 deletions(-)
> 
> 
> base-commit: 17f4b952f067b1f87d14e6df4c8c216fe7a245d1

since the "[PATCH 0/8] wifi: ath12k: Introduce device group abstraction"
series depends upon this series, is there a plan to re-spin a new version that
addresses Kalle's concerns?

Re: [PATCH 0/3] wifi: ath12k: Refactor the hardware recovery procedures

[Karthikeyan Periyasamy]

On 4/24/2024 2:25 AM, Jeff Johnson wrote:
> On 1/29/2024 10:08 PM, Karthikeyan Periyasamy wrote:
>> Currently, hardware recovery procedure supports multi wiphy model. However,
>> to support single wiphy model, we need to refactor the hardware recovery
>> procedure. This patchset allows the logic to work both multi wiphy models
>> and future single wiphy models.
>>
>> Karthikeyan Periyasamy (3):
>>    wifi: ath12k: Refactor the hardware recovery procedure
>>    wifi: ath12k: Refactor hardware recovery synchronous
>>    wifi: ath12k: Refactor the hardware state
>>
>>   drivers/net/wireless/ath/ath12k/core.c | 97 ++++++++++++++------------
>>   drivers/net/wireless/ath/ath12k/core.h | 25 +++++--
>>   drivers/net/wireless/ath/ath12k/mac.c  | 95 +++++++++++++++++--------
>>   drivers/net/wireless/ath/ath12k/reg.c  |  5 +-
>>   4 files changed, 137 insertions(+), 85 deletions(-)
>>
>>
>> base-commit: 17f4b952f067b1f87d14e6df4c8c216fe7a245d1
> 
> since the "[PATCH 0/8] wifi: ath12k: Introduce device group abstraction"
> series depends upon this series, is there a plan to re-spin a new version that
> addresses Kalle's concerns?
> 

yes, will respin the next version.

-- 
Karthikeyan Periyasamy
--
கார்த்திகேயன் பெரியசாமி