[RFC 0/3] cfg80211/nl80211/iw: add basic AMPDU/AMSDU controls

[RFC 0/3] cfg80211/nl80211/iw: add basic AMPDU/AMSDU controls

[Sergey Matyukevich]

Hello Johannes and all, 

Here are several RFC patches providing simple high-level controls of AMSDU/AMPDU
aggregation. The primary purpose of this functionality is an attempt to fill
missing gaps in nl80211 interface for basic WFA certification tests.

We experimented with QCA sigma-dut tool: https://github.com/qca/sigma-dut.
The purpose was to cover basic HT/VHT WFA STA tests for cfg80211 driver
controlled by wpa_supplicant  w/o adding any vendor specific commands.
Multiple WFA test parameters (e.g. STBC, NSS, SGI, LDPC) can be configured
by overriding HT/VHT capabilities in wpa_supplicant and applying them on
connect in cfg80211_connect callback. Others (e.g. RTS params) can be
configured using iw tool or NL80211_CMD_SET_WIPHY directly. These patches
implement simpe high-level switches for AMSDU/AMPDU aggregation.

It would be interesting to collect comments/concerns regarding this approach.
Does it make sense to enhance nl80211 in order to cover all the missing pieces
required for WFA certification tests ? Or maybe it makes sense to use
NL80211_TESTMODE subcommands for this kind of testing.


The summary of changes is as follows:
- nl80211/cfg80211: new wiphy flags and minimal set_wiphy/get_wiphy changes
- iw: new phy subcommands to enable/disable aggregation
- qtnfmac: minimal driver example - get/set aggregation

Regards,
Sergey


kernel:
 drivers/net/wireless/quantenna/qtnfmac/cfg80211.c |    2 +
 drivers/net/wireless/quantenna/qtnfmac/commands.c |   17 ++++++++++++
 drivers/net/wireless/quantenna/qtnfmac/core.h     |    2 +
 drivers/net/wireless/quantenna/qtnfmac/qlink.h    |    7 ++++
 include/net/cfg80211.h                            |    7 ++++
 include/uapi/linux/nl80211.h                      |    6 ++++
 net/wireless/core.c                               |    3 ++
 net/wireless/nl80211.c                            |   31 ++++++++++++++++++++++
 9 files changed, 76 insertions(+), 1 deletion(-)

iw:
 nl80211.h |    6 ++++++
 phy.c     |   60 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 66 insertions(+)

[RFC 1/3] cfg80211/nl80211: add wiphy flags to control aggregation

[Sergey Matyukevich]

Add two top-level switches to wiphy structure to control AMSDU and AMPDU
aggregation. Enable read/update of AMSDU and AMPDU aggregation from
the userspace using set_wiphy/get_wiphy commands.

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

