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

[PATCH v2 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.

v2:
 - Rebased to ToT
 - Renamed the lock name as per the kalle comments

Karthikeyan Periyasamy (3):
  wifi: ath12k: Refactor the hardware recovery procedure
  wifi: ath12k: Refactor the hardware state
  wifi: ath12k: Add lock to protect the hardware state

 drivers/net/wireless/ath/ath12k/core.c | 98 ++++++++++++++------------
 drivers/net/wireless/ath/ath12k/core.h | 28 +++++---
 drivers/net/wireless/ath/ath12k/mac.c  | 95 +++++++++++++++++--------
 drivers/net/wireless/ath/ath12k/reg.c  | 19 ++---
 4 files changed, 149 insertions(+), 91 deletions(-)


base-commit: 1b61047b44218a00c7a7836eff4f8e037d5634d8
-- 
2.34.1

[PATCH v2 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>
Acked-by: Jeff Johnson <quic_jjohnson@quicinc.com>
---
 drivers/net/wireless/ath/ath12k/core.c | 119 +++++++++++++------------
 drivers/net/wireless/ath/ath12k/mac.c  |  23 +++--
 2 files changed, 80 insertions(+), 62 deletions(-)

diff --git a/drivers/net/wireless/ath/ath12k/core.c b/drivers/net/wireless/ath/ath12k/core.c
index 6663f4e1792d..5fed86a0cedf 100644
--- a/drivers/net/wireless/ath/ath12k/core.c
+++ b/drivers/net/wireless/ath/ath12k/core.c
@@ -994,9 +994,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++;
@@ -1006,35 +1005,34 @@ 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;
+		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->scan.on_channel);
-		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);
+			ath12k_mac_drain_tx(ar);
+			complete(&ar->scan.started);
+			complete(&ar->scan.completed);
+			complete(&ar->scan.on_channel);
+			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);
@@ -1043,41 +1041,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 f6cf8b8c4b18..ba594d87e26e 100644
--- a/drivers/net/wireless/ath/ath12k/mac.c
+++ b/drivers/net/wireless/ath/ath12k/mac.c
@@ -7837,26 +7837,33 @@ 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, 0);
-	ab = ar->ab;
+	for_each_ar(ah, ar, 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
 			 */
@@ -7875,6 +7882,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
@@ -7884,13 +7892,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

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

[Jeff Johnson]

On 4/23/2024 11:56 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>
[...]
> @@ -1006,35 +1005,34 @@ 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;
> +		for (j = 0; j < ah->num_radio; j++) {
> +			ar = &ah->radio[j];
> +			if (!ar || ar->state == ATH12K_STATE_OFF)

remove !ar test, result of & operation can't be NULL

> +				continue;

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

[Karthikeyan Periyasamy]

On 4/25/2024 1:50 AM, Jeff Johnson wrote:
> On 4/23/2024 11:56 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>
> [...]
>> @@ -1006,35 +1005,34 @@ 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;
>> +		for (j = 0; j < ah->num_radio; j++) {
>> +			ar = &ah->radio[j];
>> +			if (!ar || ar->state == ATH12K_STATE_OFF)
> 
> remove !ar test, result of & operation can't be NULL
> 

sure, will address this comment in the next version of the patch.


>> +				continue;
> 

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

[PATCH v2 2/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 | 59 +++++++++++--------------
 drivers/net/wireless/ath/ath12k/core.h | 22 ++++++----
 drivers/net/wireless/ath/ath12k/mac.c  | 61 ++++++++++++++------------
 drivers/net/wireless/ath/ath12k/reg.c  | 19 ++++----
 4 files changed, 83 insertions(+), 78 deletions(-)

diff --git a/drivers/net/wireless/ath/ath12k/core.c b/drivers/net/wireless/ath/ath12k/core.c
index 5fed86a0cedf..a685cfd6fd92 100644
--- a/drivers/net/wireless/ath/ath12k/core.c
+++ b/drivers/net/wireless/ath/ath12k/core.c
@@ -1006,15 +1006,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);
@@ -1044,47 +1042,42 @@ 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;
 
-		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)
+			/* Restart after all the link/radio halt */
 			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;
+		}
 	}
 
 	complete(&ab->driver_recovery);
diff --git a/drivers/net/wireless/ath/ath12k/core.h b/drivers/net/wireless/ath/ath12k/core.h
index eb6dd5551f20..eff1093fbb6e 100644
--- a/drivers/net/wireless/ath/ath12k/core.h
+++ b/drivers/net/wireless/ath/ath12k/core.h
@@ -455,12 +455,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 */
 };
 
