diff mbox series

[1/6] bss coloring: add support for handling collision events and triggering CCA

Message ID 3602d12247be5d7a335ee9e3d57e04664256712b.1631091271.git.ryder.lee@mediatek.com
State Changes Requested
Headers show
Series [1/6] bss coloring: add support for handling collision events and triggering CCA | expand

Commit Message

Ryder Lee Sept. 8, 2021, 9:18 a.m. UTC 
From: John Crispin <john@phrozen.org>

Add the core code for handling bss color collision events and triggering
CCA inside the kernel.

Co-developed-by: Lorenzo Bianconi <lorenzo@kernel.org>
Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org>
Signed-off-by: John Crispin <john@phrozen.org>
Signed-off-by: Ryder Lee <ryder.lee@mediatek.com>
---
 src/ap/ap_drv_ops.h          |  12 ++++
 src/ap/hostapd.c             | 119 +++++++++++++++++++++++++++++++++++
 src/ap/hostapd.h             |  16 +++++
 src/common/ieee802_11_defs.h |   6 ++
 src/drivers/driver.h         |  31 +++++++++
 5 files changed, 184 insertions(+)

Comments

Jouni Malinen Nov. 3, 2021, 7:02 p.m. UTC | #1 
On Wed, Sep 08, 2021 at 05:18:50PM +0800, Ryder Lee wrote:
> diff --git a/src/ap/hostapd.c b/src/ap/hostapd.c
> +static void
> +hostapd_switch_color_timeout_handler(void *eloop_data, void *user_ctx);

That needs to be within #ifdef CONFIG_IEEE80211AX to avoid compiler
warnings when cONFIG_IEEE80211AX is not defined.

