Re: [PATCH 1/3] wifi: cfg80211: add support for link id attribute in NL80211_CMD_DEL_STATION

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

On 1/30/24 16:16, Johannes Berg wrote:
> On Sat, 2024-01-27 at 11:14 +0530, Aditya Kumar Singh wrote:
>> On 1/26/24 14:36, Johannes Berg wrote:
>>> On Thu, 2024-01-25 at 18:28 +0530, Aditya Kumar Singh wrote:
>>>> Currently whenever NL80211_CMD_DEL_STATION command is called without any
>>>> MAC address, all stations present on that interface are flushed.
>>>
>>> True.
>>>
>>>> However with MLO there is a need to flush the stations from a particular
>>>> link in the interface, and not from all the links associated with the MLD
>>>> interface.
>>>
>>> Fair enough, I can get behind that.
>>>
>>> Edit: reading the code - I think I misunderstand that ... you're
>>> actually trying to remove all MLDs ("STATION") that have an active link
>>> on this link?
>>
>> Yes correct. The station might not be MLD station. It could be a legacy
>> station (non EHT) as well.
> 
> We pretty much treat that the same though as an MLD station with a
> single link, with some caveats of translations, no?
> 

Yes correct!

>> Correct, for the first bring up not required but one use case I see is -
>> the hostapd interface was disabled for some reason. While going down, it
>> would have cleared the stations on the kernel but what if for some
>> reason kernel did not clear the station entries and there are some stale
>> entries present? So at next bring up (during enable) it would send the
>> command without any MAC address to flush all stale entries (probably as
>> a safety so that kernel and hostapd would now be on par).
> 
> I don't think this really makes much sense. The kernel can't keep track
> of those stations properly if they're there, and anyway that'd be a
> (pretty massive!) kernel bug?
> 

Yeah :). Even I haven't seen kernel not removing the entries while the 
interface was going down. Was just thinking out loud here. Tbh, even I 
don't know the exact reason it was written in that way. Was guessing.

> Anyway, I think there probably _is_ justification for this (link
> removal?), I'm just not sure this bringup flow really is a good
> justification.
> 

Yes we have.

>>>> Hence, add an option to pass link ID as well in the command so that if link
>>>> ID is passed, station using that passed link ID alone would be deleted
>>>> and others will not be removed.
>>>
>>> So first: Do you want some feature flag that indicates this? Or will we
>>> just eat the cost of kicking out everyone (without even sending deauth
>>> though, I think?) when running on older kernels?
>>>
>>
>> If what I said above was the actual intention, then kicking out everyone
>> without even sending deauth makes sense? Yes? If yes then we don't need
>> a feature flag.
> 
> Does it though? Even if you're talking about init, you could have init
> of one link much delayed for CSA, for example, with stations already
> connected on the other(s).
> 
hmm.. correct.

>>> Secondly: why is this part of NL80211_CMD_DEL_STATION? I'm not convinced
>>> that makes sense. I actually kind of get why you're doing that - it's
>>> easier to retrofit into the existing hostapd, but I don't necessarily
>>> think that the hostap design (problems?) should influence this too much.
>>>
>>> IOW, it would feel much more appropriate to have this as part of
>>> NL80211_CMD_REMOVE_LINK_STA? After all, when going to MLD then "STATION"
>>> now represents a "peer MLD", and "LINK_STA" now represents an affiliated
>>> STA. And flushing all affiliated STAs is what you want.
>>>
>>> So I think it should be NL80211_CMD_REMOVE_LINK_STA without a
>>> NL80211_ATTR_MLD_ADDR.
>>>
>>
>> At least as per the current way of NL80211_CMD_REMOVE_LINK_STA
>> implementation, it did not made any sense to delete all link STAs if
>> MLD_ADDR is not passed. So probably the command should be called as many
>> times as there are active links in the STA?
> 
> Not sure I understand this, we're doing a kind of flush here, so you
> could (conceptually) say "flush all link STAs on link 5", no? And
> obviously stations that have no link left after this need to be removed
> completely.
> 
> Note this raises an interesting point in mac80211, in that there's one
> link ('deflink', the link the STA used to assoc) that cannot be removed
> from an MLD station even.
> 

Good point :) I did not think about this. Let me think again how to 
handle this case. Thanks.


