diff mbox series

wifi: ath11k: mac: fix struct ieee80211_sband_iftype_data handling

Message ID 20230921075440.1539515-1-kvalo@kernel.org
State New
Headers show
Series wifi: ath11k: mac: fix struct ieee80211_sband_iftype_data handling | expand

Commit Message

Kalle Valo Sept. 21, 2023, 7:54 a.m. UTC
From: Kalle Valo <quic_kvalo@quicinc.com>

Commit e8c1841278a7 ("wifi: cfg80211: annotate iftype_data pointer with
sparse") added sparse checks for struct ieee80211_sband_iftype_data handling
which immediately found an issue in ath11k:

drivers/net/wireless/ath/ath11k/mac.c:7952:22: warning: incorrect type in argument 1 (different address spaces)
drivers/net/wireless/ath/ath11k/mac.c:7952:22:    expected struct ieee80211_sta_he_cap const *he_cap
drivers/net/wireless/ath/ath11k/mac.c:7952:22:    got struct ieee80211_sta_he_cap const [noderef] __iftype_data *

The problem here is that we are accessing sband->iftype_data directly even
though we should use for_each_sband_iftype_data(). Now we iterate over each
item in the array and use the correct vif type which this vif is using.

Tested-on: WCN6855 hw2.0 PCI WLAN.HSP.1.1-03125-QCAHSPSWPL_V1_V2_SILICONZ_LITE-3.6510.23

Reported-by: Johannes Berg <johannes@sipsolutions.net>
Signed-off-by: Kalle Valo <quic_kvalo@quicinc.com>
---
 drivers/net/wireless/ath/ath11k/mac.c | 16 ++++++++++++++--
 1 file changed, 14 insertions(+), 2 deletions(-)


base-commit: f00928012886a07ca6817ea70eb4856ce280ce05

Comments

Jeff Johnson Sept. 21, 2023, 5:09 p.m. UTC | #1
On 9/21/2023 12:54 AM, Kalle Valo wrote:
> From: Kalle Valo <quic_kvalo@quicinc.com>
> 
> Commit e8c1841278a7 ("wifi: cfg80211: annotate iftype_data pointer with
> sparse") added sparse checks for struct ieee80211_sband_iftype_data handling
> which immediately found an issue in ath11k:
> 
> drivers/net/wireless/ath/ath11k/mac.c:7952:22: warning: incorrect type in argument 1 (different address spaces)
> drivers/net/wireless/ath/ath11k/mac.c:7952:22:    expected struct ieee80211_sta_he_cap const *he_cap
> drivers/net/wireless/ath/ath11k/mac.c:7952:22:    got struct ieee80211_sta_he_cap const [noderef] __iftype_data *
> 
> The problem here is that we are accessing sband->iftype_data directly even
> though we should use for_each_sband_iftype_data(). Now we iterate over each
> item in the array and use the correct vif type which this vif is using.
> 
> Tested-on: WCN6855 hw2.0 PCI WLAN.HSP.1.1-03125-QCAHSPSWPL_V1_V2_SILICONZ_LITE-3.6510.23
> 
> Reported-by: Johannes Berg <johannes@sipsolutions.net>
> Signed-off-by: Kalle Valo <quic_kvalo@quicinc.com>

Reviewed-by: Jeff Johnson <quic_jjohnson@quicinc.com>
Kalle Valo Sept. 22, 2023, 9:49 a.m. UTC | #2
Kalle Valo <kvalo@kernel.org> writes:

> From: Kalle Valo <quic_kvalo@quicinc.com>
>
> Commit e8c1841278a7 ("wifi: cfg80211: annotate iftype_data pointer with
> sparse") added sparse checks for struct ieee80211_sband_iftype_data handling
> which immediately found an issue in ath11k:
>
> drivers/net/wireless/ath/ath11k/mac.c:7952:22: warning: incorrect type
> in argument 1 (different address spaces)
> drivers/net/wireless/ath/ath11k/mac.c:7952:22: expected struct
> ieee80211_sta_he_cap const *he_cap
> drivers/net/wireless/ath/ath11k/mac.c:7952:22: got struct
> ieee80211_sta_he_cap const [noderef] __iftype_data *
>
> The problem here is that we are accessing sband->iftype_data directly even
> though we should use for_each_sband_iftype_data(). Now we iterate over each
> item in the array and use the correct vif type which this vif is using.
>
> Tested-on: WCN6855 hw2.0 PCI
> WLAN.HSP.1.1-03125-QCAHSPSWPL_V1_V2_SILICONZ_LITE-3.6510.23
>
> Reported-by: Johannes Berg <johannes@sipsolutions.net>
> Signed-off-by: Kalle Valo <quic_kvalo@quicinc.com>

[...]

> @@ -7949,7 +7951,17 @@ ath11k_mac_bitrate_mask_get_single_nss(struct ath11k *ar,
>  			return false;
>  	}
>  
> -	he_mcs_map = le16_to_cpu(ath11k_mac_get_tx_mcs_map(&sband->iftype_data->he_cap));
> +	for_each_sband_iftype_data(sband, i, iftd) {
> +		if (iftd->types_mask & BIT(arvif->vif->type)) {
> +			iftype_data = iftd;
> +			break;
> +		}
> +	}
> +
> +	if (iftype_data == NULL)
> +		return false;
> +
> +	he_mcs_map = le16_to_cpu(ath11k_mac_get_tx_mcs_map(&iftype_data->he_cap));

Johannes pointed out that I should use ieee80211_get_he_iftype_cap_vif()
instead. I'll submit v2.
Johannes Berg Sept. 23, 2023, 8:08 p.m. UTC | #3
On Fri, 2023-09-22 at 12:49 +0300, Kalle Valo wrote:
> 
> > +		if (iftd->types_mask & BIT(arvif->vif->type)) {
> 
> Johannes pointed out that I should use ieee80211_get_he_iftype_cap_vif()
> instead. I'll submit v2.

And I'll note that it's not just because it saves open-coding it, also
the BIT(vif->type) in mac80211 will be wrong because it's always STATION
even for P2P-Client, so you need to add the vif->p2p bit in.

We maybe shouldn't have done that ... but it seemed kinda nicer at the
time to treat them both the same except in special cases.

johannes
diff mbox series

Patch

diff --git a/drivers/net/wireless/ath/ath11k/mac.c b/drivers/net/wireless/ath/ath11k/mac.c
index 6ed036b51dba..4bf302a43b0c 100644
--- a/drivers/net/wireless/ath/ath11k/mac.c
+++ b/drivers/net/wireless/ath/ath11k/mac.c
@@ -7913,11 +7913,13 @@  ath11k_mac_get_tx_mcs_map(const struct ieee80211_sta_he_cap *he_cap)
 
 static bool
 ath11k_mac_bitrate_mask_get_single_nss(struct ath11k *ar,
+				       struct ath11k_vif *arvif,
 				       enum nl80211_band band,
 				       const struct cfg80211_bitrate_mask *mask,
 				       int *nss)
 {
 	struct ieee80211_supported_band *sband = &ar->mac.sbands[band];
+	const struct ieee80211_sband_iftype_data *iftd, *iftype_data = NULL;
 	u16 vht_mcs_map = le16_to_cpu(sband->vht_cap.vht_mcs.tx_mcs_map);
 	u16 he_mcs_map = 0;
 	u8 ht_nss_mask = 0;
@@ -7949,7 +7951,17 @@  ath11k_mac_bitrate_mask_get_single_nss(struct ath11k *ar,
 			return false;
 	}
 
-	he_mcs_map = le16_to_cpu(ath11k_mac_get_tx_mcs_map(&sband->iftype_data->he_cap));
+	for_each_sband_iftype_data(sband, i, iftd) {
+		if (iftd->types_mask & BIT(arvif->vif->type)) {
+			iftype_data = iftd;
+			break;
+		}
+	}
+
+	if (iftype_data == NULL)
+		return false;
+
+	he_mcs_map = le16_to_cpu(ath11k_mac_get_tx_mcs_map(&iftype_data->he_cap));
 
 	for (i = 0; i < ARRAY_SIZE(mask->control[band].he_mcs); i++) {
 		if (mask->control[band].he_mcs[i] == 0)
@@ -8365,7 +8377,7 @@  ath11k_mac_op_set_bitrate_mask(struct ieee80211_hw *hw,
 		ieee80211_iterate_stations_atomic(ar->hw,
 						  ath11k_mac_disable_peer_fixed_rate,
 						  arvif);
-	} else if (ath11k_mac_bitrate_mask_get_single_nss(ar, band, mask,
+	} else if (ath11k_mac_bitrate_mask_get_single_nss(ar, arvif, band, mask,
 							  &single_nss)) {
 		rate = WMI_FIXED_RATE_NONE;
 		nss = single_nss;