diff mbox series

[1/2] wifi: nl80211: add link id of transmitted profile for MLO MBSSID

Message ID 20240910204538.4077640-2-quic_msinada@quicinc.com (mailing list archive)
State Changes Requested
Delegated to: Johannes Berg
Headers show
Series MLO MBSSID Support | expand

Commit Message

Muna Sinada Sept. 10, 2024, 8:45 p.m. UTC 
From: Rameshkumar Sundaram <quic_ramess@quicinc.com>

Currently non-transmitted profile provides the interface index of the
transmitted profile. The index matches one of the interface indices
advertised by the kernel.

For MLO MBSSID, if the transmitted profile if part of an MLD, then the
transmitted profile is a specific link of that MLD. Utilizing only Tx
index is no longer sufficient to identify transmitted profile for MLO.

Add a new attribute to specify link id of the transmitted profile of
MBSSID group if the profile is part of an MLD. It is required to map
the nontransmitted link with corresponding transmitted link.

Signed-off-by: Rameshkumar Sundaram <quic_ramess@quicinc.com>
Co-developed-by: Aloka Dixit <quic_alokad@quicinc.com>
Signed-off-by: Aloka Dixit <quic_alokad@quicinc.com>
Signed-off-by: Muna Sinada <quic_msinada@quicinc.com>
---
 include/net/cfg80211.h       |  2 ++
 include/uapi/linux/nl80211.h |  5 +++++
 net/wireless/nl80211.c       | 29 ++++++++++++++++++++++++++---
 3 files changed, 33 insertions(+), 3 deletions(-)

Comments

Johannes Berg Sept. 11, 2024, 9:36 a.m. UTC | #1 
On Tue, 2024-09-10 at 13:45 -0700, Muna Sinada wrote:
> 
> For MLO MBSSID, if the transmitted profile if part of an MLD, then the
> transmitted profile is a specific link of that MLD. Utilizing only Tx
> index is no longer sufficient to identify transmitted profile for MLO.

"Tx index"? Did you mean "interface index" or something?

> Add a new attribute to specify link id of the transmitted profile of
> MBSSID group if the profile is part of an MLD. It is required to map
> the nontransmitted link with corresponding transmitted link.

s/with/to/ or something?

>   * @tx_wdev: pointer to the transmitted interface in the MBSSID set
> + * @tx_link_id: link ID of the transmitted interface if it is part of an MLD.

document the value for non-MLO?

>   * @index: index of this AP in the multi bssid group.
>   * @ema: set to true if the beacons should be sent out in EMA mode.
>   */
>  struct cfg80211_mbssid_config {
>  	struct wireless_dev *tx_wdev;
> +	int tx_link_id;
>  	u8 index;
>  	bool ema;
>  };
> diff --git a/include/uapi/linux/nl80211.h b/include/uapi/linux/nl80211.h
> index f97f5adc8d51..6bd46b4998c9 100644
> --- a/include/uapi/linux/nl80211.h
> +++ b/include/uapi/linux/nl80211.h
> @@ -7987,6 +7987,10 @@ enum nl80211_sar_specs_attrs {
>   *	Setting this flag is permitted only if the driver advertises EMA support
>   *	by setting wiphy->ema_max_profile_periodicity to non-zero.
>   *
> + * @NL80211_MBSSID_CONFIG_ATTR_TX_LINK_ID: Mandatory parameter for a non-transmitted
> + *	profile which provides the link ID (u8) of the transmitted profile when the latter
> + *	is part of an MLD.

It's probably better to rewrite this, the qualification of the first
word ("mandatory") comes at the very end of the sentence. German
speakers are probably used to that, but otherwise it seems a bit hard to
understand? ;)

> +	[NL80211_MBSSID_CONFIG_ATTR_TX_LINK_ID] =
> +				NLA_POLICY_MAX(NLA_U8, IEEE80211_MLD_MAX_NUM_LINKS),

why not just indent one tab?

> @@ -5477,6 +5479,8 @@ static int nl80211_parse_mbssid_config(struct wiphy *wiphy,
>  				       u8 num_elems)
>  {
>  	struct nlattr *tb[NL80211_MBSSID_CONFIG_ATTR_MAX + 1];
> +	struct net_device *tx_netdev = NULL;
> +	int err = -EINVAL;

I don't much like this pattern of initializing the error first, can't
you initialize it in every place? I even wondered looking at the below
if it was correct everywhere.

>  	if (!wiphy->mbssid_max_interfaces)
>  		return -EOPNOTSUPP;
> @@ -5509,9 +5513,7 @@ static int nl80211_parse_mbssid_config(struct wiphy *wiphy,
>  			return -EINVAL;
>  
>  		if (tx_ifindex != dev->ifindex) {
> -			struct net_device *tx_netdev =
> -				dev_get_by_index(wiphy_net(wiphy), tx_ifindex);
> -
> +			tx_netdev = dev_get_by_index(wiphy_net(wiphy), tx_ifindex);
>  			if (!tx_netdev || !tx_netdev->ieee80211_ptr ||
>  			    tx_netdev->ieee80211_ptr->wiphy != wiphy ||
>  			    tx_netdev->ieee80211_ptr->iftype !=
> @@ -5530,7 +5532,28 @@ static int nl80211_parse_mbssid_config(struct wiphy *wiphy,
>  		return -EINVAL;
>  	}
>  
> +	config->tx_link_id = 0;
> +	if (config->tx_wdev->valid_links) {
> +		if (!tb[NL80211_MBSSID_CONFIG_ATTR_TX_LINK_ID])
> +			goto err;
> +
> +		config->tx_link_id = nla_get_u8(tb[NL80211_MBSSID_CONFIG_ATTR_TX_LINK_ID]);
> +		if (!(config->tx_wdev->valid_links & BIT(config->tx_link_id))) {
> +			err = -ENOLINK;
> +			goto err;
> +		}
> +	} else if (tb[NL80211_MBSSID_CONFIG_ATTR_TX_LINK_ID]) {
> +		goto err;
> +	}
> +
>  	return 0;
> +
> +err:
> +	if (tx_netdev) {
> +		config->tx_wdev = NULL;
> +		dev_put(tx_netdev);
> +	}

Why not use config->tx_wdev and avoid changes around tx_netdev?

There's also an existing error path that does dev_put(), so you should
unify that.

johannes
Muna Sinada Oct. 16, 2024, 7:50 p.m. UTC | #2 
On 9/11/2024 2:36 AM, Johannes Berg wrote:
> On Tue, 2024-09-10 at 13:45 -0700, Muna Sinada wrote:
>>  	if (!wiphy->mbssid_max_interfaces)
>>  		return -EOPNOTSUPP;
>> @@ -5509,9 +5513,7 @@ static int nl80211_parse_mbssid_config(struct wiphy *wiphy,
>>  			return -EINVAL;
>>  
>>  		if (tx_ifindex != dev->ifindex) {
>> -			struct net_device *tx_netdev =
>> -				dev_get_by_index(wiphy_net(wiphy), tx_ifindex);
>> -
>> +			tx_netdev = dev_get_by_index(wiphy_net(wiphy), tx_ifindex);
>>  			if (!tx_netdev || !tx_netdev->ieee80211_ptr ||
>>  			    tx_netdev->ieee80211_ptr->wiphy != wiphy ||
>>  			    tx_netdev->ieee80211_ptr->iftype !=
>> @@ -5530,7 +5532,28 @@ static int nl80211_parse_mbssid_config(struct wiphy *wiphy,
>>  		return -EINVAL;
>>  	}
>>  
>> +	config->tx_link_id = 0;
>> +	if (config->tx_wdev->valid_links) {
>> +		if (!tb[NL80211_MBSSID_CONFIG_ATTR_TX_LINK_ID])
>> +			goto err;
>> +
>> +		config->tx_link_id = nla_get_u8(tb[NL80211_MBSSID_CONFIG_ATTR_TX_LINK_ID]);
>> +		if (!(config->tx_wdev->valid_links & BIT(config->tx_link_id))) {
>> +			err = -ENOLINK;
>> +			goto err;
>> +		}
>> +	} else if (tb[NL80211_MBSSID_CONFIG_ATTR_TX_LINK_ID]) {
>> +		goto err;
>> +	}
>> +
>>  	return 0;
>> +
>> +err:
>> +	if (tx_netdev) {
>> +		config->tx_wdev = NULL;
>> +		dev_put(tx_netdev);
>> +	}
> 
> Why not use config->tx_wdev and avoid changes around tx_netdev?

tx_netdev instance is being utilized on exisiting code to perform checks and grab its ieee80211_ptr. W are not making changes to tx_netdev itself. In this specific patch we are making changes to config->tx_wdev. I was wondering if I could get clarification on this. 

> 
> There's also an existing error path that does dev_put(), so you should
> unify that.
> 
> johannes

Thank you,
Muna
Muna Sinada Oct. 16, 2024, 8:49 p.m. UTC | #3 
On 10/16/2024 12:50 PM, Muna Sinada wrote:
> On 9/11/2024 2:36 AM, Johannes Berg wrote:
>> On Tue, 2024-09-10 at 13:45 -0700, Muna Sinada wrote:
>>>  	if (!wiphy->mbssid_max_interfaces)
>>>  		return -EOPNOTSUPP;
>>> @@ -5509,9 +5513,7 @@ static int nl80211_parse_mbssid_config(struct wiphy *wiphy,
>>>  			return -EINVAL;
>>>  
>>>  		if (tx_ifindex != dev->ifindex) {
>>> -			struct net_device *tx_netdev =
>>> -				dev_get_by_index(wiphy_net(wiphy), tx_ifindex);
>>> -
>>> +			tx_netdev = dev_get_by_index(wiphy_net(wiphy), tx_ifindex);
>>>  			if (!tx_netdev || !tx_netdev->ieee80211_ptr ||
>>>  			    tx_netdev->ieee80211_ptr->wiphy != wiphy ||
>>>  			    tx_netdev->ieee80211_ptr->iftype !=
>>> @@ -5530,7 +5532,28 @@ static int nl80211_parse_mbssid_config(struct wiphy *wiphy,
>>>  		return -EINVAL;
>>>  	}
>>>  
>>> +	config->tx_link_id = 0;
>>> +	if (config->tx_wdev->valid_links) {
>>> +		if (!tb[NL80211_MBSSID_CONFIG_ATTR_TX_LINK_ID])
>>> +			goto err;
>>> +
>>> +		config->tx_link_id = nla_get_u8(tb[NL80211_MBSSID_CONFIG_ATTR_TX_LINK_ID]);
>>> +		if (!(config->tx_wdev->valid_links & BIT(config->tx_link_id))) {
>>> +			err = -ENOLINK;
>>> +			goto err;
>>> +		}
>>> +	} else if (tb[NL80211_MBSSID_CONFIG_ATTR_TX_LINK_ID]) {
>>> +		goto err;
>>> +	}
>>> +
>>>  	return 0;
>>> +
>>> +err:
>>> +	if (tx_netdev) {
>>> +		config->tx_wdev = NULL;
>>> +		dev_put(tx_netdev);
>>> +	}
>>
>> Why not use config->tx_wdev and avoid changes around tx_netdev?
> 
> tx_netdev instance is being utilized on exisiting code to perform checks and grab its 
>ieee80211_ptr. W are not making changes to tx_netdev itself. In this specific patch we 
> are making changes to config->tx_wdev. I was wondering if I could get clarification on 
>this. 
> 
>>
>> There's also an existing error path that does dev_put(), so you should
>> unify that.
>>
>> johannes
> 
> Thank you,
> Muna

I apologize for the lack of link breaks in my response. Will make sure to include them
in the future.
Johannes Berg Oct. 22, 2024, 2:22 p.m. UTC | #4 
On Wed, 2024-10-16 at 12:50 -0700, Muna Sinada wrote:
> > 
> > Why not use config->tx_wdev and avoid changes around tx_netdev?
> 
> tx_netdev instance is being utilized on exisiting code to perform checks and grab its ieee80211_ptr. W are not making changes to tx_netdev itself. In this specific patch we are making changes to config->tx_wdev. I was wondering if I could get clarification on this. 

I might've misinterpreted the code, but also you're pulling out the
tx_netdev variable and really don't need to, just like the end of
nl80211_start_ap() you can release it via the config->tx_wdev pointer?

johannes
diff mbox series

Patch

diff --git a/include/net/cfg80211.h b/include/net/cfg80211.h
index 192d72c8b465..ccdfcb33ba1e 100644
--- a/include/net/cfg80211.h
+++ b/include/net/cfg80211.h
@@ -1286,11 +1286,13 @@  struct cfg80211_crypto_settings {
  * struct cfg80211_mbssid_config - AP settings for multi bssid
  *
  * @tx_wdev: pointer to the transmitted interface in the MBSSID set
+ * @tx_link_id: link ID of the transmitted interface if it is part of an MLD.
  * @index: index of this AP in the multi bssid group.
  * @ema: set to true if the beacons should be sent out in EMA mode.
  */
 struct cfg80211_mbssid_config {
 	struct wireless_dev *tx_wdev;
+	int tx_link_id;
 	u8 index;
 	bool ema;
 };
diff --git a/include/uapi/linux/nl80211.h b/include/uapi/linux/nl80211.h
index f97f5adc8d51..6bd46b4998c9 100644
--- a/include/uapi/linux/nl80211.h
+++ b/include/uapi/linux/nl80211.h
@@ -7987,6 +7987,10 @@  enum nl80211_sar_specs_attrs {
  *	Setting this flag is permitted only if the driver advertises EMA support
  *	by setting wiphy->ema_max_profile_periodicity to non-zero.
  *
+ * @NL80211_MBSSID_CONFIG_ATTR_TX_LINK_ID: Mandatory parameter for a non-transmitted
+ *	profile which provides the link ID (u8) of the transmitted profile when the latter
+ *	is part of an MLD.
+ *
  * @__NL80211_MBSSID_CONFIG_ATTR_LAST: Internal
  * @NL80211_MBSSID_CONFIG_ATTR_MAX: highest attribute
  */
@@ -7998,6 +8002,7 @@  enum nl80211_mbssid_config_attributes {
 	NL80211_MBSSID_CONFIG_ATTR_INDEX,
 	NL80211_MBSSID_CONFIG_ATTR_TX_IFINDEX,
 	NL80211_MBSSID_CONFIG_ATTR_EMA,
+	NL80211_MBSSID_CONFIG_ATTR_TX_LINK_ID,
 
 	/* keep last */
 	__NL80211_MBSSID_CONFIG_ATTR_LAST,
diff --git a/net/wireless/nl80211.c b/net/wireless/nl80211.c
index 7397a372c78e..bd168f3d5950 100644
--- a/net/wireless/nl80211.c
+++ b/net/wireless/nl80211.c
@@ -454,6 +454,8 @@  nl80211_mbssid_config_policy[NL80211_MBSSID_CONFIG_ATTR_MAX + 1] = {
 	[NL80211_MBSSID_CONFIG_ATTR_INDEX] = { .type = NLA_U8 },
 	[NL80211_MBSSID_CONFIG_ATTR_TX_IFINDEX] = { .type = NLA_U32 },
 	[NL80211_MBSSID_CONFIG_ATTR_EMA] = { .type = NLA_FLAG },
+	[NL80211_MBSSID_CONFIG_ATTR_TX_LINK_ID] =
+				NLA_POLICY_MAX(NLA_U8, IEEE80211_MLD_MAX_NUM_LINKS),
 };
 
 static const struct nla_policy
@@ -5477,6 +5479,8 @@  static int nl80211_parse_mbssid_config(struct wiphy *wiphy,
 				       u8 num_elems)
 {
 	struct nlattr *tb[NL80211_MBSSID_CONFIG_ATTR_MAX + 1];
+	struct net_device *tx_netdev = NULL;
+	int err = -EINVAL;
 
 	if (!wiphy->mbssid_max_interfaces)
 		return -EOPNOTSUPP;
@@ -5509,9 +5513,7 @@  static int nl80211_parse_mbssid_config(struct wiphy *wiphy,
 			return -EINVAL;
 
 		if (tx_ifindex != dev->ifindex) {
-			struct net_device *tx_netdev =
-				dev_get_by_index(wiphy_net(wiphy), tx_ifindex);
-
+			tx_netdev = dev_get_by_index(wiphy_net(wiphy), tx_ifindex);
 			if (!tx_netdev || !tx_netdev->ieee80211_ptr ||
 			    tx_netdev->ieee80211_ptr->wiphy != wiphy ||
 			    tx_netdev->ieee80211_ptr->iftype !=
@@ -5530,7 +5532,28 @@  static int nl80211_parse_mbssid_config(struct wiphy *wiphy,
 		return -EINVAL;
 	}
 
+	config->tx_link_id = 0;
+	if (config->tx_wdev->valid_links) {
+		if (!tb[NL80211_MBSSID_CONFIG_ATTR_TX_LINK_ID])
+			goto err;
+
+		config->tx_link_id = nla_get_u8(tb[NL80211_MBSSID_CONFIG_ATTR_TX_LINK_ID]);
+		if (!(config->tx_wdev->valid_links & BIT(config->tx_link_id))) {
+			err = -ENOLINK;
+			goto err;
+		}
+	} else if (tb[NL80211_MBSSID_CONFIG_ATTR_TX_LINK_ID]) {
+		goto err;
+	}
+
 	return 0;
+
+err:
+	if (tx_netdev) {
+		config->tx_wdev = NULL;
+		dev_put(tx_netdev);
+	}
+	return err;
 }
 
 static struct cfg80211_mbssid_elems *