diff mbox series

wifi: ath12k: Fix pdev id sent to firmware for single phy devices

Message ID 20240611043342.2672998-1-quic_rgnanase@quicinc.com
State New
Headers show
Series wifi: ath12k: Fix pdev id sent to firmware for single phy devices | expand

Commit Message

Ramya Gnanasekar June 11, 2024, 4:33 a.m. UTC 
From: Lingbo Kong <quic_lingbok@quicinc.com>

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

For single pdev devices firmware expects pdev id as 1 for 5 GHz/6 GHz
phy and 2 for 2 GHz band. Add wrapper ath12k_mac_get_target_pdev_id()
to help fetch right pdev for single pdev devices.

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/mac.c | 59 +++++++++++++++++++++++++++
 drivers/net/wireless/ath/ath12k/mac.h |  3 ++
 2 files changed, 62 insertions(+)

Comments

Kalle Valo June 19, 2024, 2:13 p.m. UTC | #1 
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/WMI command to firmware. This causes issue with single pdev
> devices where firmware does not respond to the WMI/HTT request
> sent from host.

But WCN7850 already works so I'm not really understanding the bug
description. Can you elaborate, please?

> @@ -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 *ar);

AFAICS ath12k_mac_get_target_pdev_id_from_vif() and
ath12k_mac_get_vif_up() can be static, no need to add them to mac.h.

So we are left with ath12k_mac_get_target_pdev_id() but I don't see it
called anywhere. Isn't that dead code we are adding?
Ramya Gnanasekar June 19, 2024, 4:12 p.m. UTC | #2 
On 6/19/2024 7:43 PM, 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/WMI command to firmware. This causes issue with single pdev
>> devices where firmware does not respond to the WMI/HTT request
>> sent from host.
> 
> But WCN7850 already works so I'm not really understanding the bug
> description. Can you elaborate, please?

pdev id from mac capabilities sent as a part of HTT stats request or WMI
control path stats request was not expected by WCN7850 and hence fails
to send the stats response.
> 
>> @@ -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 *ar);
> 
> AFAICS ath12k_mac_get_target_pdev_id_from_vif() and
> ath12k_mac_get_vif_up() can be static, no need to add them to mac.h.

Ah! Sure Kalle. I will take care.
> 
> So we are left with ath12k_mac_get_target_pdev_id() but I don't see it
> called anywhere. Isn't that dead code we are adding?

The wrapper is used by upcoming HTT stats and WMI control path stats
support. Initially this change was added in the HTT stats infrastructure
support series. Since WMI control path stats also depends on this patch,
to eliminate the another patch series dependency, I pulled out this
patch from HTT stats series.
This wrapper will be used by below changes:
HTT stats:
https://lore.kernel.org/linux-wireless/20240614063404.2659089-1-quic_rgnanase@quicinc.com/T/
WMI control path stats:
https://patchwork.kernel.org/project/linux-wireless/patch/20240529180920.1472003-1-quic_rgnanase@quicinc.com/

WMI control path stats has to be rebased on this current patch. Since it
is deferred, I refrained from sending v2.

>
Kalle Valo June 19, 2024, 5:26 p.m. UTC | #3 
Ramya Gnanasekar <quic_rgnanase@quicinc.com> writes:

> On 6/19/2024 7:43 PM, 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/WMI command to firmware. This causes issue with single pdev
>>> devices where firmware does not respond to the WMI/HTT request
>>> sent from host.
>> 
>> But WCN7850 already works so I'm not really understanding the bug
>> description. Can you elaborate, please?
>
> pdev id from mac capabilities sent as a part of HTT stats request or WMI
> control path stats request was not expected by WCN7850 and hence fails
> to send the stats response.

Ok, now I understand. Please include that info to the commit message.

>>> @@ -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 *ar);
>> 
>> AFAICS ath12k_mac_get_target_pdev_id_from_vif() and
>> ath12k_mac_get_vif_up() can be static, no need to add them to mac.h.
>
> Ah! Sure Kalle. I will take care.

Thanks.

>> So we are left with ath12k_mac_get_target_pdev_id() but I don't see it
>> called anywhere. Isn't that dead code we are adding?
>
> The wrapper is used by upcoming HTT stats and WMI control path stats
> support. Initially this change was added in the HTT stats infrastructure
> support series. Since WMI control path stats also depends on this patch,
> to eliminate the another patch series dependency, I pulled out this
> patch from HTT stats series.
> This wrapper will be used by below changes:
> HTT stats:
> https://lore.kernel.org/linux-wireless/20240614063404.2659089-1-quic_rgnanase@quicinc.com/T/
> WMI control path stats:
> https://patchwork.kernel.org/project/linux-wireless/patch/20240529180920.1472003-1-quic_rgnanase@quicinc.com/
>
> WMI control path stats has to be rebased on this current patch. Since it
> is deferred, I refrained from sending v2.

