Re: [PATCH 1/4] New netlink command for TID specific configuration

From: Sergey Matyukevich <sergey.matyukevich.os@quantenna.com>
To: Tamizh chelvam <tamizhr@codeaurora.org>
Cc: "ath10k@lists.infradead.org" <ath10k@lists.infradead.org>,
	"johannes@sipsolutions.net" <johannes@sipsolutions.net>,
	"linux-wireless@vger.kernel.org" <linux-wireless@vger.kernel.org>,
	Vasanthakumar Thiagarajan <vthiagar@codeaurora.org>
Subject: Re: [PATCH 1/4] New netlink command for TID specific configuration
Date: Wed, 7 Nov 2018 14:05:58 +0000	[thread overview]
Message-ID: <20181107140552.zfuusbapgqjumoln@bars> (raw)
In-Reply-To: <1540230918-27712-2-git-send-email-tamizhr@codeaurora.org>

Hello Tamizh,

> Co-Developed-by: Tamizh Chelvam <tamizhr@codeaurora.org>
> Signed-off-by: Vasanthakumar Thiagarajan <vthiagar@codeaurora.org>
> Signed-off-by: Tamizh chelvam <tamizhr@codeaurora.org>
> ---
>  include/net/cfg80211.h       |   14 +++++++
>  include/uapi/linux/nl80211.h |   69 +++++++++++++++++++++++++++++++++
>  net/wireless/nl80211.c       |   86 ++++++++++++++++++++++++++++++++++++++++++
>  net/wireless/rdev-ops.h      |   15 ++++++++
>  net/wireless/trace.h         |   27 +++++++++++++
>  5 files changed, 211 insertions(+)

...

> diff --git a/net/wireless/nl80211.c b/net/wireless/nl80211.c
> index d744388..d386ad7 100644
> --- a/net/wireless/nl80211.c
> +++ b/net/wireless/nl80211.c

...

> +static int nl80211_set_tid_config(struct sk_buff *skb,
> +                                 struct genl_info *info)
> +{
> +       struct cfg80211_registered_device *rdev = info->user_ptr[0];
> +       struct nlattr *attrs[NL80211_ATTR_TID_MAX + 1];
> +       struct nlattr *tid;
> +       struct net_device *dev = info->user_ptr[1];
> +       const char *peer = NULL;
> +       u8 tid_no;
> +       int ret = -EINVAL, retry_short = -1, retry_long = -1;
> +
> +       tid = info->attrs[NL80211_ATTR_TID_CONFIG];
> +       if (!tid)
> +               return -EINVAL;
> +
> +       ret = nla_parse_nested(attrs, NL80211_ATTR_TID_MAX, tid,
> +                              nl80211_attr_tid_policy, info->extack);
> +       if (ret)
> +               return ret;
> +
> +       if (!attrs[NL80211_ATTR_TID])
> +               return -EINVAL;
> +
> +       if (attrs[NL80211_ATTR_TID_RETRY_SHORT]) {
> +               retry_short = nla_get_u8(attrs[NL80211_ATTR_TID_RETRY_SHORT]);
> +               if (!retry_short ||
> +                   retry_short > rdev->wiphy.max_data_retry_count)
> +                       return -EINVAL;
> +       }
> +
> +       if (attrs[NL80211_ATTR_TID_RETRY_LONG]) {
> +               retry_long = nla_get_u8(attrs[NL80211_ATTR_TID_RETRY_LONG]);
> +               if (!retry_long ||
> +                   retry_long > rdev->wiphy.max_data_retry_count)
> +                       return -EINVAL;
> +       }
> +
> +       tid_no = nla_get_u8(attrs[NL80211_ATTR_TID]);
> +       if (tid_no >= IEEE80211_FIRST_TSPEC_TSID)
> +               return -EINVAL;
> +
> +       if (info->attrs[NL80211_ATTR_MAC])
> +               peer = nla_data(info->attrs[NL80211_ATTR_MAC]);
> +
> +       if (nla_get_flag(attrs[NL80211_ATTR_TID_RETRY_CONFIG])) {

Do we really need this additional flag to indicate retry data ?
Maybe we can simply check retry attrs or even retry data, e.g.:

if (attrs[NL80211_ATTR_TID_RETRY_LONG] ||
    attrs[NL80211_ATTR_TID_RETRY_SHORT]) {
    ...

if ((retry_short > 0) || (retry_long > 0)) {
    ...

> +               if (!wiphy_ext_feature_isset(
> +                               &rdev->wiphy,
> +                               NL80211_EXT_FEATURE_PER_TID_RETRY_CONFIG))
> +                       return -EOPNOTSUPP;
> +
> +               if (peer && !wiphy_ext_feature_isset(
> +                               &rdev->wiphy,
> +                               NL80211_EXT_FEATURE_PER_STA_RETRY_CONFIG))
> +                       return -EOPNOTSUPP;
> +
> +               if (!rdev->ops->set_data_retry_count ||
> +                   !rdev->wiphy.max_data_retry_count)
> +                       return -EOPNOTSUPP;
> +
> +               ret = rdev_set_data_retry_count(rdev, dev, peer, tid_no,
> +                                               retry_short, retry_long);
> +       }
> +
> +       return ret;
> +}

Regards,
Sergey