Currently, the transmit rate, the receive rate and signal strength of
“iw dev xxx station dump” always show an invalid value.
This is because ath12k has no logic to handle this relevant information.
To solve this issue, ath12k parses the information passed by the firmware
and passes it to mac80211.
After that, "iw dev xxx station dump" show the correct value.
Such as:
Station 00:03:7f:12:03:03 (on wlo1)
inactive time: 600 ms
rx bytes: 4642228
rx packets: 23796
tx bytes: 933967
tx packets: 8761
tx retries: 66
tx failed: 0
beacon loss: 0
beacon rx: 8925
rx drop misc: 191
signal: -20 dBm
beacon signal avg: -18 dBm
tx bitrate: 1441.1 MBit/s 80MHz EHT-MCS 13 EHT-NSS 2 EHT-GI 0
tx duration: 0 us
rx bitrate: 1801.4 MBit/s 80MHz EHT-MCS 11 EHT-NSS 3 EHT-GI 0
rx duration: 0 us
v4:
1.rebase ath-pending-202404181533
2.change ATH12K_EHT_MCS_MAX from 13 to 15
v3:
1.change wmi_vdev_stats_event to wmi_vdev_stats_params
v2:
1.change copyright
2.change name according Naming conventions for structures
Lingbo Kong (3):
wifi: ath12k: report station mode transmit rate
wifi: ath12k: report station mode receive rate for IEEE 802.11be
wifi: ath12k: report station mode signal strength
drivers/net/wireless/ath/ath12k/core.h | 5 +
drivers/net/wireless/ath/ath12k/dp_rx.c | 20 ++-
drivers/net/wireless/ath/ath12k/dp_rx.h | 3 +
drivers/net/wireless/ath/ath12k/dp_tx.c | 147 +++++++++++++++++-
drivers/net/wireless/ath/ath12k/hal_tx.h | 9 +-
drivers/net/wireless/ath/ath12k/mac.c | 180 +++++++++++++++++++++-
drivers/net/wireless/ath/ath12k/mac.h | 4 +-
drivers/net/wireless/ath/ath12k/rx_desc.h | 2 +
drivers/net/wireless/ath/ath12k/wmi.c | 130 ++++++++++++++++
drivers/net/wireless/ath/ath12k/wmi.h | 48 ++++++
10 files changed, 538 insertions(+), 10 deletions(-)
base-commit: f5f3a3166c64d469150958a470f4a3ab99d45268
--
2.34.1
Currently, the receive rate of EHT of "iw dev xxx station dump" command
always show an invalid value.
This is because ath12k does not pass information about the rx_status of EHT
to mac80211. So, mac80211 not calculate the receive rate.
To address this issue, add logic for handling rx_status of EHT to the
ath12k_dp_rx_h_rate() function.
After that, "iw dev xxx station dump" show the correct receive rate.
Such as:
Station 00:03:7f:12:03:03 (on wlo1)
inactive time: 48 ms
rx bytes: 59226
rx packets: 320
tx bytes: 26556
tx packets: 191
tx retries: 99
tx failed: 0
beacon loss: 0
beacon rx: 79
rx drop misc: 68
signal: -95 dBm
beacon signal avg: -20 dBm
tx bitrate: 688.2 MBit/s 40MHz EHT-MCS 13 EHT-NSS 2 EHT-GI 0
tx duration: 0 us
rx bitrate: 619.5 MBit/s 40MHz EHT-MCS 8 EHT-NSS 3 EHT-GI 0
This patch affects the station mode of WCN7850 and QCN9274.
Tested-on: WCN7850 hw2.0 PCI WLAN.HMT.1.0.c5-00481-QCAHMTSWPL_V1.0_V2.0_SILICONZ-3
Tested-on: QCN9274 hw2.0 PCI WLAN.WBE.1.2.1-00201-QCAHKSWPL_SILICONZ-1
Signed-off-by: Lingbo Kong <[email protected]>
---
v4:
no change
v3:
no change
v2:
no change
drivers/net/wireless/ath/ath12k/dp_rx.c | 20 +++++++++++++++++++-
drivers/net/wireless/ath/ath12k/rx_desc.h | 2 ++
2 files changed, 21 insertions(+), 1 deletion(-)
diff --git a/drivers/net/wireless/ath/ath12k/dp_rx.c b/drivers/net/wireless/ath/ath12k/dp_rx.c
index 75df622f25d8..e89bf08a1f2f 100644
--- a/drivers/net/wireless/ath/ath12k/dp_rx.c
+++ b/drivers/net/wireless/ath/ath12k/dp_rx.c
@@ -2359,6 +2359,23 @@ static void ath12k_dp_rx_h_rate(struct ath12k *ar, struct hal_rx_desc *rx_desc,
rx_status->he_gi = ath12k_he_gi_to_nl80211_he_gi(sgi);
rx_status->bw = ath12k_mac_bw_to_mac80211_bw(bw);
break;
+ case RX_MSDU_START_PKT_TYPE_11BE:
+ rx_status->rate_idx = rate_mcs;
+
+ if (rate_mcs > ATH12K_EHT_MCS_MAX) {
+ ath12k_warn(ar->ab,
+ "Received with invalid mcs in EHT mode %d\n",
+ rate_mcs);
+ break;
+ }
+
+ rx_status->encoding = RX_ENC_EHT;
+ rx_status->nss = nss;
+ rx_status->eht.gi = ath12k_mac_eht_gi_to_nl80211_eht_gi(sgi);
+ rx_status->bw = ath12k_mac_bw_to_mac80211_bw(bw);
+ break;
+ default:
+ break;
}
}
@@ -2445,7 +2462,7 @@ static void ath12k_dp_rx_deliver_msdu(struct ath12k *ar, struct napi_struct *nap
spin_unlock_bh(&ab->base_lock);
ath12k_dbg(ab, ATH12K_DBG_DATA,
- "rx skb %p len %u peer %pM %d %s sn %u %s%s%s%s%s%s%s%s%s rate_idx %u vht_nss %u freq %u band %u flag 0x%x fcs-err %i mic-err %i amsdu-more %i\n",
+ "rx skb %p len %u peer %pM %d %s sn %u %s%s%s%s%s%s%s%s%s%s rate_idx %u vht_nss %u freq %u band %u flag 0x%x fcs-err %i mic-err %i amsdu-more %i\n",
msdu,
msdu->len,
peer ? peer->addr : NULL,
@@ -2456,6 +2473,7 @@ static void ath12k_dp_rx_deliver_msdu(struct ath12k *ar, struct napi_struct *nap
(status->encoding == RX_ENC_HT) ? "ht" : "",
(status->encoding == RX_ENC_VHT) ? "vht" : "",
(status->encoding == RX_ENC_HE) ? "he" : "",
+ (status->encoding == RX_ENC_EHT) ? "eht" : "",
(status->bw == RATE_INFO_BW_40) ? "40" : "",
(status->bw == RATE_INFO_BW_80) ? "80" : "",
(status->bw == RATE_INFO_BW_160) ? "160" : "",
diff --git a/drivers/net/wireless/ath/ath12k/rx_desc.h b/drivers/net/wireless/ath/ath12k/rx_desc.h
index a0db6702a189..e620f4794286 100644
--- a/drivers/net/wireless/ath/ath12k/rx_desc.h
+++ b/drivers/net/wireless/ath/ath12k/rx_desc.h
@@ -637,6 +637,8 @@ enum rx_msdu_start_pkt_type {
RX_MSDU_START_PKT_TYPE_11N,
RX_MSDU_START_PKT_TYPE_11AC,
RX_MSDU_START_PKT_TYPE_11AX,
+ RX_MSDU_START_PKT_TYPE_11BA,
+ RX_MSDU_START_PKT_TYPE_11BE,
};
enum rx_msdu_start_sgi {
--
2.34.1
Currently, the signal strength of "iw dev xxx station dump" always show an
invalid value.
This is because signal strength is only set in ath12k_mgmt_rx_event()
function, and not set for received data packet. So, change to get signal
from firmware and report to mac80211.
After that, "iw dev xxx station dump" show the correct signal strength.
Such as:
Station 00:03:7f:12:03:03 (on wlo1)
inactive time: 36 ms
rx bytes: 61571
rx packets: 336
tx bytes: 28204
tx packets: 205
tx retries: 49
tx failed: 0
beacon loss: 0
beacon rx: 83
rx drop misc: 66
signal: -24 dBm
beacon signal avg: -22 dBm
For WCN7850, the firmware supports db2dbm, so not need to add noise floor.
For QCN9274, the firmware not support db2dbm, so need to add noise floor.
This patch affects the station mode of WCN7850 and QCN9274.
Tested-on: WCN7850 hw2.0 PCI WLAN.HMT.1.0.c5-00481-QCAHMTSWPL_V1.0_V2.0_SILICONZ-3
Tested-on: QCN9274 hw2.0 PCI WLAN.WBE.1.2.1-00201-QCAHKSWPL_SILICONZ-1
Signed-off-by: Lingbo Kong <[email protected]>
---
v4:
1.no change
v3:
1.change wmi_vdev_stats_event to wmi_vdev_stats_params
v2:
1.change name according Naming conventions for structures
drivers/net/wireless/ath/ath12k/core.h | 3 +
drivers/net/wireless/ath/ath12k/mac.c | 56 ++++++++++-
drivers/net/wireless/ath/ath12k/wmi.c | 130 +++++++++++++++++++++++++
drivers/net/wireless/ath/ath12k/wmi.h | 48 +++++++++
4 files changed, 235 insertions(+), 2 deletions(-)
diff --git a/drivers/net/wireless/ath/ath12k/core.h b/drivers/net/wireless/ath/ath12k/core.h
index b2ddd1e6fb14..498da0af5e9f 100644
--- a/drivers/net/wireless/ath/ath12k/core.h
+++ b/drivers/net/wireless/ath/ath12k/core.h
@@ -450,6 +450,7 @@ struct ath12k_sta {
struct ath12k_wbm_tx_stats *wbm_tx_stats;
u32 bw_prev;
u32 peer_nss;
+ s8 rssi_beacon;
};
#define ATH12K_MIN_5G_FREQ 4150
@@ -633,6 +634,8 @@ struct ath12k {
u32 freq_low;
u32 freq_high;
+
+ struct completion fw_stats_complete;
};
struct ath12k_hw {
diff --git a/drivers/net/wireless/ath/ath12k/mac.c b/drivers/net/wireless/ath/ath12k/mac.c
index cd13fa48e97d..1d8543d5df4a 100644
--- a/drivers/net/wireless/ath/ath12k/mac.c
+++ b/drivers/net/wireless/ath/ath12k/mac.c
@@ -8170,12 +8170,52 @@ static int ath12k_mac_op_get_survey(struct ieee80211_hw *hw, int idx,
return 0;
}
+static int ath12k_mac_get_fw_stats(struct ath12k *ar, u32 pdev_id,
+ u32 vdev_id, u32 stats_id)
+{
+ struct ath12k_base *ab = ar->ab;
+ int ret, left;
+
+ mutex_lock(&ar->conf_mutex);
+
+ if (ar->state != ATH12K_STATE_ON) {
+ ret = -ENETDOWN;
+ goto err_unlock;
+ }
+
+ reinit_completion(&ar->fw_stats_complete);
+
+ ret = ath12k_wmi_send_stats_request_cmd(ar, stats_id, vdev_id, pdev_id);
+
+ if (ret) {
+ ath12k_warn(ab, "failed to request fw stats: %d\n", ret);
+ goto err_unlock;
+ }
+
+ ath12k_dbg(ab, ATH12K_DBG_WMI,
+ "get fw stat pdev id %d vdev id %d stats id 0x%x\n",
+ pdev_id, vdev_id, stats_id);
+
+ left = wait_for_completion_timeout(&ar->fw_stats_complete, 1 * HZ);
+
+ if (!left)
+ ath12k_warn(ab, "time out while waiting for get fw stats\n");
+err_unlock:
+
+ mutex_unlock(&ar->conf_mutex);
+ return ret;
+}
+
static void ath12k_mac_op_sta_statistics(struct ieee80211_hw *hw,
struct ieee80211_vif *vif,
struct ieee80211_sta *sta,
struct station_info *sinfo)
{
struct ath12k_sta *arsta = ath12k_sta_to_arsta(sta);
+ struct ath12k *ar = arsta->arvif->ar;
+ s8 signal;
+ bool db2dbm = test_bit(WMI_TLV_SERVICE_HW_DB2DBM_CONVERSION_SUPPORT,
+ ar->ab->wmi_ab.svc_map);
sinfo->rx_duration = arsta->rx_duration;
sinfo->filled |= BIT_ULL(NL80211_STA_INFO_RX_DURATION);
@@ -8202,8 +8242,18 @@ static void ath12k_mac_op_sta_statistics(struct ieee80211_hw *hw,
sinfo->filled |= BIT_ULL(NL80211_STA_INFO_TX_BITRATE);
/* TODO: Use real NF instead of default one. */
- sinfo->signal = arsta->rssi_comb + ATH12K_DEFAULT_NOISE_FLOOR;
- sinfo->filled |= BIT_ULL(NL80211_STA_INFO_SIGNAL);
+ signal = arsta->rssi_comb;
+
+ if (!signal &&
+ arsta->arvif->vdev_type == WMI_VDEV_TYPE_STA &&
+ !(ath12k_mac_get_fw_stats(ar, ar->pdev->pdev_id, 0,
+ WMI_REQUEST_VDEV_STAT)))
+ signal = arsta->rssi_beacon;
+
+ if (signal) {
+ sinfo->signal = db2dbm ? signal : signal + ATH12K_DEFAULT_NOISE_FLOOR;
+ sinfo->filled |= BIT_ULL(NL80211_STA_INFO_SIGNAL);
+ }
}
static int ath12k_mac_op_cancel_remain_on_channel(struct ieee80211_hw *hw,
@@ -8895,6 +8945,8 @@ static int ath12k_mac_hw_register(struct ath12k_hw *ah)
ath12k_debugfs_register(ar);
+ init_completion(&ar->fw_stats_complete);
+
return 0;
err_unregister_hw:
diff --git a/drivers/net/wireless/ath/ath12k/wmi.c b/drivers/net/wireless/ath/ath12k/wmi.c
index b4063c2c94be..2ca47037be44 100644
--- a/drivers/net/wireless/ath/ath12k/wmi.c
+++ b/drivers/net/wireless/ath/ath12k/wmi.c
@@ -25,6 +25,10 @@ struct ath12k_wmi_svc_ready_parse {
bool wmi_svc_bitmap_done;
};
+struct wmi_tlv_fw_stats_parse {
+ const struct wmi_stats_event *ev;
+};
+
struct ath12k_wmi_dma_ring_caps_parse {
struct ath12k_wmi_dma_ring_caps_params *dma_ring_caps;
u32 n_dma_ring_caps;
@@ -6461,8 +6465,101 @@ static void ath12k_peer_assoc_conf_event(struct ath12k_base *ab, struct sk_buff
rcu_read_unlock();
}
+static int ath12k_wmi_tlv_fw_stats_data_parse(struct ath12k_base *ab,
+ struct wmi_tlv_fw_stats_parse *parse,
+ const void *ptr,
+ u16 len)
+{
+ const struct wmi_stats_event *ev = parse->ev;
+ struct ath12k *ar;
+ struct ath12k_vif *arvif;
+ struct ieee80211_sta *sta;
+ struct ath12k_sta *arsta;
+ int i, ret = 0;
+ const void *data = ptr;
+
+ if (!ev) {
+ ath12k_warn(ab, "failed to fetch update stats ev");
+ return -EPROTO;
+ }
+
+ rcu_read_lock();
+
+ ar = ath12k_mac_get_ar_by_pdev_id(ab, le32_to_cpu(ev->pdev_id));
+ if (!ar) {
+ ath12k_warn(ab, "invalid pdev id %d in update stats event\n",
+ le32_to_cpu(ev->pdev_id));
+ ret = -EPROTO;
+ goto exit;
+ }
+
+ for (i = 0; i < le32_to_cpu(ev->num_vdev_stats); i++) {
+ const struct wmi_vdev_stats_params *src;
+
+ src = data;
+ if (len < sizeof(*src)) {
+ ret = -EPROTO;
+ goto exit;
+ }
+
+ arvif = ath12k_mac_get_arvif(ar, le32_to_cpu(src->vdev_id));
+ if (arvif) {
+ sta = ieee80211_find_sta_by_ifaddr(ath12k_ar_to_hw(ar),
+ arvif->bssid,
+ NULL);
+ if (sta) {
+ arsta = ath12k_sta_to_arsta(sta);
+ arsta->rssi_beacon = le32_to_cpu(src->beacon_snr);
+ ath12k_dbg(ab, ATH12K_DBG_WMI,
+ "wmi stats vdev id %d snr %d\n",
+ src->vdev_id, src->beacon_snr);
+ } else {
+ ath12k_dbg(ab, ATH12K_DBG_WMI,
+ "not found station bssid %pM for vdev stat\n",
+ arvif->bssid);
+ }
+ }
+
+ data += sizeof(*src);
+ len -= sizeof(*src);
+ }
+
+ complete(&ar->fw_stats_complete);
+exit:
+ rcu_read_unlock();
+ return ret;
+}
+
+static int ath12k_wmi_tlv_fw_stats_parse(struct ath12k_base *ab,
+ u16 tag, u16 len,
+ const void *ptr, void *data)
+{
+ struct wmi_tlv_fw_stats_parse *parse = data;
+ int ret = 0;
+
+ switch (tag) {
+ case WMI_TAG_STATS_EVENT:
+ parse->ev = ptr;
+ break;
+ case WMI_TAG_ARRAY_BYTE:
+ ret = ath12k_wmi_tlv_fw_stats_data_parse(ab, parse, ptr, len);
+ break;
+ default:
+ break;
+ }
+ return ret;
+}
+
static void ath12k_update_stats_event(struct ath12k_base *ab, struct sk_buff *skb)
{
+ int ret;
+ struct wmi_tlv_fw_stats_parse parse = {};
+
+ ret = ath12k_wmi_tlv_iter(ab, skb->data, skb->len,
+ ath12k_wmi_tlv_fw_stats_parse,
+ &parse);
+ if (ret)
+ ath12k_warn(ab, "failed to parse fw stats %d\n", ret);
}
/* PDEV_CTL_FAILSAFE_CHECK_EVENT is received from FW when the frequency scanned
@@ -7185,3 +7282,36 @@ void ath12k_wmi_detach(struct ath12k_base *ab)
ath12k_wmi_free_dbring_caps(ab);
}
+
+int ath12k_wmi_send_stats_request_cmd(struct ath12k *ar, u32 stats_id,
+ u32 vdev_id, u32 pdev_id)
+{
+ struct ath12k_wmi_pdev *wmi = ar->wmi;
+ struct wmi_request_stats_cmd *cmd;
+ struct sk_buff *skb;
+ int ret;
+
+ skb = ath12k_wmi_alloc_skb(wmi->wmi_ab, sizeof(*cmd));
+ if (!skb)
+ return -ENOMEM;
+
+ cmd = (struct wmi_request_stats_cmd *)skb->data;
+ cmd->tlv_header = ath12k_wmi_tlv_cmd_hdr(WMI_TAG_REQUEST_STATS_CMD,
+ sizeof(*cmd));
+
+ cmd->stats_id = cpu_to_le32(stats_id);
+ cmd->vdev_id = cpu_to_le32(vdev_id);
+ cmd->pdev_id = cpu_to_le32(pdev_id);
+
+ ret = ath12k_wmi_cmd_send(wmi, skb, WMI_REQUEST_STATS_CMDID);
+ if (ret) {
+ ath12k_warn(ar->ab, "failed to send WMI_REQUEST_STATS cmd\n");
+ dev_kfree_skb(skb);
+ }
+
+ ath12k_dbg(ar->ab, ATH12K_DBG_WMI,
+ "WMI request stats 0x%x vdev id %d pdev id %d\n",
+ stats_id, vdev_id, pdev_id);
+
+ return ret;
+}
diff --git a/drivers/net/wireless/ath/ath12k/wmi.h b/drivers/net/wireless/ath/ath12k/wmi.h
index 203de8da62b1..b81475310c07 100644
--- a/drivers/net/wireless/ath/ath12k/wmi.h
+++ b/drivers/net/wireless/ath/ath12k/wmi.h
@@ -4831,6 +4831,52 @@ struct wmi_twt_disable_event {
__le32 status;
} __packed;
+struct wmi_stats_event {
+ __le32 stats_id;
+ __le32 num_pdev_stats;
+ __le32 num_vdev_stats;
+ __le32 num_peer_stats;
+ __le32 num_bcnflt_stats;
+ __le32 num_chan_stats;
+ __le32 num_mib_stats;
+ __le32 pdev_id;
+ __le32 num_bcn_stats;
+ __le32 num_peer_extd_stats;
+ __le32 num_peer_extd2_stats;
+} __packed;
+
+enum wmi_stats_id {
+ WMI_REQUEST_VDEV_STAT = BIT(3),
+};
+
+struct wmi_request_stats_cmd {
+ __le32 tlv_header;
+ __le32 stats_id;
+ __le32 vdev_id;
+ struct ath12k_wmi_mac_addr_params peer_macaddr;
+ __le32 pdev_id;
+} __packed;
+
+#define WLAN_MAX_AC 4
+#define MAX_TX_RATE_VALUES 10
+
+struct wmi_vdev_stats_params {
+ __le32 vdev_id;
+ __le32 beacon_snr;
+ __le32 data_snr;
+ __le32 num_tx_frames[WLAN_MAX_AC];
+ __le32 num_rx_frames;
+ __le32 num_tx_frames_retries[WLAN_MAX_AC];
+ __le32 num_tx_frames_failures[WLAN_MAX_AC];
+ __le32 num_rts_fail;
+ __le32 num_rts_success;
+ __le32 num_rx_err;
+ __le32 num_rx_discard;
+ __le32 num_tx_not_acked;
+ __le32 tx_rate_history[MAX_TX_RATE_VALUES];
+ __le32 beacon_rssi_history[MAX_TX_RATE_VALUES];
+} __packed;
+
void ath12k_wmi_init_qcn9274(struct ath12k_base *ab,
struct ath12k_wmi_resource_config_arg *config);
void ath12k_wmi_init_wcn7850(struct ath12k_base *ab,
@@ -4952,6 +4998,8 @@ int ath12k_wmi_probe_resp_tmpl(struct ath12k *ar, u32 vdev_id,
struct sk_buff *tmpl);
int ath12k_wmi_set_hw_mode(struct ath12k_base *ab,
enum wmi_host_hw_mode_config_type mode);
+int ath12k_wmi_send_stats_request_cmd(struct ath12k *ar, u32 stats_id,
+ u32 vdev_id, u32 pdev_id);
static inline u32
ath12k_wmi_caps_ext_get_pdev_id(const struct ath12k_wmi_caps_ext_params *param)
--
2.34.1
Currently, the transmit rate of "iw dev xxx station dump" command
always show an invalid value.
To address this issue, ath12k parse the info of transmit complete
report from firmware and indicate the transmit rate to mac80211.
This patch affects the station mode of WCN7850 and QCN9274.
After that, "iw dev xxx station dump" show the correct transmit rate.
Such as:
Station 00:03:7f:12:03:03 (on wlo1)
inactive time: 872 ms
rx bytes: 219111
rx packets: 1133
tx bytes: 53767
tx packets: 462
tx retries: 51
tx failed: 0
beacon loss: 0
beacon rx: 403
rx drop misc: 74
signal: -95 dBm
beacon signal avg: -18 dBm
tx bitrate: 1441.1 MBit/s 80MHz EHT-MCS 13 EHT-NSS 2 EHT-GI 0
Tested-on: WCN7850 hw2.0 PCI WLAN.HMT.1.0.c5-00481-QCAHMTSWPL_V1.0_V2.0_SILICONZ-3
Tested-on: QCN9274 hw2.0 PCI WLAN.WBE.1.2.1-00201-QCAHKSWPL_SILICONZ-1
Signed-off-by: Lingbo Kong <[email protected]>
---
v4:
1.change ATH12K_EHT_MCS_MAX from 13 to 15
v3:
no change
v2:
1.change copyright
drivers/net/wireless/ath/ath12k/core.h | 2 +
drivers/net/wireless/ath/ath12k/dp_rx.h | 3 +
drivers/net/wireless/ath/ath12k/dp_tx.c | 147 ++++++++++++++++++++++-
drivers/net/wireless/ath/ath12k/hal_tx.h | 9 +-
drivers/net/wireless/ath/ath12k/mac.c | 124 +++++++++++++++++++
drivers/net/wireless/ath/ath12k/mac.h | 4 +-
6 files changed, 282 insertions(+), 7 deletions(-)
diff --git a/drivers/net/wireless/ath/ath12k/core.h b/drivers/net/wireless/ath/ath12k/core.h
index 5d3c1fb632b0..b2ddd1e6fb14 100644
--- a/drivers/net/wireless/ath/ath12k/core.h
+++ b/drivers/net/wireless/ath/ath12k/core.h
@@ -74,6 +74,7 @@ enum wme_ac {
#define ATH12K_HT_MCS_MAX 7
#define ATH12K_VHT_MCS_MAX 9
#define ATH12K_HE_MCS_MAX 11
+#define ATH12K_EHT_MCS_MAX 15
enum ath12k_crypt_mode {
/* Only use hardware crypto engine */
@@ -448,6 +449,7 @@ struct ath12k_sta {
struct ath12k_rx_peer_stats *rx_stats;
struct ath12k_wbm_tx_stats *wbm_tx_stats;
u32 bw_prev;
+ u32 peer_nss;
};
#define ATH12K_MIN_5G_FREQ 4150
diff --git a/drivers/net/wireless/ath/ath12k/dp_rx.h b/drivers/net/wireless/ath/ath12k/dp_rx.h
index 2ff421160181..1543788c0da7 100644
--- a/drivers/net/wireless/ath/ath12k/dp_rx.h
+++ b/drivers/net/wireless/ath/ath12k/dp_rx.h
@@ -79,6 +79,9 @@ static inline u32 ath12k_he_gi_to_nl80211_he_gi(u8 sgi)
case RX_MSDU_START_SGI_3_2_US:
ret = NL80211_RATE_INFO_HE_GI_3_2;
break;
+ default:
+ ret = NL80211_RATE_INFO_HE_GI_0_8;
+ break;
}
return ret;
diff --git a/drivers/net/wireless/ath/ath12k/dp_tx.c b/drivers/net/wireless/ath/ath12k/dp_tx.c
index 9b6d7d72f57c..74ef4c7a72c1 100644
--- a/drivers/net/wireless/ath/ath12k/dp_tx.c
+++ b/drivers/net/wireless/ath/ath12k/dp_tx.c
@@ -8,6 +8,8 @@
#include "dp_tx.h"
#include "debug.h"
#include "hw.h"
+#include "peer.h"
+#include "mac.h"
static enum hal_tcl_encap_type
ath12k_dp_tx_get_encap_type(struct ath12k_vif *arvif, struct sk_buff *skb)
@@ -443,6 +445,125 @@ ath12k_dp_tx_process_htt_tx_complete(struct ath12k_base *ab,
}
}
+static void ath12k_dp_tx_update_txcompl(struct ath12k *ar, struct hal_tx_status *ts)
+{
+ struct ath12k_base *ab = ar->ab;
+ struct ath12k_peer *peer;
+ struct ath12k_sta *arsta;
+ struct ieee80211_sta *sta;
+ u16 rate;
+ u8 rate_idx = 0;
+ int ret;
+
+ spin_lock_bh(&ab->base_lock);
+
+ peer = ath12k_peer_find_by_id(ab, ts->peer_id);
+ if (!peer || !peer->sta) {
+ ath12k_dbg(ab, ATH12K_DBG_DP_TX,
+ "failed to find the peer by id %u\n", ts->peer_id);
+ goto err_out;
+ }
+
+ sta = peer->sta;
+ arsta = ath12k_sta_to_arsta(sta);
+
+ memset(&arsta->txrate, 0, sizeof(arsta->txrate));
+
+ /* This is to prefer choose the real NSS value arsta->last_txrate.nss,
+ * if it is invalid, then choose the NSS value while assoc.
+ */
+ if (arsta->last_txrate.nss)
+ arsta->txrate.nss = arsta->last_txrate.nss;
+ else
+ arsta->txrate.nss = arsta->peer_nss;
+
+ if (ts->pkt_type == HAL_TX_RATE_STATS_PKT_TYPE_11A ||
+ ts->pkt_type == HAL_TX_RATE_STATS_PKT_TYPE_11B) {
+ ret = ath12k_mac_hw_ratecode_to_legacy_rate(ts->mcs,
+ ts->pkt_type,
+ &rate_idx,
+ &rate);
+ if (ret < 0) {
+ ath12k_warn(ab, "Invalid tx legacy rate %d\n", ret);
+ goto err_out;
+ }
+
+ arsta->txrate.legacy = rate;
+ } else if (ts->pkt_type == HAL_TX_RATE_STATS_PKT_TYPE_11N) {
+ if (ts->mcs > ATH12K_HT_MCS_MAX) {
+ ath12k_warn(ab, "Invalid HT mcs index %d\n", ts->mcs);
+ goto err_out;
+ }
+
+ if (arsta->txrate.nss != 0)
+ arsta->txrate.mcs = ts->mcs + 8 * (arsta->txrate.nss - 1);
+
+ arsta->txrate.flags = RATE_INFO_FLAGS_MCS;
+
+ if (ts->sgi)
+ arsta->txrate.flags |= RATE_INFO_FLAGS_SHORT_GI;
+ } else if (ts->pkt_type == HAL_TX_RATE_STATS_PKT_TYPE_11AC) {
+ if (ts->mcs > ATH12K_VHT_MCS_MAX) {
+ ath12k_warn(ab, "Invalid VHT mcs index %d\n", ts->mcs);
+ goto err_out;
+ }
+
+ arsta->txrate.mcs = ts->mcs;
+ arsta->txrate.flags = RATE_INFO_FLAGS_VHT_MCS;
+
+ if (ts->sgi)
+ arsta->txrate.flags |= RATE_INFO_FLAGS_SHORT_GI;
+ } else if (ts->pkt_type == HAL_TX_RATE_STATS_PKT_TYPE_11AX) {
+ if (ts->mcs > ATH12K_HE_MCS_MAX) {
+ ath12k_warn(ab, "Invalid HE mcs index %d\n", ts->mcs);
+ goto err_out;
+ }
+
+ arsta->txrate.mcs = ts->mcs;
+ arsta->txrate.flags = RATE_INFO_FLAGS_HE_MCS;
+ arsta->txrate.he_gi = ath12k_he_gi_to_nl80211_he_gi(ts->sgi);
+ } else if (ts->pkt_type == HAL_TX_RATE_STATS_PKT_TYPE_11BE) {
+ if (ts->mcs > ATH12K_EHT_MCS_MAX) {
+ ath12k_warn(ab, "Invalid EHT mcs index %d\n", ts->mcs);
+ goto err_out;
+ }
+
+ arsta->txrate.mcs = ts->mcs;
+ arsta->txrate.flags = RATE_INFO_FLAGS_EHT_MCS;
+ arsta->txrate.eht_gi = ath12k_mac_eht_gi_to_nl80211_eht_gi(ts->sgi);
+ }
+
+ arsta->txrate.bw = ath12k_mac_bw_to_mac80211_bw(ts->bw);
+
+ if (ts->ofdma && ts->pkt_type == HAL_TX_RATE_STATS_PKT_TYPE_11AX) {
+ arsta->txrate.bw = RATE_INFO_BW_HE_RU;
+ arsta->txrate.he_ru_alloc =
+ ath12k_mac_he_ru_tones_to_nl80211_he_ru_alloc(ts->ru_tones);
+ }
+
+ if (ts->ofdma && ts->pkt_type == HAL_TX_RATE_STATS_PKT_TYPE_11BE) {
+ arsta->txrate.bw = RATE_INFO_BW_EHT_RU;
+ arsta->txrate.eht_ru_alloc =
+ ath12k_mac_eht_ru_tones_to_nl80211_eht_ru_alloc(ts->ru_tones);
+ }
+
+err_out:
+ spin_unlock_bh(&ab->base_lock);
+}
+
+static void ath12k_dp_tx_update(struct ath12k *ar, struct hal_tx_status *ts)
+{
+ if (ar->last_ppdu_id != 0) {
+ if (ar->last_ppdu_id == ts->ppdu_id ||
+ ar->cached_ppdu_id == ar->last_ppdu_id)
+ ar->cached_ppdu_id = ar->last_ppdu_id;
+
+ ath12k_dp_tx_update_txcompl(ar, ts);
+ }
+
+ ar->last_ppdu_id = ts->ppdu_id;
+}
+
static void ath12k_dp_tx_complete_msdu(struct ath12k *ar,
struct sk_buff *msdu,
struct hal_tx_status *ts)
@@ -498,6 +619,8 @@ static void ath12k_dp_tx_complete_msdu(struct ath12k *ar,
* Might end up reporting it out-of-band from HTT stats.
*/
+ ath12k_dp_tx_update(ar, ts);
+
ieee80211_tx_status_skb(ath12k_ar_to_hw(ar), msdu);
exit:
@@ -522,10 +645,26 @@ static void ath12k_dp_tx_status_parse(struct ath12k_base *ab,
ts->ppdu_id = le32_get_bits(desc->info1,
HAL_WBM_COMPL_TX_INFO1_TQM_STATUS_NUMBER);
- if (le32_to_cpu(desc->rate_stats.info0) & HAL_TX_RATE_STATS_INFO0_VALID)
- ts->rate_stats = le32_to_cpu(desc->rate_stats.info0);
- else
- ts->rate_stats = 0;
+
+ if (le32_to_cpu(desc->info2) & HAL_WBM_COMPL_TX_INFO2_FIRST_MSDU)
+ ts->flags |= HAL_TX_STATUS_FLAGS_FIRST_MSDU;
+
+ ts->peer_id = le32_get_bits(desc->info3, HAL_WBM_COMPL_TX_INFO3_PEER_ID);
+
+ if (le32_to_cpu(desc->rate_stats.info0) & HAL_TX_RATE_STATS_INFO0_VALID) {
+ ts->pkt_type = le32_get_bits(desc->rate_stats.info0,
+ HAL_TX_RATE_STATS_INFO0_PKT_TYPE);
+ ts->mcs = le32_get_bits(desc->rate_stats.info0,
+ HAL_TX_RATE_STATS_INFO0_MCS);
+ ts->sgi = le32_get_bits(desc->rate_stats.info0,
+ HAL_TX_RATE_STATS_INFO0_SGI);
+ ts->bw = le32_get_bits(desc->rate_stats.info0,
+ HAL_TX_RATE_STATS_INFO0_BW);
+ ts->ru_tones = le32_get_bits(desc->rate_stats.info0,
+ HAL_TX_RATE_STATS_INFO0_TONES_IN_RU);
+ ts->ofdma = le32_get_bits(desc->rate_stats.info0,
+ HAL_TX_RATE_STATS_INFO0_OFDMA_TX);
+ }
}
void ath12k_dp_tx_completion_handler(struct ath12k_base *ab, int ring_id)
diff --git a/drivers/net/wireless/ath/ath12k/hal_tx.h b/drivers/net/wireless/ath/ath12k/hal_tx.h
index 7c837094a6f7..a3cf4db456e5 100644
--- a/drivers/net/wireless/ath/ath12k/hal_tx.h
+++ b/drivers/net/wireless/ath/ath12k/hal_tx.h
@@ -1,7 +1,7 @@
/* SPDX-License-Identifier: BSD-3-Clause-Clear */
/*
* Copyright (c) 2018-2021 The Linux Foundation. All rights reserved.
- * Copyright (c) 2021-2022 Qualcomm Innovation Center, Inc. All rights reserved.
+ * Copyright (c) 2021-2022, 2024 Qualcomm Innovation Center, Inc. All rights reserved.
*/
#ifndef ATH12K_HAL_TX_H
@@ -63,7 +63,12 @@ struct hal_tx_status {
u8 try_cnt;
u8 tid;
u16 peer_id;
- u32 rate_stats;
+ enum hal_tx_rate_stats_pkt_type pkt_type;
+ enum hal_tx_rate_stats_sgi sgi;
+ enum ath12k_supported_bw bw;
+ u8 mcs;
+ u16 ru_tones;
+ u8 ofdma;
};
#define HAL_TX_PHY_DESC_INFO0_BF_TYPE GENMASK(17, 16)
diff --git a/drivers/net/wireless/ath/ath12k/mac.c b/drivers/net/wireless/ath/ath12k/mac.c
index 56b1f8b6844e..cd13fa48e97d 100644
--- a/drivers/net/wireless/ath/ath12k/mac.c
+++ b/drivers/net/wireless/ath/ath12k/mac.c
@@ -329,6 +329,122 @@ static const char *ath12k_mac_phymode_str(enum wmi_phy_mode mode)
return "<unknown>";
}
+enum nl80211_he_ru_alloc ath12k_mac_he_ru_tones_to_nl80211_he_ru_alloc(u16 ru_tones)
+{
+ enum nl80211_he_ru_alloc ret;
+
+ switch (ru_tones) {
+ case 26:
+ ret = NL80211_RATE_INFO_HE_RU_ALLOC_26;
+ break;
+ case 52:
+ ret = NL80211_RATE_INFO_HE_RU_ALLOC_52;
+ break;
+ case 106:
+ ret = NL80211_RATE_INFO_HE_RU_ALLOC_106;
+ break;
+ case 242:
+ ret = NL80211_RATE_INFO_HE_RU_ALLOC_242;
+ break;
+ case 484:
+ ret = NL80211_RATE_INFO_HE_RU_ALLOC_484;
+ break;
+ case 996:
+ ret = NL80211_RATE_INFO_HE_RU_ALLOC_996;
+ break;
+ case (996 * 2):
+ ret = NL80211_RATE_INFO_HE_RU_ALLOC_2x996;
+ break;
+ default:
+ ret = NL80211_RATE_INFO_HE_RU_ALLOC_26;
+ break;
+ }
+
+ return ret;
+}
+
+enum nl80211_eht_gi ath12k_mac_eht_gi_to_nl80211_eht_gi(u8 sgi)
+{
+ enum nl80211_eht_gi ret;
+
+ switch (sgi) {
+ case RX_MSDU_START_SGI_0_8_US:
+ ret = NL80211_RATE_INFO_EHT_GI_0_8;
+ break;
+ case RX_MSDU_START_SGI_1_6_US:
+ ret = NL80211_RATE_INFO_EHT_GI_1_6;
+ break;
+ case RX_MSDU_START_SGI_3_2_US:
+ ret = NL80211_RATE_INFO_EHT_GI_3_2;
+ break;
+ default:
+ ret = NL80211_RATE_INFO_EHT_GI_0_8;
+ break;
+ }
+
+ return ret;
+}
+
+enum nl80211_eht_ru_alloc ath12k_mac_eht_ru_tones_to_nl80211_eht_ru_alloc(u16 ru_tones)
+{
+ enum nl80211_eht_ru_alloc ret;
+
+ switch (ru_tones) {
+ case 26:
+ ret = NL80211_RATE_INFO_EHT_RU_ALLOC_26;
+ break;
+ case 52:
+ ret = NL80211_RATE_INFO_EHT_RU_ALLOC_52;
+ break;
+ case (52 + 26):
+ ret = NL80211_RATE_INFO_EHT_RU_ALLOC_52P26;
+ break;
+ case 106:
+ ret = NL80211_RATE_INFO_EHT_RU_ALLOC_106;
+ break;
+ case (106 + 26):
+ ret = NL80211_RATE_INFO_EHT_RU_ALLOC_106P26;
+ break;
+ case 242:
+ ret = NL80211_RATE_INFO_EHT_RU_ALLOC_242;
+ break;
+ case 484:
+ ret = NL80211_RATE_INFO_EHT_RU_ALLOC_484;
+ break;
+ case (484 + 242):
+ ret = NL80211_RATE_INFO_EHT_RU_ALLOC_484P242;
+ break;
+ case 996:
+ ret = NL80211_RATE_INFO_EHT_RU_ALLOC_996;
+ break;
+ case (996 + 484):
+ ret = NL80211_RATE_INFO_EHT_RU_ALLOC_996P484;
+ break;
+ case (996 + 484 + 242):
+ ret = NL80211_RATE_INFO_EHT_RU_ALLOC_996P484P242;
+ break;
+ case (2 * 996):
+ ret = NL80211_RATE_INFO_EHT_RU_ALLOC_2x996;
+ break;
+ case (2 * 996 + 484):
+ ret = NL80211_RATE_INFO_EHT_RU_ALLOC_2x996P484;
+ break;
+ case (3 * 996):
+ ret = NL80211_RATE_INFO_EHT_RU_ALLOC_3x996;
+ break;
+ case (3 * 996 + 484):
+ ret = NL80211_RATE_INFO_EHT_RU_ALLOC_3x996P484;
+ break;
+ case (4 * 996):
+ ret = NL80211_RATE_INFO_EHT_RU_ALLOC_4x996;
+ break;
+ default:
+ ret = NL80211_RATE_INFO_EHT_RU_ALLOC_26;
+ }
+
+ return ret;
+}
+
enum rate_info_bw
ath12k_mac_bw_to_mac80211_bw(enum ath12k_supported_bw bw)
{
@@ -2487,8 +2603,12 @@ static void ath12k_peer_assoc_prepare(struct ath12k *ar,
struct ath12k_wmi_peer_assoc_arg *arg,
bool reassoc)
{
+ struct ath12k_sta *arsta;
+
lockdep_assert_held(&ar->conf_mutex);
+ arsta = ath12k_sta_to_arsta(sta);
+
memset(arg, 0, sizeof(*arg));
reinit_completion(&ar->peer_assoc_done);
@@ -2505,6 +2625,8 @@ static void ath12k_peer_assoc_prepare(struct ath12k *ar,
ath12k_peer_assoc_h_phymode(ar, vif, sta, arg);
ath12k_peer_assoc_h_smps(sta, arg);
+ arsta->peer_nss = arg->peer_nss;
+
/* TODO: amsdu_disable req? */
}
@@ -8073,6 +8195,8 @@ static void ath12k_mac_op_sta_statistics(struct ieee80211_hw *hw,
sinfo->txrate.he_gi = arsta->txrate.he_gi;
sinfo->txrate.he_dcm = arsta->txrate.he_dcm;
sinfo->txrate.he_ru_alloc = arsta->txrate.he_ru_alloc;
+ sinfo->txrate.eht_gi = arsta->txrate.eht_gi;
+ sinfo->txrate.eht_ru_alloc = arsta->txrate.eht_ru_alloc;
}
sinfo->txrate.flags = arsta->txrate.flags;
sinfo->filled |= BIT_ULL(NL80211_STA_INFO_TX_BITRATE);
diff --git a/drivers/net/wireless/ath/ath12k/mac.h b/drivers/net/wireless/ath/ath12k/mac.h
index 69fd282b9dd3..b22321aadc84 100644
--- a/drivers/net/wireless/ath/ath12k/mac.h
+++ b/drivers/net/wireless/ath/ath12k/mac.h
@@ -81,5 +81,7 @@ int ath12k_mac_rfkill_config(struct ath12k *ar);
int ath12k_mac_wait_tx_complete(struct ath12k *ar);
void ath12k_mac_handle_beacon(struct ath12k *ar, struct sk_buff *skb);
void ath12k_mac_handle_beacon_miss(struct ath12k *ar, u32 vdev_id);
-
+enum nl80211_he_ru_alloc ath12k_mac_he_ru_tones_to_nl80211_he_ru_alloc(u16 ru_tones);
+enum nl80211_eht_ru_alloc ath12k_mac_eht_ru_tones_to_nl80211_eht_ru_alloc(u16 ru_tones);
+enum nl80211_eht_gi ath12k_mac_eht_gi_to_nl80211_eht_gi(u8 sgi);
#endif
--
2.34.1
Lingbo Kong <[email protected]> writes:
> Currently, the transmit rate of "iw dev xxx station dump" command
> always show an invalid value.
>
> To address this issue, ath12k parse the info of transmit complete
> report from firmware and indicate the transmit rate to mac80211.
>
> This patch affects the station mode of WCN7850 and QCN9274.
>
> After that, "iw dev xxx station dump" show the correct transmit rate.
> Such as:
>
> Station 00:03:7f:12:03:03 (on wlo1)
> inactive time: 872 ms
> rx bytes: 219111
> rx packets: 1133
> tx bytes: 53767
> tx packets: 462
> tx retries: 51
> tx failed: 0
> beacon loss: 0
> beacon rx: 403
> rx drop misc: 74
> signal: -95 dBm
> beacon signal avg: -18 dBm
> tx bitrate: 1441.1 MBit/s 80MHz EHT-MCS 13 EHT-NSS 2 EHT-GI 0
>
> Tested-on: WCN7850 hw2.0 PCI WLAN.HMT.1.0.c5-00481-QCAHMTSWPL_V1.0_V2.0_SILICONZ-3
> Tested-on: QCN9274 hw2.0 PCI WLAN.WBE.1.2.1-00201-QCAHKSWPL_SILICONZ-1
>
> Signed-off-by: Lingbo Kong <[email protected]>
[...]
> +static void ath12k_dp_tx_update(struct ath12k *ar, struct hal_tx_status *ts)
> +{
> + if (ar->last_ppdu_id != 0) {
> + if (ar->last_ppdu_id == ts->ppdu_id ||
> + ar->cached_ppdu_id == ar->last_ppdu_id)
> + ar->cached_ppdu_id = ar->last_ppdu_id;
> +
> + ath12k_dp_tx_update_txcompl(ar, ts);
> + }
> +
> + ar->last_ppdu_id = ts->ppdu_id;
> +}
A code comment would help a lot. Why is ar->cached_ppdu_id needed here?
And if 'ar->cached_ppdu_id == ar->last_ppdu_id' is true why do then do
'ar->cached_ppdu_id = ar->last_ppdu_id'? The value of ar->cached_ppdu_id
is not changing here (unless I'm missing something).
Also I'm worried about locking. How is access to ar->last_ppdu_id and
ar->cached_ppdu_id protected?
--
https://patchwork.kernel.org/project/linux-wireless/list/
https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
Lingbo Kong <[email protected]> writes:
> Currently, the transmit rate of "iw dev xxx station dump" command
> always show an invalid value.
>
> To address this issue, ath12k parse the info of transmit complete
> report from firmware and indicate the transmit rate to mac80211.
>
> This patch affects the station mode of WCN7850 and QCN9274.
>
> After that, "iw dev xxx station dump" show the correct transmit rate.
> Such as:
>
> Station 00:03:7f:12:03:03 (on wlo1)
> inactive time: 872 ms
> rx bytes: 219111
> rx packets: 1133
> tx bytes: 53767
> tx packets: 462
> tx retries: 51
> tx failed: 0
> beacon loss: 0
> beacon rx: 403
> rx drop misc: 74
> signal: -95 dBm
> beacon signal avg: -18 dBm
> tx bitrate: 1441.1 MBit/s 80MHz EHT-MCS 13 EHT-NSS 2 EHT-GI 0
>
> Tested-on: WCN7850 hw2.0 PCI WLAN.HMT.1.0.c5-00481-QCAHMTSWPL_V1.0_V2.0_SILICONZ-3
> Tested-on: QCN9274 hw2.0 PCI WLAN.WBE.1.2.1-00201-QCAHKSWPL_SILICONZ-1
>
> Signed-off-by: Lingbo Kong <[email protected]>
I'm still going throught the patchset, please don't send a new version
yet. Few quick comments:
> +static void ath12k_dp_tx_update_txcompl(struct ath12k *ar, struct hal_tx_status *ts)
> +{
> + struct ath12k_base *ab = ar->ab;
> + struct ath12k_peer *peer;
> + struct ath12k_sta *arsta;
> + struct ieee80211_sta *sta;
> + u16 rate;
> + u8 rate_idx = 0;
> + int ret;
> +
> + spin_lock_bh(&ab->base_lock);
Did you analyse how this function, and especially taking the base_lock,
affects performance?
> +enum nl80211_he_ru_alloc ath12k_mac_he_ru_tones_to_nl80211_he_ru_alloc(u16 ru_tones)
> +{
> + enum nl80211_he_ru_alloc ret;
> +
> + switch (ru_tones) {
> + case 26:
> + ret = NL80211_RATE_INFO_HE_RU_ALLOC_26;
> + break;
> + case 52:
> + ret = NL80211_RATE_INFO_HE_RU_ALLOC_52;
> + break;
> + case 106:
> + ret = NL80211_RATE_INFO_HE_RU_ALLOC_106;
> + break;
> + case 242:
> + ret = NL80211_RATE_INFO_HE_RU_ALLOC_242;
> + break;
> + case 484:
> + ret = NL80211_RATE_INFO_HE_RU_ALLOC_484;
> + break;
> + case 996:
> + ret = NL80211_RATE_INFO_HE_RU_ALLOC_996;
> + break;
> + case (996 * 2):
> + ret = NL80211_RATE_INFO_HE_RU_ALLOC_2x996;
> + break;
> + default:
> + ret = NL80211_RATE_INFO_HE_RU_ALLOC_26;
> + break;
> + }
> +
> + return ret;
> +}
How does this function compare to ath12k_he_ru_tones_to_nl80211_he_ru_alloc()?
> +enum nl80211_eht_gi ath12k_mac_eht_gi_to_nl80211_eht_gi(u8 sgi)
> +{
> + enum nl80211_eht_gi ret;
> +
> + switch (sgi) {
> + case RX_MSDU_START_SGI_0_8_US:
> + ret = NL80211_RATE_INFO_EHT_GI_0_8;
> + break;
> + case RX_MSDU_START_SGI_1_6_US:
> + ret = NL80211_RATE_INFO_EHT_GI_1_6;
> + break;
> + case RX_MSDU_START_SGI_3_2_US:
> + ret = NL80211_RATE_INFO_EHT_GI_3_2;
> + break;
> + default:
> + ret = NL80211_RATE_INFO_EHT_GI_0_8;
> + break;
> + }
> +
> + return ret;
> +}
BTW the ret variable is unnessary, this could be simplified to:
switch (foo) {
case FOO1:
return BAR1;
case FOO2:
return BAR2;
default:
return BAR3;
}
> +enum nl80211_eht_ru_alloc ath12k_mac_eht_ru_tones_to_nl80211_eht_ru_alloc(u16 ru_tones)
> +{
> + enum nl80211_eht_ru_alloc ret;
> +
> + switch (ru_tones) {
> + case 26:
> + ret = NL80211_RATE_INFO_EHT_RU_ALLOC_26;
> + break;
> + case 52:
> + ret = NL80211_RATE_INFO_EHT_RU_ALLOC_52;
> + break;
> + case (52 + 26):
> + ret = NL80211_RATE_INFO_EHT_RU_ALLOC_52P26;
> + break;
> + case 106:
> + ret = NL80211_RATE_INFO_EHT_RU_ALLOC_106;
> + break;
> + case (106 + 26):
> + ret = NL80211_RATE_INFO_EHT_RU_ALLOC_106P26;
> + break;
> + case 242:
> + ret = NL80211_RATE_INFO_EHT_RU_ALLOC_242;
> + break;
> + case 484:
> + ret = NL80211_RATE_INFO_EHT_RU_ALLOC_484;
> + break;
> + case (484 + 242):
> + ret = NL80211_RATE_INFO_EHT_RU_ALLOC_484P242;
> + break;
> + case 996:
> + ret = NL80211_RATE_INFO_EHT_RU_ALLOC_996;
> + break;
> + case (996 + 484):
> + ret = NL80211_RATE_INFO_EHT_RU_ALLOC_996P484;
> + break;
> + case (996 + 484 + 242):
> + ret = NL80211_RATE_INFO_EHT_RU_ALLOC_996P484P242;
> + break;
> + case (2 * 996):
> + ret = NL80211_RATE_INFO_EHT_RU_ALLOC_2x996;
> + break;
> + case (2 * 996 + 484):
> + ret = NL80211_RATE_INFO_EHT_RU_ALLOC_2x996P484;
> + break;
> + case (3 * 996):
> + ret = NL80211_RATE_INFO_EHT_RU_ALLOC_3x996;
> + break;
> + case (3 * 996 + 484):
> + ret = NL80211_RATE_INFO_EHT_RU_ALLOC_3x996P484;
> + break;
> + case (4 * 996):
> + ret = NL80211_RATE_INFO_EHT_RU_ALLOC_4x996;
> + break;
> + default:
> + ret = NL80211_RATE_INFO_EHT_RU_ALLOC_26;
> + }
> +
> + return ret;
> +}
Same here.
--
https://patchwork.kernel.org/project/linux-wireless/list/
https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
Lingbo Kong <[email protected]> writes:
> Currently, the signal strength of "iw dev xxx station dump" always show an
> invalid value.
>
> This is because signal strength is only set in ath12k_mgmt_rx_event()
> function, and not set for received data packet. So, change to get signal
> from firmware and report to mac80211.
>
> After that, "iw dev xxx station dump" show the correct signal strength.
> Such as:
>
> Station 00:03:7f:12:03:03 (on wlo1)
> inactive time: 36 ms
> rx bytes: 61571
> rx packets: 336
> tx bytes: 28204
> tx packets: 205
> tx retries: 49
> tx failed: 0
> beacon loss: 0
> beacon rx: 83
> rx drop misc: 66
> signal: -24 dBm
> beacon signal avg: -22 dBm
>
> For WCN7850, the firmware supports db2dbm, so not need to add noise floor.
> For QCN9274, the firmware not support db2dbm, so need to add noise floor.
>
> This patch affects the station mode of WCN7850 and QCN9274.
>
> Tested-on: WCN7850 hw2.0 PCI WLAN.HMT.1.0.c5-00481-QCAHMTSWPL_V1.0_V2.0_SILICONZ-3
> Tested-on: QCN9274 hw2.0 PCI WLAN.WBE.1.2.1-00201-QCAHKSWPL_SILICONZ-1
>
> Signed-off-by: Lingbo Kong <[email protected]>
[...]
> +static int ath12k_mac_get_fw_stats(struct ath12k *ar, u32 pdev_id,
> + u32 vdev_id, u32 stats_id)
> +{
> + struct ath12k_base *ab = ar->ab;
> + int ret, left;
> +
> + mutex_lock(&ar->conf_mutex);
> +
> + if (ar->state != ATH12K_STATE_ON) {
> + ret = -ENETDOWN;
> + goto err_unlock;
> + }
> +
> + reinit_completion(&ar->fw_stats_complete);
> +
> + ret = ath12k_wmi_send_stats_request_cmd(ar, stats_id, vdev_id, pdev_id);
> +
> + if (ret) {
> + ath12k_warn(ab, "failed to request fw stats: %d\n", ret);
> + goto err_unlock;
> + }
> +
> + ath12k_dbg(ab, ATH12K_DBG_WMI,
> + "get fw stat pdev id %d vdev id %d stats id 0x%x\n",
> + pdev_id, vdev_id, stats_id);
> +
> + left = wait_for_completion_timeout(&ar->fw_stats_complete, 1 * HZ);
> +
> + if (!left)
> + ath12k_warn(ab, "time out while waiting for get fw stats\n");
> +err_unlock:
> +
> + mutex_unlock(&ar->conf_mutex);
> + return ret;
> +}
Shouldn't we return an error if there's a timeout?
> @@ -8202,8 +8242,18 @@ static void ath12k_mac_op_sta_statistics(struct ieee80211_hw *hw,
> sinfo->filled |= BIT_ULL(NL80211_STA_INFO_TX_BITRATE);
>
> /* TODO: Use real NF instead of default one. */
> - sinfo->signal = arsta->rssi_comb + ATH12K_DEFAULT_NOISE_FLOOR;
> - sinfo->filled |= BIT_ULL(NL80211_STA_INFO_SIGNAL);
> + signal = arsta->rssi_comb;
> +
> + if (!signal &&
> + arsta->arvif->vdev_type == WMI_VDEV_TYPE_STA &&
> + !(ath12k_mac_get_fw_stats(ar, ar->pdev->pdev_id, 0,
> + WMI_REQUEST_VDEV_STAT)))
> + signal = arsta->rssi_beacon;
> +
> + if (signal) {
> + sinfo->signal = db2dbm ? signal : signal + ATH12K_DEFAULT_NOISE_FLOOR;
Can this be simplified to:
if (db2dbm)
signal += ATH12K_DEFAULT_NOISE_FLOOR;
A lot more readable.
--
https://patchwork.kernel.org/project/linux-wireless/list/
https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
On 2024/4/26 0:54, Kalle Valo wrote:
> Lingbo Kong <[email protected]> writes:
>
>> Currently, the transmit rate of "iw dev xxx station dump" command
>> always show an invalid value.
>>
>> To address this issue, ath12k parse the info of transmit complete
>> report from firmware and indicate the transmit rate to mac80211.
>>
>> This patch affects the station mode of WCN7850 and QCN9274.
>>
>> After that, "iw dev xxx station dump" show the correct transmit rate.
>> Such as:
>>
>> Station 00:03:7f:12:03:03 (on wlo1)
>> inactive time: 872 ms
>> rx bytes: 219111
>> rx packets: 1133
>> tx bytes: 53767
>> tx packets: 462
>> tx retries: 51
>> tx failed: 0
>> beacon loss: 0
>> beacon rx: 403
>> rx drop misc: 74
>> signal: -95 dBm
>> beacon signal avg: -18 dBm
>> tx bitrate: 1441.1 MBit/s 80MHz EHT-MCS 13 EHT-NSS 2 EHT-GI 0
>>
>> Tested-on: WCN7850 hw2.0 PCI WLAN.HMT.1.0.c5-00481-QCAHMTSWPL_V1.0_V2.0_SILICONZ-3
>> Tested-on: QCN9274 hw2.0 PCI WLAN.WBE.1.2.1-00201-QCAHKSWPL_SILICONZ-1
>>
>> Signed-off-by: Lingbo Kong <[email protected]>
>
> I'm still going throught the patchset, please don't send a new version
> yet. Few quick comments:
>
>> +static void ath12k_dp_tx_update_txcompl(struct ath12k *ar, struct hal_tx_status *ts)
>> +{
>> + struct ath12k_base *ab = ar->ab;
>> + struct ath12k_peer *peer;
>> + struct ath12k_sta *arsta;
>> + struct ieee80211_sta *sta;
>> + u16 rate;
>> + u8 rate_idx = 0;
>> + int ret;
>> +
>> + spin_lock_bh(&ab->base_lock);
>
> Did you analyse how this function, and especially taking the base_lock,
> affects performance?
>
The base_lock is used here because of the need to look for peers based
on the ts->peer_id when calling ath12k_peer_find_by_id() function, which
i think might affect performance.
Do i need to run a throughput test?
>> +enum nl80211_he_ru_alloc ath12k_mac_he_ru_tones_to_nl80211_he_ru_alloc(u16 ru_tones)
>> +{
>> + enum nl80211_he_ru_alloc ret;
>> +
>> + switch (ru_tones) {
>> + case 26:
>> + ret = NL80211_RATE_INFO_HE_RU_ALLOC_26;
>> + break;
>> + case 52:
>> + ret = NL80211_RATE_INFO_HE_RU_ALLOC_52;
>> + break;
>> + case 106:
>> + ret = NL80211_RATE_INFO_HE_RU_ALLOC_106;
>> + break;
>> + case 242:
>> + ret = NL80211_RATE_INFO_HE_RU_ALLOC_242;
>> + break;
>> + case 484:
>> + ret = NL80211_RATE_INFO_HE_RU_ALLOC_484;
>> + break;
>> + case 996:
>> + ret = NL80211_RATE_INFO_HE_RU_ALLOC_996;
>> + break;
>> + case (996 * 2):
>> + ret = NL80211_RATE_INFO_HE_RU_ALLOC_2x996;
>> + break;
>> + default:
>> + ret = NL80211_RATE_INFO_HE_RU_ALLOC_26;
>> + break;
>> + }
>> +
>> + return ret;
>> +}
>
> How does this function compare to ath12k_he_ru_tones_to_nl80211_he_ru_alloc()?
>
ath12k_mac_he_ru_tones_to_nl80211_he_ru_alloc() is different from
ath12k_he_ru_tones_to_nl80211_he_ru_alloc().
the logic of ath12k_he_ru_tones_to_nl80211_he_ru_alloc() is
static inline
enum nl80211_he_ru_alloc ath12k_he_ru_tones_to_nl80211_he_ru_alloc(u16
ru_tones)
{
enum nl80211_he_ru_alloc ret;
switch (ru_tones) {
case RU_52:
ret = NL80211_RATE_INFO_HE_RU_ALLOC_52;
break;
case RU_106:
ret = NL80211_RATE_INFO_HE_RU_ALLOC_106;
break;
case RU_242:
ret = NL80211_RATE_INFO_HE_RU_ALLOC_242;
break;
case RU_484:
ret = NL80211_RATE_INFO_HE_RU_ALLOC_484;
break;
case RU_996:
ret = NL80211_RATE_INFO_HE_RU_ALLOC_996;
break;
case RU_26:
fallthrough;
default:
ret = NL80211_RATE_INFO_HE_RU_ALLOC_26;
break;
}
return ret;
}
#define RU_26 1
#define RU_52 2
#define RU_106 4
#define RU_242 9
#define RU_484 18
#define RU_996 37
>> +enum nl80211_eht_gi ath12k_mac_eht_gi_to_nl80211_eht_gi(u8 sgi)
>> +{
>> + enum nl80211_eht_gi ret;
>> +
>> + switch (sgi) {
>> + case RX_MSDU_START_SGI_0_8_US:
>> + ret = NL80211_RATE_INFO_EHT_GI_0_8;
>> + break;
>> + case RX_MSDU_START_SGI_1_6_US:
>> + ret = NL80211_RATE_INFO_EHT_GI_1_6;
>> + break;
>> + case RX_MSDU_START_SGI_3_2_US:
>> + ret = NL80211_RATE_INFO_EHT_GI_3_2;
>> + break;
>> + default:
>> + ret = NL80211_RATE_INFO_EHT_GI_0_8;
>> + break;
>> + }
>> +
>> + return ret;
>> +}
>
> BTW the ret variable is unnessary, this could be simplified to:
>
> switch (foo) {
> case FOO1:
> return BAR1;
> case FOO2:
> return BAR2;
> default:
> return BAR3;
> }
>
>> +enum nl80211_eht_ru_alloc ath12k_mac_eht_ru_tones_to_nl80211_eht_ru_alloc(u16 ru_tones)
>> +{
>> + enum nl80211_eht_ru_alloc ret;
>> +
>> + switch (ru_tones) {
>> + case 26:
>> + ret = NL80211_RATE_INFO_EHT_RU_ALLOC_26;
>> + break;
>> + case 52:
>> + ret = NL80211_RATE_INFO_EHT_RU_ALLOC_52;
>> + break;
>> + case (52 + 26):
>> + ret = NL80211_RATE_INFO_EHT_RU_ALLOC_52P26;
>> + break;
>> + case 106:
>> + ret = NL80211_RATE_INFO_EHT_RU_ALLOC_106;
>> + break;
>> + case (106 + 26):
>> + ret = NL80211_RATE_INFO_EHT_RU_ALLOC_106P26;
>> + break;
>> + case 242:
>> + ret = NL80211_RATE_INFO_EHT_RU_ALLOC_242;
>> + break;
>> + case 484:
>> + ret = NL80211_RATE_INFO_EHT_RU_ALLOC_484;
>> + break;
>> + case (484 + 242):
>> + ret = NL80211_RATE_INFO_EHT_RU_ALLOC_484P242;
>> + break;
>> + case 996:
>> + ret = NL80211_RATE_INFO_EHT_RU_ALLOC_996;
>> + break;
>> + case (996 + 484):
>> + ret = NL80211_RATE_INFO_EHT_RU_ALLOC_996P484;
>> + break;
>> + case (996 + 484 + 242):
>> + ret = NL80211_RATE_INFO_EHT_RU_ALLOC_996P484P242;
>> + break;
>> + case (2 * 996):
>> + ret = NL80211_RATE_INFO_EHT_RU_ALLOC_2x996;
>> + break;
>> + case (2 * 996 + 484):
>> + ret = NL80211_RATE_INFO_EHT_RU_ALLOC_2x996P484;
>> + break;
>> + case (3 * 996):
>> + ret = NL80211_RATE_INFO_EHT_RU_ALLOC_3x996;
>> + break;
>> + case (3 * 996 + 484):
>> + ret = NL80211_RATE_INFO_EHT_RU_ALLOC_3x996P484;
>> + break;
>> + case (4 * 996):
>> + ret = NL80211_RATE_INFO_EHT_RU_ALLOC_4x996;
>> + break;
>> + default:
>> + ret = NL80211_RATE_INFO_EHT_RU_ALLOC_26;
>> + }
>> +
>> + return ret;
>> +}
>
> Same here.
>
On 2024/4/25 18:37, Kalle Valo wrote:
> Lingbo Kong <[email protected]> writes:
>
>> Currently, the transmit rate of "iw dev xxx station dump" command
>> always show an invalid value.
>>
>> To address this issue, ath12k parse the info of transmit complete
>> report from firmware and indicate the transmit rate to mac80211.
>>
>> This patch affects the station mode of WCN7850 and QCN9274.
>>
>> After that, "iw dev xxx station dump" show the correct transmit rate.
>> Such as:
>>
>> Station 00:03:7f:12:03:03 (on wlo1)
>> inactive time: 872 ms
>> rx bytes: 219111
>> rx packets: 1133
>> tx bytes: 53767
>> tx packets: 462
>> tx retries: 51
>> tx failed: 0
>> beacon loss: 0
>> beacon rx: 403
>> rx drop misc: 74
>> signal: -95 dBm
>> beacon signal avg: -18 dBm
>> tx bitrate: 1441.1 MBit/s 80MHz EHT-MCS 13 EHT-NSS 2 EHT-GI 0
>>
>> Tested-on: WCN7850 hw2.0 PCI WLAN.HMT.1.0.c5-00481-QCAHMTSWPL_V1.0_V2.0_SILICONZ-3
>> Tested-on: QCN9274 hw2.0 PCI WLAN.WBE.1.2.1-00201-QCAHKSWPL_SILICONZ-1
>>
>> Signed-off-by: Lingbo Kong <[email protected]>
>
> [...]
>
>> +static void ath12k_dp_tx_update(struct ath12k *ar, struct hal_tx_status *ts)
>> +{
>> + if (ar->last_ppdu_id != 0) {
>> + if (ar->last_ppdu_id == ts->ppdu_id ||
>> + ar->cached_ppdu_id == ar->last_ppdu_id)
>> + ar->cached_ppdu_id = ar->last_ppdu_id;
>> +
>> + ath12k_dp_tx_update_txcompl(ar, ts);
>> + }
>> +
>> + ar->last_ppdu_id = ts->ppdu_id;
>> +}
>
> A code comment would help a lot. Why is ar->cached_ppdu_id needed here?
>
> And if 'ar->cached_ppdu_id == ar->last_ppdu_id' is true why do then do
> 'ar->cached_ppdu_id = ar->last_ppdu_id'? The value of ar->cached_ppdu_id
> is not changing here (unless I'm missing something).
>
> Also I'm worried about locking. How is access to ar->last_ppdu_id and
> ar->cached_ppdu_id protected?
>
Thanks for pointing to this.
you're right, the ar->cached_ppdu_id haven't used in here, so need to
delete it.
i missed something in here.
So, change the ath12k_dp_tx_update(struct ath12k *ar, struct
hal_tx_status *ts) to
static void ath12k_dp_tx_update(struct ath12k *ar, struct hal_tx_status *ts)
{
if (ts->flags & HAL_TX_STATUS_FLAGS_FIRST_MSDU) {
if (ar->last_ppdu_id != 0)
ath12k_dp_tx_update_txcompl(ar, ts);
ar->last_ppdu_id = ts->ppdu_id;
}
}
best regards
Lingbo Kong
Lingbo Kong <[email protected]> writes:
> On 2024/4/26 0:54, Kalle Valo wrote:
>> Lingbo Kong <[email protected]> writes:
>>
>>> +static void ath12k_dp_tx_update_txcompl(struct ath12k *ar, struct
>>> hal_tx_status *ts)
>>> +{
>>> + struct ath12k_base *ab = ar->ab;
>>> + struct ath12k_peer *peer;
>>> + struct ath12k_sta *arsta;
>>> + struct ieee80211_sta *sta;
>>> + u16 rate;
>>> + u8 rate_idx = 0;
>>> + int ret;
>>> +
>>> + spin_lock_bh(&ab->base_lock);
>>
>> Did you analyse how this function, and especially taking the
>> base_lock,
>> affects performance?
>
> The base_lock is used here because of the need to look for peers based
> on the ts->peer_id when calling ath12k_peer_find_by_id() function,
> which i think might affect performance.
>
> Do i need to run a throughput test?
Ok, so to answer my question: no, you didn't do any performance
analysis. Throughput test might not be enough, for example the driver
can be used on slower systems and running the test on a fast CPU might
not reveal any problem. A proper analysis would be much better.
>>> +enum nl80211_he_ru_alloc
>>> ath12k_mac_he_ru_tones_to_nl80211_he_ru_alloc(u16 ru_tones)
>>> +{
>>> + enum nl80211_he_ru_alloc ret;
>>> +
>>> + switch (ru_tones) {
>>> + case 26:
>>> + ret = NL80211_RATE_INFO_HE_RU_ALLOC_26;
>>> + break;
>>> + case 52:
>>> + ret = NL80211_RATE_INFO_HE_RU_ALLOC_52;
>>> + break;
>>> + case 106:
>>> + ret = NL80211_RATE_INFO_HE_RU_ALLOC_106;
>>> + break;
>>> + case 242:
>>> + ret = NL80211_RATE_INFO_HE_RU_ALLOC_242;
>>> + break;
>>> + case 484:
>>> + ret = NL80211_RATE_INFO_HE_RU_ALLOC_484;
>>> + break;
>>> + case 996:
>>> + ret = NL80211_RATE_INFO_HE_RU_ALLOC_996;
>>> + break;
>>> + case (996 * 2):
>>> + ret = NL80211_RATE_INFO_HE_RU_ALLOC_2x996;
>>> + break;
>>> + default:
>>> + ret = NL80211_RATE_INFO_HE_RU_ALLOC_26;
>>> + break;
>>> + }
>>> +
>>> + return ret;
>>> +}
>> How does this function compare to
>> ath12k_he_ru_tones_to_nl80211_he_ru_alloc()?
>>
>
> ath12k_mac_he_ru_tones_to_nl80211_he_ru_alloc() is different from
> ath12k_he_ru_tones_to_nl80211_he_ru_alloc().
>
> the logic of ath12k_he_ru_tones_to_nl80211_he_ru_alloc() is
Sure, I can read C. But _why_ do we have two very similar but still
different functions. That looks fishy to me.
--
https://patchwork.kernel.org/project/linux-wireless/list/
https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
Lingbo Kong <[email protected]> writes:
> On 2024/4/25 18:37, Kalle Valo wrote:
>> Lingbo Kong <[email protected]> writes:
>>
>>> Currently, the transmit rate of "iw dev xxx station dump" command
>>> always show an invalid value.
>>>
>>> To address this issue, ath12k parse the info of transmit complete
>>> report from firmware and indicate the transmit rate to mac80211.
>>>
>>> This patch affects the station mode of WCN7850 and QCN9274.
>>>
>>> After that, "iw dev xxx station dump" show the correct transmit rate.
>>> Such as:
>>>
>>> Station 00:03:7f:12:03:03 (on wlo1)
>>> inactive time: 872 ms
>>> rx bytes: 219111
>>> rx packets: 1133
>>> tx bytes: 53767
>>> tx packets: 462
>>> tx retries: 51
>>> tx failed: 0
>>> beacon loss: 0
>>> beacon rx: 403
>>> rx drop misc: 74
>>> signal: -95 dBm
>>> beacon signal avg: -18 dBm
>>> tx bitrate: 1441.1 MBit/s 80MHz EHT-MCS 13 EHT-NSS 2 EHT-GI 0
>>>
>>> Tested-on: WCN7850 hw2.0 PCI WLAN.HMT.1.0.c5-00481-QCAHMTSWPL_V1.0_V2.0_SILICONZ-3
>>> Tested-on: QCN9274 hw2.0 PCI WLAN.WBE.1.2.1-00201-QCAHKSWPL_SILICONZ-1
>>>
>>> Signed-off-by: Lingbo Kong <[email protected]>
>> [...]
>>
>>> +static void ath12k_dp_tx_update(struct ath12k *ar, struct hal_tx_status *ts)
>>> +{
>>> + if (ar->last_ppdu_id != 0) {
>>> + if (ar->last_ppdu_id == ts->ppdu_id ||
>>> + ar->cached_ppdu_id == ar->last_ppdu_id)
>>> + ar->cached_ppdu_id = ar->last_ppdu_id;
>>> +
>>> + ath12k_dp_tx_update_txcompl(ar, ts);
>>> + }
>>> +
>>> + ar->last_ppdu_id = ts->ppdu_id;
>>> +}
>> A code comment would help a lot. Why is ar->cached_ppdu_id needed
>> here?
>> And if 'ar->cached_ppdu_id == ar->last_ppdu_id' is true why do then
>> do
>> 'ar->cached_ppdu_id = ar->last_ppdu_id'? The value of ar->cached_ppdu_id
>> is not changing here (unless I'm missing something).
>> Also I'm worried about locking. How is access to ar->last_ppdu_id
>> and
>> ar->cached_ppdu_id protected?
>>
>
> Thanks for pointing to this.
> you're right, the ar->cached_ppdu_id haven't used in here, so need to
> delete it.
> i missed something in here.
>
> So, change the ath12k_dp_tx_update(struct ath12k *ar, struct
> hal_tx_status *ts) to
> static void ath12k_dp_tx_update(struct ath12k *ar, struct hal_tx_status *ts)
> {
> if (ts->flags & HAL_TX_STATUS_FLAGS_FIRST_MSDU) {
> if (ar->last_ppdu_id != 0)
> ath12k_dp_tx_update_txcompl(ar, ts);
> ar->last_ppdu_id = ts->ppdu_id;
> }
> }
Access to ar->last_ppdu_id still looks racy to me.
And why do we need to track last_ppdu_id? I don't have time to start
investigating that right now, a code comment explaining that would help
a lot.
--
https://patchwork.kernel.org/project/linux-wireless/list/
https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
On 4/19/2024 8:51 AM, Lingbo Kong wrote:
> Currently, the transmit rate of "iw dev xxx station dump" command
> always show an invalid value.
>
> To address this issue, ath12k parse the info of transmit complete
> report from firmware and indicate the transmit rate to mac80211.
>
> This patch affects the station mode of WCN7850 and QCN9274.
>
> After that, "iw dev xxx station dump" show the correct transmit rate.
> Such as:
>
> Station 00:03:7f:12:03:03 (on wlo1)
> inactive time: 872 ms
> rx bytes: 219111
> rx packets: 1133
> tx bytes: 53767
> tx packets: 462
> tx retries: 51
> tx failed: 0
> beacon loss: 0
> beacon rx: 403
> rx drop misc: 74
> signal: -95 dBm
> beacon signal avg: -18 dBm
> tx bitrate: 1441.1 MBit/s 80MHz EHT-MCS 13 EHT-NSS 2 EHT-GI 0
>
> Tested-on: WCN7850 hw2.0 PCI WLAN.HMT.1.0.c5-00481-QCAHMTSWPL_V1.0_V2.0_SILICONZ-3
> Tested-on: QCN9274 hw2.0 PCI WLAN.WBE.1.2.1-00201-QCAHKSWPL_SILICONZ-1
>
> Signed-off-by: Lingbo Kong <[email protected]>
> ---
> v4:
> 1.change ATH12K_EHT_MCS_MAX from 13 to 15
>
> v3:
> no change
>
> v2:
> 1.change copyright
>
> drivers/net/wireless/ath/ath12k/core.h | 2 +
> drivers/net/wireless/ath/ath12k/dp_rx.h | 3 +
> drivers/net/wireless/ath/ath12k/dp_tx.c | 147 ++++++++++++++++++++++-
> drivers/net/wireless/ath/ath12k/hal_tx.h | 9 +-
> drivers/net/wireless/ath/ath12k/mac.c | 124 +++++++++++++++++++
> drivers/net/wireless/ath/ath12k/mac.h | 4 +-
> 6 files changed, 282 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/net/wireless/ath/ath12k/core.h b/drivers/net/wireless/ath/ath12k/core.h
> index 5d3c1fb632b0..b2ddd1e6fb14 100644
> --- a/drivers/net/wireless/ath/ath12k/core.h
> +++ b/drivers/net/wireless/ath/ath12k/core.h
> @@ -74,6 +74,7 @@ enum wme_ac {
> #define ATH12K_HT_MCS_MAX 7
> #define ATH12K_VHT_MCS_MAX 9
> #define ATH12K_HE_MCS_MAX 11
> +#define ATH12K_EHT_MCS_MAX 15
>
> enum ath12k_crypt_mode {
> /* Only use hardware crypto engine */
> @@ -448,6 +449,7 @@ struct ath12k_sta {
> struct ath12k_rx_peer_stats *rx_stats;
> struct ath12k_wbm_tx_stats *wbm_tx_stats;
> u32 bw_prev;
> + u32 peer_nss;
> };
>
> #define ATH12K_MIN_5G_FREQ 4150
> diff --git a/drivers/net/wireless/ath/ath12k/dp_rx.h b/drivers/net/wireless/ath/ath12k/dp_rx.h
> index 2ff421160181..1543788c0da7 100644
> --- a/drivers/net/wireless/ath/ath12k/dp_rx.h
> +++ b/drivers/net/wireless/ath/ath12k/dp_rx.h
> @@ -79,6 +79,9 @@ static inline u32 ath12k_he_gi_to_nl80211_he_gi(u8 sgi)
> case RX_MSDU_START_SGI_3_2_US:
> ret = NL80211_RATE_INFO_HE_GI_3_2;
> break;
> + default:
> + ret = NL80211_RATE_INFO_HE_GI_0_8;
> + break;
> }
>
> return ret;
> diff --git a/drivers/net/wireless/ath/ath12k/dp_tx.c b/drivers/net/wireless/ath/ath12k/dp_tx.c
> index 9b6d7d72f57c..74ef4c7a72c1 100644
> --- a/drivers/net/wireless/ath/ath12k/dp_tx.c
> +++ b/drivers/net/wireless/ath/ath12k/dp_tx.c
> @@ -8,6 +8,8 @@
> #include "dp_tx.h"
> #include "debug.h"
> #include "hw.h"
> +#include "peer.h"
> +#include "mac.h"
>
> static enum hal_tcl_encap_type
> ath12k_dp_tx_get_encap_type(struct ath12k_vif *arvif, struct sk_buff *skb)
> @@ -443,6 +445,125 @@ ath12k_dp_tx_process_htt_tx_complete(struct ath12k_base *ab,
> }
> }
>
> +static void ath12k_dp_tx_update_txcompl(struct ath12k *ar, struct hal_tx_status *ts)
> +{
> + struct ath12k_base *ab = ar->ab;
> + struct ath12k_peer *peer;
> + struct ath12k_sta *arsta;
> + struct ieee80211_sta *sta;
> + u16 rate;
> + u8 rate_idx = 0;
> + int ret;
> +
> + spin_lock_bh(&ab->base_lock);
> +
> + peer = ath12k_peer_find_by_id(ab, ts->peer_id);
> + if (!peer || !peer->sta) {
> + ath12k_dbg(ab, ATH12K_DBG_DP_TX,
> + "failed to find the peer by id %u\n", ts->peer_id);
> + goto err_out;
> + }
> +
> + sta = peer->sta;
> + arsta = ath12k_sta_to_arsta(sta);
> +
> + memset(&arsta->txrate, 0, sizeof(arsta->txrate));
> +
> + /* This is to prefer choose the real NSS value arsta->last_txrate.nss,
> + * if it is invalid, then choose the NSS value while assoc.
> + */
> + if (arsta->last_txrate.nss)
> + arsta->txrate.nss = arsta->last_txrate.nss;
> + else
> + arsta->txrate.nss = arsta->peer_nss;
> +
> + if (ts->pkt_type == HAL_TX_RATE_STATS_PKT_TYPE_11A ||
> + ts->pkt_type == HAL_TX_RATE_STATS_PKT_TYPE_11B) {
> + ret = ath12k_mac_hw_ratecode_to_legacy_rate(ts->mcs,
> + ts->pkt_type,
> + &rate_idx,
> + &rate);
> + if (ret < 0) {
> + ath12k_warn(ab, "Invalid tx legacy rate %d\n", ret);
> + goto err_out;
> + }
> +
> + arsta->txrate.legacy = rate;
> + } else if (ts->pkt_type == HAL_TX_RATE_STATS_PKT_TYPE_11N) {
> + if (ts->mcs > ATH12K_HT_MCS_MAX) {
> + ath12k_warn(ab, "Invalid HT mcs index %d\n", ts->mcs);
> + goto err_out;
> + }
> +
> + if (arsta->txrate.nss != 0)
> + arsta->txrate.mcs = ts->mcs + 8 * (arsta->txrate.nss - 1);
> +
> + arsta->txrate.flags = RATE_INFO_FLAGS_MCS;
> +
> + if (ts->sgi)
> + arsta->txrate.flags |= RATE_INFO_FLAGS_SHORT_GI;
> + } else if (ts->pkt_type == HAL_TX_RATE_STATS_PKT_TYPE_11AC) {
> + if (ts->mcs > ATH12K_VHT_MCS_MAX) {
> + ath12k_warn(ab, "Invalid VHT mcs index %d\n", ts->mcs);
> + goto err_out;
> + }
> +
> + arsta->txrate.mcs = ts->mcs;
> + arsta->txrate.flags = RATE_INFO_FLAGS_VHT_MCS;
> +
> + if (ts->sgi)
> + arsta->txrate.flags |= RATE_INFO_FLAGS_SHORT_GI;
> + } else if (ts->pkt_type == HAL_TX_RATE_STATS_PKT_TYPE_11AX) {
> + if (ts->mcs > ATH12K_HE_MCS_MAX) {
> + ath12k_warn(ab, "Invalid HE mcs index %d\n", ts->mcs);
> + goto err_out;
> + }
> +
> + arsta->txrate.mcs = ts->mcs;
> + arsta->txrate.flags = RATE_INFO_FLAGS_HE_MCS;
> + arsta->txrate.he_gi = ath12k_he_gi_to_nl80211_he_gi(ts->sgi);
> + } else if (ts->pkt_type == HAL_TX_RATE_STATS_PKT_TYPE_11BE) {
> + if (ts->mcs > ATH12K_EHT_MCS_MAX) {
> + ath12k_warn(ab, "Invalid EHT mcs index %d\n", ts->mcs);
> + goto err_out;
> + }
> +
> + arsta->txrate.mcs = ts->mcs;
> + arsta->txrate.flags = RATE_INFO_FLAGS_EHT_MCS;
> + arsta->txrate.eht_gi = ath12k_mac_eht_gi_to_nl80211_eht_gi(ts->sgi);
> + }
> +
> + arsta->txrate.bw = ath12k_mac_bw_to_mac80211_bw(ts->bw);
> +
> + if (ts->ofdma && ts->pkt_type == HAL_TX_RATE_STATS_PKT_TYPE_11AX) {
> + arsta->txrate.bw = RATE_INFO_BW_HE_RU;
> + arsta->txrate.he_ru_alloc =
> + ath12k_mac_he_ru_tones_to_nl80211_he_ru_alloc(ts->ru_tones);
> + }
> +
> + if (ts->ofdma && ts->pkt_type == HAL_TX_RATE_STATS_PKT_TYPE_11BE) {
> + arsta->txrate.bw = RATE_INFO_BW_EHT_RU;
> + arsta->txrate.eht_ru_alloc =
> + ath12k_mac_eht_ru_tones_to_nl80211_eht_ru_alloc(ts->ru_tones);
> + }
> +
> +err_out:
> + spin_unlock_bh(&ab->base_lock);
> +}
> +
> +static void ath12k_dp_tx_update(struct ath12k *ar, struct hal_tx_status *ts)
> +{
> + if (ar->last_ppdu_id != 0) {
> + if (ar->last_ppdu_id == ts->ppdu_id ||
> + ar->cached_ppdu_id == ar->last_ppdu_id)
> + ar->cached_ppdu_id = ar->last_ppdu_id;
> +
> + ath12k_dp_tx_update_txcompl(ar, ts);
> + }
> +
> + ar->last_ppdu_id = ts->ppdu_id;
> +}
> +
> static void ath12k_dp_tx_complete_msdu(struct ath12k *ar,
> struct sk_buff *msdu,
> struct hal_tx_status *ts)
> @@ -498,6 +619,8 @@ static void ath12k_dp_tx_complete_msdu(struct ath12k *ar,
> * Might end up reporting it out-of-band from HTT stats.
> */
>
> + ath12k_dp_tx_update(ar, ts);
> +
> ieee80211_tx_status_skb(ath12k_ar_to_hw(ar), msdu);
>
> exit:
> @@ -522,10 +645,26 @@ static void ath12k_dp_tx_status_parse(struct ath12k_base *ab,
>
> ts->ppdu_id = le32_get_bits(desc->info1,
> HAL_WBM_COMPL_TX_INFO1_TQM_STATUS_NUMBER);
> - if (le32_to_cpu(desc->rate_stats.info0) & HAL_TX_RATE_STATS_INFO0_VALID)
> - ts->rate_stats = le32_to_cpu(desc->rate_stats.info0);
> - else
> - ts->rate_stats = 0;
> +
> + if (le32_to_cpu(desc->info2) & HAL_WBM_COMPL_TX_INFO2_FIRST_MSDU)
> + ts->flags |= HAL_TX_STATUS_FLAGS_FIRST_MSDU;
> +
> + ts->peer_id = le32_get_bits(desc->info3, HAL_WBM_COMPL_TX_INFO3_PEER_ID);
> +
> + if (le32_to_cpu(desc->rate_stats.info0) & HAL_TX_RATE_STATS_INFO0_VALID) {
> + ts->pkt_type = le32_get_bits(desc->rate_stats.info0,
> + HAL_TX_RATE_STATS_INFO0_PKT_TYPE);
> + ts->mcs = le32_get_bits(desc->rate_stats.info0,
> + HAL_TX_RATE_STATS_INFO0_MCS);
> + ts->sgi = le32_get_bits(desc->rate_stats.info0,
> + HAL_TX_RATE_STATS_INFO0_SGI);
> + ts->bw = le32_get_bits(desc->rate_stats.info0,
> + HAL_TX_RATE_STATS_INFO0_BW);
> + ts->ru_tones = le32_get_bits(desc->rate_stats.info0,
> + HAL_TX_RATE_STATS_INFO0_TONES_IN_RU);
> + ts->ofdma = le32_get_bits(desc->rate_stats.info0,
> + HAL_TX_RATE_STATS_INFO0_OFDMA_TX);
> + }
Why multiple read from dma mapped area say desc->rate_stats.info0 lead
to increase in CPU cycles. Instead you do one read from dma mapped area
desc->rate_stats.info0 and classify into your own data structure ?
And the info0 classification used within the
ath12k_dp_tx_update_txcompl(), so you can do the classification within
this API.
--
Karthikeyan Periyasamy
--
கார்த்திகேயன் பெரியசாமி
On 2024/4/29 17:11, Karthikeyan Periyasamy wrote:
>
>
> On 4/19/2024 8:51 AM, Lingbo Kong wrote:
>> Currently, the transmit rate of "iw dev xxx station dump" command
>> always show an invalid value.
>>
>> To address this issue, ath12k parse the info of transmit complete
>> report from firmware and indicate the transmit rate to mac80211.
>>
>> This patch affects the station mode of WCN7850 and QCN9274.
>>
>> After that, "iw dev xxx station dump" show the correct transmit rate.
>> Such as:
>>
>> Station 00:03:7f:12:03:03 (on wlo1)
>> inactive time: 872 ms
>> rx bytes: 219111
>> rx packets: 1133
>> tx bytes: 53767
>> tx packets: 462
>> tx retries: 51
>> tx failed: 0
>> beacon loss: 0
>> beacon rx: 403
>> rx drop misc: 74
>> signal: -95 dBm
>> beacon signal avg: -18 dBm
>> tx bitrate: 1441.1 MBit/s 80MHz EHT-MCS 13 EHT-NSS 2
>> EHT-GI 0
>>
>> Tested-on: WCN7850 hw2.0 PCI
>> WLAN.HMT.1.0.c5-00481-QCAHMTSWPL_V1.0_V2.0_SILICONZ-3
>> Tested-on: QCN9274 hw2.0 PCI WLAN.WBE.1.2.1-00201-QCAHKSWPL_SILICONZ-1
>>
>> Signed-off-by: Lingbo Kong <[email protected]>
[...]
>> @@ -522,10 +645,26 @@ static void ath12k_dp_tx_status_parse(struct
>> ath12k_base *ab,
>> ts->ppdu_id = le32_get_bits(desc->info1,
>> HAL_WBM_COMPL_TX_INFO1_TQM_STATUS_NUMBER);
>> - if (le32_to_cpu(desc->rate_stats.info0) &
>> HAL_TX_RATE_STATS_INFO0_VALID)
>> - ts->rate_stats = le32_to_cpu(desc->rate_stats.info0);
>> - else
>> - ts->rate_stats = 0;
>> +
>> + if (le32_to_cpu(desc->info2) & HAL_WBM_COMPL_TX_INFO2_FIRST_MSDU)
>> + ts->flags |= HAL_TX_STATUS_FLAGS_FIRST_MSDU;
>> +
>> + ts->peer_id = le32_get_bits(desc->info3,
>> HAL_WBM_COMPL_TX_INFO3_PEER_ID);
>> +
>> + if (le32_to_cpu(desc->rate_stats.info0) &
>> HAL_TX_RATE_STATS_INFO0_VALID) {
>> + ts->pkt_type = le32_get_bits(desc->rate_stats.info0,
>> + HAL_TX_RATE_STATS_INFO0_PKT_TYPE);
>> + ts->mcs = le32_get_bits(desc->rate_stats.info0,
>> + HAL_TX_RATE_STATS_INFO0_MCS);
>> + ts->sgi = le32_get_bits(desc->rate_stats.info0,
>> + HAL_TX_RATE_STATS_INFO0_SGI);
>> + ts->bw = le32_get_bits(desc->rate_stats.info0,
>> + HAL_TX_RATE_STATS_INFO0_BW);
>> + ts->ru_tones = le32_get_bits(desc->rate_stats.info0,
>> + HAL_TX_RATE_STATS_INFO0_TONES_IN_RU);
>> + ts->ofdma = le32_get_bits(desc->rate_stats.info0,
>> + HAL_TX_RATE_STATS_INFO0_OFDMA_TX);
>> + }
>
>
> Why multiple read from dma mapped area say desc->rate_stats.info0 lead
> to increase in CPU cycles. Instead you do one read from dma mapped area
> desc->rate_stats.info0 and classify into your own data structure ?
>
> And the info0 classification used within the
> ath12k_dp_tx_update_txcompl(), so you can do the classification within
> this API.
>
yes, thanks for pointing of this.
i will apply it in next version:),
Best regards
Lingbo Kong
On 2024/4/26 19:21, Kalle Valo wrote:
> Lingbo Kong <[email protected]> writes:
>
>> On 2024/4/26 0:54, Kalle Valo wrote:
>>> Lingbo Kong <[email protected]> writes:
>>>
>>>> +static void ath12k_dp_tx_update_txcompl(struct ath12k *ar, struct
>>>> hal_tx_status *ts)
>>>> +{
>>>> + struct ath12k_base *ab = ar->ab;
>>>> + struct ath12k_peer *peer;
>>>> + struct ath12k_sta *arsta;
>>>> + struct ieee80211_sta *sta;
>>>> + u16 rate;
>>>> + u8 rate_idx = 0;
>>>> + int ret;
>>>> +
>>>> + spin_lock_bh(&ab->base_lock);
>>>
>>> Did you analyse how this function, and especially taking the
>>> base_lock,
>>> affects performance?
>>
>> The base_lock is used here because of the need to look for peers based
>> on the ts->peer_id when calling ath12k_peer_find_by_id() function,
>> which i think might affect performance.
>>
>> Do i need to run a throughput test?
>
> Ok, so to answer my question: no, you didn't do any performance
> analysis. Throughput test might not be enough, for example the driver
> can be used on slower systems and running the test on a fast CPU might
> not reveal any problem. A proper analysis would be much better.
>
hi, kalle,
i found that ab->base_lock is used in a lot of places in ath12k, so it's
complicated to do performance analysis in here.
Do you have any suggestions? I would appreciate your suggestions:)
/lingbo kong
>>>> +enum nl80211_he_ru_alloc
>>>> ath12k_mac_he_ru_tones_to_nl80211_he_ru_alloc(u16 ru_tones)
>>>> +{
>>>> + enum nl80211_he_ru_alloc ret;
>>>> +
>>>> + switch (ru_tones) {
>>>> + case 26:
>>>> + ret = NL80211_RATE_INFO_HE_RU_ALLOC_26;
>>>> + break;
>>>> + case 52:
>>>> + ret = NL80211_RATE_INFO_HE_RU_ALLOC_52;
>>>> + break;
>>>> + case 106:
>>>> + ret = NL80211_RATE_INFO_HE_RU_ALLOC_106;
>>>> + break;
>>>> + case 242:
>>>> + ret = NL80211_RATE_INFO_HE_RU_ALLOC_242;
>>>> + break;
>>>> + case 484:
>>>> + ret = NL80211_RATE_INFO_HE_RU_ALLOC_484;
>>>> + break;
>>>> + case 996:
>>>> + ret = NL80211_RATE_INFO_HE_RU_ALLOC_996;
>>>> + break;
>>>> + case (996 * 2):
>>>> + ret = NL80211_RATE_INFO_HE_RU_ALLOC_2x996;
>>>> + break;
>>>> + default:
>>>> + ret = NL80211_RATE_INFO_HE_RU_ALLOC_26;
>>>> + break;
>>>> + }
>>>> +
>>>> + return ret;
>>>> +}
>>> How does this function compare to
>>> ath12k_he_ru_tones_to_nl80211_he_ru_alloc()?
>>>
>>
>> ath12k_mac_he_ru_tones_to_nl80211_he_ru_alloc() is different from
>> ath12k_he_ru_tones_to_nl80211_he_ru_alloc().
>>
>> the logic of ath12k_he_ru_tones_to_nl80211_he_ru_alloc() is
>
> Sure, I can read C. But _why_ do we have two very similar but still
> different functions. That looks fishy to me.
>
On 2024/4/26 19:24, Kalle Valo wrote:
> Lingbo Kong <[email protected]> writes:
>
>> On 2024/4/25 18:37, Kalle Valo wrote:
>>> Lingbo Kong <[email protected]> writes:
>>>
>>>> Currently, the transmit rate of "iw dev xxx station dump" command
>>>> always show an invalid value.
>>>>
>>>> To address this issue, ath12k parse the info of transmit complete
>>>> report from firmware and indicate the transmit rate to mac80211.
>>>>
>>>> This patch affects the station mode of WCN7850 and QCN9274.
>>>>
>>>> After that, "iw dev xxx station dump" show the correct transmit rate.
>>>> Such as:
>>>>
>>>> Station 00:03:7f:12:03:03 (on wlo1)
>>>> inactive time: 872 ms
>>>> rx bytes: 219111
>>>> rx packets: 1133
>>>> tx bytes: 53767
>>>> tx packets: 462
>>>> tx retries: 51
>>>> tx failed: 0
>>>> beacon loss: 0
>>>> beacon rx: 403
>>>> rx drop misc: 74
>>>> signal: -95 dBm
>>>> beacon signal avg: -18 dBm
>>>> tx bitrate: 1441.1 MBit/s 80MHz EHT-MCS 13 EHT-NSS 2 EHT-GI 0
>>>>
>>>> Tested-on: WCN7850 hw2.0 PCI WLAN.HMT.1.0.c5-00481-QCAHMTSWPL_V1.0_V2.0_SILICONZ-3
>>>> Tested-on: QCN9274 hw2.0 PCI WLAN.WBE.1.2.1-00201-QCAHKSWPL_SILICONZ-1
>>>>
>>>> Signed-off-by: Lingbo Kong <[email protected]>
>>> [...]
>>>
>>>> +static void ath12k_dp_tx_update(struct ath12k *ar, struct hal_tx_status *ts)
>>>> +{
>>>> + if (ar->last_ppdu_id != 0) {
>>>> + if (ar->last_ppdu_id == ts->ppdu_id ||
>>>> + ar->cached_ppdu_id == ar->last_ppdu_id)
>>>> + ar->cached_ppdu_id = ar->last_ppdu_id;
>>>> +
>>>> + ath12k_dp_tx_update_txcompl(ar, ts);
>>>> + }
>>>> +
>>>> + ar->last_ppdu_id = ts->ppdu_id;
>>>> +}
>>> A code comment would help a lot. Why is ar->cached_ppdu_id needed
>>> here?
>>> And if 'ar->cached_ppdu_id == ar->last_ppdu_id' is true why do then
>>> do
>>> 'ar->cached_ppdu_id = ar->last_ppdu_id'? The value of ar->cached_ppdu_id
>>> is not changing here (unless I'm missing something).
>>> Also I'm worried about locking. How is access to ar->last_ppdu_id
>>> and
>>> ar->cached_ppdu_id protected?
>>>
>>
>> Thanks for pointing to this.
>> you're right, the ar->cached_ppdu_id haven't used in here, so need to
>> delete it.
>> i missed something in here.
>>
>> So, change the ath12k_dp_tx_update(struct ath12k *ar, struct
>> hal_tx_status *ts) to
>> static void ath12k_dp_tx_update(struct ath12k *ar, struct hal_tx_status *ts)
>> {
>> if (ts->flags & HAL_TX_STATUS_FLAGS_FIRST_MSDU) {
>> if (ar->last_ppdu_id != 0)
>> ath12k_dp_tx_update_txcompl(ar, ts);
>> ar->last_ppdu_id = ts->ppdu_id;
>> }
>> }
>
> Access to ar->last_ppdu_id still looks racy to me.
>
> And why do we need to track last_ppdu_id? I don't have time to start
> investigating that right now, a code comment explaining that would help
> a lot.
yes, you are right, kalle, thanks for pointing of this.
There really isn't a need to add a judgement of last_ppdu_id to this place.
The ath12k_dp_tx_update_txcompl() function should be called directly and
no need to define ath12k_dp_tx_update() function.
Best regards
Lingbo Kong
On 2024/4/26 19:21, Kalle Valo wrote:
> Lingbo Kong <[email protected]> writes:
>
>> On 2024/4/26 0:54, Kalle Valo wrote:
>>> Lingbo Kong <[email protected]> writes:
>>>
>>>> +static void ath12k_dp_tx_update_txcompl(struct ath12k *ar, struct
>>>> hal_tx_status *ts)
>>>> +{
>>>> + struct ath12k_base *ab = ar->ab;
>>>> + struct ath12k_peer *peer;
>>>> + struct ath12k_sta *arsta;
>>>> + struct ieee80211_sta *sta;
>>>> + u16 rate;
>>>> + u8 rate_idx = 0;
>>>> + int ret;
>>>> +
>>>> + spin_lock_bh(&ab->base_lock);
>>>
>>> Did you analyse how this function, and especially taking the
>>> base_lock,
>>> affects performance?
>>
>> The base_lock is used here because of the need to look for peers based
>> on the ts->peer_id when calling ath12k_peer_find_by_id() function,
>> which i think might affect performance.
>>
>> Do i need to run a throughput test?
>
> Ok, so to answer my question: no, you didn't do any performance
> analysis. Throughput test might not be enough, for example the driver
> can be used on slower systems and running the test on a fast CPU might
> not reveal any problem. A proper analysis would be much better.
>
Hi, kalle,
I did a simple performance analysis of the ath12k_dp_tx_update_txcompl()
function on slower systems.
Firstly, i use perf tool to set dynamic tracepoints in
ath12k_dp_tx_complete_msdu() function, and then used the command of
"iperf -c ip address -w 4M -n 1G -i 1" to do traffic test.
During this process, use ./perf record -a -g to detect the performace of
the system.
Finally, compare the results with and without this patch.
without this patch
./perf report output
children self command symbol
7.28% 0.08% ksoftirqd/0 ath12k_dp_tx_complete_msdu
5.96% 0.03% swapper ath12k_dp_tx_complete_msdu
iperf output
[ 1] 0.0000-62.6712 sec 1.00 GBytes 137 Mbits/sec
with this patch
children self command symbol
7.42% 0.08% ksoftirqd/0 ath12k_dp_tx_complete_msdu
6.32% 0.03% swapper ath12k_dp_tx_complete_msdu
iperf output
[ 1] 0.0000-62.6732 sec 1.00 GBytes 137 Mbits/sec
As can be seen from the table above, with this patch, the CPU time
percentage will increase by 0.5%.
So, i think applying this patch will definitely have an impact on system
performance, but the impact is not that big and i think it can be ignored:)
Best regards
Lingbo Kong