From: Sven Eckelmann <sven@narfation.org>
To: ath11k@lists.infradead.org
Cc: Manikanta Pubbisetty <mpubbise@codeaurora.org>
Subject: Re: [PATCH v2] ath11k: fix firmware assert due to phymode mismatch
Date: Thu, 13 Jun 2019 10:34:14 +0200 [thread overview]
Message-ID: <22617229.yAUoFtrAxf@bentobox> (raw)
In-Reply-To: <1560411097-9094-1-git-send-email-mpubbise@codeaurora.org>
[-- Attachment #1.1: Type: text/plain, Size: 4024 bytes --]
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/
[-- Attachment #1.2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
[-- Attachment #2: Type: text/plain, Size: 146 bytes --]
_______________________________________________
ath11k mailing list
ath11k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath11k
prev parent reply other threads:[~2019-06-13 8:34 UTC|newest]
Thread overview: 3+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-06-13 7:31 [PATCH v2] ath11k: fix firmware assert due to phymode mismatch Manikanta Pubbisetty
2019-06-13 7:40 ` Manikanta Pubbisetty
2019-06-13 8:34 ` Sven Eckelmann [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=22617229.yAUoFtrAxf@bentobox \
--to=sven@narfation.org \
--cc=ath11k@lists.infradead.org \
--cc=mpubbise@codeaurora.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox