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

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

[Wen Gong]

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

-- 
ath11k mailing list
ath11k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath11k

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

[Johannes Berg]

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
> 

-- 
ath11k mailing list
ath11k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath11k

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

[Wen Gong]

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?

-- 
ath11k mailing list
ath11k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath11k

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

[Johannes Berg]

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


-- 
ath11k mailing list
ath11k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath11k

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

[Wen Gong]

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
>

-- 
ath11k mailing list
ath11k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath11k

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

[Johannes Berg]

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


-- 
ath11k mailing list
ath11k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath11k

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

[Wen Gong]

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.

-- 
ath11k mailing list
ath11k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath11k

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

[Wen Gong]

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.

-- 
ath11k mailing list
ath11k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath11k

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

[Wen Gong]

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

-- 
ath11k mailing list
ath11k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath11k

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

[Johannes Berg]

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

-- 
ath11k mailing list
ath11k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath11k

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

[Wen Gong]

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?😁


-- 
ath11k mailing list
ath11k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath11k

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

[Johannes Berg]

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

-- 
ath11k mailing list
ath11k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath11k

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

[Wen Gong]

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.

--- a/include/net/cfg80211.h
+++ b/include/net/cfg80211.h
@@ -5079,6 +5079,7 @@ struct wiphy_iftype_ext_capab {
         u8 extended_capabilities_len;
         u16 eml_capabilities;
         u16 mld_capa_and_ops;
+       struct mac_address assoc_link_addr;
  };

...


-- 
ath11k mailing list
ath11k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath11k

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

[Johannes Berg]

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

-- 
ath11k mailing list
ath11k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath11k

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

[Wen Gong]

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?

-- 
ath11k mailing list
ath11k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath11k

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

[Johannes Berg]

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

-- 
ath11k mailing list
ath11k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath11k

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

[Wen Gong]

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

-- 
ath11k mailing list
ath11k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath11k

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

[Johannes Berg]

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

-- 
ath11k mailing list
ath11k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath11k

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

[Wen Gong]

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


-- 
ath11k mailing list
ath11k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath11k

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

[Johannes Berg]

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

-- 
ath11k mailing list
ath11k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath11k

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

[Wen Gong]

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

-- 
ath11k mailing list
ath11k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath11k

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

[Johannes Berg]

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

-- 
ath11k mailing list
ath11k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath11k

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

[Wen Gong]

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

-- 
ath11k mailing list
ath11k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath11k

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

[Johannes Berg]

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
> 

-- 
ath11k mailing list
ath11k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath11k

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

[Johannes Berg]

