mbox series

[v2,0/6] wifi: rtw89: handle EHT rate

Message ID 20231010021006.6061-1-pkshih@realtek.com
Headers show
Series wifi: rtw89: handle EHT rate | expand

Message

Ping-Ke Shih Oct. 10, 2023, 2:10 a.m. UTC
We have parsed new formats used by WiFi 7 chips before, and this patchset
is to handle EHT rate. The handlers contain
 - RX rate
   * from RX descriptor, which is also used to find corresponding PPDU
     status packet
 - TX rate
   * RA (Rate adaptive) mask to tell firmware selectable rates
   * RA report tells driver the rate selected by firmware
 - debugfs
   * show TX/RX rate above
 - monitor mode
   * add enumerators of radiotap bandwidth of EHT U-SIG
   * add EHT radiotap if working on monitor mode

v2:
  - correct commit message of patch 5/6 about the source of bandwidth
    definition of EHT U-SIG suggested by Johannes

Ping-Ke Shih (6):
  wifi: rtw89: parse EHT information from RX descriptor and PPDU status
    packet
  wifi: rtw89: Add EHT rate mask as parameters of RA H2C command
  wifi: rtw89: parse TX EHT rate selected by firmware from RA C2H report
  wifi: rtw89: show EHT rate in debugfs
  wifi: radiotap: add bandwidth definition of EHT U-SIG
  wifi: rtw89: add EHT radiotap in monitor mode

 drivers/net/wireless/realtek/rtw89/core.c  | 125 ++++++++++++++++++---
 drivers/net/wireless/realtek/rtw89/core.h  |  11 +-
 drivers/net/wireless/realtek/rtw89/debug.c |  14 +++
 drivers/net/wireless/realtek/rtw89/phy.c   |  70 +++++++++++-
 drivers/net/wireless/realtek/rtw89/phy.h   |   5 +
 include/net/ieee80211_radiotap.h           |   6 +
 6 files changed, 214 insertions(+), 17 deletions(-)

Comments

Kalle Valo Oct. 11, 2023, 9:13 a.m. UTC | #1
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?

> +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];
Ping-Ke Shih Oct. 11, 2023, 9:19 a.m. UTC | #2
> -----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.