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

[Jouni Malinen]

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.

-- 
Jouni Malinen                                            PGP id EFC895FA