diff --git a/include/net/cfg80211.h b/include/net/cfg80211.h
index 1fa41b7a1be3..fbf01d156069 100644
--- a/include/net/cfg80211.h
+++ b/include/net/cfg80211.h
@@ -2368,6 +2368,8 @@ enum cfg80211_connect_params_changed {
  * @WIPHY_PARAM_TXQ_LIMIT: TXQ packet limit has been changed
  * @WIPHY_PARAM_TXQ_MEMORY_LIMIT: TXQ memory limit has been changed
  * @WIPHY_PARAM_TXQ_QUANTUM: TXQ scheduler quantum
+ * @WIPHY_PARAM_AMPDU_ENABLED: wiphy->ampdu_enabled has changed
+ * @WIPHY_PARAM_AMSDU_ENABLED: wiphy->amsdu_enabled has changed
  */
 enum wiphy_params_flags {
 	WIPHY_PARAM_RETRY_SHORT		= 1 << 0,
@@ -2379,6 +2381,8 @@ enum wiphy_params_flags {
 	WIPHY_PARAM_TXQ_LIMIT		= 1 << 6,
 	WIPHY_PARAM_TXQ_MEMORY_LIMIT	= 1 << 7,
 	WIPHY_PARAM_TXQ_QUANTUM		= 1 << 8,
+	WIPHY_PARAM_AMPDU_ENABLED	= 1 << 9,
+	WIPHY_PARAM_AMSDU_ENABLED	= 1 << 10,
 };
 
 /**
@@ -4163,6 +4167,9 @@ struct wiphy {
 	u32 txq_memory_limit;
 	u32 txq_quantum;
 
+	u8 ampdu_enabled;
+	u8 amsdu_enabled;
+
 	char priv[0] __aligned(NETDEV_ALIGN);
 };
 
diff --git a/include/uapi/linux/nl80211.h b/include/uapi/linux/nl80211.h
index 6d610bae30a9..a9f8fa814ecd 100644
--- a/include/uapi/linux/nl80211.h
+++ b/include/uapi/linux/nl80211.h
@@ -2254,6 +2254,9 @@ enum nl80211_commands {
  * @NL80211_ATTR_FTM_RESPONDER_STATS: Nested attribute with FTM responder
  *	statistics, see &enum nl80211_ftm_responder_stats.
  *
+ * @NL80211_ATTR_WIPHY_AMPDU_ENABLED: enable/disable AMPDU aggregation.
+ * @NL80211_ATTR_WIPHY_AMSDU_ENABLED: enable/disable AMSDU aggregation.
+ *
  * @NUM_NL80211_ATTR: total number of nl80211_attrs available
  * @NL80211_ATTR_MAX: highest attribute number currently defined
  * @__NL80211_ATTR_AFTER_LAST: internal use
@@ -2699,6 +2702,9 @@ enum nl80211_attrs {
 
 	NL80211_ATTR_FTM_RESPONDER_STATS,
 
+	NL80211_ATTR_WIPHY_AMPDU_ENABLED,
+	NL80211_ATTR_WIPHY_AMSDU_ENABLED,
+
 	/* add attributes here, update the policy in nl80211.c */
 
 	__NL80211_ATTR_AFTER_LAST,
diff --git a/net/wireless/core.c b/net/wireless/core.c
index 5bd01058b9e6..182f8f04166d 100644
--- a/net/wireless/core.c
+++ b/net/wireless/core.c
@@ -524,6 +524,9 @@ struct wiphy *wiphy_new_nm(const struct cfg80211_ops *ops, int sizeof_priv,
 	rdev->wiphy.max_sched_scan_plans = 1;
 	rdev->wiphy.max_sched_scan_plan_interval = U32_MAX;
 
+	rdev->wiphy.ampdu_enabled = 1;
+	rdev->wiphy.amsdu_enabled = 1;
+
 	return &rdev->wiphy;
 }
 EXPORT_SYMBOL(wiphy_new_nm);
diff --git a/net/wireless/nl80211.c b/net/wireless/nl80211.c
index 744b5851bbf9..5c04b6996e64 100644
--- a/net/wireless/nl80211.c
+++ b/net/wireless/nl80211.c
@@ -497,6 +497,9 @@ static const struct nla_policy nl80211_policy[NUM_NL80211_ATTR] = {
 		.type = NLA_NESTED,
 		.validation_data = nl80211_ftm_responder_policy,
 	},
+
+	[NL80211_ATTR_WIPHY_AMPDU_ENABLED] = { .type = NLA_U8 },
+	[NL80211_ATTR_WIPHY_AMSDU_ENABLED] = { .type = NLA_U8 },
 };
 
 /* policy for the key attributes */
@@ -2118,6 +2121,14 @@ static int nl80211_send_wiphy(struct cfg80211_registered_device *rdev,
 				goto nla_put_failure;
 		}
 
+		if (nla_put_u8(msg, NL80211_ATTR_WIPHY_AMPDU_ENABLED,
+			       rdev->wiphy.ampdu_enabled))
+			goto nla_put_failure;
+
+		if (nla_put_u8(msg, NL80211_ATTR_WIPHY_AMSDU_ENABLED,
+			       rdev->wiphy.amsdu_enabled))
+			goto nla_put_failure;
+
 		/* done */
 		state->split_start = 0;
 		break;
@@ -2514,6 +2525,7 @@ static int nl80211_set_wiphy(struct sk_buff *skb, struct genl_info *info)
 	u32 frag_threshold = 0, rts_threshold = 0;
 	u8 coverage_class = 0;
 	u32 txq_limit = 0, txq_memory_limit = 0, txq_quantum = 0;
+	u8 amsdu = 0, ampdu = 0;
 
 	ASSERT_RTNL();
 
@@ -2743,11 +2755,22 @@ static int nl80211_set_wiphy(struct sk_buff *skb, struct genl_info *info)
 		changed |= WIPHY_PARAM_TXQ_QUANTUM;
 	}
 
+	if (info->attrs[NL80211_ATTR_WIPHY_AMPDU_ENABLED]) {
+		ampdu = nla_get_u8(info->attrs[NL80211_ATTR_WIPHY_AMPDU_ENABLED]);
+		changed |= WIPHY_PARAM_AMPDU_ENABLED;
+	}
+
+	if (info->attrs[NL80211_ATTR_WIPHY_AMSDU_ENABLED]) {
+		amsdu = nla_get_u8(info->attrs[NL80211_ATTR_WIPHY_AMSDU_ENABLED]);
+		changed |= WIPHY_PARAM_AMSDU_ENABLED;
+	}
+
 	if (changed) {
 		u8 old_retry_short, old_retry_long;
 		u32 old_frag_threshold, old_rts_threshold;
 		u8 old_coverage_class;
 		u32 old_txq_limit, old_txq_memory_limit, old_txq_quantum;
+		u8 old_amsdu, old_ampdu;
 
 		if (!rdev->ops->set_wiphy_params)
 			return -EOPNOTSUPP;
@@ -2760,6 +2783,8 @@ static int nl80211_set_wiphy(struct sk_buff *skb, struct genl_info *info)
 		old_txq_limit = rdev->wiphy.txq_limit;
 		old_txq_memory_limit = rdev->wiphy.txq_memory_limit;
 		old_txq_quantum = rdev->wiphy.txq_quantum;
+		old_ampdu = rdev->wiphy.ampdu_enabled;
+		old_amsdu = rdev->wiphy.amsdu_enabled;
 
 		if (changed & WIPHY_PARAM_RETRY_SHORT)
 			rdev->wiphy.retry_short = retry_short;
@@ -2777,6 +2802,10 @@ static int nl80211_set_wiphy(struct sk_buff *skb, struct genl_info *info)
 			rdev->wiphy.txq_memory_limit = txq_memory_limit;
 		if (changed & WIPHY_PARAM_TXQ_QUANTUM)
 			rdev->wiphy.txq_quantum = txq_quantum;
+		if (changed & WIPHY_PARAM_AMPDU_ENABLED)
+			rdev->wiphy.ampdu_enabled = ampdu;
+		if (changed & WIPHY_PARAM_AMSDU_ENABLED)
+			rdev->wiphy.amsdu_enabled = amsdu;
 
 		result = rdev_set_wiphy_params(rdev, changed);
 		if (result) {
@@ -2788,6 +2817,8 @@ static int nl80211_set_wiphy(struct sk_buff *skb, struct genl_info *info)
 			rdev->wiphy.txq_limit = old_txq_limit;
 			rdev->wiphy.txq_memory_limit = old_txq_memory_limit;
 			rdev->wiphy.txq_quantum = old_txq_quantum;
+			rdev->wiphy.ampdu_enabled = old_ampdu;
+			rdev->wiphy.amsdu_enabled = old_amsdu;
 			return result;
 		}
 	}
-- 
2.11.0

[RFC 2/3] iw: add phy subcommands to configure aggregation

[Sergey Matyukevich]

Add phy subcommands to enable/disable AMPDU/AMSDU aggregation.

Signed-off-by: Sergey Matyukevich <sergey.matyukevich.os@quantenna.com>
---
 nl80211.h |  6 ++++++
 phy.c     | 60 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 66 insertions(+)

diff --git a/nl80211.h b/nl80211.h
index 1766a12..41eec4a 100644
--- a/nl80211.h
+++ b/nl80211.h
@@ -2241,6 +2241,9 @@ enum nl80211_commands {
  *	association request when used with NL80211_CMD_NEW_STATION). Can be set
  *	only if %NL80211_STA_FLAG_WME is set.
  *
+ * @NL80211_ATTR_WIPHY_AMPDU_ENABLED: enable/disable AMPDU aggregation.
+ * @NL80211_ATTR_WIPHY_AMSDU_ENABLED: enable/disable AMSDU aggregation.
+ *
  * @NUM_NL80211_ATTR: total number of nl80211_attrs available
  * @NL80211_ATTR_MAX: highest attribute number currently defined
  * @__NL80211_ATTR_AFTER_LAST: internal use
@@ -2682,6 +2685,9 @@ enum nl80211_attrs {
 
 	NL80211_ATTR_HE_CAPABILITY,
 
+	NL80211_ATTR_WIPHY_AMPDU_ENABLED,
+	NL80211_ATTR_WIPHY_AMSDU_ENABLED,
+
 	/* add attributes here, update the policy in nl80211.c */
 
 	__NL80211_ATTR_AFTER_LAST,
diff --git a/phy.c b/phy.c
index 77df7a7..be949e7 100644
--- a/phy.c
+++ b/phy.c
@@ -843,3 +843,63 @@ static int handle_get_txq(struct nl80211_state *state,
 COMMAND(get, txq, "",
 	NL80211_CMD_GET_WIPHY, 0, CIB_PHY, handle_get_txq,
 	"Get TXQ parameters.");
+
+static int handle_ampdu(struct nl80211_state *state,
+			struct nl_msg *msg,
+			int argc, char **argv,
+			enum id_input id)
+{
+	unsigned int ampdu;
+
+	if (argc != 1)
+		return 1;
+
+	if (strcmp(argv[0], "on") == 0)
+		ampdu = 1;
+	else if (strcmp(argv[0], "off") == 0)
+		ampdu = 0;
+	else {
+		printf("Invalid parameter: %s\n", argv[0]);
+		return 2;
+	}
+
+	NLA_PUT_U8(msg, NL80211_ATTR_WIPHY_AMPDU_ENABLED, ampdu);
+
+	return 0;
+
+ nla_put_failure:
+	return -ENOBUFS;
+}
+COMMAND(set, ampdu, "<on|off>",
+	NL80211_CMD_SET_WIPHY, 0, CIB_PHY, handle_ampdu,
+	"Enable/disable AMPDU aggregation.");
+
+static int handle_amsdu(struct nl80211_state *state,
+			struct nl_msg *msg,
+			int argc, char **argv,
+			enum id_input id)
+{
+	unsigned int amsdu;
+
+	if (argc != 1)
+		return 1;
+
+	if (strcmp(argv[0], "on") == 0)
+		amsdu = 1;
+	else if (strcmp(argv[0], "off") == 0)
+		amsdu = 0;
+	else {
+		printf("Invalid parameter: %s\n", argv[0]);
+		return 2;
+	}
+
+	NLA_PUT_U8(msg, NL80211_ATTR_WIPHY_AMSDU_ENABLED, amsdu);
+
+	return 0;
+
+ nla_put_failure:
+	return -ENOBUFS;
+}
+COMMAND(set, amsdu, "<on|off>",
+	NL80211_CMD_SET_WIPHY, 0, CIB_PHY, handle_amsdu,
+	"Enable/disable AMSDU aggregation.");
-- 
2.11.0

[RFC 3/3] qtnfmac: add support for basic aggregation control

[Sergey Matyukevich]

Add support for basic AMPDU/AMSDU aggregation control:
- report initial aggregation configuration to cfg80211 core
- pass AMPDU/AMSDU aggregation changes to wireless card
  using set_wiphy_params cfg80211 callback

Signed-off-by: Sergey Matyukevich <sergey.matyukevich.os@quantenna.com>
---
 drivers/net/wireless/quantenna/qtnfmac/cfg80211.c |  2 ++
 drivers/net/wireless/quantenna/qtnfmac/commands.c | 17 +++++++++++++++++
 drivers/net/wireless/quantenna/qtnfmac/core.h     |  2 ++
 drivers/net/wireless/quantenna/qtnfmac/qlink.h    |  7 +++++++
 4 files changed, 28 insertions(+)

diff --git a/drivers/net/wireless/quantenna/qtnfmac/cfg80211.c b/drivers/net/wireless/quantenna/qtnfmac/cfg80211.c
index 51b33ec78fac..98bd0a3d29db 100644
--- a/drivers/net/wireless/quantenna/qtnfmac/cfg80211.c
+++ b/drivers/net/wireless/quantenna/qtnfmac/cfg80211.c
@@ -1087,6 +1087,8 @@ int qtnf_wiphy_register(struct qtnf_hw_info *hw_info, struct qtnf_wmac *mac)
 	wiphy->retry_short = macinfo->sretry_limit;
 	wiphy->retry_long = macinfo->lretry_limit;
 	wiphy->coverage_class = macinfo->coverage_class;
+	wiphy->ampdu_enabled = macinfo->ampdu_enabled;
+	wiphy->amsdu_enabled = macinfo->amsdu_enabled;
 
 	wiphy->max_scan_ssids =
 		(hw_info->max_scan_ssids) ? hw_info->max_scan_ssids : 1;
diff --git a/drivers/net/wireless/quantenna/qtnfmac/commands.c b/drivers/net/wireless/quantenna/qtnfmac/commands.c
index bfdc1ad30c13..6ab0a25b917f 100644
--- a/drivers/net/wireless/quantenna/qtnfmac/commands.c
+++ b/drivers/net/wireless/quantenna/qtnfmac/commands.c
@@ -1531,6 +1531,7 @@ static int qtnf_cmd_resp_proc_phy_params(struct qtnf_wmac *mac,
 	struct qlink_tlv_frag_rts_thr *phy_thr;
 	struct qlink_tlv_rlimit *limit;
 	struct qlink_tlv_cclass *class;
+	struct qlink_tlv_aggr *aggr;
 	u16 tlv_type;
 	u16 tlv_value_len;
 	size_t tlv_full_len;
@@ -1571,6 +1572,14 @@ static int qtnf_cmd_resp_proc_phy_params(struct qtnf_wmac *mac,
 			class = (void *)tlv;
 			mac_info->coverage_class = class->cclass;
 			break;
+		case QTN_TLV_ID_AMPDU_ENABLED:
+			aggr = (void *)tlv;
+			mac_info->ampdu_enabled = aggr->aggr;
+			break;
+		case QTN_TLV_ID_AMSDU_ENABLED:
+			aggr = (void *)tlv;
+			mac_info->amsdu_enabled = aggr->aggr;
+			break;
 		default:
 			pr_err("MAC%u: Unknown TLV type: %#x\n", mac->macid,
 			       le16_to_cpu(tlv->type));
@@ -1814,6 +1823,14 @@ int qtnf_cmd_send_update_phy_params(struct qtnf_wmac *mac, u32 changed)
 		qtnf_cmd_skb_put_tlv_u8(cmd_skb, QTN_TLV_ID_COVERAGE_CLASS,
 					wiphy->coverage_class);
 
+	if (changed & WIPHY_PARAM_AMPDU_ENABLED)
+		qtnf_cmd_skb_put_tlv_u8(cmd_skb, QTN_TLV_ID_AMPDU_ENABLED,
+					wiphy->ampdu_enabled);
+
+	if (changed & WIPHY_PARAM_AMSDU_ENABLED)
+		qtnf_cmd_skb_put_tlv_u8(cmd_skb, QTN_TLV_ID_AMSDU_ENABLED,
+					wiphy->amsdu_enabled);
+
 	ret = qtnf_cmd_send(mac->bus, cmd_skb);
 	if (ret)
 		goto out;
diff --git a/drivers/net/wireless/quantenna/qtnfmac/core.h b/drivers/net/wireless/quantenna/qtnfmac/core.h
index 293055049caa..e6752faf0f50 100644
--- a/drivers/net/wireless/quantenna/qtnfmac/core.h
+++ b/drivers/net/wireless/quantenna/qtnfmac/core.h
@@ -94,6 +94,8 @@ struct qtnf_mac_info {
 	u8 lretry_limit;
 	u8 sretry_limit;
 	u8 coverage_class;
+	u8 amsdu_enabled;
+	u8 ampdu_enabled;
 	u8 radar_detect_widths;
 	u32 max_acl_mac_addrs;
 	struct ieee80211_ht_cap ht_cap_mod_mask;
diff --git a/drivers/net/wireless/quantenna/qtnfmac/qlink.h b/drivers/net/wireless/quantenna/qtnfmac/qlink.h
index 8d62addea895..7dede44c75d7 100644
--- a/drivers/net/wireless/quantenna/qtnfmac/qlink.h
+++ b/drivers/net/wireless/quantenna/qtnfmac/qlink.h
@@ -1154,6 +1154,8 @@ enum qlink_tlv_id {
 	QTN_TLV_ID_WOWLAN_PATTERN	= 0x0411,
 	QTN_TLV_ID_SCAN_FLUSH		= 0x0412,
 	QTN_TLV_ID_SCAN_DWELL		= 0x0413,
+	QTN_TLV_ID_AMPDU_ENABLED	= 0x0414,
+	QTN_TLV_ID_AMSDU_ENABLED	= 0x0415,
 };
 
 struct qlink_tlv_hdr {
@@ -1195,6 +1197,11 @@ struct qlink_tlv_cclass {
 	u8 cclass;
 } __packed;
 
+struct qlink_tlv_aggr {
+	struct qlink_tlv_hdr hdr;
+	u8 aggr;
+} __packed;
+
 /**
  * enum qlink_reg_rule_flags - regulatory rule flags
  *
-- 
2.11.0

Re: [RFC 0/3] cfg80211/nl80211/iw: add basic AMPDU/AMSDU controls

[Johannes Berg]

Hi,

> Here are several RFC patches providing simple high-level controls of AMSDU/AMPDU
> aggregation. The primary purpose of this functionality is an attempt to fill
> missing gaps in nl80211 interface for basic WFA certification tests.

I see you don't implement it this way in the driver, but wouldn't it
make more sense to have this as a per-STA (RA) setting? That's really
the granularity it can be done on, I think?

Arguably even per-RA/TID, though that seems a little excessive?

> We experimented with QCA sigma-dut tool: https://github.com/qca/sigma-dut.
> The purpose was to cover basic HT/VHT WFA STA tests for cfg80211 driver
> controlled by wpa_supplicant  w/o adding any vendor specific commands.
> Multiple WFA test parameters (e.g. STBC, NSS, SGI, LDPC) can be configured
> by overriding HT/VHT capabilities in wpa_supplicant and applying them on
> connect in cfg80211_connect callback. Others (e.g. RTS params) can be
> configured using iw tool or NL80211_CMD_SET_WIPHY directly. These patches
> implement simpe high-level switches for AMSDU/AMPDU aggregation.
> 
> It would be interesting to collect comments/concerns regarding this approach.
> Does it make sense to enhance nl80211 in order to cover all the missing pieces
> required for WFA certification tests ? Or maybe it makes sense to use
> NL80211_TESTMODE subcommands for this kind of testing.

Honestly, I don't really know.

I think other tests, e.g. noack, we used to just have debugfs files for,
and then we got NL80211_CMD_SET_NOACK_MAP (which *is* per TID, btw)

Perhaps if we really don't see any value beyond the testing, keeping it
in debugfs would make sense, until we have more use cases and understand
the granularity we should support?

Clearly this is enough for the testing you refer to, but other use cases
might actually need more fine-grained control (in the future), possibly
down to the RA/TID level?

johannes

Re: [RFC 0/3] cfg80211/nl80211/iw: add basic AMPDU/AMSDU controls

[Ben Greear]

On 11/05/2018 12:32 PM, Johannes Berg wrote:
> Hi,
>
>> Here are several RFC patches providing simple high-level controls of AMSDU/AMPDU
>> aggregation. The primary purpose of this functionality is an attempt to fill
>> missing gaps in nl80211 interface for basic WFA certification tests.
>
> I see you don't implement it this way in the driver, but wouldn't it
> make more sense to have this as a per-STA (RA) setting? That's really
> the granularity it can be done on, I think?
>
> Arguably even per-RA/TID, though that seems a little excessive?

I like the idea of providing this API per peer/tid.  And, just allow
peer == -1, tid == -1 or similar to mean 'all' so that you can still set the entire device
to one particular setting w/out having to iterate through all peers if you
don't want to iterate...

>> We experimented with QCA sigma-dut tool: https://github.com/qca/sigma-dut.
>> The purpose was to cover basic HT/VHT WFA STA tests for cfg80211 driver
>> controlled by wpa_supplicant  w/o adding any vendor specific commands.
>> Multiple WFA test parameters (e.g. STBC, NSS, SGI, LDPC) can be configured
>> by overriding HT/VHT capabilities in wpa_supplicant and applying them on
>> connect in cfg80211_connect callback. Others (e.g. RTS params) can be
>> configured using iw tool or NL80211_CMD_SET_WIPHY directly. These patches
>> implement simpe high-level switches for AMSDU/AMPDU aggregation.
>>
>> It would be interesting to collect comments/concerns regarding this approach.
>> Does it make sense to enhance nl80211 in order to cover all the missing pieces
>> required for WFA certification tests ? Or maybe it makes sense to use
>> NL80211_TESTMODE subcommands for this kind of testing.
>
> Honestly, I don't really know.
>
> I think other tests, e.g. noack, we used to just have debugfs files for,
> and then we got NL80211_CMD_SET_NOACK_MAP (which *is* per TID, btw)
>
> Perhaps if we really don't see any value beyond the testing, keeping it
> in debugfs would make sense, until we have more use cases and understand
> the granularity we should support?
>
> Clearly this is enough for the testing you refer to, but other use cases
> might actually need more fine-grained control (in the future), possibly
> down to the RA/TID level?

At least with ath10k, it seems to be a common struggle for AP manufacturers
to do regulatory testing.  I would image that is true for other chipsets
as well, so it seems like it might be worth adding to the official API.

Thanks,
Ben

-- 
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc  http://www.candelatech.com

Re: [RFC 0/3] cfg80211/nl80211/iw: add basic AMPDU/AMSDU controls

[Igor Mitsyanko]

On 11/05/2018 12:45 PM, Ben Greear wrote:
>>
>> I see you don't implement it this way in the driver, but wouldn't it
>> make more sense to have this as a per-STA (RA) setting? That's really
>> the granularity it can be done on, I think?
>>
>> Arguably even per-RA/TID, though that seems a little excessive?
> 
> I like the idea of providing this API per peer/tid.  And, just allow
> peer == -1, tid == -1 or similar to mean 'all' so that you can still set 
> the entire device
> to one particular setting w/out having to iterate through all peers if you
> don't want to iterate...

Maye I'm wrong, but isn't the setting we're discussing are for the 
device itself, not for its peers? I mean, disabling AMSDU, AMPDU implies 
we need to update capabilities advertised in our information elements, 
which are common for all devices, and it affects both Tx and Rx.

And per-node/per-TID aggregation settings are a separate configuration 
option related to rate adaptation on Tx path only..

Re: [RFC 0/3] cfg80211/nl80211/iw: add basic AMPDU/AMSDU controls

[Igor Mitsyanko]

On 11/05/2018 12:45 PM, Ben Greear wrote:
>>
>> I see you don't implement it this way in the driver, but wouldn't it
>> make more sense to have this as a per-STA (RA) setting? That's really
>> the granularity it can be done on, I think?
>>
>> Arguably even per-RA/TID, though that seems a little excessive?
> 
> I like the idea of providing this API per peer/tid.  And, just allow
> peer == -1, tid == -1 or similar to mean 'all' so that you can still set 
> the entire device
> to one particular setting w/out having to iterate through all peers if you
> don't want to iterate...

Maybe I'm wrong, but isn't the setting we're discussing are for the 
device itself, not for its peers? I mean, disabling AMSDU, AMPDU implies 
we need to update capabilities advertised in our information elements, 
which are common for all devices, and it affects both Tx and Rx.

And per-node/per-TID aggregation settings are a separate configuration 
option related to rate adaptation on Tx path only..

Re: [RFC 0/3] cfg80211/nl80211/iw: add basic AMPDU/AMSDU controls

[Ben Greear]

On 11/05/2018 02:49 PM, Igor Mitsyanko wrote:
> On 11/05/2018 12:45 PM, Ben Greear wrote:
>>>
>>> I see you don't implement it this way in the driver, but wouldn't it
>>> make more sense to have this as a per-STA (RA) setting? That's really
>>> the granularity it can be done on, I think?
>>>
>>> Arguably even per-RA/TID, though that seems a little excessive?
>>
>> I like the idea of providing this API per peer/tid.  And, just allow
>> peer == -1, tid == -1 or similar to mean 'all' so that you can still set
>> the entire device
>> to one particular setting w/out having to iterate through all peers if you
>> don't want to iterate...
>
> Maye I'm wrong, but isn't the setting we're discussing are for the
> device itself, not for its peers? I mean, disabling AMSDU, AMPDU implies
> we need to update capabilities advertised in our information elements,
> which are common for all devices, and it affects both Tx and Rx.
>
> And per-node/per-TID aggregation settings are a separate configuration
> option related to rate adaptation on Tx path only..

You can advertise your maximum capabilities, but just because you advertise
that you can do large AMPDU chains doesn't mean you are required to send
them.

So, to advertise stuff, it is per vdev (not per radio), but once you associate
a peer, you might decide to configure it so that you always send no more than 5
frames in an AMPDU chain, for instance.

And, you might decide that BE gets up to 32 AMPDU chain, but VI should be limitted to 16
to decrease latency just a bit.

Thanks,
Ben


-- 
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc  http://www.candelatech.com

Re: [RFC 0/3] cfg80211/nl80211/iw: add basic AMPDU/AMSDU controls

[Sergey Matyukevich]

> On 11/05/2018 02:49 PM, Igor Mitsyanko wrote:
> > On 11/05/2018 12:45 PM, Ben Greear wrote:
> > > > 
> > > > I see you don't implement it this way in the driver, but wouldn't it
> > > > make more sense to have this as a per-STA (RA) setting? That's really
> > > > the granularity it can be done on, I think?
> > > > 
> > > > Arguably even per-RA/TID, though that seems a little excessive?
> > > 
> > > I like the idea of providing this API per peer/tid.  And, just allow
> > > peer == -1, tid == -1 or similar to mean 'all' so that you can still set
> > > the entire device
> > > to one particular setting w/out having to iterate through all peers if you
> > > don't want to iterate...
> > 
> > Maye I'm wrong, but isn't the setting we're discussing are for the
> > device itself, not for its peers? I mean, disabling AMSDU, AMPDU implies
> > we need to update capabilities advertised in our information elements,
> > which are common for all devices, and it affects both Tx and Rx.
> > 
> > And per-node/per-TID aggregation settings are a separate configuration
> > option related to rate adaptation on Tx path only..
> 
> You can advertise your maximum capabilities, but just because you advertise
> that you can do large AMPDU chains doesn't mean you are required to send
> them.
> 
> So, to advertise stuff, it is per vdev (not per radio), but once you associate
> a peer, you might decide to configure it so that you always send no more than 5
> frames in an AMPDU chain, for instance.
> 
> And, you might decide that BE gets up to 32 AMPDU chain, but VI should be limitted to 16
> to decrease latency just a bit.

Hi all,

Thanks for the comments. It turns out that RA/TID-aware approach to
AMPDU configuration has been already posted. Johannes pointed me at
the patch set adding support for TID specific configuration:
https://patchwork.kernel.org/project/linux-wireless/list/?series=33855

That patch set enables per-TID and per-STA configuration. AFAICS it
can be easily extended to support AMSDU and even configurable
AMPDU chain depth.

Regards,
Sergey