DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 6D79F2086A
Message-ID: <1539340898.3141.11.camel@sipsolutions.net>
Subject: Re: [RFC v3] cfg80211: add peer measurement with FTM API
From:   Johannes Berg <johannes@sipsolutions.net>
To:     Arend van Spriel <arend.vanspriel@broadcom.com>,
        linux-wireless@vger.kernel.org
Cc:     Pradeep Kumar Chitrapu <pradeepc@codeaurora.org>,
        Lior David <liord@codeaurora.org>, luca@coelho.fi,
        Etan Cohen <etancohen@google.com>,
        Roy Want <roywant@google.com>,
        Franky Lin <franky.lin@broadcom.com>
Date:   Fri, 12 Oct 2018 12:41:38 +0200
In-Reply-To: <dc9bc6d3-6a95-3ca5-a231-14ed3e2fe302@broadcom.com> (sfid-20181012_123207_395276_6619BC73)
References: <20181012100827.24195-1-johannes@sipsolutions.net>
         <dc9bc6d3-6a95-3ca5-a231-14ed3e2fe302@broadcom.com>
         (sfid-20181012_123207_395276_6619BC73)
Content-Type: text/plain; charset="UTF-8"
Mime-Version: 1.0
Content-Transfer-Encoding: 7bit
Sender: linux-wireless-owner@vger.kernel.org
Precedence: bulk


> You have no recollection what happened in the earlier versions, right? :-p

v1 was very incomplete, it didn't have any results reporting, etc.

> > v3:
> >  - add a bit to report "final" for partial results
> >  - remove list keeping etc. and just unicast out the results
> >    to the requester (big code reduction ...)
> >  - also send complete message unicast, and as a result
> >    remove the multicast group
> >  - separate out struct cfg80211_pmsr_ftm_request_peer
> >    from struct cfg80211_pmsr_request_peer
> >  - document timeout == 0 if no timeout
> >  - disallow setting timeout nl80211 attribute to 0,
> >    must not include attribute for no timeout
> 
> All these negations make my head spin (a little). Let's look at the 
> actual documentation further down...

:-)

> > +struct cfg80211_pmsr_ftm_result {
> > +	const u8 *lci;
> > +	const u8 *civicloc;
> > +	unsigned int lci_len;
> > +	unsigned int civicloc_len;
> > +	enum nl80211_peer_measurement_ftm_failure_reasons failure_reason;
> > +	u32 num_ftmr_attempts, num_ftmr_successes;
> 
> Maybe there is a good reason, but can we move the above line a bit down...

The reason was to avoid having padding for alignment.

> > +	NL80211_ATTR_TIMEOUT,
> > +
> 
> Guess you consider reuse of the TIMEOUT attribute? 

Yes, I was actually surprised we don't have one already :-)

> I checked the policy 
> definition in nl80211_policy so it disallows 0 value as mentioned in the 
> changelog. How about adding that to the documentation here, ie. "when 
> timeout attribute is not provided the timeout is disabled for the given 
> operation" or something like that.

Sure, that makes sense, will do.

johannes