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

[Johannes Berg <johannes@sipsolutions.net>]

On Tue, 2024-10-01 at 12:47 +0530, Aditya Kumar Singh wrote:
> 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().

Sure.

> 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.

Yes, you said that.

> 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.

Yes, I also got that.

> 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.

Right, you said that too :)

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

Right.

But that doesn't explain the *comment*, which literally says:

   inform about the link info changed parameters after all stations are
   also added

but you
 (a) don't add stations here
 (b) if you're thinking about link stations, the link stations are
     only added _after_ this comment and the link info change ...


> 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.

Picking "firmware" sounds very odd here, I'd say you mean "which device
to pick"?

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

I'd think at least that?

But also this seems to break out driver for other reasons, because it
initializes rate control somewhere here and needs a station for that.
Didn't look deeply into that yet though.

johannes