[PATCH 3/3] ath10k: Add support to configure ftm responder role

[Pradeep Kumar Chitrapu]

Configure fine timing measurement (FTM) responder role from the
ftm_responder bss param sent by mac80211. With FTM functionality
offloaded to firmware, adding the interface allows userspace
to enable or disable FTM responder functionality. ath10k disables
it at the time of interface creation.

Supported FW: 10.4

Tested on QCA9984 with debug firmware: 10.4-3.2.1-00121

Signed-off-by: Pradeep Kumar Chitrapu <***@codeaurora.org>
---
drivers/net/wireless/ath/ath10k/core.h | 1 +
drivers/net/wireless/ath/ath10k/mac.c | 29 +++++++++++++++++++++++++++++
drivers/net/wireless/ath/ath10k/wmi.c | 4 ++++
drivers/net/wireless/ath/ath10k/wmi.h | 10 ++++++++++
4 files changed, 44 insertions(+)

diff --git a/drivers/net/wireless/ath/ath10k/core.h b/drivers/net/wireless/ath/ath10k/core.h
index 9feea02e7d37..db190230b292 100644
--- a/drivers/net/wireless/ath/ath10k/core.h
+++ b/drivers/net/wireless/ath/ath10k/core.h
@@ -544,6 +544,7 @@ struct ath10k_vif {
bool nohwcrypt;
int num_legacy_stations;
int txpower;
+ bool ftm_responder;
struct wmi_wmm_params_all_arg wmm_params;
struct work_struct ap_csa_work;
struct delayed_work connection_loss_work;
diff --git a/drivers/net/wireless/ath/ath10k/mac.c b/drivers/net/wireless/ath/ath10k/mac.c
index 90f9372dec25..cdb73b8cc253 100644
--- a/drivers/net/wireless/ath/ath10k/mac.c
+++ b/drivers/net/wireless/ath/ath10k/mac.c
@@ -5259,6 +5259,17 @@ static int ath10k_add_interface(struct ieee80211_hw *hw,
goto err_peer_delete;
}

+ if (test_bit(WMI_SERVICE_RTT, ar->wmi.svc_map)) {
+ vdev_param = ar->wmi.vdev_param->rtt_responder_role;
+ ret = ath10k_wmi_vdev_set_param(ar, arvif->vdev_id, vdev_param,
+ arvif->ftm_responder);
+
+ /* It is harmless to not set FTM role. Do not warn */
+ if (ret && ret != -EOPNOTSUPP)
+ ath10k_warn(ar, "failed to set vdev %i FTM Responder: %d\n",
+ arvif->vdev_id, ret);
+ }
+
if (vif->type == NL80211_IFTYPE_MONITOR) {
ar->monitor_arvif = arvif;
ret = ath10k_monitor_recalc(ar);
@@ -5532,6 +5543,20 @@ static void ath10k_bss_info_changed(struct ieee80211_hw *hw,
if (changed & BSS_CHANGED_BSSID && !is_zero_ether_addr(info->bssid))
ether_addr_copy(arvif->bssid, info->bssid);

+ if (changed & BSS_CHANGED_FTM_RESPONDER &&
+ arvif->ftm_responder != info->ftm_responder &&
+ test_bit(WMI_SERVICE_RTT, ar->wmi.svc_map)) {
+ arvif->ftm_responder = info->ftm_responder;
+
+ vdev_param = ar->wmi.vdev_param->rtt_responder_role;
+ ret = ath10k_wmi_vdev_set_param(ar, arvif->vdev_id, vdev_param,
+ arvif->ftm_responder);
+
+ ath10k_dbg(ar, ATH10K_DBG_MAC,
+ "mac vdev %d ftm_responder %d:ret %d\n",
+ arvif->vdev_id, arvif->ftm_responder, ret);
+ }
+
if (changed & BSS_CHANGED_BEACON_ENABLED)
ath10k_control_beaconing(arvif, info);

@@ -8463,6 +8488,10 @@ int ath10k_mac_register(struct ath10k *ar)
wiphy_ext_feature_set(ar->hw->wiphy,
NL80211_EXT_FEATURE_SET_SCAN_DWELL);

+ if (test_bit(WMI_SERVICE_RTT, ar->wmi.svc_map))
+ wiphy_ext_feature_set(ar->hw->wiphy,
+ NL80211_EXT_FEATURE_SET_FTM_RESPONDER);
+
/*
* on LL hardware queues are managed entirely by the FW
* so we only advertise to mac we can do the queues thing
diff --git a/drivers/net/wireless/ath/ath10k/wmi.c b/drivers/net/wireless/ath/ath10k/wmi.c
index fd612d2905b0..00c67d53eab8 100644
--- a/drivers/net/wireless/ath/ath10k/wmi.c
+++ b/drivers/net/wireless/ath/ath10k/wmi.c
@@ -825,6 +825,7 @@
.meru_vc = WMI_VDEV_PARAM_UNSUPPORTED,
.rx_decap_type = WMI_VDEV_PARAM_UNSUPPORTED,
.bw_nss_ratemask = WMI_VDEV_PARAM_UNSUPPORTED,
+ .rtt_responder_role = WMI_VDEV_PARAM_UNSUPPORTED,
};

/* 10.X WMI VDEV param map */
@@ -900,6 +901,7 @@
.meru_vc = WMI_VDEV_PARAM_UNSUPPORTED,
.rx_decap_type = WMI_VDEV_PARAM_UNSUPPORTED,
.bw_nss_ratemask = WMI_VDEV_PARAM_UNSUPPORTED,
+ .rtt_responder_role = WMI_VDEV_PARAM_UNSUPPORTED,
};

static struct wmi_vdev_param_map wmi_10_2_4_vdev_param_map = {
@@ -974,6 +976,7 @@
.meru_vc = WMI_VDEV_PARAM_UNSUPPORTED,
.rx_decap_type = WMI_VDEV_PARAM_UNSUPPORTED,
.bw_nss_ratemask = WMI_VDEV_PARAM_UNSUPPORTED,
+ .rtt_responder_role = WMI_VDEV_PARAM_UNSUPPORTED,
};

static struct wmi_vdev_param_map wmi_10_4_vdev_param_map = {
@@ -1051,6 +1054,7 @@
.bw_nss_ratemask = WMI_10_4_VDEV_PARAM_BW_NSS_RATEMASK,
.inc_tsf = WMI_10_4_VDEV_PARAM_TSF_INCREMENT,
.dec_tsf = WMI_10_4_VDEV_PARAM_TSF_DECREMENT,
+ .rtt_responder_role = WMI_10_4_VDEV_PARAM_ENABLE_DISABLE_RTT_RESPONDER_ROLE,
};

static struct wmi_pdev_param_map wmi_pdev_param_map = {
diff --git a/drivers/net/wireless/ath/ath10k/wmi.h b/drivers/net/wireless/ath/ath10k/wmi.h
index 36220258e3c7..fa94873fe46f 100644
--- a/drivers/net/wireless/ath/ath10k/wmi.h
+++ b/drivers/net/wireless/ath/ath10k/wmi.h
@@ -5035,6 +5035,7 @@ struct wmi_vdev_param_map {
u32 bw_nss_ratemask;
u32 inc_tsf;
u32 dec_tsf;
+ u32 rtt_responder_role;
};

#define WMI_VDEV_PARAM_UNSUPPORTED 0
@@ -5374,6 +5375,15 @@ enum wmi_10_4_vdev_param {
WMI_10_4_VDEV_PARAM_ATF_SSID_SCHED_POLICY,
WMI_10_4_VDEV_PARAM_DISABLE_DYN_BW_RTS,
WMI_10_4_VDEV_PARAM_TSF_DECREMENT,
+ WMI_10_4_VDEV_PARAM_SELFGEN_FIXED_RATE,
+ WMI_10_4_VDEV_PARAM_AMPDU_SUBFRAME_SIZE_PER_AC,
+ WMI_10_4_VDEV_PARAM_NSS_VHT160,
+ WMI_10_4_VDEV_PARAM_NSS_VHT80_80,
+ WMI_10_4_VDEV_PARAM_AMSDU_SUBFRAME_SIZE_PER_AC,
+ WMI_10_4_VDEV_PARAM_DISABLE_CABQ,
+ WMI_10_4_VDEV_PARAM_SIFS_TRIGGER_RATE,
+ WMI_10_4_VDEV_PARAM_TX_POWER,
+ WMI_10_4_VDEV_PARAM_ENABLE_DISABLE_RTT_RESPONDER_ROLE,
};

#define WMI_VDEV_PARAM_TXBF_SU_TX_BFEE BIT(0)
--
1.9.1

[Pradeep Kumar Chitrapu]

Allow userspace to enable or disable fine timing measurement
responder functionality in AP mode. This can be done at AP start.

A new EXT_FEATURE flag is introduced for drivers to advertise
the capability.

Signed-off-by: Pradeep Kumar Chitrapu <***@codeaurora.org>
---
include/net/cfg80211.h | 2 ++
include/uapi/linux/nl80211.h | 20 ++++++++++++++++++++
net/wireless/nl80211.c | 19 +++++++++++++++++++
3 files changed, 41 insertions(+)

diff --git a/include/net/cfg80211.h b/include/net/cfg80211.h
index 9a850973e09a..beb383a41657 100644
--- a/include/net/cfg80211.h
+++ b/include/net/cfg80211.h
@@ -851,6 +851,7 @@ struct cfg80211_bitrate_mask {
* @vht_cap: VHT capabilities (or %NULL if VHT isn't enabled)
* @ht_required: stations must support HT
* @vht_required: stations must support VHT
+ * @ftm_responder: enable or disable FTM responder functionality
*/
struct cfg80211_ap_settings {
struct cfg80211_chan_def chandef;
@@ -875,6 +876,7 @@ struct cfg80211_ap_settings {
const struct ieee80211_ht_cap *ht_cap;
const struct ieee80211_vht_cap *vht_cap;
bool ht_required, vht_required;
+ int ftm_responder;
};

/**
diff --git a/include/uapi/linux/nl80211.h b/include/uapi/linux/nl80211.h
index 7acc16f34942..0a64034343c7 100644
--- a/include/uapi/linux/nl80211.h
+++ b/include/uapi/linux/nl80211.h
@@ -2241,6 +2241,11 @@ enum nl80211_commands {
* association request when used with NL80211_CMD_NEW_STATION). Can be set
* only if %NL80211_STA_FLAG_WME is set.
*
+ * @NL80211_ATTR_FTM_RESPONDER: attribute which user-space can include in
+ * %NL80211_CMD_START_AP to enable(1) or disable(0) fine timing measurement
+ * (FTM) responder functionality. If not set, it means don't care and
+ * the device will decide what to use.
+ *
* @NUM_NL80211_ATTR: total number of nl80211_attrs available
* @NL80211_ATTR_MAX: highest attribute number currently defined
* @__NL80211_ATTR_AFTER_LAST: internal use
@@ -2682,6 +2687,8 @@ enum nl80211_attrs {

NL80211_ATTR_HE_CAPABILITY,

+ NL80211_ATTR_FTM_RESPONDER,
+
/* add attributes here, update the policy in nl80211.c */

__NL80211_ATTR_AFTER_LAST,
@@ -5223,6 +5230,8 @@ enum nl80211_feature_flags {
* @NL80211_EXT_FEATURE_SCAN_MIN_PREQ_CONTENT: Driver/device can omit all data
* except for supported rates from the probe request content if requested
* by the %NL80211_SCAN_FLAG_MIN_PREQ_CONTENT flag.
+ * @NL80211_EXT_FEATURE_SET_FTM_RESPONDER: Driver supports enabling and
+ * disabling fine timing measurement responder role.
*
* @NUM_NL80211_EXT_FEATURES: number of extended features.
* @MAX_NL80211_EXT_FEATURES: highest extended feature index.
@@ -5259,6 +5268,7 @@ enum nl80211_ext_feature_index {
NL80211_EXT_FEATURE_TXQS,
NL80211_EXT_FEATURE_SCAN_RANDOM_SN,
NL80211_EXT_FEATURE_SCAN_MIN_PREQ_CONTENT,
+ NL80211_EXT_FEATURE_SET_FTM_RESPONDER,

/* add new features before the definition below */
NUM_NL80211_EXT_FEATURES,
@@ -5798,4 +5808,14 @@ enum nl80211_external_auth_action {
NL80211_EXTERNAL_AUTH_ABORT,
};

+/**
+ * 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,
+};
+
#endif /* __LINUX_NL80211_H */
diff --git a/net/wireless/nl80211.c b/net/wireless/nl80211.c
index 5fb9b7dd9831..dfdc1cb07add 100644
--- a/net/wireless/nl80211.c
+++ b/net/wireless/nl80211.c
@@ -430,6 +430,7 @@ enum nl80211_multicast_groups {
[NL80211_ATTR_TXQ_QUANTUM] = { .type = NLA_U32 },
[NL80211_ATTR_HE_CAPABILITY] = { .type = NLA_BINARY,
.len = NL80211_HE_MAX_CAPABILITY_LEN },
+ [NL80211_ATTR_FTM_RESPONDER] = { .type = NLA_U32},
};

/* policy for the key attributes */
@@ -4339,6 +4340,24 @@ static int nl80211_start_ap(struct sk_buff *skb, struct genl_info *info)
return PTR_ERR(params.acl);
}

+ params.ftm_responder = -1;
+ if (info->attrs[NL80211_ATTR_FTM_RESPONDER]) {
+ if (!wiphy_ext_feature_isset(
+ &rdev->wiphy,
+ NL80211_EXT_FEATURE_SET_FTM_RESPONDER)) {
+ GENL_SET_ERR_MSG(info,
+ "FTM Responder config not supported\n");
+ return -EOPNOTSUPP;
+ }
+
+ params.ftm_responder =
+ nla_get_u32(info->attrs[NL80211_ATTR_FTM_RESPONDER]);
+
+ if (params.ftm_responder != NL80211_FTM_RESP_DISABLED &&
+ params.ftm_responder != NL80211_FTM_RESP_ENABLED)
+ return -EINVAL;
+ }
+
nl80211_calculate_ap_params(&params);

wdev_lock(wdev);
--
1.9.1

[Johannes Berg]

On Tue, 2018-08-14 at 17:30 -0700, Pradeep Kumar Chitrapu wrote:
>
> + int ftm_responder;

bool

> + * @NL80211_ATTR_FTM_RESPONDER: attribute which user-space can include in
> + * %NL80211_CMD_START_AP to enable(1) or disable(0) fine timing measurement
> + * (FTM) responder functionality. If not set, it means don't care and
> + * the device will decide what to use.

Why 0/1 instead of a flag attribute?

Also, I think you need the attributes (LCI/Civic location)?

johannes

[p***@codeaurora.org]

On 2018-08-15 05:09, Johannes Berg wrote:
> On Tue, 2018-08-14 at 17:30 -0700, Pradeep Kumar Chitrapu wrote:
>>
>> + int ftm_responder;
>
> bool
>
>> + * @NL80211_ATTR_FTM_RESPONDER: attribute which user-space can
>> include in
>> + * %NL80211_CMD_START_AP to enable(1) or disable(0) fine timing
>> measurement
>> + * (FTM) responder functionality. If not set, it means don't care
>> and
>> + * the device will decide what to use.
>
> Why 0/1 instead of a flag attribute?
Using FLAG attribute may create uncertainty to userspace especially when
flag attribute
is not set and the driver supports capability, instead of
assuming/relying on the default
behavior of driver/firmware.

>
> Also, I think you need the attributes (LCI/Civic location)?
>
> johannes

[Johannes Berg]

On Wed, 2018-08-15 at 18:48 -0700, ***@codeaurora.org wrote:
>
> Using FLAG attribute may create uncertainty to userspace especially when
> flag attribute
> is not set and the driver supports capability, instead of
> assuming/relying on the default
> behavior of driver/firmware.

I don't think there should be any enabling it by default in firmware,
and as such I think that's just a (separate) driver bug - I'm not
convinced we should build the API with driver bugs in mind.

johannes

[Pradeep Kumar Chitrapu]

This post might be inappropriate. Click to display it.

[Pradeep Kumar Chitrapu]

New bss param ftm_responder is used to notify the driver to
enable or disable fine timing request (FTM) responder role in AP mode.

Signed-off-by: Pradeep Kumar Chitrapu <***@codeaurora.org>
---
include/net/mac80211.h | 4 ++++
net/mac80211/cfg.c | 4 ++++
net/mac80211/util.c | 3 +++
3 files changed, 11 insertions(+)

diff --git a/include/net/mac80211.h b/include/net/mac80211.h
index 5790f55c241d..6593c611655b 100644
--- a/include/net/mac80211.h
+++ b/include/net/mac80211.h
@@ -308,6 +308,8 @@ struct ieee80211_vif_chanctx_switch {
* @BSS_CHANGED_KEEP_ALIVE: keep alive options (idle period or protected
* keep alive) changed.
* @BSS_CHANGED_MCAST_RATE: Multicast Rate setting changed for this interface
+ * @BSS_CHANGED_FTM_RESPONDER: fime timing reasurement request responder
+ * functionality changed for this BSS (AP mode).
*
*/
enum ieee80211_bss_change {
@@ -337,6 +339,7 @@ enum ieee80211_bss_change {
BSS_CHANGED_MU_GROUPS = 1<<23,
BSS_CHANGED_KEEP_ALIVE = 1<<24,
BSS_CHANGED_MCAST_RATE = 1<<25,
+ BSS_CHANGED_FTM_RESPONDER = 1<<26,

/* when adding here, make sure to change ieee80211_reconfig */
};
@@ -611,6 +614,7 @@ struct ieee80211_bss_conf {
bool allow_p2p_go_ps;
u16 max_idle_period;
bool protected_keep_alive;
+ int ftm_responder;
};

/**
diff --git a/net/mac80211/cfg.c b/net/mac80211/cfg.c
index d25da0e66da1..75e314b826fb 100644
--- a/net/mac80211/cfg.c
+++ b/net/mac80211/cfg.c
@@ -964,6 +964,10 @@ static int ieee80211_start_ap(struct wiphy *wiphy, struct net_device *dev,
sdata->vif.bss_conf.p2p_noa_attr.oppps_ctwindow |=
IEEE80211_P2P_OPPPS_ENABLE_BIT;

+ sdata->vif.bss_conf.ftm_responder = params->ftm_responder;
+ if (params->ftm_responder >= 0)
+ changed |= BSS_CHANGED_FTM_RESPONDER;
+
err = ieee80211_assign_beacon(sdata, &params->beacon, NULL);
if (err < 0) {
ieee80211_vif_release_channel(sdata);
diff --git a/net/mac80211/util.c b/net/mac80211/util.c
index 88efda7c9f8a..835fcf01fabc 100644
--- a/net/mac80211/util.c
+++ b/net/mac80211/util.c
@@ -2076,6 +2076,9 @@ int ieee80211_reconfig(struct ieee80211_local *local)
case NL80211_IFTYPE_AP:
changed |= BSS_CHANGED_SSID | BSS_CHANGED_P2P_PS;

+ if (sdata->vif.bss_conf.ftm_responder >= 0)
+ changed |= BSS_CHANGED_FTM_RESPONDER;
+
if (sdata->vif.type == NL80211_IFTYPE_AP) {
changed |= BSS_CHANGED_AP_PROBE_RESP;

--
1.9.1

[Johannes Berg]

On Tue, 2018-08-14 at 17:30 -0700, Pradeep Kumar Chitrapu wrote:
> New bss param ftm_responder is used to notify the driver to
> enable or disable fine timing request (FTM) responder role in AP mode.
>
> + * @BSS_CHANGED_FTM_RESPONDER: fime timing reasurement request responder
> + * functionality changed for this BSS (AP mode).

You don't really need this since it can only be configured the first
time you start the AP.

johannes

[Pradeep Kumar Chitrapu]

New bss param ftm_responder is used to notify the driver to
enable fine timing request (FTM) responder role in AP mode.

Plumb the new cfg80211 API for FTM responder statistics through to
the driver API in mac80211.

Signed-off-by: David Spinadel <***@intel.com>
Signed-off-by: Johannes Berg <***@intel.com>
Signed-off-by: Pradeep Kumar Chitrapu <***@codeaurora.org>
---
include/net/mac80211.h | 12 ++++++++
net/mac80211/cfg.c | 74 +++++++++++++++++++++++++++++++++++++++++++++++
net/mac80211/driver-ops.h | 16 ++++++++++
net/mac80211/trace.h | 23 +++++++++++++++
net/mac80211/util.c | 3 ++
5 files changed, 128 insertions(+)

diff --git a/include/net/mac80211.h b/include/net/mac80211.h
index 5790f55c241d..db1e505e884f 100644
--- a/include/net/mac80211.h
+++ b/include/net/mac80211.h
@@ -308,6 +308,8 @@ struct ieee80211_vif_chanctx_switch {
* @BSS_CHANGED_KEEP_ALIVE: keep alive options (idle period or protected
* keep alive) changed.
* @BSS_CHANGED_MCAST_RATE: Multicast Rate setting changed for this interface
+ * @BSS_CHANGED_FTM_RESPONDER: fime timing reasurement request responder
+ * functionality changed for this BSS (AP mode).
*
*/
enum ieee80211_bss_change {
@@ -337,6 +339,7 @@ enum ieee80211_bss_change {
BSS_CHANGED_MU_GROUPS = 1<<23,
BSS_CHANGED_KEEP_ALIVE = 1<<24,
BSS_CHANGED_MCAST_RATE = 1<<25,
+ BSS_CHANGED_FTM_RESPONDER = 1<<26,

/* when adding here, make sure to change ieee80211_reconfig */
};
@@ -561,6 +564,8 @@ struct ieee80211_mu_group_data {
* @protected_keep_alive: if set, indicates that the station should send an RSN
* protected frame to the AP to reset the idle timer at the AP for the
* station.
+ * @ftm_responder: whether to enable fine timing measurement FTM functionality
+ * @ftmr_params: configurable lci/civic parameter when enabling FTM responder.
*/
struct ieee80211_bss_conf {
const u8 *bssid;
@@ -611,6 +616,8 @@ struct ieee80211_bss_conf {
bool allow_p2p_go_ps;
u16 max_idle_period;
bool protected_keep_alive;
+ bool ftm_responder;
+ struct cfg80211_ftm_responder_params *ftmr_params;
};

/**
@@ -3542,6 +3549,8 @@ enum ieee80211_reconfig_type {
* @del_nan_func: Remove a NAN function. The driver must call
* ieee80211_nan_func_terminated() with
* NL80211_NAN_FUNC_TERM_REASON_USER_REQUEST reason code upon removal.
+ * @get_ftm_responder_stats: Retrieve FTM responder statistics, if available.
+ * Statistics should be cumulative, currently no way to reset is provided.
*/
struct ieee80211_ops {
void (*tx)(struct ieee80211_hw *hw,
@@ -3824,6 +3833,9 @@ struct ieee80211_ops {
void (*del_nan_func)(struct ieee80211_hw *hw,
struct ieee80211_vif *vif,
u8 instance_id);
+ int (*get_ftm_responder_stats)(struct ieee80211_hw *hw,
+ struct ieee80211_vif *vif,
+ struct cfg80211_ftm_responder_stats *ftm_stats);
};

/**
diff --git a/net/mac80211/cfg.c b/net/mac80211/cfg.c
index d25da0e66da1..4b2dd6bfd58c 100644
--- a/net/mac80211/cfg.c
+++ b/net/mac80211/cfg.c
@@ -792,6 +792,40 @@ static int ieee80211_set_probe_resp(struct ieee80211_sub_if_data *sdata,
return 0;
}

+static int ieee80211_set_ftm_responder_params(
+ struct ieee80211_sub_if_data *sdata,
+ const u8 *lci, size_t lci_len,
+ const u8 *civic, size_t civic_len)
+{
+ struct cfg80211_ftm_responder_params *new, *old;
+ struct ieee80211_bss_conf *bss_conf;
+
+ if ((!lci || !lci_len) && (!civic || !civic_len))
+ return 1;
+
+ bss_conf = &sdata->vif.bss_conf;
+
+ old = bss_conf->ftmr_params;
+
+ new = kzalloc(sizeof(*new) + lci_len + civic_len, GFP_KERNEL);
+ if (!new)
+ return -ENOMEM;
+
+ if (lci)
+ memcpy(new->lci, lci, lci_len);
+
+ if (civic)
+ memcpy(new->civic, civic, civic_len);
+
+ new->lci_len = lci_len;
+ new->civic_len = civic_len;
+ bss_conf->ftmr_params = new;
+
+ kfree(old);
+
+ return 0;
+}
+
static int ieee80211_assign_beacon(struct ieee80211_sub_if_data *sdata,
struct cfg80211_beacon_data *params,
const struct ieee80211_csa_settings *csa)
@@ -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) {
+ sdata->vif.bss_conf.ftm_responder = 1;
+ err = ieee80211_set_ftm_responder_params(sdata,
+ params->lci,
+ params->lci_len,
+ params->civic,
+ params->civic_len);
+
+ if (err < 0)
+ return err;
+
+ changed |= BSS_CHANGED_FTM_RESPONDER;
+ }
+
rcu_assign_pointer(sdata->u.ap.beacon, new);

if (old)
@@ -2918,6 +2966,20 @@ static int ieee80211_start_radar_detection(struct wiphy *wiphy,
memcpy(pos, beacon->probe_resp, beacon->probe_resp_len);
pos += beacon->probe_resp_len;
}
+ if (beacon->ftm_responder)
+ new_beacon->ftm_responder = beacon->ftm_responder;
+ if (beacon->lci) {
+ new_beacon->lci_len = beacon->lci_len;
+ new_beacon->lci = pos;
+ memcpy(pos, beacon->lci, beacon->lci_len);
+ pos += beacon->lci_len;
+ }
+ if (beacon->civic) {
+ new_beacon->civic_len = beacon->civic_len;
+ new_beacon->civic = pos;
+ memcpy(pos, beacon->civic, beacon->civic_len);
+ pos += beacon->civic_len;
+ }

return new_beacon;
}
@@ -3808,6 +3870,17 @@ static int ieee80211_get_txq_stats(struct wiphy *wiphy,
return ret;
}

+static int
+ieee80211_get_ftm_responder_stats(struct wiphy *wiphy,
+ struct net_device *dev,
+ struct cfg80211_ftm_responder_stats *ftm_stats)
+{
+ struct ieee80211_local *local = wiphy_priv(wiphy);
+ struct ieee80211_sub_if_data *sdata = IEEE80211_DEV_TO_SUB_IF(dev);
+
+ return drv_get_ftm_responder_stats(local, sdata, ftm_stats);
+}
+
const struct cfg80211_ops mac80211_config_ops = {
.add_virtual_intf = ieee80211_add_iface,
.del_virtual_intf = ieee80211_del_iface,
@@ -3902,4 +3975,5 @@ static int ieee80211_get_txq_stats(struct wiphy *wiphy,
.set_multicast_to_unicast = ieee80211_set_multicast_to_unicast,
.tx_control_port = ieee80211_tx_control_port,
.get_txq_stats = ieee80211_get_txq_stats,
+ .get_ftm_responder_stats = ieee80211_get_ftm_responder_stats,
};
diff --git a/net/mac80211/driver-ops.h b/net/mac80211/driver-ops.h
index 8f6998091d26..16878f7310ab 100644
--- a/net/mac80211/driver-ops.h
+++ b/net/mac80211/driver-ops.h
@@ -1173,6 +1173,22 @@ static inline void drv_wake_tx_queue(struct ieee80211_local *local,
local->ops->wake_tx_queue(&local->hw, &txq->txq);
}

+static inline int
+drv_get_ftm_responder_stats(struct ieee80211_local *local,
+ struct ieee80211_sub_if_data *sdata,
+ struct cfg80211_ftm_responder_stats *ftm_stats)
+{
+ u32 ret = -EOPNOTSUPP;
+
+ if (local->ops->get_ftm_responder_stats)
+ ret = local->ops->get_ftm_responder_stats(&local->hw,
+ &sdata->vif,
+ ftm_stats);
+ trace_drv_get_ftm_responder_stats(local, sdata, ftm_stats);
+
+ return ret;
+}
+
static inline int drv_start_nan(struct ieee80211_local *local,
struct ieee80211_sub_if_data *sdata,
struct cfg80211_nan_conf *conf)
diff --git a/net/mac80211/trace.h b/net/mac80211/trace.h
index 0ab69a1964f8..588c51a67c89 100644
--- a/net/mac80211/trace.h
+++ b/net/mac80211/trace.h
@@ -2600,6 +2600,29 @@ struct trace_switch_entry {
)
);

+TRACE_EVENT(drv_get_ftm_responder_stats,
+ TP_PROTO(struct ieee80211_local *local,
+ struct ieee80211_sub_if_data *sdata,
+ struct cfg80211_ftm_responder_stats *ftm_stats),
+
+ TP_ARGS(local, sdata, ftm_stats),
+
+ TP_STRUCT__entry(
+ LOCAL_ENTRY
+ VIF_ENTRY
+ ),
+
+ TP_fast_assign(
+ LOCAL_ASSIGN;
+ VIF_ASSIGN;
+ ),
+
+ TP_printk(
+ LOCAL_PR_FMT VIF_PR_FMT,
+ LOCAL_PR_ARG, VIF_PR_ARG
+ )
+);
+
#endif /* !__MAC80211_DRIVER_TRACE || TRACE_HEADER_MULTI_READ */

#undef TRACE_INCLUDE_PATH
diff --git a/net/mac80211/util.c b/net/mac80211/util.c
index 88efda7c9f8a..787405d483bf 100644
--- a/net/mac80211/util.c
+++ b/net/mac80211/util.c
@@ -2076,6 +2076,9 @@ int ieee80211_reconfig(struct ieee80211_local *local)
case NL80211_IFTYPE_AP:
changed |= BSS_CHANGED_SSID | BSS_CHANGED_P2P_PS;

+ if (sdata->vif.bss_conf.ftm_responder)
+ changed |= BSS_CHANGED_FTM_RESPONDER;
+
if (sdata->vif.type == NL80211_IFTYPE_AP) {
changed |= BSS_CHANGED_AP_PROBE_RESP;

--
1.9.1

[Johannes Berg]

On Tue, 2018-08-14 at 17:30 -0700, Pradeep Kumar Chitrapu wrote:
> Currently ftm_responder parameter in hostapd.conf is only used for fine
> timing measurement (FTM) capability advertisement and actual control of
> the functionality is with low-level device/driver. This leads to confusion
> to the user when the capability advertisement is different from actual FTM
> responder functionality.
>
> For example, FTM responder capability advertisement is set to 'disabled',
> which would imply that AP must not respond to FTM requests, but user sees
> AP still responding to FTM requests, as the functionality is enabled in
> the driver.

All you describe above is really a driver bug - it shouldn't have
enabled it to start with?

> The patch set allows userspace to configure FTM responder functionality
> with the addition of new Netlink attribute NL80211_ATTR_FTM_RESPONDER
> and also adds extended feature flag for the drivers to advertise the
> support. Sending '0' to disable FTM responder would imply AP does not
> respond to FTM requests and sending '1' to enable FTM responder would
> imply that AP responds to all FTM requests.

This makes sense anyway. Funny you should post this within hours of me
doing the same, basically.

I have no objection to your approach, though I guess it'd be nice if you
could take a look at the statistics I have exposed and see if those
makes sense or if additional ones are desirable for you, and then we can
combine the work that way, i.e. have your configuration and our stats?

johannes

[p***@codeaurora.org]

On 2018-08-15 02:04, Johannes Berg wrote:
> On Tue, 2018-08-14 at 17:30 -0700, Pradeep Kumar Chitrapu wrote:
>> Currently ftm_responder parameter in hostapd.conf is only used for
>> fine
>> timing measurement (FTM) capability advertisement and actual control
>> of
>> the functionality is with low-level device/driver. This leads to
>> confusion
>> to the user when the capability advertisement is different from actual
>> FTM
>> responder functionality.
>>
>> For example, FTM responder capability advertisement is set to
>> 'disabled',
>> which would imply that AP must not respond to FTM requests, but user
>> sees
>> AP still responding to FTM requests, as the functionality is enabled
>> in
>> the driver.
>
Hi Johannes,
Thanks for the review..

> All you describe above is really a driver bug - it shouldn't have
> enabled it to start with?

Sure.. But isn't it justifiable for drivers/firmware choosing to enable
ftm responder by default when there is no way for userspace to specify
this
parameter?

>
>> The patch set allows userspace to configure FTM responder
>> functionality
>> with the addition of new Netlink attribute NL80211_ATTR_FTM_RESPONDER
>> and also adds extended feature flag for the drivers to advertise the
>> support. Sending '0' to disable FTM responder would imply AP does not
>> respond to FTM requests and sending '1' to enable FTM responder would
>> imply that AP responds to all FTM requests.
>
> This makes sense anyway. Funny you should post this within hours of me
> doing the same, basically.
Sorry, I missed your patches before posting mine :)

>
> I have no objection to your approach, though I guess it'd be nice if
> you
> could take a look at the statistics I have exposed and see if those
> makes sense or if additional ones are desirable for you, and then we
> can
> combine the work that way, i.e. have your configuration and our stats?

I looked at the patch you posted and this makes sense. I will try to
align
ath10k driver changes with your approach.

Thanks
Pradeep
>
> johannes

[Johannes Berg]

On Wed, 2018-08-15 at 18:50 -0700, ***@codeaurora.org wrote:

> > All you describe above is really a driver bug - it shouldn't have
> > enabled it to start with?
>
> Sure.. But isn't it justifiable for drivers/firmware choosing to enable
> ftm responder by default when there is no way for userspace to specify
> this parameter?

No? FTM needs higher-level configuration/advertisement, so how could the
driver/firmware enable it when it's not supposed to be?

I really do think this is a driver bug.

Nothing stopped you from submitting these patches months/years ago when
the capability was first added to the firmware, after all.

> > This makes sense anyway. Funny you should post this within hours of me
> > doing the same, basically.
>
> Sorry, I missed your patches before posting mine :)

No worries.

> >
> > I have no objection to your approach, though I guess it'd be nice if
> > you
> > could take a look at the statistics I have exposed and see if those
> > makes sense or if additional ones are desirable for you, and then we
> > can combine the work that way, i.e. have your configuration and our stats?
>
> I looked at the patch you posted and this makes sense. I will try to
> align ath10k driver changes with your approach.

I tend to actually like your patch better for configuration - no new
command, though it is, I think, lacking configuration of the necessary
elements (where do you take the LCI/Civic location from?).

johannes

[Pradeep Kumar Chitrapu]

>
> No worries.
>
>> >
>> > I have no objection to your approach, though I guess it'd be nice if
>> > you
>> > could take a look at the statistics I have exposed and see if those
>> > makes sense or if additional ones are desirable for you, and then we
>> > can combine the work that way, i.e. have your configuration and our stats?
>>
>> I looked at the patch you posted and this makes sense. I will try to
>> align ath10k driver changes with your approach.
>
> I tend to actually like your patch better for configuration - no new
> command, though it is, I think, lacking configuration of the necessary
> elements (where do you take the LCI/Civic location from?).
>
> johannes

Hi Johannes,

I will change the attribute to FLAG type, also add support for LCI/CIVIC
params
and repost the patch.

Thanks
Pradeep

[Johannes Berg]

On Sat, 2018-08-18 at 00:50 -0700, Pradeep Kumar Chitrapu wrote:
> >
> I will change the attribute to FLAG type, also add support for LCI/CIVIC
> params and repost the patch.

Looking at Android (for unrelated reasons), I see that they have a
separate "enable FTM responder" command:

https://android.googlesource.com/platform/hardware/interfaces/+/master/wifi/1.0/IWifiRttController.hal#158

Is that something you've seen/worked with? If so, how does it match your
current approach where FTM responder has to be enabled from the start?

Or perhaps this API is independent of AP and starts its own AP? But it
doesn't have any setup for the SSID etc., only the channel, so not clear
how that'd work?

johannes

[Pradeep Kumar Chitrapu]

On 2018-08-20 02:33, Johannes Berg wrote:
> On Sat, 2018-08-18 at 00:50 -0700, Pradeep Kumar Chitrapu wrote:
>> >
>> I will change the attribute to FLAG type, also add support for
>> LCI/CIVIC
>> params and repost the patch.
>
> Looking at Android (for unrelated reasons), I see that they have a
> separate "enable FTM responder" command:
>
> https://android.googlesource.com/platform/hardware/interfaces/+/master/wifi/1.0/IWifiRttController.hal#158
>
> Is that something you've seen/worked with? If so, how does it match
> your
> current approach where FTM responder has to be enabled from the start?
>
> Or perhaps this API is independent of AP and starts its own AP? But it
> doesn't have any setup for the SSID etc., only the channel, so not
> clear
> how that'd work?
>
> johannes

I wasn't aware of this android api. However, looking at the api, the
assumption is that bss is started by a different
api and the 'enableResponder' api is used for enabling rtt for a given
duration.

The reason we have added enabling ftm responder through start ap is that
this can reflect beacon IE change in the
configuration in the same place. In case of the separate command,
enabling responder will not update the beacon,
however, the application must issue the new command, whenever its
updating beacon template.
Please let me know your thoughts/suggestions.

Pradeep

[Johannes Berg]

On Tue, 2018-08-21 at 11:32 -0700, Pradeep Kumar Chitrapu wrote:
>
> I wasn't aware of this android api.

OK.

> However, looking at the api, the
> assumption is that bss is started by a different
> api and the 'enableResponder' api is used for enabling rtt for a given
> duration.

It looks like. Note that there's also *disable*, which we hadn't even
implemented before.

> The reason we have added enabling ftm responder through start ap is that
> this can reflect beacon IE change in the
> configuration in the same place.

Which makes sense, yeah.

> In case of the separate command,
> enabling responder will not update the beacon,
> however, the application must issue the new command, whenever its
> updating beacon template.

Right. However, I guess we could allow updating/changing this setting on
the fly through nl80211_set_beacon() which already allows changing other
non-beacon parameters (like the probe or assoc response templates), and
then we can use your approach. Basically changing "SET_BEACON" to be a
bit like "CHANGE_AP". In that case we definitely would need the
attribute to be 0/1 as you had it so that it not present can be used to
indicate "no change".

johannes

[Kalle Valo]

***@codeaurora.org writes:

> Thanks for the review..

Could you please fix your From field to include your full name:

From: ***@codeaurora.org

It should look like this:

From: Kalle Valo <***@codeaurora.org>

This makes it a lot easier to follow threads.

--
Kalle Valo

[Sebastian Gottschall]

Am 15.08.2018 um 02:30 schrieb Pradeep Kumar Chitrapu:
> Configure fine timing measurement (FTM) responder role from the
> ftm_responder bss param sent by mac80211. With FTM functionality
> offloaded to firmware, adding the interface allows userspace
> to enable or disable FTM responder functionality. ath10k disables
> it at the time of interface creation.
>
> Supported FW: 10.4
>
> Tested on QCA9984 with debug firmware: 10.4-3.2.1-00121

Hello Pradeep

Just for curiosity 10.4-3.2.1-00121 is not available as firmware version
for the community and does seem to be based on a old tree

how does it behave with more recent firmwares like 3.5 and 3.6 series?


Sebastian


> Signed-off-by: Pradeep Kumar Chitrapu <***@codeaurora.org>
> ---
> drivers/net/wireless/ath/ath10k/core.h | 1 +
> drivers/net/wireless/ath/ath10k/mac.c | 29 +++++++++++++++++++++++++++++
> drivers/net/wireless/ath/ath10k/wmi.c | 4 ++++
> drivers/net/wireless/ath/ath10k/wmi.h | 10 ++++++++++
> 4 files changed, 44 insertions(+)
>
> diff --git a/drivers/net/wireless/ath/ath10k/core.h b/drivers/net/wireless/ath/ath10k/core.h
> index 9feea02e7d37..db190230b292 100644
> --- a/drivers/net/wireless/ath/ath10k/core.h
> +++ b/drivers/net/wireless/ath/ath10k/core.h
> @@ -544,6 +544,7 @@ struct ath10k_vif {
> bool nohwcrypt;
> int num_legacy_stations;
> int txpower;
> + bool ftm_responder;
> struct wmi_wmm_params_all_arg wmm_params;
> struct work_struct ap_csa_work;
> struct delayed_work connection_loss_work;
> diff --git a/drivers/net/wireless/ath/ath10k/mac.c b/drivers/net/wireless/ath/ath10k/mac.c
> index 90f9372dec25..cdb73b8cc253 100644
> --- a/drivers/net/wireless/ath/ath10k/mac.c
> +++ b/drivers/net/wireless/ath/ath10k/mac.c
> @@ -5259,6 +5259,17 @@ static int ath10k_add_interface(struct ieee80211_hw *hw,
> goto err_peer_delete;
> }
>
> + if (test_bit(WMI_SERVICE_RTT, ar->wmi.svc_map)) {
> + vdev_param = ar->wmi.vdev_param->rtt_responder_role;
> + ret = ath10k_wmi_vdev_set_param(ar, arvif->vdev_id, vdev_param,
> + arvif->ftm_responder);
> +
> + /* It is harmless to not set FTM role. Do not warn */
> + if (ret && ret != -EOPNOTSUPP)
> + ath10k_warn(ar, "failed to set vdev %i FTM Responder: %d\n",
> + arvif->vdev_id, ret);
> + }
> +
> if (vif->type == NL80211_IFTYPE_MONITOR) {
> ar->monitor_arvif = arvif;
> ret = ath10k_monitor_recalc(ar);
> @@ -5532,6 +5543,20 @@ static void ath10k_bss_info_changed(struct ieee80211_hw *hw,
> if (changed & BSS_CHANGED_BSSID && !is_zero_ether_addr(info->bssid))
> ether_addr_copy(arvif->bssid, info->bssid);
>
> + if (changed & BSS_CHANGED_FTM_RESPONDER &&
> + arvif->ftm_responder != info->ftm_responder &&
> + test_bit(WMI_SERVICE_RTT, ar->wmi.svc_map)) {
> + arvif->ftm_responder = info->ftm_responder;
> +
> + vdev_param = ar->wmi.vdev_param->rtt_responder_role;
> + ret = ath10k_wmi_vdev_set_param(ar, arvif->vdev_id, vdev_param,
> + arvif->ftm_responder);
> +
> + ath10k_dbg(ar, ATH10K_DBG_MAC,
> + "mac vdev %d ftm_responder %d:ret %d\n",
> + arvif->vdev_id, arvif->ftm_responder, ret);
> + }
> +
> if (changed & BSS_CHANGED_BEACON_ENABLED)
> ath10k_control_beaconing(arvif, info);
>
> @@ -8463,6 +8488,10 @@ int ath10k_mac_register(struct ath10k *ar)
> wiphy_ext_feature_set(ar->hw->wiphy,
> NL80211_EXT_FEATURE_SET_SCAN_DWELL);
>
> + if (test_bit(WMI_SERVICE_RTT, ar->wmi.svc_map))
> + wiphy_ext_feature_set(ar->hw->wiphy,
> + NL80211_EXT_FEATURE_SET_FTM_RESPONDER);
> +
> /*
> * on LL hardware queues are managed entirely by the FW
> * so we only advertise to mac we can do the queues thing
> diff --git a/drivers/net/wireless/ath/ath10k/wmi.c b/drivers/net/wireless/ath/ath10k/wmi.c
> index fd612d2905b0..00c67d53eab8 100644
> --- a/drivers/net/wireless/ath/ath10k/wmi.c
> +++ b/drivers/net/wireless/ath/ath10k/wmi.c
> @@ -825,6 +825,7 @@
> .meru_vc = WMI_VDEV_PARAM_UNSUPPORTED,
> .rx_decap_type = WMI_VDEV_PARAM_UNSUPPORTED,
> .bw_nss_ratemask = WMI_VDEV_PARAM_UNSUPPORTED,
> + .rtt_responder_role = WMI_VDEV_PARAM_UNSUPPORTED,
> };
>
> /* 10.X WMI VDEV param map */
> @@ -900,6 +901,7 @@
> .meru_vc = WMI_VDEV_PARAM_UNSUPPORTED,
> .rx_decap_type = WMI_VDEV_PARAM_UNSUPPORTED,
> .bw_nss_ratemask = WMI_VDEV_PARAM_UNSUPPORTED,
> + .rtt_responder_role = WMI_VDEV_PARAM_UNSUPPORTED,
> };
>
> static struct wmi_vdev_param_map wmi_10_2_4_vdev_param_map = {
> @@ -974,6 +976,7 @@
> .meru_vc = WMI_VDEV_PARAM_UNSUPPORTED,
> .rx_decap_type = WMI_VDEV_PARAM_UNSUPPORTED,
> .bw_nss_ratemask = WMI_VDEV_PARAM_UNSUPPORTED,
> + .rtt_responder_role = WMI_VDEV_PARAM_UNSUPPORTED,
> };
>
> static struct wmi_vdev_param_map wmi_10_4_vdev_param_map = {
> @@ -1051,6 +1054,7 @@
> .bw_nss_ratemask = WMI_10_4_VDEV_PARAM_BW_NSS_RATEMASK,
> .inc_tsf = WMI_10_4_VDEV_PARAM_TSF_INCREMENT,
> .dec_tsf = WMI_10_4_VDEV_PARAM_TSF_DECREMENT,
> + .rtt_responder_role = WMI_10_4_VDEV_PARAM_ENABLE_DISABLE_RTT_RESPONDER_ROLE,
> };
>
> static struct wmi_pdev_param_map wmi_pdev_param_map = {
> diff --git a/drivers/net/wireless/ath/ath10k/wmi.h b/drivers/net/wireless/ath/ath10k/wmi.h
> index 36220258e3c7..fa94873fe46f 100644
> --- a/drivers/net/wireless/ath/ath10k/wmi.h
> +++ b/drivers/net/wireless/ath/ath10k/wmi.h
> @@ -5035,6 +5035,7 @@ struct wmi_vdev_param_map {
> u32 bw_nss_ratemask;
> u32 inc_tsf;
> u32 dec_tsf;
> + u32 rtt_responder_role;
> };
>
> #define WMI_VDEV_PARAM_UNSUPPORTED 0
> @@ -5374,6 +5375,15 @@ enum wmi_10_4_vdev_param {
> WMI_10_4_VDEV_PARAM_ATF_SSID_SCHED_POLICY,
> WMI_10_4_VDEV_PARAM_DISABLE_DYN_BW_RTS,
> WMI_10_4_VDEV_PARAM_TSF_DECREMENT,
> + WMI_10_4_VDEV_PARAM_SELFGEN_FIXED_RATE,
> + WMI_10_4_VDEV_PARAM_AMPDU_SUBFRAME_SIZE_PER_AC,
> + WMI_10_4_VDEV_PARAM_NSS_VHT160,
> + WMI_10_4_VDEV_PARAM_NSS_VHT80_80,
> + WMI_10_4_VDEV_PARAM_AMSDU_SUBFRAME_SIZE_PER_AC,
> + WMI_10_4_VDEV_PARAM_DISABLE_CABQ,
> + WMI_10_4_VDEV_PARAM_SIFS_TRIGGER_RATE,
> + WMI_10_4_VDEV_PARAM_TX_POWER,
> + WMI_10_4_VDEV_PARAM_ENABLE_DISABLE_RTT_RESPONDER_ROLE,
> };
>
> #define WMI_VDEV_PARAM_TXBF_SU_TX_BFEE BIT(0)

[Sebastian Gottschall]

Am 15.08.2018 um 02:30 schrieb Pradeep Kumar Chitrapu:
>
> + if (test_bit(WMI_SERVICE_RTT, ar->wmi.svc_map))
> + wiphy_ext_feature_set(ar->hw->wiphy,
> + NL80211_EXT_FEATURE_SET_FTM_RESPONDER);
> +

not sure if this is accurate. even if WMI_SERVICE_RTT is set, the
firmware may not support this feature. this feature doesnt seem to be
implemented in current firmware releases
i checked the firmware sourcecode i have from 3.4 series and it doesnt
support this feature


Sebastian

[Kalle Valo]

Sebastian Gottschall <***@dd-wrt.com> writes:

> Am 15.08.2018 um 02:30 schrieb Pradeep Kumar Chitrapu:
>> + if (test_bit(WMI_SERVICE_RTT, ar->wmi.svc_map))
>> + wiphy_ext_feature_set(ar->hw->wiphy,
>> + NL80211_EXT_FEATURE_SET_FTM_RESPONDER);
>> +
>
> not sure if this is accurate. even if WMI_SERVICE_RTT is set, the
> firmware may not support this feature. this feature doesnt seem to be
> implemented in current firmware releases
> i checked the firmware sourcecode i have from 3.4 series and it doesnt
> support this feature

Pradeep, did you check this? It's really bad if ath10k enables the
feature even if the firmware doesn't support this.

--
Kalle Valo

[Kalle Valo]

Kalle Valo <***@codeaurora.org> writes:

> Sebastian Gottschall <***@dd-wrt.com> writes:
>
>> Am 15.08.2018 um 02:30 schrieb Pradeep Kumar Chitrapu:
>>> + if (test_bit(WMI_SERVICE_RTT, ar->wmi.svc_map))
>>> + wiphy_ext_feature_set(ar->hw->wiphy,
>>> + NL80211_EXT_FEATURE_SET_FTM_RESPONDER);
>>> +
>>
>> not sure if this is accurate. even if WMI_SERVICE_RTT is set, the
>> firmware may not support this feature. this feature doesnt seem to be
>> implemented in current firmware releases
>> i checked the firmware sourcecode i have from 3.4 series and it doesnt
>> support this feature
>
> Pradeep, did you check this? It's really bad if ath10k enables the
> feature even if the firmware doesn't support this.

Pradeep, please clarify the situation. I don't want to enable features
when firmware doesn't support it.

--
Kalle Valo

[Pradeep Kumar Chitrapu]

On 2018-08-21 12:24, Johannes Berg wrote:
> On Tue, 2018-08-21 at 11:32 -0700, Pradeep Kumar Chitrapu wrote:
>>
>> I wasn't aware of this android api.
>
> OK.
>
>> However, looking at the api, the
>> assumption is that bss is started by a different
>> api and the 'enableResponder' api is used for enabling rtt for a given
>> duration.
>
> It looks like. Note that there's also *disable*, which we hadn't even
> implemented before.
>
>> The reason we have added enabling ftm responder through start ap is
>> that
>> this can reflect beacon IE change in the
>> configuration in the same place.
>
> Which makes sense, yeah.
>
>> In case of the separate command,
>> enabling responder will not update the beacon,
>> however, the application must issue the new command, whenever its
>> updating beacon template.
>
> Right. However, I guess we could allow updating/changing this setting
> on
> the fly through nl80211_set_beacon() which already allows changing
> other
> non-beacon parameters (like the probe or assoc response templates), and
> then we can use your approach. Basically changing "SET_BEACON" to be a
> bit like "CHANGE_AP".
Agree. ( ftm_responder param will have to be added in
cfg80211_beacon_data
instead of cfg80211_ap_settings)

> In that case we definitely would need the
> attribute to be 0/1 as you had it so that it not present can be used to
> indicate "no change".
OK.
>
> johannes

[Johannes Berg]

On Tue, 2018-08-21 at 15:08 -0700, Pradeep Kumar Chitrapu wrote:

> > Right. However, I guess we could allow updating/changing this setting
> > on
> > the fly through nl80211_set_beacon() which already allows changing
> > other
> > non-beacon parameters (like the probe or assoc response templates), and
> > then we can use your approach. Basically changing "SET_BEACON" to be a
> > bit like "CHANGE_AP".
>
> Agree. ( ftm_responder param will have to be added in
> cfg80211_beacon_data
> instead of cfg80211_ap_settings)

Right. I'm not even sure that all devices can disable it though, so we
may be in a bind here? I think ours (Intel) can't, for example, disable
FTM responder without disabling the AP (at least momentarily)...

Should we just ignore that? Or perhaps add a separate capability for it?

Maybe there's even hardware that cannot change the elements (LCI/Civic
location) on the fly?

johannes

[Pradeep Kumar Chitrapu]

On 2018-08-22 00:27, Johannes Berg wrote:
> On Tue, 2018-08-21 at 15:08 -0700, Pradeep Kumar Chitrapu wrote:
>
>> > Right. However, I guess we could allow updating/changing this setting
>> > on
>> > the fly through () which already allows changing
>> > other
>> > non-beacon parameters (like the probe or assoc response templates), and
>> > then we can use your approach. Basically changing "SET_BEACON" to be a
>> > bit like "CHANGE_AP".
>>
>> Agree. ( ftm_responder param will have to be added in
>> cfg80211_beacon_data
>> instead of cfg80211_ap_settings)
>
> Right. I'm not even sure that all devices can disable it though, so we
> may be in a bind here? I think ours (Intel) can't, for example, disable
> FTM responder without disabling the AP (at least momentarily)...
>
> Should we just ignore that? Or perhaps add a separate capability for
> it?

Yes, even we behave the same. In that case, we can go ahead now with the
capability to only enable FTM responder. If the driver wants to support
disabling option in future, at which point, the new capability can be
added.

However, changes to nl80211_set_beacon to support enabling FTM responder
may still be needed along with start_ap, assuming that ap start will be
issued
by different android api than enableResponder api.

Please let me know your suggestion.

>
> Maybe there's even hardware that cannot change the elements (LCI/Civic
> location) on the fly?
>
> johannes

[Johannes Berg]

On Wed, 2018-08-22 at 11:22 -0700, Pradeep Kumar Chitrapu wrote:

> > Should we just ignore that? Or perhaps add a separate capability for
> > it?
>
> Yes, even we behave the same. In that case, we can go ahead now with the
> capability to only enable FTM responder. If the driver wants to support
> disabling option in future, at which point, the new capability can be
> added.

True.

> However, changes to nl80211_set_beacon to support enabling FTM responder
> may still be needed along with start_ap, assuming that ap start will be
> issued by different android api than enableResponder api.

Yes, I guess that's still needed to be able to support the Android API,
which seems useful.

johannes

[Pradeep Kumar Chitrapu]

This post might be inappropriate. Click to display it.

[Pradeep Kumar Chitrapu]

Configure fine timing measurement (FTM) responder role from the
ftm_responder bss param sent by mac80211. With FTM functionality
offloaded to firmware, adding the interface allows userspace
to enable FTM responder functionality. ath10k disables it at the
time of interface creation.

Supported FW: 10.4

Tested on QCA9984 with firmware: 10.4-3.6-00144

Signed-off-by: Pradeep Kumar Chitrapu <***@codeaurora.org>
---
drivers/net/wireless/ath/ath10k/core.h | 1 +
drivers/net/wireless/ath/ath10k/mac.c | 29 +++++++++++++++++++++++++++++
drivers/net/wireless/ath/ath10k/wmi.c | 4 ++++
drivers/net/wireless/ath/ath10k/wmi.h | 10 ++++++++++
4 files changed, 44 insertions(+)

diff --git a/drivers/net/wireless/ath/ath10k/core.h b/drivers/net/wireless/ath/ath10k/core.h
index 9feea02e7d37..db190230b292 100644
--- a/drivers/net/wireless/ath/ath10k/core.h
+++ b/drivers/net/wireless/ath/ath10k/core.h
@@ -544,6 +544,7 @@ struct ath10k_vif {
bool nohwcrypt;
int num_legacy_stations;
int txpower;
+ bool ftm_responder;
struct wmi_wmm_params_all_arg wmm_params;
struct work_struct ap_csa_work;
struct delayed_work connection_loss_work;
diff --git a/drivers/net/wireless/ath/ath10k/mac.c b/drivers/net/wireless/ath/ath10k/mac.c
index 90f9372dec25..24a747f8ec07 100644
--- a/drivers/net/wireless/ath/ath10k/mac.c
+++ b/drivers/net/wireless/ath/ath10k/mac.c
@@ -5259,6 +5259,17 @@ static int ath10k_add_interface(struct ieee80211_hw *hw,
goto err_peer_delete;
}

+ if (test_bit(WMI_SERVICE_RTT, ar->wmi.svc_map)) {
+ vdev_param = ar->wmi.vdev_param->rtt_responder_role;
+ ret = ath10k_wmi_vdev_set_param(ar, arvif->vdev_id, vdev_param,
+ arvif->ftm_responder);
+
+ /* It is harmless to not set FTM role. Do not warn */
+ if (ret && ret != -EOPNOTSUPP)
+ ath10k_warn(ar, "failed to set vdev %i FTM Responder: %d\n",
+ arvif->vdev_id, ret);
+ }
+
if (vif->type == NL80211_IFTYPE_MONITOR) {
ar->monitor_arvif = arvif;
ret = ath10k_monitor_recalc(ar);
@@ -5532,6 +5543,20 @@ static void ath10k_bss_info_changed(struct ieee80211_hw *hw,
if (changed & BSS_CHANGED_BSSID && !is_zero_ether_addr(info->bssid))
ether_addr_copy(arvif->bssid, info->bssid);

+ if (changed & BSS_CHANGED_FTM_RESPONDER &&
+ arvif->ftm_responder != info->ftm_responder &&
+ test_bit(WMI_SERVICE_RTT, ar->wmi.svc_map)) {
+ arvif->ftm_responder = info->ftm_responder;
+
+ vdev_param = ar->wmi.vdev_param->rtt_responder_role;
+ ret = ath10k_wmi_vdev_set_param(ar, arvif->vdev_id, vdev_param,
+ arvif->ftm_responder);
+
+ ath10k_dbg(ar, ATH10K_DBG_MAC,
+ "mac vdev %d ftm_responder %d:ret %d\n",
+ arvif->vdev_id, arvif->ftm_responder, ret);
+ }
+
if (changed & BSS_CHANGED_BEACON_ENABLED)
ath10k_control_beaconing(arvif, info);

@@ -8463,6 +8488,10 @@ int ath10k_mac_register(struct ath10k *ar)
wiphy_ext_feature_set(ar->hw->wiphy,
NL80211_EXT_FEATURE_SET_SCAN_DWELL);

+ if (test_bit(WMI_SERVICE_RTT, ar->wmi.svc_map))
+ wiphy_ext_feature_set(ar->hw->wiphy,
+ NL80211_EXT_FEATURE_ENABLE_FTM_RESPONDER);
+
/*
* on LL hardware queues are managed entirely by the FW
* so we only advertise to mac we can do the queues thing
diff --git a/drivers/net/wireless/ath/ath10k/wmi.c b/drivers/net/wireless/ath/ath10k/wmi.c
index fd612d2905b0..00c67d53eab8 100644
--- a/drivers/net/wireless/ath/ath10k/wmi.c
+++ b/drivers/net/wireless/ath/ath10k/wmi.c
@@ -825,6 +825,7 @@
.meru_vc = WMI_VDEV_PARAM_UNSUPPORTED,
.rx_decap_type = WMI_VDEV_PARAM_UNSUPPORTED,
.bw_nss_ratemask = WMI_VDEV_PARAM_UNSUPPORTED,
+ .rtt_responder_role = WMI_VDEV_PARAM_UNSUPPORTED,
};

/* 10.X WMI VDEV param map */
@@ -900,6 +901,7 @@
.meru_vc = WMI_VDEV_PARAM_UNSUPPORTED,
.rx_decap_type = WMI_VDEV_PARAM_UNSUPPORTED,
.bw_nss_ratemask = WMI_VDEV_PARAM_UNSUPPORTED,
+ .rtt_responder_role = WMI_VDEV_PARAM_UNSUPPORTED,
};

static struct wmi_vdev_param_map wmi_10_2_4_vdev_param_map = {
@@ -974,6 +976,7 @@
.meru_vc = WMI_VDEV_PARAM_UNSUPPORTED,
.rx_decap_type = WMI_VDEV_PARAM_UNSUPPORTED,
.bw_nss_ratemask = WMI_VDEV_PARAM_UNSUPPORTED,
+ .rtt_responder_role = WMI_VDEV_PARAM_UNSUPPORTED,
};

static struct wmi_vdev_param_map wmi_10_4_vdev_param_map = {
@@ -1051,6 +1054,7 @@
.bw_nss_ratemask = WMI_10_4_VDEV_PARAM_BW_NSS_RATEMASK,
.inc_tsf = WMI_10_4_VDEV_PARAM_TSF_INCREMENT,
.dec_tsf = WMI_10_4_VDEV_PARAM_TSF_DECREMENT,
+ .rtt_responder_role = WMI_10_4_VDEV_PARAM_ENABLE_DISABLE_RTT_RESPONDER_ROLE,
};

static struct wmi_pdev_param_map wmi_pdev_param_map = {
diff --git a/drivers/net/wireless/ath/ath10k/wmi.h b/drivers/net/wireless/ath/ath10k/wmi.h
index 36220258e3c7..fa94873fe46f 100644
--- a/drivers/net/wireless/ath/ath10k/wmi.h
+++ b/drivers/net/wireless/ath/ath10k/wmi.h
@@ -5035,6 +5035,7 @@ struct wmi_vdev_param_map {
u32 bw_nss_ratemask;
u32 inc_tsf;
u32 dec_tsf;
+ u32 rtt_responder_role;
};

#define WMI_VDEV_PARAM_UNSUPPORTED 0
@@ -5374,6 +5375,15 @@ enum wmi_10_4_vdev_param {
WMI_10_4_VDEV_PARAM_ATF_SSID_SCHED_POLICY,
WMI_10_4_VDEV_PARAM_DISABLE_DYN_BW_RTS,
WMI_10_4_VDEV_PARAM_TSF_DECREMENT,
+ WMI_10_4_VDEV_PARAM_SELFGEN_FIXED_RATE,
+ WMI_10_4_VDEV_PARAM_AMPDU_SUBFRAME_SIZE_PER_AC,
+ WMI_10_4_VDEV_PARAM_NSS_VHT160,
+ WMI_10_4_VDEV_PARAM_NSS_VHT80_80,
+ WMI_10_4_VDEV_PARAM_AMSDU_SUBFRAME_SIZE_PER_AC,
+ WMI_10_4_VDEV_PARAM_DISABLE_CABQ,
+ WMI_10_4_VDEV_PARAM_SIFS_TRIGGER_RATE,
+ WMI_10_4_VDEV_PARAM_TX_POWER,
+ WMI_10_4_VDEV_PARAM_ENABLE_DISABLE_RTT_RESPONDER_ROLE,
};

#define WMI_VDEV_PARAM_TXBF_SU_TX_BFEE BIT(0)
--
1.9.1