> +void hostapd_cleanup_cca_params(struct hostapd_data *hapd)
> +{
> +	hapd->cca_count = 0;
> +	hapd->cca_color = 0;
> +	hapd->cca_c_off_beacon = 0;
> +	hapd->cca_c_off_proberesp = 0;
> +	hapd->cca_in_progress = 0;

It would be better to use bool cca_in_progress and true/false instead of
int and 1/0.

> +static void
> +hostapd_switch_color_timeout_handler(void *eloop_data, void *user_ctx)
> +{
> +	struct hostapd_data *hapd = (struct hostapd_data *) eloop_data;
> +	struct cca_settings settings;
> +	struct os_time now;

All uses of struct os_time should be replaced with struct os_reltime to
be able to handle possible clock changes.

> +	int i, r, b, ret;

b needs to be unsigned to avoid compiler warnings.

> +	if (os_get_time(&now))

os_get_reltime

> +	/* check if there has been a recent collision */
> +	if (now.sec - hapd->last_color_collision.sec >= 10)
> +		return;

What is this trying to do? Skip changes if there has not been additional
collisions during the last 10 seconds of the previously scheduled 50
second timeout? What is that 10 seconds based on?

> +	if (i == HE_OPERATION_BSS_COLOR_MAX) {
> +		/* there are no free colors so turn bss coloring off */
> +		wpa_printf(MSG_INFO, "no free colors left, turning of BSS coloring");
> +		hapd->iface->conf->he_op.he_bss_color_disabled = 1;
> +		hapd->iface->conf->he_op.he_bss_color = 1;

Should we use random BSS color in this case similarly to the change in
https://w1.fi/cgit/hostap/commit/src?id=41ec97cd09486a6bda21f5f5b89e5242e6ade2a2

> +void
> +hostapd_switch_color(struct hostapd_data *hapd, u64 bitmap)

This function seems to be completed unused.. It would be good to include
the user for this or be clear in the commit message that the caller will
be added in another commit (patch 5/6 in this series).

> +	if (hapd->cca_in_progress)
> +		return;
> +
> +	if (os_get_time(&hapd->last_color_collision))

os_get_reltime

> +	if (!eloop_is_timeout_registered(hostapd_switch_color_timeout_handler, hapd, NULL))
> +		eloop_register_timeout(DOT11BSS_COLOR_COLLISION_AP_PERIOD, 0,
> +				       hostapd_switch_color_timeout_handler, hapd, NULL);
> +}

What is this behavior based on? DOT11BSS_COLOR_COLLISION_AP_PERIOD
definition refers to IEEE Std 802.11ax-2021, 26.17.3.5.1 which notes
that the BSS Color Disabled subfield is set to 1 if the BSS color
collision persistent for at least dot11BSSColorCollisionAPPeriod.
However, this timeout seems to be used for changing the BSS color, not
only for disabling BSS color.

> diff --git a/src/ap/hostapd.h b/src/ap/hostapd.h
> +	int cca_in_progress;
bool

> +	struct os_time last_color_collision;

os_reltime

> diff --git a/src/common/ieee802_11_defs.h b/src/common/ieee802_11_defs.h
> +/* IEEE802.11/D6.0 - 26.17.3.5.1
> + * the minimum default timeout between color collision and color change is defined as 50s
> + */
> +#define DOT11BSS_COLOR_COLLISION_AP_PERIOD	50

That comment is a bit confusing.. IEEE Std 802.11ax-2021, 26.17.3.5.1
seems to be talking about disabling BSS color after
dot11BSSColorCollisionAPPeriod and so does the definition of this MIB
variable while this comment talks about time between the color collision
and color change which sounds quite different.
Ryder Lee Nov. 4, 2021, 6:41 a.m. UTC | #2 
> > (!eloop_is_timeout_registered(hostapd_switch_color_timeout_handler,
> > hapd, NULL))
> > +		eloop_register_timeout(DOT11BSS_COLOR_COLLISION_AP_PERI
> > OD, 0,
> > +				       hostapd_switch_color_timeout_han
> > dler, hapd, NULL);
> > +}
> 
> What is this behavior based on? DOT11BSS_COLOR_COLLISION_AP_PERIOD
> definition refers to IEEE Std 802.11ax-2021, 26.17.3.5.1 which notes
> that the BSS Color Disabled subfield is set to 1 if the BSS color
> collision persistent for at least dot11BSSColorCollisionAPPeriod.
> However, this timeout seems to be used for changing the BSS color,
> not
> only for disabling BSS color.

This is a series of continuous actions I think. It depends on how we
interpret the 26.17.3.5 and what's the best time to change the color,
but I think most of implementations are similar.

1. BSS color collision persistent for at least
dot11BSSColorCollisionAPPeriod.
2. BSS Color Disabled subfield is set to 1.
3. Switch color out.

> > diff --git a/src/ap/hostapd.h b/src/ap/hostapd.h
> > +	int cca_in_progress;
> 
> bool
> 
Will do, It's better to use bool indeed. I thought of this but changed
my mind after looking at csa_in_progress, so keeping int for the sake
of consistency.

> 
> > diff --git a/src/common/ieee802_11_defs.h
> > b/src/common/ieee802_11_defs.h
> > +/* IEEE802.11/D6.0 - 26.17.3.5.1
> > + * the minimum default timeout between color collision and color
> > change is defined as 50s
> > + */
> > +#define DOT11BSS_COLOR_COLLISION_AP_PERIOD	50
> 
> That comment is a bit confusing.. IEEE Std 802.11ax-2021, 26.17.3.5.1
> seems to be talking about disabling BSS color after
> dot11BSSColorCollisionAPPeriod and so does the definition of this MIB
> variable while this comment talks about time between the color
> collision
> and color change which sounds quite different.

I will revise this comment to reflect the IEEE Std 802.11ax-2021,
26.17.3.5.1.

Thank you for reviewing I will fix other parts.

Ryder
Ryder Lee Nov. 5, 2021, 5:40 a.m. UTC | #3 
> > +	/* check if there has been a recent collision */
> > +	if (now.sec - hapd->last_color_collision.sec >= 10)
> > +		return;
> 
> What is this trying to do? Skip changes if there has not been
> additional
> collisions during the last 10 seconds of the previously scheduled 50
> second timeout? What is that 10 seconds based on?

Yes, 10s is the approximate margin of collision persistent for current
implementation. Therefore a driver needs to report collision events
constantly to update the last_color_collision.sec to trigger CCA. I
could add some comments on this as an initial implementation and a todo
for note - maybe we can come out with a better method in the future.

Ryder
diff mbox series

Patch

diff --git a/src/ap/ap_drv_ops.h b/src/ap/ap_drv_ops.h
index 61c8f64eb..003f0daf7 100644
--- a/src/ap/ap_drv_ops.h
+++ b/src/ap/ap_drv_ops.h
@@ -299,6 +299,18 @@  static inline int hostapd_drv_switch_channel(struct hostapd_data *hapd,
 	return hapd->driver->switch_channel(hapd->drv_priv, settings);
 }
 
+#ifdef CONFIG_IEEE80211AX
+static inline int hostapd_drv_switch_color(struct hostapd_data *hapd,
+					   struct cca_settings *settings)
+{
+	if (hapd->driver == NULL || hapd->driver->switch_color == NULL ||
+	    hapd->drv_priv == NULL)
+		return -1;
+
+	return hapd->driver->switch_color(hapd->drv_priv, settings);
+}
+#endif
+
 static inline int hostapd_drv_status(struct hostapd_data *hapd, char *buf,
 				     size_t buflen)
 {
diff --git a/src/ap/hostapd.c b/src/ap/hostapd.c
index 913a8e29e..57f3772e6 100644
--- a/src/ap/hostapd.c
+++ b/src/ap/hostapd.c
@@ -66,6 +66,8 @@  static int setup_interface2(struct hostapd_iface *iface);
 static void channel_list_update_timeout(void *eloop_ctx, void *timeout_ctx);
 static void hostapd_interface_setup_failure_handler(void *eloop_ctx,
 						    void *timeout_ctx);
+static void
+hostapd_switch_color_timeout_handler(void *eloop_data, void *user_ctx);
 
 
 int hostapd_for_each_interface(struct hapd_interfaces *interfaces,
@@ -462,6 +464,9 @@  void hostapd_free_hapd_data(struct hostapd_data *hapd)
 	}
 	eloop_cancel_timeout(auth_sae_process_commit, hapd, NULL);
 #endif /* CONFIG_SAE */
+#ifdef CONFIG_IEEE80211AX
+	eloop_cancel_timeout(hostapd_switch_color_timeout_handler, hapd, NULL);
+#endif
 }
 
 
@@ -3691,6 +3696,120 @@  hostapd_switch_channel_fallback(struct hostapd_iface *iface,
 	hostapd_enable_iface(iface);
 }
 
+
+#ifdef CONFIG_IEEE80211AX
+void hostapd_cleanup_cca_params(struct hostapd_data *hapd)
+{
+	hapd->cca_count = 0;
+	hapd->cca_color = 0;
+	hapd->cca_c_off_beacon = 0;
+	hapd->cca_c_off_proberesp = 0;
+	hapd->cca_in_progress = 0;
+}
+
+
+static int hostapd_fill_cca_settings(struct hostapd_data *hapd,
+				     struct cca_settings *settings)
+{
+	struct hostapd_iface *iface = hapd->iface;
+	u8 old_color;
+	int ret;
+
+	if (!iface || iface->conf->he_op.he_bss_color_disabled)
+		return -1;
+
+        old_color = iface->conf->he_op.he_bss_color;
+	iface->conf->he_op.he_bss_color = hapd->cca_color;
+	ret = hostapd_build_beacon_data(hapd, &settings->beacon_after);
+	iface->conf->he_op.he_bss_color = old_color;
+
+	settings->cca_count = hapd->cca_count;
+	settings->cca_color = hapd->cca_color,
+	hapd->cca_in_progress = 1;
+
+	ret = hostapd_build_beacon_data(hapd, &settings->beacon_cca);
+	if (ret) {
+		free_beacon_data(&settings->beacon_after);
+		return ret;
+	}
+
+	settings->counter_offset_beacon = hapd->cca_c_off_beacon;
+	settings->counter_offset_presp = hapd->cca_c_off_proberesp;
+
+	return 0;
+}
+
+
+static void
+hostapd_switch_color_timeout_handler(void *eloop_data, void *user_ctx)
+{
+	struct hostapd_data *hapd = (struct hostapd_data *) eloop_data;
+	struct cca_settings settings;
+	struct os_time now;
+	int i, r, b, ret;
+
+	if (os_get_time(&now))
+		return;
+
+	/* check if there has been a recent collision */
+	if (now.sec - hapd->last_color_collision.sec >= 10)
+		return;
+
+	r = os_random() % HE_OPERATION_BSS_COLOR_MAX;
+	for (i = 0; i < HE_OPERATION_BSS_COLOR_MAX; i++) {
+		if (r && (hapd->color_collision_bitmap & (1 << r)) == 0)
+			break;
+		r = (r + 1) % HE_OPERATION_BSS_COLOR_MAX;
+	}
+	if (i == HE_OPERATION_BSS_COLOR_MAX) {
+		/* there are no free colors so turn bss coloring off */
+		wpa_printf(MSG_INFO, "no free colors left, turning of BSS coloring");
+		hapd->iface->conf->he_op.he_bss_color_disabled = 1;
+		hapd->iface->conf->he_op.he_bss_color = 1;
+		for (b = 0; b < hapd->iface->num_bss; b++)
+			ieee802_11_set_beacon(hapd->iface->bss[b]);
+		return;
+	}
+
+	for (b = 0; b < hapd->iface->num_bss; b++) {
+		struct hostapd_data *bss = hapd->iface->bss[b];
+
+		hostapd_cleanup_cca_params(bss);
+		bss->cca_color = r;
+		bss->cca_count = 10;
+
+		if (hostapd_fill_cca_settings(bss, &settings)) {
+			hostapd_cleanup_cca_params(bss);
+			continue;
+		}
+
+		ret = hostapd_drv_switch_color(bss, &settings);
+		free_beacon_data(&settings.beacon_cca);
+		free_beacon_data(&settings.beacon_after);
+
+		if (ret)
+			hostapd_cleanup_cca_params(bss);
+	}
+}
+
+
+void
+hostapd_switch_color(struct hostapd_data *hapd, u64 bitmap)
+{
+	if (hapd->cca_in_progress)
+		return;
+
+	if (os_get_time(&hapd->last_color_collision))
+		return;
+
+	hapd->color_collision_bitmap = bitmap;
+
+	if (!eloop_is_timeout_registered(hostapd_switch_color_timeout_handler, hapd, NULL))
+		eloop_register_timeout(DOT11BSS_COLOR_COLLISION_AP_PERIOD, 0,
+				       hostapd_switch_color_timeout_handler, hapd, NULL);
+}
+#endif
+
 #endif /* NEED_AP_MLME */
 
 
diff --git a/src/ap/hostapd.h b/src/ap/hostapd.h
index 07d0aaa92..330b769ae 100644
--- a/src/ap/hostapd.h
+++ b/src/ap/hostapd.h
@@ -292,6 +292,16 @@  struct hostapd_data {
 	unsigned int cs_c_off_ecsa_beacon;
 	unsigned int cs_c_off_ecsa_proberesp;
 
+#ifdef CONFIG_IEEE80211AX
+	int cca_in_progress;
+	u8 cca_count;
+	u8 cca_color;
+	unsigned int cca_c_off_beacon;
+	unsigned int cca_c_off_proberesp;
+	struct os_time last_color_collision;
+	u64 color_collision_bitmap;
+#endif
+
 #ifdef CONFIG_P2P
 	struct p2p_data *p2p;
 	struct p2p_group *p2p_group;
@@ -645,6 +655,12 @@  void hostapd_periodic_iface(struct hostapd_iface *iface);
 int hostapd_owe_trans_get_info(struct hostapd_data *hapd);
 void hostapd_ocv_check_csa_sa_query(void *eloop_ctx, void *timeout_ctx);
 
+
+#ifdef CONFIG_IEEE80211AX
+void hostapd_switch_color(struct hostapd_data *hapd, u64 bitmap);
+void hostapd_cleanup_cca_params(struct hostapd_data *hapd);
+#endif
+
 /* utils.c */
 int hostapd_register_probereq_cb(struct hostapd_data *hapd,
 				 int (*cb)(void *ctx, const u8 *sa,
diff --git a/src/common/ieee802_11_defs.h b/src/common/ieee802_11_defs.h
index 2ed2d37b0..73d2dad03 100644
--- a/src/common/ieee802_11_defs.h
+++ b/src/common/ieee802_11_defs.h
@@ -2301,6 +2301,7 @@  struct ieee80211_spatial_reuse {
 #define HE_OPERATION_BSS_COLOR_PARTIAL		((u32) BIT(30))
 #define HE_OPERATION_BSS_COLOR_DISABLED		((u32) BIT(31))
 #define HE_OPERATION_BSS_COLOR_OFFSET		24
+#define HE_OPERATION_BSS_COLOR_MAX		64
 
 /* Spatial Reuse defines */
 #define SPATIAL_REUSE_SRP_DISALLOWED		BIT(0)
@@ -2446,4 +2447,9 @@  enum mscs_description_subelem {
  */
 #define FD_MAX_INTERVAL_6GHZ                  20 /* TUs */
 
+/* IEEE802.11/D6.0 - 26.17.3.5.1
+ * the minimum default timeout between color collision and color change is defined as 50s
+ */
+#define DOT11BSS_COLOR_COLLISION_AP_PERIOD	50
+
 #endif /* IEEE802_11_DEFS_H */
diff --git a/src/drivers/driver.h b/src/drivers/driver.h
index 2020184c5..f762ccccc 100644
--- a/src/drivers/driver.h
+++ b/src/drivers/driver.h
@@ -2395,6 +2395,26 @@  struct csa_settings {
 	u16 counter_offset_presp[2];
 };
 
+/**
+ * struct cca_settings - Settings for color switch command
+ * @cca_count: Count in Beacon frames (TBTT) to perform the switch
+ * @cca_color: The new color that we are switching to
+ * @beacon_cca: Beacon/probe resp/asooc resp info for color switch period
+ * @beacon_after: Next beacon/probe resp/asooc resp info
+ * @counter_offset_beacon: Offset to the count field in beacon's tail
+ * @counter_offset_presp: Offset to the count field in probe resp.
+ */
+struct cca_settings {
+	u8 cca_count;
+	u8 cca_color;
+
+	struct beacon_data beacon_cca;
+	struct beacon_data beacon_after;
+
+	u16 counter_offset_beacon;
+	u16 counter_offset_presp;
+};
+
 /* TDLS peer capabilities for send_tdls_mgmt() */
 enum tdls_peer_capability {
 	TDLS_PEER_HT = BIT(0),
@@ -3977,6 +3997,17 @@  struct wpa_driver_ops {
 	 */
 	int (*switch_channel)(void *priv, struct csa_settings *settings);
 
+	/**
+	 * switch_color - Announce color switch and migrate the BSS to the
+	 * given color
+	 * @priv: Private driver interface data
+	 * @settings: Settings for CCA period and new color
+	 * Returns: 0 on success, -1 on failure
+	 *
+	 * This function is used to move the BSS to its new color.
+	 */
+	int (*switch_color)(void *priv, struct cca_settings *settings);
+
 	/**
 	 * add_tx_ts - Add traffic stream
 	 * @priv: Private driver interface data