| Message ID | 20221226083328.29051-2-quic_ramess@quicinc.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Johannes Berg |
| Headers | show |
| Series | wifi: Add support to enable/disable bss color collision detection | expand |
On Mon, 2022-12-26 at 14:03 +0530, Rameshkumar Sundaram wrote: > As per 802.11ax-2021, STAs shall process BSS Color Change Announcement > (BCCA) from AP and switch to new color, but some STAs aren't processing > BCCA from AP and not doing color switch, causing them to drop data > frames from AP post color change. > > Provide an option to disable color collision detection and therefore > not to do BCCA to mitigate the same from AP. If it's required in case > where STA supports BCCA handling, then it can enabled in AP using this > option. > You should probably split this into cfg80211 and mac80211. Also, this doesn't really seem to make a lot of _sense_ since nothing in the kernel actually acts on detection of a color collision - hostapd is acting on that. So since you can easily make hostapd ignore the event, why do you even need this? johannes
On 19/01/2023 15:02, Johannes Berg wrote: > On Mon, 2022-12-26 at 14:03 +0530, Rameshkumar Sundaram wrote: >> As per 802.11ax-2021, STAs shall process BSS Color Change Announcement >> (BCCA) from AP and switch to new color, but some STAs aren't processing >> BCCA from AP and not doing color switch, causing them to drop data >> frames from AP post color change. >> >> Provide an option to disable color collision detection and therefore >> not to do BCCA to mitigate the same from AP. If it's required in case >> where STA supports BCCA handling, then it can enabled in AP using this >> option. >> > > You should probably split this into cfg80211 and mac80211. > > Also, this doesn't really seem to make a lot of _sense_ since nothing in > the kernel actually acts on detection of a color collision - hostapd is > acting on that. > > So since you can easily make hostapd ignore the event, why do you even > need this? This may not be related, but the software color collision detection sends a netlink message for every colliding frame and it can hose up the system if the other network is very active. Also, cfg80211_bss_color_notify() complains that the wdev lock isn't held.
On Thu, 2023-01-19 at 15:52 +0100, Nicolas Cavallari wrote: > > This may not be related, but the software color collision detection > sends a netlink message for every colliding frame and it can hose up the > system if the other network is very active. > > Also, cfg80211_bss_color_notify() complains that the wdev lock isn't held. Yay. Lorenzo can you take a look at that? johannes
> On Thu, 2023-01-19 at 15:52 +0100, Nicolas Cavallari wrote: > > > > This may not be related, but the software color collision detection > > sends a netlink message for every colliding frame and it can hose up the > > system if the other network is very active. > > > > Also, cfg80211_bss_color_notify() complains that the wdev lock isn't held. > > Yay. > > Lorenzo can you take a look at that? It seems fine to me, I am just wondering if we need to forward the info to the hw (if it supports hw color collision detection) if we just flip collision_detection_enabled field in ieee80211_change_beacon(). What do you think? Regards, Lorenzo > > johannes
> On 19/01/2023 15:02, Johannes Berg wrote: > > On Mon, 2022-12-26 at 14:03 +0530, Rameshkumar Sundaram wrote: > > > As per 802.11ax-2021, STAs shall process BSS Color Change Announcement > > > (BCCA) from AP and switch to new color, but some STAs aren't processing > > > BCCA from AP and not doing color switch, causing them to drop data > > > frames from AP post color change. > > > > > > Provide an option to disable color collision detection and therefore > > > not to do BCCA to mitigate the same from AP. If it's required in case > > > where STA supports BCCA handling, then it can enabled in AP using this > > > option. > > > > > > > You should probably split this into cfg80211 and mac80211. > > > > Also, this doesn't really seem to make a lot of _sense_ since nothing in > > the kernel actually acts on detection of a color collision - hostapd is > > acting on that. > > > > So since you can easily make hostapd ignore the event, why do you even > > need this? > > This may not be related, but the software color collision detection sends a > netlink message for every colliding frame and it can hose up the system if > the other network is very active. > > Also, cfg80211_bss_color_notify() complains that the wdev lock isn't held. Hi Nicolas, I agree, I think we can ratelimit netlink messages sent by the kernel to userspace (e.g. to hostapd), I would say every 500ms is ok. I guess we can move cfg80211_obss_color_collision_notify() in a dedicated delayed_work so we can grab wdev mutex (cfg80211_obss_color_collision_notify is currently running in interrupt context). To give an idea, what do you think about patch below? (please note it is just compiled tested so far). Regards, Lorenzo diff --git a/net/mac80211/cfg.c b/net/mac80211/cfg.c index f5d43f42f6d8..0aefaca989aa 100644 --- a/net/mac80211/cfg.c +++ b/net/mac80211/cfg.c @@ -4652,6 +4652,20 @@ void ieee80211_color_change_finalize_work(struct work_struct *work) sdata_unlock(sdata); } +void ieee80211_color_collision_detection_work(struct work_struct *work) +{ + struct delayed_work *delayed_work = to_delayed_work(work); + struct ieee80211_link_data *link = + container_of(delayed_work, struct ieee80211_link_data, + dfs_cac_timer_work); + struct ieee80211_sub_if_data *sdata = link->sdata; + + sdata_lock(sdata); + cfg80211_obss_color_collision_notify(sdata->dev, link->color_bitmap, + GFP_KERNEL); + sdata_unlock(sdata); +} + void ieee80211_color_change_finish(struct ieee80211_vif *vif) { struct ieee80211_sub_if_data *sdata = vif_to_sdata(vif); @@ -4666,11 +4680,21 @@ ieee80211_obss_color_collision_notify(struct ieee80211_vif *vif, u64 color_bitmap, gfp_t gfp) { struct ieee80211_sub_if_data *sdata = vif_to_sdata(vif); + struct ieee80211_link_data *link = &sdata->deflink; if (sdata->vif.bss_conf.color_change_active || sdata->vif.bss_conf.csa_active) return; - cfg80211_obss_color_collision_notify(sdata->dev, color_bitmap, gfp); + if (delayed_work_pending(&link->color_collision_detect_work)) + return; + + link->color_bitmap = color_bitmap; + /* queue the color collision detection event every 500 ms in order to + * avoid sending too much netlink messages to userspace. + */ + ieee80211_queue_delayed_work(&sdata->local->hw, + &link->color_collision_detect_work, + msecs_to_jiffies(500)); /* 500 ms */ } EXPORT_SYMBOL_GPL(ieee80211_obss_color_collision_notify); diff --git a/net/mac80211/ieee80211_i.h b/net/mac80211/ieee80211_i.h index d16606e84e22..7ca9bde3c6d2 100644 --- a/net/mac80211/ieee80211_i.h +++ b/net/mac80211/ieee80211_i.h @@ -974,6 +974,8 @@ struct ieee80211_link_data { struct cfg80211_chan_def csa_chandef; struct work_struct color_change_finalize_work; + struct delayed_work color_collision_detect_work; + u64 color_bitmap; /* context reservation -- protected with chanctx_mtx */ struct ieee80211_chanctx *reserved_chanctx; @@ -1929,6 +1931,7 @@ int ieee80211_channel_switch(struct wiphy *wiphy, struct net_device *dev, /* color change handling */ void ieee80211_color_change_finalize_work(struct work_struct *work); +void ieee80211_color_collision_detection_work(struct work_struct *work); /* interface handling */ #define MAC80211_SUPPORTED_FEATURES_TX (NETIF_F_IP_CSUM | NETIF_F_IPV6_CSUM | \ diff --git a/net/mac80211/iface.c b/net/mac80211/iface.c index 23ed13f15067..1ef970b457d1 100644 --- a/net/mac80211/iface.c +++ b/net/mac80211/iface.c @@ -541,6 +541,7 @@ static void ieee80211_do_stop(struct ieee80211_sub_if_data *sdata, bool going_do cancel_work_sync(&sdata->deflink.color_change_finalize_work); cancel_delayed_work_sync(&sdata->deflink.dfs_cac_timer_work); + cancel_delayed_work_sync(&sdata->deflink.color_collision_detect_work); if (sdata->wdev.cac_started) { chandef = sdata->vif.bss_conf.chandef; diff --git a/net/mac80211/link.c b/net/mac80211/link.c index d1f5a9f7c647..acab8309d2d6 100644 --- a/net/mac80211/link.c +++ b/net/mac80211/link.c @@ -39,6 +39,8 @@ void ieee80211_link_init(struct ieee80211_sub_if_data *sdata, ieee80211_csa_finalize_work); INIT_WORK(&link->color_change_finalize_work, ieee80211_color_change_finalize_work); + INIT_DELAYED_WORK(&link->color_collision_detect_work, + ieee80211_color_collision_detection_work); INIT_LIST_HEAD(&link->assigned_chanctx_list); INIT_LIST_HEAD(&link->reserved_chanctx_list); INIT_DELAYED_WORK(&link->dfs_cac_timer_work,
On 20/01/2023 16:55, Lorenzo Bianconi wrote: >> On 19/01/2023 15:02, Johannes Berg wrote: >>> On Mon, 2022-12-26 at 14:03 +0530, Rameshkumar Sundaram wrote: >>>> As per 802.11ax-2021, STAs shall process BSS Color Change Announcement >>>> (BCCA) from AP and switch to new color, but some STAs aren't processing >>>> BCCA from AP and not doing color switch, causing them to drop data >>>> frames from AP post color change. >>>> >>>> Provide an option to disable color collision detection and therefore >>>> not to do BCCA to mitigate the same from AP. If it's required in case >>>> where STA supports BCCA handling, then it can enabled in AP using this >>>> option. >>>> >>> >>> You should probably split this into cfg80211 and mac80211. >>> >>> Also, this doesn't really seem to make a lot of _sense_ since nothing in >>> the kernel actually acts on detection of a color collision - hostapd is >>> acting on that. >>> >>> So since you can easily make hostapd ignore the event, why do you even >>> need this? >> >> This may not be related, but the software color collision detection sends a >> netlink message for every colliding frame and it can hose up the system if >> the other network is very active. >> >> Also, cfg80211_bss_color_notify() complains that the wdev lock isn't held. > > Hi Nicolas, > > I agree, I think we can ratelimit netlink messages sent by the kernel to > userspace (e.g. to hostapd), I would say every 500ms is ok. > I guess we can move cfg80211_obss_color_collision_notify() in a dedicated > delayed_work so we can grab wdev mutex (cfg80211_obss_color_collision_notify is > currently running in interrupt context). > To give an idea, what do you think about patch below? (please note it is just > compiled tested so far). I think it should fix the problem, I'll try to test it. Thanks! > Regards, > Lorenzo
> -----Original Message----- > From: Johannes Berg <johannes@sipsolutions.net> > Sent: Thursday, January 19, 2023 7:32 PM > To: Rameshkumar Sundaram (QUIC) <quic_ramess@quicinc.com>; > ath11k@lists.infradead.org > Cc: linux-wireless@vger.kernel.org > Subject: Re: [PATCH 1/2] nl80211: add support to enable/disable bss color > collision detection > > On Mon, 2022-12-26 at 14:03 +0530, Rameshkumar Sundaram wrote: > > As per 802.11ax-2021, STAs shall process BSS Color Change Announcement > > (BCCA) from AP and switch to new color, but some STAs aren't > > processing BCCA from AP and not doing color switch, causing them to > > drop data frames from AP post color change. > > > > Provide an option to disable color collision detection and therefore > > not to do BCCA to mitigate the same from AP. If it's required in case > > where STA supports BCCA handling, then it can enabled in AP using this > > option. > > > > You should probably split this into cfg80211 and mac80211. Sure I will do it. > > Also, this doesn't really seem to make a lot of _sense_ since nothing in the > kernel actually acts on detection of a color collision - hostapd is acting on > that. > > So since you can easily make hostapd ignore the event, why do you even > need this? > Kernel will keep sending collision events until we switch color/disable BSS color Hence wanted to avoid the detection itself. > johannes
Hi Lorenzo, On 20/01/2023 16:55, Lorenzo Bianconi wrote: > I agree, I think we can ratelimit netlink messages sent by the kernel to > userspace (e.g. to hostapd), I would say every 500ms is ok. > I guess we can move cfg80211_obss_color_collision_notify() in a dedicated > delayed_work so we can grab wdev mutex (cfg80211_obss_color_collision_notify is > currently running in interrupt context). > To give an idea, what do you think about patch below? (please note it is just > compiled tested so far). The patch does not work, the fix appears easy: > +void ieee80211_color_collision_detection_work(struct work_struct *work) > +{ > + struct delayed_work *delayed_work = to_delayed_work(work); > + struct ieee80211_link_data *link = > + container_of(delayed_work, struct ieee80211_link_data, > + dfs_cac_timer_work); This should probably be color_collision_detect_work. > + struct ieee80211_sub_if_data *sdata = link->sdata; > + > + sdata_lock(sdata); It crashed here, link is NULL. > + cfg80211_obss_color_collision_notify(sdata->dev, link->color_bitmap, > + GFP_KERNEL); > + sdata_unlock(sdata); > +} Will test the fixed version later.
> Hi Lorenzo, > > On 20/01/2023 16:55, Lorenzo Bianconi wrote: > > I agree, I think we can ratelimit netlink messages sent by the kernel to > > userspace (e.g. to hostapd), I would say every 500ms is ok. > > I guess we can move cfg80211_obss_color_collision_notify() in a dedicated > > delayed_work so we can grab wdev mutex (cfg80211_obss_color_collision_notify is > > currently running in interrupt context). > > To give an idea, what do you think about patch below? (please note it is just > > compiled tested so far). > > The patch does not work, the fix appears easy: > > +void ieee80211_color_collision_detection_work(struct work_struct *work) > > +{ > > + struct delayed_work *delayed_work = to_delayed_work(work); > > + struct ieee80211_link_data *link = > > + container_of(delayed_work, struct ieee80211_link_data, > > + dfs_cac_timer_work); > > This should probably be color_collision_detect_work. Yep, sorry. It is just a copy paste issue :) I will share a new version. Regards, Lorenzo > > > + struct ieee80211_sub_if_data *sdata = link->sdata; > > + > > + sdata_lock(sdata); > > It crashed here, link is NULL. > > > + cfg80211_obss_color_collision_notify(sdata->dev, link->color_bitmap, > > + GFP_KERNEL); > > + sdata_unlock(sdata); > > +} > Will test the fixed version later.
diff --git a/include/net/cfg80211.h b/include/net/cfg80211.h index 03d4f4deadae..153a05a25d91 100644 --- a/include/net/cfg80211.h +++ b/include/net/cfg80211.h @@ -304,11 +304,13 @@ struct ieee80211_he_obss_pd { * @color: the current color. * @enabled: HE BSS color is used * @partial: define the AID equation. + * @collision_detection_enabled: HE BSS color collision detection is enabled. */ struct cfg80211_he_bss_color { u8 color; bool enabled; bool partial; + bool collision_detection_enabled; }; /** diff --git a/include/uapi/linux/nl80211.h b/include/uapi/linux/nl80211.h index c14a91bbca7c..ec514669d203 100644 --- a/include/uapi/linux/nl80211.h +++ b/include/uapi/linux/nl80211.h @@ -7440,6 +7440,8 @@ enum nl80211_obss_pd_attributes { * @NL80211_HE_BSS_COLOR_ATTR_COLOR: the current BSS Color. * @NL80211_HE_BSS_COLOR_ATTR_DISABLED: is BSS coloring disabled. * @NL80211_HE_BSS_COLOR_ATTR_PARTIAL: the AID equation to be used.. + * @NL80211_HE_BSS_COLOR_ATTR_COLLISION_DETECTION_DISABLED: is BSS + * color collision detection disabled. * * @__NL80211_HE_BSS_COLOR_ATTR_LAST: Internal * @NL80211_HE_BSS_COLOR_ATTR_MAX: highest BSS Color attribute. @@ -7450,6 +7452,7 @@ enum nl80211_bss_color_attributes { NL80211_HE_BSS_COLOR_ATTR_COLOR, NL80211_HE_BSS_COLOR_ATTR_DISABLED, NL80211_HE_BSS_COLOR_ATTR_PARTIAL, + NL80211_HE_BSS_COLOR_ATTR_COLLISION_DETECTION_DISABLED, /* keep last */ __NL80211_HE_BSS_COLOR_ATTR_LAST, diff --git a/net/mac80211/rx.c b/net/mac80211/rx.c index 7e3ab6e1b28f..382f467f0e31 100644 --- a/net/mac80211/rx.c +++ b/net/mac80211/rx.c @@ -3195,7 +3195,8 @@ ieee80211_rx_check_bss_color_collision(struct ieee80211_rx_data *rx) if (ieee80211_hw_check(&rx->local->hw, DETECTS_COLOR_COLLISION)) return; - if (rx->sdata->vif.bss_conf.csa_active) + if (rx->sdata->vif.bss_conf.csa_active || + !rx->sdata->vif.bss_conf.he_bss_color.collision_detection_enabled) return; baselen = mgmt->u.beacon.variable - rx->skb->data; diff --git a/net/wireless/nl80211.c b/net/wireless/nl80211.c index 33a82ecab9d5..8db437995e53 100644 --- a/net/wireless/nl80211.c +++ b/net/wireless/nl80211.c @@ -377,6 +377,7 @@ he_bss_color_policy[NL80211_HE_BSS_COLOR_ATTR_MAX + 1] = { [NL80211_HE_BSS_COLOR_ATTR_COLOR] = NLA_POLICY_RANGE(NLA_U8, 1, 63), [NL80211_HE_BSS_COLOR_ATTR_DISABLED] = { .type = NLA_FLAG }, [NL80211_HE_BSS_COLOR_ATTR_PARTIAL] = { .type = NLA_FLAG }, + [NL80211_HE_BSS_COLOR_ATTR_COLLISION_DETECTION_DISABLED] = { .type = NLA_FLAG }, }; static const struct nla_policy nl80211_txattr_policy[NL80211_TXRATE_MAX + 1] = { @@ -5414,6 +5415,8 @@ static int nl80211_parse_he_bss_color(struct nlattr *attrs, !nla_get_flag(tb[NL80211_HE_BSS_COLOR_ATTR_DISABLED]); he_bss_color->partial = nla_get_flag(tb[NL80211_HE_BSS_COLOR_ATTR_PARTIAL]); + he_bss_color->collision_detection_enabled = + !nla_get_flag(tb[NL80211_HE_BSS_COLOR_ATTR_COLLISION_DETECTION_DISABLED]); return 0; }
As per 802.11ax-2021, STAs shall process BSS Color Change Announcement (BCCA) from AP and switch to new color, but some STAs aren't processing BCCA from AP and not doing color switch, causing them to drop data frames from AP post color change. Provide an option to disable color collision detection and therefore not to do BCCA to mitigate the same from AP. If it's required in case where STA supports BCCA handling, then it can enabled in AP using this option. Signed-off-by: Rameshkumar Sundaram <quic_ramess@quicinc.com> --- include/net/cfg80211.h | 2 ++ include/uapi/linux/nl80211.h | 3 +++ net/mac80211/rx.c | 3 ++- net/wireless/nl80211.c | 3 +++ 4 files changed, 10 insertions(+), 1 deletion(-) base-commit: 44bacbdf9066c590423259dbd6d520baac99c1a8