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: <441771f8-6269-0ce4-fce8-513f7f3f7d95@quicinc.com>
Date:   Mon, 19 Dec 2022 10:53:55 -0800
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101
 Thunderbird/102.4.1
Subject: Re: [PATCH v12 2/4] mac80211: MBSSID support in interface handling
Content-Language: en-US
To:     Jouni Malinen <j@w1.fi>, Aloka Dixit <alokad@codeaurora.org>
CC:     <johannes@sipsolutions.net>, <linux-wireless@vger.kernel.org>
References: <20210916025437.29138-1-alokad@codeaurora.org>
 <20210916025437.29138-3-alokad@codeaurora.org>
 <20221218152420.GA906762@w1.fi>
From:   Aloka Dixit <quic_alokad@quicinc.com>
In-Reply-To: <20221218152420.GA906762@w1.fi>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 12/18/2022 7:24 AM, Jouni Malinen wrote:
> On Wed, Sep 15, 2021 at 07:54:35PM -0700, Aloka Dixit wrote:
>> Configure multiple BSSID and enhanced multi-BSSID advertisement (EMA)
>> parameters in mac80211 for AP mode.
>>
>> For each interface, 'mbssid_tx_vif' points to the transmitting interface of
>> the MBSSID set. The pointer is set to NULL if MBSSID is disabled.
>>
>> Function ieee80211_stop() is modified to always bring down all the
>> non-transmitting interfaces first and the transmitting interface last.
> 
> This has already been applied, but this has some apparent issues that
> are now showing up with mac80211_hwsim testing being available..
> 
>> diff --git a/net/mac80211/cfg.c b/net/mac80211/cfg.c
>> +static int ieee80211_set_ap_mbssid_options(struct ieee80211_sub_if_data *sdata,
>> +					   struct cfg80211_mbssid_config params)
> 
> While that does not really break behavior, why is that params argument
> passed by value instead of by reference? I see no point in copying
> struct cfg80211_mbssid_config members for this call since the function
> is only reading the value.
> 

Hi Jouni, the only reason for value instead of reference is that this 
function does not need to change anything in 'params'. I didn't 
understand your question. Are you suggesting moving the assignments to 
ieee80211_start_ap() instead of this separate function?

>> +	sdata->vif.mbssid_tx_vif = NULL;
>> +	sdata->vif.bss_conf.bssid_index = 0;
>> +	sdata->vif.bss_conf.nontransmitted = false;
>> +	sdata->vif.bss_conf.ema_ap = false;
> 
> This cleanup is important, but it is done only here in this helper
> function..
> And that is the only place where the help function is called and this
> happens only under the params->mbssid_config.tx_wdev condition. In other
> words, those bssid_index/nontransmitted/ema_ap values are not cleared in
> all cases. This results in issue when the bss_conf (link_conf in the
> current kernel snapshot) is left in the previous mbssid configuration.
> 

Will send a patch to fix this part.

Please let me know regarding the first question above so that I can 
include that in the same patch.

Thanks.