> -----Original Message-----
> From: Kalle Valo <kvalo@xxxxxxxxxx>
> Sent: Wednesday, October 11, 2023 5:14 PM
> To: Ping-Ke Shih <pkshih@xxxxxxxxxxx>
> Cc: johannes@xxxxxxxxxxxxxxxx; linux-wireless@xxxxxxxxxxxxxxx
> Subject: Re: [PATCH v2 6/6] wifi: rtw89: add EHT radiotap in monitor mode
>
> Ping-Ke Shih <pkshih@xxxxxxxxxxx> writes:
>
> > Add IEEE80211_RADIOTAP_EHT and IEEE80211_RADIOTAP_EHT_USIG radiotap to
> > fill basic EHT NSS, MCS, GI and bandwidth.
> >
> > Signed-off-by: Ping-Ke Shih <pkshih@xxxxxxxxxxx>
> > ---
> > drivers/net/wireless/realtek/rtw89/core.c | 66 +++++++++++++++++++++++
> > drivers/net/wireless/realtek/rtw89/core.h | 9 +++-
> > 2 files changed, 74 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/net/wireless/realtek/rtw89/core.c b/drivers/net/wireless/realtek/rtw89/core.c
> > index 2742e6646cf1..8cb1715d049a 100644
> > --- a/drivers/net/wireless/realtek/rtw89/core.c
> > +++ b/drivers/net/wireless/realtek/rtw89/core.c
> > @@ -1907,6 +1907,70 @@ static void rtw89_core_hw_to_sband_rate(struct ieee80211_rx_status *rx_status)
> > rx_status->rate_idx -= 4;
> > }
> >
> > +static u8 rx_status_bw_to_radiotap_eht_usig[] = {
> > + [RATE_INFO_BW_20] = IEEE80211_RADIOTAP_EHT_USIG_COMMON_BW_20MHZ,
> > + [RATE_INFO_BW_5] = U8_MAX,
> > + [RATE_INFO_BW_10] = U8_MAX,
> > + [RATE_INFO_BW_40] = IEEE80211_RADIOTAP_EHT_USIG_COMMON_BW_40MHZ,
> > + [RATE_INFO_BW_80] = IEEE80211_RADIOTAP_EHT_USIG_COMMON_BW_80MHZ,
> > + [RATE_INFO_BW_160] = IEEE80211_RADIOTAP_EHT_USIG_COMMON_BW_160MHZ,
> > + [RATE_INFO_BW_HE_RU] = U8_MAX,
> > + [RATE_INFO_BW_320] = IEEE80211_RADIOTAP_EHT_USIG_COMMON_BW_320MHZ_1,
> > + [RATE_INFO_BW_EHT_RU] = U8_MAX,
> > +};
>
> Sorry, I noticed this only when I was abot to commit this. Should this
> be static const?
Yes, I miss it.
>
> > +static void rtw89_core_update_radiotap_eht(struct rtw89_dev *rtwdev,
> > + struct sk_buff *skb,
> > + struct ieee80211_rx_status *rx_status)
> > +{
> > + struct ieee80211_radiotap_eht_usig *usig;
> > + struct ieee80211_radiotap_eht *eht;
> > + struct ieee80211_radiotap_tlv *tlv;
> > + int eht_len = struct_size(eht, user_info, 1);
> > + int usig_len = sizeof(*usig);
> > + int len;
> > + u8 bw;
> > +
> > + len = sizeof(*tlv) + ALIGN(eht_len, 4) +
> > + sizeof(*tlv) + ALIGN(usig_len, 4);
> > +
> > + rx_status->flag |= RX_FLAG_RADIOTAP_TLV_AT_END;
> > + skb_reset_mac_header(skb);
> > +
> > + /* EHT */
> > + tlv = skb_push(skb, len);
> > + memset(tlv, 0, len);
> > + tlv->type = cpu_to_le16(IEEE80211_RADIOTAP_EHT);
> > + tlv->len = cpu_to_le16(eht_len);
> > +
> > + eht = (struct ieee80211_radiotap_eht *)tlv->data;
> > + eht->known = cpu_to_le32(IEEE80211_RADIOTAP_EHT_KNOWN_GI);
> > + eht->data[0] =
> > + le32_encode_bits(rx_status->eht.gi, IEEE80211_RADIOTAP_EHT_DATA0_GI);
> > +
> > + eht->user_info[0] =
> > + cpu_to_le32(IEEE80211_RADIOTAP_EHT_USER_INFO_MCS_KNOWN |
> > + IEEE80211_RADIOTAP_EHT_USER_INFO_NSS_KNOWN_O);
> > + eht->user_info[0] |=
> > + le32_encode_bits(rx_status->rate_idx, IEEE80211_RADIOTAP_EHT_USER_INFO_MCS) |
> > + le32_encode_bits(rx_status->nss, IEEE80211_RADIOTAP_EHT_USER_INFO_NSS_O);
> > +
> > + /* U-SIG */
> > + tlv = (void *)tlv + sizeof(*tlv) + ALIGN(eht_len, 4);
> > + tlv->type = cpu_to_le16(IEEE80211_RADIOTAP_EHT_USIG);
> > + tlv->len = cpu_to_le16(usig_len);
> > +
> > + bw = rx_status->bw < ARRAY_SIZE(rx_status_bw_to_radiotap_eht_usig) ?
> > + rx_status_bw_to_radiotap_eht_usig[rx_status->bw] : U8_MAX;
> > + if (bw == U8_MAX)
> > + return;
>
> This is cosmetics but I feel that 'if' statement is more readable than
> ':' operator:
>
> if (rx_status->bw >= ARRAY_SIZE(rx_status_bw_to_radiotap_eht_usig)
> return;
>
> bw = rx_status_bw_to_radiotap_eht_usig[rx_status->bw];
>
Got it. I will prepare v3 for them.
Thank you.
