Received-SPF: pass (google.com: domain of linux-wireless-owner@vger.kernel.org designates 23.128.96.36 as permitted sender) client-ip=23.128.96.36;
From:   Ping-Ke Shih <pkshih@realtek.com>
To:     Kalle Valo <kvalo@kernel.org>
CC:     "johannes@sipsolutions.net" <johannes@sipsolutions.net>,
        "linux-wireless@vger.kernel.org" <linux-wireless@vger.kernel.org>
Subject: RE: [PATCH v2 6/6] wifi: rtw89: add EHT radiotap in monitor mode
Thread-Topic: [PATCH v2 6/6] wifi: rtw89: add EHT radiotap in monitor mode
Thread-Index: AQHZ+x8C65IQAoUJ7kiM6zWajLzKz7BEUELtgAAAk+A=
Date:   Wed, 11 Oct 2023 09:19:24 +0000
Message-ID: <167371414b9a42dbbfbdf3268fcc7991@realtek.com>
References: <20231010021006.6061-1-pkshih@realtek.com>
        <20231010021006.6061-7-pkshih@realtek.com> <87il7d4j3u.fsf@kernel.org>
In-Reply-To: <87il7d4j3u.fsf@kernel.org>
Accept-Language: en-US, zh-TW
Content-Language: zh-TW
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 8BIT
MIME-Version: 1.0
Precedence: bulk


> -----Original Message-----
> From: Kalle Valo <kvalo@kernel.org>
> Sent: Wednesday, October 11, 2023 5:14 PM
> To: Ping-Ke Shih <pkshih@realtek.com>
> Cc: johannes@sipsolutions.net; linux-wireless@vger.kernel.org
> Subject: Re: [PATCH v2 6/6] wifi: rtw89: add EHT radiotap in monitor mode
> 
> Ping-Ke Shih <pkshih@realtek.com> 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@realtek.com>
> > ---
> >  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.