@@ -509,7 +509,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;
@@ -634,10 +633,12 @@ struct ath12k {
 
 struct ath12k_hw {
 	struct ieee80211_hw *hw;
+	struct ath12k_base *ab;
+	enum ath12k_hw_state state;
 	bool regd_updated;
 	bool use_6ghz_regd;
-
 	u8 num_radio;
+
 	struct ath12k radio[] __aligned(sizeof(void *));
 };
 
@@ -1035,6 +1036,11 @@ static inline struct ath12k *ath12k_ah_to_ar(struct ath12k_hw *ah, u8 hw_link_id
 	return &ah->radio[hw_link_id];
 }
 
+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 ba594d87e26e..8b003c18796d 100644
--- a/drivers/net/wireless/ath/ath12k/mac.c
+++ b/drivers/net/wireless/ath/ath12k/mac.c
@@ -5200,6 +5200,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);
@@ -5220,8 +5221,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,
@@ -5551,22 +5552,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);
 
@@ -5657,7 +5642,6 @@ static int ath12k_mac_start(struct ath12k *ar)
 
 	return 0;
 err:
-	ar->state = ATH12K_STATE_OFF;
 	mutex_unlock(&ar->conf_mutex);
 
 	return ret;
@@ -5680,9 +5664,28 @@ static int ath12k_mac_op_start(struct ieee80211_hw *hw)
 
 	ath12k_drain_tx(ah);
 
+	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);
+		return -EINVAL;
+	}
+
 	for_each_ar(ah, ar, i) {
 		ret = ath12k_mac_start(ar);
 		if (ret) {
+			ah->state = ATH12K_HW_STATE_OFF;
+
 			ath12k_err(ar->ab, "fail to start mac operations in pdev idx %d ret %d\n",
 				   ar->pdev_idx, ret);
 			goto fail_start;
@@ -5690,11 +5693,13 @@ static int ath12k_mac_op_start(struct ieee80211_hw *hw)
 	}
 
 	return 0;
+
 fail_start:
 	for (; i > 0; i--) {
 		ar = ath12k_ah_to_ar(ah, i - 1);
 		ath12k_mac_stop(ar);
 	}
+
 	return ret;
 }
 
@@ -5767,7 +5772,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);
@@ -5796,6 +5800,8 @@ static void ath12k_mac_op_stop(struct ieee80211_hw *hw)
 
 	ath12k_drain_tx(ah);
 
+	ah->state = ATH12K_HW_STATE_OFF;
+
 	for_each_ar(ah, ar, i)
 		ath12k_mac_stop(ar);
 }
@@ -7842,22 +7848,20 @@ ath12k_mac_op_reconfig_complete(struct ieee80211_hw *hw,
 	if (reconfig_type != IEEE80211_RECONFIG_TYPE_RESTART)
 		return;
 
+	if (ah->state != ATH12K_HW_STATE_RESTARTED)
+		return;
+
+	ah->state = ATH12K_HW_STATE_ON;
+	ieee80211_wake_queues(hw);
+
 	for_each_ar(ah, ar, i) {
 		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);
 
@@ -8843,6 +8847,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;
 
 	for (i = 0; i < num_pdev_map; i++) {
diff --git a/drivers/net/wireless/ath/ath12k/reg.c b/drivers/net/wireless/ath/ath12k/reg.c
index fbf38044938c..439d61f284d8 100644
--- a/drivers/net/wireless/ath/ath12k/reg.c
+++ b/drivers/net/wireless/ath/ath12k/reg.c
@@ -206,9 +206,9 @@ 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;
-	struct ath12k_hw *ah = ar->ah;
 	int ret, regd_len, pdev_id;
 	struct ath12k_base *ab;
 	int i;
@@ -286,19 +286,20 @@ int ath12k_regd_update(struct ath12k *ar, bool init)
 	if (ret)
 		goto err;
 
+	if (ah->state != ATH12K_HW_STATE_ON)
+		goto skip;
+
 	ah->regd_updated = true;
 	/* Apply the new regd to all the radios, this is expected to be received only once
 	 * since we check for ah->regd_updated and allow here only once.
 	 */
 	for_each_ar(ah, ar, i) {
-		if (ar->state == ATH12K_STATE_ON) {
-			ab = ar->ab;
-			ret = ath12k_reg_update_chan_list(ar);
-			if (ret)
-				goto err;
-		}
+		ab = ar->ab;
+		ret = ath12k_reg_update_chan_list(ar);
+		if (ret)
+			goto err;
 	}
-
+skip:
 	return 0;
 err:
 	ath12k_warn(ab, "failed to perform regd update : %d\n", ret);
-- 
2.34.1

[PATCH v2 3/3] wifi: ath12k: Add lock to protect the hardware state

[Karthikeyan Periyasamy]

Currently, hardware state is not protected across the reconfigure
operations. 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 to protect
hardware state across the multiple radio/link at the driver hardware
abstraction (ath12k_hw) layer. Therefore, introduce a new mutex in
the ath12k_hw layer.

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 |  6 ++++++
 drivers/net/wireless/ath/ath12k/mac.c  | 29 ++++++++++++++++++++++++--
 3 files changed, 37 insertions(+), 2 deletions(-)

diff --git a/drivers/net/wireless/ath/ath12k/core.c b/drivers/net/wireless/ath/ath12k/core.c
index a685cfd6fd92..e9aabdb9341c 100644
--- a/drivers/net/wireless/ath/ath12k/core.c
+++ b/drivers/net/wireless/ath/ath12k/core.c
@@ -1048,6 +1048,8 @@ static void ath12k_core_post_reconfigure_recovery(struct ath12k_base *ab)
 		if (!ah || ah->state == ATH12K_HW_STATE_OFF)
 			continue;
 
+		mutex_lock(&ah->hw_mutex);
+
 		switch (ah->state) {
 		case ATH12K_HW_STATE_ON:
 			ah->state = ATH12K_HW_STATE_RESTARTING;
@@ -1078,6 +1080,8 @@ static void ath12k_core_post_reconfigure_recovery(struct ath12k_base *ab)
 				    "device is wedged, will not restart hw %d\n", i);
 			break;
 		}
+
+		mutex_unlock(&ah->hw_mutex);
 	}
 
 	complete(&ab->driver_recovery);
