Received-SPF: pass (google.com: domain of linux-wireless+bounces-3656-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Message-ID: <78c51497-dd6b-4d7a-b789-e3823e89a96a@quicinc.com>
Date: Fri, 16 Feb 2024 08:45:48 +0530
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH 2/2] wifi: mac80211_hwsim: add support for
 switch_vif_chanctx callback
Content-Language: en-US
To: Johannes Berg <johannes@sipsolutions.net>
CC: <linux-wireless@vger.kernel.org>
References: <20240215162811.506065-1-quic_adisi@quicinc.com>
 <20240215162811.506065-3-quic_adisi@quicinc.com>
 <de92380b3ec6d4e6bf854479fb3eca5ae88cf1fc.camel@sipsolutions.net>
From: Aditya Kumar Singh <quic_adisi@quicinc.com>
In-Reply-To: <de92380b3ec6d4e6bf854479fb3eca5ae88cf1fc.camel@sipsolutions.net>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 7bit

On 2/16/24 01:20, Johannes Berg wrote:
> On Thu, 2024-02-15 at 21:58 +0530, Aditya Kumar Singh wrote:
>> +static int mac80211_hwsim_switch_vif_chanctx(struct ieee80211_hw *hw,
>> +					     struct ieee80211_vif_chanctx_switch *vifs,
>> +					     int n_vifs,
>> +					     enum ieee80211_chanctx_switch_mode mode)
>> +{
>> +	int i;
>> +
>> +	if (n_vifs <= 0)
>> +		return -EINVAL;
>> +
>> +	wiphy_dbg(hw->wiphy,
>> +		  "switch vif channel context mode: %u\n", mode);
>> +
>> +	for (i = 0; i < n_vifs; i++) {
>> +		hwsim_check_chanctx_magic(vifs[i].old_ctx);
>> +		wiphy_dbg(hw->wiphy,
>> +			  "switch vif channel context: %d MHz/width: %d/cfreqs:%d/%d MHz -> %d MHz/width: %d/cfreqs:%d/%d MHz\n",
>> +			  vifs[i].old_ctx->def.chan->center_freq,
>> +			  vifs[i].old_ctx->def.width,
>> +			  vifs[i].old_ctx->def.center_freq1,
>> +			  vifs[i].old_ctx->def.center_freq2,
>> +			  vifs[i].new_ctx->def.chan->center_freq,
>> +			  vifs[i].new_ctx->def.width,
>> +			  vifs[i].new_ctx->def.center_freq1,
>> +			  vifs[i].new_ctx->def.center_freq2);
>> +		hwsim_set_chanctx_magic(vifs[i].new_ctx);
> 
> 
> hwsim_set_chanctx_magic() is only partially correct, I think, this
> depends on the mode? For CHANCTX_SWMODE_REASSIGN_VIF the chanctx should
> already exist as well, so should also be hwsim_check_chanctx_magic() in
> that case?
> 

Oh yeah missed the mode dependency here. Thanks for pointing it out. So 
it should be something like -

...

/* old already exist so check magic */
hwsim_check_chanctx_magic(vifs[i].old_ctx);

if (mode == CHANCTX_SWMODE_REASSIGN_VIF) {
	/* Reassign then new would also exist, just interface is
	 * switching
	 */
	hwsim_check_chanctx_magic(vifs[i].new_ctx);
} else {
	/* SWAP_* then new context does not exist hence set magic.
	 * Also old will no longer exist so clear the magic
	 */
	hwsim_set_chanctx_magic(vifs[i].new_ctx);
	hwsim_clear_chanctx_magic(vifs[i].old_ctx);
}

...

One thing, in patch should I keep those comments or those are noisy? 
Seems noisy to me (if at least mode documentation is referred then 
things are clear already)?