On Thursday, 13 June 2019 09:31:37 CEST Manikanta Pubbisetty wrote:
[...]
> diff --git a/drivers/net/wireless/ath/ath11k/core.h b/drivers/net/wireless/ath/ath11k/core.h
> index 038f905..f853c3c 100644
> --- a/drivers/net/wireless/ath/ath11k/core.h
> +++ b/drivers/net/wireless/ath/ath11k/core.h
> @@ -211,6 +211,9 @@ struct ath11k_vif {
>  	int num_legacy_stations;
>  	int rtscts_prot_mode;
>  	int txpower;
> +	bool ht_enabled;
> +	bool vht_enabled;
> +	bool he_enabled;
>  };

Don't use bool's in structs. Something like

    u8 ht_enabled:1;

is preferred in the kernel [1].

> diff --git a/drivers/net/wireless/ath/ath11k/mac.c b/drivers/net/wireless/ath/ath11k/mac.c
> index 8c47e09..7ca6958 100644
> --- a/drivers/net/wireless/ath/ath11k/mac.c
> +++ b/drivers/net/wireless/ath/ath11k/mac.c
> @@ -790,6 +790,7 @@ static int ath11k_mac_setup_bcn_tmpl(struct ath11k_vif *arvif)
>  	struct ieee80211_vif *vif = arvif->vif;
>  	struct ieee80211_mutable_offsets offs = {};
>  	struct sk_buff *bcn;
> +	u8 *ies;
>  	int ret;
>  
>  	if (arvif->vdev_type != WMI_VDEV_TYPE_AP)
> @@ -809,6 +810,20 @@ static int ath11k_mac_setup_bcn_tmpl(struct ath11k_vif *arvif)
>  		ath11k_warn(ab, "failed to submit beacon template command: %d\n",
>  			    ret);
>  
> +	ies = bcn->data + ieee80211_get_hdrlen_from_skb(bcn);
> +	ies += 12; /* fixed parameters */
> +
> +	/* ht, vht and he capabilities are required to correctly configure
> +	 * the bandwidth (phy_mode) of the connecting peer during peer assoc,
> +	 * get these capabilities from beacon since mac80211 doesn't provide
> +	 * this info to the driver.
> +	 */
> +	arvif->ht_enabled = !!cfg80211_find_ie(WLAN_EID_HT_CAPABILITY, ies,
> +					       (u8 *)skb_tail_pointer(bcn) - ies);
> +	arvif->vht_enabled = !!cfg80211_find_ie(WLAN_EID_VHT_CAPABILITY, ies,
> +					       (u8 *)skb_tail_pointer(bcn) - ies);
> +	arvif->he_enabled = !!cfg80211_find_ie(WLAN_EID_EXT_HE_CAPABILITY, ies,
> +					       (u8 *)skb_tail_pointer(bcn) - ies);
>  	return ret;
>  }

Not sure whether this is a good idea when you have beaconless vifs. I would also
call this whole approach "interesting"

>  	switch (band) {
>  	case NL80211_BAND_2GHZ:
> -		if (sta->he_cap.has_he) {
> +		if (sta->he_cap.has_he && arvif->he_enabled) {
>  			if (sta->bandwidth == IEEE80211_STA_RX_BW_80)
>  				phymode = MODE_11AX_HE80_2G;
>  			else if (sta->bandwidth == IEEE80211_STA_RX_BW_40)
>  				phymode = MODE_11AX_HE40_2G;
>  			else
>  				phymode = MODE_11AX_HE20_2G;
> -		} else if (sta->vht_cap.vht_supported &&
> +		} else if (sta->vht_cap.vht_supported && arvif->vht_enabled &&
>  		    !ath11k_peer_assoc_h_vht_masked(vht_mcs_mask)) {
>  			if (sta->bandwidth == IEEE80211_STA_RX_BW_40)
>  				phymode = MODE_11AC_VHT40;
>  			else
>  				phymode = MODE_11AC_VHT20;
> -		} else if (sta->ht_cap.ht_supported &&
> +		} else if (sta->ht_cap.ht_supported && arvif->ht_enabled &&
>  			   !ath11k_peer_assoc_h_ht_masked(ht_mcs_mask)) {
>  			if (sta->bandwidth == IEEE80211_STA_RX_BW_40)
>  				phymode = MODE_11NG_HT40;
> @@ -1564,12 +1580,12 @@ static void ath11k_peer_assoc_h_phymode(struct ath11k *ar,
>  		break;
>  	case NL80211_BAND_5GHZ:
>  		/* Check HE first */
> -		if (sta->he_cap.has_he) {
> +		if (sta->he_cap.has_he && arvif->he_enabled) {
>  			phymode = ath11k_mac_get_phymode_he(ar, sta);
> -		} else if (sta->vht_cap.vht_supported &&
> +		} else if (sta->vht_cap.vht_supported && arvif->vht_enabled &&
>  		    !ath11k_peer_assoc_h_vht_masked(vht_mcs_mask)) {
>  			phymode = ath11k_mac_get_phymode_vht(ar, sta);
> -		} else if (sta->ht_cap.ht_supported &&
> +		} else if (sta->ht_cap.ht_supported && arvif->ht_enabled &&
>  			   !ath11k_peer_assoc_h_ht_masked(ht_mcs_mask)) {
>  			if (sta->bandwidth >= IEEE80211_STA_RX_BW_40)
>  				phymode = MODE_11NA_HT40;
> 

Hm, there is still the question why we don't enable HE PHY mode for the vdev
independent of the bss_conf.he_support - like we do for VHT in ath11k.

Kind regards,
	Sven

[1] https://patchwork.kernel.org/patch/10333583/