Re: [PATCH v4 1/3] wifi: ath12k: report station mode transmit rate

[Lingbo Kong <quic_lingbok@xxxxxxxxxxx>]

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;
	}
}

best regards
Lingbo Kong