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? 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.
> 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.
> 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?
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.
> 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.
> 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?
> @@ -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?
johannes
