Re: [PATCH] wifi: mac80211: fix assigning channel in activate links

[Aditya Kumar Singh <quic_adisi@quicinc.com>]

On 10/1/24 12:01, Johannes Berg wrote:
> 
>> +		/*
>> +		 * inform about the link info changed parameters after all
>> +		 * stations are also added
>> +		 */
> 
> I don't understand that comment - you're not doing anything with the
> stations here? And per the commit log it's explicitly _not_ after doing
> the AP station. I'm not sure we should set up everything before the AP
> station?
> 

Okay let me try to explain the situation here -

In the *if (add)* block, drivers are informed about the new links added 
via drv_change_vif_links(). For drivers like ath12k, we know which link 
is changing, but since the channel contexts for the new links aren’t 
available yet, the driver can’t determine which link the firmware should 
create the interface for. This is because we have more than one firmware 
operating under this interface (grouped multiple hardware under single 
wiphy). Therefore, this notification isn’t very helpful in creating the 
link interface now.

Next, in the loop list_for_each_entry(sta, &local->sta_list, list), 
drv_change_sta_links() is called to notify drivers that the links for a 
given station (STA) have changed. Drivers use this callback to create 
the link stations after the STA has moved to the authorized state.
At this point, the driver knows which ML STA and which link STA to 
create. However, to create a link STA, the corresponding link interface 
must exist first.

Currently, it doesn’t, so the driver can’t add the link STA.

Later, in the loop for_each_set_bit(link_id, &add, 
IEEE80211_MLD_MAX_NUM_LINKS), channels are added. At this stage, the 
driver will actually create the link on the interface at its own level. 
Since here using the channel information, appropriate firmware can be 
picked. For example 2 GHz or 5 GHz or 6 GHz firmware.



>> +						  BSS_CHANGED_HE_BSS_COLOR);
>> +
>> +	}
> 
> You make it look like you just moved code but also snuck in a new blank
> line :)
> 

:) sorry about that. Will fix in next version.

> 
>> +
>>   	for_each_set_bit(link_id, &rem, IEEE80211_MLD_MAX_NUM_LINKS) {
>>   		struct ieee80211_link_data *link;
> 
> I also think you put this code too early now - you're now first using
> more channel contexts by way of _ieee80211_link_use_channel() before you
> even release the ones from deactivated ("rem" bitmap in this code)
> links. That doesn't seem like it could work correctly in general.
> 

hmm... yeah true that. May be I will move this once the old links are 
removed?

-- 
Aditya