diff mbox series

[1/3] wifi: mac80211: add support to allow driver to generate local link address for station

Message ID 20230906103458.24092-2-quic_wgong@quicinc.com
State New
Headers show
Series wifi: mac80211: change to match driver to support MLO connection | expand

Commit Message

Wen Gong Sept. 6, 2023, 10:34 a.m. UTC
Currently the local link address of all links is random generated by
eth_random_addr() in mac80211 while connecting to a MLO AP for station
mode. The MAC address of link is not passed from NL command. The 1st
link address is generated while authenticate with AP, the other links'
addresses are generated after assoc success with AP.

It is not convenient for some driver, reason is, for station mode,
the interface with its mac address is already created in driver after
wlan load, it is used for hw scan and non-MLO connection.

When connecting to MLO AP, driver reuse the interface as the 1st link of
MLO. If the mac address of the 1st link changed to a new value, then
driver need to change the mac address and do many synchronous operation
with firmware. Thus the operation become complex. After MLO disconnect,
driver need to restore the old mac address, it is also another complex
operation.

The hw scan maybe happen through the MLO connection/disconnection. And
the hw scan uses the 1st link address while MLO connected and uses the
interface address while MLO is disconnected, this leads hw scan complex.

Hence add this interface to allow driver to generate the address of each
link while MLO connection. Then driver could provide the same mac address
for the 1st link, thus hw scan/NON-MLO connection/1st link of MLO will use
the same mac address, then operation become easy.

Signed-off-by: Wen Gong <quic_wgong@quicinc.com>
---
 include/net/mac80211.h    |  7 +++++++
 net/mac80211/driver-ops.h | 21 +++++++++++++++++++++
 net/mac80211/mlme.c       | 24 ++++++++++++++++++------
 net/mac80211/trace.h      | 27 +++++++++++++++++++++++++++
 4 files changed, 73 insertions(+), 6 deletions(-)

Comments

Johannes Berg Sept. 13, 2023, 8:55 a.m. UTC | #1
On Wed, 2023-09-06 at 06:34 -0400, Wen Gong wrote:
> Currently the local link address of all links is random generated by
> eth_random_addr() in mac80211 while connecting to a MLO AP for station
> mode. The MAC address of link is not passed from NL command. The 1st
> link address is generated while authenticate with AP, the other links'
> addresses are generated after assoc success with AP.
> 
> It is not convenient for some driver, reason is, for station mode,
> the interface with its mac address is already created in driver after
> wlan load, it is used for hw scan and non-MLO connection.
> 
> When connecting to MLO AP, driver reuse the interface as the 1st link of
> MLO. If the mac address of the 1st link changed to a new value, then
> driver need to change the mac address and do many synchronous operation
> with firmware. Thus the operation become complex. After MLO disconnect,
> driver need to restore the old mac address, it is also another complex
> operation.
> 
> The hw scan maybe happen through the MLO connection/disconnection. And
> the hw scan uses the 1st link address while MLO connected and uses the
> interface address while MLO is disconnected, this leads hw scan complex.
> 
> Hence add this interface to allow driver to generate the address of each
> link while MLO connection. Then driver could provide the same mac address
> for the 1st link, thus hw scan/NON-MLO connection/1st link of MLO will use
> the same mac address, then operation become easy.

Maybe after all this explanation, all we need is a flag "reuse MLD
address for assoc link"?


> +		ret = drv_generate_link_addr(sdata->local, sdata,
> +					     link_id, link->conf->addr);
> +		if (ret)
> +			eth_random_addr(link->conf->addr);

should probably refactor this into a separate function though.

I'm also not sure how the driver even knows that a link it's being asked
to get the address for *is* the assoc link? Do you want to rely on that
being the first address handed out?

johannes
Wen Gong Sept. 15, 2023, 8:11 a.m. UTC | #2
On 9/13/2023 4:55 PM, Johannes Berg wrote:
> On Wed, 2023-09-06 at 06:34 -0400, Wen Gong wrote:
[...]
> Maybe after all this explanation, all we need is a flag "reuse MLD
> address for assoc link"?

yes. It is similar as I said before here:

https://lore.kernel.org/linux-wireless/b9c6d022-12c3-a696-c4b9-cb14a6d30a45@quicinc.com/

>
>
>> +		ret = drv_generate_link_addr(sdata->local, sdata,
>> +					     link_id, link->conf->addr);
>> +		if (ret)
>> +			eth_random_addr(link->conf->addr);
> should probably refactor this into a separate function though.
OK.
>
> I'm also not sure how the driver even knows that a link it's being asked
> to get the address for *is* the assoc link? Do you want to rely on that
> being the first address handed out?
Current I used (vif->valid_links==0) check for assoc link. When
drv_generate_link_addr() called for the assoc link, vif->valid_links
is 0, and it is not 0 for other links.
>
> johannes
>
Johannes Berg Sept. 26, 2023, 9:45 a.m. UTC | #3
On Fri, 2023-09-15 at 16:11 +0800, Wen Gong wrote:
> On 9/13/2023 4:55 PM, Johannes Berg wrote:
> > On Wed, 2023-09-06 at 06:34 -0400, Wen Gong wrote:
> [...]
> > Maybe after all this explanation, all we need is a flag "reuse MLD
> > address for assoc link"?
> 
> yes. It is similar as I said before here:
> 
> https://lore.kernel.org/linux-wireless/b9c6d022-12c3-a696-c4b9-cb14a6d30a45@quicinc.com/
> 
> > 
> > 
> > > +		ret = drv_generate_link_addr(sdata->local, sdata,
> > > +					     link_id, link->conf->addr);
> > > +		if (ret)
> > > +			eth_random_addr(link->conf->addr);
> > should probably refactor this into a separate function though.
> OK.
> > 
> > I'm also not sure how the driver even knows that a link it's being asked
> > to get the address for *is* the assoc link? Do you want to rely on that
> > being the first address handed out?
> Current I used (vif->valid_links==0) check for assoc link. When
> drv_generate_link_addr() called for the assoc link, vif->valid_links
> is 0, and it is not 0 for other links.

That seems a bit questionable?

Well then again, what do you want for AP mode? Anyway you can still
distinguish, and if we later need to change an internal API that's not
the end of the world either ...

So OK, I guess we can live with this, just would like to see it wrapped
up into a single function.

johannes
Wen Gong Sept. 26, 2023, 10:44 a.m. UTC | #4
On 9/26/2023 5:45 PM, Johannes Berg wrote:
> On Fri, 2023-09-15 at 16:11 +0800, Wen Gong wrote:
>> On 9/13/2023 4:55 PM, Johannes Berg wrote:
>>> On Wed, 2023-09-06 at 06:34 -0400, Wen Gong wrote:
>> [...]
>>> Maybe after all this explanation, all we need is a flag "reuse MLD
>>> address for assoc link"?
>> yes. It is similar as I said before here:
>>
>> https://lore.kernel.org/linux-wireless/b9c6d022-12c3-a696-c4b9-cb14a6d30a45@quicinc.com/
>>
>>>
>>>> +		ret = drv_generate_link_addr(sdata->local, sdata,
>>>> +					     link_id, link->conf->addr);
>>>> +		if (ret)
>>>> +			eth_random_addr(link->conf->addr);
>>> should probably refactor this into a separate function though.
>> OK.
>>> I'm also not sure how the driver even knows that a link it's being asked
>>> to get the address for *is* the assoc link? Do you want to rely on that
>>> being the first address handed out?
>> Current I used (vif->valid_links==0) check for assoc link. When
>> drv_generate_link_addr() called for the assoc link, vif->valid_links
>> is 0, and it is not 0 for other links.
> That seems a bit questionable?

Also we can change to indicate a new flag(e.g. 
IEEE80211_HW_MLO_FIXED_ASSOC_LINK_ADDR) to

mac80211 from driver, if the flag is not set, then keep current logic.

If it is set, then copy vif->addr to the link->conf->addr for assoc link 
in ieee80211_mgd_setup_link().

The logic keep not change for other links.

> Well then again, what do you want for AP mode? Anyway you can still
> distinguish, and if we later need to change an internal API that's not
> the end of the world either ...
I do not want any change for AP mode.
>
> So OK, I guess we can live with this, just would like to see it wrapped
> up into a single function.
>
> johannes
diff mbox series

Patch

diff --git a/include/net/mac80211.h b/include/net/mac80211.h
index 3a8a2d2c58c3..c7f92a3db359 100644
--- a/include/net/mac80211.h
+++ b/include/net/mac80211.h
@@ -3746,6 +3746,11 @@  struct ieee80211_prep_tx_info {
  *	non-MLO connections.
  *	The callback can sleep.
  *
+ * @generate_link_addr: Generate mac address of link.
+ *	This callback is optional. Returns zero if mac address is generated successfully
+ *	for the link.
+ *	This callback can sleep.
+ *
  * @prepare_multicast: Prepare for multicast filter configuration.
  *	This callback is optional, and its return value is passed
  *	to configure_filter(). This callback must be atomic.
@@ -4298,6 +4303,8 @@  struct ieee80211_ops {
 				  struct ieee80211_bss_conf *info,
 				  u64 changed);
 
+	int (*generate_link_addr)(struct ieee80211_hw *hw, struct ieee80211_vif *vif,
+				  unsigned int link_id, u8 *link_local_addr);
 	int (*start_ap)(struct ieee80211_hw *hw, struct ieee80211_vif *vif,
 			struct ieee80211_bss_conf *link_conf);
 	void (*stop_ap)(struct ieee80211_hw *hw, struct ieee80211_vif *vif,
diff --git a/net/mac80211/driver-ops.h b/net/mac80211/driver-ops.h
index c4505593ba7a..da164d4ca9a1 100644
--- a/net/mac80211/driver-ops.h
+++ b/net/mac80211/driver-ops.h
@@ -962,6 +962,27 @@  int drv_switch_vif_chanctx(struct ieee80211_local *local,
 			   struct ieee80211_vif_chanctx_switch *vifs,
 			   int n_vifs, enum ieee80211_chanctx_switch_mode mode);
 
+static inline int drv_generate_link_addr(struct ieee80211_local *local,
+					 struct ieee80211_sub_if_data *sdata,
+					 unsigned int link_id, u8 *link_local_addr)
+{
+	int ret = -EOPNOTSUPP;
+
+	might_sleep();
+
+	if (!check_sdata_in_driver(sdata))
+		return -EIO;
+
+	if (local->ops->generate_link_addr) {
+		ret = local->ops->generate_link_addr(&local->hw, &sdata->vif,
+						     link_id, link_local_addr);
+		trace_drv_generate_link_addr(local, sdata, link_local_addr, link_id);
+	}
+
+	trace_drv_return_int(local, ret);
+	return ret;
+}
+
 static inline int drv_start_ap(struct ieee80211_local *local,
 			       struct ieee80211_sub_if_data *sdata,
 			       struct ieee80211_bss_conf *link_conf)
diff --git a/net/mac80211/mlme.c b/net/mac80211/mlme.c
index f93eb38ae0b8..1679d5011fb6 100644
--- a/net/mac80211/mlme.c
+++ b/net/mac80211/mlme.c
@@ -6852,6 +6852,7 @@  void ieee80211_mgd_setup_link(struct ieee80211_link_data *link)
 	struct ieee80211_sub_if_data *sdata = link->sdata;
 	struct ieee80211_local *local = sdata->local;
 	unsigned int link_id = link->link_id;
+	int ret;
 
 	link->u.mgd.p2p_noa_index = -1;
 	link->u.mgd.conn_flags = 0;
@@ -6867,11 +6868,15 @@  void ieee80211_mgd_setup_link(struct ieee80211_link_data *link)
 	wiphy_delayed_work_init(&link->u.mgd.chswitch_work,
 				ieee80211_chswitch_work);
 
-	if (sdata->u.mgd.assoc_data)
+	if (sdata->u.mgd.assoc_data) {
 		ether_addr_copy(link->conf->addr,
 				sdata->u.mgd.assoc_data->link[link_id].addr);
-	else if (!is_valid_ether_addr(link->conf->addr))
-		eth_random_addr(link->conf->addr);
+	} else if (!is_valid_ether_addr(link->conf->addr)) {
+		ret = drv_generate_link_addr(sdata->local, sdata,
+					     link_id, link->conf->addr);
+		if (ret)
+			eth_random_addr(link->conf->addr);
+	}
 }
 
 /* scan finished notification */
@@ -7448,11 +7453,18 @@  int ieee80211_mgd_assoc(struct ieee80211_sub_if_data *sdata,
 			if (!req->links[i].bss)
 				continue;
 			link = sdata_dereference(sdata->link[i], sdata);
-			if (link)
+			if (link) {
 				ether_addr_copy(assoc_data->link[i].addr,
 						link->conf->addr);
-			else
-				eth_random_addr(assoc_data->link[i].addr);
+			} else {
+				u8 *link_addr = assoc_data->link[i].addr;
+				int ret;
+
+				ret = drv_generate_link_addr(sdata->local, sdata,
+							     i, link_addr);
+				if (ret)
+					eth_random_addr(link_addr);
+			}
 		}
 	} else {
 		memcpy(assoc_data->link[0].addr, sdata->vif.addr, ETH_ALEN);
diff --git a/net/mac80211/trace.h b/net/mac80211/trace.h
index b8c53b4a710b..b140a4e70df9 100644
--- a/net/mac80211/trace.h
+++ b/net/mac80211/trace.h
@@ -727,6 +727,33 @@  TRACE_EVENT(drv_sw_scan_start,
 		  LOCAL_PR_ARG, VIF_PR_ARG, __entry->mac_addr)
 );
 
+TRACE_EVENT(drv_generate_link_addr,
+	TP_PROTO(struct ieee80211_local *local,
+		 struct ieee80211_sub_if_data *sdata,
+		 const u8 *mac_addr,
+		 unsigned int link_id),
+
+	TP_ARGS(local, sdata, mac_addr, link_id),
+
+	TP_STRUCT__entry(
+		LOCAL_ENTRY
+		VIF_ENTRY
+		__array(char, mac_addr, ETH_ALEN)
+		__field(unsigned int, link_id)
+	),
+
+	TP_fast_assign(
+		LOCAL_ASSIGN;
+		VIF_ASSIGN;
+		memcpy(__entry->mac_addr, mac_addr, ETH_ALEN);
+		__entry->link_id = link_id;
+	),
+
+	TP_printk(LOCAL_PR_FMT ", " VIF_PR_FMT ", link addr: %pM link id: %#x",
+		  LOCAL_PR_ARG, VIF_PR_ARG, __entry->mac_addr,
+		  __entry->link_id)
+);
+
 DEFINE_EVENT(local_sdata_evt, drv_sw_scan_complete,
 	TP_PROTO(struct ieee80211_local *local,
 		 struct ieee80211_sub_if_data *sdata),