Re: [PATCH 1/4] New netlink command for TID specific configuration

From: Tamizh chelvam <tamizhr@codeaurora.org>
To: Sergey Matyukevich <sergey.matyukevich.os@quantenna.com>
Cc: ath10k@lists.infradead.org, johannes@sipsolutions.net,
	linux-wireless@vger.kernel.org,
	Vasanthakumar Thiagarajan <vthiagar@codeaurora.org>,
	Igor Mitsyanko <igor.mitsyanko.os@quantenna.com>
Subject: Re: [PATCH 1/4] New netlink command for TID specific configuration
Date: Thu, 08 Nov 2018 22:59:48 +0530	[thread overview]
Message-ID: <7a8662ce7744bee6d9e695c67d7e9643@codeaurora.org> (raw)
In-Reply-To: <20181106101601.526ovvmabfqrr7sl@bars>

On 2018-11-06 15:46, Sergey Matyukevich wrote:
> Hello Tamizh,
> 
>> Co-Developed-by: Tamizh Chelvam <tamizhr@codeaurora.org>
>> Signed-off-by: Vasanthakumar Thiagarajan <vthiagar@codeaurora.org>
>> Signed-off-by: Tamizh chelvam <tamizhr@codeaurora.org>
>> ---
>>  include/net/cfg80211.h       |   14 +++++++
>>  include/uapi/linux/nl80211.h |   69 +++++++++++++++++++++++++++++++++
>>  net/wireless/nl80211.c       |   86 
>> ++++++++++++++++++++++++++++++++++++++++++
>>  net/wireless/rdev-ops.h      |   15 ++++++++
>>  net/wireless/trace.h         |   27 +++++++++++++
>>  5 files changed, 211 insertions(+)
> 
> ...
> 
>> diff --git a/include/net/cfg80211.h b/include/net/cfg80211.h
>> index 5801d76..dd024da 100644
>> --- a/include/net/cfg80211.h
>> +++ b/include/net/cfg80211.h
> 
> ...
> 
>>  /**
>> @@ -4035,6 +4044,9 @@ struct wiphy_iftype_ext_capab {
>>   * @txq_limit: configuration of internal TX queue frame limit
>>   * @txq_memory_limit: configuration internal TX queue memory limit
>>   * @txq_quantum: configuration of internal TX queue scheduler quantum
>> + *
>> + * @max_data_retry_count: Maximum limit can be configured as retry 
>> count
>> + *     for a TID.
>>   */
>>  struct wiphy {
>>         /* assign these fields before you register the wiphy */
>> @@ -4171,6 +4183,8 @@ struct wiphy {
>>         u32 txq_memory_limit;
>>         u32 txq_quantum;
>> 
>> +       u8 max_data_retry_count;
>> +
>>         char priv[0] __aligned(NETDEV_ALIGN);
>>  };
> 
> Could you please clarify why do you define max_data_retry_count instead 
> of
> making use of existing wiphy params: retry_short (dot11ShortRetryLimit)
> and retry_long (dot11LongRetryLimit) ?

max_data_retry_count added to validate the max limit for the retry count 
supported by the driver.
existing wiphy parames: retry_short and retry_long can be modified 
through user command.
So, I've added this param for validation purpose. Correct me If I'm 
wrong.
> 
>> diff --git a/net/wireless/nl80211.c b/net/wireless/nl80211.c
>> index d744388..d386ad7 100644
>> --- a/net/wireless/nl80211.c
>> +++ b/net/wireless/nl80211.c
> 
> ...
> 
>> +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_ATTR_TID_MAX + 1];
>> +       struct nlattr *tid;
>> +       struct net_device *dev = info->user_ptr[1];
>> +       const char *peer = NULL;
>> +       u8 tid_no;
>> +       int ret = -EINVAL, retry_short = -1, retry_long = -1;
>> +
>> +       tid = info->attrs[NL80211_ATTR_TID_CONFIG];
>> +       if (!tid)
>> +               return -EINVAL;
>> +
>> +       ret = nla_parse_nested(attrs, NL80211_ATTR_TID_MAX, tid,
>> +                              nl80211_attr_tid_policy, info->extack);
>> +       if (ret)
>> +               return ret;
>> +
>> +       if (!attrs[NL80211_ATTR_TID])
>> +               return -EINVAL;
>> +
>> +       if (attrs[NL80211_ATTR_TID_RETRY_SHORT]) {
>> +               retry_short = 
>> nla_get_u8(attrs[NL80211_ATTR_TID_RETRY_SHORT]);
>> +               if (!retry_short ||
>> +                   retry_short > rdev->wiphy.max_data_retry_count)
>> +                       return -EINVAL;
>> +       }
>> +
>> +       if (attrs[NL80211_ATTR_TID_RETRY_LONG]) {
>> +               retry_long = 
>> nla_get_u8(attrs[NL80211_ATTR_TID_RETRY_LONG]);
>> +               if (!retry_long ||
>> +                   retry_long > rdev->wiphy.max_data_retry_count)
>> +                       return -EINVAL;
>> +       }
>> +
>> +       tid_no = nla_get_u8(attrs[NL80211_ATTR_TID]);
>> +       if (tid_no >= IEEE80211_FIRST_TSPEC_TSID)
>> +               return -EINVAL;
> 
> Not that important, but this tid_no check can be placed after
> attrs[NL80211_ATTR_TID].
> 
> BTW, some special tid_no value (e.g. (u8)-1) could be used to notify 
> driver
> that retry settings should be applied for all the TIDs. IIUC the only
> required change would be to modify this tid_no sanity check.

Sure, we can use that.
> 
> Regards,
> Sergey

Thanks,
Tamizh.