On 2019-11-08 15:30, Johannes Berg wrote:
+ * Station specific retry configuration is valid only for STA's
+ * current connection. i.e. the configuration will be reset to
default when
+ * the station connects back after disconnection/roaming.
+ * when user-space does not include %NL80211_ATTR_MAC, this
configuration
+ * should be treated as per-netdev configuration. This configuration
will
+ * be cleared when the interface goes down and on the disconnection
from a
+ * BSS. When retry count has never been configured using this
command, the
+ * other available radio level retry configuration
+ * (%NL80211_ATTR_WIPHY_RETRY_SHORT and
%NL80211_ATTR_WIPHY_RETRY_LONG)
+ * should be used. Driver supporting this feature should advertise
+ * NL80211_EXT_FEATURE_PER_TID_RETRY_CONFIG and supporting per
station
+ * retry count configuration should advertise
+ * NL80211_EXT_FEATURE_PER_STA_RETRY_CONFIG.
Here you pretty much copy-pasted all this text ... that's why I think
it
should be in some other section. We want *everything* to be like that,
not have to check every single thing for different validity rules.
Sure, I will add these things in DOC: section.
+ * @NL80211_TID_CONFIG_ATTR_RETRY_SHORT: Number of retries used with
data frame
+ * transmission, user-space sets this configuration in
+ * &NL80211_CMD_SET_TID_CONFIG. It is u8 type, min value is 1 and
+ * the max value should be advertised by the driver through
+ * max_data_retry_count. when this attribute is not present, the
driver
+ * would use the default configuration.
+ * @NL80211_TID_CONFIG_ATTR_RETRY_LONG: Number of retries used with
data frame
+ * transmission, user-space sets this configuration in
+ * &NL80211_CMD_SET_TID_CONFIG. Its type is u8, min value is 1 and
+ * the max value should be advertised by the driver through
+ * max_data_retry_count. when this attribute is not present, the
driver
+ * would use the default configuration.
I'm almost thinking that these should be a struct with two u8 values
instead of two separate attributes, and then renamed to
NL80211_TID_CONFIG_ATTR_RETRY, to carry both and really ensure thaty
they're always together as a single configuration.
This will make mandatory for user to send both values know ? I have did
it similar to
NL80211_ATTR_WIPHY_RETRY_SHORT and NL80211_ATTR_WIPHY_RETRY_LONG. This
way we can have
option to configure single parameter know ?
This only really works right if we go for the reset= approach I
outlined
in the previous patch though, since you otherwise need
NL80211_TID_CONFIG_ATTR_RETRY for the reset ... but that's a pretty
weird thing.
(there are also some typos here like "notfiy")
I will fix this.
--- a/net/wireless/nl80211.c
+++ b/net/wireless/nl80211.c
@@ -326,6 +326,9 @@ static int validate_ie_attr(const struct nlattr
*attr,
[NL80211_TID_CONFIG_ATTR_TID] = { .type = NLA_U8 },
[NL80211_TID_CONFIG_ATTR_NOACK] =
NLA_POLICY_MAX(NLA_U8, NL80211_TID_CONFIG_DISABLE),
+ [NL80211_TID_CONFIG_ATTR_RETRY] = { .type = NLA_FLAG },
+ [NL80211_TID_CONFIG_ATTR_RETRY_SHORT] = { .type = NLA_U8},
+ [NL80211_TID_CONFIG_ATTR_RETRY_LONG] = { .type = NLA_U8},
The min value of 1 should be reflected in the policy.
Yeah, It was there in the previous patchset and removed due to
confusion.
Do you want to keep MIN value of 1 policy ?
+ if (rdev->wiphy.max_data_retry_count) {
+ if (nla_put_u8(msg, NL80211_ATTR_MAX_RETRY_COUNT,
+ rdev->wiphy.max_data_retry_count))
bad indentation
I will fix this.
+ goto nla_put_failure;
+ }
+
state->split_start++;
if (state->split)
break;
Also not sure which section you put this in, but it looks almost like
it's under "case 1:" where it really shouldn't be ... move it to the
end
please.
Sure.
Thanks,
Tamizh.
