[RFC PATCH 0/4] cfg80211: updates and fixes for TID specific configuration

[RFC PATCH 0/4] cfg80211: updates and fixes for TID specific configuration

[Sergey Matyukevich]

Hello Johannes, Tamizh, and all

This patch series contains several minor fixes and enhancements for TID
specific configuration functionality. The first three patches include
minor fixes and TID specific AMSDU configuration.

The last patch is somewhat controversial, so the series is marked as RFC.
The last patch simplifies current override logic. It suggests to make no
difference between 'specific peer' and 'all peers' cases and to apply
new TID configuration immediately after resetting the previous one.

Regards,
Sergey

Sergey Matyukevich (4):
  cfg80211: fix mask type in cfg80211_tid_cfg structure
  mac80211: fix variable names in TID config methods
  cfg80211: add support for TID specific AMSDU configuration
  nl80211: simplify peer specific TID configuration

 include/net/cfg80211.h       |  6 ++++--
 include/uapi/linux/nl80211.h | 20 +++++++++++---------
 net/mac80211/cfg.c           |  6 +++---
 net/mac80211/driver-ops.h    |  4 ++--
 net/wireless/nl80211.c       | 11 +++++++----
 5 files changed, 27 insertions(+), 20 deletions(-)

-- 
2.11.0

[RFC PATCH 1/4] cfg80211: fix mask type in cfg80211_tid_cfg structure

[Sergey Matyukevich]

TIDs mask type is u64 in wiphy settings and nl80211 processing, see:
- wiphy TIDs mask sizes in tid_config_support structure
- prepare driver command in parse_tid_conf

Use the same type for TIDs mask in cfg80211_tid_cfg.

Signed-off-by: Sergey Matyukevich <sergey.matyukevich.os@quantenna.com>
---
 include/net/cfg80211.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/net/cfg80211.h b/include/net/cfg80211.h
index 70e48f66dac8..6ff2646ed5fa 100644
--- a/include/net/cfg80211.h
+++ b/include/net/cfg80211.h
@@ -642,7 +642,7 @@ struct cfg80211_chan_def {
 struct cfg80211_tid_cfg {
 	bool config_override;
 	u8 tids;
-	u32 mask;
+	u64 mask;
 	enum nl80211_tid_config noack;
 	u8 retry_long, retry_short;
 	enum nl80211_tid_config ampdu;
-- 
2.11.0

[RFC PATCH 2/4] mac80211: fix variable names in TID config methods

[Sergey Matyukevich]

Fix all variable names from 'tid' to 'tids' to avoid confusion.
Now this is not TID number, but TID mask.

Signed-off-by: Sergey Matyukevich <sergey.matyukevich.os@quantenna.com>
---
 net/mac80211/cfg.c        | 6 +++---
 net/mac80211/driver-ops.h | 4 ++--
 2 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/net/mac80211/cfg.c b/net/mac80211/cfg.c
index 0f72813fed53..e8ec658a3c7b 100644
--- a/net/mac80211/cfg.c
+++ b/net/mac80211/cfg.c
@@ -3925,7 +3925,7 @@ static int ieee80211_set_tid_config(struct wiphy *wiphy,
 
 static int ieee80211_reset_tid_config(struct wiphy *wiphy,
 				      struct net_device *dev,
-				      const u8 *peer, u8 tid)
+				      const u8 *peer, u8 tids)
 {
 	struct ieee80211_sub_if_data *sdata = IEEE80211_DEV_TO_SUB_IF(dev);
 	struct sta_info *sta;
@@ -3935,7 +3935,7 @@ static int ieee80211_reset_tid_config(struct wiphy *wiphy,
 		return -EOPNOTSUPP;
 
 	if (!peer)
-		return drv_reset_tid_config(sdata->local, sdata, NULL, tid);
+		return drv_reset_tid_config(sdata->local, sdata, NULL, tids);
 
 	mutex_lock(&sdata->local->sta_mtx);
 	sta = sta_info_get_bss(sdata, peer);
@@ -3944,7 +3944,7 @@ static int ieee80211_reset_tid_config(struct wiphy *wiphy,
 		return -ENOENT;
 	}
 
-	ret = drv_reset_tid_config(sdata->local, sdata, &sta->sta, tid);
+	ret = drv_reset_tid_config(sdata->local, sdata, &sta->sta, tids);
 	mutex_unlock(&sdata->local->sta_mtx);
 
 	return ret;
diff --git a/net/mac80211/driver-ops.h b/net/mac80211/driver-ops.h
index 3877710e3b48..de69fc9c4f07 100644
--- a/net/mac80211/driver-ops.h
+++ b/net/mac80211/driver-ops.h
@@ -1375,12 +1375,12 @@ static inline int drv_set_tid_config(struct ieee80211_local *local,
 
 static inline int drv_reset_tid_config(struct ieee80211_local *local,
 				       struct ieee80211_sub_if_data *sdata,
-				       struct ieee80211_sta *sta, u8 tid)
+				       struct ieee80211_sta *sta, u8 tids)
 {
 	int ret;
 
 	might_sleep();
-	ret = local->ops->reset_tid_config(&local->hw, &sdata->vif, sta, tid);
+	ret = local->ops->reset_tid_config(&local->hw, &sdata->vif, sta, tids);
 	trace_drv_return_int(local, ret);
 
 	return ret;
-- 
2.11.0

[RFC PATCH 3/4] cfg80211: add support for TID specific AMSDU configuration

[Sergey Matyukevich]

This patch adds support to control per TID MSDU aggregation
using the NL80211_TID_CONFIG_ATTR_AMSDU_CTRL attribute.

Signed-off-by: Sergey Matyukevich <sergey.matyukevich.os@quantenna.com>
---
 include/net/cfg80211.h       |  4 +++-
 include/uapi/linux/nl80211.h | 10 +++++++---
 net/wireless/nl80211.c       |  6 ++++++
 3 files changed, 16 insertions(+), 4 deletions(-)

diff --git a/include/net/cfg80211.h b/include/net/cfg80211.h
index 6ff2646ed5fa..6dfae972730c 100644
--- a/include/net/cfg80211.h
+++ b/include/net/cfg80211.h
@@ -636,8 +636,9 @@ struct cfg80211_chan_def {
  * @noack: noack configuration value for the TID
  * @retry_long: retry count value
  * @retry_short: retry count value
- * @ampdu: Enable/Disable aggregation
+ * @ampdu: Enable/Disable MPDU aggregation
  * @rtscts: Enable/Disable RTS/CTS
+ * @amsdu: Enable/Disable MSDU aggregation
  */
 struct cfg80211_tid_cfg {
 	bool config_override;
@@ -647,6 +648,7 @@ struct cfg80211_tid_cfg {
 	u8 retry_long, retry_short;
 	enum nl80211_tid_config ampdu;
 	enum nl80211_tid_config rtscts;
+	enum nl80211_tid_config amsdu;
 };
 
 /**
diff --git a/include/uapi/linux/nl80211.h b/include/uapi/linux/nl80211.h
index 2b691161830f..1bf34a39c776 100644
--- a/include/uapi/linux/nl80211.h
+++ b/include/uapi/linux/nl80211.h
@@ -4828,12 +4828,15 @@ enum nl80211_tid_config {
  *	&NL80211_CMD_SET_TID_CONFIG. Its type is u8, min value is 1 and
  *	the max value is advertised by the driver in this attribute on
  *	output in wiphy capabilities.
- * @NL80211_TID_CONFIG_ATTR_AMPDU_CTRL: Enable/Disable aggregation for the TIDs
- *	specified in %NL80211_TID_CONFIG_ATTR_TIDS. Its type is u8, using
- *	the values from &nl80211_tid_config.
+ * @NL80211_TID_CONFIG_ATTR_AMPDU_CTRL: Enable/Disable MPDU aggregation
+ *	for the TIDs specified in %NL80211_TID_CONFIG_ATTR_TIDS.
+ *	Its type is u8, using the values from &nl80211_tid_config.
  * @NL80211_TID_CONFIG_ATTR_RTSCTS_CTRL: Enable/Disable RTS_CTS for the TIDs
  *	specified in %NL80211_TID_CONFIG_ATTR_TIDS. It is u8 type, using
  *	the values from &nl80211_tid_config.
+ * @NL80211_TID_CONFIG_ATTR_AMSDU_CTRL: Enable/Disable MSDU aggregation
+ *	for the TIDs specified in %NL80211_TID_CONFIG_ATTR_TIDS.
+ *	Its type is u8, using the values from &nl80211_tid_config.
  */
 enum nl80211_tid_config_attr {
 	__NL80211_TID_CONFIG_ATTR_INVALID,
@@ -4847,6 +4850,7 @@ enum nl80211_tid_config_attr {
 	NL80211_TID_CONFIG_ATTR_RETRY_LONG,
 	NL80211_TID_CONFIG_ATTR_AMPDU_CTRL,
 	NL80211_TID_CONFIG_ATTR_RTSCTS_CTRL,
+	NL80211_TID_CONFIG_ATTR_AMSDU_CTRL,
 
 	/* keep last */
 	__NL80211_TID_CONFIG_ATTR_AFTER_LAST,
diff --git a/net/wireless/nl80211.c b/net/wireless/nl80211.c
index 692bcd35f809..ea0e588b6f6a 100644
--- a/net/wireless/nl80211.c
+++ b/net/wireless/nl80211.c
@@ -14104,6 +14104,12 @@ static int parse_tid_conf(struct cfg80211_registered_device *rdev,
 			nla_get_u8(attrs[NL80211_TID_CONFIG_ATTR_RTSCTS_CTRL]);
 	}
 
+	if (attrs[NL80211_TID_CONFIG_ATTR_AMSDU_CTRL]) {
+		tid_conf->mask |= BIT(NL80211_TID_CONFIG_ATTR_AMSDU_CTRL);
+		tid_conf->amsdu =
+			nla_get_u8(attrs[NL80211_TID_CONFIG_ATTR_AMSDU_CTRL]);
+	}
+
 	if (peer)
 		mask = rdev->wiphy.tid_config_support.peer;
 	else
-- 
2.11.0

[RFC PATCH 4/4] nl80211: simplify peer specific TID configuration

[Sergey Matyukevich]

Current rule for applying TID configuration for specific peer looks overly
complicated. No need to reject new TID configuration when override flag is
specified. Another call with the same TID configuration, but without
override flag, allows to apply new configuration anyway.

Use the same approach as for the 'all peers' case: if override flag is
specified, then reset existing TID configuration and immediately
apply a new one.

Signed-off-by: Sergey Matyukevich <sergey.matyukevich.os@quantenna.com>
---
 include/uapi/linux/nl80211.h | 10 ++++------
 net/wireless/nl80211.c       |  5 +----
 2 files changed, 5 insertions(+), 10 deletions(-)

diff --git a/include/uapi/linux/nl80211.h b/include/uapi/linux/nl80211.h
index 1bf34a39c776..003e25ce3899 100644
--- a/include/uapi/linux/nl80211.h
+++ b/include/uapi/linux/nl80211.h
@@ -4807,12 +4807,10 @@ enum nl80211_tid_config {
  *	(%NL80211_TID_CONFIG_ATTR_TIDS, %NL80211_TID_CONFIG_ATTR_OVERRIDE).
  * @NL80211_TID_CONFIG_ATTR_PEER_SUPP: same as the previous per-vif one, but
  *	per peer instead.
- * @NL80211_TID_CONFIG_ATTR_OVERRIDE: flag attribue, if no peer
- *	is selected, if set indicates that the new configuration overrides
- *	all previous peer configurations, otherwise previous peer specific
- *	configurations should be left untouched. If peer is selected then
- *	it will reset particular TID configuration of that peer and it will
- *	not accept other TID config attributes along with peer.
+ * @NL80211_TID_CONFIG_ATTR_OVERRIDE: flag attribue, if set indicates
+ *	that the new configuration overrides all previous peer
+ *	configurations, otherwise previous peer specific configurations
+ *	should be left untouched.
  * @NL80211_TID_CONFIG_ATTR_TIDS: a bitmask value of TIDs (bit 0 to 7)
  *	Its type is u16.
  * @NL80211_TID_CONFIG_ATTR_NOACK: Configure ack policy for the TID.
diff --git a/net/wireless/nl80211.c b/net/wireless/nl80211.c
index ea0e588b6f6a..03f7870875a0 100644
--- a/net/wireless/nl80211.c
+++ b/net/wireless/nl80211.c
@@ -14058,10 +14058,7 @@ static int parse_tid_conf(struct cfg80211_registered_device *rdev,
 		if (rdev->ops->reset_tid_config) {
 			err = rdev_reset_tid_config(rdev, dev, peer,
 						    tid_conf->tids);
-			/* If peer is there no other configuration will be
-			 * allowed
-			 */
-			if (err || peer)
+			if (err)
 				return err;
 		} else {
 			return -EINVAL;
-- 
2.11.0

Re: [RFC PATCH 3/4] cfg80211: add support for TID specific AMSDU configuration

[Sergey Matyukevich]

> This patch adds support to control per TID MSDU aggregation
> using the NL80211_TID_CONFIG_ATTR_AMSDU_CTRL attribute.
> 
> Signed-off-by: Sergey Matyukevich <sergey.matyukevich.os@quantenna.com>
> ---
>  include/net/cfg80211.h       |  4 +++-
>  include/uapi/linux/nl80211.h | 10 +++++++---
>  net/wireless/nl80211.c       |  6 ++++++
>  3 files changed, 16 insertions(+), 4 deletions(-)

This patch misses adding new policy for AMSDU NL80211 attribute.
To be fixed in v2.

Regards,
Sergey

Re: [RFC PATCH 0/4] cfg80211: updates and fixes for TID specific configuration

[Johannes Berg]

Hi,

> This patch series contains several minor fixes and enhancements for TID
> specific configuration functionality. The first three patches include
> minor fixes and TID specific AMSDU configuration.

Looks good, though I just saw you noticed a small bug :)

> The last patch is somewhat controversial, so the series is marked as RFC.
> The last patch simplifies current override logic. It suggests to make no
> difference between 'specific peer' and 'all peers' cases and to apply
> new TID configuration immediately after resetting the previous one.

Yeah, I tend to agree.

Can you resend with the small fix?

johannes

Re: [RFC PATCH 0/4] cfg80211: updates and fixes for TID specific configuration

[Sergey Matyukevich]

> > This patch series contains several minor fixes and enhancements for TID
> > specific configuration functionality. The first three patches include
> > minor fixes and TID specific AMSDU configuration.
> 
> Looks good, though I just saw you noticed a small bug :)
> 
> > The last patch is somewhat controversial, so the series is marked as RFC.
> > The last patch simplifies current override logic. It suggests to make no
> > difference between 'specific peer' and 'all peers' cases and to apply
> > new TID configuration immediately after resetting the previous one.
> 
> Yeah, I tend to agree.
> 
> Can you resend with the small fix?

Sure, will do. Is it ok if I add a patch supporting this feature in iw tool ?
Or you would prefer to review it separately after this series lands in
mac80211-next ?

Regards,
Sergey

Re: [RFC PATCH 0/4] cfg80211: updates and fixes for TID specific configuration

[Johannes Berg]

On Fri, 2020-04-24 at 13:26 +0300, Sergey Matyukevich wrote:
> > > This patch series contains several minor fixes and enhancements for TID
> > > specific configuration functionality. The first three patches include
> > > minor fixes and TID specific AMSDU configuration.
> > 
> > Looks good, though I just saw you noticed a small bug :)
> > 
> > > The last patch is somewhat controversial, so the series is marked as RFC.
> > > The last patch simplifies current override logic. It suggests to make no
> > > difference between 'specific peer' and 'all peers' cases and to apply
> > > new TID configuration immediately after resetting the previous one.
> > 
> > Yeah, I tend to agree.
> > 
> > Can you resend with the small fix?
> 
> Sure, will do. Is it ok if I add a patch supporting this feature in iw tool ?
> Or you would prefer to review it separately after this series lands in
> mac80211-next ?

That's fine. Not sure how you could possibly send them in the same
series with git, but go ahead :)

FWIW, I will always ignore nl80211.h update patches, I do those myself,
so no need to send that.

johannes