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;
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII;
 format=flowed
Content-Transfer-Encoding: 7bit
Date:   Mon, 20 Jan 2020 13:19:11 +0530
From:   tamizhr@codeaurora.org
To:     Tamizh Chelvam <tamizhr@codeaurora.org>
Cc:     johannes@sipsolutions.net, linux-wireless@vger.kernel.org
Subject: Re: [PATCHv9 5/6] nl80211: Add support to configure TID specific
 txrate configuration
In-Reply-To: <20200114123108.5mrzvyqkjp7defln@bars>
References: <1578921090-9758-1-git-send-email-tamizhr@codeaurora.org>
 <1578921090-9758-6-git-send-email-tamizhr@codeaurora.org>
 <20200114123108.5mrzvyqkjp7defln@bars>
Message-ID: <c1f8ed5c0d001d2647a8ef23c0329fab@codeaurora.org>
User-Agent: Roundcube Webmail/1.3.9
Sender: linux-wireless-owner@vger.kernel.org
Precedence: bulk

On 2020-01-14 18:01, Sergey Matyukevich wrote:
>> This patch adds support to configure per TID txrate configuration
>> configuration through the NL80211_TID_CONFIG_ATTR_TX_RATE_TYPE
>> and NL80211_TID_CONFIG_ATTR_TX_RATE
>> attribute. TX bitrate mask values passed
>> in NL80211_ATTR_TX_RATES attribute and 
>> NL80211_TID_CONFIG_ATTR_TX_RATES
>> attribute will have types of the TX rate should be applied. This uses
>> nl80211_parse_tx_bitrate_mask to validate and calculate the bitrate
>> mask.
>> 
>> Signed-off-by: Tamizh chelvam <tamizhr@codeaurora.org>
>> ---
>>  include/net/cfg80211.h       |    5 +++
>>  include/uapi/linux/nl80211.h |   24 +++++++++++++
>>  net/wireless/nl80211.c       |   76 
>> ++++++++++++++++++++++++++++++++----------
>>  3 files changed, 88 insertions(+), 17 deletions(-)
> 
> ...
> 
>> @@ -13936,6 +13947,37 @@ static int parse_tid_conf(struct 
>> cfg80211_registered_device *rdev,
>>  			nla_get_u8(attrs[NL80211_TID_CONFIG_ATTR_RTSCTS_CTRL]);
>>  	}
>> 
>> +	if (attrs[NL80211_TID_CONFIG_ATTR_TX_RATE_TYPE]) {
>> +		int idx;
>> +		enum nl80211_attrs attr;
>> +
>> +		err = nl80211_check_tid_config_support(rdev, extack, peer,
>> +						       attrs, tid_conf,
>> +						       TX_RATE);
>> +		if (err)
>> +			return err;
>> +		idx = NL80211_TID_CONFIG_ATTR_TX_RATE_TYPE;
>> +		tid_conf->txrate_type = nla_get_u8(attrs[idx]);
>> +		if (tid_conf->txrate_type != NL80211_TX_RATE_AUTOMATIC) {
>> +			tid_conf->mask =
>> +				kzalloc(sizeof(struct cfg80211_bitrate_mask),
>> +					GFP_KERNEL);
>> +			if (!tid_conf->mask)
>> +				return -ENOMEM;
>> +
>> +			attr =
>> +			(enum nl80211_attrs)NL80211_TID_CONFIG_ATTR_TX_RATE;
>> +			err = nl80211_parse_tx_bitrate_mask(info, attrs, attr,
>> +							    tid_conf->mask);
>> +			if (err) {
>> +				kfree(tid_conf->mask);
>> +				return err;
>> +			}
> 
> IIUC we have to free all the allocated tid_conf->mask entries in the 
> end of
> nl80211_set_tid_config, right before tid_config is freed.
Yeah, this needs to be take care by the driver, since it will be sent 
with multiple
configuration. I have added that in the comment in next patchset.
> Alternatively,struct ieee80211_tid_cfg can be modified to keep 
> cfg80211_bitrate_mask
> value rather than pointer.
I have just reused the nl80211_parse_tx_bitrate_mask, so I feel using 
the similar approach
should be good.

> 
>> +		} else {
>> +			tid_conf->mask = NULL;
>> +		}
>> +	}
>> +
>>  	return 0;
>>  }
> 
Thanks,
Tamizh.