diff mbox series

[v2,3/5] wifi: ath12k: Fix Pdev id in HTT stats request for WCN7850

Message ID 20240510050806.514126-4-quic_rgnanase@quicinc.com (mailing list archive)
State Changes Requested
Delegated to: Kalle Valo
Headers show
Series wifi: ath12k: Add support to enable debugfs_htt_stats | expand

Commit Message

Ramya Gnanasekar May 10, 2024, 5:08 a.m. UTC 
From: Lingbo Kong <quic_lingbok@quicinc.com>

Pdev id from mac phy capabilities will be sent as a part of
HTT stats request to firmware. This causes issue with single pdev
devices where fimrware does not respond to the stats request
sent from host.

Single pdev devices firmware expects pdev id as 1 for 5GHz/6GHz
phy and 2 for 2GHz band. Handle pdev id for single phy device
while sending HTT stats request message to firmware.

Tested-on: QCN9274 hw2.0 PCI WLAN.WBE.1.0.1-00029-QCAHKSWPL_SILICONZ-1
Tested-on: WCN7850 hw2.0 PCI WLAN.HMT.1.0.c5-00481-QCAHMTSWPL_V1.0_V2.0_SILICONZ-3

Signed-off-by: Lingbo Kong <quic_lingbok@quicinc.com>
Signed-off-by: Ramya Gnanasekar <quic_rgnanase@quicinc.com>
---
 drivers/net/wireless/ath/ath12k/dp_tx.c |  8 +++-
 drivers/net/wireless/ath/ath12k/mac.c   | 61 +++++++++++++++++++++++++
 drivers/net/wireless/ath/ath12k/mac.h   |  3 ++
 3 files changed, 71 insertions(+), 1 deletion(-)

Comments

Jeff Johnson May 10, 2024, 7:53 p.m. UTC | #1 
On 5/9/2024 10:08 PM, Ramya Gnanasekar wrote:
> From: Lingbo Kong <quic_lingbok@quicinc.com>
> 
> Pdev id from mac phy capabilities will be sent as a part of
> HTT stats request to firmware. This causes issue with single pdev
> devices where fimrware does not respond to the stats request
> sent from host.
> 
> Single pdev devices firmware expects pdev id as 1 for 5GHz/6GHz
> phy and 2 for 2GHz band. Handle pdev id for single phy device
> while sending HTT stats request message to firmware.
> 
> Tested-on: QCN9274 hw2.0 PCI WLAN.WBE.1.0.1-00029-QCAHKSWPL_SILICONZ-1
> Tested-on: WCN7850 hw2.0 PCI WLAN.HMT.1.0.c5-00481-QCAHMTSWPL_V1.0_V2.0_SILICONZ-3
> 
> Signed-off-by: Lingbo Kong <quic_lingbok@quicinc.com>
> Signed-off-by: Ramya Gnanasekar <quic_rgnanase@quicinc.com>

Acked-by: Jeff Johnson <quic_jjohnson@quicinc.com>
Kalle Valo May 21, 2024, 7:57 a.m. UTC | #2 
Ramya Gnanasekar <quic_rgnanase@quicinc.com> writes:

> From: Lingbo Kong <quic_lingbok@quicinc.com>
>
> Pdev id from mac phy capabilities will be sent as a part of
> HTT stats request to firmware. This causes issue with single pdev
> devices where fimrware does not respond to the stats request
> sent from host.
>
> Single pdev devices firmware expects pdev id as 1 for 5GHz/6GHz
> phy and 2 for 2GHz band. Handle pdev id for single phy device
> while sending HTT stats request message to firmware.
>
> Tested-on: QCN9274 hw2.0 PCI WLAN.WBE.1.0.1-00029-QCAHKSWPL_SILICONZ-1
> Tested-on: WCN7850 hw2.0 PCI WLAN.HMT.1.0.c5-00481-QCAHMTSWPL_V1.0_V2.0_SILICONZ-3
>
> Signed-off-by: Lingbo Kong <quic_lingbok@quicinc.com>
> Signed-off-by: Ramya Gnanasekar <quic_rgnanase@quicinc.com>

[...]

> @@ -1029,7 +1030,12 @@ ath12k_dp_tx_htt_h2t_ext_stats_req(struct ath12k *ar, u8 type,
>  	memset(cmd, 0, sizeof(*cmd));
>  	cmd->hdr.msg_type = HTT_H2T_MSG_TYPE_EXT_STATS_CFG;
>  
> -	cmd->hdr.pdev_mask = 1 << ar->pdev->pdev_id;
> +	if (ab->hw_params->single_pdev_only)
> +		pdev_id = ath12k_mac_get_target_pdev_id(ar);
> +	else
> +		pdev_id = ar->pdev->pdev_id;

Wouldn't it be cleaner to have the single_pdev_only check in
ath12k_mac_get_target_pdev_id()?

> +struct ath12k_vif *ath12k_mac_get_vif_up(struct ath12k_base *ab)
> +{
> +	struct ath12k *ar;
> +	struct ath12k_pdev *pdev;
> +	struct ath12k_vif *arvif;
> +	int i;
> +
> +	for (i = 0; i < ab->num_radios; i++) {
> +		pdev = &ab->pdevs[i];
> +		ar = pdev->ar;
> +		list_for_each_entry(arvif, &ar->arvifs, list) {
> +			if (arvif->is_up)
> +				return arvif;
> +		}
> +	}
> +
> +	return NULL;
> +}

I'm not seeing any protection here, is that on purpose?

> +u8 ath12k_mac_get_target_pdev_id(struct ath12k *ar)
> +{
> +	struct ath12k_vif *arvif;
> +
> +	arvif = ath12k_mac_get_vif_up(ar->ab);
> +
> +	if (arvif)
> +		return ath12k_mac_get_target_pdev_id_from_vif(arvif);
> +	else
> +		return ar->ab->fw_pdev[0].pdev_id;
> +}

No need to have else after return.
Lingbo Kong May 23, 2024, 12:34 p.m. UTC | #3 
On 2024/5/21 15:57, Kalle Valo wrote:
> Ramya Gnanasekar <quic_rgnanase@quicinc.com> writes:
> 
>> From: Lingbo Kong <quic_lingbok@quicinc.com>
>>
>> Pdev id from mac phy capabilities will be sent as a part of
>> HTT stats request to firmware. This causes issue with single pdev
>> devices where fimrware does not respond to the stats request
>> sent from host.
>>
>> Single pdev devices firmware expects pdev id as 1 for 5GHz/6GHz
>> phy and 2 for 2GHz band. Handle pdev id for single phy device
>> while sending HTT stats request message to firmware.
>>
>> Tested-on: QCN9274 hw2.0 PCI WLAN.WBE.1.0.1-00029-QCAHKSWPL_SILICONZ-1
>> Tested-on: WCN7850 hw2.0 PCI WLAN.HMT.1.0.c5-00481-QCAHMTSWPL_V1.0_V2.0_SILICONZ-3
>>
>> Signed-off-by: Lingbo Kong <quic_lingbok@quicinc.com>
>> Signed-off-by: Ramya Gnanasekar <quic_rgnanase@quicinc.com>
> 
> [...]
> 
>> @@ -1029,7 +1030,12 @@ ath12k_dp_tx_htt_h2t_ext_stats_req(struct ath12k *ar, u8 type,
>>   	memset(cmd, 0, sizeof(*cmd));
>>   	cmd->hdr.msg_type = HTT_H2T_MSG_TYPE_EXT_STATS_CFG;
>>   
>> -	cmd->hdr.pdev_mask = 1 << ar->pdev->pdev_id;
>> +	if (ab->hw_params->single_pdev_only)
>> +		pdev_id = ath12k_mac_get_target_pdev_id(ar);
>> +	else
>> +		pdev_id = ar->pdev->pdev_id;
> 
> Wouldn't it be cleaner to have the single_pdev_only check in
> ath12k_mac_get_target_pdev_id()?
> 

yes, i'll put the single_pdev_only check in 
ath12k_mac_get_target_pdev_id() function.

>> +struct ath12k_vif *ath12k_mac_get_vif_up(struct ath12k_base *ab)
>> +{
>> +	struct ath12k *ar;
>> +	struct ath12k_pdev *pdev;
>> +	struct ath12k_vif *arvif;
>> +	int i;
>> +
>> +	for (i = 0; i < ab->num_radios; i++) {
>> +		pdev = &ab->pdevs[i];
>> +		ar = pdev->ar;
>> +		list_for_each_entry(arvif, &ar->arvifs, list) {
>> +			if (arvif->is_up)
>> +				return arvif;
>> +		}
>> +	}
>> +
>> +	return NULL;
>> +}
> 
> I'm not seeing any protection here, is that on purpose?
> 

you means there need to add lockdep_assert_held(&ar->conf_mutex)?

>> +u8 ath12k_mac_get_target_pdev_id(struct ath12k *ar)
>> +{
>> +	struct ath12k_vif *arvif;
>> +
>> +	arvif = ath12k_mac_get_vif_up(ar->ab);
>> +
>> +	if (arvif)
>> +		return ath12k_mac_get_target_pdev_id_from_vif(arvif);
>> +	else
>> +		return ar->ab->fw_pdev[0].pdev_id;
>> +}
> 
> No need to have else after return.
>
Kalle Valo May 23, 2024, 12:55 p.m. UTC | #4 
Lingbo Kong <quic_lingbok@quicinc.com> writes:

>>> +struct ath12k_vif *ath12k_mac_get_vif_up(struct ath12k_base *ab)
>>> +{
>>> +	struct ath12k *ar;
>>> +	struct ath12k_pdev *pdev;
>>> +	struct ath12k_vif *arvif;
>>> +	int i;
>>> +
>>> +	for (i = 0; i < ab->num_radios; i++) {
>>> +		pdev = &ab->pdevs[i];
>>> +		ar = pdev->ar;
>>> +		list_for_each_entry(arvif, &ar->arvifs, list) {
>>> +			if (arvif->is_up)
>>> +				return arvif;
>>> +		}
>>> +	}
>>> +
>>> +	return NULL;
>>> +}
>> I'm not seeing any protection here, is that on purpose?
>
> you means there need to add lockdep_assert_held(&ar->conf_mutex)?

I mean what's the locking design here? Is it safe to concurrectly access
ab->pdevs and arvif->is_up?
Lingbo Kong May 23, 2024, 2:04 p.m. UTC | #5 
On 2024/5/23 20:55, Kalle Valo wrote:
> Lingbo Kong <quic_lingbok@quicinc.com> writes:
> 
>>>> +struct ath12k_vif *ath12k_mac_get_vif_up(struct ath12k_base *ab)
>>>> +{
>>>> +	struct ath12k *ar;
>>>> +	struct ath12k_pdev *pdev;
>>>> +	struct ath12k_vif *arvif;
>>>> +	int i;
>>>> +
>>>> +	for (i = 0; i < ab->num_radios; i++) {
>>>> +		pdev = &ab->pdevs[i];
>>>> +		ar = pdev->ar;
>>>> +		list_for_each_entry(arvif, &ar->arvifs, list) {
>>>> +			if (arvif->is_up)
>>>> +				return arvif;
>>>> +		}
>>>> +	}
>>>> +
>>>> +	return NULL;
>>>> +}
>>> I'm not seeing any protection here, is that on purpose?
>>
>> you means there need to add lockdep_assert_held(&ar->conf_mutex)?
> 
> I mean what's the locking design here? Is it safe to concurrectly access
> ab->pdevs and arvif->is_up?
> 

oh, i've seen other places use ar->conf_mutex to protect when accessing 
arvif->is_up.

but according to the ath12k_mac_get_vif_up()'s call stack, the 
ath12k_write_htt_stats_reset() and ath12k_open_htt_stats() have already 
executed mutex_lock(&ar->conf_mutex);

so it's best to add lockdep_assert_held(&ar->conf_mutex) here to 
determine whether conf_mutex is obtained.

/lingbo kong
diff mbox series

Patch

diff --git a/drivers/net/wireless/ath/ath12k/dp_tx.c b/drivers/net/wireless/ath/ath12k/dp_tx.c
index a22fa43c87ec..c570e55c638e 100644
--- a/drivers/net/wireless/ath/ath12k/dp_tx.c
+++ b/drivers/net/wireless/ath/ath12k/dp_tx.c
@@ -1018,6 +1018,7 @@  ath12k_dp_tx_htt_h2t_ext_stats_req(struct ath12k *ar, u8 type,
 	struct htt_ext_stats_cfg_cmd *cmd;
 	int len = sizeof(*cmd);
 	int ret;
+	u32 pdev_id;
 
 	skb = ath12k_htc_alloc_skb(ab, len);
 	if (!skb)
@@ -1029,7 +1030,12 @@  ath12k_dp_tx_htt_h2t_ext_stats_req(struct ath12k *ar, u8 type,
 	memset(cmd, 0, sizeof(*cmd));
 	cmd->hdr.msg_type = HTT_H2T_MSG_TYPE_EXT_STATS_CFG;
 
-	cmd->hdr.pdev_mask = 1 << ar->pdev->pdev_id;
+	if (ab->hw_params->single_pdev_only)
+		pdev_id = ath12k_mac_get_target_pdev_id(ar);
+	else
+		pdev_id = ar->pdev->pdev_id;
+
+	cmd->hdr.pdev_mask = 1 << pdev_id;
 
 	cmd->hdr.stats_type = type;
 	cmd->cfg_param0 = cpu_to_le32(cfg_params->cfg0);
diff --git a/drivers/net/wireless/ath/ath12k/mac.c b/drivers/net/wireless/ath/ath12k/mac.c
index 96dc5c2e096f..11b8b916084d 100644
--- a/drivers/net/wireless/ath/ath12k/mac.c
+++ b/drivers/net/wireless/ath/ath12k/mac.c
@@ -666,6 +666,67 @@  static struct ath12k *ath12k_get_ar_by_vif(struct ieee80211_hw *hw,
 	return NULL;
 }
 
+struct ath12k_vif *ath12k_mac_get_vif_up(struct ath12k_base *ab)
+{
+	struct ath12k *ar;
+	struct ath12k_pdev *pdev;
+	struct ath12k_vif *arvif;
+	int i;
+
+	for (i = 0; i < ab->num_radios; i++) {
+		pdev = &ab->pdevs[i];
+		ar = pdev->ar;
+		list_for_each_entry(arvif, &ar->arvifs, list) {
+			if (arvif->is_up)
+				return arvif;
+		}
+	}
+
+	return NULL;
+}
+
+static bool ath12k_mac_band_match(enum nl80211_band band1, enum WMI_HOST_WLAN_BAND band2)
+{
+	return (((band1 == NL80211_BAND_2GHZ) && (band2 & WMI_HOST_WLAN_2G_CAP)) ||
+		(((band1 == NL80211_BAND_5GHZ) || (band1 == NL80211_BAND_6GHZ)) &&
+		   (band2 & WMI_HOST_WLAN_5G_CAP)));
+}
+
+u8 ath12k_mac_get_target_pdev_id_from_vif(struct ath12k_vif *arvif)
+{
+	struct ath12k *ar = arvif->ar;
+	struct ath12k_base *ab = ar->ab;
+	struct ieee80211_vif *vif = arvif->vif;
+	struct cfg80211_chan_def def;
+	enum nl80211_band band;
+	u8 pdev_id = ab->fw_pdev[0].pdev_id;
+	int i;
+
+	if (WARN_ON(ath12k_mac_vif_chan(vif, &def)))
+		return pdev_id;
+
+	band = def.chan->band;
+
+	for (i = 0; i < ab->fw_pdev_count; i++) {
+		if (ath12k_mac_band_match(band, ab->fw_pdev[i].supported_bands))
+			return ab->fw_pdev[i].pdev_id;
+	}
+
+	return pdev_id;
+}
+
+u8 ath12k_mac_get_target_pdev_id(struct ath12k *ar)
+{
+	struct ath12k_vif *arvif;
+
+	arvif = ath12k_mac_get_vif_up(ar->ab);
+
+	if (arvif)
+		return ath12k_mac_get_target_pdev_id_from_vif(arvif);
+	else
+		return ar->ab->fw_pdev[0].pdev_id;
+}
+
 static void ath12k_pdev_caps_update(struct ath12k *ar)
 {
 	struct ath12k_base *ab = ar->ab;
diff --git a/drivers/net/wireless/ath/ath12k/mac.h b/drivers/net/wireless/ath/ath12k/mac.h
index 69fd282b9dd3..f7a6966ba92d 100644
--- a/drivers/net/wireless/ath/ath12k/mac.h
+++ b/drivers/net/wireless/ath/ath12k/mac.h
@@ -81,5 +81,8 @@  int ath12k_mac_rfkill_config(struct ath12k *ar);
 int ath12k_mac_wait_tx_complete(struct ath12k *ar);
 void ath12k_mac_handle_beacon(struct ath12k *ar, struct sk_buff *skb);
 void ath12k_mac_handle_beacon_miss(struct ath12k *ar, u32 vdev_id);
+u8 ath12k_mac_get_target_pdev_id(struct ath12k *ar);
+u8 ath12k_mac_get_target_pdev_id_from_vif(struct ath12k_vif *arvif);
+struct ath12k_vif *ath12k_mac_get_vif_up(struct ath12k_base *ab);
 
 #endif