Received-SPF: pass (google.com: domain of linux-wireless+bounces-5745-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
From: Kalle Valo <kvalo@kernel.org>
To: Lingbo Kong <quic_lingbok@quicinc.com>
Cc: <ath12k@lists.infradead.org>,  <linux-wireless@vger.kernel.org>
Subject: Re: [PATCH] wifi: ath12k: report signal for iw dev xxx station dump
References: <20240219111417.1185-1-quic_lingbok@quicinc.com>
	<87o7b8amcn.fsf@kernel.org>
	<3f5b9e74-d898-416b-9b08-d008883c7e58@quicinc.com>
Date: Tue, 02 Apr 2024 14:03:19 +0300
In-Reply-To: <3f5b9e74-d898-416b-9b08-d008883c7e58@quicinc.com> (Lingbo Kong's
	message of "Wed, 27 Mar 2024 20:54:42 +0800")
Message-ID: <87a5mc6n7s.fsf@kernel.org>
User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/28.2 (gnu/linux)
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain

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