| Message ID | 1644914581-21682-2-git-send-email-quic_ramess@quicinc.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Johannes Berg |
| Headers | show |
| Series | Add support to configure 6GHz non-ht duplicate transmission | expand |
Hi, Couple of notes below: > @@ -704,6 +704,7 @@ struct ieee80211_bss_conf { > struct cfg80211_he_bss_color he_bss_color; > struct ieee80211_fils_discovery fils_discovery; > u32 unsol_bcast_probe_resp_interval; > + bool he_6g_nonht_dup_beacon_set; This is missing documentation. > + cap = cfg80211_find_ext_elem(WLAN_EID_EXT_HE_OPERATION, > + params->tail, params->tail_len); > + if (cap && cap->datalen >= sizeof(*he_oper) + 1) { > + he_oper = (void *)(cap->data + 1); > + he_6ghz_oper = ieee80211_he_6ghz_oper(he_oper); > + if (he_6ghz_oper) { > + sdata->vif.bss_conf.he_6g_nonht_dup_beacon_set = false; > + if (u8_get_bits(he_6ghz_oper->control, > + IEEE80211_HE_6GHZ_OPER_CTRL_DUP_BEACON)) { > + sdata->vif.bss_conf.he_6g_nonht_dup_beacon_set = true; > + } no braces needed there, and no u8_get_bits() either, you can just & ? > + } > + } I am wondering though if this should even be detected from the HE operation element itself, rather than from the beacon rate settings that are separate in nl80211? johannes
> -----Original Message----- > From: Johannes Berg <johannes@sipsolutions.net> > Sent: Friday, March 11, 2022 4:33 PM > To: Rameshkumar Sundaram (QUIC) <quic_ramess@quicinc.com> > Cc: linux-wireless@vger.kernel.org; ath11k@lists.infradead.org > Subject: Re: [PATCH 1/2] mac80211: add support to configure 6GHz non-ht > duplicate transmission > > Hi, > > Couple of notes below: > > > @@ -704,6 +704,7 @@ struct ieee80211_bss_conf { > > struct cfg80211_he_bss_color he_bss_color; > > struct ieee80211_fils_discovery fils_discovery; > > u32 unsol_bcast_probe_resp_interval; > > + bool he_6g_nonht_dup_beacon_set; > > This is missing documentation. Thanks, will add it in next patch > > > + cap = cfg80211_find_ext_elem(WLAN_EID_EXT_HE_OPERATION, > > + params->tail, params->tail_len); > > + if (cap && cap->datalen >= sizeof(*he_oper) + 1) { > > + he_oper = (void *)(cap->data + 1); > > + he_6ghz_oper = ieee80211_he_6ghz_oper(he_oper); > > + if (he_6ghz_oper) { > > + sdata->vif.bss_conf.he_6g_nonht_dup_beacon_set = > false; > > + if (u8_get_bits(he_6ghz_oper->control, > > + > IEEE80211_HE_6GHZ_OPER_CTRL_DUP_BEACON)) { > > + sdata- > >vif.bss_conf.he_6g_nonht_dup_beacon_set = true; > > + } > > no braces needed there, and no u8_get_bits() either, you can just & ? Sure, I remember I got sparse warnings last time without this (also took reference from net/mac80211/util.c ieee80211_chandef_he_6ghz_oper()), will send v2 with 'just &' anyway. > > > + } > > + } > > I am wondering though if this should even be detected from the HE > operation element itself, rather than from the beacon rate settings that are > separate in nl80211? This is a BW dependent bit in HE Oper IE and user space can switch BW (CSA/chan switch scenarios). Which can call assign_beacon directly, Hence adding the logic here to cover all Beacon change cases. > > johannes
On Mon, 2022-03-21 at 05:12 +0000, Rameshkumar Sundaram (QUIC) wrote: > > > > > I am wondering though if this should even be detected from the HE > > operation element itself, rather than from the beacon rate settings > > that are > > separate in nl80211? > This is a BW dependent bit in HE Oper IE and user space can switch BW > (CSA/chan switch scenarios). > Which can call assign_beacon directly, Hence adding the logic here to > cover all Beacon change cases. > Yeah but still ... why do we take it from the content rather than adding a control for it? johannes
> -----Original Message----- > From: Johannes Berg <johannes@sipsolutions.net> > Sent: Monday, March 21, 2022 2:28 PM > To: Rameshkumar Sundaram (QUIC) <quic_ramess@quicinc.com> > Cc: linux-wireless@vger.kernel.org; ath11k@lists.infradead.org > Subject: Re: [PATCH 1/2] mac80211: add support to configure 6GHz non-ht > duplicate transmission > > On Mon, 2022-03-21 at 05:12 +0000, Rameshkumar Sundaram (QUIC) wrote: > > > > > > > > I am wondering though if this should even be detected from the HE > > > operation element itself, rather than from the beacon rate settings > > > that are separate in nl80211? > > This is a BW dependent bit in HE Oper IE and user space can switch BW > > (CSA/chan switch scenarios). > > Which can call assign_beacon directly, Hence adding the logic here to > > cover all Beacon change cases. > > > > Yeah but still ... why do we take it from the content rather than adding a > control for it? So, Suggestion here is to add a new NL attribute to carry the information from user space for START_AP and SET_BEACON NL commands and use it here, is that understanding correct ? > > johannes
On Tue, 2022-03-22 at 13:50 +0000, Rameshkumar Sundaram (QUIC) wrote: > > > So, Suggestion here is to add a new NL attribute to carry the > information from user space for > START_AP and SET_BEACON NL commands and use it here, is that > understanding correct ? > Well, I was thinking it would reasonably come with the beacon rate, i.e. as a new attribute like NL80211_TXRATE_DUP? johannes
> -----Original Message----- > From: Johannes Berg <johannes@sipsolutions.net> > Sent: Tuesday, March 22, 2022 8:34 PM > To: Rameshkumar Sundaram (QUIC) <quic_ramess@quicinc.com> > Cc: linux-wireless@vger.kernel.org; ath11k@lists.infradead.org > Subject: Re: [PATCH 1/2] mac80211: add support to configure 6GHz non-ht > duplicate transmission > > WARNING: This email originated from outside of Qualcomm. Please be wary > of any links or attachments, and do not enable macros. > > On Tue, 2022-03-22 at 13:50 +0000, Rameshkumar Sundaram (QUIC) wrote: > > > > > So, Suggestion here is to add a new NL attribute to carry the > > information from user space for START_AP and SET_BEACON NL commands > > and use it here, is that understanding correct ? > > > > Well, I was thinking it would reasonably come with the beacon rate, i.e. > as a new attribute like NL80211_TXRATE_DUP? Sorry not getting this, you mean hostapd would need to send new set of rates for this configuration ? The expectation is that user-space will set this bit in HE oper IE only if legacy rates are present in beacon or basic rates. > > johannes
On Sun, 2022-04-17 at 14:30 +0000, Rameshkumar Sundaram (QUIC) wrote: > > > > Well, I was thinking it would reasonably come with the beacon rate, > > i.e. > > as a new attribute like NL80211_TXRATE_DUP? > Sorry not getting this, you mean hostapd would need to send new set of > rates for this configuration ? > The expectation is that user-space will set this bit in HE oper IE > only if legacy rates are present in beacon or basic rates. > ? I don't understand. The point is - you're setting the bit in the beacon that indicates "I'm transmitting it as duplicate", right? But you're also using that bit to actually configure it to do (non-HT) duplicate transmissions. But fundamentally, those two things aren't really related, are they? Why shouldn't you be - at least theoretically - be able to configure it for duplicate transmission (or not) regardless of the bit in the beacon? To me it seems "do duplicate transmission" is a property of the *rate* you want to use to transmit, not really necessarily a property of the *content* of the frame you're transmitting. johannes
diff --git a/include/net/mac80211.h b/include/net/mac80211.h index c50221d..a4b1efb 100644 --- a/include/net/mac80211.h +++ b/include/net/mac80211.h @@ -704,6 +704,7 @@ struct ieee80211_bss_conf { struct cfg80211_he_bss_color he_bss_color; struct ieee80211_fils_discovery fils_discovery; u32 unsol_bcast_probe_resp_interval; + bool he_6g_nonht_dup_beacon_set; bool s1g; struct cfg80211_bitrate_mask beacon_tx_rate; enum ieee80211_ap_reg_power power_type; diff --git a/net/mac80211/cfg.c b/net/mac80211/cfg.c index 87a2080..9b6f55e 100644 --- a/net/mac80211/cfg.c +++ b/net/mac80211/cfg.c @@ -997,6 +997,9 @@ static int ieee80211_assign_beacon(struct ieee80211_sub_if_data *sdata, struct beacon_data *new, *old; int new_head_len, new_tail_len; int size, err; + const struct element *cap; + struct ieee80211_he_operation *he_oper; + const struct ieee80211_he_6ghz_oper *he_6ghz_oper; u32 changed = BSS_CHANGED_BEACON; old = sdata_dereference(sdata->u.ap.beacon, sdata); @@ -1084,6 +1087,20 @@ static int ieee80211_assign_beacon(struct ieee80211_sub_if_data *sdata, changed |= BSS_CHANGED_FTM_RESPONDER; } + cap = cfg80211_find_ext_elem(WLAN_EID_EXT_HE_OPERATION, + params->tail, params->tail_len); + if (cap && cap->datalen >= sizeof(*he_oper) + 1) { + he_oper = (void *)(cap->data + 1); + he_6ghz_oper = ieee80211_he_6ghz_oper(he_oper); + if (he_6ghz_oper) { + sdata->vif.bss_conf.he_6g_nonht_dup_beacon_set = false; + if (u8_get_bits(he_6ghz_oper->control, + IEEE80211_HE_6GHZ_OPER_CTRL_DUP_BEACON)) { + sdata->vif.bss_conf.he_6g_nonht_dup_beacon_set = true; + } + } + } + rcu_assign_pointer(sdata->u.ap.beacon, new); if (old)
A 6GHz AP can decide to transmit Beacon, Broadcast probe response and FILS discovery frames in a non-HT duplicate PPDU when operating in non 20MHz Bandwidth to enhance its discoverability. (IEEE Std 802.11ax‐2021-26.17.2.2) Add changes to configure 6GHz non-HT duplicate beacon transmission based on Duplicate Beacon subfield of 6GHz Operation Information field of the HE Operation element in Beacon. Signed-off-by: Rameshkumar Sundaram <quic_ramess@quicinc.com> --- include/net/mac80211.h | 1 + net/mac80211/cfg.c | 17 +++++++++++++++++ 2 files changed, 18 insertions(+)