Lingbo Kong <quic_lingbok@xxxxxxxxxxx> writes:
> 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?
I can comment better after seeing the patch but something like is needed.
>> 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.
But there's also a race that we can return the old value to the user
space, no?
--
https://patchwork.kernel.org/project/linux-wireless/list/
https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
