Re: [PATCH v9 2/9] wifi: ath12k: Add HAL_PHYRX_GENERIC_EHT_SIG TLV parsing support

[Karthikeyan Periyasamy <quic_periyasa@xxxxxxxxxxx>]

On 2/10/2025 4:23 PM, Mahendran P wrote:
> On 2/6/2025 7:08 AM, Karthikeyan Periyasamy wrote:
> > Currently, monitor is not enabled. However, in the future, the monitor
> > will be enabled. Therefore, add the necessary HAL_PHYRX_GENERIC_EHT_SIG
> > TLV parsing support in monitor Rx path, which help to populate the EHT
> > radiotap data.
> >
> > 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
> >
> > Co-developed-by: P Praneesh <quic_ppranees@xxxxxxxxxxx>
> > Signed-off-by: P Praneesh <quic_ppranees@xxxxxxxxxxx>
> > Reviewed-by: Vasanthakumar Thiagarajan <vasanthakumar.thiagarajan@xxxxxxxxxxxxxxxx>
> > Signed-off-by: Karthikeyan Periyasamy <quic_periyasa@xxxxxxxxxxx>
> > ---
> >   drivers/net/wireless/ath/ath12k/dp_mon.c | 548 +++++++++++++++++++++--
> >   drivers/net/wireless/ath/ath12k/hal_rx.h | 136 +++++-
> >   2 files changed, 638 insertions(+), 46 deletions(-)
> >
> > diff --git a/drivers/net/wireless/ath/ath12k/dp_mon.c b/drivers/net/wireless/ath/ath12k/dp_mon.c
> > index 61fd481b7a3b..dd17607d470d 100644
> > --- a/drivers/net/wireless/ath/ath12k/dp_mon.c
> > +++ b/drivers/net/wireless/ath/ath12k/dp_mon.c
> > @@ -13,6 +13,9 @@
> >   #define ATH12K_LE32_DEC_ENC(value, dec_bits, enc_bits)	\
> >   		u32_encode_bits(le32_get_bits(value, dec_bits), enc_bits)
> >   
> > +#define ATH12K_LE64_DEC_ENC(value, dec_bits, enc_bits) \
> > +		u32_encode_bits(le64_get_bits(value, dec_bits), enc_bits)
>
> u64_encode_bits?

No, Its not needed. Here we need to decode from the u64 and get assign 
to u32 as defined by eht radiotap header.


> > @@ -836,6 +1239,9 @@ ath12k_dp_mon_rx_parse_status_tlv(struct ath12k *ar,
> >   		case HAL_RX_PREAMBLE_11AX:
> >   			ppdu_info->he_flags = 1;
> >   			break;
> > +		case HAL_RX_PREAMBLE_11BE:
> > +			ppdu_info->is_eht = true;
> > +			break;
> >   		default:
> >   			break;
> >   		}
> > @@ -961,6 +1367,21 @@ ath12k_dp_mon_rx_parse_status_tlv(struct ath12k *ar,
> >   	case HAL_PHYRX_GENERIC_U_SIG:
> >   		ath12k_dp_mon_hal_rx_parse_u_sig_hdr(tlv_data, ppdu_info);
> >   		break;
> > +	case HAL_PHYRX_GENERIC_EHT_SIG:
> > +		/* Handle the case where aggregation is in progress
> > +		 * or the current TLV is one of the TLVs which should be
> > +		 * aggregated
> > +		 */
> > +		if (!ppdu_info->tlv_aggr.in_progress) {
> > +			ppdu_info->tlv_aggr.in_progress = true;
> > +			ppdu_info->tlv_aggr.tlv_tag = tlv_tag;
> > +			ppdu_info->tlv_aggr.cur_len = 0;
> > +		}
> > +
> > +		ppdu_info->is_eht = true;
> > +
> > +		ath12k_dp_mon_hal_aggr_tlv(ppdu_info, tlv_len, tlv_data);
> > +		break;
> >   	case HAL_DUMMY:
> >   		return HAL_RX_MON_STATUS_BUF_DONE;
> >   	case HAL_RX_PPDU_END_STATUS_DONE:
> > @@ -1158,22 +1579,59 @@ static void ath12k_dp_mon_update_radiotap(struct ath12k *ar,
> >   		rxs->ampdu_reference = ampdu_id;
> >   	}
> >   
> > -	if (ppduinfo->eht_usig) {
> > +	if (ppduinfo->is_eht || ppduinfo->eht_usig) {
> >   		struct ieee80211_radiotap_tlv *tlv;
> > +		struct ieee80211_radiotap_eht *eht;
> >   		struct ieee80211_radiotap_eht_usig *usig;
> > -		u16 len = sizeof(*usig);
> > +		u16 len = 0, i, eht_len, usig_len;
> > +		u8 user;
> > +
> > +		if (ppduinfo->is_eht) {
> > +			eht_len = struct_size(eht,
> > +					      user_info,
> > +					      ppduinfo->eht_info.num_user_info);
> > +			len += sizeof(*tlv) + eht_len;
> > +		}
> > +
> > +		if (ppduinfo->eht_usig) {
> > +			usig_len = sizeof(*usig);
> > +			len += sizeof(*tlv) + usig_len;
> > +		}
> >   
> >   		rxs->flag |= RX_FLAG_RADIOTAP_TLV_AT_END;
> >   		rxs->encoding = RX_ENC_EHT;
> >   
> >   		skb_reset_mac_header(mon_skb);
> >   
> > -		tlv = skb_push(mon_skb, sizeof(*tlv) + len);
> > -		tlv->type = cpu_to_le16(IEEE80211_RADIOTAP_EHT_USIG);
> > -		tlv->len = cpu_to_le16(len);
> > +		tlv = skb_push(mon_skb, len);
> > +
> > +		if (ppduinfo->is_eht) {
> > +			tlv->type = cpu_to_le16(IEEE80211_RADIOTAP_EHT);
> > +			tlv->len = cpu_to_le16(eht_len);
> >   
> > -		usig = (struct ieee80211_radiotap_eht_usig *)tlv->data;
> > -		*usig = ppduinfo->usig;
> > +			eht = (struct ieee80211_radiotap_eht *)tlv->data;
> > +			eht->known = ppduinfo->eht_info.eht.known;
> > +
> > +			for (i = 0;
> > +			     i < ARRAY_SIZE(eht->data) &&
> > +			     i < ARRAY_SIZE(ppduinfo->eht_info.eht.data);
> > +			     i++)
> > +				eht->data[i] = ppduinfo->eht_info.eht.data[i];
> > +
> > +			for (user = 0; user < ppduinfo->eht_info.num_user_info; user++)
> > +				put_unaligned_le32(ppduinfo->eht_info.user_info[user],
> > +						   &eht->user_info[user]);
> > +
> > +			tlv = (struct ieee80211_radiotap_tlv *)&tlv->data[eht_len];
> > +		}
> > +
> > +		if (ppduinfo->eht_usig) {
> > +			tlv->type = cpu_to_le16(IEEE80211_RADIOTAP_EHT_USIG);
> > +			tlv->len = cpu_to_le16(usig_len);
> > +
> > +			usig = (struct ieee80211_radiotap_eht_usig *)tlv->data;
> > +			*usig = ppduinfo->u_sig_info.usig;
> > +		}
> >   	} else if (ppduinfo->he_mu_flags) {
> >   		rxs->flag |= RX_FLAG_RADIOTAP_HE_MU;
> >   		rxs->encoding = RX_ENC_HE;
> > diff --git a/drivers/net/wireless/ath/ath12k/hal_rx.h b/drivers/net/wireless/ath/ath12k/hal_rx.h
> > index 2da16f27e76c..959f2283294c 100644
> > --- a/drivers/net/wireless/ath/ath12k/hal_rx.h
> > +++ b/drivers/net/wireless/ath/ath12k/hal_rx.h
> > @@ -71,6 +71,8 @@ enum hal_rx_preamble {
> >   	HAL_RX_PREAMBLE_11N,
> >   	HAL_RX_PREAMBLE_11AC,
> >   	HAL_RX_PREAMBLE_11AX,
> > +	HAL_RX_PREAMBLE_11BA,
>
> is it needed?

It will be good to have the defined values in the enum instead of 
initialise the HAL_RX_PREAMBLE_11BE = 6 directly.

> > +	HAL_RX_PREAMBLE_11BE,
> >   	HAL_RX_PREAMBLE_MAX,
> >   };
> >   

--
Karthikeyan Periyasamy
--
கார்த்திகேயன் பெரியசாமி