On 2024/3/21 0:39, Kalle Valo wrote:
Lingbo Kong <quic_lingbok@xxxxxxxxxxx> writes:
The signal of "iw dev xxx station dump" always show 0 dBm. This is because
currently signal is only set in ath12k_mgmt_rx_event function, and not set
for rx data packet. So, change to get signal from firmware and report to
mac80211.
/* 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 &&
+ ar->ab->hw_params->supports_rssi_stats &&
+ !(ath12k_mac_get_fw_stats(ar, ar->pdev->pdev_id, 0,
+ WMI_REQUEST_VDEV_STAT)))
+ signal = arsta->rssi_beacon;
+
+ if (signal) {
+ sinfo->signal = signal;
+ sinfo->filled |= BIT_ULL(NL80211_STA_INFO_SIGNAL);
+ }
}
If I'm reading the patch correctly this is the sequence:
1. ath12k_mac_op_sta_statistics() is called
2. WMI_REQUEST_STATS_CMDID is sent to the firmware
3. ath12k_mac_op_sta_statistics() returns
4. firmware sends WMI_UPDATE_STATS_EVENTID to host
5. ath12k_wmi_tlv_fw_stats_data_parse() stores signal to arsta->rssi_beacon
So doesn't this mean that ath12k_mac_op_sta_statistics() actually uses
the previous value? And if ath12k_mac_op_sta_statistics() is called very
seldom, like once an hour, the signal value can be one hour wrong?
Hi, kalle, you're right.
Thanks you for pointing this out.
I should add a struct completion to make up the synchronization mechanism.
So, i add a struct completion in struct ath12k, then modify the
ath12k_mac_get_fw_stats() function:
+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;
+}
then add "complete(&ar->fw_stats_complete);" at the end of
ath12k_wmi_tlv_fw_stats_data_parse() function.
what do you think of this?
Also I don't see any protection when accessing arsta->rssi_beacon.
i think add protection is unnecessary when accessing arsta->rssi_beacon
in ath12k_mac_op_sta_statistics() function, because we just take its
value and don't change it.
Best regards
Lingbo Kong
