Re: [PATCHv8 2/6] nl80211: Add new netlink attribute for TID speicific retry count

[Tamizh chelvam <tamizhr@codeaurora.org>]

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.