> But again this comes down to what you actually _want_, I think, so I'll
> keep reading for now.
> 
>> Still I feel that NL80211_CMD_DEL_STATION is the proper place to put
>> this? Without the current change also, it used to flush all STAs
>> whenever MAC address is not passed. With MLO, now we need to flush STAs
>> only if it is using the given link ID. So that link STAs from other
>> affiliated links of AP would not be flushed.
> 
> 
> Right so this is coming to the point where I wasn't sure earlier what
> you actually meant, and I'm still not entirely positive I've understood
> it. Let me read on ...
> 
>> Scenario I'm targeting is this -
>>
>> Pre-MLO
>> ----------------------------
>>
>> sdata -> 2 GHz AP interface
>> sta_lists ->
>> 	1. sta -> connected 2 GHz AP sdata
>> 	2. sta -> connected 2 GHz AP sdata
>>
>> After NL80211_CMD_DEL_STATION is given without any MAC address,
>>
>> sta_lists ->
>> 	No entry(ies)
> 
> Right.
> 
>> With MLO
>> -----------------------------
>> sdata ->
>> 	link_data -> 2 GHz AP link (link ID 0)
>> 	link_data -> 5 GHz AP link (link ID 1)
>> 	link_data -> 6 GHz AP link (link ID 2)
>> sta_lists ->
>> 	1. sta -> connected AP MLD sdata
>> 		link_sta 0 -> connected to 2 GHz link
>> 	2. sta -> connected AP MLD sdata
>> 		link_sta 1 -> connected to 5 GHz link
>> 	3. sta -> connected AP MLD sdata
>> 		link_sta 2 -> connected to 6 GHz link
>> 	4. sta -> connected AP MLD sdata
>> 		link_sta 0 -> connected to 2 GHz link
>> 		link_sta 1 -> connected to 5 GHz link
>> 		link_sta 2 -> connected to 6 GHz link
>>
>> Assume 5 GHz goes down and it gives NL80211_CMD_DEL_STATION without any
>> MAC address,
>>
>> sta_lists ->
>> 	No entry(ies)
>>
>> This is not desirable since 5 GHz link went down, why 2/6 GHz STA also
>> got flushed.
>>
>> Hence with the proposed change, only sta #2 and #4 would be flushed
>> since only these two are using passed link ID (which would be 1).
>> Hence after the command,
>>
>> sta_lists ->
>> 	1. sta -> connected AP MLD sdata
>> 		link_sta 0 -> connected to 2 GHz link
>> 	3. sta -> connected AP MLD sdata
>> 		link_sta 2 -> connected to 6 GHz link
> 
> Right, OK.
> 
> So you _are_ indeed wanting to remove all MLDs *entirely*, if they use a
> specific link.
> 

Yes correct.

> Agree that in this case, NL80211_CMD_DEL_STATION with link ID makes
> sense as implemented, but probably need to clarify a little bit overall
> that this is the operation, seeing how I was confused about whether you
> want to remove only the link STAs on on those links, or the entire MLD
> stations.
> 

okay sure, I will try to explain in the commit message as well as in 
code if needed.

> (and yeah, our terminology here is confusing and doesn't help either,
> but that's because we didn't rename STATION to MLD or something
> everywhere)
> 
>> Now, if ML re-config support is present, then hostapd (or the user space
>> controller for that matters), could first issue
>> NL80211_CMD_REMOVE_LINK_STA for the MLD STA (#4) and remove link sta
>> with ID 1 from it. So that when NL80211_CMD_DEL_STATION comes, it would
>> not remove the 2/6 GHz link STA as well from the MLD STA and hence flush
>> the whole entry.
> 
> Right, OK!
> But see above - that NL80211_CMD_REMOVE_LINK_STA as described here may
> or may not be possible today in mac80211.

Sure will check that.

> 
>> The above change is not there yet in hostapd, so for the time being,
>> whole MLD STA would be flushed.
> 
> OK.
> 
>>>> @@ -16827,6 +16840,9 @@ static const struct genl_small_ops nl80211_small_ops[] = {
>>>>    		.validate = GENL_DONT_VALIDATE_STRICT | GENL_DONT_VALIDATE_DUMP,
>>>>    		.doit = nl80211_del_station,
>>>>    		.flags = GENL_UNS_ADMIN_PERM,
>>>> +		/* cannot use NL80211_FLAG_MLO_VALID_LINK_ID, depends on
>>>> +		 * MAC address
>>>> +		 */
>>>>    		.internal_flags = IFLAGS(NL80211_FLAG_NEED_NETDEV_UP),
>>>
>>> Hmm? How does NL80211_FLAG_MLO_VALID_LINK_ID depend on the MAC address?!
>>> It ... doesn't?
>>>
>> I mean intention was that if MAC addresses is passed then no need of
>> link ID. That is why did not add the valid link flag since it would
>> expect the link ID even when MAC address is passed.
>>
> 
> Ah, OK, that makes sense.
> 
> Maybe rephrase that comment? I was also thinking of just refactoring the
> logic into a helper function, but that may be difficult, not sure. It
> just looked similar.
> 

Sure, I will see what I can do here. Thanks for your inputs. Will send 
v2 soon :)