diff --git a/drivers/net/wireless/ath/ath12k/core.h b/drivers/net/wireless/ath/ath12k/core.h
index eff1093fbb6e..3e3e157b6c56 100644
--- a/drivers/net/wireless/ath/ath12k/core.h
+++ b/drivers/net/wireless/ath/ath12k/core.h
@@ -634,11 +634,17 @@ struct ath12k {
 struct ath12k_hw {
 	struct ieee80211_hw *hw;
 	struct ath12k_base *ab;
+
+	/* Protect the write operation of the hardware state ath12k_hw::state
+	 * between hardware start<=>reconfigure<=>stop transitions.
+	 */
+	struct mutex hw_mutex;
 	enum ath12k_hw_state state;
 	bool regd_updated;
 	bool use_6ghz_regd;
 	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 8b003c18796d..3dc95d36b6a2 100644
--- a/drivers/net/wireless/ath/ath12k/mac.c
+++ b/drivers/net/wireless/ath/ath12k/mac.c
@@ -5546,10 +5546,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->hw_mutex);
+
 	mutex_lock(&ar->conf_mutex);
 
 	ret = ath12k_wmi_pdev_set_param(ar, WMI_PDEV_PARAM_PMF_QOS,
@@ -5664,6 +5667,8 @@ static int ath12k_mac_op_start(struct ieee80211_hw *hw)
 
 	ath12k_drain_tx(ah);
 
+	mutex_lock(&ah->hw_mutex);
+
 	switch (ah->state) {
 	case ATH12K_HW_STATE_OFF:
 		ah->state = ATH12K_HW_STATE_ON;
@@ -5678,7 +5683,8 @@ static int ath12k_mac_op_start(struct ieee80211_hw *hw)
 		ah->state = ATH12K_HW_STATE_OFF;
 
 		WARN_ON(1);
-		return -EINVAL;
+		ret = -EINVAL;
+		goto err;
 	}
 
 	for_each_ar(ah, ar, i) {
@@ -5692,6 +5698,8 @@ static int ath12k_mac_op_start(struct ieee80211_hw *hw)
 		}
 	}
 
+	mutex_unlock(&ah->hw_mutex);
+
 	return 0;
 
 fail_start:
@@ -5700,6 +5708,8 @@ static int ath12k_mac_op_start(struct ieee80211_hw *hw)
 		ath12k_mac_stop(ar);
 	}
 
+err:
+	mutex_unlock(&ah->hw_mutex);
 	return ret;
 }
 
@@ -5762,9 +5772,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->hw_mutex);
+
 	mutex_lock(&ar->conf_mutex);
 	ret = ath12k_mac_config_mon_status_default(ar, false);
 	if (ret && (ret != -EOPNOTSUPP))
@@ -5800,10 +5813,14 @@ static void ath12k_mac_op_stop(struct ieee80211_hw *hw)
 
 	ath12k_drain_tx(ah);
 
+	mutex_lock(&ah->hw_mutex);
+
 	ah->state = ATH12K_HW_STATE_OFF;
 
 	for_each_ar(ah, ar, i)
 		ath12k_mac_stop(ar);