(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

-- 
ath11k mailing list
ath11k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath11k

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

[Wen Gong]

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
The patches should be "wifi: mac80211: implement link switching" for 
ieee80211_set_active_links().

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;

-- 
ath11k mailing list
ath11k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath11k

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

[Johannes Berg]

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

-- 
ath11k mailing list
ath11k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath11k

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

[Wen Gong]

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

-- 
ath11k mailing list
ath11k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath11k

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

[Johannes Berg]

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

-- 
ath11k mailing list
ath11k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath11k

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

[Wen Gong]

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.


-- 
ath11k mailing list
ath11k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath11k

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

[Johannes Berg]

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

-- 
ath11k mailing list
ath11k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath11k

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

[Wen Gong]

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

-- 
ath11k mailing list
ath11k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath11k

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

[Wen Gong]

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

Hi Johannes,

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

-- 
ath11k mailing list
ath11k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath11k

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

[Wen Gong]

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

-- 
ath11k mailing list
ath11k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath11k

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

[Johannes Berg]

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

-- 
ath11k mailing list
ath11k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath11k

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

[Wen Gong]

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?😁


-- 
ath11k mailing list
ath11k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath11k

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

[Johannes Berg]

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

-- 
ath11k mailing list
ath11k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath11k

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

[Wen Gong]

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

-- 
ath11k mailing list
ath11k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath11k

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

[Wen Gong]

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

-- 
ath11k mailing list
ath11k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath11k

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

[Wen Gong]

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

-- 
ath11k mailing list
ath11k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath11k

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

[Wen Gong]

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

-- 
ath11k mailing list
ath11k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath11k

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

[Johannes Berg]

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

-- 
ath11k mailing list
ath11k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath11k

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

[Wen Gong]

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

-- 
ath11k mailing list
ath11k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath11k

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

[Johannes Berg]

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
> 

-- 
ath11k mailing list
ath11k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath11k

Re: [PATCH 00/27] another set of MLO patches

[Wen Gong]

On 9/12/2022 9:17 PM, Otcheretianski, Andrei wrote:
>>> Well, OK, you obviously are adjusting the supplicant to work with MLO
>>> (otherwise you wouldn't get an MLO connection in the first place), so
>>> yeah, this is part of the adjustments needed.
>>>
>>> Ilan/Andrei have all of this working, maybe we can share the patches
>>> even before rebase etc.
> Hi,
> Our implementation is based on our internal tree, so it will take some time to cleanup and port it for upstream.
> Hopefully I will have some time to work on it this and next week and maybe we will be able to share something initial.
May I get your patches?
>
> Andrei
>>> johannes
>> Thanks.
>>
>> It is good to share me the wpa_supplicant patches ASAP.
>>
>> And I have another question:
>>
>> When mac80211 use the MLD addr in authentication/assoc request,
>>
>> finally, it should be replaced with one link's address in air port, right?
>>
>> It means the MLD addr will never exist in mac80211 header of packet in the
>> air port, right?

-- 
ath11k mailing list
ath11k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath11k

Re: [PATCH 00/27] another set of MLO patches

[Wen Gong]

Hi Ilan/Andrei,

Will you send your patches of wpa_supplicant to upstream?😁

On 9/28/2022 11:12 PM, Wen Gong wrote:
> On 9/12/2022 9:17 PM, Otcheretianski, Andrei wrote:
>>>> Well, OK, you obviously are adjusting the supplicant to work with MLO
>>>> (otherwise you wouldn't get an MLO connection in the first place), so
>>>> yeah, this is part of the adjustments needed.
>>>>
>>>> Ilan/Andrei have all of this working, maybe we can share the patches
>>>> even before rebase etc.
>> Hi,
>> Our implementation is based on our internal tree, so it will take 
>> some time to cleanup and port it for upstream.
>> Hopefully I will have some time to work on it this and next week and 
>> maybe we will be able to share something initial.
> May I get your patches?
>>
>> Andrei
>>>> johannes
>>> Thanks.
>>>
>>> It is good to share me the wpa_supplicant patches ASAP.
>>>
>>> And I have another question:
>>>
>>> When mac80211 use the MLD addr in authentication/assoc request,
>>>
>>> finally, it should be replaced with one link's address in air port, 
>>> right?
>>>
>>> It means the MLD addr will never exist in mac80211 header of packet 
>>> in the
>>> air port, right?

-- 
ath11k mailing list
ath11k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath11k

wifi: hostapd:/wpa_supplicant MLO Re: [PATCH 00/27] another set of MLO patches

[Wen Gong]

Hi Ilan/Andrei,

Will you send your patches of wpa_supplicant for MLO to upstream?😁

On 10/11/2022 10:28 AM, Wen Gong wrote:
> Hi Ilan/Andrei,
>
> Will you send your patches of wpa_supplicant to upstream?😁
>
> On 9/28/2022 11:12 PM, Wen Gong wrote:
>> On 9/12/2022 9:17 PM, Otcheretianski, Andrei wrote:
>>>>> Well, OK, you obviously are adjusting the supplicant to work with MLO
>>>>> (otherwise you wouldn't get an MLO connection in the first place), so
>>>>> yeah, this is part of the adjustments needed.
>>>>>
>>>>> Ilan/Andrei have all of this working, maybe we can share the patches
>>>>> even before rebase etc.
>>> Hi,
>>> Our implementation is based on our internal tree, so it will take 
>>> some time to cleanup and port it for upstream.
>>> Hopefully I will have some time to work on it this and next week and 
>>> maybe we will be able to share something initial.
>> May I get your patches?
>>>
>>> Andrei
>>>>> johannes
>>>> Thanks.
>>>>
>>>> It is good to share me the wpa_supplicant patches ASAP.
>>>>
>>>> And I have another question:
>>>>
>>>> When mac80211 use the MLD addr in authentication/assoc request,
>>>>
>>>> finally, it should be replaced with one link's address in air port, 
>>>> right?
>>>>
>>>> It means the MLD addr will never exist in mac80211 header of packet 
>>>> in the
>>>> air port, right?

-- 
ath11k mailing list
ath11k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath11k

RE: wifi: hostapd:/wpa_supplicant MLO Re: [PATCH 00/27] another set of MLO patches

[Otcheretianski, Andrei]

> Hi Ilan/Andrei,
> 
> Will you send your patches of wpa_supplicant for MLO to upstream?😁
Hi,
Yeah, sorry, still WIP. We've been on a long holidays period until yesterday.

Andrei
> 
> On 10/11/2022 10:28 AM, Wen Gong wrote:
> > Hi Ilan/Andrei,
> >
> > Will you send your patches of wpa_supplicant to upstream?😁
> >
> > On 9/28/2022 11:12 PM, Wen Gong wrote:
> >> On 9/12/2022 9:17 PM, Otcheretianski, Andrei wrote:
> >>>>> Well, OK, you obviously are adjusting the supplicant to work with
> >>>>> MLO (otherwise you wouldn't get an MLO connection in the first
> >>>>> place), so yeah, this is part of the adjustments needed.
> >>>>>
> >>>>> Ilan/Andrei have all of this working, maybe we can share the
> >>>>> patches even before rebase etc.
> >>> Hi,
> >>> Our implementation is based on our internal tree, so it will take
> >>> some time to cleanup and port it for upstream.
> >>> Hopefully I will have some time to work on it this and next week and
> >>> maybe we will be able to share something initial.
> >> May I get your patches?
> >>>
> >>> Andrei
> >>>>> johannes
> >>>> Thanks.
> >>>>
> >>>> It is good to share me the wpa_supplicant patches ASAP.
> >>>>
> >>>> And I have another question:
> >>>>
> >>>> When mac80211 use the MLD addr in authentication/assoc request,
> >>>>
> >>>> finally, it should be replaced with one link's address in air port,
> >>>> right?
> >>>>
> >>>> It means the MLD addr will never exist in mac80211 header of packet
> >>>> in the air port, right?
-- 
ath11k mailing list
ath11k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath11k

Re: wifi: hostapd:/wpa_supplicant MLO Re: [PATCH 00/27] another set of MLO patches

[Wen Gong]

Hi Andrei,

Is below all your patches for MLO in wpa_suppplicant for station mode?

[00/13] MLD STA: Add SME MLO support

https://patchwork.ozlabs.org/project/hostap/list/?series=329909&state=*

On 10/19/2022 6:04 PM, Wen Gong wrote:
> Hi Ilan/Andrei,
>
> Will you send your patches of wpa_supplicant for MLO to upstream?😁
>
> On 10/11/2022 10:28 AM, Wen Gong wrote:
>> Hi Ilan/Andrei,
>>
>> Will you send your patches of wpa_supplicant to upstream?😁
>>
>> On 9/28/2022 11:12 PM, Wen Gong wrote:
>>> On 9/12/2022 9:17 PM, Otcheretianski, Andrei wrote:
>>>>>> Well, OK, you obviously are adjusting the supplicant to work with 
>>>>>> MLO
>>>>>> (otherwise you wouldn't get an MLO connection in the first 
>>>>>> place), so
>>>>>> yeah, this is part of the adjustments needed.
>>>>>>
>>>>>> Ilan/Andrei have all of this working, maybe we can share the patches
>>>>>> even before rebase etc.
>>>> Hi,
>>>> Our implementation is based on our internal tree, so it will take 
>>>> some time to cleanup and port it for upstream.
>>>> Hopefully I will have some time to work on it this and next week 
>>>> and maybe we will be able to share something initial.
>>> May I get your patches?
>>>>
>>>> Andrei
>>>>>> johannes
>>>>> Thanks.
>>>>>
>>>>> It is good to share me the wpa_supplicant patches ASAP.
>>>>>
>>>>> And I have another question:
>>>>>
>>>>> When mac80211 use the MLD addr in authentication/assoc request,
>>>>>
>>>>> finally, it should be replaced with one link's address in air 
>>>>> port, right?
>>>>>
>>>>> It means the MLD addr will never exist in mac80211 header of 
>>>>> packet in the
>>>>> air port, right?

-- 
ath11k mailing list
ath11k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath11k

RE: wifi: hostapd:/wpa_supplicant MLO Re: [PATCH 00/27] another set of MLO patches

[Otcheretianski, Andrei]

> Hi Andrei,
> 
> Is below all your patches for MLO in wpa_suppplicant for station mode?

Hi Wen,
We have few more for station side, like SAE/PMKSA support and some additional configs - but this is mostly what I have.
SAE support patch is somewhat similar to patches sent by Veerendranath.
We have more stuff for AP side (mostly for testing purposes), hwsim tests etc.. I'm starting to clean this up and will send it as well.

Andrei
-- 
ath11k mailing list
ath11k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath11k

Re: wifi: hostapd:/wpa_supplicant MLO Re: [PATCH 00/27] another set of MLO patches

[Wen Gong]

On 11/28/2022 10:05 PM, Otcheretianski, Andrei wrote:
>> Hi Andrei,
>>
>> Is below all your patches for MLO in wpa_suppplicant for station mode?
> Hi Wen,
> We have few more for station side, like SAE/PMKSA support and some additional configs - but this is mostly what I have.
> SAE support patch is somewhat similar to patches sent by Veerendranath.
> We have more stuff for AP side (mostly for testing purposes), hwsim tests etc.. I'm starting to clean this up and will send it as well.
>
> Andrei
Thanks Andrei, did you tested your station with single link or 2 link or 
more link?

-- 
ath11k mailing list
ath11k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath11k

RE: wifi: hostapd:/wpa_supplicant MLO Re: [PATCH 00/27] another set of MLO patches

[Otcheretianski, Andrei]

> Thanks Andrei, did you tested your station with single link or 2 link or more
> link?

Tested with 1 and 2 links, but should also work with more links as well

Andrei
-- 
ath11k mailing list
ath11k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath11k

Re: wifi: hostapd:/wpa_supplicant MLO Re: [PATCH 00/27] another set of MLO patches

[Wen Gong]

On 11/29/2022 2:59 PM, Otcheretianski, Andrei wrote:
>> Thanks Andrei, did you tested your station with single link or 2 link or more
>> link?
> Tested with 1 and 2 links, but should also work with more links as well
>
> Andrei
Thanks a lot!

-- 
ath11k mailing list
ath11k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath11k

Re: [PATCH 24/27] wifi: mac80211: implement link switching

[Wen Gong]

On 9/2/2022 10:12 PM, Johannes Berg wrote:
> From: Johannes Berg <johannes.berg@intel.com>
>
> Implement an API function and debugfs file to switch
> active links.
>
> Also provide an async version of the API so drivers
> can call it in arbitrary contexts, e.g. while in the
> authorized callback.
>
> Signed-off-by: Johannes Berg <johannes.berg@intel.com>
> ---
>   include/net/mac80211.h        |  41 ++++++++
>   net/mac80211/debugfs_netdev.c |  26 ++++++
>   net/mac80211/ieee80211_i.h    |   4 +
>   net/mac80211/iface.c          |  12 +++
>   net/mac80211/key.c            |  34 +++++++
>   net/mac80211/key.h            |   3 +
>   net/mac80211/link.c           | 171 ++++++++++++++++++++++++++++++++++
>   7 files changed, 291 insertions(+)
>
> ...
> +static int _ieee80211_set_active_links(struct ieee80211_sub_if_data *sdata,
> +				       u16 active_links)
> +{
> +	struct ieee80211_bss_conf *link_confs[IEEE80211_MLD_MAX_NUM_LINKS];
> +	struct ieee80211_local *local = sdata->local;
> +	u16 old_active = sdata->vif.active_links;
> +	unsigned long rem = old_active & ~active_links;
> +	unsigned long add = active_links & ~old_active;
> +	struct sta_info *sta;
> +	unsigned int link_id;
> +	int ret, i;
> +
> +	if (!ieee80211_sdata_running(sdata))
> +		return -ENETDOWN;
> +
> +	if (sdata->vif.type != NL80211_IFTYPE_STATION)
> +		return -EINVAL;
> +
> +	/* cannot activate links that don't exist */
> +	if (active_links & ~sdata->vif.valid_links)
> +		return -EINVAL;
> +
> +	/* nothing to do */
> +	if (old_active == active_links)
> +		return 0;
> +
> +	for (i = 0; i < IEEE80211_MLD_MAX_NUM_LINKS; i++)
> +		link_confs[i] = sdata_dereference(sdata->vif.link_conf[i],
> +						  sdata);
> +
> +	if (add) {
> +		sdata->vif.active_links |= active_links;
> +		ret = drv_change_vif_links(local, sdata,
> +					   old_active,
> +					   sdata->vif.active_links,
> +					   link_confs);
> +		if (ret) {
> +			sdata->vif.active_links = old_active;
> +			return ret;
> +		}
> +	}
> +
> +	for_each_set_bit(link_id, &rem, IEEE80211_MLD_MAX_NUM_LINKS) {
> +		struct ieee80211_link_data *link;
> +
> +		link = sdata_dereference(sdata->link[link_id], sdata);
> +
> +		/* FIXME: kill TDLS connections on the link */
> +
> +		ieee80211_link_release_channel(link);
> +	}
> +
> +	list_for_each_entry(sta, &local->sta_list, list) {
> +		if (sdata != sta->sdata)
> +			continue;
> +		ret = drv_change_sta_links(local, sdata, &sta->sta,
> +					   old_active,
> +					   old_active | active_links);
> +		WARN_ON_ONCE(ret);
> +	}
> +
> +	ret = ieee80211_key_switch_links(sdata, rem, add);

I see ieee80211_key_switch_link() only handler the per-link(link_id >= 
0) keys,

So I think lower driver also install the pairwise keys(link_id = -1) for 
the added links at this moment?

> +	WARN_ON_ONCE(ret);
> +
> +	list_for_each_entry(sta, &local->sta_list, list) {
> +		if (sdata != sta->sdata)
> +			continue;
> +		ret = drv_change_sta_links(local, sdata, &sta->sta,
> +					   old_active | active_links,
> +					   active_links);
> +		WARN_ON_ONCE(ret);
> +	}
> +

I see 2 times to call drv_change_sta_link() above, and with sequence  
old_active->old_active | active_links->active_links

May I know is it has some design here?

> +	for_each_set_bit(link_id, &add, IEEE80211_MLD_MAX_NUM_LINKS) {
> +		struct ieee80211_link_data *link;
> +
> +		link = sdata_dereference(sdata->link[link_id], sdata);
> +
> +		ret = ieee80211_link_use_channel(link, &link->conf->chandef,
> +						 IEEE80211_CHANCTX_SHARED);

For the 1st link of MLO connection/NON-MLO connetion, ieee80211_link_use_channel() is called before drv_change_sta_link(),
And now it is after drv_change_sta_link(), May I know is it also has some design here?

Also I see commit(8fb7e2ef4bab mac80211_hwsim: always activate all links) and ieee80211_if_parse_active_links()
will use ieee80211_set_active_links(), so I think ieee80211_set_active_links() has passed test case with some type lower driver/chip?

> +		WARN_ON_ONCE(ret);
> +
> +		ieee80211_link_info_change_notify(sdata, link,
> +						  BSS_CHANGED_ERP_CTS_PROT |
> +						  BSS_CHANGED_ERP_PREAMBLE |
> +						  BSS_CHANGED_ERP_SLOT |
> +						  BSS_CHANGED_HT |
> +						  BSS_CHANGED_BASIC_RATES |
> +						  BSS_CHANGED_BSSID |
> +						  BSS_CHANGED_CQM |
> +						  BSS_CHANGED_QOS |
> +						  BSS_CHANGED_TXPOWER |
> +						  BSS_CHANGED_BANDWIDTH |
> +						  BSS_CHANGED_TWT |
> +						  BSS_CHANGED_HE_OBSS_PD |
> +						  BSS_CHANGED_HE_BSS_COLOR);
> +		ieee80211_mgd_set_link_qos_params(link);
> +	}
> +
> +	old_active = sdata->vif.active_links;
> +	sdata->vif.active_links = active_links;
> +
> +	if (rem) {
> +		ret = drv_change_vif_links(local, sdata, old_active,
> +					   active_links, link_confs);
> +		WARN_ON_ONCE(ret);
> +	}
> +
> +	return 0;
> +}
> +
...

-- 
ath11k mailing list
ath11k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath11k

Re: [PATCH 24/27] wifi: mac80211: implement link switching

[Johannes Berg]

Hi,

> > +	list_for_each_entry(sta, &local->sta_list, list) {
> > +		if (sdata != sta->sdata)
> > +			continue;
> > +		ret = drv_change_sta_links(local, sdata, &sta->sta,
> > +					   old_active,
> > +					   old_active | active_links);
> > +		WARN_ON_ONCE(ret);
> > +	}
> > +
> > +	ret = ieee80211_key_switch_links(sdata, rem, add);
> 
> I see ieee80211_key_switch_link() only handler the per-link(link_id >= 
> 0) keys,
> 
> So I think lower driver also install the pairwise keys(link_id = -1) for 
> the added links at this moment?

Well from mac80211 POV they're already installed, so we can't really
install them again. We'd have to remove them but that's racy, obviously.
So I think the low-level driver just has to handle that, e.g. when the
station links are updated (and the key belongs to the station.)

> > +	WARN_ON_ONCE(ret);
> > +
> > +	list_for_each_entry(sta, &local->sta_list, list) {
> > +		if (sdata != sta->sdata)
> > +			continue;
> > +		ret = drv_change_sta_links(local, sdata, &sta->sta,
> > +					   old_active | active_links,
> > +					   active_links);
> > +		WARN_ON_ONCE(ret);
> > +	}
> > +
> 
> I see 2 times to call drv_change_sta_link() above, and with sequence  
> old_active->old_active | active_links->active_links
> 
> May I know is it has some design here?

The problem is that we can't really have no links active even as an
intermediate step, so you can't just deactivate old and then activate
new.

> > +	for_each_set_bit(link_id, &add, IEEE80211_MLD_MAX_NUM_LINKS) {
> > +		struct ieee80211_link_data *link;
> > +
> > +		link = sdata_dereference(sdata->link[link_id], sdata);
> > +
> > +		ret = ieee80211_link_use_channel(link, &link->conf->chandef,
> > +						 IEEE80211_CHANCTX_SHARED);
> 
> For the 1st link of MLO connection/NON-MLO connetion, ieee80211_link_use_channel() is called before drv_change_sta_link(),
> And now it is after drv_change_sta_link(), May I know is it also has some design here?

Hmm, probably not really, at least I don't remember anything about that.

Not sure it makes a huge difference? But I suppose we could change it, I
don't really see why not either.

> Also I see commit(8fb7e2ef4bab mac80211_hwsim: always activate all links) and ieee80211_if_parse_active_links()
> will use ieee80211_set_active_links(), so I think ieee80211_set_active_links() has passed test case with some type lower driver/chip?

Yes, we have this working on iwlwifi/mvm.

johannes

-- 
ath11k mailing list
ath11k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath11k

Re: [PATCH 24/27] wifi: mac80211: implement link switching

[Wen Gong]

On 3/27/2023 4:31 PM, Johannes Berg wrote:
> Hi,
>
>>> +	list_for_each_entry(sta, &local->sta_list, list) {
>>> +		if (sdata != sta->sdata)
>>> +			continue;
>>> +		ret = drv_change_sta_links(local, sdata, &sta->sta,
>>> +					   old_active,
>>> +					   old_active | active_links);
>>> +		WARN_ON_ONCE(ret);
>>> +	}
>>> +
>>> +	ret = ieee80211_key_switch_links(sdata, rem, add);
>> I see ieee80211_key_switch_link() only handler the per-link(link_id >=
>> 0) keys,
>>
>> So I think lower driver also install the pairwise keys(link_id = -1) for
>> the added links at this moment?
> Well from mac80211 POV they're already installed, so we can't really
> install them again. We'd have to remove them but that's racy, obviously.
> So I think the low-level driver just has to handle that, e.g. when the
> station links are updated (and the key belongs to the station.)
Got it, thanks.
>
>>> +	WARN_ON_ONCE(ret);
>>> +
>>> +	list_for_each_entry(sta, &local->sta_list, list) {
>>> +		if (sdata != sta->sdata)
>>> +			continue;
>>> +		ret = drv_change_sta_links(local, sdata, &sta->sta,
>>> +					   old_active | active_links,
>>> +					   active_links);
>>> +		WARN_ON_ONCE(ret);
>>> +	}
>>> +
>> I see 2 times to call drv_change_sta_link() above, and with sequence
>> old_active->old_active | active_links->active_links
>>
>> May I know is it has some design here?
> The problem is that we can't really have no links active even as an
> intermediate step, so you can't just deactivate old and then activate
> new.
Got it, thanks.
>
>>> +	for_each_set_bit(link_id, &add, IEEE80211_MLD_MAX_NUM_LINKS) {
>>> +		struct ieee80211_link_data *link;
>>> +
>>> +		link = sdata_dereference(sdata->link[link_id], sdata);
>>> +
>>> +		ret = ieee80211_link_use_channel(link, &link->conf->chandef,
>>> +						 IEEE80211_CHANCTX_SHARED);
>> For the 1st link of MLO connection/NON-MLO connetion, ieee80211_link_use_channel() is called before drv_change_sta_link(),
>> And now it is after drv_change_sta_link(), May I know is it also has some design here?
> Hmm, probably not really, at least I don't remember anything about that.
>
> Not sure it makes a huge difference? But I suppose we could change it, I
> don't really see why not either.
Not huge difference, I have made little change in lower-driver to match 
that. So it is OK now.
>> Also I see commit(8fb7e2ef4bab mac80211_hwsim: always activate all links) and ieee80211_if_parse_active_links()
>> will use ieee80211_set_active_links(), so I think ieee80211_set_active_links() has passed test case with some type lower driver/chip?
> Yes, we have this working on iwlwifi/mvm.

Got it, thanks.

I also have tested ieee80211_set_active_links() to enable the 2nd link 
for station success in my lower-driver after a little change in 
lower-driver.

>
> johannes

-- 
ath11k mailing list
ath11k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath11k

Re: [PATCH 24/27] wifi: mac80211: implement link switching

[Johannes Berg]

On Mon, 2023-03-27 at 16:40 +0800, Wen Gong wrote:
> > 
> > > > +	for_each_set_bit(link_id, &add, IEEE80211_MLD_MAX_NUM_LINKS) {
> > > > +		struct ieee80211_link_data *link;
> > > > +
> > > > +		link = sdata_dereference(sdata->link[link_id], sdata);
> > > > +
> > > > +		ret = ieee80211_link_use_channel(link, &link->conf->chandef,
> > > > +						 IEEE80211_CHANCTX_SHARED);
> > > For the 1st link of MLO connection/NON-MLO connetion, ieee80211_link_use_channel() is called before drv_change_sta_link(),
> > > And now it is after drv_change_sta_link(), May I know is it also has some design here?
> > Hmm, probably not really, at least I don't remember anything about that.
> > 
> > Not sure it makes a huge difference? But I suppose we could change it, I
> > don't really see why not either.
> Not huge difference, I have made little change in lower-driver to match 
> that. So it is OK now.

OK. Still maybe we should change it for consistency? I can try that
later with our driver.

johannes


-- 
ath11k mailing list
ath11k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath11k

Re: [PATCH 24/27] wifi: mac80211: implement link switching

[Wen Gong]

On 3/27/2023 5:04 PM, Johannes Berg wrote:
> On Mon, 2023-03-27 at 16:40 +0800, Wen Gong wrote:
>>>>> +	for_each_set_bit(link_id, &add, IEEE80211_MLD_MAX_NUM_LINKS) {
>>>>> +		struct ieee80211_link_data *link;
>>>>> +
>>>>> +		link = sdata_dereference(sdata->link[link_id], sdata);
>>>>> +
>>>>> +		ret = ieee80211_link_use_channel(link, &link->conf->chandef,
>>>>> +						 IEEE80211_CHANCTX_SHARED);
>>>> For the 1st link of MLO connection/NON-MLO connetion, ieee80211_link_use_channel() is called before drv_change_sta_link(),
>>>> And now it is after drv_change_sta_link(), May I know is it also has some design here?
>>> Hmm, probably not really, at least I don't remember anything about that.
>>>
>>> Not sure it makes a huge difference? But I suppose we could change it, I
>>> don't really see why not either.
>> Not huge difference, I have made little change in lower-driver to match
>> that. So it is OK now.
> OK. Still maybe we should change it for consistency? I can try that
> later with our driver.
>
> johannes

I think it is not urgent for that:)

And lower-drvier should also handler different case.

>

-- 
ath11k mailing list
ath11k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath11k

Re: [PATCH 24/27] wifi: mac80211: implement link switching

[Wen Gong]

On 3/27/2023 4:31 PM, Johannes Berg wrote:
...
>> Also I see commit(8fb7e2ef4bab mac80211_hwsim: always activate all links) and ieee80211_if_parse_active_links()
>> will use ieee80211_set_active_links(), so I think ieee80211_set_active_links() has passed test case with some type lower driver/chip?
> Yes, we have this working on iwlwifi/mvm.
>
> johannes
May I know how did you test it?

Did you test with tool like this with parameter "ActiveTxMultiLinks"?

  ".\sigma-dut -l 
sta_set_rfeature,Interface,wlan0,prog,EHT,ActiveTxMultiLinks,02:03:7f:95:21:97,ActiveRxMultiLinks,02:03:7f:95:21:97"


-- 
ath11k mailing list
ath11k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath11k

Re: [PATCH 24/27] wifi: mac80211: implement link switching

[Johannes Berg]

On Tue, 2023-03-28 at 15:37 +0800, Wen Gong wrote:
> On 3/27/2023 4:31 PM, Johannes Berg wrote:
> ...
> > > Also I see commit(8fb7e2ef4bab mac80211_hwsim: always activate all links) and ieee80211_if_parse_active_links()
> > > will use ieee80211_set_active_links(), so I think ieee80211_set_active_links() has passed test case with some type lower driver/chip?
> > Yes, we have this working on iwlwifi/mvm.
> > 
> > johannes
> May I know how did you test it?

Just writing to the debugfs file. We have various tests using that now.


johannes

-- 
ath11k mailing list
ath11k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath11k

Re: [PATCH 24/27] wifi: mac80211: implement link switching

[Wen Gong]

On 3/28/2023 3:39 PM, Johannes Berg wrote:
> On Tue, 2023-03-28 at 15:37 +0800, Wen Gong wrote:
>> On 3/27/2023 4:31 PM, Johannes Berg wrote:
>> ...
>>>> Also I see commit(8fb7e2ef4bab mac80211_hwsim: always activate all links) and ieee80211_if_parse_active_links()
>>>> will use ieee80211_set_active_links(), so I think ieee80211_set_active_links() has passed test case with some type lower driver/chip?
>>> Yes, we have this working on iwlwifi/mvm.
>>>
>>> johannes
>> May I know how did you test it?
> Just writing to the debugfs file. We have various tests using that now.
>
Do you mean the various tests using debugfs or using 
ieee80211_set_active_links() directly?
> johannes

-- 
ath11k mailing list
ath11k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath11k

Re: [PATCH 24/27] wifi: mac80211: implement link switching

[Johannes Berg]

On Mon, 2023-04-03 at 22:15 +0800, Wen Gong wrote:
> On 3/28/2023 3:39 PM, Johannes Berg wrote:
> > On Tue, 2023-03-28 at 15:37 +0800, Wen Gong wrote:
> > > On 3/27/2023 4:31 PM, Johannes Berg wrote:
> > > ...
> > > > > Also I see commit(8fb7e2ef4bab mac80211_hwsim: always activate all links) and ieee80211_if_parse_active_links()
> > > > > will use ieee80211_set_active_links(), so I think ieee80211_set_active_links() has passed test case with some type lower driver/chip?
> > > > Yes, we have this working on iwlwifi/mvm.
> > > > 
> > > > johannes
> > > May I know how did you test it?
> > Just writing to the debugfs file. We have various tests using that now.
> > 
> Do you mean the various tests using debugfs or using 
> ieee80211_set_active_links() directly?
> 

Yeah the tests write to debugfs.

johannes

-- 
ath11k mailing list
ath11k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath11k

Re: [PATCH 24/27] wifi: mac80211: implement link switching

[Wen Gong]

On 3/27/2023 4:31 PM, Johannes Berg wrote:
> Hi,
>
>>> +	list_for_each_entry(sta, &local->sta_list, list) {
>>> +		if (sdata != sta->sdata)
>>> +			continue;
>>> +		ret = drv_change_sta_links(local, sdata, &sta->sta,
>>> +					   old_active,
>>> +					   old_active | active_links);
>>> +		WARN_ON_ONCE(ret);
>>> +	}
>>> +
>>> +	ret = ieee80211_key_switch_links(sdata, rem, add);
>> I see ieee80211_key_switch_link() only handler the per-link(link_id >=
>> 0) keys,
>>
>> So I think lower driver also install the pairwise keys(link_id = -1) for
>> the added links at this moment?
> Well from mac80211 POV they're already installed, so we can't really
> install them again. We'd have to remove them but that's racy, obviously.
> So I think the low-level driver just has to handle that, e.g. when the
> station links are updated (and the key belongs to the station.)
>
>
Thanks Johannes,

Also it does not have BSS_CHANGED_ASSOC(exists in 
ieee80211_set_associated()) for

ieee80211_link_info_change_notify()/ieee80211_vif_cfg_change_notify().

So I think low-level driver also need to auto add BSS_CHANGED_ASSOC 
logic for the added link as well as the pairwise key you said, right?



-- 
ath11k mailing list
ath11k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath11k

Re: [PATCH 24/27] wifi: mac80211: implement link switching

[Johannes Berg]

On Mon, 2023-04-03 at 22:21 +0800, Wen Gong wrote:
> 
> Also it does not have BSS_CHANGED_ASSOC(exists in 
> ieee80211_set_associated()) for
> 
> ieee80211_link_info_change_notify()/ieee80211_vif_cfg_change_notify().

Well, that's clearly intentional though, since the assoc state is at the
MLD level, so changing a link doesn't affect the assoc state, right?
This is also done through _vif_cfg_change_notify().

> So I think low-level driver also need to auto add BSS_CHANGED_ASSOC 
> logic for the added link as well as the pairwise key you said, right?
> 

That doesn't make a lot of sense as written, IMHO, but yes if you have
something you need to do on links during assoc, then you'd have to take
care of that as well, just like the keys.

johannes

-- 
ath11k mailing list
ath11k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath11k