Re: [PATCH] wifi: ath12k: report signal for iw dev xxx station dump

[Kalle Valo <kvalo@kernel.org>]

Lingbo Kong <quic_lingbok@quicinc.com> writes:

> On 2024/3/21 0:39, Kalle Valo wrote:
>> Lingbo Kong <quic_lingbok@quicinc.com> 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