+
+	mutex_unlock(&ah->hw_mutex);
 }
 
 static u8
@@ -7848,8 +7865,12 @@ ath12k_mac_op_reconfig_complete(struct ieee80211_hw *hw,
 	if (reconfig_type != IEEE80211_RECONFIG_TYPE_RESTART)
 		return;
 
-	if (ah->state != ATH12K_HW_STATE_RESTARTED)
+	mutex_lock(&ah->hw_mutex);
+
+	if (ah->state != ATH12K_HW_STATE_RESTARTED) {
+		mutex_unlock(&ah->hw_mutex);
 		return;
+	}
 
 	ah->state = ATH12K_HW_STATE_ON;
 	ieee80211_wake_queues(hw);
@@ -7904,6 +7925,8 @@ ath12k_mac_op_reconfig_complete(struct ieee80211_hw *hw,
 
 		mutex_unlock(&ar->conf_mutex);
 	}
+
+	mutex_unlock(&ah->hw_mutex);
 }
 
 static void
@@ -8850,6 +8873,8 @@ static struct ath12k_hw *ath12k_mac_hw_allocate(struct ath12k_base *ab,
 	ah->ab = ab;
 	ah->num_radio = num_pdev_map;
 
+	mutex_init(&ah->hw_mutex);
+
 	for (i = 0; i < num_pdev_map; i++) {
 		ab = pdev_map[i].ab;
 		pdev_idx = pdev_map[i].pdev_idx;
-- 
2.34.1

Re: [PATCH v2 3/3] wifi: ath12k: Add lock to protect the hardware state

[Johannes Berg]

On Wed, 2024-04-24 at 12:26 +0530, Karthikeyan Periyasamy wrote:
> Currently, hardware state is not protected across the reconfigure
> operations. 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 to protect
> hardware state across the multiple radio/link at the driver hardware
> abstraction (ath12k_hw) layer. Therefore, introduce a new mutex in
> the ath12k_hw layer.
> 

It's your driver, but ... it would seem _simpler_ to do locking across
the hw with a single wiphy model, because everything (except currently
for ath12k_core_reset and ath12k_core_restart) already holds the wiphy
mutex. You can probably move those to wiphy work.

I'd avoid doing lock explosion like we had in mac80211, it's going to
come back and bite you eventually :)

johannes

Re: [PATCH v2 3/3] wifi: ath12k: Add lock to protect the hardware state

[Kalle Valo]

Johannes Berg <johannes@sipsolutions.net> writes:

> On Wed, 2024-04-24 at 12:26 +0530, Karthikeyan Periyasamy wrote:
>> Currently, hardware state is not protected across the reconfigure
>> operations. 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 to protect
>> hardware state across the multiple radio/link at the driver hardware
>> abstraction (ath12k_hw) layer. Therefore, introduce a new mutex in
>> the ath12k_hw layer.
>> 
>
> It's your driver, but ... it would seem _simpler_ to do locking across
> the hw with a single wiphy model, because everything (except currently
> for ath12k_core_reset and ath12k_core_restart) already holds the wiphy
> mutex. You can probably move those to wiphy work.
>
> I'd avoid doing lock explosion like we had in mac80211, it's going to
> come back and bite you eventually :)

Exactly. Swithing to use wiphy_lock() definitely one of my longterm
goals in ath12k. I already started working on switching ath12k to use
wiphy_lock() but IIRC I got blocked by not being able to call
wiphy_delayed_work_cancel() without taking wiphy_lock. One way to solve
that might be to convert WMI event handling to use wiphy work but didn't
have the time to work on that so I put it on the backborner. I should
resurrect that branch and submit it as RFC.

But for the time being I think we need to add another mutex to ath12k, I
don't want to block Karthikeyan for too long.

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

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

Re: [PATCH v2 3/3] wifi: ath12k: Add lock to protect the hardware state

[Johannes Berg]

On Wed, 2024-04-24 at 12:39 +0300, Kalle Valo wrote:
> 
> Exactly. Swithing to use wiphy_lock() definitely one of my longterm
> goals in ath12k. I already started working on switching ath12k to use
> wiphy_lock() but IIRC I got blocked by not being able to call
> wiphy_delayed_work_cancel() without taking wiphy_lock.

That's because I didn't want to have an async version, but theoretically
we could have a version of it that just cancels the timer. If you don't
hold the wiphy mutex already you could even wait for it to finish. It
just adds more complexity there, and I was trying to make it all a lot
more obvious :)

johannes

Re: [PATCH v2 3/3] wifi: ath12k: Add lock to protect the hardware state

