DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org B4F6C20871
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII;
 format=flowed
Content-Transfer-Encoding: 7bit
Date:   Sun, 11 Nov 2018 19:27:56 +0530
From:   Tamizh chelvam <tamizhr@codeaurora.org>
To:     Johannes Berg <johannes@sipsolutions.net>
Cc:     linux-wireless@vger.kernel.org
Subject: Re: [PATCH 2/3] mac80211: Implement API to configure station specific
 rssi threshold
In-Reply-To: <f1c8ce7daf62c60197da9cc62d550fdc1cac6303.camel@sipsolutions.net>
References: <1539626250-769-1-git-send-email-tamizhr@codeaurora.org>
 <1539626250-769-3-git-send-email-tamizhr@codeaurora.org>
 <f1c8ce7daf62c60197da9cc62d550fdc1cac6303.camel@sipsolutions.net>
Message-ID: <f3c8d129a8b7dea76febbb5a5adee113@codeaurora.org>
User-Agent: Roundcube Webmail/1.2.5
Sender: linux-wireless-owner@vger.kernel.org
Precedence: bulk

On 2018-11-09 17:19, Johannes Berg wrote:
> On Mon, 2018-10-15 at 23:27 +0530, Tamizh chelvam wrote:
>> 
>> +		sta_mon_rssi_config_free(sta);
>> +		sta->rssi_hyst = rssi_hyst;
>> +		if (fixed_thold) {
>> +			if (n_rssi_tholds > 2) {
>> +				ret = -EINVAL;
>> +				goto out;
>> +			}
> 
> This might be slightly wrong, you free and then can still return an
> error.
> 
Sure. I'll move the free part after the validation check.

>> +			if (n_rssi_tholds == 1) {
>> +				sta->rssi_low = rssi_tholds[0];
>> +				sta->rssi_high = rssi_tholds[0];
>> +			} else {
>> +				sta->rssi_low = rssi_tholds[0];
>> +				sta->rssi_high = rssi_tholds[1];
>> +			}
>> +		} else {
>> +			const s32 *rssi_tholds;
>> +
>> +			rssi_tholds = kmemdup(rssi_tholds,
>> +					      n_rssi_tholds * sizeof(s32),
>> +					      GFP_KERNEL);
>> +			if (!rssi_tholds) {
>> +				ret = -ENOMEM;
>> +				goto out;
>> +			}
> 
> Similarly here, I guess you should do the allocation (and other error
> checking) before freeing.
> 
ditto, Sure. I'll move the free part after the validation check.

>> +			sta->rssi_tholds = rssi_tholds;
>> +			sta->n_rssi_tholds = n_rssi_tholds;
>> +			ieee80211_update_rssi_config(sta);
> 
> 
> 
>> +struct sta_mon_rssi_config {
>> +};
> 
> Huh?
> 
oops:( I have kept all configuring parameters in sta_info itself, 
mistakenly didn't remove this struct:(
> 
> The commit log also makes it sounds like mac80211 actually *supports*
> this, but clearly that's not the case. However you don't give any data
> to the driver either, did you lose a patch along the way? Previously 
> you
> had patch 5 ("mac80211: Implement functionality to monitor station's
> rssi cross event") and if I remember correctly I said you should squash
> some, but now that's not here?
> 

Thanks,
Tamizh.