Hi Tamizh,
> Hi Sergey,
> >
> > > Signed-off-by: Tamizh chelvam <tamizhr@xxxxxxxxxxxxxx>
> > > ---
> > > 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,
> > > &tid_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.
Great!
Thanks,
Sergey
