Re: [PATCH v2 2/3] mac80211: support FTM responder configuration/statistics

[Pradeep Kumar Chitrapu <pradeepc@codeaurora.org>]

On 2018-09-03 02:33, Johannes Berg wrote:
> On Fri, 2018-08-31 at 12:56 -0700, Pradeep Kumar Chitrapu wrote:
>> 
>> + * @ftm_responder: whether to enable fine timing measurement FTM 
>> functionality
> 
> missing "responder" somewhere there :)
> 
>> + * @ftmr_params: configurable lci/civic parameter when enabling FTM 
>> responder.
> 

Hi Johannes,

Thanks for the review..
> Perhaps the *existence* of ftmr_params could indicate that it's 
> enabled?
My understanding is these are only optional parameters. So, I have kept 
ftm_responder
separate from params.
I have tried to fix other review comments and posted v3 patch set.
Please let me know your comments.

Thanks
Pradeep

> 
>> @@ -865,6 +899,20 @@ static int ieee80211_assign_beacon(struct 
>> ieee80211_sub_if_data *sdata,
>>  	if (err == 0)
>>  		changed |= BSS_CHANGED_AP_PROBE_RESP;
>> 
>> +	if (params->ftm_responder) {
> 
> Here you immediately have the problem I talked about in the earlier
> patch ... yes, we don't have drivers that allow disabling it (so should
> probably reject that now), but without it you can *enable* it on the
> fly, but you cannot *disable*, in fact we assume saying "disable" means
> "no change" which is rather confusing.
> 
> johannes