Re: [RFC v2] cfg80211: add peer measurement with FTM API
On Wed, 2018-10-17 at 13:05 +0300, Lior David wrote: > > On 10/11/2018 1:05 PM, Johannes Berg wrote: > > > > > Thanks for the explanation. The send to same socket does sound more > > > efficient. > > > (In our internal implementation with vendor commands we were forced > > > to send the results as broadcast...) > > > > I suppose we can fix that, in the sense that we can add API to allow > > vendor commands to know the socket to send back to etc. > > > > I think that would be useful in general though as far as I know we don't have > any pending feature that requires it right now. Ok. For the record, I have no objection to somebody who needs this doing it, but it's not entirely trivial so I'm not going to just do it now. johannes
Re: [RFC v2] cfg80211: add peer measurement with FTM API
On 10/11/2018 1:05 PM, Johannes Berg wrote: > >> Thanks for the explanation. The send to same socket does sound more >> efficient. >> (In our internal implementation with vendor commands we were forced >> to send the results as broadcast...) > > I suppose we can fix that, in the sense that we can add API to allow > vendor commands to know the socket to send back to etc. > I think that would be useful in general though as far as I know we don't have any pending feature that requires it right now. [...] >> >> As I remember the driver/FW can easily find the connected channel for >> connected >> station. For unconnected station we should probably force this parameter. > > Should be able to, yes, but I suppose even userspace can. It just seems > like extra complexity to impose on the driver, since cfg80211 doesn't > necessarily have all the right information. > Agree with this. I guess we can keep the channel parameter mandatory. > Actually, hmm, that would imply "use maximum bandwidth" or something? > And then what if that bandwidth isn't possible with FTM for some reason? > It's a bit tricky then. > DMG always uses the entire channel (2160MHz). We may need to use partial channel in EDMG but not sure yet. We have an upcoming patch for EDMG support so we can deal with this later. >> >> Good point. I see we currently use 20_NOHT for DMG, guess we can continue >> using it. > > Well, I think it'd make more sense to just enforce the DMG bandwidth > everywhere, but I won't force the issue over this. > Ok. I think using 20_NOHT for now is more consistent since other APIs specify it for DMG channels. Thanks, Lior
Re: [RFC v2] cfg80211: add peer measurement with FTM API
On Sun, 2018-10-07 at 21:58 +0200, Johannes Berg wrote: > > > > + * @partial: indicates that this is a partial result for this type > > > > Is partial set to false for the last result of this measurement type? This > > may > > be useful, for example if requesting multiple measurement types, user space > > can > > start processing one measurement type before the entire session is > > completed. > > Yes, that was the intent, e.g. for multiple FTM bursts, but I see how > this might be misleading at this level. I'll clarify the documentation > (and probably over in nl80211.h as well) Actually, I changed my mind - I'll add a "final" bit as well, so "partial" will be set on all of them. johannes
Re: [RFC v2] cfg80211: add peer measurement with FTM API
On Tue, 2018-10-09 at 17:40 +0300, Lior David wrote: > Thanks for the explanation. The send to same socket does sound more efficient. > (In our internal implementation with vendor commands we were forced > to send the results as broadcast...) I suppose we can fix that, in the sense that we can add API to allow vendor commands to know the socket to send back to etc. > Yes as far as I remember rtt<->distance is a trivial calculation. What I meant > here, you return the average of few measurements done in a burst, and for > debugging we could return all the measurements done in the burst. For example > if > there were 4 measurements per burst return the 4 distance measurements done. Ah, OK. I guess you'd have to report separate measurements or something. Though if it's for debugging I'd think it may not make sense in this API but rather something out-of-band. > I meant we could have an AoA value (angle in degrees or other units) which > drivers can fill up if they want. For example if the driver has 2 antennas on > both sides it can report either 90 or 270 degrees. It will be usually very low > accuracy but at least can provide some information like from which side of the > AP you are. This can certainly be added later if at all (hopefully we will > have > AoA measurement with higher accuracy in the future) Yeah, ok, I get it now - I guess we can add it if somebody plays with it and finds how to obtain and use the value. > > > For connected station, usually you will want to do the measurement on the > > > connected channel (possibly some chips will not be able to do otherwise) > > > Maybe add option to use default channel? > > > > Perhaps. It's somewhat complicated to look up in general, userspace > > generally has a decent idea, and making it optional means you end up > > with an invalid chandef? > > > > I'll take a look, perhaps just leaving all the fields 0/NULL can work > > reasonably well, but drivers would have to support it. > > > > As I remember the driver/FW can easily find the connected channel for > connected > station. For unconnected station we should probably force this parameter. Should be able to, yes, but I suppose even userspace can. It just seems like extra complexity to impose on the driver, since cfg80211 doesn't necessarily have all the right information. Actually, hmm, that would imply "use maximum bandwidth" or something? And then what if that bandwidth isn't possible with FTM for some reason? It's a bit tricky then. > > If so, that sounds like something that generally needs improvement? > > > > Good point. I see we currently use 20_NOHT for DMG, guess we can continue > using it. Well, I think it'd make more sense to just enforce the DMG bandwidth everywhere, but I won't force the issue over this. > Ok I thought 15 meant to actually request 65535 bursts :-) > I still prefer the default to be 1 burst. Supporting the "no preference" means > it will be difficult to pre-allocate memory for burst results. Also all > drivers > should know to do one burst. Fair, I'll change it. johannes
Re: [RFC v2] cfg80211: add peer measurement with FTM API
On 10/7/2018 10:58 PM, Johannes Berg wrote:
>>
>> Just curious, what's the advantage of this compared to sending the reply
>> only on
>> the socket that started the measurement?
>
> TBH, I don't really see any. Some people really wanted this for things
> like "let's do something in iw for measurements", though even that is
> not strictly necessary since you can always start listening for events
> as (before) you send off your request. It's slightly more complicated in
> terms of socket handling (as you need to be able to handle events while
> waiting for the netlink ACK message) but that's not so bad.
>
> I'm all for it, so perhaps I'll change that.
Thanks for the explanation. The send to same socket does sound more efficient.
(In our internal implementation with vendor commands we were forced
to send the results as broadcast...)
>
> In that case, it might even make sense to continue with the simple "send
> out results to userspace as we get them" approach, since the application
> that made the request will know to dedicate a socket with socket buffer
> to it, and handle it quickly, avoiding the problems with running out of
> socket buffer space and losing the "measurement complete" event (that I
> was worried about with our [Intel] original code of just multicasting
> the results).
>
Sounds good. That way you may be able to pre-allocate memory for results... In
our implementation we pre-allocated and sent results for each burst as we
received it from FW (though not sure if it works for generic measurement types).
>>> + * @lci_len: length of LCI information (if present)
>>> + * @civicloc_len: length of civic location information (if present)
>>
>> Consider naming lcr (location civic report) instead of civicloc (this is
>> what we
>> used in our QCA vendor API)
>
> Hmm. I guess we already did "civic location" in the FTM-responder side
> API, but perhaps we can change it.
>
Ok with either naming. lcr has just a small advantage since it is shorter :-)
>>> + * @rtt_avg: average of RTTs measured (must have either this or @dist_avg)
>
>> For debugging it might be useful to report the distance in each measurement,
>
> We did in fact originally report the distance unconditionally (rather
> than RTT and/or distance) but it ends up a multiplication by the speed
> of light (in air, but it was approximate enough this didn't matter), but
> I felt that such a calculation is so easy to do we may as well do it in
> iw/userspace?
>
> Unless I'm misunderstanding?
Yes as far as I remember rtt<->distance is a trivial calculation. What I meant
here, you return the average of few measurements done in a burst, and for
debugging we could return all the measurements done in the burst. For example if
there were 4 measurements per burst return the 4 distance measurements done.
[...]
>> Also, Wigig chip has multiple antennas in a single array each covering a
>> sector,
>> and WiFi may have multiple antenna arrays where one or more will be used for
>> measurement, this means we may provide low-accuracy AoA result - at minimum
>> this
>> may tell you on which side of the AP you are... Consider adding this as
>> optional
>> reporting, not critical for initial patch...
>
> Hmm. I'm not sure we have the ability to distinguish the TOA by antenna,
> but I don't know. Frankly, I'm not even sure what you'd want to add
> here? Timestamp per antenna?
>
I meant we could have an AoA value (angle in degrees or other units) which
drivers can fill up if they want. For example if the driver has 2 antennas on
both sides it can report either 90 or 270 degrees. It will be usually very low
accuracy but at least can provide some information like from which side of the
AP you are. This can certainly be added later if at all (hopefully we will have
AoA measurement with higher accuracy in the future)
>>> +/**
>>> + * struct cfg80211_pmsr_request_peer - peer data for a peer measurement
>>> request
>>> + * @addr: MAC address
>>> + * @chandef: channel to use
>>
>> For connected station, usually you will want to do the measurement on the
>> connected channel (possibly some chips will not be able to do otherwise)
>> Maybe add option to use default channel?
>
> Perhaps. It's somewhat complicated to look up in general, userspace
> generally has a decent idea, and making it optional means you end up
> with an invalid chandef?
>
> I'll take a look, perhaps just leaving all the fields 0/NULL can work
> reasonably well, but drivers would have to support it.
>
As I remember the driver/FW can easily find the connected channel for connected
station. For unconnected station we should probably force this parameter.
>>> + * @report_ap_tsf: report the associated AP's TSF
>>> + * @ftm: FTM data, see in particular @ftm.requested
>>> + */
>>> +struct cfg80211_pmsr_request_peer {
>>> + u8 addr[ETH_ALEN];
>>> + struct cfg80211_chan_def chandef;
>>> + bool report_ap_tsf;
>>> + struct {
>>> + enum nl80211_preamble preamble;
>>> +
Re: [RFC v2] cfg80211: add peer measurement with FTM API
Hi Lior,
Thanks for taking the time :-)
> > Results availability is multicast on a new "peer-measurement"
> > multicast group, and results can be retrieved by dumping the
> > data given the measurement cookie. Note that dumping it from
> > the netlink socket that started the measurement will delete
> > data, to allow not hanging on to measurement data forever when
> > the measurement is long-running and only updates periodically.
>
> Just curious, what's the advantage of this compared to sending the reply only
> on
> the socket that started the measurement?
TBH, I don't really see any. Some people really wanted this for things
like "let's do something in iw for measurements", though even that is
not strictly necessary since you can always start listening for events
as (before) you send off your request. It's slightly more complicated in
terms of socket handling (as you need to be able to handle events while
waiting for the netlink ACK message) but that's not so bad.
I'm all for it, so perhaps I'll change that.
In that case, it might even make sense to continue with the simple "send
out results to userspace as we get them" approach, since the application
that made the request will know to dedicate a socket with socket buffer
to it, and handle it quickly, avoiding the problems with running out of
socket buffer space and losing the "measurement complete" event (that I
was worried about with our [Intel] original code of just multicasting
the results).
> > + * @lci_len: length of LCI information (if present)
> > + * @civicloc_len: length of civic location information (if present)
>
> Consider naming lcr (location civic report) instead of civicloc (this is what
> we
> used in our QCA vendor API)
Hmm. I guess we already did "civic location" in the FTM-responder side
API, but perhaps we can change it.
> > + * @rtt_avg: average of RTTs measured (must have either this or @dist_avg)
> For debugging it might be useful to report the distance in each measurement,
We did in fact originally report the distance unconditionally (rather
than RTT and/or distance) but it ends up a multiplication by the speed
of light (in air, but it was approximate enough this didn't matter), but
I felt that such a calculation is so easy to do we may as well do it in
iw/userspace?
Unless I'm misunderstanding?
> and
> also the raw T1-T4 values.
Makes sense.
> Having T1-T4 in user space also allow running clock
> drift correction algorithm, if only final value is reported the FW/driver
> should
> probably perform the correction. Anyhow guess this can be added in the future.
Yeah, we can add more fields - although then they're sort of immediately
optional ones :-)
> Also, Wigig chip has multiple antennas in a single array each covering a
> sector,
> and WiFi may have multiple antenna arrays where one or more will be used for
> measurement, this means we may provide low-accuracy AoA result - at minimum
> this
> may tell you on which side of the AP you are... Consider adding this as
> optional
> reporting, not critical for initial patch...
Hmm. I'm not sure we have the ability to distinguish the TOA by antenna,
but I don't know. Frankly, I'm not even sure what you'd want to add
here? Timestamp per antenna?
> > + * @partial: indicates that this is a partial result for this type
>
> Is partial set to false for the last result of this measurement type? This may
> be useful, for example if requesting multiple measurement types, user space
> can
> start processing one measurement type before the entire session is completed.
Yes, that was the intent, e.g. for multiple FTM bursts, but I see how
this might be misleading at this level. I'll clarify the documentation
(and probably over in nl80211.h as well)
> > +/**
> > + * struct cfg80211_pmsr_request_peer - peer data for a peer measurement
> > request
> > + * @addr: MAC address
> > + * @chandef: channel to use
>
> For connected station, usually you will want to do the measurement on the
> connected channel (possibly some chips will not be able to do otherwise)
> Maybe add option to use default channel?
Perhaps. It's somewhat complicated to look up in general, userspace
generally has a decent idea, and making it optional means you end up
with an invalid chandef?
I'll take a look, perhaps just leaving all the fields 0/NULL can work
reasonably well, but drivers would have to support it.
> > + * @report_ap_tsf: report the associated AP's TSF
> > + * @ftm: FTM data, see in particular @ftm.requested
> > + */
> > +struct cfg80211_pmsr_request_peer {
> > + u8 addr[ETH_ALEN];
> > + struct cfg80211_chan_def chandef;
> > + bool report_ap_tsf;
> > + struct {
> > + enum nl80211_preamble preamble;
> > + u16 burst_period;
> > + bool requested;
>
> Maybe "requested" should be the first field? it is common for all measurement
> structures?
It's required in all measurement types, but I don't think we care what
offset it has inside each, since
Re: [RFC v2] cfg80211: add peer measurement with FTM API
Hi Johannes,
On 10/1/2018 4:35 PM, Johannes Berg wrote:
> From: Johannes Berg
>
> Add a new "peer measurement" API, that can be used to measure
> certain things related to a peer. Right now, only implement
> FTM (flight time measurement) over it, but the idea is that
> it'll be extensible to also support measuring the necessary
> things to calculate e.g. angle-of-arrival for WiGig.
>
It's good that you started with FTM only, we are still not sure about how
to best support AoA measurement, so better add it as a future patch.
> The API is structured to have a generic list of peers and
> channels to measure with/on, and then for each of those a
> set of measurements (again, only FTM right now) to perform.
>
> Results availability is multicast on a new "peer-measurement"
> multicast group, and results can be retrieved by dumping the
> data given the measurement cookie. Note that dumping it from
> the netlink socket that started the measurement will delete
> data, to allow not hanging on to measurement data forever when
> the measurement is long-running and only updates periodically.
>
Just curious, what's the advantage of this compared to sending the reply only on
the socket that started the measurement?
> Similarly, closing the controlling netlink socket will abort
> a running measurement automatically.
>
> Signed-off-by: Johannes Berg
> ---
> include/net/cfg80211.h | 243
> include/uapi/linux/nl80211.h | 411 ++
> net/wireless/Makefile| 1 +
> net/wireless/core.c | 33 +++
> net/wireless/core.h | 4 +
> net/wireless/nl80211.c | 202 ++---
> net/wireless/nl80211.h | 42 +++
> net/wireless/pmsr.c | 678
> +++
> net/wireless/rdev-ops.h | 25 ++
> net/wireless/trace.h | 70 +
> 10 files changed, 1675 insertions(+), 34 deletions(-)
> create mode 100644 net/wireless/pmsr.c
>
> diff --git a/include/net/cfg80211.h b/include/net/cfg80211.h
> index 8f5ee2c2da04..f9d4872e5123 100644
> --- a/include/net/cfg80211.h
> +++ b/include/net/cfg80211.h
> @@ -2841,6 +2841,178 @@ struct cfg80211_ftm_responder_stats {
> u32 out_of_window_triggers_num;
> };
>
> +/**
> + * struct cfg80211_pmsr_ftm_result - FTM result
> + * @failure_reason: if this measurement failed (PMSR status is
> + * %NL80211_PMSR_STATUS_FAILURE), this gives a more precise
> + * reason than just "failure"
> + * @burst_index: if reporting partial results, this is the index
> + * in [0 .. num_bursts-1] of the burst that's being reported
> + * @num_ftmr_attempts: number of FTM request frames transmitted
> + * @num_ftmr_successes: number of FTM request frames acked
> + * @busy_retry_time: if failure_reason is
> %NL80211_PMSR_FTM_FAILURE_PEER_BUSY,
> + * fill this to indicate in how many seconds a retry is deemed possible
> + * by the responder
> + * @num_bursts_exp: actual number of bursts exponent negotiated
> + * @burst_duration: actual burst duration negotiated
> + * @frames_per_burst: actual frames per burst negotiated
> + * @lci_len: length of LCI information (if present)
> + * @civicloc_len: length of civic location information (if present)
Consider naming lcr (location civic report) instead of civicloc (this is what we
used in our QCA vendor API)
> + * @lci: LCI data (may be %NULL)
> + * @civicloc: civic location data (may be %NULL)
> + * @rssi_avg: average RSSI over FTM action frames reported
> + * @rssi_spread: spread of the RSSI over FTM action frames reported
> + * @tx_rate: bitrate for transmitted FTM action frame response
> + * @rx_rate: bitrate of received FTM action frame
> + * @rtt_avg: average of RTTs measured (must have either this or @dist_avg)
> + * @rtt_variance: variance of RTTs measured (note that standard deviation is
> + * the square root of the variance)
> + * @rtt_spread: spread of the RTTs measured
> + * @dist_avg: average of distances (mm) measured
> + * (must have either this or @rtt_avg)
> + * @dist_variance: variance of distances measured (see also @rtt_variance)
> + * @dist_spread: spread of distances measured (see also @rtt_spread)
For debugging it might be useful to report the distance in each measurement, and
also the raw T1-T4 values. Having T1-T4 in user space also allow running clock
drift correction algorithm, if only final value is reported the FW/driver should
probably perform the correction. Anyhow guess this can be added in the future.
Also, Wigig chip has multiple antennas in a single array each covering a sector,
and WiFi may have multiple antenna arrays where one or more will be used for
measurement, this means we may provide low-accuracy AoA result - at minimum this
may tell you on which side of the AP you are... Consider adding this as optional
reporting, not critical for initial patch...
> + * @num_ftmr_attempts_valid: @num_ftmr_attempts is valid
> + * @num_ftmr_successes_valid: @num_ftmr_successes is valid
> + *
