Received-SPF: pass (google.com: domain of linux-wireless-owner@vger.kernel.org designates 23.128.96.35 as permitted sender) client-ip=23.128.96.35;
Message-ID: <7f049725302e06040194e0aa76a8c34d6ecc024c.camel@sipsolutions.net>
Subject: Re: [PATCH v7 2/2] wifi: mac80211: S1G beacon/short beacon support
From:   Johannes Berg <johannes@sipsolutions.net>
To:     Bassem Dawood <bassem@morsemicro.com>,
        linux-wireless@vger.kernel.org
Cc:     Kieran Frewen <kieran.frewen@morsemicro.com>,
        Gilad Itzkovitch <gilad.itzkovitch@morsemicro.com>
Date:   Wed, 13 Sep 2023 13:56:25 +0200
In-Reply-To: <20230810093556.33800-2-bassem@morsemicro.com>
References: <20230810093556.33800-1-bassem@morsemicro.com>
         <20230810093556.33800-2-bassem@morsemicro.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
User-Agent: Evolution 3.48.4 (3.48.4-1.fc38) 
MIME-Version: 1.0
Precedence: bulk

On Thu, 2023-08-10 at 19:35 +1000, Bassem Dawood wrote:
> From: Kieran Frewen <kieran.frewen@morsemicro.com>
>=20
> With the kernel able to send both short and long S1G beacons, include
> the ability for setting the short beacon period. If configured, use
> the S1G short beacon format. The S1G short beacon format includes a
> limited set of information elements. It also adds the support for
> distinguish short beacon head and tail.

Also here, I think that could be more elaborate.

And we like to also have commit logs in imperative voice.

> +++ b/net/mac80211/ieee80211_i.h
> @@ -259,10 +259,13 @@ struct ieee80211_color_change_settings {
> =20
>  struct beacon_data {
>  	u8 *head, *tail;
> +	u8 *short_head, *short_tail;
>  	int head_len, tail_len;
> +	int short_head_len, short_tail_len;
>  	struct ieee80211_meshconf_ie *meshconf;
>  	u16 cntdwn_counter_offsets[IEEE80211_MAX_CNTDWN_COUNTERS_NUM];
>  	u8 cntdwn_current_counter;
> +	u8 long_beacon_count;

I don't understand why this is called long_beacon_count?

> +++ b/net/mac80211/tx.c
> @@ -5260,6 +5260,18 @@ ieee80211_beacon_get_ap(struct ieee80211_hw *hw,
>  	struct sk_buff *skb =3D NULL;
>  	u16 csa_off_base =3D 0;
>  	int mbssid_len;
> +	bool is_short =3D false;
> +
> +	if (vif->cfg.s1g) {
> +		if (beacon->long_beacon_count =3D=3D 0) {
> +			is_short =3D false;
> +			beacon->long_beacon_count =3D
> +				vif->bss_conf.short_beacon_period - 1;
> +		} else {
> +			is_short =3D true;
> +			beacon->long_beacon_count--;

It's decremented for every _short_ beacon, and then you get a long one
when it reaches 0 ... so it's surely not counting long beacons?


Seems like is_short now needs to be taken into account for the beacon
skb allocation, nowhere did you actually ensure that the short beacon is
indeed shorter than the long beacon ... you don't want to let userspace
crash us with that.

johannes