Ok, you could for example mention this is in a comment after '---' line.
Adding dead code is usually suspicious.
Kalle Valo June 19, 2024, 5:35 p.m. UTC | #4 
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/WMI command to firmware. This causes issue with single pdev
> devices where firmware does not respond to the WMI/HTT request
> sent from host.
>
> For single pdev devices firmware expects pdev id as 1 for 5 GHz/6 GHz
> phy and 2 for 2 GHz band. Add wrapper ath12k_mac_get_target_pdev_id()
> to help fetch right pdev for single pdev devices.
>
> 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>

[...]

> +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)));
> +}

This is not really pleasent to read. What about something like this:

switch (band1) {
case NL80211_BAND_2GHZ:
        if (band2 & WMI_HOST_WLAN_2G_CAP)
                return true;

        break;
case NL80211_BAND_5GHZ:
case NL80211_BAND_6GHZ:
        if (band2 & WMI_HOST_WLAN_5G_CAP)
                return true;

        break;
}

return false;

Or any other ideas?

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

I find this easier to read:

arvif = ath12k_mac_get_vif_up(ar);
if (!arvif)
	return ar->ab->fw_pdev[0].pdev_id;

return ath12k_mac_get_target_pdev_id_from_vif(arvif);

But I still would prefer to have some a code comment explaining the idea
behind here, especially why it's safe to use fw_pdev[0].pdev_id directly
and what scenario that is.
Ramya Gnanasekar June 20, 2024, 4:23 a.m. UTC | #5 
On 6/19/2024 10:56 PM, Kalle Valo wrote:
> Ramya Gnanasekar <quic_rgnanase@quicinc.com> writes:
> 
>> On 6/19/2024 7:43 PM, 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/WMI command to firmware. This causes issue with single pdev
>>>> devices where firmware does not respond to the WMI/HTT request
>>>> sent from host.
>>>
>>> But WCN7850 already works so I'm not really understanding the bug
>>> description. Can you elaborate, please?
>>
>> pdev id from mac capabilities sent as a part of HTT stats request or WMI
>> control path stats request was not expected by WCN7850 and hence fails
>> to send the stats response.
> 
> Ok, now I understand. Please include that info to the commit message.

Sure I will add the information.
> 
>>>> @@ -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 *ar);
>>>
>>> AFAICS ath12k_mac_get_target_pdev_id_from_vif() and
>>> ath12k_mac_get_vif_up() can be static, no need to add them to mac.h.
>>
>> Ah! Sure Kalle. I will take care.
> 
> Thanks.
> 
>>> So we are left with ath12k_mac_get_target_pdev_id() but I don't see it
>>> called anywhere. Isn't that dead code we are adding?
>>
>> The wrapper is used by upcoming HTT stats and WMI control path stats
>> support. Initially this change was added in the HTT stats infrastructure
>> support series. Since WMI control path stats also depends on this patch,
>> to eliminate the another patch series dependency, I pulled out this
>> patch from HTT stats series.
>> This wrapper will be used by below changes:
>> HTT stats:
>> https://lore.kernel.org/linux-wireless/20240614063404.2659089-1-quic_rgnanase@quicinc.com/T/
>> WMI control path stats:
>> https://patchwork.kernel.org/project/linux-wireless/patch/20240529180920.1472003-1-quic_rgnanase@quicinc.com/
>>
>> WMI control path stats has to be rebased on this current patch. Since it
>> is deferred, I refrained from sending v2.
> 
> Ok, you could for example mention this is in a comment after '---' line.
> Adding dead code is usually suspicious.

Yeah sure. I will brief the usage in commit message as you mentioned.
>
diff mbox series

Patch

diff --git a/drivers/net/wireless/ath/ath12k/mac.c b/drivers/net/wireless/ath/ath12k/mac.c
index 784964ae03ec..31a66ade7852 100644
--- a/drivers/net/wireless/ath/ath12k/mac.c
+++ b/drivers/net/wireless/ath/ath12k/mac.c
@@ -670,6 +670,65 @@  static struct ath12k *ath12k_get_ar_by_vif(struct ieee80211_hw *hw,
 	return NULL;
 }
 
+struct ath12k_vif *ath12k_mac_get_vif_up(struct ath12k *ar)
+{
+	struct ath12k_vif *arvif;
+
+	lockdep_assert_held(&ar->conf_mutex);
+	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;
+	struct ath12k_base *ab = ar->ab;
+
+	if (!ab->hw_params->single_pdev_only)
+		return ar->pdev->pdev_id;
+
+	arvif = ath12k_mac_get_vif_up(ar);
+
+	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..877005b56a7c 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 *ar);
 
 #endif