Received-SPF: pass (google.com: domain of linux-wireless-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <3163fef05bde096540020e58a93003c6048d9a0b.camel@sipsolutions.net>
Subject: Re: [PATCH 1/9] mac80211: generate EMA beacons in AP mode
From:   Johannes Berg <johannes@sipsolutions.net>
To:     Aloka Dixit <quic_alokad@quicinc.com>,
        linux-wireless@vger.kernel.org, ath11k@lists.infradead.org
Cc:     John Crispin <john@phrozen.org>
Date:   Fri, 01 Jul 2022 10:59:43 +0200
In-Reply-To: <20220523060108.23982-2-quic_alokad@quicinc.com>
References: <20220523060108.23982-1-quic_alokad@quicinc.com>
         <20220523060108.23982-2-quic_alokad@quicinc.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
User-Agent: Evolution 3.44.2 (3.44.2-1.fc36) 
MIME-Version: 1.0
Precedence: bulk

>=20
> +++ b/include/net/mac80211.h
> @@ -5050,6 +5050,83 @@ ieee80211_beacon_get_template(struct ieee80211_hw =
*hw,
>  			      struct ieee80211_vif *vif,
>  			      struct ieee80211_mutable_offsets *offs);
> =20
> +/**
> + * This macro should be used to get total length for all MBSSID elements
> + * in the beacon, and also to generate a single beacon template with
> + * all MBSSID elements.
> + */
> +#define IEEE80211_MBSSID_ELEMS_ALL -1

Not sure where the driver should use this? Why is it defined in the
driver API?

> + * @ema_index: index of the beacon in the EMA set, should be more than
> + *	IEEE80211_MBSSID_ELEMS_ALL otherwise the set/get functions include al=
l
> + *	MBSSID elements in a single beacon template.

The parameter is a u8 so it cannot be negative anyway ...?

> +struct sk_buff *
> +ieee80211_beacon_get_template_ema_index(struct ieee80211_hw *hw,
> +					struct ieee80211_vif *vif,
> +					struct ieee80211_mutable_offsets *offs,
> +					u8 ema_index);

here ^^

>  static inline int
> -ieee80211_get_mbssid_beacon_len(struct cfg80211_mbssid_elems *elems)
> +ieee80211_get_mbssid_beacon_len(struct cfg80211_mbssid_elems *elems, int=
 i)
>  {
> -	int i, len =3D 0;
> +	int len =3D 0;
> =20
>  	if (!elems)
>  		return 0;
> +	else if (i < IEEE80211_MBSSID_ELEMS_ALL || i >=3D elems->cnt)
> +		return -1;

-1 is a strange return value (it means -EPERM but you didn't say it);
maybe use -EINVAL?

> @@ -5059,7 +5065,8 @@ ieee80211_beacon_get_ap(struct ieee80211_hw *hw,
>  			struct ieee80211_mutable_offsets *offs,
>  			bool is_template,
>  			struct beacon_data *beacon,
> -			struct ieee80211_chanctx_conf *chanctx_conf)
> +			struct ieee80211_chanctx_conf *chanctx_conf,
> +			int ema_index)

A lot of this stuff is now conflicting with MLO work, so also needs a
rebase.

> -	mbssid_len =3D ieee80211_get_mbssid_beacon_len(beacon->mbssid_ies);
> +	mbssid_len =3D ieee80211_get_mbssid_beacon_len(beacon->mbssid_ies,
> +						     ema_index);
> +	if (mbssid_len =3D=3D -1)

< 0 then here

> +static void
> +ieee80211_beacon_get_ap_ema_list(struct ieee80211_hw *hw,
> +				 struct ieee80211_vif *vif,
> +				 struct ieee80211_mutable_offsets *offs,
> +				 bool is_template, struct beacon_data *beacon,
> +				 struct ieee80211_chanctx_conf *chanctx_conf,
> +				 struct ieee80211_ema_beacons **ema_beacons)

The API here is weird, why not *return* what goes into *ema_beacons?

> +struct ieee80211_ema_beacons *
> +ieee80211_beacon_get_template_ema_list(struct ieee80211_hw *hw,
> +				       struct ieee80211_vif *vif)
> +{
> +	struct ieee80211_ema_beacons *ema_beacons =3D NULL;
> +	(void)__ieee80211_beacon_get(hw, vif, NULL, false, 0, &ema_beacons);
> +	return ema_beacons;


couple of blank lines would be nice, and maybe check the return value
and WARN_ON() it not being NULL? It should be NULL in this case, I
think?

johannes