Re: [PATCH v9 3/4] mac80211: add multiple bssid/EMA support to beacon handling

[Aloka Dixit <alokad@codeaurora.org>]

On 2021-04-08 05:11, Johannes Berg wrote:
> On Wed, 2021-03-10 at 10:26 -0800, Aloka Dixit wrote:
>> 
>> +/**
>> + * enum ieee80211_bcn_tmpl_ema - EMA beacon generation type
>> + * @IEEE80211_BCN_EMA_NONE: don't generate an EMA beacon.
>> + * @IEEE80211_BCN_EMA_NEXT: generate the next periodicity beacon.
>> + * @IEEE80211_BCN_EMA_INDEX: generate beacon by periodicity index
>> + *	if the value is >= this enum value.
>> + */
>> +enum ieee80211_bcn_tmpl_ema {
>> +	IEEE80211_BCN_EMA_NONE	= -2,
>> +	IEEE80211_BCN_EMA_NEXT	= -1,
>> +	IEEE80211_BCN_EMA_INDEX	= 0,
> 
> Maybe call it _BASE instead of _INDEX, it's not really meant to be used
> as is?
> 

Sure

>> +static u8 *ieee80211_copy_multiple_bssid_beacon(u8 *offset,
>> +						struct cfg80211_multiple_bssid_data *dest,
>> +						struct cfg80211_multiple_bssid_data *src)
>> +{
>> +	int i;
>> +
>> +	if (!dest || !src)
>> +		return offset;
>> +
>> +	dest->cnt = src->cnt;
>> +	for (i = 0; i < dest->cnt; i++) {
>> +		dest->elems[i].len = src->elems[i].len;
>> +		dest->elems[i].data = offset;
>> +		memcpy(dest->elems[i].data, src->elems[i].data,
>> +		       dest->elems[i].len);
>> +		offset += dest->elems[i].len;
>> +	}
> 
> 
> Following my earlier question - here you just copy all the elements one
> after another, as far as I can tell, so why did they need to be 
> separate
> in the first place? Might be a lot simpler everywhere if all of this 
> was
> just a single buffer, starting from the userspace API?
> 
> 

Separate buffers required as only one buffer is copied when
(ema_index >= IEEE80211_BCN_EMA_INDEX) in
__ieee80211_beacon_get().


>> @@ -4740,13 +4800,11 @@ __ieee80211_beacon_get(struct ieee80211_hw 
>> *hw,
>>  	struct ieee80211_chanctx_conf *chanctx_conf;
>>  	int csa_off_base = 0;
> 
>> -	rcu_read_lock();
>> -
>>  	sdata = vif_to_sdata(vif);
>>  	chanctx_conf = rcu_dereference(sdata->vif.chanctx_conf);
> 
> OK, but ...
> 
>>  struct sk_buff *ieee80211_beacon_get_tim(struct ieee80211_hw *hw,
>>  					 struct ieee80211_vif *vif,
>>  					 u16 *tim_offset, u16 *tim_length)
>>  {
>>  	struct ieee80211_mutable_offsets offs = {};
>> -	struct sk_buff *bcn = __ieee80211_beacon_get(hw, vif, &offs, false);
>> +	struct sk_buff *bcn = __ieee80211_beacon_get(hw, vif, &offs, false,
>> +						     IEEE80211_BCN_EMA_NONE);
> 
> You didn't add the protection everywhere.
> 

Will revise this part, thanks for bringing it up.

> johannes