Re: [PATCHv2 1/9] nl80211: New netlink command for TID specific configuration
Hi Tamizh,
> Hi Sergey,
> >
> > > Signed-off-by: Tamizh chelvam
> > > ---
> > > include/net/cfg80211.h | 35 +++
> > > include/uapi/linux/nl80211.h | 51 ++
> > > net/wireless/nl80211.c | 102
> > > +++
> > > net/wireless/rdev-ops.h | 11 +
> > > net/wireless/trace.h | 18
> > > 5 files changed, 217 insertions(+)
> >
> > ...
> >
> > > diff --git a/net/wireless/nl80211.c b/net/wireless/nl80211.c
> > > index 82d5e1e..352eb4a2 100644
> > > --- a/net/wireless/nl80211.c
> > > +++ b/net/wireless/nl80211.c
> > > @@ -280,6 +280,13 @@ static int validate_ie_attr(const struct nlattr
> > > *attr,
> > >
> > > NLA_POLICY_NESTED_ARRAY(nl80211_psmr_peer_attr_policy),
> > > };
> > >
> > > +static const struct nla_policy
> > > +nl80211_attr_tid_config_policy[NL80211_ATTR_TID_CONFIG_MAX + 1] = {
> > > + [NL80211_ATTR_TID_CONFIG_TID] = NLA_POLICY_MAX(NLA_U8, 7),
> >
> > Such a policy permits configuration of per-TID settings, either
> > for per-STA or for all the STAs. However it is not possible to
> > perform configuration for all TIDs at once, e.g. passing -1 value
> > to driver.
> >
> > Maybe simplify policy and use .type = NLA_U8 ?
> >
> > Sanity check, if needed, can be performed by driver or even by
> > firmware.
> >
> Sure, we can have like that. And do you feel driver should advertise
> support to perform configuration for all TIDs(like accepting tid -1) ?
Do you think this additional level of granularity is needed ?
IIUC if driver/firmware supports per TID feature configuration,
then can handle all-TIDs configuration as well. Anyway, attempt
for global feature configuration can be rejected on driver
or firmware level if needed.
> > > + [NL80211_ATTR_TID_CONFIG_NOACK] =
> > > + NLA_POLICY_MAX(NLA_U8,
> > > NL80211_TID_CONFIG_DISABLE),
> > > +};
> > > +
> > > const struct nla_policy nl80211_policy[NUM_NL80211_ATTR] = {
> > > [NL80211_ATTR_WIPHY] = { .type = NLA_U32 },
> > > [NL80211_ATTR_WIPHY_NAME] = { .type = NLA_NUL_STRING,
> > > @@ -541,6 +548,8 @@ static int validate_ie_attr(const struct nlattr
> > > *attr,
> > > [NL80211_ATTR_PEER_MEASUREMENTS] =
> > > NLA_POLICY_NESTED(nl80211_pmsr_attr_policy),
> > > [NL80211_ATTR_AIRTIME_WEIGHT] = NLA_POLICY_MIN(NLA_U16, 1),
> > > + [NL80211_ATTR_TID_CONFIG] =
> > > +
> > > NLA_POLICY_NESTED(nl80211_attr_tid_config_policy),
> > > };
> >
> > ...
> >
> > > +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_CONFIG_MAX + 1];
> > > + struct net_device *dev = info->user_ptr[1];
> > > + struct ieee80211_tid_config *tid_conf;
> > > + struct nlattr *tid;
> > > + int conf_idx = 0, rem_conf;
> > > + u32 num_conf = 0, size_of_conf;
> > > + int ret = -EINVAL;
> > > +
> > > + 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++;
> > > +
> > > + size_of_conf = sizeof(struct ieee80211_tid_config) +
> > > + num_conf * sizeof(struct ieee80211_tid_cfg);
> > > +
> > > + tid_conf = kzalloc(size_of_conf, GFP_KERNEL);
> > > + if (!tid_conf)
> > > + return -ENOMEM;
> > > +
> > > + tid_conf->n_tid_conf = num_conf;
> > > +
> > > + if (info->attrs[NL80211_ATTR_MAC])
> > > + tid_conf->peer =
> > > nla_data(info->attrs[NL80211_ATTR_MAC]);
> > > + else
> > > + tid_conf->peer = NULL;
> > > +
> > > + nla_for_each_nested(tid, info->attrs[NL80211_ATTR_TID_CONFIG],
> > > + rem_conf) {
> > > + ret = nla_parse_nested(attrs,
> > > NL80211_ATTR_TID_CONFIG_MAX, tid,
> > > + NULL, NULL);
> > > +
> > > + if (ret)
> > > + return ret;
> > > +
> > > + if (!attrs[NL80211_ATTR_TID_CONFIG_TID])
> > > + return -EINVAL;
> > > +
> > > + ret = parse_tid_conf(rdev, attrs,
> > > _conf->tid_conf[conf_idx],
> > > +tid_conf->peer);
> > > + if (ret)
> > > + goto bad_tid_conf;
> > > +
> > > + conf_idx++;
> > > + }
> > > +
> > > + return rdev_set_tid_config(rdev, dev, tid_conf);
> >
> > What is the ownership rule for tid_conf ? In other words, who is
> > responsible for freeing this structure ?
> >
> > Unless I am missing something, in the
Re: [PATCHv2 1/9] nl80211: New netlink command for TID specific configuration
Hi Sergey,
Signed-off-by: Tamizh chelvam
---
include/net/cfg80211.h | 35 +++
include/uapi/linux/nl80211.h | 51 ++
net/wireless/nl80211.c | 102
+++
net/wireless/rdev-ops.h | 11 +
net/wireless/trace.h | 18
5 files changed, 217 insertions(+)
...
diff --git a/net/wireless/nl80211.c b/net/wireless/nl80211.c
index 82d5e1e..352eb4a2 100644
--- a/net/wireless/nl80211.c
+++ b/net/wireless/nl80211.c
@@ -280,6 +280,13 @@ static int validate_ie_attr(const struct nlattr
*attr,
NLA_POLICY_NESTED_ARRAY(nl80211_psmr_peer_attr_policy),
};
+static const struct nla_policy
+nl80211_attr_tid_config_policy[NL80211_ATTR_TID_CONFIG_MAX + 1] = {
+ [NL80211_ATTR_TID_CONFIG_TID] = NLA_POLICY_MAX(NLA_U8, 7),
Such a policy permits configuration of per-TID settings, either
for per-STA or for all the STAs. However it is not possible to
perform configuration for all TIDs at once, e.g. passing -1 value
to driver.
Maybe simplify policy and use .type = NLA_U8 ?
Sanity check, if needed, can be performed by driver or even by
firmware.
Sure, we can have like that. And do you feel driver should advertise
support to perform configuration for all TIDs(like accepting tid -1) ?
+ [NL80211_ATTR_TID_CONFIG_NOACK] =
+ NLA_POLICY_MAX(NLA_U8,
NL80211_TID_CONFIG_DISABLE),
+};
+
const struct nla_policy nl80211_policy[NUM_NL80211_ATTR] = {
[NL80211_ATTR_WIPHY] = { .type = NLA_U32 },
[NL80211_ATTR_WIPHY_NAME] = { .type = NLA_NUL_STRING,
@@ -541,6 +548,8 @@ static int validate_ie_attr(const struct nlattr
*attr,
[NL80211_ATTR_PEER_MEASUREMENTS] =
NLA_POLICY_NESTED(nl80211_pmsr_attr_policy),
[NL80211_ATTR_AIRTIME_WEIGHT] = NLA_POLICY_MIN(NLA_U16, 1),
+ [NL80211_ATTR_TID_CONFIG] =
+
NLA_POLICY_NESTED(nl80211_attr_tid_config_policy),
};
...
+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_CONFIG_MAX + 1];
+ struct net_device *dev = info->user_ptr[1];
+ struct ieee80211_tid_config *tid_conf;
+ struct nlattr *tid;
+ int conf_idx = 0, rem_conf;
+ u32 num_conf = 0, size_of_conf;
+ int ret = -EINVAL;
+
+ 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++;
+
+ size_of_conf = sizeof(struct ieee80211_tid_config) +
+ num_conf * sizeof(struct ieee80211_tid_cfg);
+
+ tid_conf = kzalloc(size_of_conf, GFP_KERNEL);
+ if (!tid_conf)
+ return -ENOMEM;
+
+ tid_conf->n_tid_conf = num_conf;
+
+ if (info->attrs[NL80211_ATTR_MAC])
+ tid_conf->peer =
nla_data(info->attrs[NL80211_ATTR_MAC]);
+ else
+ tid_conf->peer = NULL;
+
+ nla_for_each_nested(tid, info->attrs[NL80211_ATTR_TID_CONFIG],
+ rem_conf) {
+ ret = nla_parse_nested(attrs,
NL80211_ATTR_TID_CONFIG_MAX, tid,
+ NULL, NULL);
+
+ if (ret)
+ return ret;
+
+ if (!attrs[NL80211_ATTR_TID_CONFIG_TID])
+ return -EINVAL;
+
+ ret = parse_tid_conf(rdev, attrs,
_conf->tid_conf[conf_idx],
+tid_conf->peer);
+ if (ret)
+ goto bad_tid_conf;
+
+ conf_idx++;
+ }
+
+ return rdev_set_tid_config(rdev, dev, tid_conf);
What is the ownership rule for tid_conf ? In other words, who is
responsible for freeing this structure ?
Unless I am missing something, in the suggested patches there is no
kfree for this structure and all the nested structures (see Tx bitrate
patch), neither in cfg80211/mac80211 nor in ath10k.
As far as I understand, we have two options here. One option is to
explicitly specify that ownership is passed to the caller, similar
to nl80211_set_reg. Another option is to release memory in this
function after driver makes copies of all necessary fields,
e.g. see nl80211_set_mac_acl.
Thanks for pointing out, will free it in this function(cfg80211) itself.
I will make the change and send next patchset.
+
+bad_tid_conf:
+ kfree(tid_conf);
+ return ret;
+}
Thanks,
Tamizh.
___
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k
Re: [PATCHv2 1/9] nl80211: New netlink command for TID specific configuration
Hello Tamizh,
> Signed-off-by: Tamizh chelvam
> ---
> include/net/cfg80211.h | 35 +++
> include/uapi/linux/nl80211.h | 51 ++
> net/wireless/nl80211.c | 102
> +++
> net/wireless/rdev-ops.h | 11 +
> net/wireless/trace.h | 18
> 5 files changed, 217 insertions(+)
...
> diff --git a/net/wireless/nl80211.c b/net/wireless/nl80211.c
> index 82d5e1e..352eb4a2 100644
> --- a/net/wireless/nl80211.c
> +++ b/net/wireless/nl80211.c
> @@ -280,6 +280,13 @@ static int validate_ie_attr(const struct nlattr *attr,
> NLA_POLICY_NESTED_ARRAY(nl80211_psmr_peer_attr_policy),
> };
>
> +static const struct nla_policy
> +nl80211_attr_tid_config_policy[NL80211_ATTR_TID_CONFIG_MAX + 1] = {
> + [NL80211_ATTR_TID_CONFIG_TID] = NLA_POLICY_MAX(NLA_U8, 7),
Such a policy permits configuration of per-TID settings, either
for per-STA or for all the STAs. However it is not possible to
perform configuration for all TIDs at once, e.g. passing -1 value
to driver.
Maybe simplify policy and use .type = NLA_U8 ?
Sanity check, if needed, can be performed by driver or even by firmware.
> + [NL80211_ATTR_TID_CONFIG_NOACK] =
> + NLA_POLICY_MAX(NLA_U8, NL80211_TID_CONFIG_DISABLE),
> +};
> +
> const struct nla_policy nl80211_policy[NUM_NL80211_ATTR] = {
> [NL80211_ATTR_WIPHY] = { .type = NLA_U32 },
> [NL80211_ATTR_WIPHY_NAME] = { .type = NLA_NUL_STRING,
> @@ -541,6 +548,8 @@ static int validate_ie_attr(const struct nlattr *attr,
> [NL80211_ATTR_PEER_MEASUREMENTS] =
> NLA_POLICY_NESTED(nl80211_pmsr_attr_policy),
> [NL80211_ATTR_AIRTIME_WEIGHT] = NLA_POLICY_MIN(NLA_U16, 1),
> + [NL80211_ATTR_TID_CONFIG] =
> + NLA_POLICY_NESTED(nl80211_attr_tid_config_policy),
> };
...
> +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_CONFIG_MAX + 1];
> + struct net_device *dev = info->user_ptr[1];
> + struct ieee80211_tid_config *tid_conf;
> + struct nlattr *tid;
> + int conf_idx = 0, rem_conf;
> + u32 num_conf = 0, size_of_conf;
> + int ret = -EINVAL;
> +
> + 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++;
> +
> + size_of_conf = sizeof(struct ieee80211_tid_config) +
> + num_conf * sizeof(struct ieee80211_tid_cfg);
> +
> + tid_conf = kzalloc(size_of_conf, GFP_KERNEL);
> + if (!tid_conf)
> + return -ENOMEM;
> +
> + tid_conf->n_tid_conf = num_conf;
> +
> + if (info->attrs[NL80211_ATTR_MAC])
> + tid_conf->peer = nla_data(info->attrs[NL80211_ATTR_MAC]);
> + else
> + tid_conf->peer = NULL;
> +
> + nla_for_each_nested(tid, info->attrs[NL80211_ATTR_TID_CONFIG],
> + rem_conf) {
> + ret = nla_parse_nested(attrs, NL80211_ATTR_TID_CONFIG_MAX,
> tid,
> + NULL, NULL);
> +
> + if (ret)
> + return ret;
> +
> + if (!attrs[NL80211_ATTR_TID_CONFIG_TID])
> + return -EINVAL;
> +
> + ret = parse_tid_conf(rdev, attrs,
> _conf->tid_conf[conf_idx],
> +tid_conf->peer);
> + if (ret)
> + goto bad_tid_conf;
> +
> + conf_idx++;
> + }
> +
> + return rdev_set_tid_config(rdev, dev, tid_conf);
What is the ownership rule for tid_conf ? In other words, who is
responsible for freeing this structure ?
Unless I am missing something, in the suggested patches there is no
kfree for this structure and all the nested structures (see Tx bitrate
patch), neither in cfg80211/mac80211 nor in ath10k.
As far as I understand, we have two options here. One option is to
explicitly specify that ownership is passed to the caller, similar
to nl80211_set_reg. Another option is to release memory in this
function after driver makes copies of all necessary fields,
e.g. see nl80211_set_mac_acl.
> +
> +bad_tid_conf:
> + kfree(tid_conf);
> + return ret;
> +}
Regards,
Sergey
___
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k
[PATCHv2 1/9] nl80211: New netlink command for TID specific configuration
Add a new NL command, NL80211_CMD_SET_TID_CONFIG to support
data TID specific configuration. This per TID configurations
are passed in NL80211_ATTR_TID_CONFIG which is a nested
attribute. This patch adds support to configure per TID
noack policy through NL80211_ATTR_TID_CONFIG_NOACK attribute.
Data TID value for this configuration will be passed through
NL80211_ATTR_TID_CONFIG_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
---
include/net/cfg80211.h | 35 +++
include/uapi/linux/nl80211.h | 51 ++
net/wireless/nl80211.c | 102 +++
net/wireless/rdev-ops.h | 11 +
net/wireless/trace.h | 18
5 files changed, 217 insertions(+)
diff --git a/include/net/cfg80211.h b/include/net/cfg80211.h
index f81677f..07eb2de 100644
--- a/include/net/cfg80211.h
+++ b/include/net/cfg80211.h
@@ -509,6 +509,35 @@ struct cfg80211_chan_def {
u32 center_freq2;
};
+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;
+ enum ieee80211_tid_conf_mask 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
@@ -3436,6 +3465,10 @@ struct cfg80211_pmsr_request {
* Statistics should be cumulative, currently no way to reset is provided.
* @start_pmsr: start peer measurement (e.g. FTM)
* @abort_pmsr: abort peer measurement
+ * @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);
@@ -3750,6 +3783,8 @@ struct cfg80211_ops {
struct cfg80211_pmsr_request *request);
void(*abort_pmsr)(struct wiphy *wiphy, struct wireless_dev *wdev,
struct cfg80211_pmsr_request *request);
+ 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 dd4f86e..c901a48 100644
--- a/include/uapi/linux/nl80211.h
+++ b/include/uapi/linux/nl80211.h
@@ -1065,6 +1065,10 @@
* indicated by %NL80211_ATTR_WIPHY_FREQ and other attributes
* determining the width and type.
*
+ * @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
*/
@@ -1285,6 +1289,8 @@ enum nl80211_commands {
NL80211_CMD_NOTIFY_RADAR,
+ NL80211_CMD_SET_TID_CONFIG,
+
/* add new commands above here */
/* used to define NL80211_CMD_MAX below */
@@ -2308,6 +2314,9 @@ enum nl80211_commands {
* @NL80211_ATTR_AIRTIME_WEIGHT: Station's weight when scheduled by the airtime
* scheduler.
*
+ * @NL80211_ATTR_TID_CONFIG: TID specific configuration in a
+ * nested attribute with %NL80211_ATTR_TID_* 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
@@ -2759,6 +2768,8 @@ enum nl80211_attrs {
NL80211_ATTR_AIRTIME_WEIGHT,
+ NL80211_ATTR_TID_CONFIG,
+
/* add attributes here, update the policy in nl80211.c */
__NL80211_ATTR_AFTER_LAST,
@@ -4543,6 +4554,39 @@ enum nl80211_tx_power_setting {
NL80211_TX_POWER_FIXED,
};
+enum
