diff mbox series

[10/27] wifi: mac80211: isolate driver from inactive links

Message ID 20220902161143.5ce3dad3be7c.I92e9f7a6c120cd4a3631baf486ad8b6aafcd796f@changeid
State New
Headers show
Series another set of MLO patches | expand

Commit Message

Johannes Berg Sept. 2, 2022, 2:12 p.m. UTC 
From: Johannes Berg <johannes.berg@intel.com>

In order to let the driver select active links and properly
make multi-link connections, as a first step isolate the
driver from inactive links, and set the active links to be
only the association link for client-side interfaces. For
AP side nothing changes since APs always have to have all
their links active.

To simplify things, update the for_each_sta_active_link()
API to include the appropriate vif pointer.

This also implies not allocating a chanctx for an inactive
link, which requires a few more changes.

Since we now no longer try to program multiple links to the
driver, remove the check in the MLME code.

Signed-off-by: Johannes Berg <johannes.berg@intel.com>
---
 include/net/mac80211.h    |  30 +++----
 net/mac80211/chan.c       |   6 ++
 net/mac80211/driver-ops.c | 172 ++++++++++++++++++++++++++++++++++++++
 net/mac80211/driver-ops.h | 165 ++++++------------------------------
 net/mac80211/key.c        |   8 ++
 net/mac80211/link.c       |  66 ++++++++++++---
 net/mac80211/mlme.c       |  25 ++----
 net/mac80211/util.c       |   2 +-
 8 files changed, 286 insertions(+), 188 deletions(-)

Comments

Wen Gong Sept. 8, 2022, 3:23 p.m. UTC | #1 
On 9/2/2022 10:12 PM, Johannes Berg wrote:
> From: Johannes Berg <johannes.berg@intel.com>
>
> In order to let the driver select active links and properly
> make multi-link connections, as a first step isolate the
> driver from inactive links, and set the active links to be
> only the association link for client-side interfaces. For
> AP side nothing changes since APs always have to have all
> their links active.
>
> To simplify things, update the for_each_sta_active_link()
> API to include the appropriate vif pointer.
>
> This also implies not allocating a chanctx for an inactive
> link, which requires a few more changes.
>
> Since we now no longer try to program multiple links to the
> driver, remove the check in the MLME code.
>
> Signed-off-by: Johannes Berg <johannes.berg@intel.com>
> ---
>   include/net/mac80211.h    |  30 +++----
>   net/mac80211/chan.c       |   6 ++
>   net/mac80211/driver-ops.c | 172 ++++++++++++++++++++++++++++++++++++++
>   net/mac80211/driver-ops.h | 165 ++++++------------------------------
>   net/mac80211/key.c        |   8 ++
>   net/mac80211/link.c       |  66 ++++++++++++---
>   net/mac80211/mlme.c       |  25 ++----
>   net/mac80211/util.c       |   2 +-
>   8 files changed, 286 insertions(+), 188 deletions(-)
>
> diff --git a/include/net/mac80211.h b/include/net/mac80211.h
> index d4e1d73d88cc..20a2f25a38fa 100644
> --- a/include/net/mac80211.h
> +++ b/include/net/mac80211.h
> @@ -1799,6 +1799,9 @@ struct ieee80211_vif_cfg {
>    * @link_conf: in case of MLD, the per-link BSS configuration,
>    *	indexed by link ID
>    * @valid_links: bitmap of valid links, or 0 for non-MLO.
> + * @active_links: The bitmap of active links, or 0 for non-MLO.
> + *	The driver shouldn't change this directly, but use the
> + *	API calls meant for that purpose.
>    * @addr: address of this interface
>    * @p2p: indicates whether this AP or STA interface is a p2p
>    *	interface, i.e. a GO or p2p-sta respectively
> @@ -1834,7 +1837,7 @@ struct ieee80211_vif {
>   	struct ieee80211_vif_cfg cfg;
>   	struct ieee80211_bss_conf bss_conf;
>   	struct ieee80211_bss_conf __rcu *link_conf[IEEE80211_MLD_MAX_NUM_LINKS];
> -	u16 valid_links;
> +	u16 valid_links, active_links;
>   	u8 addr[ETH_ALEN] __aligned(2);
>   	bool p2p;
>   
...
> @@ -123,11 +132,38 @@ static int ieee80211_check_dup_link_addrs(struct ieee80211_sub_if_data *sdata)
>   	return 0;
>   }
>   
> +static void ieee80211_set_vif_links_bitmaps(struct ieee80211_sub_if_data *sdata,
> +					    u16 links)
> +{
> +	sdata->vif.valid_links = links;
> +
> +	if (!links) {
> +		sdata->vif.active_links = 0;
> +		return;
> +	}
> +
> +	switch (sdata->vif.type) {
> +	case NL80211_IFTYPE_AP:
> +		/* in an AP all links are always active */
> +		sdata->vif.active_links = links;
> +		break;
> +	case NL80211_IFTYPE_STATION:
> +		if (sdata->vif.active_links)
> +			break;
> +		WARN_ON(hweight16(links) > 1);
> +		sdata->vif.active_links = links;
> +		break;
> +	default:
> +		WARN_ON(1);
> +	}
> +}
> +
Now I found it only active the primay link(the link for 
authentication/assoc request) in my station MLO test,
change_vif_links of struct ieee80211_ops *ops of driver will only be 
called one time for the primary link.
it means only one link for MLO.
I plan to revert this patch in my local test now.

Will you implement muti-links later?
> ...
Johannes Berg Sept. 8, 2022, 3:36 p.m. UTC | #2 
On Thu, 2022-09-08 at 23:23 +0800, Wen Gong wrote:
> 
> Now I found it only active the primay link(the link for 
> authentication/assoc request) in my station MLO test,

Yes, that's intentional. It gives the driver choice about which links to
activate; first of all because we don't have interface/link combinations
stuff yet (waiting for your side on that), and secondly because we might
very well (want to) negotiate more links than we can concurrently have
active, e.g. a NIC that can have two active might still want to
negotiate four and switch dynamically.

> change_vif_links of struct ieee80211_ops *ops of driver will only be 
> called one time for the primary link.

Correct.

> it means only one link for MLO.

Right.

> I plan to revert this patch in my local test now.
> 
> Will you implement muti-links later?

Yes. I have patches pending to add API that the driver can call to pick
the active links (as a bitmap).

I'll send it out when I can, likely tomorrow.

johannes
>
Wen Gong Sept. 8, 2022, 3:51 p.m. UTC | #3 
On 9/8/2022 11:36 PM, Johannes Berg wrote:
> On Thu, 2022-09-08 at 23:23 +0800, Wen Gong wrote:
>> Now I found it only active the primay link(the link for
>> authentication/assoc request) in my station MLO test,
> Yes, that's intentional. It gives the driver choice about which links to
> activate; first of all because we don't have interface/link combinations
> stuff yet (waiting for your side on that), and secondly because we might
> very well (want to) negotiate more links than we can concurrently have
> active, e.g. a NIC that can have two active might still want to
> negotiate four and switch dynamically.
>
>> change_vif_links of struct ieee80211_ops *ops of driver will only be
>> called one time for the primary link.
> Correct.
>
>> it means only one link for MLO.
> Right.
>
>> I plan to revert this patch in my local test now.
>>
>> Will you implement muti-links later?
> Yes. I have patches pending to add API that the driver can call to pick
> the active links (as a bitmap).
>
> I'll send it out when I can, likely tomorrow.
>
> johannes
Thanks.
Another thing is what is the local MLD addr and local primary link(send 
authentication/assoc requset) addr relation?
I think they are same address for station, right?

And the others local link address is random generated by eth_random_addr 
in ieee80211_mgd_assoc() , right?
Johannes Berg Sept. 8, 2022, 3:52 p.m. UTC | #4 
On Thu, 2022-09-08 at 23:51 +0800, Wen Gong wrote:
> 
> Another thing is what is the local MLD addr and local primary link(send 
> authentication/assoc requset) addr relation?
> I think they are same address for station, right?

No, they aren't, and shouldn't be.

> And the others local link address is random generated by eth_random_addr 
> in ieee80211_mgd_assoc() , right?

Yes, at least for now all the link addresses are randomly generated.

johannes
Wen Gong Sept. 9, 2022, 4:16 a.m. UTC | #5 
On 9/8/2022 11:52 PM, Johannes Berg wrote:
> On Thu, 2022-09-08 at 23:51 +0800, Wen Gong wrote:
>> Another thing is what is the local MLD addr and local primary link(send
>> authentication/assoc requset) addr relation?
>> I think they are same address for station, right?
> No, they aren't, and shouldn't be.
IEEE P802.11be™/D2.0
35.3.3 Multi-link device addressing
An MLD has an MLD MAC address that singly identifies the MLD.
Each STA affiliated with an MLD shall have a different MAC address.
NOTE 1—The MLD MAC address of an MLD might be the same as the MAC 
address of one affiliated STA or different
from the MAC address of any affiliated STA.

This means the MLD address can be same with one link.

I suggest to set primary link local addr same with MLD address for station.
reason is:
When station up, one link interface of driver will be created with the 
addr of struct ieee80211_vif,
it is used for scan and non-MLO connection.
If station start to do MLO connection now, then random local link addr 
will be generated by below call stack.
for the 1st link. This lead driver must change the link interface local 
address to this random addr.
After disconnect MLO connection, driver also need to change the link 
interface local address back to
addr of struct ieee80211_vif. It increased the complexity and driver 
need to sync the link interface
if this is a scan running at this moment.

ieee80211_mgd_auth()
     ->ieee80211_prep_connection()
         ->ieee80211_vif_set_links()
             ->ieee80211_vif_update_links()
                 ->ieee80211_link_setup()
                     ->ieee80211_mgd_setup_link()
eth_random_addr(link->conf->addr);//sdata->u.mgd.assoc_data is null at 
this point
>> And the others local link address is random generated by eth_random_addr
>> in ieee80211_mgd_assoc() , right?
> Yes, at least for now all the link addresses are randomly generated.
>
> johannes
>
Johannes Berg Sept. 9, 2022, 7:28 a.m. UTC | #6 
Hi,

> > No, they aren't, and shouldn't be.

> IEEE P802.11be™/D2.0
> 35.3.3 Multi-link device addressing

> An MLD has an MLD MAC address that singly identifies the MLD.
> Each STA affiliated with an MLD shall have a different MAC address.
> NOTE 1—The MLD MAC address of an MLD might be the same as the MAC 
> address of one affiliated STA or different
> from the MAC address of any affiliated STA.

Right. I was over-simplifying, that was basically the "tl;dr" version of
my statement, without the longer one ;-)

> This means the MLD address can be same with one link.

True.

> I suggest to set primary link local addr same with MLD address for station.

I wouldn't suggest that, but YMMV.

> reason is:
> When station up, one link interface of driver will be created with the 
> addr of struct ieee80211_vif,
> it is used for scan and non-MLO connection.
> If station start to do MLO connection now, then random local link addr 
> will be generated by below call stack.
> for the 1st link. This lead driver must change the link interface local 
> address to this random addr.

Well, that depends how you treat "address of an interface", no? I don't
think there's really any need to "install" a MAC address to the NIC
until you even start any kind of operation.

True, if you cannot scan using the MLD address while you also have a
different link address, you might be in trouble - but I find this
unrealistic because you would want to be able to scan on any part of the
hardware that is doing any of the links?


In any case, changing this makes the receive logic a bit different. You
would have to ensure that your driver does indeed indicate the link a
frame was received on, I think? Also, ieee80211_rx_for_interface() might
have to change, something like the below maybe?

If we just change the first link's address to the same as the MLD
address without any changes then the code without the changes below
would overwrite the link ID because it can find the link STA address,
even if the device already did address translation. Of course this is
only relevant if it does address translation w/o indicating the link,
which it shouldn't ... hence the patch.

