Received-SPF: pass (google.com: domain of linux-wireless+bounces-5351-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Message-ID: <3f5b9e74-d898-416b-9b08-d008883c7e58@quicinc.com>
Date: Wed, 27 Mar 2024 20:54:42 +0800
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH] wifi: ath12k: report signal for iw dev xxx station dump
To: Kalle Valo <kvalo@kernel.org>
CC: <ath12k@lists.infradead.org>, <linux-wireless@vger.kernel.org>
References: <20240219111417.1185-1-quic_lingbok@quicinc.com>
 <87o7b8amcn.fsf@kernel.org>
Content-Language: en-US
From: Lingbo Kong <quic_lingbok@quicinc.com>
In-Reply-To: <87o7b8amcn.fsf@kernel.org>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 7bit


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?

> 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