2023-07-26 01:14:16

by Aloka Dixit

[permalink] [raw]
Subject: [PATCH v5 06/11] wifi: ath12k: prepare EHT peer assoc parameters

Add new parameters and prepare the association data for an EHT peer.
MCS data uses the format described in IEEE P802.11be/D2.0, May 2022,
9.4.2.313.4, convert it into the format expected by the firmware.

Tested-on: QCN9274 hw2.0 PCI WLAN.WBE.1.0.1-00029-QCAHKSWPL_SILICONZ-1
Signed-off-by: Aloka Dixit <[email protected]>
Signed-off-by: Pradeep Kumar Chitrapu <[email protected]>
---
v5: Cleaned up bit shifting of val in ath12k_mac_set_eht_ppe_threshold().
Used hweight16() instead of hweight8() for
IEEE80211_EHT_PPE_THRES_RU_INDEX_BITMASK_MASK.
Instead of using peer_phymode to decide between a 20 MHz-only peer and
a peer supporting bandwidths <= 80 MHz, ath12k_peer_assoc_h_eht() uses
HE capabilities.
v4: No change from v3.
v3: Incremented peer_eht_mcs_count for IEEE80211_STA_RX_BW_160 in
ath12k_peer_assoc_h_eht().
v2: No change from v1.

drivers/net/wireless/ath/ath12k/mac.c | 144 ++++++++++++++++++++++++++
drivers/net/wireless/ath/ath12k/wmi.h | 17 +++
2 files changed, 161 insertions(+)

diff --git a/drivers/net/wireless/ath/ath12k/mac.c b/drivers/net/wireless/ath/ath12k/mac.c
index 379090ba107f..6fc645dfa96f 100644
--- a/drivers/net/wireless/ath/ath12k/mac.c
+++ b/drivers/net/wireless/ath/ath12k/mac.c
@@ -2067,6 +2067,149 @@ static void ath12k_peer_assoc_h_phymode(struct ath12k *ar,
WARN_ON(phymode == MODE_UNKNOWN);
}

