Received-SPF: pass (google.com: domain of linux-wireless+bounces-5266-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:40f1:3f00::1 as permitted sender) client-ip=2604:1380:40f1:3f00::1;
Message-ID: <bc2422eb-efae-423e-b60a-2aa0a3ea94fb@quicinc.com>
Date: Tue, 26 Mar 2024 09:58:26 +0530
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH] wifi: mac80211: validate link status before deciding on
 off channel Tx
Content-Language: en-US
To: Johannes Berg <johannes@sipsolutions.net>
CC: <linux-wireless@vger.kernel.org>, Sriram R <quic_srirrama@quicinc.com>
References: <20240312154620.242773-1-quic_adisi@quicinc.com>
 <2c8338a29d3c42ebbf1fd5efd348c14471fa5004.camel@sipsolutions.net>
From: Aditya Kumar Singh <quic_adisi@quicinc.com>
In-Reply-To: <2c8338a29d3c42ebbf1fd5efd348c14471fa5004.camel@sipsolutions.net>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 7bit

On 3/25/24 20:51, Johannes Berg wrote:
> On Tue, 2024-03-12 at 21:16 +0530, Aditya Kumar Singh wrote:
> 
>> In such cases, the bss active flag might not provide the exact
>> status of the MLD links.
> 
> That's ... true I guess, but then I'm suspicious, why are you sending
> this patch? Does that mean 'active' is managed incorrectly and actually
> becomes false when one link is removed, rather than all?

Yes I believe. I would say it is managing the pre-MLO time data 
structures. As of today, the active flag is set at assign beacon. But 
since it is maintained at sdata level, as soon as first link is assigned 
beacon, the flag would be set. Similarly, at stop_ap() it is set to 
false, so as soon as first link is brought down, it is set to false.

Hence checking that sdata level flag during MLO scenario might be 
misleading.

> Can you fix
> that too? And if you fix that ... yeah we probably still should have
> this patch but ... _without_ this:
> 

Sure let me try to fix that as well. So here's what Im planning -
1. Separate the ether_addr changes into a separate independent patch.
2. Patch series to fix the active flag handling at link level.


>> +	/* This is consolidated status of the MLD or non ML bss */
>> +	if (sdata->bss->active)
>> +		return true;
> 
> I'd think?

Yes :)

> 
>> While at it, when source address is same as the link conf's address and
>> if userspace requested Tx on a specific link, add changes to use the same
>> link id if the link bss is matching the requested channel.
> 
> Why not separate that? It's really not related much?

Yeah will put this in separate patch. Thanks.

> 
>> +	if (link_id < 0)
>> +		return false;
>> +
>> +	if (!sdata->vif.valid_links)
>> +		return false;
>> +
>> +	if (!(sdata->vif.valid_links & BIT(link_id)))
>> +		return false;
> 
> The second condition seems useless then?

Yeah. I would say "if (!sdata->vif.valid_links)" this check can be 
removed. Will remove it.
> But probably better to check *active* links, and then might as well use
> ieee80211_vif_link_active()?

Sure, I will see what I can do here. Thanks for the suggestion.

> 
>> +	link = sdata_dereference(sdata->link[link_id], sdata);
>> +	if (!link)
>> +		return false;
> 
> That might be a WARN_ON()? After all, if links are valid (or active per
> above) that really should be there?
> 

Sure, will do.

>>   int ieee80211_mgmt_tx(struct wiphy *wiphy, struct wireless_dev *wdev,
>>   		      struct cfg80211_mgmt_tx_params *params, u64 *cookie)
>>   {
>> @@ -817,7 +850,7 @@ int ieee80211_mgmt_tx(struct wiphy *wiphy, struct wireless_dev *wdev,
>>   	case NL80211_IFTYPE_P2P_GO:
>>   		if (sdata->vif.type != NL80211_IFTYPE_ADHOC &&
>>   		    !ieee80211_vif_is_mesh(&sdata->vif) &&
>> -		    !sdata->bss->active)
>> +		    !ieee80211_is_link_bss_active(sdata, params->link_id))
>>   			need_offchan = true;
>>   
>>   		rcu_read_lock();
>> @@ -897,8 +930,17 @@ int ieee80211_mgmt_tx(struct wiphy *wiphy, struct wireless_dev *wdev,
>>   				break;
>>   			}
>>   
>> -			if (ether_addr_equal(conf->addr, mgmt->sa))
>> +			if (ether_addr_equal(conf->addr, mgmt->sa)) {
>> +				/* If userspace requested Tx on a specific link
>> +				 * use the same link id if the link bss is matching
>> +				 * the requested chan.
>> +				 */
>> +				if (sdata->vif.valid_links &&
>> +				    params->link_id >= 0 && params->link_id == i &&
>> +				    params->chan == chanctx_conf->def.chan)
>> +					link_id = i;
>>   				break;
>> +			}
>>   
>>   			chanctx_conf = NULL;
>>   		}
>>
>> base-commit: c2b25092864a16c7865e406badedece5cc25fc2b
>