[Kalle Valo]

Johannes Berg <johannes@sipsolutions.net> writes:

> On Wed, 2024-04-24 at 12:39 +0300, Kalle Valo wrote:
>> 
>> Exactly. Swithing to use wiphy_lock() definitely one of my longterm
>> goals in ath12k. I already started working on switching ath12k to use
>> wiphy_lock() but IIRC I got blocked by not being able to call
>> wiphy_delayed_work_cancel() without taking wiphy_lock.
>
> That's because I didn't want to have an async version, but theoretically
> we could have a version of it that just cancels the timer. If you don't
> hold the wiphy mutex already you could even wait for it to finish. It
> just adds more complexity there, and I was trying to make it all a lot
> more obvious :)

Yeah, understandable. I think changing ath12k WMI event handling to use
wiphy_work makes sense, running them in tasklet context feels overkill.
I just need to write the patch :) But after that I hope we can switch to
using wiphy mutex.

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

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

Re: [PATCH v2 3/3] wifi: ath12k: Add lock to protect the hardware state

[Johannes Berg]

On Wed, 2024-04-24 at 12:50 +0300, Kalle Valo wrote:
> Johannes Berg <johannes@sipsolutions.net> writes:
> 
> > On Wed, 2024-04-24 at 12:39 +0300, Kalle Valo wrote:
> > > 
> > > Exactly. Swithing to use wiphy_lock() definitely one of my longterm
> > > goals in ath12k. I already started working on switching ath12k to use
> > > wiphy_lock() but IIRC I got blocked by not being able to call
> > > wiphy_delayed_work_cancel() without taking wiphy_lock.
> > 
> > That's because I didn't want to have an async version, but theoretically
> > we could have a version of it that just cancels the timer. If you don't
> > hold the wiphy mutex already you could even wait for it to finish. It
> > just adds more complexity there, and I was trying to make it all a lot
> > more obvious :)
> 
> Yeah, understandable. I think changing ath12k WMI event handling to use
> wiphy_work makes sense, running them in tasklet context feels overkill.

Maybe. Note that the wiphy lock _can_ create delays etc. here, if you're
doing other configuration work at the same time, or similar. Though I
guess eventually you need locking there anyway, to access the HW. So it
can be a bit of a trade-off.

I expect this will evolve over time even in mac80211, though so far we
haven't seen any bad effect from it.

johannes

Re: [PATCH v2 3/3] wifi: ath12k: Add lock to protect the hardware state

[Kalle Valo]

Johannes Berg <johannes@sipsolutions.net> writes:

> On Wed, 2024-04-24 at 12:50 +0300, Kalle Valo wrote:
>> Johannes Berg <johannes@sipsolutions.net> writes:
>> 
>> > On Wed, 2024-04-24 at 12:39 +0300, Kalle Valo wrote:
>> > > 
>> > > Exactly. Swithing to use wiphy_lock() definitely one of my longterm
>> > > goals in ath12k. I already started working on switching ath12k to use
>> > > wiphy_lock() but IIRC I got blocked by not being able to call
>> > > wiphy_delayed_work_cancel() without taking wiphy_lock.
>> > 
>> > That's because I didn't want to have an async version, but theoretically
>> > we could have a version of it that just cancels the timer. If you don't
>> > hold the wiphy mutex already you could even wait for it to finish. It
>> > just adds more complexity there, and I was trying to make it all a lot
>> > more obvious :)
>> 
>> Yeah, understandable. I think changing ath12k WMI event handling to use
>> wiphy_work makes sense, running them in tasklet context feels overkill.
>
> Maybe. Note that the wiphy lock _can_ create delays etc. here, if you're
> doing other configuration work at the same time, or similar. Though I
> guess eventually you need locking there anyway, to access the HW. So it
> can be a bit of a trade-off.
>
> I expect this will evolve over time even in mac80211, though so far we
> haven't seen any bad effect from it.

Good point. And now that I think of this more I don't think we can use
wiphy_work with WMI events as we are waiting some events while holding
wiphy_lock, that would end up as deadlock. So most likely we need to use
a normal workqueue with WMI events and take wiphy_lock manually in
certain cases. But I'll need to investigate more and do some
experiments.

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

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

Re: [PATCH v2 3/3] wifi: ath12k: Add lock to protect the hardware state

[Jeff Johnson]