+static void ath12k_mac_set_eht_mcs(u8 rx_tx_mcs7, u8 rx_tx_mcs9,
+ u8 rx_tx_mcs11, u8 rx_tx_mcs13,
+ u32 *rx_mcs, u32 *tx_mcs)
+{
+ *rx_mcs = 0;
+ u32p_replace_bits(rx_mcs,
+ u8_get_bits(rx_tx_mcs7, IEEE80211_EHT_MCS_NSS_RX),
+ WMI_EHT_MCS_NSS_0_7);
+ u32p_replace_bits(rx_mcs,
+ u8_get_bits(rx_tx_mcs9, IEEE80211_EHT_MCS_NSS_RX),
+ WMI_EHT_MCS_NSS_8_9);
+ u32p_replace_bits(rx_mcs,
+ u8_get_bits(rx_tx_mcs11, IEEE80211_EHT_MCS_NSS_RX),
+ WMI_EHT_MCS_NSS_10_11);
+ u32p_replace_bits(rx_mcs,
+ u8_get_bits(rx_tx_mcs13, IEEE80211_EHT_MCS_NSS_RX),
+ WMI_EHT_MCS_NSS_12_13);
+
+ *tx_mcs = 0;
+ u32p_replace_bits(tx_mcs,
+ u8_get_bits(rx_tx_mcs7, IEEE80211_EHT_MCS_NSS_TX),
+ WMI_EHT_MCS_NSS_0_7);
+ u32p_replace_bits(tx_mcs,
+ u8_get_bits(rx_tx_mcs9, IEEE80211_EHT_MCS_NSS_TX),
+ WMI_EHT_MCS_NSS_8_9);
+ u32p_replace_bits(tx_mcs,
+ u8_get_bits(rx_tx_mcs11, IEEE80211_EHT_MCS_NSS_TX),
+ WMI_EHT_MCS_NSS_10_11);
+ u32p_replace_bits(tx_mcs,
+ u8_get_bits(rx_tx_mcs13, IEEE80211_EHT_MCS_NSS_TX),
+ WMI_EHT_MCS_NSS_12_13);
+}
+
+static void ath12k_mac_set_eht_ppe_threshold(const u8 *ppe_thres,
+ struct ath12k_wmi_ppe_threshold_arg *ppet)
+{
+ u32 bit_pos = IEEE80211_EHT_PPE_THRES_INFO_HEADER_SIZE, val;
+ u8 nss, ru, i;
+ u8 ppet_bit_len_per_ru = IEEE80211_EHT_PPE_THRES_INFO_PPET_SIZE * 2;
+
+ ppet->numss_m1 = u8_get_bits(ppe_thres[0], IEEE80211_EHT_PPE_THRES_NSS_MASK);
+ ppet->ru_bit_mask = u16_get_bits(get_unaligned_le16(ppe_thres),
+ IEEE80211_EHT_PPE_THRES_RU_INDEX_BITMASK_MASK);
+
+ for (nss = 0; nss <= ppet->numss_m1; nss++) {
+ for (ru = 0;
+ ru < hweight16(IEEE80211_EHT_PPE_THRES_RU_INDEX_BITMASK_MASK);
+ ru++) {
+ if ((ppet->ru_bit_mask & BIT(ru)) == 0)
+ continue;
+
+ val = 0;
+ for (i = 0; i < ppet_bit_len_per_ru; i++) {
+ val |= (((ppe_thres[bit_pos / 8] >>
+ (bit_pos % 8)) & 0x1) << i);
+ bit_pos++;
+ }
+ ppet->ppet16_ppet8_ru3_ru0[nss] |=
+ (val << (ru * ppet_bit_len_per_ru));
+ }
+ }
+}
+
+static void ath12k_peer_assoc_h_eht(struct ath12k *ar,
+ struct ieee80211_vif *vif,
+ struct ieee80211_sta *sta,
+ struct ath12k_wmi_peer_assoc_arg *arg)
+{
+ const struct ieee80211_sta_eht_cap *eht_cap = &sta->deflink.eht_cap;
+ const struct ieee80211_sta_he_cap *he_cap = &sta->deflink.he_cap;
+ const struct ieee80211_eht_mcs_nss_supp_bw *bw;
+ u32 *rx_mcs, *tx_mcs;
+
+ if (!sta->deflink.he_cap.has_he || !eht_cap->has_eht)
+ return;
+
+ arg->eht_flag = true;
+
+ if ((eht_cap->eht_cap_elem.phy_cap_info[5] &
+ IEEE80211_EHT_PHY_CAP5_PPE_THRESHOLD_PRESENT) &&
+ eht_cap->eht_ppe_thres[0] != 0)
+ ath12k_mac_set_eht_ppe_threshold(eht_cap->eht_ppe_thres,
+ &arg->peer_eht_ppet);
+
+ memcpy(arg->peer_eht_cap_mac, eht_cap->eht_cap_elem.mac_cap_info,
+ sizeof(eht_cap->eht_cap_elem.mac_cap_info));
+ memcpy(arg->peer_eht_cap_phy, eht_cap->eht_cap_elem.phy_cap_info,
+ sizeof(eht_cap->eht_cap_elem.phy_cap_info));
+
+ rx_mcs = arg->peer_eht_rx_mcs_set;
+ tx_mcs = arg->peer_eht_tx_mcs_set;
+
+ switch (sta->deflink.bandwidth) {
+ case IEEE80211_STA_RX_BW_320:
+ bw = &eht_cap->eht_mcs_nss_supp.bw._320;
+ ath12k_mac_set_eht_mcs(bw->rx_tx_mcs9_max_nss,
+ bw->rx_tx_mcs9_max_nss,
+ bw->rx_tx_mcs11_max_nss,
+ bw->rx_tx_mcs13_max_nss,
+ &rx_mcs[WMI_EHTCAP_TXRX_MCS_NSS_IDX_320],
+ &tx_mcs[WMI_EHTCAP_TXRX_MCS_NSS_IDX_320]);
+ arg->peer_eht_mcs_count++;
+ fallthrough;
+ case IEEE80211_STA_RX_BW_160:
+ bw = &eht_cap->eht_mcs_nss_supp.bw._160;
+ ath12k_mac_set_eht_mcs(bw->rx_tx_mcs9_max_nss,
+ bw->rx_tx_mcs9_max_nss,
+ bw->rx_tx_mcs11_max_nss,
+ bw->rx_tx_mcs13_max_nss,
+ &rx_mcs[WMI_EHTCAP_TXRX_MCS_NSS_IDX_160],
+ &tx_mcs[WMI_EHTCAP_TXRX_MCS_NSS_IDX_160]);
+ arg->peer_eht_mcs_count++;
+ fallthrough;
+ default:
+ if ((he_cap->he_cap_elem.phy_cap_info[0] &
+ (IEEE80211_HE_PHY_CAP0_CHANNEL_WIDTH_SET_40MHZ_IN_2G |
+ IEEE80211_HE_PHY_CAP0_CHANNEL_WIDTH_SET_40MHZ_80MHZ_IN_5G |
+ IEEE80211_HE_PHY_CAP0_CHANNEL_WIDTH_SET_160MHZ_IN_5G |
+ IEEE80211_HE_PHY_CAP0_CHANNEL_WIDTH_SET_80PLUS80_MHZ_IN_5G)) == 0) {
+ const struct ieee80211_eht_mcs_nss_supp_20mhz_only *bw_20 =
+ &eht_cap->eht_mcs_nss_supp.only_20mhz;
+
+ ath12k_mac_set_eht_mcs(bw_20->rx_tx_mcs7_max_nss,
+ bw_20->rx_tx_mcs9_max_nss,
+ bw_20->rx_tx_mcs11_max_nss,
+ bw_20->rx_tx_mcs13_max_nss,
+ &rx_mcs[WMI_EHTCAP_TXRX_MCS_NSS_IDX_80],
+ &tx_mcs[WMI_EHTCAP_TXRX_MCS_NSS_IDX_80]);
+ } else {
+ bw = &eht_cap->eht_mcs_nss_supp.bw._80;
+ ath12k_mac_set_eht_mcs(bw->rx_tx_mcs9_max_nss,
+ bw->rx_tx_mcs9_max_nss,
+ bw->rx_tx_mcs11_max_nss,
+ bw->rx_tx_mcs13_max_nss,
+ &rx_mcs[WMI_EHTCAP_TXRX_MCS_NSS_IDX_80],
+ &tx_mcs[WMI_EHTCAP_TXRX_MCS_NSS_IDX_80]);
+ }
+
+ arg->peer_eht_mcs_count++;
+ break;
+ }
+}
+
static void ath12k_peer_assoc_prepare(struct ath12k *ar,
struct ieee80211_vif *vif,
struct ieee80211_sta *sta,
@@ -2086,6 +2229,7 @@ static void ath12k_peer_assoc_prepare(struct ath12k *ar,
ath12k_peer_assoc_h_ht(ar, vif, sta, arg);
ath12k_peer_assoc_h_vht(ar, vif, sta, arg);
ath12k_peer_assoc_h_he(ar, vif, sta, arg);
+ ath12k_peer_assoc_h_eht(ar, vif, sta, arg);
ath12k_peer_assoc_h_qos(ar, vif, sta, arg);
ath12k_peer_assoc_h_phymode(ar, vif, sta, arg);
ath12k_peer_assoc_h_smps(sta, arg);
diff --git a/drivers/net/wireless/ath/ath12k/wmi.h b/drivers/net/wireless/ath/ath12k/wmi.h
index 67604224046b..23cdb3543fc2 100644
--- a/drivers/net/wireless/ath/ath12k/wmi.h
+++ b/drivers/net/wireless/ath/ath12k/wmi.h
@@ -2583,6 +2583,7 @@ struct ath12k_wmi_soc_hal_reg_caps_params {

#define WMI_MAX_EHTCAP_MAC_SIZE 2
#define WMI_MAX_EHTCAP_PHY_SIZE 3
+#define WMI_MAX_EHTCAP_RATE_SET 3

/* Used for EHT MCS-NSS array. Data at each array index follows the format given
* in IEEE P802.11be/D2.0, May 20229.4.2.313.4.
@@ -2596,6 +2597,15 @@ struct ath12k_wmi_soc_hal_reg_caps_params {
#define WMI_MAX_EHT_SUPP_MCS_2G_SIZE 2
#define WMI_MAX_EHT_SUPP_MCS_5G_SIZE 4

+#define WMI_EHTCAP_TXRX_MCS_NSS_IDX_80 0
+#define WMI_EHTCAP_TXRX_MCS_NSS_IDX_160 1
+#define WMI_EHTCAP_TXRX_MCS_NSS_IDX_320 2
+
+#define WMI_EHT_MCS_NSS_0_7 GENMASK(3, 0)
+#define WMI_EHT_MCS_NSS_8_9 GENMASK(7, 4)
+#define WMI_EHT_MCS_NSS_10_11 GENMASK(11, 8)
+#define WMI_EHT_MCS_NSS_12_13 GENMASK(15, 12)
+
struct ath12k_wmi_caps_ext_params {
__le32 hw_mode_id;
union {
@@ -3565,6 +3575,13 @@ struct ath12k_wmi_peer_assoc_arg {
bool twt_responder;
bool twt_requester;
struct ath12k_wmi_ppe_threshold_arg peer_ppet;
+ bool eht_flag;
+ u32 peer_eht_cap_mac[WMI_MAX_EHTCAP_MAC_SIZE];
+ u32 peer_eht_cap_phy[WMI_MAX_EHTCAP_PHY_SIZE];
+ u32 peer_eht_mcs_count;
+ u32 peer_eht_rx_mcs_set[WMI_MAX_EHTCAP_RATE_SET];
+ u32 peer_eht_tx_mcs_set[WMI_MAX_EHTCAP_RATE_SET];
+ struct ath12k_wmi_ppe_threshold_arg peer_eht_ppet;
};

struct wmi_peer_assoc_complete_cmd {
--
2.39.0



2023-07-26 03:59:26

by Wen Gong

[permalink] [raw]
Subject: Re: [PATCH v5 06/11] wifi: ath12k: prepare EHT peer assoc parameters

On 7/26/2023 6:40 AM, Aloka Dixit wrote:
[...]
> +
> +static void ath12k_peer_assoc_h_eht(struct ath12k *ar,
> + struct ieee80211_vif *vif,
> + struct ieee80211_sta *sta,
> + struct ath12k_wmi_peer_assoc_arg *arg)
> +{
> + const struct ieee80211_sta_eht_cap *eht_cap = &sta->deflink.eht_cap;
> + const struct ieee80211_sta_he_cap *he_cap = &sta->deflink.he_cap;
[...]
> + default:
> + if ((he_cap->he_cap_elem.phy_cap_info[0] &
> + (IEEE80211_HE_PHY_CAP0_CHANNEL_WIDTH_SET_40MHZ_IN_2G |
> + IEEE80211_HE_PHY_CAP0_CHANNEL_WIDTH_SET_40MHZ_80MHZ_IN_5G |
> + IEEE80211_HE_PHY_CAP0_CHANNEL_WIDTH_SET_160MHZ_IN_5G |
> + IEEE80211_HE_PHY_CAP0_CHANNEL_WIDTH_SET_80PLUS80_MHZ_IN_5G)) == 0) {
> + const struct ieee80211_eht_mcs_nss_supp_20mhz_only *bw_20 =
> + &eht_cap->eht_mcs_nss_supp.only_20mhz;
> +
This is the IEs of my AP below, then it will still entered into this
branch for my ath12k station mode.
sta->deflink.eht_cap and sta->deflink.he_cap is copied from peer remote,
for ath12k station mode, peer
remote is the AP. The field only_20mhz is only valid when the IEs is
from a station which only support
20 MHz.

I think the flag from_ap should be added here as well as function
ieee80211_eht_mcs_nss_size().

Please correct me if wrong.

Ext Tag: HE Capabilities (IEEE Std 802.11ax/D3.0)
    Tag Number: Element ID Extension (255)
    Ext Tag length: 29
    Ext Tag Number: HE Capabilities (IEEE Std 802.11ax/D3.0) (35)
    HE MAC Capabilities Information: 0x10401a08010d
    HE Phy Capabilities Information
        .... ...0 = Reserved: 0x0
        0000 000. = Channel Width Set: 0x00
            .... ..0. = 40MHz in 2.4GHz band: Not supported
            .... .0.. = 40 & 80MHz in the 5GHz or 6GHz band: Not supported
            .... 0... = 160MHz in the 5GHz or 6GHz band: Not supported
            ...0 .... = 160/80+80MHz in the 5GHz or 6GHz band: Not
supported
            ..0. .... = 242 tone RUs in the 2.4GHz band: Not supported
            .0.. .... = 242 tone RUs in the 5GHz or 6GHz band: Not
supported
            0... .... = Reserved: 0x0
        Bits 8 to 23: 0x4063
        Bits 24 to 39: 0x1f88
        Bits 40 to 55: 0x8141
        Bits 56 to 71: 0x111c
        Bits 72 to 87: 0x0008
    Supported HE-MCS and NSS Set
    PPE Thresholds

Ext Tag: EHT Capabilities (IEEE Std 802.11be/D2.0)
    Tag Number: Element ID Extension (255)
    Ext Tag length: 15
    Ext Tag Number: EHT Capabilities (IEEE Std 802.11be/D2.0) (108)
    EHT MAC Capabilities Information: 0x0000
    EHT PHY Capabilities Information
    Supported EHT-MCS and NSS Set
        EHT-MCS Map (BW <= 80MHz): 0x222222
            .... .... .... .... .... 0010 = Rx Max Nss That Supports
EHT-MCS 0-9: 2
            .... .... .... .... 0010 .... = Tx Max Nss That Supports
EHT-MCS 0-9: 2
            .... .... .... 0010 .... .... = Rx Max Nss That Supports
EHT-MCS 10-11: 2
            .... .... 0010 .... .... .... = Tx Max Nss That Supports
EHT-MCS 10-11: 2
            .... 0010 .... .... .... .... = Rx Max Nss That Supports
EHT-MCS 12-13: 2
            0010 .... .... .... .... .... = Tx Max Nss That Supports
EHT-MCS 12-13: 2
    EHT PPE Thresholds: <MISSING>

> + ath12k_mac_set_eht_mcs(bw_20->rx_tx_mcs7_max_nss,
> + bw_20->rx_tx_mcs9_max_nss,
> + bw_20->rx_tx_mcs11_max_nss,
> + bw_20->rx_tx_mcs13_max_nss,
> + &rx_mcs[WMI_EHTCAP_TXRX_MCS_NSS_IDX_80],
> + &tx_mcs[WMI_EHTCAP_TXRX_MCS_NSS_IDX_80]);
> + } else {
> + bw = &eht_cap->eht_mcs_nss_supp.bw._80;
> + ath12k_mac_set_eht_mcs(bw->rx_tx_mcs9_max_nss,
> + bw->rx_tx_mcs9_max_nss,
> + bw->rx_tx_mcs11_max_nss,
> + bw->rx_tx_mcs13_max_nss,
> + &rx_mcs[WMI_EHTCAP_TXRX_MCS_NSS_IDX_80],
> + &tx_mcs[WMI_EHTCAP_TXRX_MCS_NSS_IDX_80]);
> + }
> +
> + arg->peer_eht_mcs_count++;
> + break;
> + }
> +}
> +
[...]

2023-07-26 16:45:46

by Aloka Dixit

[permalink] [raw]
Subject: Re: [PATCH v5 06/11] wifi: ath12k: prepare EHT peer assoc parameters

On 7/25/2023 8:43 PM, Wen Gong wrote:
> On 7/26/2023 6:40 AM, Aloka Dixit wrote:
> [...]
>> +
>> +static void ath12k_peer_assoc_h_eht(struct ath12k *ar,
>> +                    struct ieee80211_vif *vif,
>> +                    struct ieee80211_sta *sta,
>> +                    struct ath12k_wmi_peer_assoc_arg *arg)
>> +{
>> +    const struct ieee80211_sta_eht_cap *eht_cap = &sta->deflink.eht_cap;
>> +    const struct ieee80211_sta_he_cap *he_cap = &sta->deflink.he_cap;
> [...]
>> +    default:
>> +        if ((he_cap->he_cap_elem.phy_cap_info[0] &
>> +             (IEEE80211_HE_PHY_CAP0_CHANNEL_WIDTH_SET_40MHZ_IN_2G |
>> +
>> IEEE80211_HE_PHY_CAP0_CHANNEL_WIDTH_SET_40MHZ_80MHZ_IN_5G |
>> +              IEEE80211_HE_PHY_CAP0_CHANNEL_WIDTH_SET_160MHZ_IN_5G |
>> +
>> IEEE80211_HE_PHY_CAP0_CHANNEL_WIDTH_SET_80PLUS80_MHZ_IN_5G)) == 0) {
>> +            const struct ieee80211_eht_mcs_nss_supp_20mhz_only *bw_20 =
>> +                    &eht_cap->eht_mcs_nss_supp.only_20mhz;
>> +
> This is the IEs of my AP below, then it will still entered into this
> branch for my ath12k station mode.
> sta->deflink.eht_cap and sta->deflink.he_cap is copied from peer remote,
> for ath12k station mode, peer
> remote is the AP. The field only_20mhz is only valid when the IEs is
> from a station which only support
> 20 MHz.
>
> I think the flag from_ap should be added here as well as function
> ieee80211_eht_mcs_nss_size().
>
> Please correct me if wrong.
>

Okay, can you fix this in a follow-up patch?
I don't have a device to test the station mode to verify a fix.
Thanks.

2023-07-27 06:10:58

by Wen Gong

[permalink] [raw]
Subject: Re: [PATCH v5 06/11] wifi: ath12k: prepare EHT peer assoc parameters

On 7/27/2023 12:38 AM, Aloka Dixit wrote:
> On 7/25/2023 8:43 PM, Wen Gong wrote:
>> On 7/26/2023 6:40 AM, Aloka Dixit wrote:
>> [...]
>>> +
>>> +static void ath12k_peer_assoc_h_eht(struct ath12k *ar,
>>> +                    struct ieee80211_vif *vif,
>>> +                    struct ieee80211_sta *sta,
>>> +                    struct ath12k_wmi_peer_assoc_arg *arg)
>>> +{
>>> +    const struct ieee80211_sta_eht_cap *eht_cap =
>>> &sta->deflink.eht_cap;
>>> +    const struct ieee80211_sta_he_cap *he_cap = &sta->deflink.he_cap;
>> [...]
>>> +    default:
>>> +        if ((he_cap->he_cap_elem.phy_cap_info[0] &
>>> + (IEEE80211_HE_PHY_CAP0_CHANNEL_WIDTH_SET_40MHZ_IN_2G |
>>> + IEEE80211_HE_PHY_CAP0_CHANNEL_WIDTH_SET_40MHZ_80MHZ_IN_5G |
>>> + IEEE80211_HE_PHY_CAP0_CHANNEL_WIDTH_SET_160MHZ_IN_5G |
>>> + IEEE80211_HE_PHY_CAP0_CHANNEL_WIDTH_SET_80PLUS80_MHZ_IN_5G)) == 0) {
>>> +            const struct ieee80211_eht_mcs_nss_supp_20mhz_only
>>> *bw_20 =
>>> + &eht_cap->eht_mcs_nss_supp.only_20mhz;
>>> +
>> This is the IEs of my AP below, then it will still entered into this
>> branch for my ath12k station mode.
>> sta->deflink.eht_cap and sta->deflink.he_cap is copied from peer
>> remote, for ath12k station mode, peer
>> remote is the AP. The field only_20mhz is only valid when the IEs is
>> from a station which only support
>> 20 MHz.
>>
>> I think the flag from_ap should be added here as well as function
>> ieee80211_eht_mcs_nss_size().
>>
>> Please correct me if wrong.
>>
>
> Okay, can you fix this in a follow-up patch?
> I don't have a device to test the station mode to verify a fix.
> Thanks.
The fix patch is simple like this (I have verified OK), you can merge it
to your patch.????

It is to not use only_20mhz when ath12k is station mode.

diff --git a/drivers/net/wireless/ath/ath12k/mac.c
b/drivers/net/wireless/ath/ath12k/mac.c
index f4226d0a4726..7e099abb99e6 100644
--- a/drivers/net/wireless/ath/ath12k/mac.c
+++ b/drivers/net/wireless/ath/ath12k/mac.c
@@ -2482,6 +2482,7 @@ static void ath12k_peer_assoc_h_eht(struct ath12k *ar,
        const struct ieee80211_eht_mcs_nss_supp_bw *mcs_nss_supp_bw;
        u8 mcs_idx = WMI_EHTCAP_TXRX_MCS_NSS_IDX_80;
+       bool is_local_sta = arvif->vif->type == NL80211_IFTYPE_STATION;

        eht_cap = &sta->link[link_id]->eht_cap;

@@ -2522,7 +2523,7 @@ static void ath12k_peer_assoc_h_eht(struct ath12k *ar,
                fallthrough;

        default:
-               if ((he_cap->he_cap_elem.phy_cap_info[0] &
+               if (!is_local_sta &&
(he_cap->he_cap_elem.phy_cap_info[0] &) {


2023-07-27 17:08:09

by Aloka Dixit

[permalink] [raw]
Subject: Re: [PATCH v5 06/11] wifi: ath12k: prepare EHT peer assoc parameters

On 7/26/2023 10:48 PM, Wen Gong wrote:
> On 7/27/2023 12:38 AM, Aloka Dixit wrote:
>> On 7/25/2023 8:43 PM, Wen Gong wrote:
>>> On 7/26/2023 6:40 AM, Aloka Dixit wrote:
>>> [...]
>>>> +
>>>> +static void ath12k_peer_assoc_h_eht(struct ath12k *ar,
>>>> +                    struct ieee80211_vif *vif,
>>>> +                    struct ieee80211_sta *sta,
>>>> +                    struct ath12k_wmi_peer_assoc_arg *arg)
>>>> +{
>>>> +
>>> This is the IEs of my AP below, then it will still entered into this
>>> branch for my ath12k station mode.
>>> sta->deflink.eht_cap and sta->deflink.he_cap is copied from peer
>>> remote, for ath12k station mode, peer
>>> remote is the AP. The field only_20mhz is only valid when the IEs is
>>> from a station which only support
>>> 20 MHz.
>>>
>>> I think the flag from_ap should be added here as well as function
>>> ieee80211_eht_mcs_nss_size().
>>>
>>> Please correct me if wrong.
>>>
>>
>> Okay, can you fix this in a follow-up patch?
>> I don't have a device to test the station mode to verify a fix.
>> Thanks.
> The fix patch is simple like this (I have verified OK), you can merge it
> to your patch.????
>
> It is to not use only_20mhz when ath12k is station mode.
>

Thanks Wen.

It would be simpler if the current version, which works for the majority
of cases for both AP and non-AP STA mode, gets reviewed first
considering 10/11 patches will remain same for the next version.
Every time I re-base the series I have to test, add cover-letter,
change-log in 11 patches. Seems like an overkill for a non-crash related
fix which can be reviewed separately.

Let's see what Kalle thinks.


2023-07-28 03:07:19

by Wen Gong

[permalink] [raw]
Subject: Re: [PATCH v5 06/11] wifi: ath12k: prepare EHT peer assoc parameters

On 7/28/2023 12:59 AM, Aloka Dixit wrote:
> On 7/26/2023 10:48 PM, Wen Gong wrote:
>> On 7/27/2023 12:38 AM, Aloka Dixit wrote:
>>> On 7/25/2023 8:43 PM, Wen Gong wrote:
>>>> On 7/26/2023 6:40 AM, Aloka Dixit wrote:
>>>> [...]
>>>>> +
>>>>> +static void ath12k_peer_assoc_h_eht(struct ath12k *ar,
>>>>> +                    struct ieee80211_vif *vif,
>>>>> +                    struct ieee80211_sta *sta,
>>>>> +                    struct ath12k_wmi_peer_assoc_arg *arg)
>>>>> +{
>>>>> +
>>>> This is the IEs of my AP below, then it will still entered into
>>>> this branch for my ath12k station mode.
>>>> sta->deflink.eht_cap and sta->deflink.he_cap is copied from peer
>>>> remote, for ath12k station mode, peer
>>>> remote is the AP. The field only_20mhz is only valid when the IEs
>>>> is from a station which only support
>>>> 20 MHz.
>>>>
>>>> I think the flag from_ap should be added here as well as function
>>>> ieee80211_eht_mcs_nss_size().
>>>>
>>>> Please correct me if wrong.
>>>>
>>>
>>> Okay, can you fix this in a follow-up patch?
>>> I don't have a device to test the station mode to verify a fix.
>>> Thanks.
>> The fix patch is simple like this (I have verified OK), you can merge
>> it to your patch.????
>>
>> It is to not use only_20mhz when ath12k is station mode.
>>
>
> Thanks Wen.
>
> It would be simpler if the current version, which works for the
> majority of cases for both AP and non-AP STA mode, gets reviewed first
> considering 10/11 patches will remain same for the next version.
> Every time I re-base the series I have to test, add cover-letter,
> change-log in 11 patches. Seems like an overkill for a non-crash
> related fix which can be reviewed separately.
>
> Let's see what Kalle thinks.

The change is simple here, and I have verified on WCN7850 station mode
successfully.

I think you do not need re-base and re-test????

Anyway, let's see what Kalle thinks.

@ -2482,6 +2482,7 @@ static void ath12k_peer_assoc_h_eht(struct ath12k *ar,
        const struct ieee80211_eht_mcs_nss_supp_bw *mcs_nss_supp_bw;
        u8 mcs_idx = WMI_EHTCAP_TXRX_MCS_NSS_IDX_80;
+       bool is_local_sta = arvif->vif->type == NL80211_IFTYPE_STATION;

        eht_cap = &sta->link[link_id]->eht_cap;

@@ -2522,7 +2523,7 @@ static void ath12k_peer_assoc_h_eht(struct ath12k
*ar,
                fallthrough;

        default:
-               if ((he_cap->he_cap_elem.phy_cap_info[0] &
+               if (!is_local_sta &&
(he_cap->he_cap_elem.phy_cap_info[0] &) {


2023-08-03 09:17:25

by Kalle Valo

[permalink] [raw]
Subject: Re: [PATCH v5 06/11] wifi: ath12k: prepare EHT peer assoc parameters

Aloka Dixit <[email protected]> writes:

> On 7/26/2023 10:48 PM, Wen Gong wrote:
>
>> On 7/27/2023 12:38 AM, Aloka Dixit wrote:
>>> On 7/25/2023 8:43 PM, Wen Gong wrote:
>>>> On 7/26/2023 6:40 AM, Aloka Dixit wrote:
>>>> [...]
>>>>> +
>>>>> +static void ath12k_peer_assoc_h_eht(struct ath12k *ar,
>>>>> +                    struct ieee80211_vif *vif,
>>>>> +                    struct ieee80211_sta *sta,
>>>>> +                    struct ath12k_wmi_peer_assoc_arg *arg)
>>>>> +{
>>>>> +
>>>> This is the IEs of my AP below, then it will still entered into
>>>> this branch for my ath12k station mode.
>>>> sta->deflink.eht_cap and sta->deflink.he_cap is copied from peer
>>>> remote, for ath12k station mode, peer
>>>> remote is the AP. The field only_20mhz is only valid when the IEs
>>>> is from a station which only support
>>>> 20 MHz.
>>>>
>>>> I think the flag from_ap should be added here as well as function
>>>> ieee80211_eht_mcs_nss_size().
>>>>
>>>> Please correct me if wrong.
>>>>
>>>
>>> Okay, can you fix this in a follow-up patch?
>>> I don't have a device to test the station mode to verify a fix.
>>> Thanks.
>> The fix patch is simple like this (I have verified OK), you can
>> merge it to your patch.????
>> It is to not use only_20mhz when ath12k is station mode.

Wen, please submit a follow-up patch which explains the situation
clearly. The reasoning for your change is not obvious for me. Also
consider if there should be a comment in the code.

> Every time I re-base the series I have to test, add cover-letter,
> change-log in 11 patches.

BTW you don't need to add a changelog on every patch. IMHO it's a lot
easier for everyone if all the changes are listed in the cover letter,
no need repeat that information in individual patches.

--
https://patchwork.kernel.org/project/linux-wireless/list/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches