Received-SPF: pass (google.com: best guess record for domain of linux-wireless-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Tue, 14 Jan 2020 15:09:50 +0300
From:   Sergey Matyukevich <sergey.matyukevich.os@quantenna.com>
To:     Tamizh Chelvam <tamizhr@codeaurora.org>
CC:     johannes@sipsolutions.net, linux-wireless@vger.kernel.org
Subject: Re: [PATCHv9 1/6] nl80211: Add NL command to support TID speicific
 configurations
Message-ID: <20200114120949.7yl2qcahfgifohvl@bars>
Mail-Followup-To: Tamizh Chelvam <tamizhr@codeaurora.org>,
        johannes@sipsolutions.net, linux-wireless@vger.kernel.org
References: <1578921090-9758-1-git-send-email-tamizhr@codeaurora.org>
 <1578921090-9758-2-git-send-email-tamizhr@codeaurora.org>
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <1578921090-9758-2-git-send-email-tamizhr@codeaurora.org>
User-Agent: NeoMutt/20170113 (1.7.2)
MIME-Version: 1.0
Received-SPF: None (protection.outlook.com: quantenna.com does not designate
 permitted sender hosts)
X-MS-Exchange-CrossTenant-Network-Message-Id: 26e81a2d-7b0f-407b-de1b-08d798eaab5e
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: a355dbce-62b4-4789-9446-c1d5582180ff
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: K6SVl0+7E8R2dAw7Y3z//rX4wLTg5CzVd3Wlybt0XfzHxn3HwPj4sRoxv6cUKMkMoWGpUs53qZg6pSDRgwoTjc1RRu2quoi82r98pBO8nhggmw6LgIhzZiksqKyncp2m7kJqNMliUVtVv9rPZaR5nYnyhDq+9Yljov4QxAcjGP9pgiCqYraYDO6EF20iOeqD0mHZNtAPoSL5KAS8PxKZ9F5hmDN+J9agom2L9Ruyp4U=
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 14 Jan 2020 12:10:23.8707
 (UTC)
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BYAPR05MB4821
X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10434:6.0.138,18.0.572
 definitions=2020-01-14_03:2020-01-13,2020-01-14 signatures=0
X-Proofpoint-Spam-Reason: safe
Sender: linux-wireless-owner@vger.kernel.org
Precedence: bulk
List-ID: <linux-wireless.vger.kernel.org>
X-Mailing-List: linux-wireless@vger.kernel.org

> 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.
> 
> Signed-off-by: Tamizh chelvam <tamizhr@codeaurora.org>
> ---
>  include/net/cfg80211.h       |   40 ++++++++++++
>  include/uapi/linux/nl80211.h |   71 +++++++++++++++++++++
>  net/wireless/nl80211.c       |  142 ++++++++++++++++++++++++++++++++++++++++++
>  net/wireless/rdev-ops.h      |   24 +++++++
>  net/wireless/trace.h         |   37 +++++++++++
>  5 files changed, 314 insertions(+)

...

> +/**
> + * struct ieee80211_tid_cfg - TID specific configuration
> + * @config_override: Flag to notify driver to reset TID configuration
> + *	of the peer.
> + * @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 {
> +	bool config_override;
> +	u8 tid;
> +	u32 tid_conf_mask;
> +	enum nl80211_tid_config noack;

You are using nl80211_tid_conf type for noack, but u8 for ampdu and
rtscts in further patches. On the other hand, they are using the same
validation policy: NLA_POLICY_MAX(NLA_U8, NL80211_TID_CONFIG_DISABLE).
It looks like it makes sense to use the same type for all of them,
either u8 or nl80211_tid_config.

> +};
> +
> +/**
> + * 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[];
> +};

...

>  /**
> + * DOC: TID configuration
> + *
> + * TID configuration support can be advertised by drivers by setting
> + * @NL80211_EXT_FEATURE_PER_TID_* and/or @NL80211_EXT_FEATURE_PER_STA_* config
> + * mentioned in &enum nl80211_tid_config_attr.
> + * Needed configuration parameters are mentioned &enum nl80211_tid_config_attr

are mentioned in ?

> + * and it will be passed using %NL80211_CMD_SET_TID_CONFIG through
> + * %NL80211_ATTR_TID_CONFIG. If the configuration needs to be applied for
> + * specific peer then MAC address of the peer needs to be passed in
> + * %NL80211_ATT_MAC, otherwise the configuration will be applied for all the
> + * connected peers in the vif except the peer which has peer specific
> + * configuration for the TID. And the peer specific configuration will be
> + * override if %NL80211_TID_CONFIG_ATTR_OVERRIDE flag is set.

overridden ?

BTW, it looks like there is a minor mismatch between docs and code here.
Looking into the implementation, peer specific configuration can be
overridden only if both override flag and peer address are specified.

> + * All this configurations are valid only for STA's current connection
> + * i.e. the configurations will be reset to default when the STA connects back
> + * after disconnectiion/roaming, and this configuration will be cleared when

typo: ii

> + * the interface goes down.
> + */

...

> +static int
> +__nl80211_check_tid_conf_support(struct cfg80211_registered_device *rdev,
> +				 struct netlink_ext_ack *extack,
> +				 const u8 *peer, struct nlattr *attrs[],
> +				 struct ieee80211_tid_cfg *tid_conf,
> +				 enum nl80211_tid_config_attr 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, attrs[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, attrs[attr],
> +				    "peer specific TID configuration not supported");
> +		return -ENOTSUPP;
> +	}
> +
> +	tid_conf->tid_conf_mask |= BIT(attr);

Unless I missing something, the first 3 bits in mask are going to be unused
since NL80211_TID_CONFIG_ATTR_NOACK is 3.

> +	return 0;
> +}

Regards,
Sergey