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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.