Re: [PATCHv9 1/6] nl80211: Add NL command to support TID speicific configurations

[Sergey Matyukevich <sergey.matyukevich.os@quantenna.com>]

> 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