Re: [PATCH v4 1/3] cfg80211: support FTM responder configuration/statistics

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

On 2018-09-18 00:52, Johannes Berg wrote:
> Hi,
> 
> Sorry for the delay with this. I was a bit fed up by all the resends,
> but I see the latest one did in fact finally make it to the list and
> patchwork.
> (As an aside - it would've helped to actually bump the version number
> for every resend, even if it's identical - now I don't know which
> version to reply to in my email so it goes to patchwork)
Hi Johannes,

Sorry for the trouble. Hopefully, there will not be any issues for 
future
patches, However, I will keep this in mind..
> 
> 
>> + * @civic: CIVIC subelement content
> 
>> + * @civic_len: Civic data length
> 
> I was continuing work on the FTM initiator, and I think now that we
> should call this "civicloc", otherwise we're missing the arguably more
> important part of "civic location".
Sure, I will make this change in next next version.
> 
>> + * @NL80211_ATTR_FTM_RESPONDER: attribute which user-space can 
>> include in
>> + *	%NL80211_CMD_START_AP or %NL80211_CMD_SET_BEACON to enable(1)
>> + *	fine timing measurement (FTM) responder functionality.
>> + * @NL80211_ATTR_LCI: The content of Measurement Report Element 
>> (9.4.2.22
>> + *	in 802.11-2016) with type 8 - LCI (9.4.2.22.10)
>> + * @NL80211_ATTR_CIVIC: The content of Measurement Report Element 
>> (9.4.2.22
>> + *	in 802.11-2016) with type 11 - Civic (Section 9.4.2.22.13)
> 
> Again, thinking about the FTM initiator side code, I think we're
> probably better off to nest these.
> 
> NL80211_ATTR_FTM_RESPONDER -> nested
> 
> enum nl80211_ftm_responder_attr {
> 	NL80211_FTM_RESP_ATTR_INVALID,
> 
> 	NL80211_FTM_RESP_ATTR_ENABLED,
> 	NL80211_FTM_RESP_ATTR_LCI,
> 	NL80211_FTM_RESP_ATTR_CIVICLOC,
> 
> 	NUM/MAX...
> };
> 
> That way we can extend this very easily in the future and don't need
> these attributes at the top level.
> 
> The logic also makes sense in the beacon change command - if you don't
> include the NL80211_ATTR_FTM_RESPONDER then it means no change; if you
> do include it the new settings within it (which may be completely empty
> to disable it) should take effect.
> 
>> +/**
>> + * enum nl80211_ftm_responder_state - fine timing measurement 
>> responder state
>> + * @NL80211_FTM_RESP_DISABLED: FTM responder is disabled
>> + * @NL80211_FTM_RESP_ENABLED: FTM responder is enabled
>> + */
>> +enum nl80211_ftm_responder_state {
>> +	NL80211_FTM_RESP_DISABLED,
>> +	NL80211_FTM_RESP_ENABLED,
>> +};
> 
> We won't need this either then.
> 
>> +	[NL80211_ATTR_FTM_RESPONDER] = { .type = NLA_U8 },
> 
> And that of course becomes NLA_NESTED
> 
>> +	[NL80211_ATTR_LCI] = { .type = NLA_BINARY },
>> +	[NL80211_ATTR_CIVIC] = { .type = NLA_BINARY },
> 
> with those moving to a separate policy.
> 
> What do you think?
This makes sense.. I will make these changes and send next patch 
version.
> 
> johannes