| Message ID | 20241219001006.1036495-4-quic_periyasa@quicinc.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | wifi: ath12k: Refactor monitor Rx handler | expand |
Karthikeyan Periyasamy <quic_periyasa@quicinc.com> writes: > Currently, ath12k_dp_mon_rx_parse_status_tlv() takes the TLV tag, TLV data > and TLV userid as separate arguments from the caller. These argument can > be fetched from the TLV header itself. Therefore, pass the TLV header and > retrieve the necessary fields from the header instead of passing them from > the caller. The commit message doesn't reply to "why?". Is there a clear benefit in the future or are you just doing random code cleanup?
On 12/19/2024 5:04 PM, Kalle Valo wrote: > Karthikeyan Periyasamy <quic_periyasa@quicinc.com> writes: > >> Currently, ath12k_dp_mon_rx_parse_status_tlv() takes the TLV tag, TLV data >> and TLV userid as separate arguments from the caller. These argument can >> be fetched from the TLV header itself. Therefore, pass the TLV header and >> retrieve the necessary fields from the header instead of passing them from >> the caller. > > The commit message doesn't reply to "why?". Is there a clear benefit in > the future or are you just doing random code cleanup? > It benefit in the future for supporting EHT monitor functionality.
Karthikeyan Periyasamy <quic_periyasa@quicinc.com> writes: > On 12/19/2024 5:04 PM, Kalle Valo wrote: >> Karthikeyan Periyasamy <quic_periyasa@quicinc.com> writes: >> >>> Currently, ath12k_dp_mon_rx_parse_status_tlv() takes the TLV tag, TLV data >>> and TLV userid as separate arguments from the caller. These argument can >>> be fetched from the TLV header itself. Therefore, pass the TLV header and >>> retrieve the necessary fields from the header instead of passing them from >>> the caller. >> The commit message doesn't reply to "why?". Is there a clear benefit >> in >> the future or are you just doing random code cleanup? >> > > It benefit in the future for supporting EHT monitor functionality. How does it help exactly? Having even just one sentence explaining the motivation in the commit message would help a lot. Otherwise these just look random changes.
On 12/20/2024 12:24 AM, Kalle Valo wrote: > Karthikeyan Periyasamy <quic_periyasa@quicinc.com> writes: > >> On 12/19/2024 5:04 PM, Kalle Valo wrote: >>> Karthikeyan Periyasamy <quic_periyasa@quicinc.com> writes: >>> >>>> Currently, ath12k_dp_mon_rx_parse_status_tlv() takes the TLV tag, TLV data >>>> and TLV userid as separate arguments from the caller. These argument can >>>> be fetched from the TLV header itself. Therefore, pass the TLV header and >>>> retrieve the necessary fields from the header instead of passing them from >>>> the caller. >>> The commit message doesn't reply to "why?". Is there a clear benefit >>> in >>> the future or are you just doing random code cleanup? >>> >> >> It benefit in the future for supporting EHT monitor functionality. > > How does it help exactly? Having even just one sentence explaining the > motivation in the commit message would help a lot. Otherwise these just > look random changes. > Currently, ath12k_dp_mon_rx_parse_status_tlv() takes the TLV tag, TLV data and TLV user ID as separate arguments from the caller. In the future, the TLV length will be needed for the EHT monitor TLV tags. These arguments can be fetched from the TLV header itself. Therefore, instead of increasing the arguments, pass the TLV header and retrieve the necessary fields from the TLV header instead of passing them from the caller. Above commit is ok ?
diff --git a/drivers/net/wireless/ath/ath12k/dp_mon.c b/drivers/net/wireless/ath/ath12k/dp_mon.c index ae2f6847bc88..53f8e8f8959a 100644 --- a/drivers/net/wireless/ath/ath12k/dp_mon.c +++ b/drivers/net/wireless/ath/ath12k/dp_mon.c @@ -565,12 +565,16 @@ static void ath12k_dp_mon_parse_he_sig_su(const struct hal_rx_he_sig_a_su_info * static enum hal_rx_mon_status ath12k_dp_mon_rx_parse_status_tlv(struct ath12k *ar, struct ath12k_mon_data *pmon, - u32 tlv_tag, const void *tlv_data, - u32 userid) + const struct hal_tlv_64_hdr *tlv) { struct ath12k_base *ab = ar->ab; struct hal_rx_mon_ppdu_info *ppdu_info = &pmon->mon_ppdu_info; - u32 info[7]; + const void *tlv_data = tlv->value; + u32 info[7], userid; + u16 tlv_tag; + + tlv_tag = le64_get_bits(tlv->tl, HAL_TLV_64_HDR_TAG); + userid = le64_get_bits(tlv->tl, HAL_TLV_64_USR_ID); switch (tlv_tag) { case HAL_RX_PPDU_START: { @@ -1187,7 +1191,6 @@ ath12k_dp_mon_parse_rx_dest(struct ath12k *ar, struct ath12k_mon_data *pmon, struct hal_rx_mon_ppdu_info *ppdu_info = &pmon->mon_ppdu_info; struct hal_tlv_64_hdr *tlv; enum hal_rx_mon_status hal_status; - u32 tlv_userid; u16 tlv_tag, tlv_len; u8 *ptr = skb->data; @@ -1196,9 +1199,6 @@ ath12k_dp_mon_parse_rx_dest(struct ath12k *ar, struct ath12k_mon_data *pmon, do { tlv = (struct hal_tlv_64_hdr *)ptr; tlv_tag = le64_get_bits(tlv->tl, HAL_TLV_64_HDR_TAG); - tlv_len = le64_get_bits(tlv->tl, HAL_TLV_64_HDR_LEN); - tlv_userid = le64_get_bits(tlv->tl, HAL_TLV_64_USR_ID); - ptr += sizeof(*tlv); /* The actual length of PPDU_END is the combined length of many PHY * TLVs that follow. Skip the TLV header and @@ -1208,10 +1208,11 @@ ath12k_dp_mon_parse_rx_dest(struct ath12k *ar, struct ath12k_mon_data *pmon, if (tlv_tag == HAL_RX_PPDU_END) tlv_len = sizeof(struct hal_rx_rxpcu_classification_overview); + else + tlv_len = le64_get_bits(tlv->tl, HAL_TLV_64_HDR_LEN); - hal_status = ath12k_dp_mon_rx_parse_status_tlv(ar, pmon, - tlv_tag, ptr, tlv_userid); - ptr += tlv_len; + hal_status = ath12k_dp_mon_rx_parse_status_tlv(ar, pmon, tlv); + ptr += sizeof(*tlv) + tlv_len; ptr = PTR_ALIGN(ptr, HAL_TLV_64_ALIGN); if ((ptr - skb->data) >= DP_RX_BUFFER_SIZE)
Currently, ath12k_dp_mon_rx_parse_status_tlv() takes the TLV tag, TLV data and TLV userid as separate arguments from the caller. These argument can be fetched from the TLV header itself. Therefore, pass the TLV header and retrieve the necessary fields from the header instead of passing them from the caller. Tested-on: QCN9274 hw2.0 PCI WLAN.WBE.1.3.1-00173-QCAHKSWPL_SILICONZ-1 Tested-on: WCN7850 hw2.0 PCI WLAN.HMT.1.0.c5-00481-QCAHMTSWPL_V1.0_V2.0_SILICONZ-3 Signed-off-by: Karthikeyan Periyasamy <quic_periyasa@quicinc.com> --- drivers/net/wireless/ath/ath12k/dp_mon.c | 21 +++++++++++---------- 1 file changed, 11 insertions(+), 10 deletions(-)