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>
Subject: Re: [PATCH 1/9] mac80211: generate EMA beacons in AP mode
Date: Fri, 01 Jul 2022 10:59:43 +0200	[thread overview]
Message-ID: <3163fef05bde096540020e58a93003c6048d9a0b.camel@sipsolutions.net> (raw)
In-Reply-To: <20220523060108.23982-2-quic_alokad@quicinc.com>

> 
> +++ 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);
>  
> +/**
> + * 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 all
> + *	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 = 0;
> +	int len = 0;
>  
>  	if (!elems)
>  		return 0;
> +	else if (i < IEEE80211_MBSSID_ELEMS_ALL || i >= 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 = ieee80211_get_mbssid_beacon_len(beacon->mbssid_ies);
> +	mbssid_len = ieee80211_get_mbssid_beacon_len(beacon->mbssid_ies,
> +						     ema_index);
> +	if (mbssid_len == -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 = 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