Received-SPF: pass (google.com: domain of linux-wireless-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Message-ID: <12f0ac8678ef356cf09095324062ead5e0e37e2a.camel@sipsolutions.net>
Subject: Re: [PATCH V2 2/3] mac80211: add support for BSS coloring
From:   Johannes Berg <johannes@sipsolutions.net>
To:     John Crispin <john@phrozen.org>
Cc:     linux-wireless@vger.kernel.org, ath11k@lists.infradead.org
Date:   Thu, 30 Jul 2020 16:33:21 +0200
In-Reply-To: <20200706170616.1764626-2-john@phrozen.org>
References: <20200706170616.1764626-1-john@phrozen.org>
         <20200706170616.1764626-2-john@phrozen.org>
Content-Type: text/plain; charset="UTF-8"
User-Agent: Evolution 3.36.4 (3.36.4-1.fc32) 
MIME-Version: 1.0
Content-Transfer-Encoding: 7bit
Sender: linux-wireless-owner@vger.kernel.org
Precedence: bulk

On Mon, 2020-07-06 at 19:06 +0200, John Crispin wrote:
> The CCA (color change announcement) is very similar to how CSA works where
> we have an IE that includes a counter. When the counter hits 0, the new
> color is applied via an updated beacon.
> 
> This patch makes the CSA counter functionality reusable, rather than
> implementing it again. This also allows for future reuse incase support
> for other counter IEs gets added.

Makes sense. But

> @@ -4744,12 +4756,15 @@ void ieee80211_report_low_ack(struct ieee80211_sta *sta, u32 num_packets);
>   * @csa_counter_offs: array of IEEE80211_MAX_CSA_COUNTERS_NUM offsets
>   *	to CSA counters.  This array can contain zero values which
>   *	should be ignored.
> + * @cca_counter_off: offset to the cca counter. zero values should be
> + *	ignored.
>   */
>  struct ieee80211_mutable_offsets {
>  	u16 tim_offset;
>  	u16 tim_length;
>  
>  	u16 csa_counter_offs[IEEE80211_MAX_CSA_COUNTERS_NUM];
> +	u16 cca_counter_off;

why then do you need a new field here?

> +/**
> + * ieee80211_cca_update_counter - request mac80211 to decrement the cca counter
> + * @vif: &struct ieee80211_vif pointer from the add_interface callback.
> + *
> + * The cca counter should be updated after each beacon transmission.
> + * This function is called implicitly when
> + * ieee80211_beacon_get/ieee80211_beacon_get_tim are called, however if the
> + * beacon frames are generated by the device, the driver should call this
> + * function after each beacon transmission to sync mac80211's cca counters.
> + *
> + * Return: new csa counter value


that's a bit ... mixed up with cca vs. csa? Please capitalize these too.

> +u8 ieee80211_cca_update_counter(struct ieee80211_vif *vif);

(except in the function name of course :) )

> +/**
> + * ieee80211_cca_finish - notify mac80211 about color change
> + * @vif: &struct ieee80211_vif pointer from the add_interface callback.
> + *
> + * After a color change announcement was scheduled and the counter in this
> + * announcement hits 1, this function must be called by the driver to
> + * notify mac80211 that the color can be changed
> + */
> +void ieee80211_cca_finish(struct ieee80211_vif *vif);
> +
> +/**
> + * ieee80211_cca_is_complete - find out if counters reached 1
> + * @vif: &struct ieee80211_vif pointer from the add_interface callback.
> + *
> + * This function returns whether the color change counters reached zero.
> + */
> +bool ieee80211_cca_is_complete(struct ieee80211_vif *vif);

If you're reusing, why do you even need the new functions? Wouldn't it
make more sense to rename the CSA functions to something like

ieee80211_beacon_countdown_...()?

or something like that?

> +/**
> + * ieeee80211_obss_color_collision_notify notify userland about a BSS color
> + * collision.
> + *
> + * @vif: &struct ieee80211_vif pointer from the add_interface callback.
> + * @color_bitmap: a 64 bit bitmap representing the colors that the local BSS is
> + *	aware of.
> + */
> +void
> +ieeee80211_obss_color_collision_notify(struct ieee80211_vif *vif,
> +				       u64 color_bitmap);

Come to think of it, it might be good to break this whole collision
stuff out into two separate patches. Obviously one is only useful with
the other, but easier to look at.

>  static int ieee80211_set_probe_resp(struct ieee80211_sub_if_data *sdata,
>  				    const u8 *resp, size_t resp_len,
> -				    const struct ieee80211_csa_settings *csa)
> +				    const struct ieee80211_csa_settings *csa,
> +				    const struct ieee80211_cca_settings *cca)

How do these differ? Why need both instead of renaming one?

> +	if (cca)
> +		new->cca_counter_offset = cca->counter_offset_presp;

What if we were to need two counters for this in the future? I think the
nl80211 API even allowed for that?


Hmm. This doesn't seem like that much "reuse" if all the APIs are
duplicated?

johannes