In any case, I expect this will end up being some kind of driver policy,
so I can imagine that we could make a relatively simple patch with a new
method to let drivers set the link address that gets used. It cannot be
changing the link address when it's added to the driver since this patch
that this thread is based on means the driver doesn't get to know about
the links until it's far too late (and even before this patch, the links
were only created after assoc, when the link addresses were already sent
to the AP)

johannes



diff --git a/include/net/mac80211.h b/include/net/mac80211.h
index ac2bad57933f..648b2de8dd3e 100644
--- a/include/net/mac80211.h
+++ b/include/net/mac80211.h
@@ -1482,7 +1482,8 @@ enum mac80211_rx_encoding {
  * @ampdu_delimiter_crc: A-MPDU delimiter CRC
  * @zero_length_psdu_type: radiotap type of the 0-length PSDU
  * @link_valid: if the link which is identified by @link_id is valid. This flag
- *	is set only when connection is MLO.
+ *	is set only when connection is MLO. Note that setting this also implies
+ *	address translation was done.
  * @link_id: id of the link used to receive the packet. This is used along with
  *	@link_valid.
  */
diff --git a/net/mac80211/rx.c b/net/mac80211/rx.c
index a57811372027..963de5d880d7 100644
--- a/net/mac80211/rx.c
+++ b/net/mac80211/rx.c
@@ -4946,22 +4946,24 @@ static void __ieee80211_rx_handle_8023(struct ieee80211_hw *hw,
 static bool ieee80211_rx_for_interface(struct ieee80211_rx_data *rx,
 				       struct sk_buff *skb, bool consume)
 {
-	struct link_sta_info *link_sta;
+	struct ieee80211_rx_status *status = IEEE80211_SKB_RXCB(skb);
 	struct ieee80211_hdr *hdr = (void *)skb->data;
+	struct link_sta_info *link_sta = NULL;
 
 	/*
-	 * Look up link station first, in case there's a
-	 * chance that they might have a link address that
-	 * is identical to the MLD address, that way we'll
-	 * have the link information if needed.
+	 * Unless the driver did addr translation and provided the link
+	 * ID, look up link station first. Note that if we get a frame
+	 * without link ID in the status and the device happens to use
+	 * identical addresses for one of the links and the MLD, then
+	 * we cannot identify whether it was translated already or not.
 	 */
-	link_sta = link_sta_info_get_bss(rx->sdata, hdr->addr2);
+	if (!status->link_valid)
+		link_sta = link_sta_info_get_bss(rx->sdata, hdr->addr2);
+
 	if (link_sta) {
 		rx->sta = link_sta->sta;
 		rx->link_id = link_sta->link_id;
 	} else {
-		struct ieee80211_rx_status *status = IEEE80211_SKB_RXCB(skb);
-
 		rx->sta = sta_info_get_bss(rx->sdata, hdr->addr2);
 		if (rx->sta) {
 			if (status->link_valid &&
Wen Gong Sept. 9, 2022, 8:38 a.m. UTC | #7 
On 9/9/2022 3:28 PM, Johannes Berg wrote:
> Hi,
>
>>> No, they aren't, and shouldn't be.
>> IEEE P802.11be™/D2.0
>> 35.3.3 Multi-link device addressing
>> An MLD has an MLD MAC address that singly identifies the MLD.
>> Each STA affiliated with an MLD shall have a different MAC address.
>> NOTE 1—The MLD MAC address of an MLD might be the same as the MAC
>> address of one affiliated STA or different
>> from the MAC address of any affiliated STA.
> Right. I was over-simplifying, that was basically the "tl;dr" version of
> my statement, without the longer one ;-)
>
>> This means the MLD address can be same with one link.
> True.
>
>> I suggest to set primary link local addr same with MLD address for station.
> I wouldn't suggest that, but YMMV.
>
>> reason is:
>> When station up, one link interface of driver will be created with the
>> addr of struct ieee80211_vif,
>> it is used for scan and non-MLO connection.
>> If station start to do MLO connection now, then random local link addr
>> will be generated by below call stack.
>> for the 1st link. This lead driver must change the link interface local
>> address to this random addr.
> Well, that depends how you treat "address of an interface", no? I don't
> think there's really any need to "install" a MAC address to the NIC
> until you even start any kind of operation.
>
> True, if you cannot scan using the MLD address while you also have a
> different link address, you might be in trouble - but I find this
> unrealistic because you would want to be able to scan on any part of the
> hardware that is doing any of the links?
Scan probe request needs the local address, so we must fill one address 
to it.
And we use the same local address to scan for 2.4 GHz/5 GHz/6 GHz band.
>
>
> In any case, changing this makes the receive logic a bit different. You
> would have to ensure that your driver does indeed indicate the link a
> frame was received on, I think? Also, ieee80211_rx_for_interface() might
> have to change, something like the below maybe?
I looked the ieee80211_rx_for_interface(), it is to find struct 
link_sta_info with the source
address of an rx frame. For station, the hdr->addr2 means the address of 
the AP, so
the the change of mac80211/wireless will not effect the 
ieee80211_rx_for_interface().
Because it is the MLD/link address of the AP(maybe it is same addr for 
MLD/one link) when we use as station.
>
> If we just change the first link's address to the same as the MLD
> address without any changes then the code without the changes below
> would overwrite the link ID because it can find the link STA address,
> even if the device already did address translation. Of course this is
> only relevant if it does address translation w/o indicating the link,
> which it shouldn't ... hence the patch.
>
> In any case, I expect this will end up being some kind of driver policy,
> so I can imagine that we could make a relatively simple patch with a new
> method to let drivers set the link address that gets used. It cannot be
> changing the link address when it's added to the driver since this patch
> that this thread is based on means the driver doesn't get to know about
> the links until it's far too late (and even before this patch, the links
> were only created after assoc, when the link addresses were already sent
> to the AP)
>
> johannes
>
>
>
> diff --git a/include/net/mac80211.h b/include/net/mac80211.h
> index ac2bad57933f..648b2de8dd3e 100644
> --- a/include/net/mac80211.h
> +++ b/include/net/mac80211.h
> @@ -1482,7 +1482,8 @@ enum mac80211_rx_encoding {
>    * @ampdu_delimiter_crc: A-MPDU delimiter CRC
>    * @zero_length_psdu_type: radiotap type of the 0-length PSDU
>    * @link_valid: if the link which is identified by @link_id is valid. This flag
> - *	is set only when connection is MLO.
> + *	is set only when connection is MLO. Note that setting this also implies
> + *	address translation was done.
>    * @link_id: id of the link used to receive the packet. This is used along with
>    *	@link_valid.
>    */
> diff --git a/net/mac80211/rx.c b/net/mac80211/rx.c
> index a57811372027..963de5d880d7 100644
> --- a/net/mac80211/rx.c
> +++ b/net/mac80211/rx.c
> @@ -4946,22 +4946,24 @@ static void __ieee80211_rx_handle_8023(struct ieee80211_hw *hw,
>   static bool ieee80211_rx_for_interface(struct ieee80211_rx_data *rx,
>   				       struct sk_buff *skb, bool consume)
>   {
> -	struct link_sta_info *link_sta;
> +	struct ieee80211_rx_status *status = IEEE80211_SKB_RXCB(skb);
>   	struct ieee80211_hdr *hdr = (void *)skb->data;
> +	struct link_sta_info *link_sta = NULL;
>   
>   	/*
> -	 * Look up link station first, in case there's a
> -	 * chance that they might have a link address that
> -	 * is identical to the MLD address, that way we'll
> -	 * have the link information if needed.
> +	 * Unless the driver did addr translation and provided the link
> +	 * ID, look up link station first. Note that if we get a frame
> +	 * without link ID in the status and the device happens to use
> +	 * identical addresses for one of the links and the MLD, then
> +	 * we cannot identify whether it was translated already or not.
>   	 */
> -	link_sta = link_sta_info_get_bss(rx->sdata, hdr->addr2);
> +	if (!status->link_valid)
> +		link_sta = link_sta_info_get_bss(rx->sdata, hdr->addr2);
> +
>   	if (link_sta) {
>   		rx->sta = link_sta->sta;
>   		rx->link_id = link_sta->link_id;
>   	} else {
> -		struct ieee80211_rx_status *status = IEEE80211_SKB_RXCB(skb);
> -
>   		rx->sta = sta_info_get_bss(rx->sdata, hdr->addr2);
>   		if (rx->sta) {
>   			if (status->link_valid &&
Thanks.
Below patch has said driver should report link id if addr translated.

https://git.kernel.org/pub/scm/linux/kernel/git/wireless/wireless-next.git/commit/?h=mld&id=ea9d807b56428d65cf43030cbd7ae5a580077147
wifi: mac80211: add link information in ieee80211_rx_status
In MLO, when the address translation from link to MLD is done
in fw/hw, it is necessary to be able to have some information
on the link on which the frame has been received. Extend the
rx API to include link_id and a valid flag in ieee80211_rx_status.
Also make chanes to mac80211 rx APIs to make use of the reported
link_id after sanity checks.
Wen Gong Sept. 9, 2022, 8:58 a.m. UTC | #8 
On 9/9/2022 3:28 PM, Johannes Berg wrote:
> Hi,
>
>>> No, they aren't, and shouldn't be.
>> IEEE P802.11be™/D2.0
>> 35.3.3 Multi-link device addressing
>> An MLD has an MLD MAC address that singly identifies the MLD.
>> Each STA affiliated with an MLD shall have a different MAC address.
>> NOTE 1—The MLD MAC address of an MLD might be the same as the MAC
>> address of one affiliated STA or different
>> from the MAC address of any affiliated STA.
> Right. I was over-simplifying, that was basically the "tl;dr" version of
> my statement, without the longer one ;-)
>
>> This means the MLD address can be same with one link.
> True.
>
>> I suggest to set primary link local addr same with MLD address for station.
> I wouldn't suggest that, but YMMV.
>
>> reason is:
>> When station up, one link interface of driver will be created with the
>> addr of struct ieee80211_vif,
>> it is used for scan and non-MLO connection.
>> If station start to do MLO connection now, then random local link addr
>> will be generated by below call stack.
>> for the 1st link. This lead driver must change the link interface local
>> address to this random addr.
> Well, that depends how you treat "address of an interface", no? I don't
> think there's really any need to "install" a MAC address to the NIC
> until you even start any kind of operation.
>
> True, if you cannot scan using the MLD address while you also have a
> different link address, you might be in trouble - but I find this
> unrealistic because you would want to be able to scan on any part of the
> hardware that is doing any of the links?
Scan probe request needs the local address, so we must fill one address 
to it.
And we use the same local address to scan for 2.4 GHz/5 GHz/6 GHz band.
>
>
> In any case, changing this makes the receive logic a bit different. You
> would have to ensure that your driver does indeed indicate the link a
> frame was received on, I think? Also, ieee80211_rx_for_interface() might
> have to change, something like the below maybe?
I looked the ieee80211_rx_for_interface(), it is to find struct 
link_sta_info with the source
address of an rx frame. For station, the hdr->addr2 means the address of 
the AP, so
the the change of mac80211/wireless will not effect the 
ieee80211_rx_for_interface().
Because it is the MLD/link address of the AP(maybe it is same addr for 
MLD/one link) when we as station.
> If we just change the first link's address to the same as the MLD
> address without any changes then the code without the changes below
> would overwrite the link ID because it can find the link STA address,
> even if the device already did address translation. Of course this is
> only relevant if it does address translation w/o indicating the link,
> which it shouldn't ... hence the patch.
>
> In any case, I expect this will end up being some kind of driver policy,
> so I can imagine that we could make a relatively simple patch with a new
> method to let drivers set the link address that gets used. It cannot be
> changing the link address when it's added to the driver since this patch
> that this thread is based on means the driver doesn't get to know about
> the links until it's far too late (and even before this patch, the links
> were only created after assoc, when the link addresses were already sent
> to the AP)
Thanks for the incoming  new method to let drivers set the link address.
It is better to let driver to fill all the links' address in load phase.
And then it never change again. And one of the address array is always
used for primary link.
>
> johannes
>
>
>
> diff --git a/include/net/mac80211.h b/include/net/mac80211.h
> index ac2bad57933f..648b2de8dd3e 100644
> --- a/include/net/mac80211.h
> +++ b/include/net/mac80211.h
> @@ -1482,7 +1482,8 @@ enum mac80211_rx_encoding {
>    * @ampdu_delimiter_crc: A-MPDU delimiter CRC
>    * @zero_length_psdu_type: radiotap type of the 0-length PSDU
>    * @link_valid: if the link which is identified by @link_id is valid. This flag
> - *	is set only when connection is MLO.
> + *	is set only when connection is MLO. Note that setting this also implies
> + *	address translation was done.
>    * @link_id: id of the link used to receive the packet. This is used along with
>    *	@link_valid.
>    */
> diff --git a/net/mac80211/rx.c b/net/mac80211/rx.c
> index a57811372027..963de5d880d7 100644
> --- a/net/mac80211/rx.c
> +++ b/net/mac80211/rx.c
> @@ -4946,22 +4946,24 @@ static void __ieee80211_rx_handle_8023(struct ieee80211_hw *hw,
>   static bool ieee80211_rx_for_interface(struct ieee80211_rx_data *rx,
>   				       struct sk_buff *skb, bool consume)
>   {
> -	struct link_sta_info *link_sta;
> +	struct ieee80211_rx_status *status = IEEE80211_SKB_RXCB(skb);
>   	struct ieee80211_hdr *hdr = (void *)skb->data;
> +	struct link_sta_info *link_sta = NULL;
>   
>   	/*
> -	 * Look up link station first, in case there's a
> -	 * chance that they might have a link address that
> -	 * is identical to the MLD address, that way we'll
> -	 * have the link information if needed.
> +	 * Unless the driver did addr translation and provided the link
> +	 * ID, look up link station first. Note that if we get a frame
> +	 * without link ID in the status and the device happens to use
> +	 * identical addresses for one of the links and the MLD, then
> +	 * we cannot identify whether it was translated already or not.
>   	 */
> -	link_sta = link_sta_info_get_bss(rx->sdata, hdr->addr2);
> +	if (!status->link_valid)
> +		link_sta = link_sta_info_get_bss(rx->sdata, hdr->addr2);
> +
>   	if (link_sta) {
>   		rx->sta = link_sta->sta;
>   		rx->link_id = link_sta->link_id;
>   	} else {
> -		struct ieee80211_rx_status *status = IEEE80211_SKB_RXCB(skb);
> -
>   		rx->sta = sta_info_get_bss(rx->sdata, hdr->addr2);
>   		if (rx->sta) {
>   			if (status->link_valid &&
Thanks.
Below patch has said driver should report link id if addr translated.

https://git.kernel.org/pub/scm/linux/kernel/git/wireless/wireless-next.git/commit/?h=mld&id=ea9d807b56428d65cf43030cbd7ae5a580077147
wifi: mac80211: add link information in ieee80211_rx_status
In MLO, when the address translation from link to MLD is done
in fw/hw, it is necessary to be able to have some information
on the link on which the frame has been received. Extend the
rx API to include link_id and a valid flag in ieee80211_rx_status.
Also make chanes to mac80211 rx APIs to make use of the reported
link_id after sanity checks.
Wen Gong Sept. 28, 2022, 3:20 p.m. UTC | #9 
Hi johannes,

May I know some more info/status about the "incoming  new method to let 
drivers set the link address"?

On 9/9/2022 4:58 PM, Wen Gong wrote:
> On 9/9/2022 3:28 PM, Johannes Berg wrote:
>
...
>> If we just change the first link's address to the same as the MLD
>> address without any changes then the code without the changes below
>> would overwrite the link ID because it can find the link STA address,
>> even if the device already did address translation. Of course this is
>> only relevant if it does address translation w/o indicating the link,
>> which it shouldn't ... hence the patch.
>>
>> In any case, I expect this will end up being some kind of driver policy,
>> so I can imagine that we could make a relatively simple patch with a new
>> method to let drivers set the link address that gets used. It cannot be
>> changing the link address when it's added to the driver since this patch
>> that this thread is based on means the driver doesn't get to know about
>> the links until it's far too late (and even before this patch, the links
>> were only created after assoc, when the link addresses were already sent
>> to the AP)
> Thanks for the incoming  new method to let drivers set the link address.
> It is better to let driver to fill all the links' address in load phase.
> And then it never change again. And one of the address array is always
> used for primary link.
>
...
Johannes Berg Sept. 28, 2022, 3:28 p.m. UTC | #10 
Hi,

Sorry - still catching up from PF related matters ...

> May I know some more info/status about the "incoming  new method to let 
> drivers set the link address"?
> 

I wasn't actually planning to work on that myself, FWIW.

johannes
Wen Gong Oct. 11, 2022, 4:07 a.m. UTC | #11 
On 9/28/2022 11:28 PM, Johannes Berg wrote:
...
>
>> May I know some more info/status about the "incoming  new method to let
>> drivers set the link address"?
>>
> I wasn't actually planning to work on that myself, FWIW.
>
> johannes

OK. So has some body will work for that now?😁
Johannes Berg Oct. 11, 2022, 7:26 a.m. UTC | #12 
On Tue, 2022-10-11 at 12:07 +0800, Wen Gong wrote:
> On 9/28/2022 11:28 PM, Johannes Berg wrote:
> ...
> > 
> > > May I know some more info/status about the "incoming  new method to let
> > > drivers set the link address"?
> > > 
> > I wasn't actually planning to work on that myself, FWIW.
> > 
> > johannes
> 
> OK. So has some body will work for that now?😁
> 

Yes, I don't personally have a need for anything other than what we have
right now.

Btw, I also merged pretty much all the things into wireless-next now, I
think only maybe some debugfs updates are still not upstream, and a few
minor bugfixes perhaps.

johannes
Johannes Berg April 11, 2023, 7:32 a.m. UTC | #13 
On Tue, 2023-04-04 at 10:54 +0800, Wen Gong wrote:
> On 10/11/2022 3:26 PM, Johannes Berg wrote:
> > On Tue, 2022-10-11 at 12:07 +0800, Wen Gong wrote:
> > > On 9/28/2022 11:28 PM, Johannes Berg wrote:
> > > ...
> > > > > May I know some more info/status about the "incoming  new method to let
> > > > > drivers set the link address"?
> > > > > 
> > > > I wasn't actually planning to work on that myself, FWIW.
> > > > 
> > > > johannes
> > > OK. So has some body will work for that now?😁
> > > 
> > Yes, I don't personally have a need for anything other than what we have
> > right now.
> 
> May I add method to let low-drivers set the primay link address like below?

> I add a field in struct wiphy_iftype_ext_capab, if it is valid, then it 
> will be used as
> 
> local primary/assoc link addr in function ieee80211_mgd_setup_link() for 
> station.
> 

I don't really think that it makes sense to push this to cfg80211 when
we only need it in mac80211?

johannes
Johannes Berg April 11, 2023, 7:38 a.m. UTC | #14 
On Tue, 2023-04-04 at 11:28 +0800, Wen Gong wrote:
> 
> May I also add a field such as "u16 active_links_count" in struct 
> wiphy_iftype_ext_capab,
> and add logic in function ieee80211_set_vif_links_bitmaps() for station 
> like this ?:
> if (active_links_count && hweight16(links) <= active_links_count)
>      then sdata->vif.active_links = links;
> 

Also here, not sure it makes sense in cfg80211 level?

Though I'm not sure what the idea here is at all - you can refuse to
link switch etc, what would you use this for?

Then again, we haven't really designed out all the link selection stuff,
do we want wpa_s to do it, driver to do it, etc.? Hence debugfs. So
depending on what end up doing there, we will obviously need to
advertise some level of link-concurrency to userspace.

johannes
Wen Gong April 17, 2023, 2:07 p.m. UTC | #15 
On 4/11/2023 3:32 PM, Johannes Berg wrote:
> On Tue, 2023-04-04 at 10:54 +0800, Wen Gong wrote:
>> On 10/11/2022 3:26 PM, Johannes Berg wrote:
>>> On Tue, 2022-10-11 at 12:07 +0800, Wen Gong wrote:
>>>> On 9/28/2022 11:28 PM, Johannes Berg wrote:
>>>> ...
>>>>>> May I know some more info/status about the "incoming  new method to let
>>>>>> drivers set the link address"?
>>>>>>
>>>>> I wasn't actually planning to work on that myself, FWIW.
>>>>>
>>>>> johannes
>>>> OK. So has some body will work for that now?😁
>>>>
>>> Yes, I don't personally have a need for anything other than what we have
>>> right now.
>> May I add method to let low-drivers set the primay link address like below?
>> I add a field in struct wiphy_iftype_ext_capab, if it is valid, then it
>> will be used as
>>
>> local primary/assoc link addr in function ieee80211_mgd_setup_link() for
>> station.
>>
> I don't really think that it makes sense to push this to cfg80211 when
> we only need it in mac80211?
>
> johannes
OK. So I will try to put this in mac80211 layer, is it OK?
Wen Gong April 17, 2023, 2:13 p.m. UTC | #16 
On 4/11/2023 3:38 PM, Johannes Berg wrote:
> On Tue, 2023-04-04 at 11:28 +0800, Wen Gong wrote:
>> May I also add a field such as "u16 active_links_count" in struct
>> wiphy_iftype_ext_capab,
>> and add logic in function ieee80211_set_vif_links_bitmaps() for station
>> like this ?:
>> if (active_links_count && hweight16(links) <= active_links_count)
>>       then sdata->vif.active_links = links;
>>
> Also here, not sure it makes sense in cfg80211 level?
>
> Though I'm not sure what the idea here is at all - you can refuse to
> link switch etc, what would you use this for?
If I use ieee80211_set_active_links(),
then I need add BSS_CHANGED_ASSOC and key for 2nd link in lower-driver.

I would like to active all links while assoc,
then BSS_CHANGED_ASSOC and key will auto set for the 2nd link to 
lower-driver from mac80211.
>
> Then again, we haven't really designed out all the link selection stuff,
> do we want wpa_s to do it, driver to do it, etc.? Hence debugfs. So
> depending on what end up doing there, we will obviously need to
> advertise some level of link-concurrency to userspace.
>
> johannes
Johannes Berg April 18, 2023, 8:15 a.m. UTC | #17 
On Mon, 2023-04-17 at 22:07 +0800, Wen Gong wrote:
> 
> OK. So I will try to put this in mac80211 layer, is it OK?
> 
I guess? I'm still not really sure why you even want it, but hey, that's
up to you in a way. I really didn't like the suggestion with
wiphy_iftype_ext_capab (or any other capability for that matter), it
feels like it should be more dynamic, like maybe a new "add link"
callback or something? At least then you can't blame mac80211 for when
it breaks when you have two 5 GHz links ...

johannes
Johannes Berg April 18, 2023, 8:18 a.m. UTC | #18 
On Mon, 2023-04-17 at 22:13 +0800, Wen Gong wrote:
> On 4/11/2023 3:38 PM, Johannes Berg wrote:
> > On Tue, 2023-04-04 at 11:28 +0800, Wen Gong wrote:
> > > May I also add a field such as "u16 active_links_count" in struct
> > > wiphy_iftype_ext_capab,
> > > and add logic in function ieee80211_set_vif_links_bitmaps() for station
> > > like this ?:
> > > if (active_links_count && hweight16(links) <= active_links_count)
> > >       then sdata->vif.active_links = links;
> > > 
> > Also here, not sure it makes sense in cfg80211 level?
> > 
> > Though I'm not sure what the idea here is at all - you can refuse to
> > link switch etc, what would you use this for?
> If I use ieee80211_set_active_links(),
> then I need add BSS_CHANGED_ASSOC and key for 2nd link in lower-driver.
> 
> I would like to active all links while assoc,
> then BSS_CHANGED_ASSOC and key will auto set for the 2nd link to 
> lower-driver from mac80211.

I'm not convinced that makes sense. You're going to have to be able to
deal with changing links after association _anyway_, unless you plan on
breaking the entire connection once any of the links is getting out of
range or something?

So anyway you're going to have to be able to this for new links anyway?
I mean doing key management when link switching, and "association" (in
quotes, because as a term doesn't even make sense since this state is on
the MLD level, not the link level)...

So not sure I get it?

johannes
Wen Gong April 18, 2023, 8:59 a.m. UTC | #19 
On 4/18/2023 4:15 PM, Johannes Berg wrote:
> On Mon, 2023-04-17 at 22:07 +0800, Wen Gong wrote:
>> OK. So I will try to put this in mac80211 layer, is it OK?
>>
> I guess? I'm still not really sure why you even want it, but hey, that's
> up to you in a way. I really didn't like the suggestion with
> wiphy_iftype_ext_capab (or any other capability for that matter), it
> feels like it should be more dynamic, like maybe a new "add link"
> callback or something? At least then you can't blame mac80211 for when
> it breaks when you have two 5 GHz links ...

ok, so I would like to add callback such as

"add_link(struct ieee80211_hw *hw, struct ieee80211_vif vif, struct 
ieee80211_bss_conf *link_conf, unsigned int link_id)"

in struct ieee80211_ops, and mac80211 call it in 
ieee80211_mgd_setup_link()/ieee80211_vif_update_links,

then lower-drvier could dynamic set the local addr of assoc 
link_conf(also for 2nd link_conf), is it OK?

>
> johannes
Johannes Berg April 18, 2023, 9:11 a.m. UTC | #20 
On Tue, 2023-04-18 at 16:59 +0800, Wen Gong wrote:
> On 4/18/2023 4:15 PM, Johannes Berg wrote:
> > On Mon, 2023-04-17 at 22:07 +0800, Wen Gong wrote:
> > > OK. So I will try to put this in mac80211 layer, is it OK?
> > > 
> > I guess? I'm still not really sure why you even want it, but hey, that's
> > up to you in a way. I really didn't like the suggestion with
> > wiphy_iftype_ext_capab (or any other capability for that matter), it
> > feels like it should be more dynamic, like maybe a new "add link"
> > callback or something? At least then you can't blame mac80211 for when
> > it breaks when you have two 5 GHz links ...
> 
> ok, so I would like to add callback such as
> 
> "add_link(struct ieee80211_hw *hw, struct ieee80211_vif vif, struct 
> ieee80211_bss_conf *link_conf, unsigned int link_id)"
> 
> in struct ieee80211_ops, and mac80211 call it in 
> ieee80211_mgd_setup_link()/ieee80211_vif_update_links,
> 
> then lower-drvier could dynamic set the local addr of assoc 
> link_conf(also for 2nd link_conf), is it OK?
> 

Seems OK, but I'm not sure that _works_?

After all, we first set the addresses in assoc_data, when we don't have
a link_conf yet, no? Just what we were discussing in the other thread
about the leak.

johannes
Wen Gong April 18, 2023, 9:22 a.m. UTC | #21 
On 4/18/2023 5:11 PM, Johannes Berg wrote:
> On Tue, 2023-04-18 at 16:59 +0800, Wen Gong wrote:
>> On 4/18/2023 4:15 PM, Johannes Berg wrote:
>>> On Mon, 2023-04-17 at 22:07 +0800, Wen Gong wrote:
>>>> OK. So I will try to put this in mac80211 layer, is it OK?
>>>>
>>> I guess? I'm still not really sure why you even want it, but hey, that's
>>> up to you in a way. I really didn't like the suggestion with
>>> wiphy_iftype_ext_capab (or any other capability for that matter), it
>>> feels like it should be more dynamic, like maybe a new "add link"
>>> callback or something? At least then you can't blame mac80211 for when
>>> it breaks when you have two 5 GHz links ...
>> ok, so I would like to add callback such as
>>
>> "add_link(struct ieee80211_hw *hw, struct ieee80211_vif vif, struct
>> ieee80211_bss_conf *link_conf, unsigned int link_id)"
>>
>> in struct ieee80211_ops, and mac80211 call it in
>> ieee80211_mgd_setup_link()/ieee80211_vif_update_links,
>>
>> then lower-drvier could dynamic set the local addr of assoc
>> link_conf(also for 2nd link_conf), is it OK?
>>
> Seems OK, but I'm not sure that _works_?
>
> After all, we first set the addresses in assoc_data, when we don't have
> a link_conf yet, no? Just what we were discussing in the other thread
> about the leak.
>
> johannes

It should work, I will test it later.

For the 1st assoc link, the data->u.mgd.assoc_data is empty in 
ieee80211_mgd_setup_link(),

because ieee80211_mgd_setup_link() is called from nl80211_authenticate() 
for the 1st assoc link.

So ieee80211_mgd_setup_link() use eth_random_addr() for the 1st assoc link.


For the 2nd link, ieee80211_mgd_setup_link() is called from 
nl80211_associate(),

the sdata->u.mgd.assoc_data is NOT empty,

and the sdata->u.mgd.assoc_data->link[link_id].addr is valid,

it is addr by eth_random_addr(assoc_data->link[i].addr) in 
ieee80211_mgd_assoc().
Wen Gong April 18, 2023, 9:27 a.m. UTC | #22 
On 4/18/2023 4:18 PM, Johannes Berg wrote:
> On Mon, 2023-04-17 at 22:13 +0800, Wen Gong wrote:
>> On 4/11/2023 3:38 PM, Johannes Berg wrote:
>>> On Tue, 2023-04-04 at 11:28 +0800, Wen Gong wrote:
>>>> May I also add a field such as "u16 active_links_count" in struct
>>>> wiphy_iftype_ext_capab,
>>>> and add logic in function ieee80211_set_vif_links_bitmaps() for station
>>>> like this ?:
>>>> if (active_links_count && hweight16(links) <= active_links_count)
>>>>        then sdata->vif.active_links = links;
>>>>
>>> Also here, not sure it makes sense in cfg80211 level?
>>>
>>> Though I'm not sure what the idea here is at all - you can refuse to
>>> link switch etc, what would you use this for?
>> If I use ieee80211_set_active_links(),
>> then I need add BSS_CHANGED_ASSOC and key for 2nd link in lower-driver.
>>
>> I would like to active all links while assoc,
>> then BSS_CHANGED_ASSOC and key will auto set for the 2nd link to
>> lower-driver from mac80211.
> I'm not convinced that makes sense. You're going to have to be able to
> deal with changing links after association _anyway_, unless you plan on
> breaking the entire connection once any of the links is getting out of
> range or something?
>
> So anyway you're going to have to be able to this for new links anyway?
> I mean doing key management when link switching, and "association" (in
> quotes, because as a term doesn't even make sense since this state is on
> the MLD level, not the link level)...
>
> So not sure I get it?
>
> johannes

Yes, you are right.

Now lower driver I used do not store the key and do not trigger
BSS_CHANGED_ASSOC for new links after assoc.

So my suggestion is a way to active all links while assoc, this way is 
simple for lower driver I used.

Also ieee80211_set_active_links() is another way to active all links 
after assoc.
Johannes Berg April 18, 2023, 9:31 a.m. UTC | #23 
On Tue, 2023-04-18 at 17:22 +0800, Wen Gong wrote:
> 
> It should work, I will test it later.
> 
> For the 1st assoc link, the data->u.mgd.assoc_data is empty in 
> ieee80211_mgd_setup_link(),

Yeah for the first link it should work.

> because ieee80211_mgd_setup_link() is called from nl80211_authenticate() 
> for the 1st assoc link.
> 
> So ieee80211_mgd_setup_link() use eth_random_addr() for the 1st assoc link.

Right.

> For the 2nd link, ieee80211_mgd_setup_link() is called from 
> nl80211_associate()

I don't think so, it should only be called from
ieee80211_assoc_success()?

> the sdata->u.mgd.assoc_data is NOT empty,
> 
> and the sdata->u.mgd.assoc_data->link[link_id].addr is valid,
> 
> it is addr by eth_random_addr(assoc_data->link[i].addr) in 
> ieee80211_mgd_assoc().
> 

Exactly, so we've already decided on the address long before we actually
add the link data structure, so your callback would be much too late.
We'd need to have it called from ieee80211_mgd_assoc() already?

johannes
Johannes Berg April 18, 2023, 9:34 a.m. UTC | #24 
On Tue, 2023-04-18 at 17:27 +0800, Wen Gong wrote:
> 
> Now lower driver I used do not store the key 
> 

Sure, that's fine.

> and do not trigger
> BSS_CHANGED_ASSOC for new links after assoc.

I think you need to think hard about this ... whatever BSS_CHANGED_ASSOC
causes is likely no longer correct in MLO. Again, the assoc state
*itself* is only changed once, when the whole MLD associated.

> So my suggestion is a way to active all links while assoc, this way is 
> simple for lower driver I used.

Sure, and we do that.

But that's not what you're asking - you're asking to re-do some *MLD*
state when a new link is added, and I'm saying that it doesn't make
sense to "add" (again) a key to the MLD that was already added, nor
calling a vif (MLD!) level method saying the MLD changed state to
associated (again).

I really think you should solve this in the driver, that doesn't mean
you have to _store_ he key, you can use one of the iteration functions
as well.

> Also ieee80211_set_active_links() is another way to active all links 
> after assoc.
> 

Sure.

johannes
Wen Gong April 18, 2023, 9:37 a.m. UTC | #25 
On 4/18/2023 5:31 PM, Johannes Berg wrote:
> On Tue, 2023-04-18 at 17:22 +0800, Wen Gong wrote:
>> It should work, I will test it later.
>>
>> For the 1st assoc link, the data->u.mgd.assoc_data is empty in
>> ieee80211_mgd_setup_link(),
> Yeah for the first link it should work.
>
>> because ieee80211_mgd_setup_link() is called from nl80211_authenticate()
>> for the 1st assoc link.
>>
>> So ieee80211_mgd_setup_link() use eth_random_addr() for the 1st assoc link.
> Right.
>
>> For the 2nd link, ieee80211_mgd_setup_link() is called from
>> nl80211_associate()
> I don't think so, it should only be called from
> ieee80211_assoc_success()?
Yes, I checked again, you are right. It is not from nl80211_associate().
>> the sdata->u.mgd.assoc_data is NOT empty,
>>
>> and the sdata->u.mgd.assoc_data->link[link_id].addr is valid,
>>
>> it is addr by eth_random_addr(assoc_data->link[i].addr) in
>> ieee80211_mgd_assoc().
>>
> Exactly, so we've already decided on the address long before we actually
> add the link data structure, so your callback would be much too late.
> We'd need to have it called from ieee80211_mgd_assoc() already?

For the 2nd link, is it OK for me to use the random addr which is set in 
ieee80211_mgd_assoc().

I only need to set the 1st assoc link in low driver.

> johannes
Johannes Berg April 18, 2023, 9:38 a.m. UTC | #26 
On Tue, 2023-04-18 at 17:37 +0800, Wen Gong wrote:


> > > the sdata->u.mgd.assoc_data is NOT empty,
> > > 
> > > and the sdata->u.mgd.assoc_data->link[link_id].addr is valid,
> > > 
> > > it is addr by eth_random_addr(assoc_data->link[i].addr) in
> > > ieee80211_mgd_assoc().
> > > 
> > Exactly, so we've already decided on the address long before we actually
> > add the link data structure, so your callback would be much too late.
> > We'd need to have it called from ieee80211_mgd_assoc() already?
> 
> For the 2nd link, is it OK for me to use the random addr which is set in 
> ieee80211_mgd_assoc().
> 
> I only need to set the 1st assoc link in low driver.
> 

Ah. But does it make sense to restrict the API for that? I mean, if you
just change the prototype a little bit and call it without the link
conf, you can easily solve this problem too, no?

Then your driver just has to call eth_radnom_addr() when it's "don't
care", but that's OK?

johannes
Wen Gong April 18, 2023, 9:44 a.m. UTC | #27 
On 4/18/2023 5:38 PM, Johannes Berg wrote:
> On Tue, 2023-04-18 at 17:37 +0800, Wen Gong wrote:
>
>
>>>> the sdata->u.mgd.assoc_data is NOT empty,
>>>>
>>>> and the sdata->u.mgd.assoc_data->link[link_id].addr is valid,
>>>>
>>>> it is addr by eth_random_addr(assoc_data->link[i].addr) in
>>>> ieee80211_mgd_assoc().
>>>>
>>> Exactly, so we've already decided on the address long before we actually
>>> add the link data structure, so your callback would be much too late.
>>> We'd need to have it called from ieee80211_mgd_assoc() already?
>> For the 2nd link, is it OK for me to use the random addr which is set in
>> ieee80211_mgd_assoc().
>>
>> I only need to set the 1st assoc link in low driver.
>>
> Ah. But does it make sense to restrict the API for that? I mean, if you
> just change the prototype a little bit and call it without the link
> conf, you can easily solve this problem too, no?
Sorry, I am not sure how to solve this problem by remove the link conf 
in prototype.
>
> Then your driver just has to call eth_radnom_addr() when it's "don't
> care", but that's OK?
Yes, it is also OK for me to call eth_radnom_addr()for 2nd link.
>
> johannes
Wen Gong April 18, 2023, 9:52 a.m. UTC | #28 
On 4/18/2023 5:34 PM, Johannes Berg wrote:
> On Tue, 2023-04-18 at 17:27 +0800, Wen Gong wrote:
>> Now lower driver I used do not store the key
>>
> Sure, that's fine.
>
>> and do not trigger
>> BSS_CHANGED_ASSOC for new links after assoc.
> I think you need to think hard about this ... whatever BSS_CHANGED_ASSOC
> causes is likely no longer correct in MLO. Again, the assoc state
> *itself* is only changed once, when the whole MLD associated.
>
>> So my suggestion is a way to active all links while assoc, this way is
>> simple for lower driver I used.
> Sure, and we do that.
>
> But that's not what you're asking - you're asking to re-do some *MLD*
> state when a new link is added, and I'm saying that it doesn't make
> sense to "add" (again) a key to the MLD that was already added, nor
> calling a vif (MLD!) level method saying the MLD changed state to
> associated (again).

My purpose it to:

add logic in function ieee80211_set_vif_links_bitmaps() for station,

and to active all link as same as AP type.

>
> I really think you should solve this in the driver, that doesn't mean
> you have to _store_ he key, you can use one of the iteration functions
> as well.
Ok, I will try to find the iteration functions.
>
>> Also ieee80211_set_active_links() is another way to active all links
>> after assoc.
>>
> Sure.
>
> johannes
Johannes Berg April 18, 2023, 10:18 a.m. UTC | #29 
On Tue, 2023-04-18 at 17:44 +0800, Wen Gong wrote:
> On 4/18/2023 5:38 PM, Johannes Berg wrote:
> > On Tue, 2023-04-18 at 17:37 +0800, Wen Gong wrote:
> > 
> > 
> > > > > the sdata->u.mgd.assoc_data is NOT empty,
> > > > > 
> > > > > and the sdata->u.mgd.assoc_data->link[link_id].addr is valid,
> > > > > 
> > > > > it is addr by eth_random_addr(assoc_data->link[i].addr) in
> > > > > ieee80211_mgd_assoc().
> > > > > 
> > > > Exactly, so we've already decided on the address long before we actually
> > > > add the link data structure, so your callback would be much too late.
> > > > We'd need to have it called from ieee80211_mgd_assoc() already?
> > > For the 2nd link, is it OK for me to use the random addr which is set in
> > > ieee80211_mgd_assoc().
> > > 
> > > I only need to set the 1st assoc link in low driver.
> > > 
> > Ah. But does it make sense to restrict the API for that? I mean, if you
> > just change the prototype a little bit and call it without the link
> > conf, you can easily solve this problem too, no?
> Sorry, I am not sure how to solve this problem by remove the link conf 
> in prototype.

Why, then you can have an output parameter for the address, and call it
in mac80211 wherever it calls eth_random_addr() today, no?

johannes
>
Johannes Berg April 18, 2023, 10:47 a.m. UTC | #30 
(removing HTML)

On Tue, 2023-04-18 at 18:42 +0800, Wen Gong wrote:
> 
> > Why, then you can have an output parameter for the address, and call
> > it
> > in mac80211 wherever it calls eth_random_addr() today, no?
> > 
> OK. Got it. So it is like this, right?
> add_link(struct ieee80211_hw *hw, struct ieee80211_vif vif, u8
> *link_local_addr, unsigned int link_id)" 

Seems like that could work, yeah.

johannes
Wen Gong May 10, 2023, 11:06 a.m. UTC | #31 
On 4/11/2023 3:38 PM, Johannes Berg wrote:
> On Tue, 2023-04-04 at 11:28 +0800, Wen Gong wrote:
>> May I also add a field such as "u16 active_links_count" in struct
>> wiphy_iftype_ext_capab,
>> and add logic in function ieee80211_set_vif_links_bitmaps() for station
>> like this ?:
>> if (active_links_count && hweight16(links) <= active_links_count)
>>       then sdata->vif.active_links = links;
>>
> Also here, not sure it makes sense in cfg80211 level?
>
> Though I'm not sure what the idea here is at all - you can refuse to
> link switch etc, what would you use this for?
>
> Then again, we haven't really designed out all the link selection stuff,
> do we want wpa_s to do it, driver to do it, etc.? Hence debugfs. So
> depending on what end up doing there, we will obviously need to
> advertise some level of link-concurrency to userspace.

So will you plan to do something to let wpa_s/userspace app 
active/deactive links?

Or you already have implemented that?

>
> johannes
Johannes Berg May 10, 2023, 11:24 a.m. UTC | #32 
On Wed, 2023-05-10 at 19:06 +0800, Wen Gong wrote:
> On 4/11/2023 3:38 PM, Johannes Berg wrote:
> > On Tue, 2023-04-04 at 11:28 +0800, Wen Gong wrote:
> > > May I also add a field such as "u16 active_links_count" in struct
> > > wiphy_iftype_ext_capab,
> > > and add logic in function ieee80211_set_vif_links_bitmaps() for station
> > > like this ?:
> > > if (active_links_count && hweight16(links) <= active_links_count)
> > >       then sdata->vif.active_links = links;
> > > 
> > Also here, not sure it makes sense in cfg80211 level?
> > 
> > Though I'm not sure what the idea here is at all - you can refuse to
> > link switch etc, what would you use this for?
> > 
> > Then again, we haven't really designed out all the link selection stuff,
> > do we want wpa_s to do it, driver to do it, etc.? Hence debugfs. So
> > depending on what end up doing there, we will obviously need to
> > advertise some level of link-concurrency to userspace.
> 
> So will you plan to do something to let wpa_s/userspace app 
> active/deactive links?
> 
> Or you already have implemented that?
> 

No plans right now, and honestly not sure what the right thing even is.

johannes
Wen Gong May 10, 2023, 12:25 p.m. UTC | #33 
On 5/10/2023 7:24 PM, Johannes Berg wrote:
> On Wed, 2023-05-10 at 19:06 +0800, Wen Gong wrote:
>> On 4/11/2023 3:38 PM, Johannes Berg wrote:
>>> On Tue, 2023-04-04 at 11:28 +0800, Wen Gong wrote:
>>>> May I also add a field such as "u16 active_links_count" in struct
>>>> wiphy_iftype_ext_capab,
>>>> and add logic in function ieee80211_set_vif_links_bitmaps() for station
>>>> like this ?:
>>>> if (active_links_count && hweight16(links) <= active_links_count)
>>>>        then sdata->vif.active_links = links;
>>>>
>>> Also here, not sure it makes sense in cfg80211 level?
>>>
>>> Though I'm not sure what the idea here is at all - you can refuse to
>>> link switch etc, what would you use this for?
>>>
>>> Then again, we haven't really designed out all the link selection stuff,
>>> do we want wpa_s to do it, driver to do it, etc.? Hence debugfs. So
>>> depending on what end up doing there, we will obviously need to
>>> advertise some level of link-concurrency to userspace.
>> So will you plan to do something to let wpa_s/userspace app
>> active/deactive links?
>>
>> Or you already have implemented that?
>>
> No plans right now, and honestly not sure what the right thing even is.
OK. For "advertise some level of link-concurrency to userspace", do you 
have any plan/idea?
>
> johannes
Johannes Berg May 10, 2023, 12:25 p.m. UTC | #34 
On Wed, 2023-05-10 at 20:25 +0800, Wen Gong wrote:
> On 5/10/2023 7:24 PM, Johannes Berg wrote:
> > On Wed, 2023-05-10 at 19:06 +0800, Wen Gong wrote:
> > > On 4/11/2023 3:38 PM, Johannes Berg wrote:
> > > > On Tue, 2023-04-04 at 11:28 +0800, Wen Gong wrote:
> > > > > May I also add a field such as "u16 active_links_count" in struct
> > > > > wiphy_iftype_ext_capab,
> > > > > and add logic in function ieee80211_set_vif_links_bitmaps() for station
> > > > > like this ?:
> > > > > if (active_links_count && hweight16(links) <= active_links_count)
> > > > >        then sdata->vif.active_links = links;
> > > > > 
> > > > Also here, not sure it makes sense in cfg80211 level?
> > > > 
> > > > Though I'm not sure what the idea here is at all - you can refuse to
> > > > link switch etc, what would you use this for?
> > > > 
> > > > Then again, we haven't really designed out all the link selection stuff,
> > > > do we want wpa_s to do it, driver to do it, etc.? Hence debugfs. So
> > > > depending on what end up doing there, we will obviously need to
> > > > advertise some level of link-concurrency to userspace.
> > > So will you plan to do something to let wpa_s/userspace app
> > > active/deactive links?
> > > 
> > > Or you already have implemented that?
> > > 
> > No plans right now, and honestly not sure what the right thing even is.
> OK. For "advertise some level of link-concurrency to userspace", do you 
> have any plan/idea?

No, not really.

johannes
>
Johannes Berg June 14, 2023, 6:32 p.m. UTC | #35 
On Wed, 2023-05-24 at 15:41 +0800, Wen Gong wrote:
> 
> May I add a new ops in struct ieee80211_ops? like this:
> 
> u16 active_links(struct ieee80211_hw *hw, struct ieee80211_vif vif, u16 
> new_links)"
> 
> then ieee80211_set_vif_links_bitmaps() call the ops to get the links for 
> station and set the sdata->vif.active_links with the return value from 
> lower driver,
> it means lower driver will dynamic select the links count at this moment.
> 
> If lower driver not register ops active_links, then keep current logic.
> 

I guess you can can send patches for whatever you want :)

But I have no idea what you're trying to do? Why would you need to have
a callback?

Was this for link selection in the driver? We should have a patch
somewhere that adds a BSS_CHANGE flag for when the valid links change,
so the driver can select others.

johannes
Wen Gong June 15, 2023, 2:26 a.m. UTC | #36 
On 6/15/2023 2:32 AM, Johannes Berg wrote:
> On Wed, 2023-05-24 at 15:41 +0800, Wen Gong wrote:
>> May I add a new ops in struct ieee80211_ops? like this:
>>
>> u16 active_links(struct ieee80211_hw *hw, struct ieee80211_vif vif, u16
>> new_links)"
>>
>> then ieee80211_set_vif_links_bitmaps() call the ops to get the links for
>> station and set the sdata->vif.active_links with the return value from
>> lower driver,
>> it means lower driver will dynamic select the links count at this moment.
>>
>> If lower driver not register ops active_links, then keep current logic.
>>
> I guess you can can send patches for whatever you want :)
>
> But I have no idea what you're trying to do? Why would you need to have
> a callback?

Currently driver could use ieee80211_set_active_links_async() to active 
links after connection completed.

But I would like to allow driver to select active links in a early time, 
it will be more convenient for driver.

>
> Was this for link selection in the driver? We should have a patch
> somewhere that adds a BSS_CHANGE flag for when the valid links change,
> so the driver can select others.
>
> johannes

Yes, it is for link selection in driver at a early time before 
connection completed.

Could you tell detail about how the BSS_CHANGE flag works?😁
Johannes Berg June 15, 2023, 7:56 a.m. UTC | #37 
On Thu, 2023-06-15 at 10:26 +0800, Wen Gong wrote:
> On 6/15/2023 2:32 AM, Johannes Berg wrote:
> > On Wed, 2023-05-24 at 15:41 +0800, Wen Gong wrote:
> > > May I add a new ops in struct ieee80211_ops? like this:
> > > 
> > > u16 active_links(struct ieee80211_hw *hw, struct ieee80211_vif vif, u16
> > > new_links)"
> > > 
> > > then ieee80211_set_vif_links_bitmaps() call the ops to get the links for
> > > station and set the sdata->vif.active_links with the return value from
> > > lower driver,
> > > it means lower driver will dynamic select the links count at this moment.
> > > 
> > > If lower driver not register ops active_links, then keep current logic.
> > > 
> > I guess you can can send patches for whatever you want :)
> > 
> > But I have no idea what you're trying to do? Why would you need to have
> > a callback?
> 
> Currently driver could use ieee80211_set_active_links_async() to active 
> links after connection completed.

Right.

> But I would like to allow driver to select active links in a early time, 
> it will be more convenient for driver.

How so? All you have to do is look for the connection becoming
authorized (e.g. sta state for the AP moving to authorized) and then
selecting the links you want. We've already been working on that, it's
really easy?

On the flip-side, it would be highly inconvenient for mac80211 to try to
enable more links *during* the association process, and actually it's
not even allowed by spec until the 4-way-HS finishes. So the earliest
possible time is pretty much when you can just do it in the driver as I
just described.

> > Was this for link selection in the driver? We should have a patch
> > somewhere that adds a BSS_CHANGE flag for when the valid links change,
> > so the driver can select others.
> > 
> > johannes
> 
> Yes, it is for link selection in driver at a early time before 
> connection completed.

This is not really allowed ... At least not without also finding ways to
really transmit the 802.1X and 4-way-HS only on the right link, etc.

> Could you tell detail about how the BSS_CHANGE flag works?😁

The work isn't complete yet, but basically it just calls the callback
whenever the valid_links changed, say by link-reconfiguration.

johannes
Wen Gong June 27, 2023, 11:02 a.m. UTC | #38 
On 6/21/2023 3:55 PM, Wen Gong wrote:
> On 6/15/2023 3:56 PM, Johannes Berg wrote:
>> On Thu, 2023-06-15 at 10:26 +0800, Wen Gong wrote:
>>> On 6/15/2023 2:32 AM, Johannes Berg wrote:
>>>> On Wed, 2023-05-24 at 15:41 +0800, Wen Gong wrote:
>>>>> May I add a new ops in struct ieee80211_ops? like this:
>>>>>
>>>>> u16 active_links(struct ieee80211_hw *hw, struct ieee80211_vif 
>>>>> vif, u16
>>>>> new_links)"
>>>>>
>>>>> then ieee80211_set_vif_links_bitmaps() call the ops to get the 
>>>>> links for
>>>>> station and set the sdata->vif.active_links with the return value 
>>>>> from
>>>>> lower driver,
>>>>> it means lower driver will dynamic select the links count at this 
>>>>> moment.
>>>>>
>>>>> If lower driver not register ops active_links, then keep current 
>>>>> logic.
>>>>>
>>>> I guess you can can send patches for whatever you want :)
>>>>
>>>> But I have no idea what you're trying to do? Why would you need to 
>>>> have
>>>> a callback?
>>> Currently driver could use ieee80211_set_active_links_async() to active
>>> links after connection completed.
>> Right.
>>
>>> But I would like to allow driver to select active links in a early 
>>> time,
>>> it will be more convenient for driver.
>> How so? All you have to do is look for the connection becoming
>> authorized (e.g. sta state for the AP moving to authorized) and then
>> selecting the links you want. We've already been working on that, it's
>> really easy?
> It is more complex for ath12k drivers.
>>
>> On the flip-side, it would be highly inconvenient for mac80211 to try to
>> enable more links *during* the association process, and actually it's
>> not even allowed by spec until the 4-way-HS finishes. So the earliest
>> possible time is pretty much when you can just do it in the driver as I
>> just described.
>>
>>>> Was this for link selection in the driver? We should have a patch
>>>> somewhere that adds a BSS_CHANGE flag for when the valid links change,
>>>> so the driver can select others.
>>>>
>>>> johannes
>>> Yes, it is for link selection in driver at a early time before
>>> connection completed.
>> This is not really allowed ... At least not without also finding ways to
>> really transmit the 802.1X and 4-way-HS only on the right link, etc.
> Yes, I also found this in section "2.7.6.1 General"  of IEEE P802.11be:
> "For MLO, if RSNA has not been established, each message of the 4-way 
> handshake shall be sent on
> the same link used by the latest exchange of successful 
> (Re)Association Request/Response frames."
>
> For ath12k drivers, only the primary link(the link used by the latest 
> exchange of successful (Re)Association
> Request/Response frames) is active before key installed, so the 802.1X 
> and 4-way-HS will always sent on
> the primary link, so it will meet the rule above of section "2.7.6.1 
> General".
>
> It means: lower driver will ensure the rule when it implmented the 
> active_links callback such as ath12k.
Johannes, do you have more comments for my answer above?😁
>>> Could you tell detail about how the BSS_CHANGE flag works?😁
>> The work isn't complete yet, but basically it just calls the callback
>> whenever the valid_links changed, say by link-reconfiguration.
>>
>> johannes
Wen Gong June 30, 2023, 9:32 a.m. UTC | #39 
On 6/15/2023 3:56 PM, Johannes Berg wrote:
> On Thu, 2023-06-15 at 10:26 +0800, Wen Gong wrote:
>> On 6/15/2023 2:32 AM, Johannes Berg wrote:
>>> On Wed, 2023-05-24 at 15:41 +0800, Wen Gong wrote:
>>>
...
>> Could you tell detail about how the BSS_CHANGE flag works?😁
> The work isn't complete yet, but basically it just calls the callback
> whenever the valid_links changed, say by link-reconfiguration.
>
> johannes
I guess the link-reconfiguration you said is for station, it means 
station will do corresponding
link-reconfiguration after receive link reconfiguration indication(e.g. 
Reconfiguration
Multi-Link element) from MLO AP, right?

I guess you will add enum BSS_CHANGED_xxx(e.g. 
BSS_CHANGED_LINK_RECONFIG), and call
vif_cfg_changed of struct ieee80211_ops for link-reconfiguration, right?

And do you will implement both remove link and add link of station?

For add link, it should calculate the new key of the new link("35.3.6.4 
ML reconfiguration
to the ML setup" of IEEE P802.11be™/D3.2).
diff mbox series

Patch

diff --git a/include/net/mac80211.h b/include/net/mac80211.h
index d4e1d73d88cc..20a2f25a38fa 100644
--- a/include/net/mac80211.h
+++ b/include/net/mac80211.h
@@ -1799,6 +1799,9 @@  struct ieee80211_vif_cfg {
  * @link_conf: in case of MLD, the per-link BSS configuration,
  *	indexed by link ID
  * @valid_links: bitmap of valid links, or 0 for non-MLO.
+ * @active_links: The bitmap of active links, or 0 for non-MLO.
+ *	The driver shouldn't change this directly, but use the
+ *	API calls meant for that purpose.
  * @addr: address of this interface
  * @p2p: indicates whether this AP or STA interface is a p2p
  *	interface, i.e. a GO or p2p-sta respectively
@@ -1834,7 +1837,7 @@  struct ieee80211_vif {
 	struct ieee80211_vif_cfg cfg;
 	struct ieee80211_bss_conf bss_conf;
 	struct ieee80211_bss_conf __rcu *link_conf[IEEE80211_MLD_MAX_NUM_LINKS];
-	u16 valid_links;
+	u16 valid_links, active_links;
 	u8 addr[ETH_ALEN] __aligned(2);
 	bool p2p;
 
@@ -1861,12 +1864,11 @@  struct ieee80211_vif {
 	u8 drv_priv[] __aligned(sizeof(void *));
 };
 
-/* FIXME: for now loop over all the available links; later will be changed
- * to loop only over the active links.
- */
-#define for_each_vif_active_link(vif, link, link_id)			     \
-	for (link_id = 0; link_id < ARRAY_SIZE((vif)->link_conf); link_id++) \
-		if ((link = rcu_dereference((vif)->link_conf[link_id])))
+#define for_each_vif_active_link(vif, link, link_id)				\
+	for (link_id = 0; link_id < ARRAY_SIZE((vif)->link_conf); link_id++)	\
+		if ((!(vif)->active_links ||					\
+		     (vif)->active_links & BIT(link_id)) &&			\
+		    (link = rcu_dereference((vif)->link_conf[link_id])))
 
 static inline bool ieee80211_vif_is_mesh(struct ieee80211_vif *vif)
 {
@@ -2264,13 +2266,13 @@  struct ieee80211_sta {
 	u8 drv_priv[] __aligned(sizeof(void *));
 };
 
-/* FIXME: need to loop only over links which are active and check the actual
- * lock
- */
-#define for_each_sta_active_link(sta, link_sta, link_id)		         \
-	for (link_id = 0; link_id < ARRAY_SIZE((sta)->link); link_id++)	         \
-		if (((link_sta) = rcu_dereference_protected((sta)->link[link_id],\
-							    1)))	         \
+/* FIXME: check the locking correctly */
+#define for_each_sta_active_link(vif, sta, link_sta, link_id)			\
+	for (link_id = 0; link_id < ARRAY_SIZE((sta)->link); link_id++)		\
+		if ((!(vif)->active_links ||					\
+		     (vif)->active_links & BIT(link_id)) &&			\
+		    ((link_sta) = rcu_dereference_protected((sta)->link[link_id],\
+							    1)))
 
 /**
  * enum sta_notify_cmd - sta notify command
diff --git a/net/mac80211/chan.c b/net/mac80211/chan.c
index f247daa41563..e72cf0749d49 100644
--- a/net/mac80211/chan.c
+++ b/net/mac80211/chan.c
@@ -1799,6 +1799,12 @@  int ieee80211_link_use_channel(struct ieee80211_link_data *link,
 
 	lockdep_assert_held(&local->mtx);
 
+	if (sdata->vif.active_links &&
+	    !(sdata->vif.active_links & BIT(link->link_id))) {
+		ieee80211_link_update_chandef(link, chandef);
+		return 0;
+	}
+
 	mutex_lock(&local->chanctx_mtx);
 
 	ret = cfg80211_chandef_dfs_required(local->hw.wiphy,
diff --git a/net/mac80211/driver-ops.c b/net/mac80211/driver-ops.c
index 9b61dc7889c2..5392ffa18270 100644
--- a/net/mac80211/driver-ops.c
+++ b/net/mac80211/driver-ops.c
@@ -192,6 +192,10 @@  int drv_conf_tx(struct ieee80211_local *local,
 	if (!check_sdata_in_driver(sdata))
 		return -EIO;
 
+	if (sdata->vif.active_links &&
+	    !(sdata->vif.active_links & BIT(link->link_id)))
+		return 0;
+
 	if (params->cw_min == 0 || params->cw_min > params->cw_max) {
 		/*
 		 * If we can't configure hardware anyway, don't warn. We may
@@ -272,6 +276,60 @@  void drv_reset_tsf(struct ieee80211_local *local,
 	trace_drv_return_void(local);
 }
 
+int drv_assign_vif_chanctx(struct ieee80211_local *local,
+			   struct ieee80211_sub_if_data *sdata,
+			   struct ieee80211_bss_conf *link_conf,
+			   struct ieee80211_chanctx *ctx)
+{
+	int ret = 0;
+
+	drv_verify_link_exists(sdata, link_conf);
+	if (!check_sdata_in_driver(sdata))
+		return -EIO;
+
+	if (sdata->vif.active_links &&
+	    !(sdata->vif.active_links & BIT(link_conf->link_id)))
+		return 0;
+
+	trace_drv_assign_vif_chanctx(local, sdata, link_conf, ctx);
+	if (local->ops->assign_vif_chanctx) {
+		WARN_ON_ONCE(!ctx->driver_present);
+		ret = local->ops->assign_vif_chanctx(&local->hw,
+						     &sdata->vif,
+						     link_conf,
+						     &ctx->conf);
+	}
+	trace_drv_return_int(local, ret);
+
+	return ret;
+}
+
+void drv_unassign_vif_chanctx(struct ieee80211_local *local,
+			      struct ieee80211_sub_if_data *sdata,
+			      struct ieee80211_bss_conf *link_conf,
+			      struct ieee80211_chanctx *ctx)
+{
+	might_sleep();
+
+	drv_verify_link_exists(sdata, link_conf);
+	if (!check_sdata_in_driver(sdata))
+		return;
+
+	if (sdata->vif.active_links &&
+	    !(sdata->vif.active_links & BIT(link_conf->link_id)))
+		return;
+
+	trace_drv_unassign_vif_chanctx(local, sdata, link_conf, ctx);
+	if (local->ops->unassign_vif_chanctx) {
+		WARN_ON_ONCE(!ctx->driver_present);
+		local->ops->unassign_vif_chanctx(&local->hw,
+						 &sdata->vif,
+						 link_conf,
+						 &ctx->conf);
+	}
+	trace_drv_return_void(local);
+}
+
 int drv_switch_vif_chanctx(struct ieee80211_local *local,
 			   struct ieee80211_vif_chanctx_switch *vifs,
 			   int n_vifs, enum ieee80211_chanctx_switch_mode mode)
@@ -346,3 +404,117 @@  int drv_ampdu_action(struct ieee80211_local *local,
 
 	return ret;
 }
+
+void drv_link_info_changed(struct ieee80211_local *local,
+			   struct ieee80211_sub_if_data *sdata,
+			   struct ieee80211_bss_conf *info,
+			   int link_id, u64 changed)
+{
+	might_sleep();
+
+	if (WARN_ON_ONCE(changed & (BSS_CHANGED_BEACON |
+				    BSS_CHANGED_BEACON_ENABLED) &&
+			 sdata->vif.type != NL80211_IFTYPE_AP &&
+			 sdata->vif.type != NL80211_IFTYPE_ADHOC &&
+			 sdata->vif.type != NL80211_IFTYPE_MESH_POINT &&
+			 sdata->vif.type != NL80211_IFTYPE_OCB))
+		return;
+
+	if (WARN_ON_ONCE(sdata->vif.type == NL80211_IFTYPE_P2P_DEVICE ||
+			 sdata->vif.type == NL80211_IFTYPE_NAN ||
+			 (sdata->vif.type == NL80211_IFTYPE_MONITOR &&
+			  !sdata->vif.bss_conf.mu_mimo_owner &&
+			  !(changed & BSS_CHANGED_TXPOWER))))
+		return;
+
+	if (!check_sdata_in_driver(sdata))
+		return;
+
+	if (sdata->vif.active_links &&
+	    !(sdata->vif.active_links & BIT(link_id)))
+		return;
+
+	trace_drv_link_info_changed(local, sdata, info, changed);
+	if (local->ops->link_info_changed)
+		local->ops->link_info_changed(&local->hw, &sdata->vif,
+					      info, changed);
+	else if (local->ops->bss_info_changed)
+		local->ops->bss_info_changed(&local->hw, &sdata->vif,
+					     info, changed);
+	trace_drv_return_void(local);
+}
+
+int drv_set_key(struct ieee80211_local *local,
+		enum set_key_cmd cmd,
+		struct ieee80211_sub_if_data *sdata,
+		struct ieee80211_sta *sta,
+		struct ieee80211_key_conf *key)
+{
+	int ret;
+
+	might_sleep();
+
+	sdata = get_bss_sdata(sdata);
+	if (!check_sdata_in_driver(sdata))
+		return -EIO;
+
+	if (WARN_ON(key->link_id >= 0 && sdata->vif.active_links &&
+		    !(sdata->vif.active_links & BIT(key->link_id))))
+		return -ENOLINK;
+
+	trace_drv_set_key(local, cmd, sdata, sta, key);
+	ret = local->ops->set_key(&local->hw, cmd, &sdata->vif, sta, key);
+	trace_drv_return_int(local, ret);
+	return ret;
+}
+
+int drv_change_vif_links(struct ieee80211_local *local,
+			 struct ieee80211_sub_if_data *sdata,
+			 u16 old_links, u16 new_links,
+			 struct ieee80211_bss_conf *old[IEEE80211_MLD_MAX_NUM_LINKS])
+{
+	int ret = -EOPNOTSUPP;
+
+	might_sleep();
+
+	if (!check_sdata_in_driver(sdata))
+		return -EIO;
+
+	if (old_links == new_links)
+		return 0;
+
+	trace_drv_change_vif_links(local, sdata, old_links, new_links);
+	if (local->ops->change_vif_links)
+		ret = local->ops->change_vif_links(&local->hw, &sdata->vif,
+						   old_links, new_links, old);
+	trace_drv_return_int(local, ret);
+
+	return ret;
+}
+
+int drv_change_sta_links(struct ieee80211_local *local,
+			 struct ieee80211_sub_if_data *sdata,
+			 struct ieee80211_sta *sta,
+			 u16 old_links, u16 new_links)
+{
+	int ret = -EOPNOTSUPP;
+
+	might_sleep();
+
+	if (!check_sdata_in_driver(sdata))
+		return -EIO;
+
+	old_links &= sdata->vif.active_links;
+	new_links &= sdata->vif.active_links;
+
+	if (old_links == new_links)
+		return 0;
+
+	trace_drv_change_sta_links(local, sdata, sta, old_links, new_links);
+	if (local->ops->change_sta_links)
+		ret = local->ops->change_sta_links(&local->hw, &sdata->vif, sta,
+						   old_links, new_links);
+	trace_drv_return_int(local, ret);
+
+	return ret;
+}
diff --git a/net/mac80211/driver-ops.h b/net/mac80211/driver-ops.h
index 482f5c97a72b..81e40b0a3b16 100644
--- a/net/mac80211/driver-ops.h
+++ b/net/mac80211/driver-ops.h
@@ -165,40 +165,10 @@  static inline void drv_vif_cfg_changed(struct ieee80211_local *local,
 	trace_drv_return_void(local);
 }
 
-static inline void drv_link_info_changed(struct ieee80211_local *local,
-					 struct ieee80211_sub_if_data *sdata,
-					 struct ieee80211_bss_conf *info,
-					 int link_id, u64 changed)
-{
-	might_sleep();
-
-	if (WARN_ON_ONCE(changed & (BSS_CHANGED_BEACON |
-				    BSS_CHANGED_BEACON_ENABLED) &&
-			 sdata->vif.type != NL80211_IFTYPE_AP &&
-			 sdata->vif.type != NL80211_IFTYPE_ADHOC &&
-			 sdata->vif.type != NL80211_IFTYPE_MESH_POINT &&
-			 sdata->vif.type != NL80211_IFTYPE_OCB))
-		return;
-
-	if (WARN_ON_ONCE(sdata->vif.type == NL80211_IFTYPE_P2P_DEVICE ||
-			 sdata->vif.type == NL80211_IFTYPE_NAN ||
-			 (sdata->vif.type == NL80211_IFTYPE_MONITOR &&
-			  !sdata->vif.bss_conf.mu_mimo_owner &&
-			  !(changed & BSS_CHANGED_TXPOWER))))
-		return;
-
-	if (!check_sdata_in_driver(sdata))
-		return;
-
-	trace_drv_link_info_changed(local, sdata, info, changed);
-	if (local->ops->link_info_changed)
-		local->ops->link_info_changed(&local->hw, &sdata->vif,
-					      info, changed);
-	else if (local->ops->bss_info_changed)
-		local->ops->bss_info_changed(&local->hw, &sdata->vif,
-					     info, changed);
-	trace_drv_return_void(local);
-}
+void drv_link_info_changed(struct ieee80211_local *local,
+			   struct ieee80211_sub_if_data *sdata,
+			   struct ieee80211_bss_conf *info,
+			   int link_id, u64 changed);
 
 static inline u64 drv_prepare_multicast(struct ieee80211_local *local,
 					struct netdev_hw_addr_list *mc_list)
@@ -256,25 +226,11 @@  static inline int drv_set_tim(struct ieee80211_local *local,
 	return ret;
 }
 
-static inline int drv_set_key(struct ieee80211_local *local,
-			      enum set_key_cmd cmd,
-			      struct ieee80211_sub_if_data *sdata,
-			      struct ieee80211_sta *sta,
-			      struct ieee80211_key_conf *key)
-{
-	int ret;
-
-	might_sleep();
-
-	sdata = get_bss_sdata(sdata);
-	if (!check_sdata_in_driver(sdata))
-		return -EIO;
-
-	trace_drv_set_key(local, cmd, sdata, sta, key);
-	ret = local->ops->set_key(&local->hw, cmd, &sdata->vif, sta, key);
-	trace_drv_return_int(local, ret);
-	return ret;
-}
+int drv_set_key(struct ieee80211_local *local,
+		enum set_key_cmd cmd,
+		struct ieee80211_sub_if_data *sdata,
+		struct ieee80211_sta *sta,
+		struct ieee80211_key_conf *key);
 
 static inline void drv_update_tkip_key(struct ieee80211_local *local,
 				       struct ieee80211_sub_if_data *sdata,
@@ -945,52 +901,14 @@  static inline void drv_verify_link_exists(struct ieee80211_sub_if_data *sdata,
 		sdata_assert_lock(sdata);
 }
 
-static inline int drv_assign_vif_chanctx(struct ieee80211_local *local,
-					 struct ieee80211_sub_if_data *sdata,
-					 struct ieee80211_bss_conf *link_conf,
-					 struct ieee80211_chanctx *ctx)
-{
-	int ret = 0;
-
-	drv_verify_link_exists(sdata, link_conf);
-	if (!check_sdata_in_driver(sdata))
-		return -EIO;
-
-	trace_drv_assign_vif_chanctx(local, sdata, link_conf, ctx);
-	if (local->ops->assign_vif_chanctx) {
-		WARN_ON_ONCE(!ctx->driver_present);
-		ret = local->ops->assign_vif_chanctx(&local->hw,
-						     &sdata->vif,
-						     link_conf,
-						     &ctx->conf);
-	}
-	trace_drv_return_int(local, ret);
-
-	return ret;
-}
-
-static inline void drv_unassign_vif_chanctx(struct ieee80211_local *local,
-					    struct ieee80211_sub_if_data *sdata,
-					    struct ieee80211_bss_conf *link_conf,
-					    struct ieee80211_chanctx *ctx)
-{
-	might_sleep();
-
-	drv_verify_link_exists(sdata, link_conf);
-	if (!check_sdata_in_driver(sdata))
-		return;
-
-	trace_drv_unassign_vif_chanctx(local, sdata, link_conf, ctx);
-	if (local->ops->unassign_vif_chanctx) {
-		WARN_ON_ONCE(!ctx->driver_present);
-		local->ops->unassign_vif_chanctx(&local->hw,
-						 &sdata->vif,
-						 link_conf,
-						 &ctx->conf);
-	}
-	trace_drv_return_void(local);
-}
-
+int drv_assign_vif_chanctx(struct ieee80211_local *local,
+			   struct ieee80211_sub_if_data *sdata,
+			   struct ieee80211_bss_conf *link_conf,
+			   struct ieee80211_chanctx *ctx);
+void drv_unassign_vif_chanctx(struct ieee80211_local *local,
+			      struct ieee80211_sub_if_data *sdata,
+			      struct ieee80211_bss_conf *link_conf,
+			      struct ieee80211_chanctx *ctx);
 int drv_switch_vif_chanctx(struct ieee80211_local *local,
 			   struct ieee80211_vif_chanctx_switch *vifs,
 			   int n_vifs, enum ieee80211_chanctx_switch_mode mode);
@@ -1552,46 +1470,13 @@  static inline int drv_net_fill_forward_path(struct ieee80211_local *local,
 	return ret;
 }
 
-static inline int drv_change_vif_links(struct ieee80211_local *local,
-				       struct ieee80211_sub_if_data *sdata,
-				       u16 old_links, u16 new_links,
-				       struct ieee80211_bss_conf *old[IEEE80211_MLD_MAX_NUM_LINKS])
-{
-	int ret = -EOPNOTSUPP;
-
-	might_sleep();
-
-	if (!check_sdata_in_driver(sdata))
-		return -EIO;
-
-	trace_drv_change_vif_links(local, sdata, old_links, new_links);
-	if (local->ops->change_vif_links)
-		ret = local->ops->change_vif_links(&local->hw, &sdata->vif,
-						   old_links, new_links, old);
-	trace_drv_return_int(local, ret);
-
-	return ret;
-}
-
-static inline int drv_change_sta_links(struct ieee80211_local *local,
-				       struct ieee80211_sub_if_data *sdata,
-				       struct ieee80211_sta *sta,
-				       u16 old_links, u16 new_links)
-{
-	int ret = -EOPNOTSUPP;
-
-	might_sleep();
-
-	if (!check_sdata_in_driver(sdata))
-		return -EIO;
-
-	trace_drv_change_sta_links(local, sdata, sta, old_links, new_links);
-	if (local->ops->change_sta_links)
-		ret = local->ops->change_sta_links(&local->hw, &sdata->vif, sta,
-						   old_links, new_links);
-	trace_drv_return_int(local, ret);
-
-	return ret;
-}
+int drv_change_vif_links(struct ieee80211_local *local,
+			 struct ieee80211_sub_if_data *sdata,
+			 u16 old_links, u16 new_links,
+			 struct ieee80211_bss_conf *old[IEEE80211_MLD_MAX_NUM_LINKS]);
+int drv_change_sta_links(struct ieee80211_local *local,
+			 struct ieee80211_sub_if_data *sdata,
+			 struct ieee80211_sta *sta,
+			 u16 old_links, u16 new_links);
 
 #endif /* __MAC80211_DRIVER_OPS */
diff --git a/net/mac80211/key.c b/net/mac80211/key.c
index d89ec93b243b..f6f0f65fb255 100644
--- a/net/mac80211/key.c
+++ b/net/mac80211/key.c
@@ -177,6 +177,10 @@  static int ieee80211_key_enable_hw_accel(struct ieee80211_key *key)
 		}
 	}
 
+	if (key->conf.link_id >= 0 && sdata->vif.active_links &&
+	    !(sdata->vif.active_links & BIT(key->conf.link_id)))
+		return 0;
+
 	ret = drv_set_key(key->local, SET_KEY, sdata,
 			  sta ? &sta->sta : NULL, &key->conf);
 
@@ -246,6 +250,10 @@  static void ieee80211_key_disable_hw_accel(struct ieee80211_key *key)
 	sta = key->sta;
 	sdata = key->sdata;
 
+	if (key->conf.link_id >= 0 && sdata->vif.active_links &&
+	    !(sdata->vif.active_links & BIT(key->conf.link_id)))
+		return;
+
 	if (!(key->conf.flags & (IEEE80211_KEY_FLAG_GENERATE_MMIC |
 				 IEEE80211_KEY_FLAG_PUT_MIC_SPACE |
 				 IEEE80211_KEY_FLAG_RESERVE_TAILROOM)))
diff --git a/net/mac80211/link.c b/net/mac80211/link.c
index 096f313c2a6e..8df348a5edce 100644
--- a/net/mac80211/link.c
+++ b/net/mac80211/link.c
@@ -73,28 +73,37 @@  struct link_container {
 	struct ieee80211_bss_conf conf;
 };
 
-static void ieee80211_free_links(struct ieee80211_sub_if_data *sdata,
-				 struct link_container **links)
+static void ieee80211_tear_down_links(struct ieee80211_sub_if_data *sdata,
+				      struct link_container **links, u16 mask)
 {
+	struct ieee80211_link_data *link;
 	LIST_HEAD(keys);
 	unsigned int link_id;
 
 	for (link_id = 0; link_id < IEEE80211_MLD_MAX_NUM_LINKS; link_id++) {
-		if (!links[link_id])
+		if (!(mask & BIT(link_id)))
 			continue;
-		ieee80211_remove_link_keys(&links[link_id]->data, &keys);
+		link = &links[link_id]->data;
+		if (link_id == 0 && !link)
+			link = &sdata->deflink;
+		if (WARN_ON(!link))
+			continue;
+		ieee80211_remove_link_keys(link, &keys);
+		ieee80211_link_stop(link);
 	}
 
 	synchronize_rcu();
 
 	ieee80211_free_key_list(sdata->local, &keys);
+}
 
-	for (link_id = 0; link_id < IEEE80211_MLD_MAX_NUM_LINKS; link_id++) {
-		if (!links[link_id])
-			continue;
-		ieee80211_link_stop(&links[link_id]->data);
+static void ieee80211_free_links(struct ieee80211_sub_if_data *sdata,
+				 struct link_container **links)
+{
+	unsigned int link_id;
+
+	for (link_id = 0; link_id < IEEE80211_MLD_MAX_NUM_LINKS; link_id++)
 		kfree(links[link_id]);
-	}
 }
 
 static int ieee80211_check_dup_link_addrs(struct ieee80211_sub_if_data *sdata)
@@ -123,11 +132,38 @@  static int ieee80211_check_dup_link_addrs(struct ieee80211_sub_if_data *sdata)
 	return 0;
 }
 
+static void ieee80211_set_vif_links_bitmaps(struct ieee80211_sub_if_data *sdata,
+					    u16 links)
+{
+	sdata->vif.valid_links = links;
+
+	if (!links) {
+		sdata->vif.active_links = 0;
+		return;
+	}
+
+	switch (sdata->vif.type) {
+	case NL80211_IFTYPE_AP:
+		/* in an AP all links are always active */
+		sdata->vif.active_links = links;
+		break;
+	case NL80211_IFTYPE_STATION:
+		if (sdata->vif.active_links)
+			break;
+		WARN_ON(hweight16(links) > 1);
+		sdata->vif.active_links = links;
+		break;
+	default:
+		WARN_ON(1);
+	}
+}
+
 static int ieee80211_vif_update_links(struct ieee80211_sub_if_data *sdata,
 				      struct link_container **to_free,
 				      u16 new_links)
 {
 	u16 old_links = sdata->vif.valid_links;
+	u16 old_active = sdata->vif.active_links;
 	unsigned long add = new_links & ~old_links;
 	unsigned long rem = old_links & ~new_links;
 	unsigned int link_id;
@@ -195,13 +231,17 @@  static int ieee80211_vif_update_links(struct ieee80211_sub_if_data *sdata,
 		ieee80211_link_init(sdata, -1, &sdata->deflink,
 				    &sdata->vif.bss_conf);
 
-	sdata->vif.valid_links = new_links;
-
 	ret = ieee80211_check_dup_link_addrs(sdata);
 	if (!ret) {
+		/* for keys we will not be able to undo this */
+		ieee80211_tear_down_links(sdata, to_free, rem);
+
+		ieee80211_set_vif_links_bitmaps(sdata, new_links);
+
 		/* tell the driver */
 		ret = drv_change_vif_links(sdata->local, sdata,
-					   old_links, new_links,
+					   old_links & old_active,
+					   new_links & sdata->vif.active_links,
 					   old);
 	}
 
@@ -209,7 +249,7 @@  static int ieee80211_vif_update_links(struct ieee80211_sub_if_data *sdata,
 		/* restore config */
 		memcpy(sdata->link, old_data, sizeof(old_data));
 		memcpy(sdata->vif.link_conf, old, sizeof(old));
-		sdata->vif.valid_links = old_links;
+		ieee80211_set_vif_links_bitmaps(sdata, old_links);
 		/* and free (only) the newly allocated links */
 		memset(to_free, 0, sizeof(links));
 		goto free;
diff --git a/net/mac80211/mlme.c b/net/mac80211/mlme.c
index 4c8bbf0bd25b..30edac8724d5 100644
--- a/net/mac80211/mlme.c
+++ b/net/mac80211/mlme.c
@@ -4043,11 +4043,11 @@  static bool ieee80211_assoc_config_link(struct ieee80211_link_data *link,
 		goto out;
 	}
 
-	sband = ieee80211_get_link_sband(link);
-	if (!sband) {
+	if (WARN_ON(!link->conf->chandef.chan)) {
 		ret = false;
 		goto out;
 	}
+	sband = local->hw.wiphy->bands[link->conf->chandef.chan->band];
 
 	if (!(link->u.mgd.conn_flags & IEEE80211_CONN_DISABLE_HE) &&
 	    (!elems->he_cap || !elems->he_operation)) {
@@ -4871,8 +4871,10 @@  static bool ieee80211_assoc_success(struct ieee80211_sub_if_data *sdata,
 			err = ieee80211_prep_channel(sdata, link,
 						     assoc_data->link[link_id].bss,
 						     &link->u.mgd.conn_flags);
-			if (err)
+			if (err) {
+				link_info(link, "prep_channel failed\n");
 				goto out_err;
+			}
 		}
 
 		err = ieee80211_mgd_setup_link_sta(link, sta, link_sta,
@@ -6876,23 +6878,6 @@  int ieee80211_mgd_assoc(struct ieee80211_sub_if_data *sdata,
 	for (i = 0; i < IEEE80211_MLD_MAX_NUM_LINKS; i++)
 		size += req->links[i].elems_len;
 
-	if (req->ap_mld_addr) {
-		for (i = 0; i < IEEE80211_MLD_MAX_NUM_LINKS; i++) {
-			if (!req->links[i].bss)
-				continue;
-			if (i == assoc_link_id)
-				continue;
-			/*
-			 * For now, support only a single link in MLO, we
-			 * don't have the necessary parsing of the multi-
-			 * link element in the association response, etc.
-			 */
-			sdata_info(sdata,
-				   "refusing MLO association with >1 links\n");
-			return -EINVAL;
-		}
-	}
-
 	assoc_data = kzalloc(size, GFP_KERNEL);
 	if (!assoc_data)
 		return -ENOMEM;
diff --git a/net/mac80211/util.c b/net/mac80211/util.c
index 8b4c6b7abafa..0eed18189491 100644
--- a/net/mac80211/util.c
+++ b/net/mac80211/util.c
@@ -2902,7 +2902,7 @@  void ieee80211_recalc_min_chandef(struct ieee80211_sub_if_data *sdata,
 		 */
 		rcu_read_unlock();
 
-		if (WARN_ON_ONCE(!chanctx_conf))
+		if (!chanctx_conf)
 			goto unlock;
 
 		chanctx = container_of(chanctx_conf, struct ieee80211_chanctx,