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
Date: Fri, 26 Apr 2024 14:21:10 +0300 [thread overview]
Message-ID: <87v844qsih.fsf@kernel.org> (raw)
In-Reply-To: <050ae0d4-c879-40c2-b2ac-1766aaa2c789@quicinc.com> (Lingbo Kong's message of "Fri, 26 Apr 2024 14:41:33 +0800")
Lingbo Kong <quic_lingbok@quicinc.com> writes:
> On 2024/4/26 0:54, Kalle Valo wrote:
>> Lingbo Kong <quic_lingbok@quicinc.com> writes:
>>
>>> +static void ath12k_dp_tx_update_txcompl(struct ath12k *ar, struct
>>> hal_tx_status *ts)
>>> +{
>>> + struct ath12k_base *ab = ar->ab;
>>> + struct ath12k_peer *peer;
>>> + struct ath12k_sta *arsta;
>>> + struct ieee80211_sta *sta;
>>> + u16 rate;
>>> + u8 rate_idx = 0;
>>> + int ret;
>>> +
>>> + spin_lock_bh(&ab->base_lock);
>>
>> Did you analyse how this function, and especially taking the
>> base_lock,
>> affects performance?
>
> The base_lock is used here because of the need to look for peers based
> on the ts->peer_id when calling ath12k_peer_find_by_id() function,
> which i think might affect performance.
>
> Do i need to run a throughput test?
Ok, so to answer my question: no, you didn't do any performance
analysis. Throughput test might not be enough, for example the driver
can be used on slower systems and running the test on a fast CPU might
not reveal any problem. A proper analysis would be much better.
>>> +enum nl80211_he_ru_alloc
>>> ath12k_mac_he_ru_tones_to_nl80211_he_ru_alloc(u16 ru_tones)
>>> +{
>>> + enum nl80211_he_ru_alloc ret;
>>> +
>>> + switch (ru_tones) {
>>> + case 26:
>>> + ret = NL80211_RATE_INFO_HE_RU_ALLOC_26;
>>> + break;
>>> + case 52:
>>> + ret = NL80211_RATE_INFO_HE_RU_ALLOC_52;
>>> + break;
>>> + case 106:
>>> + ret = NL80211_RATE_INFO_HE_RU_ALLOC_106;
>>> + break;
>>> + case 242:
>>> + ret = NL80211_RATE_INFO_HE_RU_ALLOC_242;
>>> + break;
>>> + case 484:
>>> + ret = NL80211_RATE_INFO_HE_RU_ALLOC_484;
>>> + break;
>>> + case 996:
>>> + ret = NL80211_RATE_INFO_HE_RU_ALLOC_996;
>>> + break;
>>> + case (996 * 2):
>>> + ret = NL80211_RATE_INFO_HE_RU_ALLOC_2x996;
>>> + break;
>>> + default:
>>> + ret = NL80211_RATE_INFO_HE_RU_ALLOC_26;
>>> + break;
>>> + }
>>> +
>>> + return ret;
>>> +}
>> How does this function compare to
>> ath12k_he_ru_tones_to_nl80211_he_ru_alloc()?
>>
>
> ath12k_mac_he_ru_tones_to_nl80211_he_ru_alloc() is different from
> ath12k_he_ru_tones_to_nl80211_he_ru_alloc().
>
> the logic of ath12k_he_ru_tones_to_nl80211_he_ru_alloc() is
Sure, I can read C. But _why_ do we have two very similar but still
different functions. That looks fishy to me.
--
https://patchwork.kernel.org/project/linux-wireless/list/
https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
next prev parent reply other threads:[~2024-04-26 11:21 UTC|newest]
Thread overview: 16+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-04-19 3:21 [PATCH v4 0/3] wifi: ath12k: report station mode stats Lingbo Kong
2024-04-19 3:21 ` [PATCH v4 1/3] wifi: ath12k: report station mode transmit rate Lingbo Kong
2024-04-25 10:37 ` Kalle Valo
2024-04-26 8:01 ` Lingbo Kong
2024-04-26 11:24 ` Kalle Valo
2024-05-07 11:06 ` Lingbo Kong
2024-04-25 16:54 ` Kalle Valo
2024-04-26 6:41 ` Lingbo Kong
2024-04-26 11:21 ` Kalle Valo [this message]
2024-04-30 11:41 ` Lingbo Kong
2024-06-05 6:31 ` Lingbo Kong
2024-04-29 9:11 ` Karthikeyan Periyasamy
2024-04-29 9:29 ` Lingbo Kong
2024-04-19 3:21 ` [PATCH v4 2/3] wifi: ath12k: report station mode receive rate for IEEE 802.11be Lingbo Kong
2024-04-19 3:21 ` [PATCH v4 3/3] wifi: ath12k: report station mode signal strength Lingbo Kong
2024-04-25 17:03 ` Kalle Valo
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=87v844qsih.fsf@kernel.org \
--to=kvalo@kernel.org \
--cc=ath12k@lists.infradead.org \
--cc=linux-wireless@vger.kernel.org \
--cc=quic_lingbok@quicinc.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.