Received-SPF: pass (google.com: domain of linux-wireless+bounces-2593-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: <033151e5-7857-4c44-9b12-d8d8317a4139@quicinc.com>
Date: Sat, 27 Jan 2024 10:28:37 +0530
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH v5 2/3] wifi: mac80211: add support for AP channel switch
 with MLO
Content-Language: en-US
To: Johannes Berg <johannes@sipsolutions.net>
CC: <linux-wireless@vger.kernel.org>
References: <20240125130410.827701-1-quic_adisi@quicinc.com>
 <20240125130410.827701-3-quic_adisi@quicinc.com>
 <d4e77541479560147d453b7b5c06de3f940bad35.camel@sipsolutions.net>
From: Aditya Kumar Singh <quic_adisi@quicinc.com>
In-Reply-To: <d4e77541479560147d453b7b5c06de3f940bad35.camel@sipsolutions.net>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 7bit

On 1/26/24 15:01, Johannes Berg wrote:
> On Thu, 2024-01-25 at 18:34 +0530, Aditya Kumar Singh wrote:
>> Currently, during channel switch, deflink (or link_id 0)
> 
> The parenthetical is wrong: deflink isn't even a valid/used link *at
> all* in MLD data structures. Speaking about "link_id 0" would be a valid
> thing to do even for MLD, but speaking about "deflink" isn't.
> 

Okay got it. Will address in next version.

> 
>> +++ b/drivers/net/wireless/virtual/mac80211_hwsim.c
>> @@ -2285,7 +2285,7 @@ static void mac80211_hwsim_beacon_tx(void *arg, u8 *mac,
>>   	}
>>   
>>   	if (link_conf->csa_active && ieee80211_beacon_cntdwn_is_complete(vif))
>> -		ieee80211_csa_finish(vif);
>> +		ieee80211_csa_finish(vif, link_id);
> 
> Might make sense to keep the actual logic changes out of this patch?
> It's pretty big, and this is pretty hidden...
> 

Sure so I will move this ieee80211_csa_finish() changes to a different 
patch.

>> +++ b/net/mac80211/cfg.c
>> @@ -1588,6 +1588,8 @@ static int ieee80211_stop_ap(struct wiphy *wiphy, struct net_device *dev,
>>   		link->csa_block_tx = false;
>>   	}
>>   
>> +	wiphy_work_cancel(wiphy, &link->csa_finalize_work);
> 
> You don't _really_ need that here (if it runs, it'll just find
> csa_active==false), and I'd feel better about it if we actually had this
> in ieee80211_link_stop() in some way, that's called from
> ieee80211_tear_down_links() and then it's really obvious to see that
> this is removed before freeing the link.
> 

Yeah makes sense. Let me see what I can do here. Thanks for the inputs! :)

>> -	if (sdata->vif.bss_conf.eht_puncturing != sdata->vif.bss_conf.csa_punct_bitmap) {
>> -		sdata->vif.bss_conf.eht_puncturing =
>> -					sdata->vif.bss_conf.csa_punct_bitmap;
>> +	if (link_conf->eht_puncturing != link_conf->csa_punct_bitmap) {
>> +		link_conf->eht_puncturing = link_conf->csa_punct_bitmap;
>>   		changed |= BSS_CHANGED_EHT_PUNCTURING;
>>   	}
> 
> Hm. I'm going to send some patches soon that remove the puncturing stuff
> and move it into the chandef (as we discussed elsewhere), but just
> noting that - doesn't need to concern you here, I think.
> 

Oh okay! Let's see if at all it conflicts, will send a re-based once 
those gets merged.

>> @@ -3875,16 +3894,23 @@ __ieee80211_channel_switch(struct wiphy *wiphy, struct net_device *dev,
>>   	if (sdata->wdev.cac_started)
>>   		return -EBUSY;
>>   
>> -	if (cfg80211_chandef_identical(&params->chandef,
>> -				       &sdata->vif.bss_conf.chandef))
>> +	if (WARN_ON(link_id >= IEEE80211_MLD_MAX_NUM_LINKS))
>> +		return -EINVAL;
>> +
>> +	link_data = wiphy_dereference(wiphy, sdata->link[link_id]);
>> +	if (!link_data)
>> +		return -ENOLINK;
>> +
>> +	link_conf = link_data->conf;
>> +
>> +	if (cfg80211_chandef_identical(&params->chandef, &link_conf->chandef))
>>   		return -EINVAL;
> 
> Also another thing unrelated to your patch - I'm thinking about removing
> that identical() check entirely - you might want to switch to the same
> channel with quiet=1. At least for testing that'd be really useful, and
> I don't think it really serves any purpose to forbid it.
> 
Yeah, we can do. But is there any actual use case? Also, what if some 
notorious user space application simply sends NL command without even 
quiet=1? There should be some check I guess?