Received-SPF: pass (google.com: domain of linux-wireless+bounces-6928-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:40f1:3f00::1 as permitted sender) client-ip=2604:1380:40f1:3f00::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 v4 1/3] wifi: ath12k: report station mode transmit rate
References: <20240419032122.7009-1-quic_lingbok@quicinc.com>
	<20240419032122.7009-2-quic_lingbok@quicinc.com>
	<87r0etsp6u.fsf@kernel.org>
	<c01db4fc-03a0-41f6-9f16-bfa3b2c08007@quicinc.com>
Date: Fri, 26 Apr 2024 14:24:21 +0300
In-Reply-To: <c01db4fc-03a0-41f6-9f16-bfa3b2c08007@quicinc.com> (Lingbo Kong's
	message of "Fri, 26 Apr 2024 16:01:19 +0800")
Message-ID: <87r0esqsd6.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/4/25 18:37, Kalle Valo wrote:
>> Lingbo Kong <quic_lingbok@quicinc.com> writes:
>> 
>>> Currently, the transmit rate of "iw dev xxx station dump" command
>>> always show an invalid value.
>>>
>>> To address this issue, ath12k parse the info of transmit complete
>>> report from firmware and indicate the transmit rate to mac80211.
>>>
>>> This patch affects the station mode of WCN7850 and QCN9274.
>>>
>>> After that, "iw dev xxx station dump" show the correct transmit rate.
>>> Such as:
>>>
>>> Station 00:03:7f:12:03:03 (on wlo1)
>>>          inactive time:  872 ms
>>>          rx bytes:       219111
>>>          rx packets:     1133
>>>          tx bytes:       53767
>>>          tx packets:     462
>>>          tx retries:     51
>>>          tx failed:      0
>>>          beacon loss:    0
>>>          beacon rx:      403
>>>          rx drop misc:   74
>>>          signal:         -95 dBm
>>>          beacon signal avg:      -18 dBm
>>>          tx bitrate:     1441.1 MBit/s 80MHz EHT-MCS 13 EHT-NSS 2 EHT-GI 0
>>>
>>> Tested-on: WCN7850 hw2.0 PCI WLAN.HMT.1.0.c5-00481-QCAHMTSWPL_V1.0_V2.0_SILICONZ-3
>>> Tested-on: QCN9274 hw2.0 PCI WLAN.WBE.1.2.1-00201-QCAHKSWPL_SILICONZ-1
>>>
>>> Signed-off-by: Lingbo Kong <quic_lingbok@quicinc.com>
>> [...]
>> 
>>> +static void ath12k_dp_tx_update(struct ath12k *ar, struct hal_tx_status *ts)
>>> +{
>>> +	if (ar->last_ppdu_id != 0) {
>>> +		if (ar->last_ppdu_id == ts->ppdu_id ||
>>> +		    ar->cached_ppdu_id == ar->last_ppdu_id)
>>> +			ar->cached_ppdu_id = ar->last_ppdu_id;
>>> +
>>> +		ath12k_dp_tx_update_txcompl(ar, ts);
>>> +	}
>>> +
>>> +	ar->last_ppdu_id = ts->ppdu_id;
>>> +}
>> A code comment would help a lot. Why is ar->cached_ppdu_id needed
>> here?
>> And if 'ar->cached_ppdu_id == ar->last_ppdu_id' is true why do then
>> do
>> 'ar->cached_ppdu_id = ar->last_ppdu_id'? The value of ar->cached_ppdu_id
>> is not changing here (unless I'm missing something).
>> Also I'm worried about locking. How is access to ar->last_ppdu_id
>> and
>> ar->cached_ppdu_id protected?
>> 
>
> Thanks for pointing to this.
> you're right, the ar->cached_ppdu_id haven't used in here, so need to
> delete it.
> i missed something in here.
>
> So, change the ath12k_dp_tx_update(struct ath12k *ar, struct
> hal_tx_status *ts) to
> static void ath12k_dp_tx_update(struct ath12k *ar, struct hal_tx_status *ts)
> {
> 	if (ts->flags & HAL_TX_STATUS_FLAGS_FIRST_MSDU) {
> 		if (ar->last_ppdu_id != 0)
> 			ath12k_dp_tx_update_txcompl(ar, ts);
> 		ar->last_ppdu_id = ts->ppdu_id;
> 	}
> }

Access to ar->last_ppdu_id still looks racy to me.

And why do we need to track last_ppdu_id? I don't have time to start
investigating that right now, a code comment explaining that would help
a lot.

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

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