Received-SPF: pass (google.com: domain of linux-wireless+bounces-4990-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
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>
Date: Wed, 20 Mar 2024 18:39:52 +0200
In-Reply-To: <20240219111417.1185-1-quic_lingbok@quicinc.com> (Lingbo Kong's
	message of "Mon, 19 Feb 2024 19:14:17 +0800")
Message-ID: <87o7b8amcn.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:

> 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.
>
> After that, "iw dev xxx station dump" show the correct signal such as:
> signal: -50 dBm
> signal: -49 dBm
>
> Tested-on: WCN7850 hw2.0 PCI
> WLAN.HMT.1.0-03427-QCAHMTSWPL_V1.0_V2.0_SILICONZ-1.15378.4
>
> Signed-off-by: Lingbo Kong <quic_lingbok@quicinc.com>

I'll omit the cosmetics for now and focus on the big picture.

First of all, no mention of WCN7850 and QCN9274 in the commit message.
Not good.

> --- a/drivers/net/wireless/ath/ath12k/hw.c
> +++ b/drivers/net/wireless/ath/ath12k/hw.c
> @@ -920,6 +920,8 @@ static const struct ath12k_hw_params ath12k_hw_params[] = {
>  		.otp_board_id_register = QCN9274_QFPROM_RAW_RFA_PDET_ROW13_LSB,
>  
>  		.supports_sta_ps = false,
> +
> +		.supports_rssi_stats = false,
>  	},
>  	{
>  		.name = "wcn7850 hw2.0",
> @@ -993,6 +995,8 @@ static const struct ath12k_hw_params ath12k_hw_params[] = {
>  		.otp_board_id_register = 0,
>  
>  		.supports_sta_ps = true,
> +
> +		.supports_rssi_stats = true,
>  	},
>  	{
>  		.name = "qcn9274 hw2.0",
> @@ -1061,6 +1065,8 @@ static const struct ath12k_hw_params ath12k_hw_params[] = {
>  		.otp_board_id_register = QCN9274_QFPROM_RAW_RFA_PDET_ROW13_LSB,
>  
>  		.supports_sta_ps = false,
> +
> +		.supports_rssi_stats = false,
>  	},
>  };

Ok, based on this you enable this only for WCN7850 hw2.0. Why is that?

We really should get rid of "I work only on WCN7850 and not care about
QCN9274" mindset. And of course the other team says "I work only on
QCN9274 and not care about WCN7850", sigh. If you work on ath12k you
need to consider *BOTH* WCN7850 and QCN9274 families!

>  static void ath12k_mac_op_sta_statistics(struct ieee80211_hw *hw,
>  					 struct ieee80211_vif *vif,
>  					 struct ieee80211_sta *sta,
>  					 struct station_info *sinfo)
>  {
>  	struct ath12k_sta *arsta = ath12k_sta_to_arsta(sta);
> +	struct ath12k *ar = arsta->arvif->ar;
> +	s8 signal;
>  
>  	sinfo->rx_duration = arsta->rx_duration;
>  	sinfo->filled |= BIT_ULL(NL80211_STA_INFO_RX_DURATION);
> @@ -7473,8 +7503,19 @@ static void ath12k_mac_op_sta_statistics(struct ieee80211_hw *hw,
>  	sinfo->filled |= BIT_ULL(NL80211_STA_INFO_TX_BITRATE);
>  
>  	/* 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?

Also I don't see any protection when accessing arsta->rssi_beacon.

-- 
https://patchwork.kernel.org/project/linux-wireless/list/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches