Add infrastructure for per TID aggregation/retry count configurations
such as retry count and AMPDU aggregation control(disable/enable).
In some scenario reducing the number of retry count for a specific data
traffic can reduce the latency by proceeding with the next packet
instead of retrying the same packet more time. This will be useful
where the next packet can resume the operation without an issue.
Here added NL80211_CMD_SET_TID_CONFIG to support this operation by
accepting retry count and AMPDU aggregation control.
This command can accept STA mac addreess to make the configuration
station specific rather than applying to all the connected stations
to the netdev.
Tamizh chelvam (3):
nl80211: Add netlink attribute for AMPDU aggregation enable/disable
tid conf 3
ath10k: Add support to configure TID specific configuration
Vasanthakumar Thiagarajan (1):
New netlink command for TID specific configuration
Note:
* This patchset rebased on top of [PATCH 0/6] wireless: Per-sta NoAck and offload support
drivers/net/wireless/ath/ath10k/core.h | 23 ++++
drivers/net/wireless/ath/ath10k/mac.c | 240 +++++++++++++++++++++++++++++----
drivers/net/wireless/ath/ath10k/wmi.c | 6 +-
drivers/net/wireless/ath/ath10k/wmi.h | 1 +
include/net/cfg80211.h | 20 +++
include/net/mac80211.h | 36 +++++
include/uapi/linux/nl80211.h | 90 +++++++++++++
net/mac80211/cfg.c | 71 ++++++++++
net/mac80211/driver-ops.h | 16 +++
net/mac80211/trace.h | 34 +++++
net/wireless/nl80211.c | 103 ++++++++++++++
net/wireless/rdev-ops.h | 30 +++++
net/wireless/trace.h | 50 +++++++
13 files changed, 695 insertions(+), 25 deletions(-)
--
1.9.1
From: Vasanthakumar Thiagarajan <[email protected]>
Add a new NL command, NL80211_CMD_SET_TID_CONFIG, for TID specific
configurations such as retry count and AMPDU aggregation
control(disable/enable). These per-TID configurations are passed in
NL80211_ATTR_TID_CONFIG which is a nested attribute of TID configuration.
TID for which the retry configuration is to be applied is passed in
NL80211_ATTR_TID attribute. When the user-space wants this configuration
peer specific rather than being applied for all the connected stations,
MAC address of the peer can be passed in NL80211_ATTR_MAC attribute.
The retry count is passed in NL80211_ATTR_TID_RETRY_SHORT and/or
NL80211_ATTR_TID_RETRY_LONG attribute.
Driver should advertise WIPHY_FLAG_HAS_MAX_DATA_RETRY_COUNT and
max_data_retry_count value to notify user space to avoid of passing
greater than the allowed limit.
Driver supporting TID specific configuration should advertise
NL80211_EXT_FEATURE_PER_TID_* and supporting per-STA data
retry count configuration should advertise NL80211_EXT_FEATURE_PER_STA_*.
Co-Developed-by: Tamizh Chelvam <[email protected]>
Signed-off-by: Vasanthakumar Thiagarajan <[email protected]>
Signed-off-by: Tamizh chelvam <[email protected]>
---
include/net/cfg80211.h | 14 +++++++
include/uapi/linux/nl80211.h | 69 +++++++++++++++++++++++++++++++++
net/wireless/nl80211.c | 86 ++++++++++++++++++++++++++++++++++++++++++
net/wireless/rdev-ops.h | 15 ++++++++
net/wireless/trace.h | 27 +++++++++++++
5 files changed, 211 insertions(+)
diff --git a/include/net/cfg80211.h b/include/net/cfg80211.h
index 5801d76..dd024da 100644
--- a/include/net/cfg80211.h
+++ b/include/net/cfg80211.h
@@ -3191,6 +3191,9 @@ struct cfg80211_ftm_responder_stats {
*
* @get_ftm_responder_stats: Retrieve FTM responder statistics, if available.
* Statistics should be cumulative, currently no way to reset is provided.
+ * @set_data_retry_count: configure the number of retries for the data frames
+ * for the given TID. If the retry configuration needs to be peer specific,
+ * peer MAC address can be passed.
*/
struct cfg80211_ops {
int (*suspend)(struct wiphy *wiphy, struct cfg80211_wowlan *wow);
@@ -3500,6 +3503,10 @@ struct cfg80211_ops {
int (*get_ftm_responder_stats)(struct wiphy *wiphy,
struct net_device *dev,
struct cfg80211_ftm_responder_stats *ftm_stats);
+ int (*set_data_retry_count)(struct wiphy *wiphy,
+ struct net_device *dev,
+ const u8 *peer, u8 tid,
+ int retry_short, int retry_long);
};
/*
@@ -3547,6 +3554,7 @@ struct cfg80211_ops {
* beaconing mode (AP, IBSS, Mesh, ...).
* @WIPHY_FLAG_HAS_STATIC_WEP: The device supports static WEP key installation
* before connection.
+ * @WIPHY_FLAG_HAS_MAX_DATA_RETRY_COUNT: Device supports data retry count call.
*/
enum wiphy_flags {
/* use hole at 0 */
@@ -3573,6 +3581,7 @@ enum wiphy_flags {
WIPHY_FLAG_SUPPORTS_5_10_MHZ = BIT(22),
WIPHY_FLAG_HAS_CHANNEL_SWITCH = BIT(23),
WIPHY_FLAG_HAS_STATIC_WEP = BIT(24),
+ WIPHY_FLAG_HAS_MAX_DATA_RETRY_COUNT = BIT(25),
};
/**
@@ -4035,6 +4044,9 @@ struct wiphy_iftype_ext_capab {
* @txq_limit: configuration of internal TX queue frame limit
* @txq_memory_limit: configuration internal TX queue memory limit
* @txq_quantum: configuration of internal TX queue scheduler quantum
+ *
+ * @max_data_retry_count: Maximum limit can be configured as retry count
+ * for a TID.
*/
struct wiphy {
/* assign these fields before you register the wiphy */
@@ -4171,6 +4183,8 @@ struct wiphy {
u32 txq_memory_limit;
u32 txq_quantum;
+ u8 max_data_retry_count;
+
char priv[0] __aligned(NETDEV_ALIGN);
};
diff --git a/include/uapi/linux/nl80211.h b/include/uapi/linux/nl80211.h
index 31b7a4b..9dfcf0a6 100644
--- a/include/uapi/linux/nl80211.h
+++ b/include/uapi/linux/nl80211.h
@@ -1047,6 +1047,10 @@
* @NL80211_CMD_GET_FTM_RESPONDER_STATS: Retrieve FTM responder statistics, in
* the %NL80211_ATTR_FTM_RESPONDER_STATS attribute.
*
+ * @NL80211_CMD_SET_TID_CONFIG: Data frame TID specific configuration
+ * is passed through this command using %NL80211_ATTR_TID_CONFIG
+ * nested attributes.
+ *
* @NL80211_CMD_MAX: highest used command number
* @__NL80211_CMD_AFTER_LAST: internal use
*/
@@ -1261,6 +1265,8 @@ enum nl80211_commands {
NL80211_CMD_GET_FTM_RESPONDER_STATS,
+ NL80211_CMD_SET_TID_CONFIG,
+
/* add new commands above here */
/* used to define NL80211_CMD_MAX below */
@@ -2264,6 +2270,10 @@ enum nl80211_commands {
*
* @NL80211_ATTR_FTM_RESPONDER_STATS: Nested attribute with FTM responder
* statistics, see &enum nl80211_ftm_responder_stats.
+ * @NL80211_ATTR_TID_CONFIG: TID specific configuration in a
+ * nested attribute with %NL80211_ATTR_TID_* sub-attributes.
+ * @NL80211_ATTR_MAX_RETRY_COUNT: The upper limit for the retry count
+ * configuration that the driver can accept.
*
* @NUM_NL80211_ATTR: total number of nl80211_attrs available
* @NL80211_ATTR_MAX: highest attribute number currently defined
@@ -2710,6 +2720,9 @@ enum nl80211_attrs {
NL80211_ATTR_FTM_RESPONDER_STATS,
+ NL80211_ATTR_TID_CONFIG,
+ NL80211_ATTR_MAX_RETRY_COUNT,
+
/* add attributes here, update the policy in nl80211.c */
__NL80211_ATTR_AFTER_LAST,
@@ -4399,6 +4412,56 @@ enum nl80211_ps_state {
NL80211_PS_ENABLED,
};
+/*
+ * @NL80211_ATTR_TID: a TID value (u8 attribute)
+ * @NL80211_ATTR_TID_RETRY_CONFIG: Data frame retry count should be
+ * applied with the value passed through %NL80211_ATTR_RETRY_LONG
+ * and/or %NL80211_ATTR_RETRY_SHORT. This configuration is per-TID,
+ * TID is specified with %NL80211_ATTR_TID. If the peer MAC address
+ * is passed in %NL80211_ATTR_MAC, the retry configuration is applied
+ * to the data frame for the tid to that connected station.
+ * This attribute will be useful to notfiy the driver to apply default
+ * retry values for the connected station (%NL80211_ATTR_MAC), when the
+ * command received without %NL80211_ATTR_RETRY_LONG and/or
+ * %NL80211_ATTR_RETRY_SHORT.
+ * Station specific retry configuration is valid only for STA's
+ * current connection. i.e. the configuration will be reset to default when
+ * the station connects back after disconnection/roaming.
+ * when user-space does not include %NL80211_ATTR_MAC, this configuration
+ * should be treated as per-netdev configuration. This configuration will
+ * be cleared when the interface goes down and on the disconnection from a
+ * BSS. When retry count has never been configured using this command, the
+ * other available radio level retry configuration
+ * (%NL80211_ATTR_WIPHY_RETRY_SHORT and %NL80211_ATTR_WIPHY_RETRY_LONG)
+ * should be used. Driver supporting this feature should advertise
+ * NL80211_EXT_FEATURE_PER_TID_RETRY_CONFIG and supporting per station
+ * retry count configuration should advertise
+ * NL80211_EXT_FEATURE_PER_STA_RETRY_CONFIG.
+ * @NL80211_ATTR_TID_RETRY_SHORT: Number of retries used with data frame
+ * transmission, user-space sets this configuration in
+ * &NL80211_CMD_SET_TID_CONFIG. Its type is u8, min value is 1 and
+ * the max value should be advertised by the driver through
+ * max_data_retry_count. when this attribute is not present, the driver
+ * would use the default configuration.
+ * @NL80211_ATTR_TID_RETRY_LONG: Number of retries used with data frame
+ * transmission, user-space sets this configuration in
+ * &NL80211_CMD_SET_TID_CONFIG. Its type is u8, min value is 1 and
+ * the max value should be advertised by the driver through
+ * max_data_retry_count. when this attribute is not present, the driver
+ * would use the default configuration.
+ */
+enum nl80211_attr_tid_config {
+ __NL80211_ATTR_TID_INVALID,
+ NL80211_ATTR_TID,
+ NL80211_ATTR_TID_RETRY_CONFIG,
+ NL80211_ATTR_TID_RETRY_SHORT,
+ NL80211_ATTR_TID_RETRY_LONG,
+
+ /* keep last */
+ __NL80211_ATTR_TID_AFTER_LAST,
+ NL80211_ATTR_TID_MAX = __NL80211_ATTR_TID_AFTER_LAST - 1
+};
+
/**
* enum nl80211_attr_cqm - connection quality monitor attributes
* @__NL80211_ATTR_CQM_INVALID: invalid
@@ -5270,6 +5333,10 @@ enum nl80211_feature_flags {
* freeze the connection.
* @NL80211_EXT_FEATURE_PER_STA_NOACK_MAP: Driver supports STA specific NoAck
* policy functionality.
+ * @NL80211_EXT_FEATURE_PER_TID_RETRY_CONFIG: Driver supports per TID data retry
+ * count funcationality.
+ * @NL80211_EXT_FEATURE_PER_STA_RETRY_CONFIG: Driver supports STA specific
+ * data retry count functionality.
*
* @NUM_NL80211_EXT_FEATURES: number of extended features.
* @MAX_NL80211_EXT_FEATURES: highest extended feature index.
@@ -5311,6 +5378,8 @@ enum nl80211_ext_feature_index {
NL80211_EXT_FEATURE_CAN_REPLACE_PTK0,
NL80211_EXT_FEATURE_ENABLE_FTM_RESPONDER,
NL80211_EXT_FEATURE_PER_STA_NOACK_MAP,
+ NL80211_EXT_FEATURE_PER_TID_RETRY_CONFIG,
+ NL80211_EXT_FEATURE_PER_STA_RETRY_CONFIG,
/* add new features before the definition below */
NUM_NL80211_EXT_FEATURES,
diff --git a/net/wireless/nl80211.c b/net/wireless/nl80211.c
index d744388..d386ad7 100644
--- a/net/wireless/nl80211.c
+++ b/net/wireless/nl80211.c
@@ -1703,6 +1703,10 @@ static int nl80211_send_wiphy(struct cfg80211_registered_device *rdev,
if ((rdev->wiphy.flags & WIPHY_FLAG_TDLS_EXTERNAL_SETUP) &&
nla_put_flag(msg, NL80211_ATTR_TDLS_EXTERNAL_SETUP))
goto nla_put_failure;
+ if ((rdev->wiphy.flags & WIPHY_FLAG_HAS_MAX_DATA_RETRY_COUNT) &&
+ nla_put_u8(msg, NL80211_ATTR_MAX_RETRY_COUNT,
+ rdev->wiphy.max_data_retry_count))
+ goto nla_put_failure;
state->split_start++;
if (state->split)
break;
@@ -12992,6 +12996,80 @@ static int nl80211_get_ftm_responder_stats(struct sk_buff *skb,
return -ENOBUFS;
}
+static const struct nla_policy
+nl80211_attr_tid_policy[NL80211_ATTR_TID_MAX + 1] = {
+ [NL80211_ATTR_TID] = { .type = NLA_U8 },
+ [NL80211_ATTR_TID_RETRY_CONFIG] = { .type = NLA_FLAG },
+ [NL80211_ATTR_TID_RETRY_SHORT] = { .type = NLA_U8 },
+ [NL80211_ATTR_TID_RETRY_LONG] = { .type = NLA_U8 },
+};
+
+static int nl80211_set_tid_config(struct sk_buff *skb,
+ struct genl_info *info)
+{
+ struct cfg80211_registered_device *rdev = info->user_ptr[0];
+ struct nlattr *attrs[NL80211_ATTR_TID_MAX + 1];
+ struct nlattr *tid;
+ struct net_device *dev = info->user_ptr[1];
+ const char *peer = NULL;
+ u8 tid_no;
+ int ret = -EINVAL, retry_short = -1, retry_long = -1;
+
+ tid = info->attrs[NL80211_ATTR_TID_CONFIG];
+ if (!tid)
+ return -EINVAL;
+
+ ret = nla_parse_nested(attrs, NL80211_ATTR_TID_MAX, tid,
+ nl80211_attr_tid_policy, info->extack);
+ if (ret)
+ return ret;
+
+ if (!attrs[NL80211_ATTR_TID])
+ return -EINVAL;
+
+ if (attrs[NL80211_ATTR_TID_RETRY_SHORT]) {
+ retry_short = nla_get_u8(attrs[NL80211_ATTR_TID_RETRY_SHORT]);
+ if (!retry_short ||
+ retry_short > rdev->wiphy.max_data_retry_count)
+ return -EINVAL;
+ }
+
+ if (attrs[NL80211_ATTR_TID_RETRY_LONG]) {
+ retry_long = nla_get_u8(attrs[NL80211_ATTR_TID_RETRY_LONG]);
+ if (!retry_long ||
+ retry_long > rdev->wiphy.max_data_retry_count)
+ return -EINVAL;
+ }
+
+ tid_no = nla_get_u8(attrs[NL80211_ATTR_TID]);
+ if (tid_no >= IEEE80211_FIRST_TSPEC_TSID)
+ return -EINVAL;
+
+ if (info->attrs[NL80211_ATTR_MAC])
+ peer = nla_data(info->attrs[NL80211_ATTR_MAC]);
+
+ if (nla_get_flag(attrs[NL80211_ATTR_TID_RETRY_CONFIG])) {
+ if (!wiphy_ext_feature_isset(
+ &rdev->wiphy,
+ NL80211_EXT_FEATURE_PER_TID_RETRY_CONFIG))
+ return -EOPNOTSUPP;
+
+ if (peer && !wiphy_ext_feature_isset(
+ &rdev->wiphy,
+ NL80211_EXT_FEATURE_PER_STA_RETRY_CONFIG))
+ return -EOPNOTSUPP;
+
+ if (!rdev->ops->set_data_retry_count ||
+ !rdev->wiphy.max_data_retry_count)
+ return -EOPNOTSUPP;
+
+ ret = rdev_set_data_retry_count(rdev, dev, peer, tid_no,
+ retry_short, retry_long);
+ }
+
+ return ret;
+}
+
#define NL80211_FLAG_NEED_WIPHY 0x01
#define NL80211_FLAG_NEED_NETDEV 0x02
#define NL80211_FLAG_NEED_RTNL 0x04
@@ -13910,6 +13988,14 @@ static void nl80211_post_doit(const struct genl_ops *ops, struct sk_buff *skb,
.internal_flags = NL80211_FLAG_NEED_NETDEV |
NL80211_FLAG_NEED_RTNL,
},
+ {
+ .cmd = NL80211_CMD_SET_TID_CONFIG,
+ .doit = nl80211_set_tid_config,
+ .policy = nl80211_policy,
+ .flags = GENL_UNS_ADMIN_PERM,
+ .internal_flags = NL80211_FLAG_NEED_NETDEV |
+ NL80211_FLAG_NEED_RTNL,
+ },
};
static struct genl_family nl80211_fam __ro_after_init = {
diff --git a/net/wireless/rdev-ops.h b/net/wireless/rdev-ops.h
index 8426eb1..b61e476 100644
--- a/net/wireless/rdev-ops.h
+++ b/net/wireless/rdev-ops.h
@@ -1248,4 +1248,19 @@ static inline int rdev_del_pmk(struct cfg80211_registered_device *rdev,
return ret;
}
+static inline int
+rdev_set_data_retry_count(struct cfg80211_registered_device *rdev,
+ struct net_device *dev, const u8 *peer,
+ u8 tid, int retry_short, int retry_long)
+{
+ int ret;
+
+ trace_rdev_set_data_retry_count(&rdev->wiphy, dev, peer, tid,
+ retry_short, retry_long);
+ ret = rdev->ops->set_data_retry_count(&rdev->wiphy, dev, peer,
+ tid, retry_short, retry_long);
+ trace_rdev_return_int(&rdev->wiphy, ret);
+ return ret;
+}
+
#endif /* __CFG80211_RDEV_OPS */
diff --git a/net/wireless/trace.h b/net/wireless/trace.h
index 2ff78dd..7b55da9 100644
--- a/net/wireless/trace.h
+++ b/net/wireless/trace.h
@@ -3297,6 +3297,33 @@
TP_printk(WIPHY_PR_FMT ", " WDEV_PR_FMT,
WIPHY_PR_ARG, WDEV_PR_ARG)
);
+
+TRACE_EVENT(rdev_set_data_retry_count,
+ TP_PROTO(struct wiphy *wiphy, struct net_device *netdev,
+ const u8 *peer, u8 tid, int retry_short, int retry_long),
+ TP_ARGS(wiphy, netdev, peer, tid, retry_short, retry_long),
+ TP_STRUCT__entry(
+ WIPHY_ENTRY
+ NETDEV_ENTRY
+ MAC_ENTRY(peer)
+ __field(u8, tid)
+ __field(int, retry_short)
+ __field(int, retry_long)
+ ),
+ TP_fast_assign(
+ WIPHY_ASSIGN;
+ NETDEV_ASSIGN;
+ MAC_ASSIGN(peer, peer);
+ __entry->tid = tid;
+ __entry->retry_short = retry_short;
+ __entry->retry_long = retry_long;
+ ),
+ TP_printk(WIPHY_PR_FMT ", " NETDEV_PR_FMT ", peer: " MAC_PR_FMT
+ ", tid: %u, retry_short: %d, retry_long: %d", WIPHY_PR_ARG,
+ NETDEV_PR_ARG, MAC_PR_ARG(peer), __entry->tid,
+ __entry->retry_short, __entry->retry_long)
+);
+
#endif /* !__RDEV_OPS_TRACE || TRACE_HEADER_MULTI_READ */
#undef TRACE_INCLUDE_PATH
--
1.7.9.5
Introduce NL80211_ATTR_TID_AMPDU_AGGR_CTRL in nl80211_attr_tid_config
to accept TID specific AMPDU aggregation enable/disable configuration
through NL80211_CMD_SET_TID_CONFIG command. TID for which the
aggregation control configuration is to be applied is passed in
NL80211_ATTR_TID attribute. When the user-space wants this
configuration peer specific rather than being applied for all the
connected stations, MAC address of the peer can be passed in
NL80211_ATTR_MAC attribute.
Driver supporting this feature should advertise
NL80211_EXT_FEATURE_PER_TID_AMPDU_AGGR_CTRL and supporting per-STA aggregation
configuration should advertise NL80211_EXT_FEATURE_PER_STA_AMPDU_AGGR_CTRL.
Signed-off-by: Tamizh chelvam <[email protected]>
---
include/net/cfg80211.h | 6 ++++++
include/uapi/linux/nl80211.h | 21 +++++++++++++++++++++
net/wireless/nl80211.c | 17 +++++++++++++++++
net/wireless/rdev-ops.h | 15 +++++++++++++++
net/wireless/trace.h | 23 +++++++++++++++++++++++
5 files changed, 82 insertions(+)
diff --git a/include/net/cfg80211.h b/include/net/cfg80211.h
index dd024da..f2b4280 100644
--- a/include/net/cfg80211.h
+++ b/include/net/cfg80211.h
@@ -3194,6 +3194,9 @@ struct cfg80211_ftm_responder_stats {
* @set_data_retry_count: configure the number of retries for the data frames
* for the given TID. If the retry configuration needs to be peer specific,
* peer MAC address can be passed.
+ * @set_tid_aggr_config: enable/disable aggregation configuration for the given
+ * TID. If this configuration needs to be peer specific peer MAC address
+ * can be passed.
*/
struct cfg80211_ops {
int (*suspend)(struct wiphy *wiphy, struct cfg80211_wowlan *wow);
@@ -3507,6 +3510,9 @@ struct cfg80211_ops {
struct net_device *dev,
const u8 *peer, u8 tid,
int retry_short, int retry_long);
+ int (*set_tid_aggr_config)(struct wiphy *wiphy,
+ struct net_device *dev, const u8 *peer,
+ u8 tid, bool aggr);
};
/*
diff --git a/include/uapi/linux/nl80211.h b/include/uapi/linux/nl80211.h
index 9dfcf0a6..7ba0fb7 100644
--- a/include/uapi/linux/nl80211.h
+++ b/include/uapi/linux/nl80211.h
@@ -4449,6 +4449,20 @@ enum nl80211_ps_state {
* the max value should be advertised by the driver through
* max_data_retry_count. when this attribute is not present, the driver
* would use the default configuration.
+ * @NL80211_ATTR_TID_AMPDU_AGGR_CTRL: Enable/Disable aggregation for the TID
+ * specified in %%NL80211_ATTR_TID. Its type is u8, if the peer MAC address
+ * is passed in %NL80211_ATTR_MAC, the aggregation configuration is applied
+ * to the data frame for the tid to that connected station.
+ * Station specific aggregation configuration is valid only for STA's
+ * current connection. i.e. the configuration will be reset to default when
+ * the station connects back after disconnection/roaming.
+ * when user-space does not include %NL80211_ATTR_MAC, this configuration
+ * should be treated as per-netdev configuration. This configuration will
+ * be cleared when the interface goes down and on the disconnection from a
+ * BSS. Driver supporting this feature should advertise
+ * NL80211_EXT_FEATURE_PER_STA_AMPDU_AGGR_CTRL and supporting per station
+ * aggregation configuration should advertise
+ * NL80211_EXT_FEATURE_PER_STA_AMPDU_AGGR_CTRL.
*/
enum nl80211_attr_tid_config {
__NL80211_ATTR_TID_INVALID,
@@ -4456,6 +4470,7 @@ enum nl80211_attr_tid_config {
NL80211_ATTR_TID_RETRY_CONFIG,
NL80211_ATTR_TID_RETRY_SHORT,
NL80211_ATTR_TID_RETRY_LONG,
+ NL80211_ATTR_TID_AMPDU_AGGR_CTRL,
/* keep last */
__NL80211_ATTR_TID_AFTER_LAST,
@@ -5337,6 +5352,10 @@ enum nl80211_feature_flags {
* count funcationality.
* @NL80211_EXT_FEATURE_PER_STA_RETRY_CONFIG: Driver supports STA specific
* data retry count functionality.
+ * @NL80211_EXT_FEATURE_PER_TID_AMPDU_AGGR_CTRL: Driver supports TID specific
+ * aggregation control(enable/disable).
+ * @NL80211_EXT_FEATURE_PER_STA_AMPDU_AGGR_CTRL: Driver supports per STA
+ * specific TID aggregation control(enable/disable).
*
* @NUM_NL80211_EXT_FEATURES: number of extended features.
* @MAX_NL80211_EXT_FEATURES: highest extended feature index.
@@ -5380,6 +5399,8 @@ enum nl80211_ext_feature_index {
NL80211_EXT_FEATURE_PER_STA_NOACK_MAP,
NL80211_EXT_FEATURE_PER_TID_RETRY_CONFIG,
NL80211_EXT_FEATURE_PER_STA_RETRY_CONFIG,
+ NL80211_EXT_FEATURE_PER_TID_AMPDU_AGGR_CTRL,
+ NL80211_EXT_FEATURE_PER_STA_AMPDU_AGGR_CTRL,
/* add new features before the definition below */
NUM_NL80211_EXT_FEATURES,
diff --git a/net/wireless/nl80211.c b/net/wireless/nl80211.c
index d386ad7..20c349b 100644
--- a/net/wireless/nl80211.c
+++ b/net/wireless/nl80211.c
@@ -13002,6 +13002,7 @@ static int nl80211_get_ftm_responder_stats(struct sk_buff *skb,
[NL80211_ATTR_TID_RETRY_CONFIG] = { .type = NLA_FLAG },
[NL80211_ATTR_TID_RETRY_SHORT] = { .type = NLA_U8 },
[NL80211_ATTR_TID_RETRY_LONG] = { .type = NLA_U8 },
+ [NL80211_ATTR_TID_AMPDU_AGGR_CTRL] = { .type = NLA_U8 },
};
static int nl80211_set_tid_config(struct sk_buff *skb,
@@ -13014,6 +13015,7 @@ static int nl80211_set_tid_config(struct sk_buff *skb,
const char *peer = NULL;
u8 tid_no;
int ret = -EINVAL, retry_short = -1, retry_long = -1;
+ bool aggr;
tid = info->attrs[NL80211_ATTR_TID_CONFIG];
if (!tid)
@@ -13067,6 +13069,21 @@ static int nl80211_set_tid_config(struct sk_buff *skb,
retry_short, retry_long);
}
+ if (attrs[NL80211_ATTR_TID_AMPDU_AGGR_CTRL]) {
+ if (!rdev->ops->set_tid_aggr_config ||
+ !wiphy_ext_feature_isset(&rdev->wiphy,
+ NL80211_EXT_FEATURE_PER_TID_AMPDU_AGGR_CTRL))
+ return -EOPNOTSUPP;
+
+ if (peer && !wiphy_ext_feature_isset(
+ &rdev->wiphy,
+ NL80211_EXT_FEATURE_PER_STA_AMPDU_AGGR_CTRL))
+ return -EOPNOTSUPP;
+
+ aggr = !!nla_get_u8(attrs[NL80211_ATTR_TID_AMPDU_AGGR_CTRL]);
+ ret = rdev_set_tid_aggr_config(rdev, dev, peer, tid_no, aggr);
+ }
+
return ret;
}
diff --git a/net/wireless/rdev-ops.h b/net/wireless/rdev-ops.h
index b61e476..6c35b75 100644
--- a/net/wireless/rdev-ops.h
+++ b/net/wireless/rdev-ops.h
@@ -1263,4 +1263,19 @@ static inline int rdev_del_pmk(struct cfg80211_registered_device *rdev,
return ret;
}
+static inline int
+rdev_set_tid_aggr_config(struct cfg80211_registered_device *rdev,
+ struct net_device *dev, const u8 *peer,
+ u8 tid, bool aggr)
+{
+ int ret;
+
+ trace_rdev_set_tid_aggr_config(&rdev->wiphy, dev, peer, tid,
+ aggr);
+ ret = rdev->ops->set_tid_aggr_config(&rdev->wiphy, dev, peer,
+ tid, aggr);
+ trace_rdev_return_int(&rdev->wiphy, ret);
+ return ret;
+}
+
#endif /* __CFG80211_RDEV_OPS */
diff --git a/net/wireless/trace.h b/net/wireless/trace.h
index 7b55da9..246f122 100644
--- a/net/wireless/trace.h
+++ b/net/wireless/trace.h
@@ -3324,6 +3324,29 @@
__entry->retry_short, __entry->retry_long)
);
+TRACE_EVENT(rdev_set_tid_aggr_config,
+ TP_PROTO(struct wiphy *wiphy, struct net_device *netdev,
+ const u8 *peer, u8 tid, bool aggr),
+ TP_ARGS(wiphy, netdev, peer, tid, aggr),
+ TP_STRUCT__entry(
+ WIPHY_ENTRY
+ NETDEV_ENTRY
+ MAC_ENTRY(peer)
+ __field(u8, tid)
+ __field(bool, aggr)
+ ),
+ TP_fast_assign(
+ WIPHY_ASSIGN;
+ NETDEV_ASSIGN;
+ MAC_ASSIGN(peer, peer);
+ __entry->tid = tid;
+ __entry->aggr = aggr;
+ ),
+ TP_printk(WIPHY_PR_FMT ", " NETDEV_PR_FMT ", peer: " MAC_PR_FMT
+ ", tid: %u, aggregation: %s", WIPHY_PR_ARG,
+ NETDEV_PR_ARG, MAC_PR_ARG(peer), __entry->tid,
+ BOOL_TO_STR(__entry->aggr))
+);
#endif /* !__RDEV_OPS_TRACE || TRACE_HEADER_MULTI_READ */
#undef TRACE_INCLUDE_PATH
--
1.7.9.5
Implement drv_set_tid_conf api to allow TID specific
retry count for data frames and aggregation enable/disable
configuration. If the retry_short and/or
retry_long value is default (-1), use the nedev wide retry
configuration for the station. This per-TID configuration
will be applied for all the connected stations when MAC is NULL.
enum ieee80211_tid_conf_change introduced to notify the the driver
about which configuration parameter got changed in
ieee80211_tid_conf structure.
Signed-off-by: Tamizh chelvam <[email protected]>
---
include/net/mac80211.h | 40 +++++++++++++++++++++++++
net/mac80211/cfg.c | 71 +++++++++++++++++++++++++++++++++++++++++++++
net/mac80211/driver-ops.h | 16 ++++++++++
net/mac80211/trace.h | 34 ++++++++++++++++++++++
4 files changed, 161 insertions(+)
diff --git a/include/net/mac80211.h b/include/net/mac80211.h
index b6cc3e33..7fa7e25 100644
--- a/include/net/mac80211.h
+++ b/include/net/mac80211.h
@@ -1478,6 +1478,35 @@ struct ieee80211_channel_switch {
u8 count;
};
+/*
+ * enum ieee80211_tid_conf_change - TID change configuration notification flags
+ *
+ * These flags are used with the set_tid_conf() callback
+ * to indicate which TID configuration parameter changed.
+ *
+ * @TID_RETRY_CONF_CHANGED: retry configuration changed.
+ * @TID_AGGR_CONF_CHANGED: Aggregation config changed for the TID.
+ */
+enum ieee80211_tid_conf_change {
+ TID_RETRY_CONF_CHANGED = BIT(0),
+ TID_AGGR_CONF_CHANGED = BIT(1),
+};
+
+/*
+ * struct ieee80211_tid_conf - holds the tid configiuration data
+ * The information provided in the structure is required for the driver
+ * to configure TID specific configuration.
+ * @tid: TID number
+ * @retry_short: retry count value
+ * @retry_long: retry count value
+ * @aggr: enable/disable aggregation
+ */
+struct ieee80211_tid_conf {
+ u8 tid;
+ int retry_short;
+ int retry_long;
+ bool aggr;
+};
/**
* enum ieee80211_vif_flags - virtual interface flags
*
@@ -1565,6 +1594,8 @@ struct ieee80211_vif {
bool txqs_stopped[IEEE80211_NUM_ACS];
+ struct ieee80211_tid_conf tid_conf;
+
/* must be last */
u8 drv_priv[0] __aligned(sizeof(void *));
};
@@ -3632,6 +3663,10 @@ enum ieee80211_reconfig_type {
* level. Drivers mplementing this callback must take care of setting NoAck
* policy in QOS control field based on the configured TID bitmap.
* This callback may sleep.
+ * @set_tid_conf: TID specific configuration like number of retries for
+ * the given TID. Apply this configuration for a particular station when
+ * @sta is non-NULL. When @sta is NULL, the configuration will be for all
+ * the connected clients in the vif. This callback may sleep.
*/
struct ieee80211_ops {
void (*tx)(struct ieee80211_hw *hw,
@@ -3924,6 +3959,11 @@ struct ieee80211_ops {
int (*set_noack_tid_bitmap)(struct ieee80211_hw *hw,
struct ieee80211_vif *vif,
struct ieee80211_sta *sta, int noack_map);
+ int (*set_tid_conf)(struct ieee80211_hw *hw,
+ struct ieee80211_vif *vif,
+ struct ieee80211_sta *sta,
+ struct ieee80211_tid_conf *tid_conf,
+ u8 changed);
};
/**
diff --git a/net/mac80211/cfg.c b/net/mac80211/cfg.c
index 02e6d49..786b561 100644
--- a/net/mac80211/cfg.c
+++ b/net/mac80211/cfg.c
@@ -3889,6 +3889,75 @@ static int ieee80211_get_txq_stats(struct wiphy *wiphy,
return drv_get_ftm_responder_stats(local, sdata, ftm_stats);
}
+static int ieee80211_set_data_retry_count(struct wiphy *wiphy,
+ struct net_device *dev,
+ const u8 *peer, u8 tid,
+ int retry_short, int retry_long)
+{
+ struct ieee80211_sub_if_data *sdata = IEEE80211_DEV_TO_SUB_IF(dev);
+ struct sta_info *sta;
+ int ret;
+
+ if (!sdata->local->ops->set_tid_conf)
+ return -EOPNOTSUPP;
+
+ sdata->vif.tid_conf.tid = tid;
+ sdata->vif.tid_conf.retry_short = retry_short;
+ sdata->vif.tid_conf.retry_long = retry_long;
+
+ if (!peer)
+ return drv_set_tid_conf(sdata->local, sdata, NULL,
+ TID_RETRY_CONF_CHANGED);
+
+ mutex_lock(&sdata->local->sta_mtx);
+
+ sta = sta_info_get_bss(sdata, peer);
+ if (!sta) {
+ mutex_unlock(&sdata->local->sta_mtx);
+ return -ENOENT;
+ }
+
+ ret = drv_set_tid_conf(sdata->local, sdata, &sta->sta,
+ TID_RETRY_CONF_CHANGED);
+
+ mutex_unlock(&sdata->local->sta_mtx);
+ return ret;
+}
+
+static int ieee80211_set_tid_aggr_config(struct wiphy *wiphy,
+ struct net_device *dev,
+ const u8 *peer, u8 tid,
+ bool aggr)
+{
+ struct ieee80211_sub_if_data *sdata = IEEE80211_DEV_TO_SUB_IF(dev);
+ struct sta_info *sta;
+ int ret;
+
+ if (!sdata->local->ops->set_tid_conf)
+ return -EOPNOTSUPP;
+
+ sdata->vif.tid_conf.tid = tid;
+ sdata->vif.tid_conf.aggr = aggr;
+
+ if (!peer)
+ return drv_set_tid_conf(sdata->local, sdata, NULL,
+ TID_AGGR_CONF_CHANGED);
+
+ mutex_lock(&sdata->local->sta_mtx);
+
+ sta = sta_info_get_bss(sdata, peer);
+ if (!sta) {
+ mutex_unlock(&sdata->local->sta_mtx);
+ return -ENOENT;
+ }
+
+ ret = drv_set_tid_conf(sdata->local, sdata, &sta->sta,
+ TID_AGGR_CONF_CHANGED);
+
+ mutex_unlock(&sdata->local->sta_mtx);
+ return ret;
+}
+
const struct cfg80211_ops mac80211_config_ops = {
.add_virtual_intf = ieee80211_add_iface,
.del_virtual_intf = ieee80211_del_iface,
@@ -3984,4 +4053,6 @@ static int ieee80211_get_txq_stats(struct wiphy *wiphy,
.tx_control_port = ieee80211_tx_control_port,
.get_txq_stats = ieee80211_get_txq_stats,
.get_ftm_responder_stats = ieee80211_get_ftm_responder_stats,
+ .set_data_retry_count = ieee80211_set_data_retry_count,
+ .set_tid_aggr_config = ieee80211_set_tid_aggr_config,
};
diff --git a/net/mac80211/driver-ops.h b/net/mac80211/driver-ops.h
index ed9bd59..e717cc0 100644
--- a/net/mac80211/driver-ops.h
+++ b/net/mac80211/driver-ops.h
@@ -1300,4 +1300,20 @@ static inline int drv_set_noack_tid_bitmap(struct ieee80211_local *local,
return ret;
}
+static inline int drv_set_tid_conf(struct ieee80211_local *local,
+ struct ieee80211_sub_if_data *sdata,
+ struct ieee80211_sta *sta,
+ u8 changed)
+{
+ int ret;
+
+ might_sleep();
+ trace_drv_set_tid_conf(local, sdata, sta, changed);
+ ret = local->ops->set_tid_conf(&local->hw, &sdata->vif, sta,
+ &sdata->vif.tid_conf, changed);
+ trace_drv_return_int(local, ret);
+
+ return ret;
+}
+
#endif /* __MAC80211_DRIVER_OPS */
diff --git a/net/mac80211/trace.h b/net/mac80211/trace.h
index e0e6c9a..5aed6ad 100644
--- a/net/mac80211/trace.h
+++ b/net/mac80211/trace.h
@@ -2648,6 +2648,40 @@ struct trace_switch_entry {
)
);
+TRACE_EVENT(drv_set_tid_conf,
+ TP_PROTO(struct ieee80211_local *local,
+ struct ieee80211_sub_if_data *sdata,
+ struct ieee80211_sta *sta,
+ u8 changed),
+
+ TP_ARGS(local, sdata, sta, changed),
+
+ TP_STRUCT__entry(
+ LOCAL_ENTRY
+ VIF_ENTRY
+ STA_ENTRY
+ __field(u8, tid)
+ __field(int, retry_short)
+ __field(int, retry_long)
+ __field(u8, changed)
+ ),
+
+ TP_fast_assign(
+ LOCAL_ASSIGN;
+ VIF_ASSIGN;
+ STA_ASSIGN;
+ __entry->tid = sdata->vif.tid_conf.tid;
+ __entry->retry_short = sdata->vif.tid_conf.retry_short;
+ __entry->retry_long = sdata->vif.tid_conf.retry_long;
+ __entry->changed = changed;
+ ),
+
+ TP_printk(
+ LOCAL_PR_FMT VIF_PR_FMT STA_PR_FMT " changed: %#x",
+ LOCAL_PR_ARG, VIF_PR_ARG, STA_PR_ARG, __entry->changed
+ )
+);
+
#endif /* !__MAC80211_DRIVER_TRACE || TRACE_HEADER_MULTI_READ */
#undef TRACE_INCLUDE_PATH
--
1.7.9.5
This patch add ops for set_tid_conf to support
station specific retry and aggregation configuration for a TID.
station and configuration parameter(TID, configuration changed
parameter through ieee80211_tid_conf) information will be passed
as an argument for this ops.
Since target accepts one parameter as a retry count, ath10k
pass retry_long along with TID and station's mac address to target
in the command and store this if TID_RETRY_CONF_CHANGED.
And pass enable/disable aggregation for the TID
if TID_AGGR_CONF_CHANGED is passed. Depends on the changed
parameter the respective value stored in arsta struct.
Suppose if the station entry is not mentioned in the command,
the configuration will be applied for all the connected stations
in the vif.
And this configuration will be stored in arvif to apply this
configuration for newly connecting station.
Testing:
* Tested HW: QCA9888
* Tested FW: 10.4-3.5.1-00052
Signed-off-by: Tamizh chelvam <[email protected]>
---
drivers/net/wireless/ath/ath10k/core.h | 23 ++++
drivers/net/wireless/ath/ath10k/mac.c | 240 +++++++++++++++++++++++++++++----
drivers/net/wireless/ath/ath10k/wmi.c | 6 +-
drivers/net/wireless/ath/ath10k/wmi.h | 1 +
4 files changed, 245 insertions(+), 25 deletions(-)
diff --git a/drivers/net/wireless/ath/ath10k/core.h b/drivers/net/wireless/ath/ath10k/core.h
index 418eb19..1ce46f4 100644
--- a/drivers/net/wireless/ath/ath10k/core.h
+++ b/drivers/net/wireless/ath/ath10k/core.h
@@ -93,6 +93,9 @@
/* Number of TID target accepts from host */
#define ATH10K_MAX_TIDS 0x8
+/* Upper limit/default retry count for a ppdu */
+#define ATH10K_MAX_RETRY_COUNT 30
+
struct ath10k;
static inline const char *ath10k_bus_str(enum ath10k_bus bus)
@@ -506,6 +509,16 @@ struct ath10k_sta {
int noack_map;
struct work_struct noack_map_wk;
+ /* TID retry count for a station and by default
+ * this will have -1 for all the TIDs until user changes
+ * the retry count by specifying station's MAC address
+ */
+ int retry_count[ATH10K_MAX_TIDS];
+
+ /* TID aggregation control value for the station */
+ u8 aggr_ctrl[ATH10K_MAX_TIDS];
+ struct work_struct tid_cfg_wk;
+
#ifdef CONFIG_MAC80211_DEBUGFS
/* protected by conf_mutex */
bool aggr_mode;
@@ -584,6 +597,16 @@ struct ath10k_vif {
/* TID bitmap for station's NoAck policy, protected by conf_mutex */
int noack_map;
+
+ /* TID retry count for all the stations in the vif */
+ int retry_count[ATH10K_MAX_TIDS];
+
+ /* TID aggregation control parameter fo all the connected
+ * stations in the vif
+ */
+ u8 aggr_ctrl[ATH10K_MAX_TIDS];
+ u8 tid_conf_changed;
+ u8 tid;
};
struct ath10k_vif_iter {
diff --git a/drivers/net/wireless/ath/ath10k/mac.c b/drivers/net/wireless/ath/ath10k/mac.c
index 54ec919..258473e 100644
--- a/drivers/net/wireless/ath/ath10k/mac.c
+++ b/drivers/net/wireless/ath/ath10k/mac.c
@@ -2973,6 +2973,57 @@ static void ath10k_bss_disassoc(struct ieee80211_hw *hw,
return ret;
}
+static int ath10k_new_peer_tid_config(struct ath10k *ar,
+ struct ieee80211_sta *sta,
+ struct ath10k_vif *arvif)
+{
+ struct ath10k_sta *arsta = (struct ath10k_sta *)sta->drv_priv;
+ struct wmi_per_peer_per_tid_cfg_arg arg = {};
+ int i, ret;
+
+ if (arvif->noack_map) {
+ arg.vdev_id = arvif->vdev_id;
+ ether_addr_copy(arg.peer_macaddr.addr, sta->addr);
+
+ ret = ath10k_mac_set_noack_tid_bitmap(ar, &arg,
+ arvif->noack_map);
+ if (ret) {
+ ath10k_warn(ar, "failed to set peer tid noack policy for sta %pM: %d\n",
+ sta->addr, ret);
+ return ret;
+ }
+ }
+
+ /* Assign default noack_map value (-1) to newly connecting station.
+ * This default value used to identify the station configured with
+ * vif specific noack configuration rather than station specific.
+ */
+ arsta->noack_map = -1;
+ memset(&arg, 0, sizeof(arg));
+
+ for (i = 0; i < ATH10K_MAX_TIDS; i++) {
+ if (arvif->retry_count[i] || arvif->aggr_ctrl[i]) {
+ arg.tid = i;
+ arg.vdev_id = arvif->vdev_id;
+ arg.retry_count = arvif->retry_count[i];
+ arg.aggr_control = arvif->aggr_ctrl[i];
+ ether_addr_copy(arg.peer_macaddr.addr, sta->addr);
+ ret = ath10k_wmi_set_per_peer_per_tid_cfg(ar, &arg);
+ if (ret) {
+ ath10k_warn(ar, "failed to set per tid retry/aggr config for sta %pM: %d\n",
+ sta->addr, ret);
+ return ret;
+ }
+ }
+ /* Assign default retry count(-1) to newly connected station.
+ * This is to identify station specific tid retry count not
+ * configured for the station.
+ */
+ arsta->retry_count[i] = -1;
+ }
+ return 0;
+}
+
static int ath10k_station_assoc(struct ath10k *ar,
struct ieee80211_vif *vif,
struct ieee80211_sta *sta,
@@ -2980,7 +3031,6 @@ static int ath10k_station_assoc(struct ath10k *ar,
{
struct ath10k_vif *arvif = (void *)vif->drv_priv;
struct wmi_peer_assoc_complete_arg peer_arg;
- struct ath10k_sta *arsta = (struct ath10k_sta *)sta->drv_priv;
int ret = 0;
lockdep_assert_held(&ar->conf_mutex);
@@ -3039,24 +3089,10 @@ static int ath10k_station_assoc(struct ath10k *ar,
}
}
- if (arvif->noack_map) {
- struct wmi_per_peer_per_tid_cfg_arg arg = {};
-
- arg.vdev_id = arvif->vdev_id;
- ether_addr_copy(arg.peer_macaddr.addr, sta->addr);
-
- ret = ath10k_mac_set_noack_tid_bitmap(ar, &arg,
- arvif->noack_map);
- if (ret)
- return ret;
- }
+ if (!test_bit(WMI_SERVICE_PEER_TID_CONFIGS_SUPPORT, ar->wmi.svc_map))
+ return 0;
- /* Assign default noack_map value (-1) to newly connecting station.
- * This default value used to identify the station configured with
- * vif specific noack configuration rather than station specific.
- */
- arsta->noack_map = -1;
- return ret;
+ return ath10k_new_peer_tid_config(ar, sta, arvif);
}
static int ath10k_station_disassoc(struct ath10k *ar,
@@ -6315,7 +6351,7 @@ static void ath10k_mac_dec_num_stations(struct ath10k_vif *arvif,
ar->num_stations--;
}
-struct ath10k_mac_iter_noack_map_data {
+struct ath10k_mac_iter_tid_config {
struct ieee80211_vif *curr_vif;
struct ath10k *ar;
};
@@ -6342,7 +6378,7 @@ static void ath10k_mac_vif_stations_noack_map(void *data,
struct ieee80211_sta *sta)
{
struct ath10k_sta *arsta = (struct ath10k_sta *)sta->drv_priv;
- struct ath10k_mac_iter_noack_map_data *iter_data = data;
+ struct ath10k_mac_iter_tid_config *iter_data = data;
struct ieee80211_vif *sta_vif = arsta->arvif->vif;
if (sta_vif != iter_data->curr_vif || arsta->noack_map != -1)
@@ -6351,6 +6387,56 @@ static void ath10k_mac_vif_stations_noack_map(void *data,
ieee80211_queue_work(iter_data->ar->hw, &arsta->noack_map_wk);
}
+static void ath10k_sta_tid_cfg_wk(struct work_struct *wk)
+{
+ struct wmi_per_peer_per_tid_cfg_arg arg = {};
+ struct ieee80211_sta *sta;
+ struct ath10k_sta *arsta;
+ struct ath10k_vif *arvif;
+ struct ath10k *ar;
+ int ret;
+
+ arsta = container_of(wk, struct ath10k_sta, noack_map_wk);
+ sta = container_of((void *)arsta, struct ieee80211_sta, drv_priv);
+ arvif = arsta->arvif;
+ ar = arvif->ar;
+ arg.vdev_id = arvif->vdev_id;
+ arg.tid = arvif->tid;
+ ether_addr_copy(arg.peer_macaddr.addr, sta->addr);
+
+ if (arvif->tid_conf_changed & TID_RETRY_CONF_CHANGED) {
+ if (arsta->retry_count[arvif->tid] != -1)
+ return;
+
+ arg.retry_count = arvif->retry_count[arvif->tid];
+ }
+
+ if (arvif->tid_conf_changed & TID_AGGR_CONF_CHANGED) {
+ if (arsta->aggr_ctrl[arvif->tid])
+ return;
+
+ arg.aggr_control = arvif->aggr_ctrl[arvif->tid];
+ }
+
+ ret = ath10k_wmi_set_per_peer_per_tid_cfg(ar, &arg);
+ if (ret)
+ ath10k_warn(ar, "failed to set per tid retry/aggr config for sta %pM: %d\n",
+ sta->addr, ret);
+}
+
+static void ath10k_mac_vif_stations_tid_conf(void *data,
+ struct ieee80211_sta *sta)
+{
+ struct ath10k_sta *arsta = (struct ath10k_sta *)sta->drv_priv;
+ struct ath10k_mac_iter_tid_config *iter_data = data;
+ struct ieee80211_vif *sta_vif = arsta->arvif->vif;
+
+ if (sta_vif != iter_data->curr_vif)
+ return;
+
+ ieee80211_queue_work(iter_data->ar->hw, &arsta->tid_cfg_wk);
+}
+
static int ath10k_sta_state(struct ieee80211_hw *hw,
struct ieee80211_vif *vif,
struct ieee80211_sta *sta,
@@ -6372,6 +6458,7 @@ static int ath10k_sta_state(struct ieee80211_hw *hw,
INIT_WORK(&arsta->update_wk, ath10k_sta_rc_update_wk);
INIT_WORK(&arsta->noack_map_wk,
ath10k_sta_set_noack_tid_bitmap);
+ INIT_WORK(&arsta->tid_cfg_wk, ath10k_sta_tid_cfg_wk);
for (i = 0; i < ARRAY_SIZE(sta->txq); i++)
ath10k_mac_txq_init(sta->txq[i]);
@@ -6382,6 +6469,7 @@ static int ath10k_sta_state(struct ieee80211_hw *hw,
new_state == IEEE80211_STA_NOTEXIST)) {
cancel_work_sync(&arsta->update_wk);
cancel_work_sync(&arsta->noack_map_wk);
+ cancel_work_sync(&arsta->tid_cfg_wk);
}
mutex_lock(&ar->conf_mutex);
@@ -7996,7 +8084,7 @@ static int ath10k_mac_op_set_noack_tid_bitmap(struct ieee80211_hw *hw,
int noack_map)
{
struct ath10k_vif *arvif = (void *)vif->drv_priv;
- struct ath10k_mac_iter_noack_map_data data = {};
+ struct ath10k_mac_iter_tid_config data = {};
struct wmi_per_peer_per_tid_cfg_arg arg = {};
struct ath10k *ar = hw->priv;
struct ath10k_sta *arsta;
@@ -8039,6 +8127,98 @@ static int ath10k_mac_op_set_noack_tid_bitmap(struct ieee80211_hw *hw,
return ret;
}
+static int ath10k_mac_op_set_tid_conf(struct ieee80211_hw *hw,
+ struct ieee80211_vif *vif,
+ struct ieee80211_sta *sta,
+ struct ieee80211_tid_conf *tid_conf,
+ u8 changed)
+{
+ int ret;
+ struct ath10k *ar = hw->priv;
+ struct ath10k_vif *arvif = (void *)vif->drv_priv;
+ struct ath10k_mac_iter_tid_config data = {};
+ struct wmi_per_peer_per_tid_cfg_arg arg = {};
+ struct ath10k_sta *arsta;
+
+ if (!(changed & TID_RETRY_CONF_CHANGED) &&
+ !(changed & TID_AGGR_CONF_CHANGED))
+ return 0;
+
+ mutex_lock(&ar->conf_mutex);
+ arg.vdev_id = arvif->vdev_id;
+ arg.tid = tid_conf->tid;
+
+ if (sta) {
+ arsta = (struct ath10k_sta *)sta->drv_priv;
+ ether_addr_copy(arg.peer_macaddr.addr, sta->addr);
+
+ if (changed & TID_RETRY_CONF_CHANGED) {
+ if (tid_conf->retry_long ==
+ arsta->retry_count[arg.tid]) {
+ ret = 0;
+ goto exit;
+ }
+
+ if (tid_conf->retry_long == -1) {
+ if (arvif->retry_count[arg.tid])
+ arg.retry_count =
+ arvif->retry_count[arg.tid];
+ else
+ arg.retry_count =
+ ATH10K_MAX_RETRY_COUNT;
+ } else {
+ arg.retry_count = tid_conf->retry_long;
+ }
+ }
+ if (changed & TID_AGGR_CONF_CHANGED) {
+ if (tid_conf->aggr)
+ arg.aggr_control =
+ WMI_TID_CONFIG_AGGR_CONTROL_ENABLE;
+ else
+ arg.aggr_control =
+ WMI_TID_CONFIG_AGGR_CONTROL_DISABLE;
+ }
+
+ ret = ath10k_wmi_set_per_peer_per_tid_cfg(ar, &arg);
+ if (!ret) {
+ /* Store the configured parameters in success case */
+ if (changed & TID_RETRY_CONF_CHANGED)
+ arsta->retry_count[arg.tid] =
+ tid_conf->retry_long;
+ if (changed & TID_AGGR_CONF_CHANGED)
+ arsta->aggr_ctrl[arg.tid] = arg.aggr_control;
+ }
+
+ goto exit;
+ }
+
+ ret = 0;
+
+ if (changed & TID_RETRY_CONF_CHANGED)
+ arvif->retry_count[tid_conf->tid] = tid_conf->retry_long;
+
+ if (changed & TID_AGGR_CONF_CHANGED) {
+ if (tid_conf->aggr)
+ arvif->aggr_ctrl[tid_conf->tid] =
+ WMI_TID_CONFIG_AGGR_CONTROL_ENABLE;
+ else
+ arvif->aggr_ctrl[tid_conf->tid] =
+ WMI_TID_CONFIG_AGGR_CONTROL_DISABLE;
+ }
+
+ data.curr_vif = vif;
+ data.ar = ar;
+ arvif->tid_conf_changed = changed;
+ arvif->tid = tid_conf->tid;
+ ieee80211_iterate_stations_atomic(hw,
+ ath10k_mac_vif_stations_tid_conf,
+ &data);
+
+exit:
+ mutex_unlock(&ar->conf_mutex);
+ return ret;
+}
+
static const struct ieee80211_ops ath10k_ops = {
.tx = ath10k_mac_op_tx,
.wake_tx_queue = ath10k_mac_op_wake_tx_queue,
@@ -8082,6 +8262,7 @@ static int ath10k_mac_op_set_noack_tid_bitmap(struct ieee80211_hw *hw,
.sta_pre_rcu_remove = ath10k_mac_op_sta_pre_rcu_remove,
.sta_statistics = ath10k_sta_statistics,
.set_noack_tid_bitmap = ath10k_mac_op_set_noack_tid_bitmap,
+ .set_tid_conf = ath10k_mac_op_set_tid_conf,
CFG80211_TESTMODE_CMD(ath10k_tm_cmd)
@@ -8736,11 +8917,24 @@ int ath10k_mac_register(struct ath10k *ar)
wiphy_ext_feature_set(ar->hw->wiphy,
NL80211_EXT_FEATURE_ACK_SIGNAL_SUPPORT);
- if (test_bit(WMI_SERVICE_PEER_TID_CONFIGS_SUPPORT, ar->wmi.svc_map))
+ if (test_bit(WMI_SERVICE_PEER_TID_CONFIGS_SUPPORT, ar->wmi.svc_map)) {
wiphy_ext_feature_set(ar->hw->wiphy,
NL80211_EXT_FEATURE_PER_STA_NOACK_MAP);
- else
+ wiphy_ext_feature_set(ar->hw->wiphy,
+ NL80211_EXT_FEATURE_PER_TID_RETRY_CONFIG);
+ wiphy_ext_feature_set(ar->hw->wiphy,
+ NL80211_EXT_FEATURE_PER_STA_RETRY_CONFIG);
+ wiphy_ext_feature_set(ar->hw->wiphy,
+ NL80211_EXT_FEATURE_PER_TID_AMPDU_AGGR_CTRL);
+ wiphy_ext_feature_set(ar->hw->wiphy,
+ NL80211_EXT_FEATURE_PER_STA_AMPDU_AGGR_CTRL);
+ ar->hw->wiphy->max_data_retry_count = ATH10K_MAX_RETRY_COUNT;
+ ar->hw->wiphy->flags |= WIPHY_FLAG_HAS_MAX_DATA_RETRY_COUNT;
+ } else {
ar->ops->set_noack_tid_bitmap = NULL;
+ ar->ops->set_tid_conf = NULL;
+ ar->hw->wiphy->flags &= ~WIPHY_FLAG_HAS_MAX_DATA_RETRY_COUNT;
+ }
/*
* on LL hardware queues are managed entirely by the FW
diff --git a/drivers/net/wireless/ath/ath10k/wmi.c b/drivers/net/wireless/ath/ath10k/wmi.c
index 14fdc94..5666232 100644
--- a/drivers/net/wireless/ath/ath10k/wmi.c
+++ b/drivers/net/wireless/ath/ath10k/wmi.c
@@ -8764,10 +8764,12 @@ static u32 ath10k_wmi_prepare_peer_qos(u8 uapsd_queues, u8 sp)
cmd->ack_policy = cpu_to_le32(arg->ack_policy);
cmd->aggr_control = cpu_to_le32(arg->aggr_control);
cmd->rate_control = cpu_to_le32(arg->rate_ctrl);
+ cmd->sw_retry_threshold = cpu_to_le32(arg->retry_count);
ath10k_dbg(ar, ATH10K_DBG_WMI,
- "wmi noack tid %d vdev id %d ack_policy %d aggr %u rate %u mac_addr %pM\n",
- arg->tid, arg->vdev_id, arg->ack_policy, arg->aggr_control, arg->rate_ctrl, arg->peer_macaddr.addr);
+ "wmi noack tid %d vdev id %d ack_policy %d aggr %u rate %u retry_count %d mac_addr %pM\n",
+ arg->tid, arg->vdev_id, arg->ack_policy, arg->aggr_control, arg->rate_ctrl, arg->retry_count,
+ arg->peer_macaddr.addr);
return skb;
}
diff --git a/drivers/net/wireless/ath/ath10k/wmi.h b/drivers/net/wireless/ath/ath10k/wmi.h
index d3571b6..0f1826d 100644
--- a/drivers/net/wireless/ath/ath10k/wmi.h
+++ b/drivers/net/wireless/ath/ath10k/wmi.h
@@ -7086,6 +7086,7 @@ struct wmi_per_peer_per_tid_cfg_arg {
enum wmi_noack_tid_conf ack_policy;
enum wmi_tid_aggr_control_conf aggr_control;
enum wmi_tid_rate_ctrl_conf rate_ctrl;
+ u8 retry_count;
};
struct wmi_peer_per_tid_cfg_cmd {
--
1.9.1
On Mon, 2018-10-22 at 23:25 +0530, Tamizh chelvam wrote:
> Add infrastructure for per TID aggregation/retry count configurations
> such as retry count and AMPDU aggregation control(disable/enable).
> In some scenario reducing the number of retry count for a specific data
> traffic can reduce the latency by proceeding with the next packet
> instead of retrying the same packet more time. This will be useful
> where the next packet can resume the operation without an issue.
Not sure I understand this, how can you expect to control something on a
per-packet basis using this?
Sergey, looks like your A-MPDU control is already in here per RA/TID,
and A-MSDU could be added easily?
johannes
Hello Tamizh,
> Co-Developed-by: Tamizh Chelvam <[email protected]>
> Signed-off-by: Vasanthakumar Thiagarajan <[email protected]>
> Signed-off-by: Tamizh chelvam <[email protected]>
> ---
> include/net/cfg80211.h | 14 +++++++
> include/uapi/linux/nl80211.h | 69 +++++++++++++++++++++++++++++++++
> net/wireless/nl80211.c | 86 ++++++++++++++++++++++++++++++++++++++++++
> net/wireless/rdev-ops.h | 15 ++++++++
> net/wireless/trace.h | 27 +++++++++++++
> 5 files changed, 211 insertions(+)
...
> diff --git a/include/net/cfg80211.h b/include/net/cfg80211.h
> index 5801d76..dd024da 100644
> --- a/include/net/cfg80211.h
> +++ b/include/net/cfg80211.h
...
> /**
> @@ -4035,6 +4044,9 @@ struct wiphy_iftype_ext_capab {
> * @txq_limit: configuration of internal TX queue frame limit
> * @txq_memory_limit: configuration internal TX queue memory limit
> * @txq_quantum: configuration of internal TX queue scheduler quantum
> + *
> + * @max_data_retry_count: Maximum limit can be configured as retry count
> + * for a TID.
> */
> struct wiphy {
> /* assign these fields before you register the wiphy */
> @@ -4171,6 +4183,8 @@ struct wiphy {
> u32 txq_memory_limit;
> u32 txq_quantum;
>
> + u8 max_data_retry_count;
> +
> char priv[0] __aligned(NETDEV_ALIGN);
> };
Could you please clarify why do you define max_data_retry_count instead of
making use of existing wiphy params: retry_short (dot11ShortRetryLimit)
and retry_long (dot11LongRetryLimit) ?
> diff --git a/net/wireless/nl80211.c b/net/wireless/nl80211.c
> index d744388..d386ad7 100644
> --- a/net/wireless/nl80211.c
> +++ b/net/wireless/nl80211.c
...
> +static int nl80211_set_tid_config(struct sk_buff *skb,
> + struct genl_info *info)
> +{
> + struct cfg80211_registered_device *rdev = info->user_ptr[0];
> + struct nlattr *attrs[NL80211_ATTR_TID_MAX + 1];
> + struct nlattr *tid;
> + struct net_device *dev = info->user_ptr[1];
> + const char *peer = NULL;
> + u8 tid_no;
> + int ret = -EINVAL, retry_short = -1, retry_long = -1;
> +
> + tid = info->attrs[NL80211_ATTR_TID_CONFIG];
> + if (!tid)
> + return -EINVAL;
> +
> + ret = nla_parse_nested(attrs, NL80211_ATTR_TID_MAX, tid,
> + nl80211_attr_tid_policy, info->extack);
> + if (ret)
> + return ret;
> +
> + if (!attrs[NL80211_ATTR_TID])
> + return -EINVAL;
> +
> + if (attrs[NL80211_ATTR_TID_RETRY_SHORT]) {
> + retry_short = nla_get_u8(attrs[NL80211_ATTR_TID_RETRY_SHORT]);
> + if (!retry_short ||
> + retry_short > rdev->wiphy.max_data_retry_count)
> + return -EINVAL;
> + }
> +
> + if (attrs[NL80211_ATTR_TID_RETRY_LONG]) {
> + retry_long = nla_get_u8(attrs[NL80211_ATTR_TID_RETRY_LONG]);
> + if (!retry_long ||
> + retry_long > rdev->wiphy.max_data_retry_count)
> + return -EINVAL;
> + }
> +
> + tid_no = nla_get_u8(attrs[NL80211_ATTR_TID]);
> + if (tid_no >= IEEE80211_FIRST_TSPEC_TSID)
> + return -EINVAL;
Not that important, but this tid_no check can be placed after
attrs[NL80211_ATTR_TID].
BTW, some special tid_no value (e.g. (u8)-1) could be used to notify driver
that retry settings should be applied for all the TIDs. IIUC the only
required change would be to modify this tid_no sanity check.
Regards,
Sergey
Hello Tamizh,
> Signed-off-by: Tamizh chelvam <[email protected]>
> ---
> include/net/cfg80211.h | 6 ++++++
> include/uapi/linux/nl80211.h | 21 +++++++++++++++++++++
> net/wireless/nl80211.c | 17 +++++++++++++++++
> net/wireless/rdev-ops.h | 15 +++++++++++++++
> net/wireless/trace.h | 23 +++++++++++++++++++++++
> 5 files changed, 82 insertions(+)
...
> diff --git a/include/uapi/linux/nl80211.h b/include/uapi/linux/nl80211.h
> index 9dfcf0a6..7ba0fb7 100644
> --- a/include/uapi/linux/nl80211.h
> +++ b/include/uapi/linux/nl80211.h
> @@ -4449,6 +4449,20 @@ enum nl80211_ps_state {
> * the max value should be advertised by the driver through
> * max_data_retry_count. when this attribute is not present, the driver
> * would use the default configuration.
> + * @NL80211_ATTR_TID_AMPDU_AGGR_CTRL: Enable/Disable aggregation for the TID
> + * specified in %%NL80211_ATTR_TID. Its type is u8, if the peer MAC address
> + * is passed in %NL80211_ATTR_MAC, the aggregation configuration is applied
> + * to the data frame for the tid to that connected station.
> + * Station specific aggregation configuration is valid only for STA's
> + * current connection. i.e. the configuration will be reset to default when
> + * the station connects back after disconnection/roaming.
> + * when user-space does not include %NL80211_ATTR_MAC, this configuration
> + * should be treated as per-netdev configuration. This configuration will
> + * be cleared when the interface goes down and on the disconnection from a
> + * BSS. Driver supporting this feature should advertise
> + * NL80211_EXT_FEATURE_PER_STA_AMPDU_AGGR_CTRL and supporting per station
typo: should be NL80211_EXT_FEATURE_PER_TID_AMPDU_AGGR_CTRL
> + * aggregation configuration should advertise
> + * NL80211_EXT_FEATURE_PER_STA_AMPDU_AGGR_CTRL.
> */
Regards,
Sergey
> Signed-off-by: Tamizh chelvam <[email protected]>
> ---
> include/net/mac80211.h | 40 +++++++++++++++++++++++++
> net/mac80211/cfg.c | 71 +++++++++++++++++++++++++++++++++++++++++++++
> net/mac80211/driver-ops.h | 16 ++++++++++
> net/mac80211/trace.h | 34 ++++++++++++++++++++++
> 4 files changed, 161 insertions(+)
>
> diff --git a/include/net/mac80211.h b/include/net/mac80211.h
> index b6cc3e33..7fa7e25 100644
> --- a/include/net/mac80211.h
> +++ b/include/net/mac80211.h
> @@ -1478,6 +1478,35 @@ struct ieee80211_channel_switch {
> u8 count;
> };
>
> +/*
> + * enum ieee80211_tid_conf_change - TID change configuration notification flags
> + *
> + * These flags are used with the set_tid_conf() callback
> + * to indicate which TID configuration parameter changed.
> + *
> + * @TID_RETRY_CONF_CHANGED: retry configuration changed.
> + * @TID_AGGR_CONF_CHANGED: Aggregation config changed for the TID.
> + */
> +enum ieee80211_tid_conf_change {
> + TID_RETRY_CONF_CHANGED = BIT(0),
> + TID_AGGR_CONF_CHANGED = BIT(1),
> +};
Following your approach, AMSDU support can be added in addition to
AMPDU. So I would suggest to replace AGGR by AMPDU right away.
> +
> +/*
> + * struct ieee80211_tid_conf - holds the tid configiuration data
> + * The information provided in the structure is required for the driver
> + * to configure TID specific configuration.
> + * @tid: TID number
> + * @retry_short: retry count value
> + * @retry_long: retry count value
> + * @aggr: enable/disable aggregation
> + */
> +struct ieee80211_tid_conf {
> + u8 tid;
> + int retry_short;
> + int retry_long;
> + bool aggr;
> +};
ditto: aggr -> ampdu
Regards,
Sergey
> On Mon, 2018-10-22 at 23:25 +0530, Tamizh chelvam wrote:
> > Add infrastructure for per TID aggregation/retry count configurations
> > such as retry count and AMPDU aggregation control(disable/enable).
> > In some scenario reducing the number of retry count for a specific data
> > traffic can reduce the latency by proceeding with the next packet
> > instead of retrying the same packet more time. This will be useful
> > where the next packet can resume the operation without an issue.
>
> Not sure I understand this, how can you expect to control something on a
> per-packet basis using this?
>
> Sergey, looks like your A-MPDU control is already in here per RA/TID,
> and A-MSDU could be added easily?
Hello Johannes,
Thanks for pointing me at this patch series. Indeed, it looks like an
exact match for proper RA/TID aware implementation of AMPDU control.
AMSDU can be added following the same approach.
Regards,
Sergey
Hi,
On Tue, 2018-11-06 at 10:45 +0000, Sergey Matyukevich wrote:
> > On Mon, 2018-10-22 at 23:25 +0530, Tamizh chelvam wrote:
> > > Add infrastructure for per TID aggregation/retry count configurations
> > > such as retry count and AMPDU aggregation control(disable/enable).
> > > In some scenario reducing the number of retry count for a specific data
> > > traffic can reduce the latency by proceeding with the next packet
> > > instead of retrying the same packet more time. This will be useful
> > > where the next packet can resume the operation without an issue.
> >
> > Not sure I understand this, how can you expect to control something on a
> > per-packet basis using this?
> >
> > Sergey, looks like your A-MPDU control is already in here per RA/TID,
> > and A-MSDU could be added easily?
> Thanks for pointing me at this patch series. Indeed, it looks like an
> exact match for proper RA/TID aware implementation of AMPDU control.
> AMSDU can be added following the same approach.
>
Great. I guess if you could take a look that'd be nice, and perhaps also
see if you could actually implement it? Your driver patch seemed to
imply the firmware only has global control, rather than per RA/TID.
Also, do you think A-MPDU length control would be something useful?
Perhaps that should be instead of enable/disable (since setting length
to 0 or 1 could easily mean "no A-MPDU")
johannes
> +static const struct nla_policy
> +nl80211_attr_tid_policy[NL80211_ATTR_TID_MAX + 1] = {
> + [NL80211_ATTR_TID] = { .type = NLA_U8 },
> + [NL80211_ATTR_TID_RETRY_CONFIG] = { .type = NLA_FLAG },
> + [NL80211_ATTR_TID_RETRY_SHORT] = { .type = NLA_U8 },
> + [NL80211_ATTR_TID_RETRY_LONG] = { .type = NLA_U8 },
> +};
> +
> +static int nl80211_set_tid_config(struct sk_buff *skb,
> + struct genl_info *info)
> +{
> + struct cfg80211_registered_device *rdev = info->user_ptr[0];
> + struct nlattr *attrs[NL80211_ATTR_TID_MAX + 1];
> + struct nlattr *tid;
> + struct net_device *dev = info->user_ptr[1];
> + const char *peer = NULL;
> + u8 tid_no;
> + int ret = -EINVAL, retry_short = -1, retry_long = -1;
> +
> + tid = info->attrs[NL80211_ATTR_TID_CONFIG];
> + if (!tid)
> + return -EINVAL;
> +
> + ret = nla_parse_nested(attrs, NL80211_ATTR_TID_MAX, tid,
> + nl80211_attr_tid_policy, info->extack);
> + if (ret)
> + return ret;
> +
> + if (!attrs[NL80211_ATTR_TID])
> + return -EINVAL;
Why not allow configuring multiple at the same time, and make TID_CONFIG
an array of
tid => [config attrs, i.e. retry cfg/short/long]
If not, then you should probably use NLA_POLICY_RANGE() for it.
johannes
On Tue, Nov 06, 2018 at 12:28:11PM +0100, Johannes Berg wrote:
>
> WARNING: External email to Quantenna Communications. Please exercise caution!
>
>
> Hi,
>
> On Tue, 2018-11-06 at 10:45 +0000, Sergey Matyukevich wrote:
> > > On Mon, 2018-10-22 at 23:25 +0530, Tamizh chelvam wrote:
> > > > Add infrastructure for per TID aggregation/retry count configurations
> > > > such as retry count and AMPDU aggregation control(disable/enable).
> > > > In some scenario reducing the number of retry count for a specific data
> > > > traffic can reduce the latency by proceeding with the next packet
> > > > instead of retrying the same packet more time. This will be useful
> > > > where the next packet can resume the operation without an issue.
> > >
> > > Not sure I understand this, how can you expect to control something on a
> > > per-packet basis using this?
> > >
> > > Sergey, looks like your A-MPDU control is already in here per RA/TID,
> > > and A-MSDU could be added easily?
>
> > Thanks for pointing me at this patch series. Indeed, it looks like an
> > exact match for proper RA/TID aware implementation of AMPDU control.
> > AMSDU can be added following the same approach.
> >
>
> Great. I guess if you could take a look that'd be nice, and perhaps also
> see if you could actually implement it? Your driver patch seemed to
> imply the firmware only has global control, rather than per RA/TID.
>
> Also, do you think A-MPDU length control would be something useful?
> Perhaps that should be instead of enable/disable (since setting length
> to 0 or 1 could easily mean "no A-MPDU")
Hello Johannes,
I will send a follow-up A-MSDU patch after this patch set lands in your tree.
And then qtnfmac driver patches for both A-MPDU/A-MSDU changes.
As for A-MPDU chain length control, I don't think we have any practical
use-case for this feature at the moment. However Ben Greear suggested
one possible use-case: to decrease A-MPDU chain length for voice TID
in order to decrease latency. Anyways, current A-MPDU patches can be
easily adapted to support it if needed.
Regards,
Sergey
Hello Tamizh,
> Co-Developed-by: Tamizh Chelvam <[email protected]>
> Signed-off-by: Vasanthakumar Thiagarajan <[email protected]>
> Signed-off-by: Tamizh chelvam <[email protected]>
> ---
> include/net/cfg80211.h | 14 +++++++
> include/uapi/linux/nl80211.h | 69 +++++++++++++++++++++++++++++++++
> net/wireless/nl80211.c | 86 ++++++++++++++++++++++++++++++++++++++++++
> net/wireless/rdev-ops.h | 15 ++++++++
> net/wireless/trace.h | 27 +++++++++++++
> 5 files changed, 211 insertions(+)
...
> diff --git a/net/wireless/nl80211.c b/net/wireless/nl80211.c
> index d744388..d386ad7 100644
> --- a/net/wireless/nl80211.c
> +++ b/net/wireless/nl80211.c
...
> +static int nl80211_set_tid_config(struct sk_buff *skb,
> + struct genl_info *info)
> +{
> + struct cfg80211_registered_device *rdev = info->user_ptr[0];
> + struct nlattr *attrs[NL80211_ATTR_TID_MAX + 1];
> + struct nlattr *tid;
> + struct net_device *dev = info->user_ptr[1];
> + const char *peer = NULL;
> + u8 tid_no;
> + int ret = -EINVAL, retry_short = -1, retry_long = -1;
> +
> + tid = info->attrs[NL80211_ATTR_TID_CONFIG];
> + if (!tid)
> + return -EINVAL;
> +
> + ret = nla_parse_nested(attrs, NL80211_ATTR_TID_MAX, tid,
> + nl80211_attr_tid_policy, info->extack);
> + if (ret)
> + return ret;
> +
> + if (!attrs[NL80211_ATTR_TID])
> + return -EINVAL;
> +
> + if (attrs[NL80211_ATTR_TID_RETRY_SHORT]) {
> + retry_short = nla_get_u8(attrs[NL80211_ATTR_TID_RETRY_SHORT]);
> + if (!retry_short ||
> + retry_short > rdev->wiphy.max_data_retry_count)
> + return -EINVAL;
> + }
> +
> + if (attrs[NL80211_ATTR_TID_RETRY_LONG]) {
> + retry_long = nla_get_u8(attrs[NL80211_ATTR_TID_RETRY_LONG]);
> + if (!retry_long ||
> + retry_long > rdev->wiphy.max_data_retry_count)
> + return -EINVAL;
> + }
> +
> + tid_no = nla_get_u8(attrs[NL80211_ATTR_TID]);
> + if (tid_no >= IEEE80211_FIRST_TSPEC_TSID)
> + return -EINVAL;
> +
> + if (info->attrs[NL80211_ATTR_MAC])
> + peer = nla_data(info->attrs[NL80211_ATTR_MAC]);
> +
> + if (nla_get_flag(attrs[NL80211_ATTR_TID_RETRY_CONFIG])) {
Do we really need this additional flag to indicate retry data ?
Maybe we can simply check retry attrs or even retry data, e.g.:
if (attrs[NL80211_ATTR_TID_RETRY_LONG] ||
attrs[NL80211_ATTR_TID_RETRY_SHORT]) {
...
if ((retry_short > 0) || (retry_long > 0)) {
...
> + if (!wiphy_ext_feature_isset(
> + &rdev->wiphy,
> + NL80211_EXT_FEATURE_PER_TID_RETRY_CONFIG))
> + return -EOPNOTSUPP;
> +
> + if (peer && !wiphy_ext_feature_isset(
> + &rdev->wiphy,
> + NL80211_EXT_FEATURE_PER_STA_RETRY_CONFIG))
> + return -EOPNOTSUPP;
> +
> + if (!rdev->ops->set_data_retry_count ||
> + !rdev->wiphy.max_data_retry_count)
> + return -EOPNOTSUPP;
> +
> + ret = rdev_set_data_retry_count(rdev, dev, peer, tid_no,
> + retry_short, retry_long);
> + }
> +
> + return ret;
> +}
Regards,
Sergey
On 10/22/2018 10:55 AM, Tamizh chelvam wrote:
> /**
> * enum ieee80211_vif_flags - virtual interface flags
> *
> @@ -1565,6 +1594,8 @@ struct ieee80211_vif {
>
> bool txqs_stopped[IEEE80211_NUM_ACS];
>
> + struct ieee80211_tid_conf tid_conf;
> +
Why is there only one TID config? If ieee80211_vif::tid_conf member is
used only to pass data to driver through drv_set_tid_conf(), and driver
will save it for a specific TID, why do we need to save it in struct
ieee80211_vif too?
On 10/22/2018 10:55 AM, Tamizh chelvam wrote:
>
> Add infrastructure for per TID aggregation/retry count configurations
> such as retry count and AMPDU aggregation control(disable/enable).
> In some scenario reducing the number of retry count for a specific data
> traffic can reduce the latency by proceeding with the next packet
> instead of retrying the same packet more time. This will be useful
> where the next packet can resume the operation without an issue.
> Here added NL80211_CMD_SET_TID_CONFIG to support this operation by
> accepting retry count and AMPDU aggregation control.
> This command can accept STA mac addreess to make the configuration
> station specific rather than applying to all the connected stations
> to the netdev.
>
It's not immediately clear how to make use of these settings, here are
several comments:
1. What max retry count limit should actually be applied to? Retries
decisions are in a rate adaptation domain, it should know how many
retries should be done on each rate, single "max retry" value will not
suffice. For example, it can retry twice on MCS9, twice on MCS7, three
times on MCS5 or something like that.
I'm not familiar with what ATH10k is doing, 4th patch defines
ATH10K_MAX_RETRY_COUNT=30, what does it actually mean? It's unlikely "do
30 retries on the same rate". Does retry limit setting interacts with
rate adaptation somehow in ath10k?
Maybe it makes sense to extend max retry value specification to make it
possible to define per-rate? I'm not sure how much flexibility we want
here, something like retry value per MCS:BW:SGI?
2. AMPDU/AMSDU - the way it is, it is also relevant to rate in Tx
direction only, correct? We keep advertised capabilities intact and peer
has all rights to send AMPDUs/AMSDUs of whatever size that was advertised.
Additionally, posted patches do not do anything with established
blockack agreement.
3 With above being said, perhaps it would make sense for this new
interface to indicate explicitly that it's related to Tx rate? That can
be controlled per-TID, per-node or globally, depending on device
capabilities.
Some other settings that may be useful are fixed MCS, MCS limit, SGI
on/off, bandwidth, maybe even provide rate retry rules.
I don't see how it can be used in real product, unless there is an
external rate adaptation logic of some kind. But definitely it will be
useful for testing, and can be used for WFA certification.
Hi Sergey,
> Hello Tamizh,
>
>> Signed-off-by: Tamizh chelvam <[email protected]>
>> ---
>> include/net/cfg80211.h | 6 ++++++
>> include/uapi/linux/nl80211.h | 21 +++++++++++++++++++++
>> net/wireless/nl80211.c | 17 +++++++++++++++++
>> net/wireless/rdev-ops.h | 15 +++++++++++++++
>> net/wireless/trace.h | 23 +++++++++++++++++++++++
>> 5 files changed, 82 insertions(+)
> ...
>
>> diff --git a/include/uapi/linux/nl80211.h
>> b/include/uapi/linux/nl80211.h
>> index 9dfcf0a6..7ba0fb7 100644
>> --- a/include/uapi/linux/nl80211.h
>> +++ b/include/uapi/linux/nl80211.h
>> @@ -4449,6 +4449,20 @@ enum nl80211_ps_state {
>> * the max value should be advertised by the driver through
>> * max_data_retry_count. when this attribute is not present, the
>> driver
>> * would use the default configuration.
>> + * @NL80211_ATTR_TID_AMPDU_AGGR_CTRL: Enable/Disable aggregation for
>> the TID
>> + * specified in %%NL80211_ATTR_TID. Its type is u8, if the peer
>> MAC address
>> + * is passed in %NL80211_ATTR_MAC, the aggregation configuration
>> is applied
>> + * to the data frame for the tid to that connected station.
>> + * Station specific aggregation configuration is valid only for
>> STA's
>> + * current connection. i.e. the configuration will be reset to
>> default when
>> + * the station connects back after disconnection/roaming.
>> + * when user-space does not include %NL80211_ATTR_MAC, this
>> configuration
>> + * should be treated as per-netdev configuration. This
>> configuration will
>> + * be cleared when the interface goes down and on the
>> disconnection from a
>> + * BSS. Driver supporting this feature should advertise
>> + * NL80211_EXT_FEATURE_PER_STA_AMPDU_AGGR_CTRL and supporting per
>> station
>
> typo: should be NL80211_EXT_FEATURE_PER_TID_AMPDU_AGGR_CTRL
Sure, I'll update in the next version.
>
>> + * aggregation configuration should advertise
>> + * NL80211_EXT_FEATURE_PER_STA_AMPDU_AGGR_CTRL.
>> */
>
> Regards,
> Sergey
Thanks,
Tamizh.
On 2018-11-06 16:03, Sergey Matyukevich wrote:
>> Signed-off-by: Tamizh chelvam <[email protected]>
>> ---
>> include/net/mac80211.h | 40 +++++++++++++++++++++++++
>> net/mac80211/cfg.c | 71
>> +++++++++++++++++++++++++++++++++++++++++++++
>> net/mac80211/driver-ops.h | 16 ++++++++++
>> net/mac80211/trace.h | 34 ++++++++++++++++++++++
>> 4 files changed, 161 insertions(+)
>>
>> diff --git a/include/net/mac80211.h b/include/net/mac80211.h
>> index b6cc3e33..7fa7e25 100644
>> --- a/include/net/mac80211.h
>> +++ b/include/net/mac80211.h
>> @@ -1478,6 +1478,35 @@ struct ieee80211_channel_switch {
>> u8 count;
>> };
>>
>> +/*
>> + * enum ieee80211_tid_conf_change - TID change configuration
>> notification flags
>> + *
>> + * These flags are used with the set_tid_conf() callback
>> + * to indicate which TID configuration parameter changed.
>> + *
>> + * @TID_RETRY_CONF_CHANGED: retry configuration changed.
>> + * @TID_AGGR_CONF_CHANGED: Aggregation config changed for the TID.
>> + */
>> +enum ieee80211_tid_conf_change {
>> + TID_RETRY_CONF_CHANGED = BIT(0),
>> + TID_AGGR_CONF_CHANGED = BIT(1),
>> +};
>
> Following your approach, AMSDU support can be added in addition to
> AMPDU. So I would suggest to replace AGGR by AMPDU right away.
>
>> +
>> +/*
>> + * struct ieee80211_tid_conf - holds the tid configiuration data
>> + * The information provided in the structure is required for the
>> driver
>> + * to configure TID specific configuration.
>> + * @tid: TID number
>> + * @retry_short: retry count value
>> + * @retry_long: retry count value
>> + * @aggr: enable/disable aggregation
>> + */
>> +struct ieee80211_tid_conf {
>> + u8 tid;
>> + int retry_short;
>> + int retry_long;
>> + bool aggr;
>> +};
>
> ditto: aggr -> ampdu
>
Sure. I'll update in the next version.
Thanks,
Tamizh.
On 11/07/2018 04:55 PM, Igor Mitsyanko wrote:
> On 10/22/2018 10:55 AM, Tamizh chelvam wrote:
>>
>> Add infrastructure for per TID aggregation/retry count configurations
>> such as retry count and AMPDU aggregation control(disable/enable).
>> In some scenario reducing the number of retry count for a specific data
>> traffic can reduce the latency by proceeding with the next packet
>> instead of retrying the same packet more time. This will be useful
>> where the next packet can resume the operation without an issue.
>> Here added NL80211_CMD_SET_TID_CONFIG to support this operation by
>> accepting retry count and AMPDU aggregation control.
>> This command can accept STA mac addreess to make the configuration
>> station specific rather than applying to all the connected stations
>> to the netdev.
>>
>
> It's not immediately clear how to make use of these settings, here are
> several comments:
>
> 1. What max retry count limit should actually be applied to? Retries
> decisions are in a rate adaptation domain, it should know how many
> retries should be done on each rate, single "max retry" value will not
> suffice. For example, it can retry twice on MCS9, twice on MCS7, three
> times on MCS5 or something like that.
>
> I'm not familiar with what ATH10k is doing, 4th patch defines
> ATH10K_MAX_RETRY_COUNT=30, what does it actually mean? It's unlikely "do
> 30 retries on the same rate". Does retry limit setting interacts with
> rate adaptation somehow in ath10k?
>
> Maybe it makes sense to extend max retry value specification to make it
> possible to define per-rate? I'm not sure how much flexibility we want
> here, something like retry value per MCS:BW:SGI?
For ath10k, my understanding is that each time it (re)sends a packet, it will
query FW rate-ctrl and choose the optimal rate. It doesn't pay much attention to
whether a specific frame is retried or not, other than to maybe enable RTS/CTS,
but lots of retries will bump the rate-ctrl down to a lower rate.
There are no per-rate retry counter logic, but I think there is per-tid
control, though currently it might not be wired up to the driver.
>
> 2. AMPDU/AMSDU - the way it is, it is also relevant to rate in Tx
> direction only, correct? We keep advertised capabilities intact and peer
> has all rights to send AMPDUs/AMSDUs of whatever size that was advertised.
> Additionally, posted patches do not do anything with established
> blockack agreement.
>
> 3 With above being said, perhaps it would make sense for this new
> interface to indicate explicitly that it's related to Tx rate? That can
> be controlled per-TID, per-node or globally, depending on device
> capabilities.
> Some other settings that may be useful are fixed MCS, MCS limit, SGI
> on/off, bandwidth, maybe even provide rate retry rules.
I think there should be a way to configure the advertised capabilities, and also a way to
configure the settings actually used for transmit. This is what we use
for test-related use cases, but maybe there is not a great deal of general
use for this type of thing. For general use, the 'transmit' settings are probably
more useful. I do know that several ath10k users are forcing it back to /n
mode which works around some bugs in their mesh setup.
You can already set a fixed transmit rate or set the MCS rates allowed to be used
(my supplicant, ath10k-ct driver/firmware is needed to take full advantage of this
for ath10k). In upstream kernels, this will not much affect the advertised capabilities.
I also have patches that allow setting the advertised rates and capabilities, so you can force
a station to advertise only a/n rates even though it and peer have /AC capability.
Those patches are not upstream, though if opinions are changed, I'd be happy
to repost and try to get them upstream.
Thanks,
Ben
> I don't see how it can be used in real product, unless there is an
> external rate adaptation logic of some kind. But definitely it will be
> useful for testing, and can be used for WFA certification.
>
--
Ben Greear <[email protected]>
Candela Technologies Inc http://www.candelatech.com
On 2018-11-06 15:46, Sergey Matyukevich wrote:
> Hello Tamizh,
>
>> Co-Developed-by: Tamizh Chelvam <[email protected]>
>> Signed-off-by: Vasanthakumar Thiagarajan <[email protected]>
>> Signed-off-by: Tamizh chelvam <[email protected]>
>> ---
>> include/net/cfg80211.h | 14 +++++++
>> include/uapi/linux/nl80211.h | 69 +++++++++++++++++++++++++++++++++
>> net/wireless/nl80211.c | 86
>> ++++++++++++++++++++++++++++++++++++++++++
>> net/wireless/rdev-ops.h | 15 ++++++++
>> net/wireless/trace.h | 27 +++++++++++++
>> 5 files changed, 211 insertions(+)
>
> ...
>
>> diff --git a/include/net/cfg80211.h b/include/net/cfg80211.h
>> index 5801d76..dd024da 100644
>> --- a/include/net/cfg80211.h
>> +++ b/include/net/cfg80211.h
>
> ...
>
>> /**
>> @@ -4035,6 +4044,9 @@ struct wiphy_iftype_ext_capab {
>> * @txq_limit: configuration of internal TX queue frame limit
>> * @txq_memory_limit: configuration internal TX queue memory limit
>> * @txq_quantum: configuration of internal TX queue scheduler quantum
>> + *
>> + * @max_data_retry_count: Maximum limit can be configured as retry
>> count
>> + * for a TID.
>> */
>> struct wiphy {
>> /* assign these fields before you register the wiphy */
>> @@ -4171,6 +4183,8 @@ struct wiphy {
>> u32 txq_memory_limit;
>> u32 txq_quantum;
>>
>> + u8 max_data_retry_count;
>> +
>> char priv[0] __aligned(NETDEV_ALIGN);
>> };
>
> Could you please clarify why do you define max_data_retry_count instead
> of
> making use of existing wiphy params: retry_short (dot11ShortRetryLimit)
> and retry_long (dot11LongRetryLimit) ?
max_data_retry_count added to validate the max limit for the retry count
supported by the driver.
existing wiphy parames: retry_short and retry_long can be modified
through user command.
So, I've added this param for validation purpose. Correct me If I'm
wrong.
>
>> diff --git a/net/wireless/nl80211.c b/net/wireless/nl80211.c
>> index d744388..d386ad7 100644
>> --- a/net/wireless/nl80211.c
>> +++ b/net/wireless/nl80211.c
>
> ...
>
>> +static int nl80211_set_tid_config(struct sk_buff *skb,
>> + struct genl_info *info)
>> +{
>> + struct cfg80211_registered_device *rdev = info->user_ptr[0];
>> + struct nlattr *attrs[NL80211_ATTR_TID_MAX + 1];
>> + struct nlattr *tid;
>> + struct net_device *dev = info->user_ptr[1];
>> + const char *peer = NULL;
>> + u8 tid_no;
>> + int ret = -EINVAL, retry_short = -1, retry_long = -1;
>> +
>> + tid = info->attrs[NL80211_ATTR_TID_CONFIG];
>> + if (!tid)
>> + return -EINVAL;
>> +
>> + ret = nla_parse_nested(attrs, NL80211_ATTR_TID_MAX, tid,
>> + nl80211_attr_tid_policy, info->extack);
>> + if (ret)
>> + return ret;
>> +
>> + if (!attrs[NL80211_ATTR_TID])
>> + return -EINVAL;
>> +
>> + if (attrs[NL80211_ATTR_TID_RETRY_SHORT]) {
>> + retry_short =
>> nla_get_u8(attrs[NL80211_ATTR_TID_RETRY_SHORT]);
>> + if (!retry_short ||
>> + retry_short > rdev->wiphy.max_data_retry_count)
>> + return -EINVAL;
>> + }
>> +
>> + if (attrs[NL80211_ATTR_TID_RETRY_LONG]) {
>> + retry_long =
>> nla_get_u8(attrs[NL80211_ATTR_TID_RETRY_LONG]);
>> + if (!retry_long ||
>> + retry_long > rdev->wiphy.max_data_retry_count)
>> + return -EINVAL;
>> + }
>> +
>> + tid_no = nla_get_u8(attrs[NL80211_ATTR_TID]);
>> + if (tid_no >= IEEE80211_FIRST_TSPEC_TSID)
>> + return -EINVAL;
>
> Not that important, but this tid_no check can be placed after
> attrs[NL80211_ATTR_TID].
>
> BTW, some special tid_no value (e.g. (u8)-1) could be used to notify
> driver
> that retry settings should be applied for all the TIDs. IIUC the only
> required change would be to modify this tid_no sanity check.
Sure, we can use that.
>
> Regards,
> Sergey
Thanks,
Tamizh.
> Hello Tamizh,
>
>> Co-Developed-by: Tamizh Chelvam <[email protected]>
>> Signed-off-by: Vasanthakumar Thiagarajan <[email protected]>
>> Signed-off-by: Tamizh chelvam <[email protected]>
>> ---
>> include/net/cfg80211.h | 14 +++++++
>> include/uapi/linux/nl80211.h | 69 +++++++++++++++++++++++++++++++++
>> net/wireless/nl80211.c | 86
>> ++++++++++++++++++++++++++++++++++++++++++
>> net/wireless/rdev-ops.h | 15 ++++++++
>> net/wireless/trace.h | 27 +++++++++++++
>> 5 files changed, 211 insertions(+)
>
> ...
>
>> diff --git a/net/wireless/nl80211.c b/net/wireless/nl80211.c
>> index d744388..d386ad7 100644
>> --- a/net/wireless/nl80211.c
>> +++ b/net/wireless/nl80211.c
>
> ...
>
>> +static int nl80211_set_tid_config(struct sk_buff *skb,
>> + struct genl_info *info)
>> +{
>> + struct cfg80211_registered_device *rdev = info->user_ptr[0];
>> + struct nlattr *attrs[NL80211_ATTR_TID_MAX + 1];
>> + struct nlattr *tid;
>> + struct net_device *dev = info->user_ptr[1];
>> + const char *peer = NULL;
>> + u8 tid_no;
>> + int ret = -EINVAL, retry_short = -1, retry_long = -1;
>> +
>> + tid = info->attrs[NL80211_ATTR_TID_CONFIG];
>> + if (!tid)
>> + return -EINVAL;
>> +
>> + ret = nla_parse_nested(attrs, NL80211_ATTR_TID_MAX, tid,
>> + nl80211_attr_tid_policy, info->extack);
>> + if (ret)
>> + return ret;
>> +
>> + if (!attrs[NL80211_ATTR_TID])
>> + return -EINVAL;
>> +
>> + if (attrs[NL80211_ATTR_TID_RETRY_SHORT]) {
>> + retry_short =
>> nla_get_u8(attrs[NL80211_ATTR_TID_RETRY_SHORT]);
>> + if (!retry_short ||
>> + retry_short > rdev->wiphy.max_data_retry_count)
>> + return -EINVAL;
>> + }
>> +
>> + if (attrs[NL80211_ATTR_TID_RETRY_LONG]) {
>> + retry_long =
>> nla_get_u8(attrs[NL80211_ATTR_TID_RETRY_LONG]);
>> + if (!retry_long ||
>> + retry_long > rdev->wiphy.max_data_retry_count)
>> + return -EINVAL;
>> + }
>> +
>> + tid_no = nla_get_u8(attrs[NL80211_ATTR_TID]);
>> + if (tid_no >= IEEE80211_FIRST_TSPEC_TSID)
>> + return -EINVAL;
>> +
>> + if (info->attrs[NL80211_ATTR_MAC])
>> + peer = nla_data(info->attrs[NL80211_ATTR_MAC]);
>> +
>> + if (nla_get_flag(attrs[NL80211_ATTR_TID_RETRY_CONFIG])) {
>
> Do we really need this additional flag to indicate retry data ?
> Maybe we can simply check retry attrs or even retry data, e.g.:
Yes, because this implementation gives provision to set default retry
count for TID traffic type for a station.
Since we use single NL command for all TID configurations, this flag
will be useful to notify the driver about
retry TID configuration change.
>
> if (attrs[NL80211_ATTR_TID_RETRY_LONG] ||
> attrs[NL80211_ATTR_TID_RETRY_SHORT]) {
> ...
>
> if ((retry_short > 0) || (retry_long > 0)) {
> ...
>
>> + if (!wiphy_ext_feature_isset(
>> + &rdev->wiphy,
>> +
>> NL80211_EXT_FEATURE_PER_TID_RETRY_CONFIG))
>> + return -EOPNOTSUPP;
>> +
>> + if (peer && !wiphy_ext_feature_isset(
>> + &rdev->wiphy,
>> +
>> NL80211_EXT_FEATURE_PER_STA_RETRY_CONFIG))
>> + return -EOPNOTSUPP;
>> +
>> + if (!rdev->ops->set_data_retry_count ||
>> + !rdev->wiphy.max_data_retry_count)
>> + return -EOPNOTSUPP;
>> +
>> + ret = rdev_set_data_retry_count(rdev, dev, peer,
>> tid_no,
>> + retry_short,
>> retry_long);
>> + }
>> +
>> + return ret;
>> +}
>
> Regards,
> Sergey
Thanks,
Tamizh.
Hello Tamizh,
> Signed-off-by: Tamizh chelvam <[email protected]>
> ---
> drivers/net/wireless/ath/ath10k/core.h | 23 ++++
> drivers/net/wireless/ath/ath10k/mac.c | 240 +++++++++++++++++++++++++++++----
> drivers/net/wireless/ath/ath10k/wmi.c | 6 +-
> drivers/net/wireless/ath/ath10k/wmi.h | 1 +
> 4 files changed, 245 insertions(+), 25 deletions(-)
...
> +static int ath10k_mac_op_set_tid_conf(struct ieee80211_hw *hw,
> + struct ieee80211_vif *vif,
> + struct ieee80211_sta *sta,
> + struct ieee80211_tid_conf *tid_conf,
> + u8 changed)
> +{
> + int ret;
> + struct ath10k *ar = hw->priv;
> + struct ath10k_vif *arvif = (void *)vif->drv_priv;
> + struct ath10k_mac_iter_tid_config data = {};
> + struct wmi_per_peer_per_tid_cfg_arg arg = {};
> + struct ath10k_sta *arsta;
> +
> + if (!(changed & TID_RETRY_CONF_CHANGED) &&
> + !(changed & TID_AGGR_CONF_CHANGED))
> + return 0;
> +
> + mutex_lock(&ar->conf_mutex);
> + arg.vdev_id = arvif->vdev_id;
> + arg.tid = tid_conf->tid;
> +
> + if (sta) {
> + arsta = (struct ath10k_sta *)sta->drv_priv;
> + ether_addr_copy(arg.peer_macaddr.addr, sta->addr);
> +
> + if (changed & TID_RETRY_CONF_CHANGED) {
> + if (tid_conf->retry_long ==
> + arsta->retry_count[arg.tid]) {
> + ret = 0;
> + goto exit;
> + }
> +
> + if (tid_conf->retry_long == -1) {
> + if (arvif->retry_count[arg.tid])
> + arg.retry_count =
> + arvif->retry_count[arg.tid];
> + else
> + arg.retry_count =
> + ATH10K_MAX_RETRY_COUNT;
> + } else {
> + arg.retry_count = tid_conf->retry_long;
> + }
> + }
> + if (changed & TID_AGGR_CONF_CHANGED) {
> + if (tid_conf->aggr)
> + arg.aggr_control =
> + WMI_TID_CONFIG_AGGR_CONTROL_ENABLE;
> + else
> + arg.aggr_control =
> + WMI_TID_CONFIG_AGGR_CONTROL_DISABLE;
> + }
> +
> + ret = ath10k_wmi_set_per_peer_per_tid_cfg(ar, &arg);
> + if (!ret) {
> + /* Store the configured parameters in success case */
> + if (changed & TID_RETRY_CONF_CHANGED)
> + arsta->retry_count[arg.tid] =
> + tid_conf->retry_long;
> + if (changed & TID_AGGR_CONF_CHANGED)
> + arsta->aggr_ctrl[arg.tid] = arg.aggr_control;
> + }
> +
> + goto exit;
> + }
> +
> + ret = 0;
> +
> + if (changed & TID_RETRY_CONF_CHANGED)
> + arvif->retry_count[tid_conf->tid] = tid_conf->retry_long;
Shouldn't it use default ATH10K_MAX_RETRY_COUNT value when
incoming retry_long value is -1 ?
Regards,
Sergey
Hello Tamizh,
...
> > > +static int nl80211_set_tid_config(struct sk_buff *skb,
> > > + struct genl_info *info)
> > > +{
> > > + struct cfg80211_registered_device *rdev = info->user_ptr[0];
> > > + struct nlattr *attrs[NL80211_ATTR_TID_MAX + 1];
> > > + struct nlattr *tid;
> > > + struct net_device *dev = info->user_ptr[1];
> > > + const char *peer = NULL;
> > > + u8 tid_no;
> > > + int ret = -EINVAL, retry_short = -1, retry_long = -1;
> > > +
> > > + tid = info->attrs[NL80211_ATTR_TID_CONFIG];
> > > + if (!tid)
> > > + return -EINVAL;
> > > +
> > > + ret = nla_parse_nested(attrs, NL80211_ATTR_TID_MAX, tid,
> > > + nl80211_attr_tid_policy, info->extack);
> > > + if (ret)
> > > + return ret;
> > > +
> > > + if (!attrs[NL80211_ATTR_TID])
> > > + return -EINVAL;
> > > +
> > > + if (attrs[NL80211_ATTR_TID_RETRY_SHORT]) {
> > > + retry_short =
> > > nla_get_u8(attrs[NL80211_ATTR_TID_RETRY_SHORT]);
> > > + if (!retry_short ||
> > > + retry_short > rdev->wiphy.max_data_retry_count)
> > > + return -EINVAL;
> > > + }
> > > +
> > > + if (attrs[NL80211_ATTR_TID_RETRY_LONG]) {
> > > + retry_long =
> > > nla_get_u8(attrs[NL80211_ATTR_TID_RETRY_LONG]);
> > > + if (!retry_long ||
> > > + retry_long > rdev->wiphy.max_data_retry_count)
> > > + return -EINVAL;
> > > + }
> > > +
> > > + tid_no = nla_get_u8(attrs[NL80211_ATTR_TID]);
> > > + if (tid_no >= IEEE80211_FIRST_TSPEC_TSID)
> > > + return -EINVAL;
> > > +
> > > + if (info->attrs[NL80211_ATTR_MAC])
> > > + peer = nla_data(info->attrs[NL80211_ATTR_MAC]);
> > > +
> > > + if (nla_get_flag(attrs[NL80211_ATTR_TID_RETRY_CONFIG])) {
> >
> > Do we really need this additional flag to indicate retry data ?
> > Maybe we can simply check retry attrs or even retry data, e.g.:
>
> Yes, because this implementation gives provision to set default retry
> count for TID traffic type for a station.
> Since we use single NL command for all TID configurations, this flag
> will be useful to notify the driver about
> retry TID configuration change.
Ok. So if driver receives retry value (-1), it should reset to some
default value known to driver or firmware. IMHO it worth making it
more explicit: in its current form this convention will not be obvious
for driver authors. Though I don't have a good idea how to do it.
Maybe merge both aggregation and retry cfg80211 callbacks into one
and use structure for params and bitmask for operations...
> >
> > if (attrs[NL80211_ATTR_TID_RETRY_LONG] ||
> > attrs[NL80211_ATTR_TID_RETRY_SHORT]) {
> > ...
> >
> > if ((retry_short > 0) || (retry_long > 0)) {
> > ...
Regards,
Sergey
Hello Tamizh,
> > > Co-Developed-by: Tamizh Chelvam <[email protected]>
> > > Signed-off-by: Vasanthakumar Thiagarajan <[email protected]>
> > > Signed-off-by: Tamizh chelvam <[email protected]>
> > > ---
> > > include/net/cfg80211.h | 14 +++++++
> > > include/uapi/linux/nl80211.h | 69 +++++++++++++++++++++++++++++++++
> > > net/wireless/nl80211.c | 86
> > > ++++++++++++++++++++++++++++++++++++++++++
> > > net/wireless/rdev-ops.h | 15 ++++++++
> > > net/wireless/trace.h | 27 +++++++++++++
> > > 5 files changed, 211 insertions(+)
...
> > > /**
> > > @@ -4035,6 +4044,9 @@ struct wiphy_iftype_ext_capab {
> > > * @txq_limit: configuration of internal TX queue frame limit
> > > * @txq_memory_limit: configuration internal TX queue memory limit
> > > * @txq_quantum: configuration of internal TX queue scheduler quantum
> > > + *
> > > + * @max_data_retry_count: Maximum limit can be configured as retry
> > > count
> > > + * for a TID.
> > > */
> > > struct wiphy {
> > > /* assign these fields before you register the wiphy */
> > > @@ -4171,6 +4183,8 @@ struct wiphy {
> > > u32 txq_memory_limit;
> > > u32 txq_quantum;
> > >
> > > + u8 max_data_retry_count;
> > > +
> > > char priv[0] __aligned(NETDEV_ALIGN);
> > > };
> >
> > Could you please clarify why do you define max_data_retry_count instead
> > of
> > making use of existing wiphy params: retry_short (dot11ShortRetryLimit)
> > and retry_long (dot11LongRetryLimit) ?
>
> max_data_retry_count added to validate the max limit for the retry count
> supported by the driver.
> existing wiphy parames: retry_short and retry_long can be modified
> through user command.
> So, I've added this param for validation purpose. Correct me If I'm
> wrong.
Well, but then it probably makes sense to use max_data_retry_count value
to validate retry_short and retry_long values in nl80211_set_wiphy.
Regards,
Sergey
On Mon, 2018-10-22 at 23:25 +0530, Tamizh chelvam wrote:
>
> +/*
> + * @NL80211_ATTR_TID: a TID value (u8 attribute)
This comment should have the kernel-doc boilerplate
Also, the names should be NL80211_TID_ATTR_* to disambiguate the
namespace from the top-level NL80211_ATTR_*.
> +static const struct nla_policy
> +nl80211_attr_tid_policy[NL80211_ATTR_TID_MAX + 1] = {
> + [NL80211_ATTR_TID] = { .type = NLA_U8 },
> + [NL80211_ATTR_TID_RETRY_CONFIG] = { .type = NLA_FLAG },
> + [NL80211_ATTR_TID_RETRY_SHORT] = { .type = NLA_U8 },
> + [NL80211_ATTR_TID_RETRY_LONG] = { .type = NLA_U8 },
> +};
Like I said elsewhere, I'm not sure we really should support only
setting one at a time?
OTOH, it may not matter so much, and be convenient to actually get
"atomic" behaviour, otherwise you have to iterate & check and then
iterate & apply again. So perhaps it /is/ better this way.
Also, please use NLA_POLICY_NESTED(). This ensures the attributes are
*always* well-formed, even if they end up ignored.
> + ret = nla_parse_nested(attrs, NL80211_ATTR_TID_MAX, tid,
> + nl80211_attr_tid_policy, info->extack);
And then you also no longer need to pass a policy/extack here.
> + if (ret)
> + return ret;
> +
> + if (!attrs[NL80211_ATTR_TID])
> + return -EINVAL;
> +
> + if (attrs[NL80211_ATTR_TID_RETRY_SHORT]) {
> + retry_short = nla_get_u8(attrs[NL80211_ATTR_TID_RETRY_SHORT]);
> + if (!retry_short ||
> + retry_short > rdev->wiphy.max_data_retry_count)
> + return -EINVAL;
> + }
It would also be good to annotate the errors with appropriate extack
error message, i.e. use NL_SET_ERR_MSG_ATTR().
> + if (nla_get_flag(attrs[NL80211_ATTR_TID_RETRY_CONFIG])) {
> + if (!wiphy_ext_feature_isset(
> + &rdev->wiphy,
> + NL80211_EXT_FEATURE_PER_TID_RETRY_CONFIG))
> + return -EOPNOTSUPP;
> +
> + if (peer && !wiphy_ext_feature_isset(
> + &rdev->wiphy,
> + NL80211_EXT_FEATURE_PER_STA_RETRY_CONFIG))
> + return -EOPNOTSUPP;
I guess you also need to clarify a bit what all this means. There are
various possibilities, and I don't think you (want to) cover them all:
1. setting for a specific TID for all STAs
2. setting for a specific TID for a single STA
3. setting for all TIDs for all STAs
4. setting for all TIDs for a single STA
The documentation reads like you're doing 1. & 4., but this code looks
more like you're doing 1. & 2., so I think that should be clarified.
> + ret = rdev_set_data_retry_count(rdev, dev, peer, tid_no,
> + retry_short, retry_long);
You should also rename this to set_tid_config() or so, I guess, since 1.
& 2. is what you're doing, and I'm asking to add more things into it.
In fact, in patch 2 you yourself add more into it, but I don't see a
good reason to split the methods, we can pass a struct that can easily
be extended in the future (e.g. noack).
I also think you should expose the current configuration in STA_INFO.
johannes
On Fri, 2018-11-09 at 09:40 +0000, Sergey Matyukevich wrote:
>
> Ok. So if driver receives retry value (-1), it should reset to some
> default value known to driver or firmware. IMHO it worth making it
> more explicit: in its current form this convention will not be obvious
> for driver authors. Though I don't have a good idea how to do it.
> Maybe merge both aggregation and retry cfg80211 callbacks into one
> and use structure for params and bitmask for operations...
I think we want that anyway, I just suggested to also add the "noack" to
the same API, and indeed it's pointless to set retries and noack.
johannes
On 2018-11-09 17:54, Johannes Berg wrote:
> On Fri, 2018-11-09 at 09:40 +0000, Sergey Matyukevich wrote:
>>
>> Ok. So if driver receives retry value (-1), it should reset to some
>> default value known to driver or firmware. IMHO it worth making it
>> more explicit: in its current form this convention will not be obvious
>> for driver authors. Though I don't have a good idea how to do it.
>> Maybe merge both aggregation and retry cfg80211 callbacks into one
>> and use structure for params and bitmask for operations...
>
> I think we want that anyway, I just suggested to also add the "noack"
> to
> the same API, and indeed it's pointless to set retries and noack.
>
Sure, I'll change it to a single api for all the tid_config operations.
Hello Tamizh,
> Add infrastructure for per TID aggregation/retry count configurations
> such as retry count and AMPDU aggregation control(disable/enable).
> In some scenario reducing the number of retry count for a specific data
> traffic can reduce the latency by proceeding with the next packet
> instead of retrying the same packet more time. This will be useful
> where the next packet can resume the operation without an issue.
> Here added NL80211_CMD_SET_TID_CONFIG to support this operation by
> accepting retry count and AMPDU aggregation control.
> This command can accept STA mac addreess to make the configuration
> station specific rather than applying to all the connected stations
> to the netdev.
>
> Tamizh chelvam (3):
> nl80211: Add netlink attribute for AMPDU aggregation enable/disable
> tid conf 3
> ath10k: Add support to configure TID specific configuration
>
> Vasanthakumar Thiagarajan (1):
> New netlink command for TID specific configuration
Could you please share your further plans regarding this patch series ?
Do you plan to send next revision in the forseable future ?
Or have you decided to abandon this approach ?
Regards,
Sergey
Hi Sergey,
> Hello Tamizh,
>
>> Add infrastructure for per TID aggregation/retry count configurations
>> such as retry count and AMPDU aggregation control(disable/enable).
>> In some scenario reducing the number of retry count for a specific
>> data traffic can reduce the latency by proceeding with the next packet
>> instead of retrying the same packet more time. This will be useful
>> where the next packet can resume the operation without an issue.
>> Here added NL80211_CMD_SET_TID_CONFIG to support this operation by
>> accepting retry count and AMPDU aggregation control.
>> This command can accept STA mac addreess to make the configuration
>> station specific rather than applying to all the connected stations to
>> the netdev.
>>
>> Tamizh chelvam (3):
>> nl80211: Add netlink attribute for AMPDU aggregation enable/disable
>> tid conf 3
>> ath10k: Add support to configure TID specific configuration
>>
>> Vasanthakumar Thiagarajan (1):
>> New netlink command for TID specific configuration
>
> Could you please share your further plans regarding this patch series ?
> Do you plan to send next revision in the forseable future ?
> Or have you decided to abandon this approach ?
>
I'm working on new patchsets. I will send it in a week.
Thanks,
Tamizh.