From: Aditya Kumar Singh <quic_adisi@quicinc.com>
To: Johannes Berg <johannes@sipsolutions.net>
Cc: <linux-wireless@vger.kernel.org>
Subject: Re: [PATCH 1/3] wifi: cfg80211: add support for link id attribute in NL80211_CMD_DEL_STATION
Date: Sat, 27 Jan 2024 11:14:22 +0530 [thread overview]
Message-ID: <307eaecc-fd88-4fd8-8857-dd3910257d34@quicinc.com> (raw)
In-Reply-To: <61ad9e10e42c9f114c2a7de534690f8c0133bf58.camel@sipsolutions.net>
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.
> So then maybe disregard all the below, and just write a
> better commit message?
> But I'll leave the below because I'm not really sure what you're trying
> to do here.
>
Sure.
>
>> For example - 2 GHz and 5 GHz are part of an AP MLD. When 2 GHz BSS is
>> brought up, it sends flush command on the interface (MLD). Then eventually
>> 5 GHZ links comes up and that also sends the command on the same interface.
>> Now by the time 5 GHz link comes up, if any station gets connected to 2 GHz
>> link, it would be flushed while 5 GHz link is started which is wrong.
>
> Right. Though in this case - after bringup - you wouldn't really have to
> flush anyway, so it could just not do that, I guess? Feels a bit like a
> broken flow which is a bad justification, but I do understand there's
> justification for this.
>
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).
>> 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.
>
> 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?
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.
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)
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
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.
The above change is not there yet in hostapd, so for the time being,
whole MLD STA would be flushed.
>> A subsequent patch would add logic to delete only the station using the
>> passed link ID.
>
> Not sure I'd say that here - I mean, (1) yeah obviously, otherwise we
> won't apply this patch? and (2) it's not related to cfg80211.
Sure got it.
>
>> case NL80211_IFTYPE_MESH_POINT:
>> @@ -7675,6 +7677,17 @@ static int nl80211_del_station(struct sk_buff *skb, struct genl_info *info)
>> params.reason_code = WLAN_REASON_PREV_AUTH_NOT_VALID;
>> }
>>
>> + /* Link ID not expected in case of non-ML operation */
>> + if (!wdev->valid_links && link_id != -1)
>> + return -EINVAL;
>> +
>> + /* If given, a valid link ID should be passed during MLO */
>> + if (wdev->valid_links && link_id >= 0 &&
>> + !(wdev->valid_links & BIT(link_id)))
>> + return -EINVAL;
>
> Maybe refactor this with the NL80211_FLAG_MLO_VALID_LINK_ID checks?
See comment below -
>
>> @@ -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.
next prev parent reply other threads:[~2024-01-27 5:44 UTC|newest]
Thread overview: 20+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-01-25 12:58 [PATCH 0/3] wifi: cfg80211/mac80211: add support to flush stations based on link ID Aditya Kumar Singh
2024-01-25 12:58 ` [PATCH 1/3] wifi: cfg80211: add support for link id attribute in NL80211_CMD_DEL_STATION Aditya Kumar Singh
2024-01-26 9:06 ` Johannes Berg
2024-01-27 5:44 ` Aditya Kumar Singh [this message]
2024-01-30 10:46 ` Johannes Berg
2024-01-30 11:11 ` Aditya Kumar Singh
2024-01-30 11:54 ` Johannes Berg
2024-01-30 13:49 ` Aditya Kumar Singh
2024-01-25 12:58 ` [PATCH 2/3] wifi: mac80211: add link id argument for sta_flush() function Aditya Kumar Singh
2024-01-26 9:11 ` Johannes Berg
2024-01-27 5:46 ` Aditya Kumar Singh
2024-01-25 12:58 ` [PATCH 3/3] wifi: mac80211: remove only own link stations during stop_ap Aditya Kumar Singh
2024-01-25 12:58 ` [PATCH v5 0/3] wifi: cfg80211/mac80211: add link_id handling in AP channel switch during Multi-Link Operation Aditya Kumar Singh
2024-01-25 12:58 ` [PATCH v5 1/3] wifi: cfg80211: send link id in channel_switch ops Aditya Kumar Singh
2024-01-25 12:58 ` [PATCH v5 2/3] wifi: mac80211: add support for AP channel switch with MLO Aditya Kumar Singh
2024-01-25 12:58 ` [PATCH v5 3/3] wifi: mac80211: update beacon counters per link basis Aditya Kumar Singh
2024-01-25 13:02 ` [PATCH 0/3] wifi: cfg80211/mac80211: add support to flush stations based on link ID Aditya Kumar Singh
-- strict thread matches above, loose matches on Subject: below --
2024-01-24 13:28 Aditya Kumar Singh
2024-01-24 13:28 ` [PATCH 1/3] wifi: cfg80211: add support for link id attribute in NL80211_CMD_DEL_STATION Aditya Kumar Singh
2024-01-24 23:40 ` Jeff Johnson
2024-01-25 3:32 ` Aditya Kumar Singh
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=307eaecc-fd88-4fd8-8857-dd3910257d34@quicinc.com \
--to=quic_adisi@quicinc.com \
--cc=johannes@sipsolutions.net \
--cc=linux-wireless@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).