On 2024/4/26 19:24, Kalle Valo wrote:
Lingbo Kong <quic_lingbok@xxxxxxxxxxx> writes:
On 2024/4/25 18:37, Kalle Valo wrote:
Lingbo Kong <quic_lingbok@xxxxxxxxxxx> 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@xxxxxxxxxxx>
[...]
+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.
yes, you are right, kalle, thanks for pointing of this.
There really isn't a need to add a judgement of last_ppdu_id to this place.
The ath12k_dp_tx_update_txcompl() function should be called directly and
no need to define ath12k_dp_tx_update() function.
Best regards
Lingbo Kong
