Re: [PATCHv6 1/9] nl80211: New netlink command for TID specific configuration

From: Tamizh chelvam <tamizhr@codeaurora.org>
To: Sergey Matyukevich <sergey.matyukevich.os@quantenna.com>
Cc: johannes@sipsolutions.net, linux-wireless@vger.kernel.org,
	ath10k@lists.infradead.org
Subject: Re: [PATCHv6 1/9] nl80211: New netlink command for TID specific configuration
Date: Wed, 31 Jul 2019 15:34:18 +0530	[thread overview]
Message-ID: <edc16458451377681b540553e1ddea75@codeaurora.org> (raw)
In-Reply-To: <20190711125111.vd4rfixmotyusuai@bars>

On 2019-07-11 18:21, Sergey Matyukevich wrote:
>> 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_TID_ATTR_CONFIG_NOACK attribute.
>> Data TID value for this configuration will be passed through
>> NL80211_TID_ATTR_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 <tamizhr@codeaurora.org>
> 
> ...
> 
>> +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)
>> +{
> 
> ...
> 
> 
>> +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_ATTR_CONFIG_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_deprecated(attrs, 
>> NL80211_TID_ATTR_CONFIG_MAX,
>> +                                                 tid, NULL, NULL);
>> +
>> +               if (ret)
>> +                       goto bad_tid_conf;
>> +
>> +               if (!attrs[NL80211_TID_ATTR_CONFIG_TID]) {
>> +                       ret = -EINVAL;
>> +                       goto bad_tid_conf;
>> +               }
>> +
>> +               ret = parse_tid_conf(rdev, attrs,
>> +                                    &tid_config->tid_conf[conf_idx],
>> +                                    tid_config->peer);
> 
> 
> Argument 'info' is missing here from parse_tid_conf. IIUC, appropriate
> fixup is included into patch #5. So it looks like rebase issue. But it
> makes sense to fix it since this issue makes the patch set 
> non-bisectable.
> 
My bad. Yes, it is missed in the rebase. I will fix that.

Thanks,
Tamizh.