Received-SPF: pass (google.com: domain of linux-wireless+bounces-3675-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Message-ID: <213c603a-8516-4352-9e38-61ed7bf17a8e@quicinc.com>
Date: Fri, 16 Feb 2024 19:51:12 +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>
 <78c51497-dd6b-4d7a-b789-e3823e89a96a@quicinc.com>
 <e5893311f9a5cf31f2bcb1eff9018e0924f83eba.camel@sipsolutions.net>
From: Aditya Kumar Singh <quic_adisi@quicinc.com>
In-Reply-To: <e5893311f9a5cf31f2bcb1eff9018e0924f83eba.camel@sipsolutions.net>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 7bit

On 2/16/24 12:42, Johannes Berg wrote:
> On Fri, 2024-02-16 at 08:45 +0530, Aditya Kumar Singh wrote:
>>
>>>> +	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)?
>>
> 
> I'm not sure I care all that much, but I'd say even with the reference
> to the modes, it's fairly easy to figure out at least by looking at the
> mode docs?
> 

Yes correct. I will drop inline comments. Not required tbh.

> I'd probably go for a switch () statement on the modes and even WARN on
> invalid mode, while at it? hwsim is a test vehicle, after all.
> 

Yes, good point :). Thanks will incorporate and send v2 soon.