Add infrastructure to support per TID configurations like noack policy,
retry count, AMPDU control(disable/enable), RTSCTS control(enable/disable)
and TX rate mask configurations.
This will be useful for the driver which can supports data TID
specific configuration rather than phy level configurations.
Here NL80211_CMD_SET_TID_CONFIG added to support this operation by
accepting TID configuration.
This command can accept STA mac addreess to make the configuration
station specific rather than applying to all the connected stations
to the netdev.
And this nested command configuration can accept multiple number of
data TID specific configuration in a single command,
enum ieee80211_tid_conf_mask used to notify the driver that which
configuration got modified for the TID.
Tamizh chelvam (6):
nl80211: New netlink command for TID specific configuration
nl80211: Add new netlink attribute for TID speicific retry count
nl80211: Add netlink attribute for AMPDU aggregation enable/disable
nl80211: Add netlink attribute to enable/disable RTS_CTS
nl80211: Add netlink attribute to configure TID specific tx rate
mac80211: Add api to support configuring TID specific configuration
include/net/cfg80211.h | 55 +++++++++
include/net/mac80211.h | 8 ++
include/uapi/linux/nl80211.h | 190 +++++++++++++++++++++++++++++
net/mac80211/cfg.c | 28 +++++
net/mac80211/driver-ops.h | 15 +++
net/wireless/nl80211.c | 276 +++++++++++++++++++++++++++++++++++++++---
net/wireless/rdev-ops.h | 12 ++
net/wireless/trace.h | 17 +++
8 files changed, 584 insertions(+), 17 deletions(-)
v8:
* Fixed enum typecast warning.
v7:
* Fixed compilation error and removed tid config variables from mac80211
v6:
* Addressed Johannes comments.
v5:
* Fixed possible memleak of 'tid_conf' in nl80211_set_tid_config.
v4:
* Fixed kbuild warnings.
v3:
* Modified "nl80211: Add netlink attribute to configure TID specific tx rate" patch
to accept multiple TX rate configuration at a time.
* Modified noack and ampdu variable data type to int in
"mac80211: Add api to support configuring TID specific configuration" patch to store
default configuration.
* Modified "ath10k: Add new api to support TID specific configuration" patch to handle
default values for noack and ampdu. And added sta pointer sanity check in
ath10k_mac_tid_bitrate_config function.
* Fixed "ath10k: Add extended TID configuration support" wmi command parameters
assigned part.
v2:
* Added support to accept multiple TID configuration
* Added support to configure TX rate and RTSCTS control
--
1.7.9.5
Add a new NL command, NL80211_CMD_SET_TID_CONFIG to support
data TID specific configuration. This per TID configurations
are passed in NL80211_TID_CONFIG_ATTR_TID which is a nested
attribute. This patch adds support to configure per TID
noack policy through NL80211_TID_CONFIG_ATTR_NOACK attribute.
Data TID value for this configuration will be passed through
NL80211_TID_CONFIG_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. This patch introduced
enum ieee80211_tid_conf_mask to notify the driver that which
configuration modified.
Driver supporting data TID specific noack policy configuration
should be advertise through NL80211_EXT_FEATURE_PER_TID_NOACK_CONFIG
and supporting per STA data TID noack policy configuration
should be advertise through NL80211_EXT_FEATURE_PER_STA_NOACK_CONFIG
Signed-off-by: Tamizh chelvam <[email protected]>
---
include/net/cfg80211.h | 35 ++++++++++++
include/uapi/linux/nl80211.h | 59 ++++++++++++++++++++
net/wireless/nl80211.c | 122 ++++++++++++++++++++++++++++++++++++++++++
net/wireless/rdev-ops.h | 12 +++++
net/wireless/trace.h | 17 ++++++
5 files changed, 245 insertions(+)
diff --git a/include/net/cfg80211.h b/include/net/cfg80211.h
index 4ab2c49..58e956d 100644
--- a/include/net/cfg80211.h
+++ b/include/net/cfg80211.h
@@ -596,6 +596,35 @@ struct cfg80211_chan_def {
struct ieee80211_edmg edmg;
};
+enum ieee80211_tid_conf_mask {
+ IEEE80211_TID_CONF_NOACK = BIT(0),
+};
+
+/**
+ * struct ieee80211_tid_cfg - TID specific configuration
+ * @tid: TID number
+ * @tid_conf_mask: bitmap indicating which parameter changed
+ * see %enum ieee80211_tid_conf_mask
+ * @noack: noack configuration value for the TID
+ */
+struct ieee80211_tid_cfg {
+ u8 tid;
+ u32 tid_conf_mask;
+ u8 noack;
+};
+
+/**
+ * struct ieee80211_tid_config - TID configuration
+ * @peer: Station's MAC address
+ * @n_tid_conf: Number of TID specific configurations to be applied
+ * @tid_conf: Configuration change info
+ */
+struct ieee80211_tid_config {
+ const u8 *peer;
+ u32 n_tid_conf;
+ struct ieee80211_tid_cfg tid_conf[];
+};
+
/**
* cfg80211_get_chandef_type - return old channel type from chandef
* @chandef: the channel definition
@@ -3625,6 +3654,10 @@ struct cfg80211_update_owe_info {
*
* @probe_mesh_link: Probe direct Mesh peer's link quality by sending data frame
* and overrule HWMP path selection algorithm.
+ * @set_tid_config: TID specific configuration. Apply this configuration for
+ * all the connected stations in the BSS if peer is %NULL. Otherwise
+ * apply this configuration to the specific station.
+ * This callback may sleep.
*/
struct cfg80211_ops {
int (*suspend)(struct wiphy *wiphy, struct cfg80211_wowlan *wow);
@@ -3943,6 +3976,8 @@ struct cfg80211_ops {
struct cfg80211_update_owe_info *owe_info);
int (*probe_mesh_link)(struct wiphy *wiphy, struct net_device *dev,
const u8 *buf, size_t len);
+ int (*set_tid_config)(struct wiphy *wiphy, struct net_device *dev,
+ struct ieee80211_tid_config *tid_conf);
};
/*
diff --git a/include/uapi/linux/nl80211.h b/include/uapi/linux/nl80211.h
index 64135ab..c0b43da 100644
--- a/include/uapi/linux/nl80211.h
+++ b/include/uapi/linux/nl80211.h
@@ -1109,6 +1109,10 @@
* peer MAC address and %NL80211_ATTR_FRAME is used to specify the frame
* content. The frame is ethernet data.
*
+ * @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
*/
@@ -1333,6 +1337,8 @@ enum nl80211_commands {
NL80211_CMD_PROBE_MESH_LINK,
+ NL80211_CMD_SET_TID_CONFIG,
+
/* add new commands above here */
/* used to define NL80211_CMD_MAX below */
@@ -2381,6 +2387,9 @@ enum nl80211_commands {
* the allowed channel bandwidth configurations. (u8 attribute)
* Defined by IEEE P802.11ay/D4.0 section 9.4.2.251, Table 13.
*
+ * @NL80211_ATTR_TID_CONFIG: TID specific configuration in a
+ * nested attribute with %NL80211_TID_CONFIG_ATTR_* sub-attributes.
+ *
* @NUM_NL80211_ATTR: total number of nl80211_attrs available
* @NL80211_ATTR_MAX: highest attribute number currently defined
* @__NL80211_ATTR_AFTER_LAST: internal use
@@ -2843,6 +2852,8 @@ enum nl80211_attrs {
NL80211_ATTR_WIPHY_EDMG_CHANNELS,
NL80211_ATTR_WIPHY_EDMG_BW_CONFIG,
+ NL80211_ATTR_TID_CONFIG,
+
/* add attributes here, update the policy in nl80211.c */
__NL80211_ATTR_AFTER_LAST,
@@ -4682,6 +4693,47 @@ enum nl80211_tx_power_setting {
};
/**
+ * enum nl80211_tid_config - TID config state
+ * @NL80211_TID_CONFIG_DEFAULT: Default config for the TID. This is to
+ * notify driver to reset the previous config and use vif specific
+ * default config
+ * @NL80211_TID_CONFIG_ENABLE: Enable config for the TID
+ * NL80211_TID_CONFIG_DISABLE: Disable config for the TID
+ */
+enum nl80211_tid_config {
+ NL80211_TID_CONFIG_DEFAULT,
+ NL80211_TID_CONFIG_ENABLE,
+ NL80211_TID_CONFIG_DISABLE,
+};
+
+/* enum nl80211_tid_config_attr - TID specific configuration.
+ * @NL80211_TID_CONFIG_ATTR_TID: a TID value (u8 attribute).
+ * @NL80211_TID_CONFIG_ATTR_NOACK: Configure ack policy for the TID.
+ * specified in %NL80211_TID_CONFIG_ATTR_TID. see %enum nl80211_tid_config.
+ * Its type is u8, if the peer MAC address is passed in %NL80211_ATTR_MAC,
+ * then the noack configuration is applied to the data frame for the tid
+ * to that connected station. This 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, then 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_TID_NOACK_CONFIG and
+ * NL80211_EXT_FEATURE_PER_STA_NOACK_CONFIG for supporting per sta
+ * configuration.
+ */
+enum nl80211_tid_config_attr {
+ __NL80211_TID_CONFIG_ATTR_INVALID,
+ NL80211_TID_CONFIG_ATTR_TID,
+ NL80211_TID_CONFIG_ATTR_NOACK,
+
+ /* keep last */
+ __NL80211_TID_CONFIG_ATTR_AFTER_LAST,
+ NL80211_TID_CONFIG_ATTR_MAX = __NL80211_TID_CONFIG_ATTR_AFTER_LAST - 1
+};
+
+/**
* enum nl80211_packet_pattern_attr - packet pattern attribute
* @__NL80211_PKTPAT_INVALID: invalid number for nested attribute
* @NL80211_PKTPAT_PATTERN: the pattern, values where the mask has
@@ -5492,6 +5544,11 @@ enum nl80211_feature_flags {
* @NL80211_EXT_FEATURE_SAE_OFFLOAD: Device wants to do SAE authentication in
* station mode (SAE password is passed as part of the connect command).
*
+ * @NL80211_EXT_FEATURE_PER_TID_NOACK_CONFIG: Driver supports per TID NoAck
+ * policy functionality.
+ * @NL80211_EXT_FEATURE_PER_STA_NOACK_CONFIG: Driver supports STA specific NoAck
+ * policy functionality.
+ *
* @NUM_NL80211_EXT_FEATURES: number of extended features.
* @MAX_NL80211_EXT_FEATURES: highest extended feature index.
*/
@@ -5537,6 +5594,8 @@ enum nl80211_ext_feature_index {
NL80211_EXT_FEATURE_EXT_KEY_ID,
NL80211_EXT_FEATURE_STA_TX_PWR,
NL80211_EXT_FEATURE_SAE_OFFLOAD,
+ NL80211_EXT_FEATURE_PER_TID_NOACK_CONFIG,
+ NL80211_EXT_FEATURE_PER_STA_NOACK_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 d1451e7..c5c3af5 100644
--- a/net/wireless/nl80211.c
+++ b/net/wireless/nl80211.c
@@ -321,6 +321,13 @@ static int validate_ie_attr(const struct nlattr *attr,
NLA_POLICY_RANGE(NLA_U8, 1, 20),
};
+static const struct nla_policy
+nl80211_tid_config_attr_policy[NL80211_TID_CONFIG_ATTR_MAX + 1] = {
+ [NL80211_TID_CONFIG_ATTR_TID] = { .type = NLA_U8 },
+ [NL80211_TID_CONFIG_ATTR_NOACK] =
+ NLA_POLICY_MAX(NLA_U8, NL80211_TID_CONFIG_DISABLE),
+};
+
const struct nla_policy nl80211_policy[NUM_NL80211_ATTR] = {
[0] = { .strict_start_type = NL80211_ATTR_HE_OBSS_PD },
[NL80211_ATTR_WIPHY] = { .type = NLA_U32 },
@@ -624,6 +631,8 @@ static int validate_ie_attr(const struct nlattr *attr,
.len = SAE_PASSWORD_MAX_LEN },
[NL80211_ATTR_TWT_RESPONDER] = { .type = NLA_FLAG },
[NL80211_ATTR_HE_OBSS_PD] = NLA_POLICY_NESTED(he_obss_pd_policy),
+ [NL80211_ATTR_TID_CONFIG] =
+ NLA_POLICY_NESTED_ARRAY(nl80211_tid_config_attr_policy),
};
/* policy for the key attributes */
@@ -13788,6 +13797,112 @@ static int nl80211_probe_mesh_link(struct sk_buff *skb, struct genl_info *info)
return rdev_probe_mesh_link(rdev, dev, dest, buf, len);
}
+static int
+nl80211_check_tid_config_support(struct cfg80211_registered_device *rdev,
+ struct netlink_ext_ack *extack,
+ const u8 *peer, struct nlattr *attr,
+ enum nl80211_ext_feature_index per_tid_config,
+ enum nl80211_ext_feature_index per_sta_config)
+{
+ if (!wiphy_ext_feature_isset(&rdev->wiphy, per_tid_config)) {
+ NL_SET_ERR_MSG_ATTR(extack, attr, "TID specific configuration not supported");
+ return -ENOTSUPP;
+ }
+
+ if (peer && !wiphy_ext_feature_isset(&rdev->wiphy, per_sta_config)) {
+ NL_SET_ERR_MSG_ATTR(extack, attr, "peer specific TID configuration not supported");
+ return -ENOTSUPP;
+ }
+
+ return 0;
+}
+
+static int parse_tid_conf(struct cfg80211_registered_device *rdev,
+ struct nlattr *attrs[],
+ struct ieee80211_tid_cfg *tid_conf,
+ struct genl_info *info, const u8 *peer)
+{
+ struct netlink_ext_ack *extack = info->extack;
+ int err;
+
+ tid_conf->tid = nla_get_u8(attrs[NL80211_TID_CONFIG_ATTR_TID]);
+ if (attrs[NL80211_TID_CONFIG_ATTR_NOACK]) {
+ err = nl80211_check_tid_config_support(rdev, extack, peer,
+ attrs[NL80211_TID_CONFIG_ATTR_NOACK],
+ NL80211_EXT_FEATURE_PER_TID_NOACK_CONFIG,
+ NL80211_EXT_FEATURE_PER_STA_NOACK_CONFIG);
+ if (err)
+ return err;
+
+ tid_conf->tid_conf_mask |= IEEE80211_TID_CONF_NOACK;
+ tid_conf->noack =
+ nla_get_u8(attrs[NL80211_TID_CONFIG_ATTR_NOACK]);
+ }
+
+ return 0;
+}
+
+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_TID_CONFIG_ATTR_MAX + 1];
+ struct net_device *dev = info->user_ptr[1];
+ struct ieee80211_tid_config *tid_config;
+ struct nlattr *tid;
+ int conf_idx = 0, rem_conf;
+ int ret = -EINVAL;
+ u32 num_conf = 0;
+
+ if (!info->attrs[NL80211_ATTR_TID_CONFIG])
+ return -EINVAL;
+
+ if (!rdev->ops->set_tid_config)
+ return -EOPNOTSUPP;
+
+ nla_for_each_nested(tid, info->attrs[NL80211_ATTR_TID_CONFIG],
+ rem_conf)
+ num_conf++;
+
+ tid_config = kzalloc(struct_size(tid_config, tid_conf, num_conf),
+ GFP_KERNEL);
+ if (!tid_config)
+ return -ENOMEM;
+
+ tid_config->n_tid_conf = num_conf;
+
+ if (info->attrs[NL80211_ATTR_MAC])
+ tid_config->peer = nla_data(info->attrs[NL80211_ATTR_MAC]);
+
+ nla_for_each_nested(tid, info->attrs[NL80211_ATTR_TID_CONFIG],
+ rem_conf) {
+ ret = nla_parse_nested(attrs, NL80211_TID_CONFIG_ATTR_MAX,
+ tid, NULL, NULL);
+
+ if (ret)
+ goto bad_tid_conf;
+
+ if (!attrs[NL80211_TID_CONFIG_ATTR_TID]) {
+ ret = -EINVAL;
+ goto bad_tid_conf;
+ }
+
+ ret = parse_tid_conf(rdev, attrs,
+ &tid_config->tid_conf[conf_idx],
+ info, tid_config->peer);
+ if (ret)
+ goto bad_tid_conf;
+
+ conf_idx++;
+ }
+
+ ret = rdev_set_tid_config(rdev, dev, tid_config);
+
+bad_tid_conf:
+ kfree(tid_config);
+ return ret;
+}
+
#define NL80211_FLAG_NEED_WIPHY 0x01
#define NL80211_FLAG_NEED_NETDEV 0x02
#define NL80211_FLAG_NEED_RTNL 0x04
@@ -14742,6 +14857,13 @@ static void nl80211_post_doit(const struct genl_ops *ops, struct sk_buff *skb,
.internal_flags = NL80211_FLAG_NEED_NETDEV_UP |
NL80211_FLAG_NEED_RTNL,
},
+ {
+ .cmd = NL80211_CMD_SET_TID_CONFIG,
+ .doit = nl80211_set_tid_config,
+ .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 e853a4f..887cda2 100644
--- a/net/wireless/rdev-ops.h
+++ b/net/wireless/rdev-ops.h
@@ -1299,4 +1299,16 @@ static inline int rdev_update_owe_info(struct cfg80211_registered_device *rdev,
return ret;
}
+static inline int rdev_set_tid_config(struct cfg80211_registered_device *rdev,
+ struct net_device *dev,
+ struct ieee80211_tid_config *tid_conf)
+{
+ int ret;
+
+ trace_rdev_set_tid_config(&rdev->wiphy, dev, tid_conf);
+ ret = rdev->ops->set_tid_config(&rdev->wiphy, dev, tid_conf);
+ 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 d98ad2b..522da8d 100644
--- a/net/wireless/trace.h
+++ b/net/wireless/trace.h
@@ -3458,6 +3458,23 @@
WIPHY_PR_ARG, NETDEV_PR_ARG, MAC_PR_ARG(dest))
);
+TRACE_EVENT(rdev_set_tid_config,
+ TP_PROTO(struct wiphy *wiphy, struct net_device *netdev,
+ struct ieee80211_tid_config *tid_conf),
+ TP_ARGS(wiphy, netdev, tid_conf),
+ TP_STRUCT__entry(
+ WIPHY_ENTRY
+ NETDEV_ENTRY
+ MAC_ENTRY(peer)
+ ),
+ TP_fast_assign(
+ WIPHY_ASSIGN;
+ NETDEV_ASSIGN;
+ MAC_ASSIGN(peer, tid_conf->peer);
+ ),
+ TP_printk(WIPHY_PR_FMT ", " NETDEV_PR_FMT ", peer: " MAC_PR_FMT,
+ WIPHY_PR_ARG, NETDEV_PR_ARG, MAC_PR_ARG(peer))
+);
#endif /* !__RDEV_OPS_TRACE || TRACE_HEADER_MULTI_READ */
#undef TRACE_INCLUDE_PATH
--
1.7.9.5
This patch introduced below NL attributes to add support for
configuring data TID specific retry count
NL80211_TID_CONFIG_ATTR_RETRY
NL80211_TID_CONFIG_ATTR_RETRY_SHORT
NL80211_TID_CONFIG_ATTR_RETRY_LONG
These attributes are added in NL80211_ATTR_TID_CONFIG nested attribute.
This will be useful for the driver which supports data TID specific retry
count configuration rather using phy level retry configuration.
This TID specific retry configuration will have more precedence than
phy level configuration. This configuration can be applied for a
specific peer. To apply this configuration specific to a peer
rather than being applied for all the connected stations,
MAC address of the peer can be passed in NL80211_ATTR_MAC 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 retry configuration should advertise
NL80211_EXT_FEATURE_PER_TID_RETRY_CONFIG and per STA specific
data TID retry configuration should advertise
NL80211_EXT_FEATURE_PER_STA_RETRY_CONFIG.
Signed-off-by: Tamizh chelvam <[email protected]>
---
include/net/cfg80211.h | 9 ++++++++
include/uapi/linux/nl80211.h | 48 ++++++++++++++++++++++++++++++++++++++++++
net/wireless/nl80211.c | 45 +++++++++++++++++++++++++++++++++++++++
3 files changed, 102 insertions(+)
diff --git a/include/net/cfg80211.h b/include/net/cfg80211.h
index 58e956d..dd40774 100644
--- a/include/net/cfg80211.h
+++ b/include/net/cfg80211.h
@@ -598,6 +598,7 @@ struct cfg80211_chan_def {
enum ieee80211_tid_conf_mask {
IEEE80211_TID_CONF_NOACK = BIT(0),
+ IEEE80211_TID_CONF_RETRY = BIT(1),
};
/**
@@ -606,11 +607,15 @@ enum ieee80211_tid_conf_mask {
* @tid_conf_mask: bitmap indicating which parameter changed
* see %enum ieee80211_tid_conf_mask
* @noack: noack configuration value for the TID
+ * @retry_long: retry count value
+ * @retry_short: retry count value
*/
struct ieee80211_tid_cfg {
u8 tid;
u32 tid_conf_mask;
u8 noack;
+ int retry_long;
+ int retry_short;
};
/**
@@ -4567,6 +4572,8 @@ struct cfg80211_pmsr_capabilities {
* @support_mbssid must be set for this to have any effect.
*
* @pmsr_capa: peer measurement capabilities
+ * @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 */
@@ -4711,6 +4718,8 @@ struct wiphy {
const struct cfg80211_pmsr_capabilities *pmsr_capa;
+ 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 c0b43da..69b3229 100644
--- a/include/uapi/linux/nl80211.h
+++ b/include/uapi/linux/nl80211.h
@@ -2389,6 +2389,8 @@ enum nl80211_commands {
*
* @NL80211_ATTR_TID_CONFIG: TID specific configuration in a
* nested attribute with %NL80211_TID_CONFIG_ATTR_* 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
@@ -2853,6 +2855,7 @@ enum nl80211_attrs {
NL80211_ATTR_WIPHY_EDMG_BW_CONFIG,
NL80211_ATTR_TID_CONFIG,
+ NL80211_ATTR_MAX_RETRY_COUNT,
/* add attributes here, update the policy in nl80211.c */
@@ -4722,11 +4725,50 @@ enum nl80211_tid_config {
* advertise NL80211_EXT_FEATURE_PER_TID_NOACK_CONFIG and
* NL80211_EXT_FEATURE_PER_STA_NOACK_CONFIG for supporting per sta
* configuration.
+ * @NL80211_TID_CONFIG_ATTR_RETRY: Data TID retry count should be applied
+ * with the value passed through %NL80211_TID_CONFIG_ATTR_RETRY_LONG
+ * and/or %NL80211_TID_CONFIG_ATTR_RETRY_SHORT. This configuration
+ * is per-TID, and the TID is specified with %NL80211_TID_CONFIG_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_TID_CONFIG_ATTR_RETRY_SHORT: Number of retries used with data frame
+ * transmission, user-space sets this configuration in
+ * &NL80211_CMD_SET_TID_CONFIG. It is u8 type, 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_TID_CONFIG_ATTR_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_tid_config_attr {
__NL80211_TID_CONFIG_ATTR_INVALID,
NL80211_TID_CONFIG_ATTR_TID,
NL80211_TID_CONFIG_ATTR_NOACK,
+ NL80211_TID_CONFIG_ATTR_RETRY,
+ NL80211_TID_CONFIG_ATTR_RETRY_SHORT,
+ NL80211_TID_CONFIG_ATTR_RETRY_LONG,
/* keep last */
__NL80211_TID_CONFIG_ATTR_AFTER_LAST,
@@ -5548,6 +5590,10 @@ enum nl80211_feature_flags {
* policy functionality.
* @NL80211_EXT_FEATURE_PER_STA_NOACK_CONFIG: Driver supports STA specific NoAck
* policy functionality.
+ * @NL80211_EXT_FEATURE_PER_TID_RETRY_CONFIG: Driver supports per TID data retry
+ * count functionality.
+ * @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.
@@ -5596,6 +5642,8 @@ enum nl80211_ext_feature_index {
NL80211_EXT_FEATURE_SAE_OFFLOAD,
NL80211_EXT_FEATURE_PER_TID_NOACK_CONFIG,
NL80211_EXT_FEATURE_PER_STA_NOACK_CONFIG,
+ 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 c5c3af5..7facc93 100644
--- a/net/wireless/nl80211.c
+++ b/net/wireless/nl80211.c
@@ -326,6 +326,9 @@ static int validate_ie_attr(const struct nlattr *attr,
[NL80211_TID_CONFIG_ATTR_TID] = { .type = NLA_U8 },
[NL80211_TID_CONFIG_ATTR_NOACK] =
NLA_POLICY_MAX(NLA_U8, NL80211_TID_CONFIG_DISABLE),
+ [NL80211_TID_CONFIG_ATTR_RETRY] = { .type = NLA_FLAG },
+ [NL80211_TID_CONFIG_ATTR_RETRY_SHORT] = { .type = NLA_U8},
+ [NL80211_TID_CONFIG_ATTR_RETRY_LONG] = { .type = NLA_U8},
};
const struct nla_policy nl80211_policy[NUM_NL80211_ATTR] = {
@@ -2028,6 +2031,12 @@ static int nl80211_send_wiphy(struct cfg80211_registered_device *rdev,
}
}
+ if (rdev->wiphy.max_data_retry_count) {
+ if (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;
@@ -13839,6 +13848,42 @@ static int parse_tid_conf(struct cfg80211_registered_device *rdev,
nla_get_u8(attrs[NL80211_TID_CONFIG_ATTR_NOACK]);
}
+ if (nla_get_flag(attrs[NL80211_TID_CONFIG_ATTR_RETRY])) {
+ err = nl80211_check_tid_config_support(rdev, extack, peer,
+ attrs[NL80211_TID_CONFIG_ATTR_RETRY],
+ NL80211_EXT_FEATURE_PER_TID_RETRY_CONFIG,
+ NL80211_EXT_FEATURE_PER_STA_RETRY_CONFIG);
+ if (err)
+ return err;
+
+ tid_conf->tid_conf_mask |= IEEE80211_TID_CONF_RETRY;
+ if (attrs[NL80211_TID_CONFIG_ATTR_RETRY_SHORT]) {
+ tid_conf->retry_short =
+ nla_get_u8(attrs[NL80211_TID_CONFIG_ATTR_RETRY_SHORT]);
+ if (tid_conf->retry_short >
+ rdev->wiphy.max_data_retry_count)
+ return -EINVAL;
+ } else {
+ /* Use driver default retry count or VIF specific
+ * retry count
+ */
+ tid_conf->retry_short = -1;
+ }
+
+ if (attrs[NL80211_TID_CONFIG_ATTR_RETRY_LONG]) {
+ tid_conf->retry_long =
+ nla_get_u8(attrs[NL80211_TID_CONFIG_ATTR_RETRY_LONG]);
+ if (tid_conf->retry_long >
+ rdev->wiphy.max_data_retry_count)
+ return -EINVAL;
+ } else {
+ /* Use driver default retry count or VIF specific
+ * retry count
+ */
+ tid_conf->retry_long = -1;
+ }
+ }
+
return 0;
}
--
1.7.9.5
Introduce NL80211_TID_CONFIG_ATTR_AMPDU_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_TID_CONFIG_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_CTRL and supporting per-STA data TID
ampdu configuration should advertise NL80211_EXT_FEATURE_PER_STA_AMPDU_CTRL.
Signed-off-by: Tamizh chelvam <[email protected]>
---
include/net/cfg80211.h | 3 +++
include/uapi/linux/nl80211.h | 22 ++++++++++++++++++++++
net/wireless/nl80211.c | 14 ++++++++++++++
3 files changed, 39 insertions(+)
diff --git a/include/net/cfg80211.h b/include/net/cfg80211.h
index dd40774..baed260 100644
--- a/include/net/cfg80211.h
+++ b/include/net/cfg80211.h
@@ -599,6 +599,7 @@ struct cfg80211_chan_def {
enum ieee80211_tid_conf_mask {
IEEE80211_TID_CONF_NOACK = BIT(0),
IEEE80211_TID_CONF_RETRY = BIT(1),
+ IEEE80211_TID_CONF_AMPDU = BIT(2),
};
/**
@@ -609,6 +610,7 @@ enum ieee80211_tid_conf_mask {
* @noack: noack configuration value for the TID
* @retry_long: retry count value
* @retry_short: retry count value
+ * @ampdu: Enable/Disable aggregation
*/
struct ieee80211_tid_cfg {
u8 tid;
@@ -616,6 +618,7 @@ struct ieee80211_tid_cfg {
u8 noack;
int retry_long;
int retry_short;
+ u8 ampdu;
};
/**
diff --git a/include/uapi/linux/nl80211.h b/include/uapi/linux/nl80211.h
index 69b3229..32ca476 100644
--- a/include/uapi/linux/nl80211.h
+++ b/include/uapi/linux/nl80211.h
@@ -4761,6 +4761,21 @@ enum nl80211_tid_config {
* 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_TID_CONFIG_ATTR_AMPDU_CTRL: Enable/Disable aggregation for the TID
+ * specified in %%NL80211_TID_CONFIG_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_TID_AMPDU_CTRL and supporting per station
+ * aggregation configuration should advertise
+ * NL80211_EXT_FEATURE_PER_STA_AMPDU_CTRL.
*/
enum nl80211_tid_config_attr {
__NL80211_TID_CONFIG_ATTR_INVALID,
@@ -4769,6 +4784,7 @@ enum nl80211_tid_config_attr {
NL80211_TID_CONFIG_ATTR_RETRY,
NL80211_TID_CONFIG_ATTR_RETRY_SHORT,
NL80211_TID_CONFIG_ATTR_RETRY_LONG,
+ NL80211_TID_CONFIG_ATTR_AMPDU_CTRL,
/* keep last */
__NL80211_TID_CONFIG_ATTR_AFTER_LAST,
@@ -5594,6 +5610,10 @@ enum nl80211_feature_flags {
* count functionality.
* @NL80211_EXT_FEATURE_PER_STA_RETRY_CONFIG: Driver supports STA specific
* data retry count functionality.
+ * @NL80211_EXT_FEATURE_PER_TID_AMPDU_CTRL: Driver supports TID specific
+ * aggregation control(enable/disable).
+ * @NL80211_EXT_FEATURE_PER_STA_AMPDU_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.
@@ -5644,6 +5664,8 @@ enum nl80211_ext_feature_index {
NL80211_EXT_FEATURE_PER_STA_NOACK_CONFIG,
NL80211_EXT_FEATURE_PER_TID_RETRY_CONFIG,
NL80211_EXT_FEATURE_PER_STA_RETRY_CONFIG,
+ NL80211_EXT_FEATURE_PER_TID_AMPDU_CTRL,
+ NL80211_EXT_FEATURE_PER_STA_AMPDU_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 7facc93..5ac8862 100644
--- a/net/wireless/nl80211.c
+++ b/net/wireless/nl80211.c
@@ -329,6 +329,8 @@ static int validate_ie_attr(const struct nlattr *attr,
[NL80211_TID_CONFIG_ATTR_RETRY] = { .type = NLA_FLAG },
[NL80211_TID_CONFIG_ATTR_RETRY_SHORT] = { .type = NLA_U8},
[NL80211_TID_CONFIG_ATTR_RETRY_LONG] = { .type = NLA_U8},
+ [NL80211_TID_CONFIG_ATTR_AMPDU_CTRL] =
+ NLA_POLICY_MAX(NLA_U8, NL80211_TID_CONFIG_DISABLE),
};
const struct nla_policy nl80211_policy[NUM_NL80211_ATTR] = {
@@ -13884,6 +13886,18 @@ static int parse_tid_conf(struct cfg80211_registered_device *rdev,
}
}
+ if (attrs[NL80211_TID_CONFIG_ATTR_AMPDU_CTRL]) {
+ err = nl80211_check_tid_config_support(rdev, extack, peer,
+ attrs[NL80211_TID_CONFIG_ATTR_AMPDU_CTRL],
+ NL80211_EXT_FEATURE_PER_TID_AMPDU_CTRL,
+ NL80211_EXT_FEATURE_PER_STA_AMPDU_CTRL);
+ if (err)
+ return err;
+
+ tid_conf->tid_conf_mask |= IEEE80211_TID_CONF_AMPDU;
+ tid_conf->ampdu =
+ nla_get_u8(attrs[NL80211_TID_CONFIG_ATTR_AMPDU_CTRL]);
+ }
return 0;
}
--
1.7.9.5
Introduce NL80211_TID_CONFIG_ATTR_RTSCTS_CTRL in nl80211_attr_tid_config
to accept TID specific RTS_CTS enable/disable configuration
through NL80211_CMD_SET_TID_CONFIG command. TID for which the
RTS_CTS control configuration is to be applied is passed in
NL80211_TID_CONFIG_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_RTSCTS_CTRL and supporting per-STA data TID
RTS_CTS configuration should advertise NL80211_EXT_FEATURE_PER_STA_RTSCTS_CTRL.
Signed-off-by: Tamizh chelvam <[email protected]>
---
include/net/cfg80211.h | 3 +++
include/uapi/linux/nl80211.h | 22 ++++++++++++++++++++++
net/wireless/nl80211.c | 16 ++++++++++++++++
3 files changed, 41 insertions(+)
diff --git a/include/net/cfg80211.h b/include/net/cfg80211.h
index baed260..deeb1c1 100644
--- a/include/net/cfg80211.h
+++ b/include/net/cfg80211.h
@@ -600,6 +600,7 @@ enum ieee80211_tid_conf_mask {
IEEE80211_TID_CONF_NOACK = BIT(0),
IEEE80211_TID_CONF_RETRY = BIT(1),
IEEE80211_TID_CONF_AMPDU = BIT(2),
+ IEEE80211_TID_CONF_RTSCTS = BIT(3),
};
/**
@@ -611,6 +612,7 @@ enum ieee80211_tid_conf_mask {
* @retry_long: retry count value
* @retry_short: retry count value
* @ampdu: Enable/Disable aggregation
+ * @rtscts: Enable/Disable RTS/CTS
*/
struct ieee80211_tid_cfg {
u8 tid;
@@ -619,6 +621,7 @@ struct ieee80211_tid_cfg {
int retry_long;
int retry_short;
u8 ampdu;
+ u8 rtscts;
};
/**
diff --git a/include/uapi/linux/nl80211.h b/include/uapi/linux/nl80211.h
index 32ca476..f75f243 100644
--- a/include/uapi/linux/nl80211.h
+++ b/include/uapi/linux/nl80211.h
@@ -4776,6 +4776,21 @@ enum nl80211_tid_config {
* NL80211_EXT_FEATURE_PER_TID_AMPDU_CTRL and supporting per station
* aggregation configuration should advertise
* NL80211_EXT_FEATURE_PER_STA_AMPDU_CTRL.
+ * @NL80211_TID_CONFIG_ATTR_RTSCTS_CTRL: Enable/Disable RTS_CTS for the TID
+ * specified in %%NL80211_TID_CONFIG_ATTR_TID. It is u8 type, if the
+ * peer MAC address is passed in %NL80211_ATTR_MAC, then this
+ * configuration is applied to the data frame for the tid to that
+ * connected station.
+ * Station specific RTS_CTS 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_TID_RTSCTS_CTRL and supporting per station
+ * RTS_CTS configuration should advertise
+ * NL80211_EXT_FEATURE_PER_STA_RTSCTS_CTRL.
*/
enum nl80211_tid_config_attr {
__NL80211_TID_CONFIG_ATTR_INVALID,
@@ -4785,6 +4800,7 @@ enum nl80211_tid_config_attr {
NL80211_TID_CONFIG_ATTR_RETRY_SHORT,
NL80211_TID_CONFIG_ATTR_RETRY_LONG,
NL80211_TID_CONFIG_ATTR_AMPDU_CTRL,
+ NL80211_TID_CONFIG_ATTR_RTSCTS_CTRL,
/* keep last */
__NL80211_TID_CONFIG_ATTR_AFTER_LAST,
@@ -5614,6 +5630,10 @@ enum nl80211_feature_flags {
* aggregation control(enable/disable).
* @NL80211_EXT_FEATURE_PER_STA_AMPDU_CTRL: Driver supports per STA
* specific TID aggregation control(enable/disable).
+ * @NL80211_EXT_FEATURE_PER_TID_RTSCTS_CTRL: Driver supports TID specific
+ * RTS_CTS control(enable/disable).
+ * @NL80211_EXT_FEATURE_PER_STA_RTSCTS_CTRL: Driver supports STA specific
+ * RTS_CTS control(enable/disable).
*
* @NUM_NL80211_EXT_FEATURES: number of extended features.
* @MAX_NL80211_EXT_FEATURES: highest extended feature index.
@@ -5666,6 +5686,8 @@ enum nl80211_ext_feature_index {
NL80211_EXT_FEATURE_PER_STA_RETRY_CONFIG,
NL80211_EXT_FEATURE_PER_TID_AMPDU_CTRL,
NL80211_EXT_FEATURE_PER_STA_AMPDU_CTRL,
+ NL80211_EXT_FEATURE_PER_TID_RTSCTS_CTRL,
+ NL80211_EXT_FEATURE_PER_STA_RTSCTS_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 5ac8862..abce915 100644
--- a/net/wireless/nl80211.c
+++ b/net/wireless/nl80211.c
@@ -331,6 +331,8 @@ static int validate_ie_attr(const struct nlattr *attr,
[NL80211_TID_CONFIG_ATTR_RETRY_LONG] = { .type = NLA_U8},
[NL80211_TID_CONFIG_ATTR_AMPDU_CTRL] =
NLA_POLICY_MAX(NLA_U8, NL80211_TID_CONFIG_DISABLE),
+ [NL80211_TID_CONFIG_ATTR_RTSCTS_CTRL] =
+ NLA_POLICY_MAX(NLA_U8, NL80211_TID_CONFIG_DISABLE),
};
const struct nla_policy nl80211_policy[NUM_NL80211_ATTR] = {
@@ -13898,6 +13900,20 @@ static int parse_tid_conf(struct cfg80211_registered_device *rdev,
tid_conf->ampdu =
nla_get_u8(attrs[NL80211_TID_CONFIG_ATTR_AMPDU_CTRL]);
}
+
+ if (attrs[NL80211_TID_CONFIG_ATTR_RTSCTS_CTRL]) {
+ err = nl80211_check_tid_config_support(rdev, extack, peer,
+ attrs[NL80211_TID_CONFIG_ATTR_RTSCTS_CTRL],
+ NL80211_EXT_FEATURE_PER_TID_RTSCTS_CTRL,
+ NL80211_EXT_FEATURE_PER_STA_RTSCTS_CTRL);
+ if (err)
+ return err;
+
+ tid_conf->tid_conf_mask |= IEEE80211_TID_CONF_RTSCTS;
+ tid_conf->rtscts =
+ nla_get_u8(attrs[NL80211_TID_CONFIG_ATTR_RTSCTS_CTRL]);
+ }
+
return 0;
}
--
1.7.9.5
Introduce NL80211_TID_CONFIG_ATTR_TX_RATES in nl80211_tid_attr_config
to accept data TID specific TX bitrate configuration
through NL80211_CMD_SET_TID_CONFIG command. TID for which the
this configuration is to be applied is passed in
NL80211_TID_CONFIG_ATTR_TID attribute. TX bitrate mask values passed
in NL80211_ATTR_TX_RATES attribute and NL80211_TID_CONFIG_ATTR_TX_RATES
attribute will have types of the TX rate should be applied. This uses
nl80211_parse_tx_bitrate_mask to validate and calculate the bitrate
mask. 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_TX_BITRATE_MASK and supporting per-STA data TID
TX bitrate configuration should advertise
NL80211_EXT_FEATURE_PER_STA_TX_BITRATE_MASK.
Signed-off-by: Tamizh chelvam <[email protected]>
---
include/net/cfg80211.h | 5 +++
include/uapi/linux/nl80211.h | 39 +++++++++++++++++++++
net/wireless/nl80211.c | 79 +++++++++++++++++++++++++++++++++---------
3 files changed, 106 insertions(+), 17 deletions(-)
diff --git a/include/net/cfg80211.h b/include/net/cfg80211.h
index deeb1c1..69f4769 100644
--- a/include/net/cfg80211.h
+++ b/include/net/cfg80211.h
@@ -601,6 +601,7 @@ enum ieee80211_tid_conf_mask {
IEEE80211_TID_CONF_RETRY = BIT(1),
IEEE80211_TID_CONF_AMPDU = BIT(2),
IEEE80211_TID_CONF_RTSCTS = BIT(3),
+ IEEE80211_TID_CONF_TX_BITRATE = BIT(4),
};
/**
@@ -613,6 +614,8 @@ enum ieee80211_tid_conf_mask {
* @retry_short: retry count value
* @ampdu: Enable/Disable aggregation
* @rtscts: Enable/Disable RTS/CTS
+ * @txrate_type: TX bitrate mask type
+ * @mask: bitrate to be applied for the TID
*/
struct ieee80211_tid_cfg {
u8 tid;
@@ -622,6 +625,8 @@ struct ieee80211_tid_cfg {
int retry_short;
u8 ampdu;
u8 rtscts;
+ enum nl80211_tx_rate_setting txrate_type;
+ struct cfg80211_bitrate_mask *mask;
};
/**
diff --git a/include/uapi/linux/nl80211.h b/include/uapi/linux/nl80211.h
index f75f243..034d8ff 100644
--- a/include/uapi/linux/nl80211.h
+++ b/include/uapi/linux/nl80211.h
@@ -4696,6 +4696,18 @@ enum nl80211_tx_power_setting {
};
/**
+ * enum nl80211_tx_rate_setting - TX rate configuration type
+ * @NL80211_TX_RATE_AUTOMATIC: automatically determine TX rate
+ * @NL80211_TX_RATE_LIMITED: limit the TX rate by the TX rate parameter
+ * @NL80211_TX_RATE_FIXED: fix TX rate to the TX rate parameter
+ */
+enum nl80211_tx_rate_setting {
+ NL80211_TX_RATE_AUTOMATIC,
+ NL80211_TX_RATE_LIMITED,
+ NL80211_TX_RATE_FIXED,
+};
+
+/**
* enum nl80211_tid_config - TID config state
* @NL80211_TID_CONFIG_DEFAULT: Default config for the TID. This is to
* notify driver to reset the previous config and use vif specific
@@ -4791,6 +4803,25 @@ enum nl80211_tid_config {
* NL80211_EXT_FEATURE_PER_TID_RTSCTS_CTRL and supporting per station
* RTS_CTS configuration should advertise
* NL80211_EXT_FEATURE_PER_STA_RTSCTS_CTRL.
+ * @NL80211_TID_CONFIG_ATTR_TX_RATES: Data frame TX rate mask should be applied
+ * with the parameters passed through %NL80211_ATTR_TX_RATES. This
+ * configuration is per TID, TID is specified with
+ * %NL80211_TID_CONFIG_ATTR_TID.
+ * If the peer MAC address is passed in %NL80211_ATTR_MAC, then this
+ * configuration is applied to the data frame for the tid to that connected
+ * station. This attribute will be useful to notfiy the driver that what
+ * type of txrate should be applied(%enum enum nl80211_tx_rate_setting)
+ * for the connected station (%NL80211_ATTR_MAC),
+ * 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. Driver supporting this feature should advertise
+ * NL80211_EXT_FEATURE_PER_TID_TX_BITRATE_MASK and supporting per station
+ * TX bitrate configuration should advertise
+ * NL80211_EXT_FEATURE_PER_STA_TX_BITRATE_MASK.
*/
enum nl80211_tid_config_attr {
__NL80211_TID_CONFIG_ATTR_INVALID,
@@ -4801,6 +4832,8 @@ enum nl80211_tid_config_attr {
NL80211_TID_CONFIG_ATTR_RETRY_LONG,
NL80211_TID_CONFIG_ATTR_AMPDU_CTRL,
NL80211_TID_CONFIG_ATTR_RTSCTS_CTRL,
+ NL80211_TID_CONFIG_ATTR_TX_RATES_TYPE,
+ NL80211_TID_CONFIG_ATTR_TX_RATES,
/* keep last */
__NL80211_TID_CONFIG_ATTR_AFTER_LAST,
@@ -5634,6 +5667,10 @@ enum nl80211_feature_flags {
* RTS_CTS control(enable/disable).
* @NL80211_EXT_FEATURE_PER_STA_RTSCTS_CTRL: Driver supports STA specific
* RTS_CTS control(enable/disable).
+ * @NL80211_EXT_FEATURE_PER_TID_TX_BITRATE_MASK: Driver supports TID specific
+ * TX bitrate configuration.
+ * @NL80211_EXT_FEATURE_PER_STA_TX_BITRATE_MASK: Driver supports STA specific
+ * TX bitrate configuration.
*
* @NUM_NL80211_EXT_FEATURES: number of extended features.
* @MAX_NL80211_EXT_FEATURES: highest extended feature index.
@@ -5688,6 +5725,8 @@ enum nl80211_ext_feature_index {
NL80211_EXT_FEATURE_PER_STA_AMPDU_CTRL,
NL80211_EXT_FEATURE_PER_TID_RTSCTS_CTRL,
NL80211_EXT_FEATURE_PER_STA_RTSCTS_CTRL,
+ NL80211_EXT_FEATURE_PER_TID_TX_BITRATE_MASK,
+ NL80211_EXT_FEATURE_PER_STA_TX_BITRATE_MASK,
/* add new features before the definition below */
NUM_NL80211_EXT_FEATURES,
diff --git a/net/wireless/nl80211.c b/net/wireless/nl80211.c
index abce915..81af51b 100644
--- a/net/wireless/nl80211.c
+++ b/net/wireless/nl80211.c
@@ -321,6 +321,18 @@ static int validate_ie_attr(const struct nlattr *attr,
NLA_POLICY_RANGE(NLA_U8, 1, 20),
};
+static const struct nla_policy nl80211_txattr_policy[NL80211_TXRATE_MAX + 1] = {
+ [NL80211_TXRATE_LEGACY] = { .type = NLA_BINARY,
+ .len = NL80211_MAX_SUPP_RATES },
+ [NL80211_TXRATE_HT] = { .type = NLA_BINARY,
+ .len = NL80211_MAX_SUPP_HT_RATES },
+ [NL80211_TXRATE_VHT] = {
+ .type = NLA_EXACT_LEN_WARN,
+ .len = sizeof(struct nl80211_txrate_vht),
+ },
+ [NL80211_TXRATE_GI] = { .type = NLA_U8 },
+};
+
static const struct nla_policy
nl80211_tid_config_attr_policy[NL80211_TID_CONFIG_ATTR_MAX + 1] = {
[NL80211_TID_CONFIG_ATTR_TID] = { .type = NLA_U8 },
@@ -333,6 +345,10 @@ static int validate_ie_attr(const struct nlattr *attr,
NLA_POLICY_MAX(NLA_U8, NL80211_TID_CONFIG_DISABLE),
[NL80211_TID_CONFIG_ATTR_RTSCTS_CTRL] =
NLA_POLICY_MAX(NLA_U8, NL80211_TID_CONFIG_DISABLE),
+ [NL80211_TID_CONFIG_ATTR_TX_RATES_TYPE] =
+ NLA_POLICY_MAX(NLA_U8, NL80211_TX_RATE_FIXED),
+ [NL80211_TID_CONFIG_ATTR_TX_RATES] =
+ NLA_POLICY_NESTED(nl80211_txattr_policy),
};
const struct nla_policy nl80211_policy[NUM_NL80211_ATTR] = {
@@ -4240,19 +4256,9 @@ static bool vht_set_mcs_mask(struct ieee80211_supported_band *sband,
return true;
}
-static const struct nla_policy nl80211_txattr_policy[NL80211_TXRATE_MAX + 1] = {
- [NL80211_TXRATE_LEGACY] = { .type = NLA_BINARY,
- .len = NL80211_MAX_SUPP_RATES },
- [NL80211_TXRATE_HT] = { .type = NLA_BINARY,
- .len = NL80211_MAX_SUPP_HT_RATES },
- [NL80211_TXRATE_VHT] = {
- .type = NLA_EXACT_LEN_WARN,
- .len = sizeof(struct nl80211_txrate_vht),
- },
- [NL80211_TXRATE_GI] = { .type = NLA_U8 },
-};
-
static int nl80211_parse_tx_bitrate_mask(struct genl_info *info,
+ struct nlattr *attrs[],
+ enum nl80211_attrs attr,
struct cfg80211_bitrate_mask *mask)
{
struct nlattr *tb[NL80211_TXRATE_MAX + 1];
@@ -4283,14 +4289,14 @@ static int nl80211_parse_tx_bitrate_mask(struct genl_info *info,
}
/* if no rates are given set it back to the defaults */
- if (!info->attrs[NL80211_ATTR_TX_RATES])
+ if (!attrs[attr])
goto out;
/* The nested attribute uses enum nl80211_band as the index. This maps
* directly to the enum nl80211_band values used in cfg80211.
*/
BUILD_BUG_ON(NL80211_MAX_SUPP_HT_RATES > IEEE80211_HT_MCS_MASK_LEN * 8);
- nla_for_each_nested(tx_rates, info->attrs[NL80211_ATTR_TX_RATES], rem) {
+ nla_for_each_nested(tx_rates, attrs[attr], rem) {
enum nl80211_band band = nla_type(tx_rates);
int err;
@@ -4766,7 +4772,9 @@ static int nl80211_start_ap(struct sk_buff *skb, struct genl_info *info)
return -EINVAL;
if (info->attrs[NL80211_ATTR_TX_RATES]) {
- err = nl80211_parse_tx_bitrate_mask(info, ¶ms.beacon_rate);
+ err = nl80211_parse_tx_bitrate_mask(info, info->attrs,
+ NL80211_ATTR_TX_RATES,
+ ¶ms.beacon_rate);
if (err)
return err;
@@ -10510,7 +10518,8 @@ static int nl80211_set_tx_bitrate_mask(struct sk_buff *skb,
if (!rdev->ops->set_bitrate_mask)
return -EOPNOTSUPP;
- err = nl80211_parse_tx_bitrate_mask(info, &mask);
+ err = nl80211_parse_tx_bitrate_mask(info, info->attrs,
+ NL80211_ATTR_TX_RATES, &mask);
if (err)
return err;
@@ -11105,7 +11114,9 @@ static int nl80211_join_mesh(struct sk_buff *skb, struct genl_info *info)
}
if (info->attrs[NL80211_ATTR_TX_RATES]) {
- err = nl80211_parse_tx_bitrate_mask(info, &setup.beacon_rate);
+ err = nl80211_parse_tx_bitrate_mask(info, info->attrs,
+ NL80211_ATTR_TX_RATES,
+ &setup.beacon_rate);
if (err)
return err;
@@ -13914,6 +13925,40 @@ static int parse_tid_conf(struct cfg80211_registered_device *rdev,
nla_get_u8(attrs[NL80211_TID_CONFIG_ATTR_RTSCTS_CTRL]);
}
+ if (attrs[NL80211_TID_CONFIG_ATTR_TX_RATES_TYPE]) {
+ int idx;
+ enum nl80211_attrs attr;
+
+ err = nl80211_check_tid_config_support(rdev, extack, peer,
+ attrs[NL80211_TID_CONFIG_ATTR_TX_RATES_TYPE],
+ NL80211_EXT_FEATURE_PER_TID_TX_BITRATE_MASK,
+ NL80211_EXT_FEATURE_PER_STA_TX_BITRATE_MASK);
+ if (err)
+ return err;
+
+ idx = NL80211_TID_CONFIG_ATTR_TX_RATES_TYPE;
+ tid_conf->txrate_type = nla_get_u8(attrs[idx]);
+
+ tid_conf->tid_conf_mask |= IEEE80211_TID_CONF_TX_BITRATE;
+ if (tid_conf->txrate_type != NL80211_TX_RATE_AUTOMATIC) {
+ tid_conf->mask =
+ kzalloc(sizeof(struct cfg80211_bitrate_mask),
+ GFP_KERNEL);
+ if (!tid_conf->mask)
+ return -ENOMEM;
+
+ attr = (enum nl80211_attrs) NL80211_TID_CONFIG_ATTR_TX_RATES;
+ err = nl80211_parse_tx_bitrate_mask(info, attrs, attr,
+ tid_conf->mask);
+ if (err) {
+ kfree(tid_conf->mask);
+ return err;
+ }
+ } else {
+ tid_conf->mask = NULL;
+ }
+ }
+
return 0;
}
--
1.7.9.5
Implement drv_set_tid_config api to allow TID specific
configuration. This per-TID configuration
will be applied for all the connected stations when MAC is NULL.
Signed-off-by: Tamizh chelvam <[email protected]>
---
include/net/mac80211.h | 8 ++++++++
net/mac80211/cfg.c | 28 ++++++++++++++++++++++++++++
net/mac80211/driver-ops.h | 15 +++++++++++++++
3 files changed, 51 insertions(+)
diff --git a/include/net/mac80211.h b/include/net/mac80211.h
index f599696..017bd44 100644
--- a/include/net/mac80211.h
+++ b/include/net/mac80211.h
@@ -3756,6 +3756,10 @@ enum ieee80211_reconfig_type {
*
* @start_pmsr: start peer measurement (e.g. FTM) (this call can sleep)
* @abort_pmsr: abort peer measurement (this call can sleep)
+ * @set_tid_config: TID specific configurations will be applied for a particular
+ * station when @sta is non-NULL. When @sta is %NULL, then 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,
@@ -4060,6 +4064,10 @@ struct ieee80211_ops {
struct cfg80211_pmsr_request *request);
void (*abort_pmsr)(struct ieee80211_hw *hw, struct ieee80211_vif *vif,
struct cfg80211_pmsr_request *request);
+ int (*set_tid_config)(struct ieee80211_hw *hw,
+ struct ieee80211_vif *vif,
+ struct ieee80211_sta *sta,
+ struct ieee80211_tid_config *tid_conf);
};
/**
diff --git a/net/mac80211/cfg.c b/net/mac80211/cfg.c
index 70739e7..22aa640 100644
--- a/net/mac80211/cfg.c
+++ b/net/mac80211/cfg.c
@@ -3942,6 +3942,33 @@ static int ieee80211_get_txq_stats(struct wiphy *wiphy,
return drv_abort_pmsr(local, sdata, request);
}
+static int ieee80211_set_tid_config(struct wiphy *wiphy,
+ struct net_device *dev,
+ struct ieee80211_tid_config *tid_conf)
+{
+ struct ieee80211_sub_if_data *sdata = IEEE80211_DEV_TO_SUB_IF(dev);
+ struct sta_info *sta;
+ int ret;
+
+ if (!sdata->local->ops->set_tid_config)
+ return -EOPNOTSUPP;
+
+ if (!tid_conf->peer)
+ return drv_set_tid_config(sdata->local, sdata, NULL, tid_conf);
+
+ mutex_lock(&sdata->local->sta_mtx);
+
+ sta = sta_info_get_bss(sdata, tid_conf->peer);
+ if (!sta) {
+ mutex_unlock(&sdata->local->sta_mtx);
+ return -ENOENT;
+ }
+
+ ret = drv_set_tid_config(sdata->local, sdata, &sta->sta, tid_conf);
+ 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,
@@ -4040,4 +4067,5 @@ static int ieee80211_get_txq_stats(struct wiphy *wiphy,
.start_pmsr = ieee80211_start_pmsr,
.abort_pmsr = ieee80211_abort_pmsr,
.probe_mesh_link = ieee80211_probe_mesh_link,
+ .set_tid_config = ieee80211_set_tid_config,
};
diff --git a/net/mac80211/driver-ops.h b/net/mac80211/driver-ops.h
index 2c9b3eb8..c4954c7 100644
--- a/net/mac80211/driver-ops.h
+++ b/net/mac80211/driver-ops.h
@@ -1358,4 +1358,19 @@ static inline void drv_del_nan_func(struct ieee80211_local *local,
trace_drv_return_void(local);
}
+static inline int drv_set_tid_config(struct ieee80211_local *local,
+ struct ieee80211_sub_if_data *sdata,
+ struct ieee80211_sta *sta,
+ struct ieee80211_tid_config *tid_conf)
+{
+ int ret;
+
+ might_sleep();
+ ret = local->ops->set_tid_config(&local->hw, &sdata->vif, sta,
+ tid_conf);
+ trace_drv_return_int(local, ret);
+
+ return ret;
+}
+
#endif /* __MAC80211_DRIVER_OPS */
--
1.7.9.5
> Add infrastructure to support per TID configurations like noack policy,
> retry count, AMPDU control(disable/enable), RTSCTS control(enable/disable)
> and TX rate mask configurations.
> This will be useful for the driver which can supports data TID
> specific configuration rather than phy level configurations.
> Here NL80211_CMD_SET_TID_CONFIG added to support this operation by
> accepting TID configuration.
> This command can accept STA mac addreess to make the configuration
> station specific rather than applying to all the connected stations
> to the netdev.
> And this nested command configuration can accept multiple number of
> data TID specific configuration in a single command,
> enum ieee80211_tid_conf_mask used to notify the driver that which
> configuration got modified for the TID.
>
> Tamizh chelvam (6):
> nl80211: New netlink command for TID specific configuration
> nl80211: Add new netlink attribute for TID speicific retry count
> nl80211: Add netlink attribute for AMPDU aggregation enable/disable
> nl80211: Add netlink attribute to enable/disable RTS_CTS
> nl80211: Add netlink attribute to configure TID specific tx rate
> mac80211: Add api to support configuring TID specific configuration
>
> include/net/cfg80211.h | 55 +++++++++
> include/net/mac80211.h | 8 ++
> include/uapi/linux/nl80211.h | 190 +++++++++++++++++++++++++++++
> net/mac80211/cfg.c | 28 +++++
> net/mac80211/driver-ops.h | 15 +++
> net/wireless/nl80211.c | 276 +++++++++++++++++++++++++++++++++++++++---
> net/wireless/rdev-ops.h | 12 ++
> net/wireless/trace.h | 17 +++
> 8 files changed, 584 insertions(+), 17 deletions(-)
Hi Tamizh, Johannes, and all,
Looks very good to me:
Reviewed-by: Sergey Matyukevich <[email protected]>
BTW, there are two open questions remaining from the previous reviews:
- NL80211_TX_RATE_LIMITED and NL80211_TX_RATE_FIXED
Interpretation and validation of these two rate options is left
up to drivers.
- 'apply to all TIDs' usecase
Currently, if peer is not specified, then configuration is applied to
all the connected STAs. It is tempting to use some spare TID value
to inform drivers that provided configuration should be applied to
all TIDs of the specified STA or even to all TIDS and STAs. But that
can not be left up to drivers since this value needs to be passed
from userspace tools, e.g. from iw.
IIUC, the first question could be addressed later, after we see some
actual users and figure out generic use-cases. But what about the
second question ? Maybe it worth to add and document a single define,
e.g. using TID value 0xff, that can be used between userspace
and drivers for such usecases ?
Regards,
Sergey
Hi Sergey,
Thanks for looking!
> BTW, there are two open questions remaining from the previous reviews:
>
> - NL80211_TX_RATE_LIMITED and NL80211_TX_RATE_FIXED
> Interpretation and validation of these two rate options is left
> up to drivers.
(need to look at this still)
> - 'apply to all TIDs' usecase
> Currently, if peer is not specified, then configuration is applied to
> all the connected STAs. It is tempting to use some spare TID value
> to inform drivers that provided configuration should be applied to
> all TIDs of the specified STA or even to all TIDS and STAs. But that
> can not be left up to drivers since this value needs to be passed
> from userspace tools, e.g. from iw.
I was *just* replying on exactly the same point over in patch 1 (not
sent yet). It's actually not even clear to me that the configuration
really would be applied to *all* STAs, it's sort of left open for the
driver, afaict?
But I agree with you that this is not a good thing.
I don't think using a spare TID value is the right signalling, we can
add another attribute? E.g. we could easily add
NL80211_TID_CONFIG_ATTR_OVERRIDE
and make that be
@NL80211_TID_CONFIG_ATTR_OVERRIDE: flag attribute, valid only if no STA
is selected, if set indicates that the new configuration
overrides all previous STA configurations, otherwise previous
STA-specific configurations should be left untouched
You also raise a good point wrt. "all TIDs" - but then we should
probably just remove NL80211_TID_CONFIG_ATTR_TID and add a new
NL80211_TID_CONFIG_ATTR_TIDS as a bitmap? OTOH, it's not hard to just
explicitly spell out all TIDs either, I guess, just makes the message a
bit bigger.
johannes
Hi,
I was tempted to fix this up myself and finally get it over with, but
then I thought it's probably better if I don't - it's a lot of things.
> Add a new NL command, NL80211_CMD_SET_TID_CONFIG to support
> data TID specific configuration. This per TID configurations
> are passed in NL80211_TID_CONFIG_ATTR_TID which is a nested
> attribute. This patch adds support to configure per TID
> noack policy through NL80211_TID_CONFIG_ATTR_NOACK attribute.
> Data TID value for this configuration will be passed through
> NL80211_TID_CONFIG_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. This patch introduced
> enum ieee80211_tid_conf_mask to notify the driver that which
> configuration modified.
> Driver supporting data TID specific noack policy configuration
> should be advertise through NL80211_EXT_FEATURE_PER_TID_NOACK_CONFIG
> and supporting per STA data TID noack policy configuration
> should be advertise through NL80211_EXT_FEATURE_PER_STA_NOACK_CONFIG
This is just a wall of text, it's also wrong in some places. I was going
to replace it with this:
nl80211: add command for TID specific configuration
Add the new NL80211_CMD_SET_TID_CONFIG command to support
data TID specific configuration. Per TID configuration is
passed in the nested NL80211_ATTR_TID_CONFIG attribute.
This patch adds support to configure per TID noack policy
through the NL80211_TID_CONFIG_ATTR_NOACK attribute.
Configuration can be STA-specific (if supported) or for
all stations on a given interface if no STA is selected.
Two new feature flags are added:
* NL80211_EXT_FEATURE_PER_TID_NOACK_CONFIG for noack, and
* NL80211_EXT_FEATURE_PER_STA_NOACK_CONFIG for STA-specific
noack support.
which IMHO makes more sense. Can you rewrite it? Feel free to copy this
I guess.
> +enum ieee80211_tid_conf_mask {
> + IEEE80211_TID_CONF_NOACK = BIT(0),
> +};
Why not remove this and use BIT(NL80211_TID_CONFIG_ATTR_NOACK)?
> + * @tid_conf_mask: bitmap indicating which parameter changed
> + * see %enum ieee80211_tid_conf_mask
Either way use &enum so you get a link.
> + * @noack: noack configuration value for the TID
Should be enum nl80211_tid_config not u8?
> @@ -3625,6 +3654,10 @@ struct cfg80211_update_owe_info {
> *
> * @probe_mesh_link: Probe direct Mesh peer's link quality by sending data frame
> * and overrule HWMP path selection algorithm.
> + * @set_tid_config: TID specific configuration. Apply this configuration for
> + * all the connected stations in the BSS if peer is %NULL. Otherwise
> + * apply this configuration to the specific station.
> + * This callback may sleep.
This should document the stuff discussed with Sergey, whatever we decide
there.
> + * @NL80211_CMD_SET_TID_CONFIG: Data frame TID specific configuration
> + * is passed through this command using %NL80211_ATTR_TID_CONFIG
to not through
> + * nested attributes.
Using the %NL80211_ATTR_TID_CONFIG attribute.
> + * @NL80211_ATTR_TID_CONFIG: TID specific configuration in a
> + * nested attribute with %NL80211_TID_CONFIG_ATTR_* sub-attributes.
Would be worthwhile to link to the enum (&enum nl80211_tid_config_attr)
instead of giving the %NL80211_... thing.
> /**
> + * enum nl80211_tid_config - TID config state
> + * @NL80211_TID_CONFIG_DEFAULT: Default config for the TID. This is to
> + * notify driver to reset the previous config and use vif specific
> + * default config
> + * @NL80211_TID_CONFIG_ENABLE: Enable config for the TID
> + * NL80211_TID_CONFIG_DISABLE: Disable config for the TID
missing @
I'm still a bit on the fence on this.
How about we add an explicit "reset" API instead? So you could say
set_tid(sta=..., tid=7, reset=BIT(NL80211_TID_CONFIG_ATTR_NOACK) or so?
(or IEEE80211_TID_CONF_NOACK if we prefer that, not sure)
Just throwing that out here for ideas, not for you to go implement it
right now :)
Then we don't really need NL80211_TID_CONFIG_DEFAULT. We still need
ENABLE/DISABLE to allow "leave unchanged", though I'd probably formulate
that a bit more generic than anything with "TID" in the name then.
That would also address something you can't do now - you cannot reset
the TX rate to the default this way, i.e. removing the STA override for
a TX rate isn't possible.
> + * @NL80211_TID_CONFIG_ATTR_TID: a TID value (u8 attribute).
> + * @NL80211_TID_CONFIG_ATTR_NOACK: Configure ack policy for the TID.
> + * specified in %NL80211_TID_CONFIG_ATTR_TID. see %enum nl80211_tid_config.
> + * Its type is u8, if the peer MAC address is passed in %NL80211_ATTR_MAC,
> + * then the noack configuration is applied to the data frame for the tid
> + * to that connected station. This 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, then 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_TID_NOACK_CONFIG and
> + * NL80211_EXT_FEATURE_PER_STA_NOACK_CONFIG for supporting per sta
> + * configuration.
Most of this text should be somewhere else, in a general text about the
TID-specific configuration, in particular all the stuff about the
lifetime etc. You can put it into the method documentation, but it might
be better to just add a "DOC:" section explaining it all.
> + if (!wiphy_ext_feature_isset(&rdev->wiphy, per_tid_config)) {
> + NL_SET_ERR_MSG_ATTR(extack, attr, "TID specific configuration not supported");
please add linebreaks - it'll still be longer than 80 cols, but at least
if you break after "attr," it's not *that* long.
> + return -ENOTSUPP;
> + }
> +
there's a stray space here
> + if (peer && !wiphy_ext_feature_isset(&rdev->wiphy, per_sta_config)) {
> + NL_SET_ERR_MSG_ATTR(extack, attr, "peer specific TID configuration not supported");
same as above
> + tid_conf->tid = nla_get_u8(attrs[NL80211_TID_CONFIG_ATTR_TID]);
> + if (attrs[NL80211_TID_CONFIG_ATTR_NOACK]) {
> + err = nl80211_check_tid_config_support(rdev, extack, peer,
> + attrs[NL80211_TID_CONFIG_ATTR_NOACK],
> + NL80211_EXT_FEATURE_PER_TID_NOACK_CONFIG,
> + NL80211_EXT_FEATURE_PER_STA_NOACK_CONFIG);
> + if (err)
> + return err;
> +
> + tid_conf->tid_conf_mask |= IEEE80211_TID_CONF_NOACK;
If you change the tid_conf_mask to be BIT() you can roll that |= into
the nl80211_check_tid_config_support() function, which seems nicer?
I might go as far as suggesting a wrapper macro for this that lets you
save typing on the whole "NL80211_*" prefixes, so you just have
err = nl80211_check_tid_config_supoprt(rdev, extack, peer, attrs,
NOACK);
and the other arguments are generated by the macro based on the NOACK
portion. That'll save a lot of typing in the next patches ...
> + tid_conf->noack =
> + nla_get_u8(attrs[NL80211_TID_CONFIG_ATTR_NOACK]);
Maybe even roll that in by having some kind of function pointer, like
the mesh code did? But that's harder, might not be worth it. But if you
do then you can also roll in the "if (attr[])" presence check and make
this a lot more streamlined.
> + if (!attrs[NL80211_TID_CONFIG_ATTR_TID]) {
> + ret = -EINVAL;
> + goto bad_tid_conf;
> + }
Maybe move that into the function:
> + ret = parse_tid_conf(rdev, attrs,
> + &tid_config->tid_conf[conf_idx],
> + info, tid_config->peer);
johannes
> + * 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.
Here you pretty much copy-pasted all this text ... that's why I think it
should be in some other section. We want *everything* to be like that,
not have to check every single thing for different validity rules.
> + * @NL80211_TID_CONFIG_ATTR_RETRY_SHORT: Number of retries used with data frame
> + * transmission, user-space sets this configuration in
> + * &NL80211_CMD_SET_TID_CONFIG. It is u8 type, 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_TID_CONFIG_ATTR_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.
I'm almost thinking that these should be a struct with two u8 values
instead of two separate attributes, and then renamed to
NL80211_TID_CONFIG_ATTR_RETRY, to carry both and really ensure thaty
they're always together as a single configuration.
This only really works right if we go for the reset= approach I outlined
in the previous patch though, since you otherwise need
NL80211_TID_CONFIG_ATTR_RETRY for the reset ... but that's a pretty
weird thing.
(there are also some typos here like "notfiy")
> --- a/net/wireless/nl80211.c
> +++ b/net/wireless/nl80211.c
> @@ -326,6 +326,9 @@ static int validate_ie_attr(const struct nlattr *attr,
> [NL80211_TID_CONFIG_ATTR_TID] = { .type = NLA_U8 },
> [NL80211_TID_CONFIG_ATTR_NOACK] =
> NLA_POLICY_MAX(NLA_U8, NL80211_TID_CONFIG_DISABLE),
> + [NL80211_TID_CONFIG_ATTR_RETRY] = { .type = NLA_FLAG },
> + [NL80211_TID_CONFIG_ATTR_RETRY_SHORT] = { .type = NLA_U8},
> + [NL80211_TID_CONFIG_ATTR_RETRY_LONG] = { .type = NLA_U8},
The min value of 1 should be reflected in the policy.
> + if (rdev->wiphy.max_data_retry_count) {
> + if (nla_put_u8(msg, NL80211_ATTR_MAX_RETRY_COUNT,
> + rdev->wiphy.max_data_retry_count))
bad indentation
> + goto nla_put_failure;
> + }
> +
> state->split_start++;
> if (state->split)
> break;
Also not sure which section you put this in, but it looks almost like
it's under "case 1:" where it really shouldn't be ... move it to the end
please.
johannes
> > - 'apply to all TIDs' usecase
> > Currently, if peer is not specified, then configuration is applied to
> > all the connected STAs. It is tempting to use some spare TID value
> > to inform drivers that provided configuration should be applied to
> > all TIDs of the specified STA or even to all TIDS and STAs. But that
> > can not be left up to drivers since this value needs to be passed
> > from userspace tools, e.g. from iw.
>
> I was *just* replying on exactly the same point over in patch 1 (not
> sent yet). It's actually not even clear to me that the configuration
> really would be applied to *all* STAs, it's sort of left open for the
> driver, afaict?
>
> But I agree with you that this is not a good thing.
>
> I don't think using a spare TID value is the right signalling, we can
> add another attribute? E.g. we could easily add
>
> NL80211_TID_CONFIG_ATTR_OVERRIDE
>
> and make that be
>
> @NL80211_TID_CONFIG_ATTR_OVERRIDE: flag attribute, valid only if no STA
> is selected, if set indicates that the new configuration
> overrides all previous STA configurations, otherwise previous
> STA-specific configurations should be left untouched
>
> You also raise a good point wrt. "all TIDs" - but then we should
> probably just remove NL80211_TID_CONFIG_ATTR_TID and add a new
> NL80211_TID_CONFIG_ATTR_TIDS as a bitmap? OTOH, it's not hard to just
> explicitly spell out all TIDs either, I guess, just makes the message a
> bit bigger.
The idea with bitmask for TIDs looks good. It eliminates the need for both
'all TIDs' define and additional attribute NL80211_TID_CONFIG_ATTR_OVERRIDE.
In fact, almost no changes are needed for the patch, mostly comments need
to be updated. Manual typing in iw will be less convenient, but I guess
this interface is not supposed to be used by humans after all...
Regards,
Sergey
On Fri, 2019-11-08 at 12:05 +0000, Sergey Matyukevich wrote:
> > @NL80211_TID_CONFIG_ATTR_OVERRIDE: flag attribute, valid only if no STA
> > is selected, if set indicates that the new configuration
> > overrides all previous STA configurations, otherwise previous
> > STA-specific configurations should be left untouched
> >
> > You also raise a good point wrt. "all TIDs" - but then we should
> > probably just remove NL80211_TID_CONFIG_ATTR_TID and add a new
> > NL80211_TID_CONFIG_ATTR_TIDS as a bitmap? OTOH, it's not hard to just
> > explicitly spell out all TIDs either, I guess, just makes the message a
> > bit bigger.
>
> The idea with bitmask for TIDs looks good. It eliminates the need for both
> 'all TIDs' define and additional attribute NL80211_TID_CONFIG_ATTR_OVERRIDE.
I think we still need NL80211_TID_CONFIG_ATTR_OVERRIDE in some way
(maybe only as a flag attribute), since you could have
* change all stations (some subset of TIDs) *including* already
configured stations
* or *excluding* already configured stations
> In fact, almost no changes are needed for the patch, mostly comments need
> to be updated. Manual typing in iw will be less convenient, but I guess
> this interface is not supposed to be used by humans after all...
If that's a concern we can parse a list in iw, e.g. "0,1,2,3" instaed of
"0xf", right?
johannes
> > > @NL80211_TID_CONFIG_ATTR_OVERRIDE: flag attribute, valid only if no STA
> > > is selected, if set indicates that the new configuration
> > > overrides all previous STA configurations, otherwise previous
> > > STA-specific configurations should be left untouched
> > >
> > > You also raise a good point wrt. "all TIDs" - but then we should
> > > probably just remove NL80211_TID_CONFIG_ATTR_TID and add a new
> > > NL80211_TID_CONFIG_ATTR_TIDS as a bitmap? OTOH, it's not hard to just
> > > explicitly spell out all TIDs either, I guess, just makes the message a
> > > bit bigger.
> >
> > The idea with bitmask for TIDs looks good. It eliminates the need for both
> > 'all TIDs' define and additional attribute NL80211_TID_CONFIG_ATTR_OVERRIDE.
>
> I think we still need NL80211_TID_CONFIG_ATTR_OVERRIDE in some way
> (maybe only as a flag attribute), since you could have
>
> * change all stations (some subset of TIDs) *including* already
> configured stations
> * or *excluding* already configured stations
Hmmm... Logic is straightforwad without this flag:
- settings are applied to bitmasked TIDs of a single peer if address is specifed
- settings are applied to bitmasked TIDs of all the peers if no address is specified
It looks like you want to infer too much from a single flag. Why keep this logic in
cfg80211/mac80211/driver ?
> > In fact, almost no changes are needed for the patch, mostly comments need
> > to be updated. Manual typing in iw will be less convenient, but I guess
> > this interface is not supposed to be used by humans after all...
>
> If that's a concern we can parse a list in iw, e.g. "0,1,2,3" instaed of
> "0xf", right?
Oh, right, it can be that simple. But this is by no means a concern.
Regards,
Sergey
On Fri, 2019-11-08 at 16:01 +0000, Sergey Matyukevich wrote:
> > I think we still need NL80211_TID_CONFIG_ATTR_OVERRIDE in some way
> > (maybe only as a flag attribute), since you could have
> >
> > * change all stations (some subset of TIDs) *including* already
> > configured stations
> > * or *excluding* already configured stations
>
> Hmmm... Logic is straightforwad without this flag:
> - settings are applied to bitmasked TIDs of a single peer if address is specifed
> - settings are applied to bitmasked TIDs of all the peers if no address is specified
Sure, this is obvious, but what exactly does "all the peers" mean?
Say I do
set_tid_config(tids=0x1, peer=02:11:22:33:44:55, noack=yes)
set_tid_config(tids=0x1, peer=NULL, noack=no)
Does that reset peer 02:11:22:33:44:55, or not? This is not documented
right now, and one could argue both ways - the override for that
particular peer should stick, or should be removed. Which one is it?
> It looks like you want to infer too much from a single flag. Why keep this logic in
> cfg80211/mac80211/driver ?
I just want to disambiguate what "all the peers" means. Not sure what
you mean by keeping the logic?
johannes
> > > I think we still need NL80211_TID_CONFIG_ATTR_OVERRIDE in some way
> > > (maybe only as a flag attribute), since you could have
> > >
> > > * change all stations (some subset of TIDs) *including* already
> > > configured stations
> > > * or *excluding* already configured stations
> >
> > Hmmm... Logic is straightforwad without this flag:
> > - settings are applied to bitmasked TIDs of a single peer if address is specifed
> > - settings are applied to bitmasked TIDs of all the peers if no address is specified
>
> Sure, this is obvious, but what exactly does "all the peers" mean?
>
> Say I do
>
> set_tid_config(tids=0x1, peer=02:11:22:33:44:55, noack=yes)
> set_tid_config(tids=0x1, peer=NULL, noack=no)
>
> Does that reset peer 02:11:22:33:44:55, or not? This is not documented
> right now, and one could argue both ways - the override for that
> particular peer should stick, or should be removed. Which one is it?
Ok, I got the point. My understanding was that any further command would rewrite
the previous settings. But now I agree that this is not obvious and should be
explicitly documented.
Sergey
Hi Johannes/Sergey,
Thanks for your review comments
> Hi,
>
> I was tempted to fix this up myself and finally get it over with, but
> then I thought it's probably better if I don't - it's a lot of things.
>
>> Add a new NL command, NL80211_CMD_SET_TID_CONFIG to support
>> data TID specific configuration. This per TID configurations
>> are passed in NL80211_TID_CONFIG_ATTR_TID which is a nested
>> attribute. This patch adds support to configure per TID
>> noack policy through NL80211_TID_CONFIG_ATTR_NOACK attribute.
>> Data TID value for this configuration will be passed through
>> NL80211_TID_CONFIG_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. This patch introduced
>> enum ieee80211_tid_conf_mask to notify the driver that which
>> configuration modified.
>> Driver supporting data TID specific noack policy configuration
>> should be advertise through NL80211_EXT_FEATURE_PER_TID_NOACK_CONFIG
>> and supporting per STA data TID noack policy configuration
>> should be advertise through NL80211_EXT_FEATURE_PER_STA_NOACK_CONFIG
>
> This is just a wall of text, it's also wrong in some places. I was
> going
> to replace it with this:
>
> nl80211: add command for TID specific configuration
>
> Add the new NL80211_CMD_SET_TID_CONFIG command to support
> data TID specific configuration. Per TID configuration is
> passed in the nested NL80211_ATTR_TID_CONFIG attribute.
>
> This patch adds support to configure per TID noack policy
> through the NL80211_TID_CONFIG_ATTR_NOACK attribute.
>
> Configuration can be STA-specific (if supported) or for
> all stations on a given interface if no STA is selected.
>
> Two new feature flags are added:
> * NL80211_EXT_FEATURE_PER_TID_NOACK_CONFIG for noack, and
> * NL80211_EXT_FEATURE_PER_STA_NOACK_CONFIG for STA-specific
> noack support.
>
> which IMHO makes more sense. Can you rewrite it? Feel free to copy this
> I guess.
>
Sure, I will use this.
>> +enum ieee80211_tid_conf_mask {
>> + IEEE80211_TID_CONF_NOACK = BIT(0),
>> +};
>
> Why not remove this and use BIT(NL80211_TID_CONFIG_ATTR_NOACK)?
>
Sure.
>> + * @tid_conf_mask: bitmap indicating which parameter changed
>> + * see %enum ieee80211_tid_conf_mask
>
> Either way use &enum so you get a link.
>
Sure.
>> + * @noack: noack configuration value for the TID
>
> Should be enum nl80211_tid_config not u8?
>
Yes, I will change it.
>> @@ -3625,6 +3654,10 @@ struct cfg80211_update_owe_info {
>> *
>> * @probe_mesh_link: Probe direct Mesh peer's link quality by sending
>> data frame
>> * and overrule HWMP path selection algorithm.
>> + * @set_tid_config: TID specific configuration. Apply this
>> configuration for
>> + * all the connected stations in the BSS if peer is %NULL. Otherwise
>> + * apply this configuration to the specific station.
>> + * This callback may sleep.
>
> This should document the stuff discussed with Sergey, whatever we
> decide
> there.
Sure.
>
>> + * @NL80211_CMD_SET_TID_CONFIG: Data frame TID specific configuration
>> + * is passed through this command using %NL80211_ATTR_TID_CONFIG
>
> to not through
>
>> + * nested attributes.
>
> Using the %NL80211_ATTR_TID_CONFIG attribute.
>
Sure.
>> + * @NL80211_ATTR_TID_CONFIG: TID specific configuration in a
>> + * nested attribute with %NL80211_TID_CONFIG_ATTR_* sub-attributes.
>
> Would be worthwhile to link to the enum (&enum nl80211_tid_config_attr)
> instead of giving the %NL80211_... thing.
>
Sure.
>> /**
>> + * enum nl80211_tid_config - TID config state
>> + * @NL80211_TID_CONFIG_DEFAULT: Default config for the TID. This is
>> to
>> + * notify driver to reset the previous config and use vif specific
>> + * default config
>> + * @NL80211_TID_CONFIG_ENABLE: Enable config for the TID
>> + * NL80211_TID_CONFIG_DISABLE: Disable config for the TID
>
> missing @
>
> I'm still a bit on the fence on this.
>
> How about we add an explicit "reset" API instead? So you could say
>
> set_tid(sta=..., tid=7, reset=BIT(NL80211_TID_CONFIG_ATTR_NOACK) or so?
>
> (or IEEE80211_TID_CONF_NOACK if we prefer that, not sure)
>
> Just throwing that out here for ideas, not for you to go implement it
> right now :)
>
>
> Then we don't really need NL80211_TID_CONFIG_DEFAULT. We still need
> ENABLE/DISABLE to allow "leave unchanged", though I'd probably
> formulate
> that a bit more generic than anything with "TID" in the name then.
>
> That would also address something you can't do now - you cannot reset
> the TX rate to the default this way, i.e. removing the STA override for
> a TX rate isn't possible.
>
I thought of resetting it through "NL80211_TX_RATE_AUTOMATIC". Are you
okay with that ?
I will document that in the next patchset.
>> + * @NL80211_TID_CONFIG_ATTR_TID: a TID value (u8 attribute).
>> + * @NL80211_TID_CONFIG_ATTR_NOACK: Configure ack policy for the TID.
>> + * specified in %NL80211_TID_CONFIG_ATTR_TID. see %enum
>> nl80211_tid_config.
>> + * Its type is u8, if the peer MAC address is passed in
>> %NL80211_ATTR_MAC,
>> + * then the noack configuration is applied to the data frame for the
>> tid
>> + * to that connected station. This 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, then 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_TID_NOACK_CONFIG and
>> + * NL80211_EXT_FEATURE_PER_STA_NOACK_CONFIG for supporting per sta
>> + * configuration.
>
> Most of this text should be somewhere else, in a general text about the
> TID-specific configuration, in particular all the stuff about the
> lifetime etc. You can put it into the method documentation, but it
> might
> be better to just add a "DOC:" section explaining it all.
>
Okay sure, I will reduce these texts in the commit log and add it to
DOC: section.
>> + if (!wiphy_ext_feature_isset(&rdev->wiphy, per_tid_config)) {
>> + NL_SET_ERR_MSG_ATTR(extack, attr, "TID specific configuration not
>> supported");
>
> please add linebreaks - it'll still be longer than 80 cols, but at
> least
> if you break after "attr," it's not *that* long.
>
Okay.
>> + return -ENOTSUPP;
>> + }
>> +
>
> there's a stray space here
>
Will fix this in next patchset.
>> + if (peer && !wiphy_ext_feature_isset(&rdev->wiphy, per_sta_config))
>> {
>> + NL_SET_ERR_MSG_ATTR(extack, attr, "peer specific TID configuration
>> not supported");
>
> same as above
>
>> + tid_conf->tid = nla_get_u8(attrs[NL80211_TID_CONFIG_ATTR_TID]);
>> + if (attrs[NL80211_TID_CONFIG_ATTR_NOACK]) {
>> + err = nl80211_check_tid_config_support(rdev, extack, peer,
>> + attrs[NL80211_TID_CONFIG_ATTR_NOACK],
>> + NL80211_EXT_FEATURE_PER_TID_NOACK_CONFIG,
>> + NL80211_EXT_FEATURE_PER_STA_NOACK_CONFIG);
>> + if (err)
>> + return err;
>> +
>> + tid_conf->tid_conf_mask |= IEEE80211_TID_CONF_NOACK;
>
> If you change the tid_conf_mask to be BIT() you can roll that |= into
> the nl80211_check_tid_config_support() function, which seems nicer?
>
> I might go as far as suggesting a wrapper macro for this that lets you
> save typing on the whole "NL80211_*" prefixes, so you just have
>
> err = nl80211_check_tid_config_supoprt(rdev, extack, peer, attrs,
> NOACK);
>
> and the other arguments are generated by the macro based on the NOACK
> portion. That'll save a lot of typing in the next patches ...
>
Sure, I will go with wrapper macro method.
>> + tid_conf->noack =
>> + nla_get_u8(attrs[NL80211_TID_CONFIG_ATTR_NOACK]);
>
> Maybe even roll that in by having some kind of function pointer, like
> the mesh code did? But that's harder, might not be worth it. But if you
> do then you can also roll in the "if (attr[])" presence check and make
> this a lot more streamlined.
>
>> + if (!attrs[NL80211_TID_CONFIG_ATTR_TID]) {
>> + ret = -EINVAL;
>> + goto bad_tid_conf;
>> + }
>
> Maybe move that into the function:
>
Sure.
>> + ret = parse_tid_conf(rdev, attrs,
>> + &tid_config->tid_conf[conf_idx],
>> + info, tid_config->peer);
>
Tamizh.
On 2019-11-08 15:30, Johannes Berg wrote:
>> + * 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.
>
> Here you pretty much copy-pasted all this text ... that's why I think
> it
> should be in some other section. We want *everything* to be like that,
> not have to check every single thing for different validity rules.
>
Sure, I will add these things in DOC: section.
>> + * @NL80211_TID_CONFIG_ATTR_RETRY_SHORT: Number of retries used with
>> data frame
>> + * transmission, user-space sets this configuration in
>> + * &NL80211_CMD_SET_TID_CONFIG. It is u8 type, 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_TID_CONFIG_ATTR_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.
>
> I'm almost thinking that these should be a struct with two u8 values
> instead of two separate attributes, and then renamed to
> NL80211_TID_CONFIG_ATTR_RETRY, to carry both and really ensure thaty
> they're always together as a single configuration.
>
This will make mandatory for user to send both values know ? I have did
it similar to
NL80211_ATTR_WIPHY_RETRY_SHORT and NL80211_ATTR_WIPHY_RETRY_LONG. This
way we can have
option to configure single parameter know ?
> This only really works right if we go for the reset= approach I
> outlined
> in the previous patch though, since you otherwise need
> NL80211_TID_CONFIG_ATTR_RETRY for the reset ... but that's a pretty
> weird thing.
>
> (there are also some typos here like "notfiy")
>
I will fix this.
>> --- a/net/wireless/nl80211.c
>> +++ b/net/wireless/nl80211.c
>> @@ -326,6 +326,9 @@ static int validate_ie_attr(const struct nlattr
>> *attr,
>> [NL80211_TID_CONFIG_ATTR_TID] = { .type = NLA_U8 },
>> [NL80211_TID_CONFIG_ATTR_NOACK] =
>> NLA_POLICY_MAX(NLA_U8, NL80211_TID_CONFIG_DISABLE),
>> + [NL80211_TID_CONFIG_ATTR_RETRY] = { .type = NLA_FLAG },
>> + [NL80211_TID_CONFIG_ATTR_RETRY_SHORT] = { .type = NLA_U8},
>> + [NL80211_TID_CONFIG_ATTR_RETRY_LONG] = { .type = NLA_U8},
>
> The min value of 1 should be reflected in the policy.
>
Yeah, It was there in the previous patchset and removed due to
confusion.
Do you want to keep MIN value of 1 policy ?
>> + if (rdev->wiphy.max_data_retry_count) {
>> + if (nla_put_u8(msg, NL80211_ATTR_MAX_RETRY_COUNT,
>> + rdev->wiphy.max_data_retry_count))
>
> bad indentation
I will fix this.
>
>> + goto nla_put_failure;
>> + }
>> +
>> state->split_start++;
>> if (state->split)
>> break;
>
> Also not sure which section you put this in, but it looks almost like
> it's under "case 1:" where it really shouldn't be ... move it to the
> end
> please.
>
Sure.
Thanks,
Tamizh.
On 2019-11-08 22:37, Johannes Berg wrote:
> On Fri, 2019-11-08 at 16:01 +0000, Sergey Matyukevich wrote:
>
>> > I think we still need NL80211_TID_CONFIG_ATTR_OVERRIDE in some way
>> > (maybe only as a flag attribute), since you could have
>> >
>> > * change all stations (some subset of TIDs) *including* already
>> > configured stations
>> > * or *excluding* already configured stations
>>
>> Hmmm... Logic is straightforwad without this flag:
>> - settings are applied to bitmasked TIDs of a single peer if address
>> is specifed
>> - settings are applied to bitmasked TIDs of all the peers if no
>> address is specified
>
> Sure, this is obvious, but what exactly does "all the peers" mean?
>
> Say I do
>
> set_tid_config(tids=0x1, peer=02:11:22:33:44:55, noack=yes)
> set_tid_config(tids=0x1, peer=NULL, noack=no)
>
> Does that reset peer 02:11:22:33:44:55, or not? This is not documented
> right now, and one could argue both ways - the override for that
> particular peer should stick, or should be removed. Which one is it?
>
Here, the second command won't reset the peer 02:11:22:33:44:55. Here we
are giving more
preference to the peer specific configuration. We have to reset the peer
02:11:22:33:44:55 using the set_tid_config(tids=0x1,
peer=02:11:22:33:44:55, DEFAULT). I will add these in the DOC section
and send it in next patchset.
>> It looks like you want to infer too much from a single flag. Why keep
>> this logic in
>> cfg80211/mac80211/driver ?
>
> I just want to disambiguate what "all the peers" means. Not sure what
> you mean by keeping the logic?
>
Thanks,
Tamizh.
On Wed, 2019-11-13 at 21:30 +0530, Tamizh chelvam wrote:
> I thought of resetting it through "NL80211_TX_RATE_AUTOMATIC". Are you
> okay with that ?
"automatic" can also be a legitimate non-reset option though, right? If
all stations are fixed to 10 dBm, then setting one station to automatic
is still a difference?
I think I'd prefer this whole reset to be handled independent of the
type of value/attribute, like I described earlier with a "reset bitmap"
or so.
johannes
On Thu, 2019-11-14 at 12:50 +0530, Tamizh chelvam wrote:
>
> > I'm almost thinking that these should be a struct with two u8 values
> > instead of two separate attributes, and then renamed to
> > NL80211_TID_CONFIG_ATTR_RETRY, to carry both and really ensure thaty
> > they're always together as a single configuration.
> >
> This will make mandatory for user to send both values know ?
Yes.
> I have did
> it similar to
> NL80211_ATTR_WIPHY_RETRY_SHORT and NL80211_ATTR_WIPHY_RETRY_LONG. This
> way we can have
> option to configure single parameter know ?
Is it worth it? If you need them to be separate, then you should
actually separate them and let each one be managed per station with
override etc., but that seems a lot of effort?
> > > + [NL80211_TID_CONFIG_ATTR_RETRY] = { .type = NLA_FLAG },
> > > + [NL80211_TID_CONFIG_ATTR_RETRY_SHORT] = { .type = NLA_U8},
> > > + [NL80211_TID_CONFIG_ATTR_RETRY_LONG] = { .type = NLA_U8},
> >
> > The min value of 1 should be reflected in the policy.
> >
> Yeah, It was there in the previous patchset and removed due to
> confusion.
> Do you want to keep MIN value of 1 policy ?
If 0 is invalid, yeah, keep min 1 in the policy.
johannes
On Thu, 2019-11-14 at 13:02 +0530, Tamizh chelvam wrote:
> On 2019-11-08 22:37, Johannes Berg wrote:
> > On Fri, 2019-11-08 at 16:01 +0000, Sergey Matyukevich wrote:
> >
> > > > I think we still need NL80211_TID_CONFIG_ATTR_OVERRIDE in some way
> > > > (maybe only as a flag attribute), since you could have
> > > >
> > > > * change all stations (some subset of TIDs) *including* already
> > > > configured stations
> > > > * or *excluding* already configured stations
> > >
> > > Hmmm... Logic is straightforwad without this flag:
> > > - settings are applied to bitmasked TIDs of a single peer if address
> > > is specifed
> > > - settings are applied to bitmasked TIDs of all the peers if no
> > > address is specified
> >
> > Sure, this is obvious, but what exactly does "all the peers" mean?
> >
> > Say I do
> >
> > set_tid_config(tids=0x1, peer=02:11:22:33:44:55, noack=yes)
> > set_tid_config(tids=0x1, peer=NULL, noack=no)
> >
> > Does that reset peer 02:11:22:33:44:55, or not? This is not documented
> > right now, and one could argue both ways - the override for that
> > particular peer should stick, or should be removed. Which one is it?
> >
> Here, the second command won't reset the peer 02:11:22:33:44:55. Here we
> are giving more
> preference to the peer specific configuration. We have to reset the peer
> 02:11:22:33:44:55 using the set_tid_config(tids=0x1,
> peer=02:11:22:33:44:55, DEFAULT). I will add these in the DOC section
> and send it in next patchset.
OK, but maybe in some cases it _is_ desired to actually clear all peer-
specific overrides (somehow)?
johannes