On 4/23/2024 11:56 PM, Karthikeyan Periyasamy wrote:
> Currently, hardware state is not protected across the reconfigure
> operations. 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 to protect
> hardware state across the multiple radio/link at the driver hardware
> abstraction (ath12k_hw) layer. Therefore, introduce a new mutex in
> the ath12k_hw layer.
> 
> 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 |  6 ++++++
>  drivers/net/wireless/ath/ath12k/mac.c  | 29 ++++++++++++++++++++++++--
>  3 files changed, 37 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/wireless/ath/ath12k/core.c b/drivers/net/wireless/ath/ath12k/core.c
> index a685cfd6fd92..e9aabdb9341c 100644
> --- a/drivers/net/wireless/ath/ath12k/core.c
> +++ b/drivers/net/wireless/ath/ath12k/core.c
> @@ -1048,6 +1048,8 @@ static void ath12k_core_post_reconfigure_recovery(struct ath12k_base *ab)
>  		if (!ah || ah->state == ATH12K_HW_STATE_OFF)
>  			continue;
>  
> +		mutex_lock(&ah->hw_mutex);
> +
>  		switch (ah->state) {
>  		case ATH12K_HW_STATE_ON:
>  			ah->state = ATH12K_HW_STATE_RESTARTING;
> @@ -1078,6 +1080,8 @@ static void ath12k_core_post_reconfigure_recovery(struct ath12k_base *ab)
>  				    "device is wedged, will not restart hw %d\n", i);
>  			break;
>  		}
> +
> +		mutex_unlock(&ah->hw_mutex);
>  	}
>  
>  	complete(&ab->driver_recovery);
> diff --git a/drivers/net/wireless/ath/ath12k/core.h b/drivers/net/wireless/ath/ath12k/core.h
> index eff1093fbb6e..3e3e157b6c56 100644
> --- a/drivers/net/wireless/ath/ath12k/core.h
> +++ b/drivers/net/wireless/ath/ath12k/core.h
> @@ -634,11 +634,17 @@ struct ath12k {
>  struct ath12k_hw {
>  	struct ieee80211_hw *hw;
>  	struct ath12k_base *ab;
> +
> +	/* Protect the write operation of the hardware state ath12k_hw::state
> +	 * between hardware start<=>reconfigure<=>stop transitions.
> +	 */
> +	struct mutex hw_mutex;
>  	enum ath12k_hw_state state;
>  	bool regd_updated;
>  	bool use_6ghz_regd;
>  	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 8b003c18796d..3dc95d36b6a2 100644
> --- a/drivers/net/wireless/ath/ath12k/mac.c
> +++ b/drivers/net/wireless/ath/ath12k/mac.c
> @@ -5546,10 +5546,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->hw_mutex);
> +
>  	mutex_lock(&ar->conf_mutex);
>  
>  	ret = ath12k_wmi_pdev_set_param(ar, WMI_PDEV_PARAM_PMF_QOS,
> @@ -5664,6 +5667,8 @@ static int ath12k_mac_op_start(struct ieee80211_hw *hw)
>  
>  	ath12k_drain_tx(ah);
>  
> +	mutex_lock(&ah->hw_mutex);

since this is always unlocked just before we return, it is a good candidate
for the cleanup.h functionality. just change this to:
	guard(mutex)(&ah->hw_mutex);

and remove all of the other edits to this function. the mutex will always be
unlocked when the function returns

> +
>  	switch (ah->state) {
>  	case ATH12K_HW_STATE_OFF:
>  		ah->state = ATH12K_HW_STATE_ON;
> @@ -5678,7 +5683,8 @@ static int ath12k_mac_op_start(struct ieee80211_hw *hw)
>  		ah->state = ATH12K_HW_STATE_OFF;
>  
>  		WARN_ON(1);
> -		return -EINVAL;
> +		ret = -EINVAL;
> +		goto err;
>  	}
>  
>  	for_each_ar(ah, ar, i) {
> @@ -5692,6 +5698,8 @@ static int ath12k_mac_op_start(struct ieee80211_hw *hw)
>  		}
>  	}
>  
> +	mutex_unlock(&ah->hw_mutex);
> +
>  	return 0;
>  
>  fail_start:
> @@ -5700,6 +5708,8 @@ static int ath12k_mac_op_start(struct ieee80211_hw *hw)
>  		ath12k_mac_stop(ar);
>  	}
>  
> +err:
> +	mutex_unlock(&ah->hw_mutex);
>  	return ret;
>  }
>  
> @@ -5762,9 +5772,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->hw_mutex);
> +
>  	mutex_lock(&ar->conf_mutex);
>  	ret = ath12k_mac_config_mon_status_default(ar, false);
>  	if (ret && (ret != -EOPNOTSUPP))
> @@ -5800,10 +5813,14 @@ static void ath12k_mac_op_stop(struct ieee80211_hw *hw)
>  
>  	ath12k_drain_tx(ah);
>  
> +	mutex_lock(&ah->hw_mutex);
> +
>  	ah->state = ATH12K_HW_STATE_OFF;
>  
>  	for_each_ar(ah, ar, i)
>  		ath12k_mac_stop(ar);
> +
> +	mutex_unlock(&ah->hw_mutex);
>  }
>  
>  static u8
> @@ -7848,8 +7865,12 @@ ath12k_mac_op_reconfig_complete(struct ieee80211_hw *hw,
>  	if (reconfig_type != IEEE80211_RECONFIG_TYPE_RESTART)
>  		return;
>  
> -	if (ah->state != ATH12K_HW_STATE_RESTARTED)
> +	mutex_lock(&ah->hw_mutex);

another good candidate for
	guard(mutex)(&ah->hw_mutex);

> +
> +	if (ah->state != ATH12K_HW_STATE_RESTARTED) {
> +		mutex_unlock(&ah->hw_mutex);
>  		return;
> +	}
>  
>  	ah->state = ATH12K_HW_STATE_ON;
>  	ieee80211_wake_queues(hw);
> @@ -7904,6 +7925,8 @@ ath12k_mac_op_reconfig_complete(struct ieee80211_hw *hw,
>  
>  		mutex_unlock(&ar->conf_mutex);
>  	}
> +
> +	mutex_unlock(&ah->hw_mutex);
>  }
>  
>  static void
> @@ -8850,6 +8873,8 @@ static struct ath12k_hw *ath12k_mac_hw_allocate(struct ath12k_base *ab,
>  	ah->ab = ab;
>  	ah->num_radio = num_pdev_map;
>  
> +	mutex_init(&ah->hw_mutex);
> +
>  	for (i = 0; i < num_pdev_map; i++) {
>  		ab = pdev_map[i].ab;
>  		pdev_idx = pdev_map[i].pdev_idx;

Re: [PATCH v2 3/3] wifi: ath12k: Add lock to protect the hardware state

[Karthikeyan Periyasamy]

On 4/25/2024 2:11 AM, Jeff Johnson wrote:
> On 4/23/2024 11:56 PM, Karthikeyan Periyasamy wrote:
>> Currently, hardware state is not protected across the reconfigure
>> operations. 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 to protect
>> hardware state across the multiple radio/link at the driver hardware
>> abstraction (ath12k_hw) layer. Therefore, introduce a new mutex in
>> the ath12k_hw layer.
>>
>> 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 |  6 ++++++
>>   drivers/net/wireless/ath/ath12k/mac.c  | 29 ++++++++++++++++++++++++--
>>   3 files changed, 37 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/net/wireless/ath/ath12k/core.c b/drivers/net/wireless/ath/ath12k/core.c
>> index a685cfd6fd92..e9aabdb9341c 100644
>> --- a/drivers/net/wireless/ath/ath12k/core.c
>> +++ b/drivers/net/wireless/ath/ath12k/core.c
>> @@ -1048,6 +1048,8 @@ static void ath12k_core_post_reconfigure_recovery(struct ath12k_base *ab)
>>   		if (!ah || ah->state == ATH12K_HW_STATE_OFF)
>>   			continue;
>>   
>> +		mutex_lock(&ah->hw_mutex);
>> +
>>   		switch (ah->state) {
>>   		case ATH12K_HW_STATE_ON:
>>   			ah->state = ATH12K_HW_STATE_RESTARTING;
>> @@ -1078,6 +1080,8 @@ static void ath12k_core_post_reconfigure_recovery(struct ath12k_base *ab)
>>   				    "device is wedged, will not restart hw %d\n", i);
>>   			break;
>>   		}
>> +
>> +		mutex_unlock(&ah->hw_mutex);
>>   	}
>>   
>>   	complete(&ab->driver_recovery);
>> diff --git a/drivers/net/wireless/ath/ath12k/core.h b/drivers/net/wireless/ath/ath12k/core.h
>> index eff1093fbb6e..3e3e157b6c56 100644
>> --- a/drivers/net/wireless/ath/ath12k/core.h
>> +++ b/drivers/net/wireless/ath/ath12k/core.h
>> @@ -634,11 +634,17 @@ struct ath12k {
>>   struct ath12k_hw {
>>   	struct ieee80211_hw *hw;
>>   	struct ath12k_base *ab;
>> +
>> +	/* Protect the write operation of the hardware state ath12k_hw::state
>> +	 * between hardware start<=>reconfigure<=>stop transitions.
>> +	 */
>> +	struct mutex hw_mutex;
>>   	enum ath12k_hw_state state;
>>   	bool regd_updated;
>>   	bool use_6ghz_regd;
>>   	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 8b003c18796d..3dc95d36b6a2 100644
>> --- a/drivers/net/wireless/ath/ath12k/mac.c
>> +++ b/drivers/net/wireless/ath/ath12k/mac.c
>> @@ -5546,10 +5546,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->hw_mutex);
>> +
>>   	mutex_lock(&ar->conf_mutex);
>>   
>>   	ret = ath12k_wmi_pdev_set_param(ar, WMI_PDEV_PARAM_PMF_QOS,
>> @@ -5664,6 +5667,8 @@ static int ath12k_mac_op_start(struct ieee80211_hw *hw)
>>   
>>   	ath12k_drain_tx(ah);
>>   
>> +	mutex_lock(&ah->hw_mutex);
> 
> since this is always unlocked just before we return, it is a good candidate
> for the cleanup.h functionality. just change this to:
> 	guard(mutex)(&ah->hw_mutex);
> 
> and remove all of the other edits to this function. the mutex will always be
> unlocked when the function returns
> 

sure, will address this comment in the next version of the patch.


>> +
>>   	switch (ah->state) {
>>   	case ATH12K_HW_STATE_OFF:
>>   		ah->state = ATH12K_HW_STATE_ON;
>> @@ -5678,7 +5683,8 @@ static int ath12k_mac_op_start(struct ieee80211_hw *hw)
>>   		ah->state = ATH12K_HW_STATE_OFF;
>>   
>>   		WARN_ON(1);
>> -		return -EINVAL;
>> +		ret = -EINVAL;
>> +		goto err;
>>   	}
>>   
>>   	for_each_ar(ah, ar, i) {
>> @@ -5692,6 +5698,8 @@ static int ath12k_mac_op_start(struct ieee80211_hw *hw)
>>   		}
>>   	}
>>   
>> +	mutex_unlock(&ah->hw_mutex);
>> +
>>   	return 0;
>>   
>>   fail_start:
>> @@ -5700,6 +5708,8 @@ static int ath12k_mac_op_start(struct ieee80211_hw *hw)
>>   		ath12k_mac_stop(ar);
>>   	}
>>   
>> +err:
>> +	mutex_unlock(&ah->hw_mutex);
>>   	return ret;
>>   }
>>   
>> @@ -5762,9 +5772,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->hw_mutex);
>> +
>>   	mutex_lock(&ar->conf_mutex);
>>   	ret = ath12k_mac_config_mon_status_default(ar, false);
>>   	if (ret && (ret != -EOPNOTSUPP))
>> @@ -5800,10 +5813,14 @@ static void ath12k_mac_op_stop(struct ieee80211_hw *hw)
>>   
>>   	ath12k_drain_tx(ah);
>>   
>> +	mutex_lock(&ah->hw_mutex);
>> +
>>   	ah->state = ATH12K_HW_STATE_OFF;
>>   
>>   	for_each_ar(ah, ar, i)
>>   		ath12k_mac_stop(ar);
>> +
>> +	mutex_unlock(&ah->hw_mutex);
>>   }
>>   
>>   static u8
>> @@ -7848,8 +7865,12 @@ ath12k_mac_op_reconfig_complete(struct ieee80211_hw *hw,
>>   	if (reconfig_type != IEEE80211_RECONFIG_TYPE_RESTART)
>>   		return;
>>   
>> -	if (ah->state != ATH12K_HW_STATE_RESTARTED)
>> +	mutex_lock(&ah->hw_mutex);
> 
> another good candidate for
> 	guard(mutex)(&ah->hw_mutex);

sure, will address this comment in the next version of the patch.

> 
>> +
>> +	if (ah->state != ATH12K_HW_STATE_RESTARTED) {
>> +		mutex_unlock(&ah->hw_mutex);
>>   		return;
>> +	}
>>   
>>   	ah->state = ATH12K_HW_STATE_ON;
>>   	ieee80211_wake_queues(hw);
>> @@ -7904,6 +7925,8 @@ ath12k_mac_op_reconfig_complete(struct ieee80211_hw *hw,
>>   
>>   		mutex_unlock(&ar->conf_mutex);
>>   	}
>> +
>> +	mutex_unlock(&ah->hw_mutex);
>>   }
>>   
>>   static void
>> @@ -8850,6 +8873,8 @@ static struct ath12k_hw *ath12k_mac_hw_allocate(struct ath12k_base *ab,
>>   	ah->ab = ab;
>>   	ah->num_radio = num_pdev_map;
>>   
>> +	mutex_init(&ah->hw_mutex);
>> +
>>   	for (i = 0; i < num_pdev_map; i++) {
>>   		ab = pdev_map[i].ab;
>>   		pdev_idx = pdev_map[i].pdev_idx;
> 

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