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


>> +	int	(*set_sta_mon_rssi_config)(struct wiphy *wiphy,
>> +					  struct net_device *dev,
>> +					  const u8 *addr,
>> +					  const s32 *rssi_tholds,
>> +					  u32 rssi_hyst, int rssi_n_tholds,
>> +					  bool fixed_thold);
>>  };
> 
> I think it might be better to pass all the last 4 arguments (rssi
> related ones) as a struct? That's a pattern we typically have elsewhere
> too, and it makes things easier to extend and also easier to pass
> around.
> 
Sure.

>> + * @NL80211_CMD_SET_STA_MON: This command is used to configure 
>> station's
>> + *     connection monitoring notification trigger levels.
>> + * @NL80211_CMD_NOTIFY_STA_MON: This is used as an event to notify
>> + *     the user space that a trigger level was reached for a station.
> 
> Please describe the attributes to use with this.
> 
I'll add that in the next version of patchset
> 
>> + * @NL80211_ATTR_STA_MON_FIXED_THOLD: Flag attribute is used with
>> + *	%NL80211_CMD_SET_STA_MON to indicate driver that the monitoring
>> + *	configuration is fixed limit or a moving range threshold.
> 
> This isn't really clear to me, what does it mean?
> 
Sorry for not clear. This flag introduced to mention the driver that if 
this flag set then don't change the rssi_low and rssi_high limit upon
the station's rssi crossing the configured limit. Keep the rssi_low and 
rssi_high as a fixed limit.

>> +	if (!sta_mon || !(info->attrs[NL80211_ATTR_MAC]))
> 
> Don't really need the parentheses in !(info->...)
> 
Yes, I'll remove it.

>> +	err = nla_parse_nested(attrs, NL80211_ATTR_CQM_MAX, sta_mon,
>> +			       nl80211_attr_cqm_policy, info->extack);
> 
> I *think* I made that a proper nested policy, check and then you can
> remove passing it here.
> 
Sure. I'll modify it in the next patchset version.

>> +void
>> +cfg80211_sta_mon_rssi_notify(struct net_device *dev, const u8 *peer,
>> +			     enum nl80211_cqm_rssi_threshold_event rssi_event,
>> +			     s32 rssi_level, gfp_t gfp)
>> +{
>> +	struct sk_buff *msg;
>> +
>> +	if (WARN_ON(!peer))
>> +		return;
> 
> Tracing for this might be nice too?
> 
Sure.

Thanks,
Tamizh.