[PATCH v6 0/2] mac80211_hwsim: Add PMSR support

[PATCH v6 0/2] mac80211_hwsim: Add PMSR support

[Jaewan Kim]

Dear Kernel maintainers,

First of all, thank you for spending your precious time for reviewing
my changes, and also sorry for my mistakes in previous patchsets.

Let me propose series of CLs for adding PMSR support in the mac80211_hwsim.

PMSR (peer measurement) is generalized measurement between STAs,
and currently FTM (fine time measurement or flight time measurement)
is the one and only measurement.

FTM measures the RTT (round trip time) and FTM can be used to measure
distances between two STAs. RTT is often referred as 'measuring distance'
as well.

Kernel had already defined protocols for PMSR in the
include/uapi/linux/nl80211.h and relevant parsing/sending code are in the
net/wireless/pmsr.c, but they are only used in intel's iwlwifi driver.

CLs are tested with iw tool on Virtual Android device (a.k.a. Cuttlefish).
Hope this explains my CLs.

Many Thanks,

-- 
2.39.0.246.g2a6d74b583-goog

V5 -> V6: Added per CL change history.
V4 -> V5: Fixed style
V3 -> V4: Added detailed explanation to cover letter and per CL commit
          messages, includes explanation of PMSR and FTM.
          Also fixed memory leak.
V1 -> V3: Initial commits (include resends)

Jaewan Kim (2):
  mac80211_hwsim: add PMSR capability support
  mac80211_hwsim: handle FTM requests with virtio

 drivers/net/wireless/mac80211_hwsim.c | 826 +++++++++++++++++++++++-
 drivers/net/wireless/mac80211_hwsim.h |  56 +-
 include/net/cfg80211.h                |  20 +
 net/wireless/nl80211.c                |  28 +-
 4 files changed, 912 insertions(+), 18 deletions(-)

[PATCH v6 1/2] mac80211_hwsim: add PMSR capability support

[Jaewan Kim]

This CL allows mac80211_hwsim to be configured with PMSR capability.
The capability is mandatory because nl80211_pmsr_start() ignores
incoming PMSR request if PMSR capability isn't set in the wiphy.

This CL adds HWSIM_ATTR_PMSR_SUPPORT.
It can be used to set PMSR capability when creating a new radio.
To send extra details, HWSIM_ATTR_PMSR_SUPPORT can have nested
PMSR capability attributes defined in the nl80211.h.
Data format is the same as cfg80211_pmsr_capabilities.

If HWSIM_ATTR_PMSR_SUPPORT is specified, mac80211_hwsim builds
cfg80211_pmsr_capabilities and sets wiphy.pmsr_capa.

Signed-off-by: Jaewan Kim <jaewan@google.com>
---
V5 -> V6: Added per change patch history.
V4 -> V5: Fixed style for commit messages.
V3 -> V4: Added change details for new attribute, and fixed memory leak.
V1 -> V3: Initial commit (includes resends).
---
 drivers/net/wireless/mac80211_hwsim.c | 159 +++++++++++++++++++++++-
 drivers/net/wireless/mac80211_hwsim.h |   2 +
 include/net/cfg80211.h                |  10 ++
 net/wireless/nl80211.c                |  17 ++-
 4 files changed, 181 insertions(+), 7 deletions(-)

diff --git a/drivers/net/wireless/mac80211_hwsim.c b/drivers/net/wireless/mac80211_hwsim.c
index c57c8903b7c0..84c9db9178c3 100644
--- a/drivers/net/wireless/mac80211_hwsim.c
+++ b/drivers/net/wireless/mac80211_hwsim.c
@@ -719,6 +719,9 @@ struct mac80211_hwsim_data {
 	/* RSSI in rx status of the receiver */
 	int rx_rssi;
 
+	/* only used when pmsr capability is supplied */
+	struct cfg80211_pmsr_capabilities pmsr_capa;
+
 	struct mac80211_hwsim_link_data link_data[IEEE80211_MLD_MAX_NUM_LINKS];
 };
 
@@ -760,6 +763,37 @@ static const struct genl_multicast_group hwsim_mcgrps[] = {
 
 /* MAC80211_HWSIM netlink policy */
 
+static const struct nla_policy
+hwsim_ftm_capa_policy[NL80211_PMSR_FTM_CAPA_ATTR_MAX + 1] = {
+	[NL80211_PMSR_FTM_CAPA_ATTR_ASAP] = { .type = NLA_FLAG },
+	[NL80211_PMSR_FTM_CAPA_ATTR_NON_ASAP] = { .type = NLA_FLAG },
+	[NL80211_PMSR_FTM_CAPA_ATTR_REQ_LCI] = { .type = NLA_FLAG },
+	[NL80211_PMSR_FTM_CAPA_ATTR_REQ_CIVICLOC] = { .type = NLA_FLAG },
+	[NL80211_PMSR_FTM_CAPA_ATTR_PREAMBLES] = { .type = NLA_U32 },
+	[NL80211_PMSR_FTM_CAPA_ATTR_BANDWIDTHS] = { .type = NLA_U32 },
+	[NL80211_PMSR_FTM_CAPA_ATTR_MAX_BURSTS_EXPONENT] =
+		NLA_POLICY_MAX(NLA_U8, 15),
+	[NL80211_PMSR_FTM_CAPA_ATTR_MAX_FTMS_PER_BURST] =
+		NLA_POLICY_MAX(NLA_U8, 31),
+	[NL80211_PMSR_FTM_CAPA_ATTR_TRIGGER_BASED] = { .type = NLA_FLAG },
+	[NL80211_PMSR_FTM_CAPA_ATTR_NON_TRIGGER_BASED] = { .type = NLA_FLAG },
+};
+
+static const struct nla_policy
+hwsim_pmsr_type_policy[NL80211_PMSR_TYPE_MAX + 1] = {
+	[NL80211_PMSR_TYPE_FTM] = NLA_POLICY_NESTED(hwsim_ftm_capa_policy),
+};
+
+static const struct nla_policy
+hwsim_pmsr_capa_policy[NL80211_PMSR_ATTR_MAX + 1] = {
+	[NL80211_PMSR_ATTR_MAX_PEERS] = { .type = NLA_U32 },
+	[NL80211_PMSR_ATTR_REPORT_AP_TSF] = { .type = NLA_FLAG },
+	[NL80211_PMSR_ATTR_RANDOMIZE_MAC_ADDR] = { .type = NLA_FLAG },
+	[NL80211_PMSR_ATTR_TYPE_CAPA] =
+		NLA_POLICY_NESTED(hwsim_pmsr_type_policy),
+	[NL80211_PMSR_ATTR_PEERS] = { .type = NLA_REJECT }, // only for request.
+};
+
 static const struct nla_policy hwsim_genl_policy[HWSIM_ATTR_MAX + 1] = {
 	[HWSIM_ATTR_ADDR_RECEIVER] = NLA_POLICY_ETH_ADDR_COMPAT,
 	[HWSIM_ATTR_ADDR_TRANSMITTER] = NLA_POLICY_ETH_ADDR_COMPAT,
@@ -788,6 +822,7 @@ static const struct nla_policy hwsim_genl_policy[HWSIM_ATTR_MAX + 1] = {
 	[HWSIM_ATTR_IFTYPE_SUPPORT] = { .type = NLA_U32 },
 	[HWSIM_ATTR_CIPHER_SUPPORT] = { .type = NLA_BINARY },
 	[HWSIM_ATTR_MLO_SUPPORT] = { .type = NLA_FLAG },
+	[HWSIM_ATTR_PMSR_SUPPORT] = NLA_POLICY_NESTED(hwsim_pmsr_capa_policy),
 };
 
 #if IS_REACHABLE(CONFIG_VIRTIO)
@@ -3107,6 +3142,18 @@ static int mac80211_hwsim_change_sta_links(struct ieee80211_hw *hw,
 	return 0;
 }
 
+static int mac80211_hwsim_start_pmsr(struct ieee80211_hw *hw, struct ieee80211_vif *vif,
+				     struct cfg80211_pmsr_request *request)
+{
+	return -EOPNOTSUPP;
+}
+
+static void mac80211_hwsim_abort_pmsr(struct ieee80211_hw *hw, struct ieee80211_vif *vif,
+				      struct cfg80211_pmsr_request *request)
+{
+	// Do nothing for now.
+}
+
 #define HWSIM_COMMON_OPS					\
 	.tx = mac80211_hwsim_tx,				\
 	.wake_tx_queue = ieee80211_handle_wake_tx_queue,	\
@@ -3129,7 +3176,9 @@ static int mac80211_hwsim_change_sta_links(struct ieee80211_hw *hw,
 	.flush = mac80211_hwsim_flush,				\
 	.get_et_sset_count = mac80211_hwsim_get_et_sset_count,	\
 	.get_et_stats = mac80211_hwsim_get_et_stats,		\
-	.get_et_strings = mac80211_hwsim_get_et_strings,
+	.get_et_strings = mac80211_hwsim_get_et_strings,	\
+	.start_pmsr = mac80211_hwsim_start_pmsr,		\
+	.abort_pmsr = mac80211_hwsim_abort_pmsr,
 
 #define HWSIM_NON_MLO_OPS					\
 	.sta_add = mac80211_hwsim_sta_add,			\
@@ -3186,6 +3235,7 @@ struct hwsim_new_radio_params {
 	u32 *ciphers;
 	u8 n_ciphers;
 	bool mlo;
+	const struct cfg80211_pmsr_capabilities *pmsr_capa;
 };
 
 static void hwsim_mcast_config_msg(struct sk_buff *mcast_skb,
@@ -3260,7 +3310,10 @@ static int append_radio_msg(struct sk_buff *skb, int id,
 			return ret;
 	}
 
-	return 0;
+	if (param->pmsr_capa)
+		ret = cfg80211_send_pmsr_capa(param->pmsr_capa, skb);
+
+	return ret;
 }
 
 static void hwsim_mcast_new_radio(int id, struct genl_info *info,
@@ -4606,6 +4659,11 @@ static int mac80211_hwsim_new_radio(struct genl_info *info,
 				    data->debugfs,
 				    data, &hwsim_simulate_radar);
 
+	if (param->pmsr_capa) {
+		data->pmsr_capa = *param->pmsr_capa;
+		hw->wiphy->pmsr_capa = &data->pmsr_capa;
+	}
+
 	spin_lock_bh(&hwsim_radio_lock);
 	err = rhashtable_insert_fast(&hwsim_radios_rht, &data->rht,
 				     hwsim_rht_params);
@@ -4715,6 +4773,7 @@ static int mac80211_hwsim_get_radio(struct sk_buff *skb,
 	param.regd = data->regd;
 	param.channels = data->channels;
 	param.hwname = wiphy_name(data->hw->wiphy);
+	param.pmsr_capa = &data->pmsr_capa;
 
 	res = append_radio_msg(skb, data->idx, &param);
 	if (res < 0)
@@ -5053,6 +5112,83 @@ static bool hwsim_known_ciphers(const u32 *ciphers, int n_ciphers)
 	return true;
 }
 
+static int parse_ftm_capa(const struct nlattr *ftm_capa,
+			  struct cfg80211_pmsr_capabilities *out)
+{
+	struct nlattr *tb[NL80211_PMSR_FTM_CAPA_ATTR_MAX + 1];
+	int ret = nla_parse_nested(tb, NL80211_PMSR_FTM_CAPA_ATTR_MAX,
+				   ftm_capa, hwsim_ftm_capa_policy, NULL);
+	if (ret) {
+		pr_err("mac80211_hwsim: malformed FTM capability");
+		return -EINVAL;
+	}
+
+	out->ftm.supported = 1;
+	if (tb[NL80211_PMSR_FTM_CAPA_ATTR_PREAMBLES])
+		out->ftm.preambles =
+			nla_get_u32(tb[NL80211_PMSR_FTM_CAPA_ATTR_PREAMBLES]);
+	if (tb[NL80211_PMSR_FTM_CAPA_ATTR_BANDWIDTHS])
+		out->ftm.bandwidths =
+			nla_get_u32(tb[NL80211_PMSR_FTM_CAPA_ATTR_BANDWIDTHS]);
+	if (tb[NL80211_PMSR_FTM_CAPA_ATTR_MAX_BURSTS_EXPONENT])
+		out->ftm.max_bursts_exponent =
+			nla_get_u8(tb[NL80211_PMSR_FTM_CAPA_ATTR_MAX_BURSTS_EXPONENT]);
+	if (tb[NL80211_PMSR_FTM_CAPA_ATTR_MAX_FTMS_PER_BURST])
+		out->ftm.max_ftms_per_burst =
+			nla_get_u8(tb[NL80211_PMSR_FTM_CAPA_ATTR_MAX_FTMS_PER_BURST]);
+	out->ftm.asap =
+		!!tb[NL80211_PMSR_FTM_CAPA_ATTR_ASAP];
+	out->ftm.non_asap =
+		!!tb[NL80211_PMSR_FTM_CAPA_ATTR_NON_ASAP];
+	out->ftm.request_lci =
+		!!tb[NL80211_PMSR_FTM_CAPA_ATTR_REQ_LCI];
+	out->ftm.request_civicloc =
+		!!tb[NL80211_PMSR_FTM_CAPA_ATTR_REQ_CIVICLOC];
+	out->ftm.trigger_based =
+		!!tb[NL80211_PMSR_FTM_CAPA_ATTR_TRIGGER_BASED];
+	out->ftm.non_trigger_based =
+		!!tb[NL80211_PMSR_FTM_CAPA_ATTR_NON_TRIGGER_BASED];
+
+	return 0;
+}
+
+static int parse_pmsr_capa(const struct nlattr *pmsr_capa,
+			   struct cfg80211_pmsr_capabilities *out)
+{
+	struct nlattr *tb[NL80211_PMSR_ATTR_MAX + 1];
+	struct nlattr *nla;
+	int size;
+	int ret = nla_parse_nested(tb, NL80211_PMSR_ATTR_MAX, pmsr_capa,
+				   hwsim_pmsr_capa_policy, NULL);
+	if (ret) {
+		pr_err("mac80211_hwsim: malformed PMSR capability");
+		return -EINVAL;
+	}
+
+	if (tb[NL80211_PMSR_ATTR_MAX_PEERS])
+		out->max_peers =
+			nla_get_u32(tb[NL80211_PMSR_ATTR_MAX_PEERS]);
+	out->report_ap_tsf = !!tb[NL80211_PMSR_ATTR_REPORT_AP_TSF];
+	out->randomize_mac_addr =
+		!!tb[NL80211_PMSR_ATTR_RANDOMIZE_MAC_ADDR];
+
+	if (!tb[NL80211_PMSR_ATTR_TYPE_CAPA]) {
+		pr_err("mac80211_hwsim: malformed PMSR type");
+		return -EINVAL;
+	}
+
+	nla_for_each_nested(nla, tb[NL80211_PMSR_ATTR_TYPE_CAPA], size) {
+		switch (nla_type(nla)) {
+		case NL80211_PMSR_TYPE_FTM:
+			parse_ftm_capa(nla, out);
+			break;
+		default:
+			pr_warn("mac80211_hwsim: Unknown PMSR type\n");
+		}
+	}
+	return 0;
+}
+
 static int hwsim_new_radio_nl(struct sk_buff *msg, struct genl_info *info)
 {
 	struct hwsim_new_radio_params param = { 0 };
@@ -5173,8 +5309,26 @@ static int hwsim_new_radio_nl(struct sk_buff *msg, struct genl_info *info)
 		param.hwname = hwname;
 	}
 
+	if (info->attrs[HWSIM_ATTR_PMSR_SUPPORT]) {
+		struct cfg80211_pmsr_capabilities *pmsr_capa =
+			kmalloc(sizeof(struct cfg80211_pmsr_capabilities),
+				GFP_KERNEL);
+		if (!pmsr_capa) {
+			ret = -ENOMEM;
+			goto out_free;
+		}
+		ret = parse_pmsr_capa(info->attrs[HWSIM_ATTR_PMSR_SUPPORT],
+				      pmsr_capa);
+		if (ret)
+			goto out_free;
+		param.pmsr_capa = pmsr_capa;
+	}
+
 	ret = mac80211_hwsim_new_radio(info, &param);
+
+out_free:
 	kfree(hwname);
+	kfree(param.pmsr_capa);
 	return ret;
 }
 
@@ -5419,7 +5573,6 @@ static struct notifier_block hwsim_netlink_notifier = {
 static int __init hwsim_init_netlink(void)
 {
 	int rc;
-
 	printk(KERN_INFO "mac80211_hwsim: initializing netlink\n");
 
 	rc = genl_register_family(&hwsim_genl_family);
diff --git a/drivers/net/wireless/mac80211_hwsim.h b/drivers/net/wireless/mac80211_hwsim.h
index 527799b2de0f..81cd02d2555c 100644
--- a/drivers/net/wireless/mac80211_hwsim.h
+++ b/drivers/net/wireless/mac80211_hwsim.h
@@ -142,6 +142,7 @@ enum {
  * @HWSIM_ATTR_CIPHER_SUPPORT: u32 array of supported cipher types
  * @HWSIM_ATTR_MLO_SUPPORT: claim MLO support (exact parameters TBD) for
  *	the new radio
+ * @HWSIM_ATTR_PMSR_SUPPORT: claim peer measurement support
  * @__HWSIM_ATTR_MAX: enum limit
  */
 
@@ -173,6 +174,7 @@ enum {
 	HWSIM_ATTR_IFTYPE_SUPPORT,
 	HWSIM_ATTR_CIPHER_SUPPORT,
 	HWSIM_ATTR_MLO_SUPPORT,
+	HWSIM_ATTR_PMSR_SUPPORT,
 	__HWSIM_ATTR_MAX,
 };
 #define HWSIM_ATTR_MAX (__HWSIM_ATTR_MAX - 1)
diff --git a/include/net/cfg80211.h b/include/net/cfg80211.h
index 03d4f4deadae..33f775b0f0b0 100644
--- a/include/net/cfg80211.h
+++ b/include/net/cfg80211.h
@@ -8751,6 +8751,16 @@ void cfg80211_pmsr_complete(struct wireless_dev *wdev,
 			    struct cfg80211_pmsr_request *req,
 			    gfp_t gfp);
 
+/**
+ * cfg80211_send_pmsr_capa - send the pmsr capabilities.
+ * @cap: peer measurement capabilities
+ * @skb: The skb to send pmsr capa
+ *
+ * Send the peer measurement capabilities to skb.
+ */
+int cfg80211_send_pmsr_capa(const struct cfg80211_pmsr_capabilities *cap,
+			    struct sk_buff *msg);
+
 /**
  * cfg80211_iftype_allowed - check whether the interface can be allowed
  * @wiphy: the wiphy
diff --git a/net/wireless/nl80211.c b/net/wireless/nl80211.c
index 33a82ecab9d5..b972a2135654 100644
--- a/net/wireless/nl80211.c
+++ b/net/wireless/nl80211.c
@@ -2152,10 +2152,9 @@ nl80211_send_pmsr_ftm_capa(const struct cfg80211_pmsr_capabilities *cap,
 	return 0;
 }
 
-static int nl80211_send_pmsr_capa(struct cfg80211_registered_device *rdev,
-				  struct sk_buff *msg)
+int cfg80211_send_pmsr_capa(const struct cfg80211_pmsr_capabilities *cap,
+			    struct sk_buff *msg)
 {
-	const struct cfg80211_pmsr_capabilities *cap = rdev->wiphy.pmsr_capa;
 	struct nlattr *pmsr, *caps;
 
 	if (!cap)
@@ -2193,6 +2192,13 @@ static int nl80211_send_pmsr_capa(struct cfg80211_registered_device *rdev,
 
 	return 0;
 }
+EXPORT_SYMBOL_GPL(cfg80211_send_pmsr_capa);
+
+static int nl80211_send_pmsr_capa(struct cfg80211_registered_device *rdev,
+				  struct sk_buff *msg)
+{
+	return cfg80211_send_pmsr_capa(rdev->wiphy.pmsr_capa, msg);
+}
 
 static int
 nl80211_put_iftype_akm_suites(struct cfg80211_registered_device *rdev,
@@ -3181,8 +3187,11 @@ int nl80211_parse_chandef(struct cfg80211_registered_device *rdev,
 	struct nlattr **attrs = info->attrs;
 	u32 control_freq;
 
-	if (!attrs[NL80211_ATTR_WIPHY_FREQ])
+	if (!attrs[NL80211_ATTR_WIPHY_FREQ]) {
+		NL_SET_ERR_MSG_ATTR(extack, attrs[NL80211_ATTR_WIPHY_FREQ],
+				    "Frequency is missing");
 		return -EINVAL;
+	}
 
 	control_freq = MHZ_TO_KHZ(
 			nla_get_u32(info->attrs[NL80211_ATTR_WIPHY_FREQ]));
-- 
2.39.0.246.g2a6d74b583-goog

Re: [PATCH v6 1/2] mac80211_hwsim: add PMSR capability support

[Greg KH]

On Tue, Jan 24, 2023 at 02:54:29PM +0000, Jaewan Kim wrote:
> This CL allows mac80211_hwsim to be configured with PMSR capability.

What is a "CL"?

What is "PMSR"?

> The capability is mandatory because nl80211_pmsr_start() ignores
> incoming PMSR request if PMSR capability isn't set in the wiphy.

Mandatory for what?

> 
> This CL adds HWSIM_ATTR_PMSR_SUPPORT.

"CL"?

> It can be used to set PMSR capability when creating a new radio.
> To send extra details, HWSIM_ATTR_PMSR_SUPPORT can have nested
> PMSR capability attributes defined in the nl80211.h.
> Data format is the same as cfg80211_pmsr_capabilities.
> 
> If HWSIM_ATTR_PMSR_SUPPORT is specified, mac80211_hwsim builds
> cfg80211_pmsr_capabilities and sets wiphy.pmsr_capa.
> 
> Signed-off-by: Jaewan Kim <jaewan@google.com>
> ---
> V5 -> V6: Added per change patch history.
> V4 -> V5: Fixed style for commit messages.
> V3 -> V4: Added change details for new attribute, and fixed memory leak.
> V1 -> V3: Initial commit (includes resends).
> ---
>  drivers/net/wireless/mac80211_hwsim.c | 159 +++++++++++++++++++++++-
>  drivers/net/wireless/mac80211_hwsim.h |   2 +
>  include/net/cfg80211.h                |  10 ++
>  net/wireless/nl80211.c                |  17 ++-
>  4 files changed, 181 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/net/wireless/mac80211_hwsim.c b/drivers/net/wireless/mac80211_hwsim.c
> index c57c8903b7c0..84c9db9178c3 100644
> --- a/drivers/net/wireless/mac80211_hwsim.c
> +++ b/drivers/net/wireless/mac80211_hwsim.c
> @@ -719,6 +719,9 @@ struct mac80211_hwsim_data {
>  	/* RSSI in rx status of the receiver */
>  	int rx_rssi;
>  
> +	/* only used when pmsr capability is supplied */
> +	struct cfg80211_pmsr_capabilities pmsr_capa;
> +
>  	struct mac80211_hwsim_link_data link_data[IEEE80211_MLD_MAX_NUM_LINKS];
>  };
>  
> @@ -760,6 +763,37 @@ static const struct genl_multicast_group hwsim_mcgrps[] = {
>  
>  /* MAC80211_HWSIM netlink policy */
>  
> +static const struct nla_policy
> +hwsim_ftm_capa_policy[NL80211_PMSR_FTM_CAPA_ATTR_MAX + 1] = {
> +	[NL80211_PMSR_FTM_CAPA_ATTR_ASAP] = { .type = NLA_FLAG },
> +	[NL80211_PMSR_FTM_CAPA_ATTR_NON_ASAP] = { .type = NLA_FLAG },
> +	[NL80211_PMSR_FTM_CAPA_ATTR_REQ_LCI] = { .type = NLA_FLAG },
> +	[NL80211_PMSR_FTM_CAPA_ATTR_REQ_CIVICLOC] = { .type = NLA_FLAG },
> +	[NL80211_PMSR_FTM_CAPA_ATTR_PREAMBLES] = { .type = NLA_U32 },
> +	[NL80211_PMSR_FTM_CAPA_ATTR_BANDWIDTHS] = { .type = NLA_U32 },
> +	[NL80211_PMSR_FTM_CAPA_ATTR_MAX_BURSTS_EXPONENT] =
> +		NLA_POLICY_MAX(NLA_U8, 15),
> +	[NL80211_PMSR_FTM_CAPA_ATTR_MAX_FTMS_PER_BURST] =
> +		NLA_POLICY_MAX(NLA_U8, 31),
> +	[NL80211_PMSR_FTM_CAPA_ATTR_TRIGGER_BASED] = { .type = NLA_FLAG },
> +	[NL80211_PMSR_FTM_CAPA_ATTR_NON_TRIGGER_BASED] = { .type = NLA_FLAG },
> +};
> +
> +static const struct nla_policy
> +hwsim_pmsr_type_policy[NL80211_PMSR_TYPE_MAX + 1] = {
> +	[NL80211_PMSR_TYPE_FTM] = NLA_POLICY_NESTED(hwsim_ftm_capa_policy),
> +};
> +
> +static const struct nla_policy
> +hwsim_pmsr_capa_policy[NL80211_PMSR_ATTR_MAX + 1] = {
> +	[NL80211_PMSR_ATTR_MAX_PEERS] = { .type = NLA_U32 },
> +	[NL80211_PMSR_ATTR_REPORT_AP_TSF] = { .type = NLA_FLAG },
> +	[NL80211_PMSR_ATTR_RANDOMIZE_MAC_ADDR] = { .type = NLA_FLAG },
> +	[NL80211_PMSR_ATTR_TYPE_CAPA] =
> +		NLA_POLICY_NESTED(hwsim_pmsr_type_policy),
> +	[NL80211_PMSR_ATTR_PEERS] = { .type = NLA_REJECT }, // only for request.
> +};
> +
>  static const struct nla_policy hwsim_genl_policy[HWSIM_ATTR_MAX + 1] = {
>  	[HWSIM_ATTR_ADDR_RECEIVER] = NLA_POLICY_ETH_ADDR_COMPAT,
>  	[HWSIM_ATTR_ADDR_TRANSMITTER] = NLA_POLICY_ETH_ADDR_COMPAT,
> @@ -788,6 +822,7 @@ static const struct nla_policy hwsim_genl_policy[HWSIM_ATTR_MAX + 1] = {
>  	[HWSIM_ATTR_IFTYPE_SUPPORT] = { .type = NLA_U32 },
>  	[HWSIM_ATTR_CIPHER_SUPPORT] = { .type = NLA_BINARY },
>  	[HWSIM_ATTR_MLO_SUPPORT] = { .type = NLA_FLAG },
> +	[HWSIM_ATTR_PMSR_SUPPORT] = NLA_POLICY_NESTED(hwsim_pmsr_capa_policy),
>  };
>  
>  #if IS_REACHABLE(CONFIG_VIRTIO)
> @@ -3107,6 +3142,18 @@ static int mac80211_hwsim_change_sta_links(struct ieee80211_hw *hw,
>  	return 0;
>  }
>  
> +static int mac80211_hwsim_start_pmsr(struct ieee80211_hw *hw, struct ieee80211_vif *vif,
> +				     struct cfg80211_pmsr_request *request)
> +{
> +	return -EOPNOTSUPP;
> +}
> +
> +static void mac80211_hwsim_abort_pmsr(struct ieee80211_hw *hw, struct ieee80211_vif *vif,
> +				      struct cfg80211_pmsr_request *request)
> +{
> +	// Do nothing for now.
> +}
> +
>  #define HWSIM_COMMON_OPS					\
>  	.tx = mac80211_hwsim_tx,				\
>  	.wake_tx_queue = ieee80211_handle_wake_tx_queue,	\
> @@ -3129,7 +3176,9 @@ static int mac80211_hwsim_change_sta_links(struct ieee80211_hw *hw,
>  	.flush = mac80211_hwsim_flush,				\
>  	.get_et_sset_count = mac80211_hwsim_get_et_sset_count,	\
>  	.get_et_stats = mac80211_hwsim_get_et_stats,		\
> -	.get_et_strings = mac80211_hwsim_get_et_strings,
> +	.get_et_strings = mac80211_hwsim_get_et_strings,	\
> +	.start_pmsr = mac80211_hwsim_start_pmsr,		\
> +	.abort_pmsr = mac80211_hwsim_abort_pmsr,
>  
>  #define HWSIM_NON_MLO_OPS					\
>  	.sta_add = mac80211_hwsim_sta_add,			\
> @@ -3186,6 +3235,7 @@ struct hwsim_new_radio_params {
>  	u32 *ciphers;
>  	u8 n_ciphers;
>  	bool mlo;
> +	const struct cfg80211_pmsr_capabilities *pmsr_capa;
>  };
>  
>  static void hwsim_mcast_config_msg(struct sk_buff *mcast_skb,
> @@ -3260,7 +3310,10 @@ static int append_radio_msg(struct sk_buff *skb, int id,
>  			return ret;
>  	}
>  
> -	return 0;
> +	if (param->pmsr_capa)
> +		ret = cfg80211_send_pmsr_capa(param->pmsr_capa, skb);
> +
> +	return ret;
>  }
>  
>  static void hwsim_mcast_new_radio(int id, struct genl_info *info,
> @@ -4606,6 +4659,11 @@ static int mac80211_hwsim_new_radio(struct genl_info *info,
>  				    data->debugfs,
>  				    data, &hwsim_simulate_radar);
>  
> +	if (param->pmsr_capa) {
> +		data->pmsr_capa = *param->pmsr_capa;
> +		hw->wiphy->pmsr_capa = &data->pmsr_capa;
> +	}
> +
>  	spin_lock_bh(&hwsim_radio_lock);
>  	err = rhashtable_insert_fast(&hwsim_radios_rht, &data->rht,
>  				     hwsim_rht_params);
> @@ -4715,6 +4773,7 @@ static int mac80211_hwsim_get_radio(struct sk_buff *skb,
>  	param.regd = data->regd;
>  	param.channels = data->channels;
>  	param.hwname = wiphy_name(data->hw->wiphy);
> +	param.pmsr_capa = &data->pmsr_capa;
>  
>  	res = append_radio_msg(skb, data->idx, &param);
>  	if (res < 0)
> @@ -5053,6 +5112,83 @@ static bool hwsim_known_ciphers(const u32 *ciphers, int n_ciphers)
>  	return true;
>  }
>  
> +static int parse_ftm_capa(const struct nlattr *ftm_capa,
> +			  struct cfg80211_pmsr_capabilities *out)
> +{
> +	struct nlattr *tb[NL80211_PMSR_FTM_CAPA_ATTR_MAX + 1];
> +	int ret = nla_parse_nested(tb, NL80211_PMSR_FTM_CAPA_ATTR_MAX,
> +				   ftm_capa, hwsim_ftm_capa_policy, NULL);
> +	if (ret) {
> +		pr_err("mac80211_hwsim: malformed FTM capability");

dev_err()?

And can userspace trigger this?  If so, should it be rate limited?

And you need a blank line before this one, didn't checkpatch complain
about that?

> +		return -EINVAL;
> +	}
> +
> +	out->ftm.supported = 1;
> +	if (tb[NL80211_PMSR_FTM_CAPA_ATTR_PREAMBLES])
> +		out->ftm.preambles =
> +			nla_get_u32(tb[NL80211_PMSR_FTM_CAPA_ATTR_PREAMBLES]);
> +	if (tb[NL80211_PMSR_FTM_CAPA_ATTR_BANDWIDTHS])
> +		out->ftm.bandwidths =
> +			nla_get_u32(tb[NL80211_PMSR_FTM_CAPA_ATTR_BANDWIDTHS]);
> +	if (tb[NL80211_PMSR_FTM_CAPA_ATTR_MAX_BURSTS_EXPONENT])
> +		out->ftm.max_bursts_exponent =
> +			nla_get_u8(tb[NL80211_PMSR_FTM_CAPA_ATTR_MAX_BURSTS_EXPONENT]);
> +	if (tb[NL80211_PMSR_FTM_CAPA_ATTR_MAX_FTMS_PER_BURST])
> +		out->ftm.max_ftms_per_burst =
> +			nla_get_u8(tb[NL80211_PMSR_FTM_CAPA_ATTR_MAX_FTMS_PER_BURST]);
> +	out->ftm.asap =
> +		!!tb[NL80211_PMSR_FTM_CAPA_ATTR_ASAP];
> +	out->ftm.non_asap =
> +		!!tb[NL80211_PMSR_FTM_CAPA_ATTR_NON_ASAP];
> +	out->ftm.request_lci =
> +		!!tb[NL80211_PMSR_FTM_CAPA_ATTR_REQ_LCI];
> +	out->ftm.request_civicloc =
> +		!!tb[NL80211_PMSR_FTM_CAPA_ATTR_REQ_CIVICLOC];
> +	out->ftm.trigger_based =
> +		!!tb[NL80211_PMSR_FTM_CAPA_ATTR_TRIGGER_BASED];
> +	out->ftm.non_trigger_based =
> +		!!tb[NL80211_PMSR_FTM_CAPA_ATTR_NON_TRIGGER_BASED];
> +
> +	return 0;
> +}
> +
> +static int parse_pmsr_capa(const struct nlattr *pmsr_capa,
> +			   struct cfg80211_pmsr_capabilities *out)
> +{
> +	struct nlattr *tb[NL80211_PMSR_ATTR_MAX + 1];
> +	struct nlattr *nla;
> +	int size;
> +	int ret = nla_parse_nested(tb, NL80211_PMSR_ATTR_MAX, pmsr_capa,
> +				   hwsim_pmsr_capa_policy, NULL);
> +	if (ret) {
> +		pr_err("mac80211_hwsim: malformed PMSR capability");
> +		return -EINVAL;
> +	}
> +
> +	if (tb[NL80211_PMSR_ATTR_MAX_PEERS])
> +		out->max_peers =
> +			nla_get_u32(tb[NL80211_PMSR_ATTR_MAX_PEERS]);
> +	out->report_ap_tsf = !!tb[NL80211_PMSR_ATTR_REPORT_AP_TSF];
> +	out->randomize_mac_addr =
> +		!!tb[NL80211_PMSR_ATTR_RANDOMIZE_MAC_ADDR];
> +
> +	if (!tb[NL80211_PMSR_ATTR_TYPE_CAPA]) {
> +		pr_err("mac80211_hwsim: malformed PMSR type");
> +		return -EINVAL;
> +	}
> +
> +	nla_for_each_nested(nla, tb[NL80211_PMSR_ATTR_TYPE_CAPA], size) {
> +		switch (nla_type(nla)) {
> +		case NL80211_PMSR_TYPE_FTM:
> +			parse_ftm_capa(nla, out);
> +			break;
> +		default:
> +			pr_warn("mac80211_hwsim: Unknown PMSR type\n");
> +		}
> +	}
> +	return 0;
> +}
> +
>  static int hwsim_new_radio_nl(struct sk_buff *msg, struct genl_info *info)
>  {
>  	struct hwsim_new_radio_params param = { 0 };
> @@ -5173,8 +5309,26 @@ static int hwsim_new_radio_nl(struct sk_buff *msg, struct genl_info *info)
>  		param.hwname = hwname;
>  	}
>  
> +	if (info->attrs[HWSIM_ATTR_PMSR_SUPPORT]) {
> +		struct cfg80211_pmsr_capabilities *pmsr_capa =
> +			kmalloc(sizeof(struct cfg80211_pmsr_capabilities),
> +				GFP_KERNEL);
> +		if (!pmsr_capa) {
> +			ret = -ENOMEM;
> +			goto out_free;
> +		}
> +		ret = parse_pmsr_capa(info->attrs[HWSIM_ATTR_PMSR_SUPPORT],
> +				      pmsr_capa);
> +		if (ret)
> +			goto out_free;
> +		param.pmsr_capa = pmsr_capa;
> +	}
> +
>  	ret = mac80211_hwsim_new_radio(info, &param);
> +
> +out_free:
>  	kfree(hwname);
> +	kfree(param.pmsr_capa);
>  	return ret;
>  }
>  
> @@ -5419,7 +5573,6 @@ static struct notifier_block hwsim_netlink_notifier = {
>  static int __init hwsim_init_netlink(void)
>  {
>  	int rc;
> -
>  	printk(KERN_INFO "mac80211_hwsim: initializing netlink\n");
>  
>  	rc = genl_register_family(&hwsim_genl_family);

Why did you delete this line for no reason?

You might want to take some time and get an internal-Google review
before submitting this again to save the community some time and effort
in reviewing these.  I'm sure there are developers there you can find to
help you out.

thanks,

greg k-h

Re: [PATCH v6 1/2] mac80211_hwsim: add PMSR capability support

[Jaewan Kim]

On Wed, Jan 25, 2023 at 12:55 AM Greg KH <gregkh@linuxfoundation.org> wrote:
>
> On Tue, Jan 24, 2023 at 02:54:29PM +0000, Jaewan Kim wrote:
> > This CL allows mac80211_hwsim to be configured with PMSR capability.
>
> What is a "CL"?
>
> What is "PMSR"?
>
> > The capability is mandatory because nl80211_pmsr_start() ignores
> > incoming PMSR request if PMSR capability isn't set in the wiphy.
>
> Mandatory for what?
>
> >
> > This CL adds HWSIM_ATTR_PMSR_SUPPORT.
>
> "CL"?
>
> > It can be used to set PMSR capability when creating a new radio.
> > To send extra details, HWSIM_ATTR_PMSR_SUPPORT can have nested
> > PMSR capability attributes defined in the nl80211.h.
> > Data format is the same as cfg80211_pmsr_capabilities.
> >
> > If HWSIM_ATTR_PMSR_SUPPORT is specified, mac80211_hwsim builds
> > cfg80211_pmsr_capabilities and sets wiphy.pmsr_capa.
> >
> > Signed-off-by: Jaewan Kim <jaewan@google.com>
> > ---
> > V5 -> V6: Added per change patch history.
> > V4 -> V5: Fixed style for commit messages.
> > V3 -> V4: Added change details for new attribute, and fixed memory leak.
> > V1 -> V3: Initial commit (includes resends).
> > ---
> >  drivers/net/wireless/mac80211_hwsim.c | 159 +++++++++++++++++++++++-
> >  drivers/net/wireless/mac80211_hwsim.h |   2 +
> >  include/net/cfg80211.h                |  10 ++
> >  net/wireless/nl80211.c                |  17 ++-
> >  4 files changed, 181 insertions(+), 7 deletions(-)
> >
> > diff --git a/drivers/net/wireless/mac80211_hwsim.c b/drivers/net/wireless/mac80211_hwsim.c
> > index c57c8903b7c0..84c9db9178c3 100644
> > --- a/drivers/net/wireless/mac80211_hwsim.c
> > +++ b/drivers/net/wireless/mac80211_hwsim.c
> > @@ -719,6 +719,9 @@ struct mac80211_hwsim_data {
> >       /* RSSI in rx status of the receiver */
> >       int rx_rssi;
> >
> > +     /* only used when pmsr capability is supplied */
> > +     struct cfg80211_pmsr_capabilities pmsr_capa;
> > +
> >       struct mac80211_hwsim_link_data link_data[IEEE80211_MLD_MAX_NUM_LINKS];
> >  };
> >
> > @@ -760,6 +763,37 @@ static const struct genl_multicast_group hwsim_mcgrps[] = {
> >
> >  /* MAC80211_HWSIM netlink policy */
> >
> > +static const struct nla_policy
> > +hwsim_ftm_capa_policy[NL80211_PMSR_FTM_CAPA_ATTR_MAX + 1] = {
> > +     [NL80211_PMSR_FTM_CAPA_ATTR_ASAP] = { .type = NLA_FLAG },
> > +     [NL80211_PMSR_FTM_CAPA_ATTR_NON_ASAP] = { .type = NLA_FLAG },
> > +     [NL80211_PMSR_FTM_CAPA_ATTR_REQ_LCI] = { .type = NLA_FLAG },
> > +     [NL80211_PMSR_FTM_CAPA_ATTR_REQ_CIVICLOC] = { .type = NLA_FLAG },
> > +     [NL80211_PMSR_FTM_CAPA_ATTR_PREAMBLES] = { .type = NLA_U32 },
> > +     [NL80211_PMSR_FTM_CAPA_ATTR_BANDWIDTHS] = { .type = NLA_U32 },
> > +     [NL80211_PMSR_FTM_CAPA_ATTR_MAX_BURSTS_EXPONENT] =
> > +             NLA_POLICY_MAX(NLA_U8, 15),
> > +     [NL80211_PMSR_FTM_CAPA_ATTR_MAX_FTMS_PER_BURST] =
> > +             NLA_POLICY_MAX(NLA_U8, 31),
> > +     [NL80211_PMSR_FTM_CAPA_ATTR_TRIGGER_BASED] = { .type = NLA_FLAG },
> > +     [NL80211_PMSR_FTM_CAPA_ATTR_NON_TRIGGER_BASED] = { .type = NLA_FLAG },
> > +};
> > +
> > +static const struct nla_policy
> > +hwsim_pmsr_type_policy[NL80211_PMSR_TYPE_MAX + 1] = {
> > +     [NL80211_PMSR_TYPE_FTM] = NLA_POLICY_NESTED(hwsim_ftm_capa_policy),
> > +};
> > +
> > +static const struct nla_policy
> > +hwsim_pmsr_capa_policy[NL80211_PMSR_ATTR_MAX + 1] = {
> > +     [NL80211_PMSR_ATTR_MAX_PEERS] = { .type = NLA_U32 },
> > +     [NL80211_PMSR_ATTR_REPORT_AP_TSF] = { .type = NLA_FLAG },
> > +     [NL80211_PMSR_ATTR_RANDOMIZE_MAC_ADDR] = { .type = NLA_FLAG },
> > +     [NL80211_PMSR_ATTR_TYPE_CAPA] =
> > +             NLA_POLICY_NESTED(hwsim_pmsr_type_policy),
> > +     [NL80211_PMSR_ATTR_PEERS] = { .type = NLA_REJECT }, // only for request.
> > +};
> > +
> >  static const struct nla_policy hwsim_genl_policy[HWSIM_ATTR_MAX + 1] = {
> >       [HWSIM_ATTR_ADDR_RECEIVER] = NLA_POLICY_ETH_ADDR_COMPAT,
> >       [HWSIM_ATTR_ADDR_TRANSMITTER] = NLA_POLICY_ETH_ADDR_COMPAT,
> > @@ -788,6 +822,7 @@ static const struct nla_policy hwsim_genl_policy[HWSIM_ATTR_MAX + 1] = {
> >       [HWSIM_ATTR_IFTYPE_SUPPORT] = { .type = NLA_U32 },
> >       [HWSIM_ATTR_CIPHER_SUPPORT] = { .type = NLA_BINARY },
> >       [HWSIM_ATTR_MLO_SUPPORT] = { .type = NLA_FLAG },
> > +     [HWSIM_ATTR_PMSR_SUPPORT] = NLA_POLICY_NESTED(hwsim_pmsr_capa_policy),
> >  };
> >
> >  #if IS_REACHABLE(CONFIG_VIRTIO)
> > @@ -3107,6 +3142,18 @@ static int mac80211_hwsim_change_sta_links(struct ieee80211_hw *hw,
> >       return 0;
> >  }
> >
> > +static int mac80211_hwsim_start_pmsr(struct ieee80211_hw *hw, struct ieee80211_vif *vif,
> > +                                  struct cfg80211_pmsr_request *request)
> > +{
> > +     return -EOPNOTSUPP;
> > +}
> > +
> > +static void mac80211_hwsim_abort_pmsr(struct ieee80211_hw *hw, struct ieee80211_vif *vif,
> > +                                   struct cfg80211_pmsr_request *request)
> > +{
> > +     // Do nothing for now.
> > +}
> > +
> >  #define HWSIM_COMMON_OPS                                     \
> >       .tx = mac80211_hwsim_tx,                                \
> >       .wake_tx_queue = ieee80211_handle_wake_tx_queue,        \
> > @@ -3129,7 +3176,9 @@ static int mac80211_hwsim_change_sta_links(struct ieee80211_hw *hw,
> >       .flush = mac80211_hwsim_flush,                          \
> >       .get_et_sset_count = mac80211_hwsim_get_et_sset_count,  \
> >       .get_et_stats = mac80211_hwsim_get_et_stats,            \
> > -     .get_et_strings = mac80211_hwsim_get_et_strings,
> > +     .get_et_strings = mac80211_hwsim_get_et_strings,        \
> > +     .start_pmsr = mac80211_hwsim_start_pmsr,                \
> > +     .abort_pmsr = mac80211_hwsim_abort_pmsr,
> >
> >  #define HWSIM_NON_MLO_OPS                                    \
> >       .sta_add = mac80211_hwsim_sta_add,                      \
> > @@ -3186,6 +3235,7 @@ struct hwsim_new_radio_params {
> >       u32 *ciphers;
> >       u8 n_ciphers;
> >       bool mlo;
> > +     const struct cfg80211_pmsr_capabilities *pmsr_capa;
> >  };
> >
> >  static void hwsim_mcast_config_msg(struct sk_buff *mcast_skb,
> > @@ -3260,7 +3310,10 @@ static int append_radio_msg(struct sk_buff *skb, int id,
> >                       return ret;
> >       }
> >
> > -     return 0;
> > +     if (param->pmsr_capa)
> > +             ret = cfg80211_send_pmsr_capa(param->pmsr_capa, skb);
> > +
> > +     return ret;
> >  }
> >
> >  static void hwsim_mcast_new_radio(int id, struct genl_info *info,
> > @@ -4606,6 +4659,11 @@ static int mac80211_hwsim_new_radio(struct genl_info *info,
> >                                   data->debugfs,
> >                                   data, &hwsim_simulate_radar);
> >
> > +     if (param->pmsr_capa) {
> > +             data->pmsr_capa = *param->pmsr_capa;
> > +             hw->wiphy->pmsr_capa = &data->pmsr_capa;
> > +     }
> > +
> >       spin_lock_bh(&hwsim_radio_lock);
> >       err = rhashtable_insert_fast(&hwsim_radios_rht, &data->rht,
> >                                    hwsim_rht_params);
> > @@ -4715,6 +4773,7 @@ static int mac80211_hwsim_get_radio(struct sk_buff *skb,
> >       param.regd = data->regd;
> >       param.channels = data->channels;
> >       param.hwname = wiphy_name(data->hw->wiphy);
> > +     param.pmsr_capa = &data->pmsr_capa;
> >
> >       res = append_radio_msg(skb, data->idx, &param);
> >       if (res < 0)
> > @@ -5053,6 +5112,83 @@ static bool hwsim_known_ciphers(const u32 *ciphers, int n_ciphers)
> >       return true;
> >  }
> >
> > +static int parse_ftm_capa(const struct nlattr *ftm_capa,
> > +                       struct cfg80211_pmsr_capabilities *out)
> > +{
> > +     struct nlattr *tb[NL80211_PMSR_FTM_CAPA_ATTR_MAX + 1];
> > +     int ret = nla_parse_nested(tb, NL80211_PMSR_FTM_CAPA_ATTR_MAX,
> > +                                ftm_capa, hwsim_ftm_capa_policy, NULL);
> > +     if (ret) {
> > +             pr_err("mac80211_hwsim: malformed FTM capability");
>
> dev_err()?

Is dev_err() the printing error for device code?
If so, would it be better to propose another change for replacing all
pr_err() with dev_err() in this file?
I asked some people but I couldn't get the answer.

>
> And can userspace trigger this?  If so, should it be rate limited?
>
> And you need a blank line before this one, didn't checkpatch complain
> about that?
>
> > +             return -EINVAL;
> > +     }
> > +
> > +     out->ftm.supported = 1;
> > +     if (tb[NL80211_PMSR_FTM_CAPA_ATTR_PREAMBLES])
> > +             out->ftm.preambles =
> > +                     nla_get_u32(tb[NL80211_PMSR_FTM_CAPA_ATTR_PREAMBLES]);
> > +     if (tb[NL80211_PMSR_FTM_CAPA_ATTR_BANDWIDTHS])
> > +             out->ftm.bandwidths =
> > +                     nla_get_u32(tb[NL80211_PMSR_FTM_CAPA_ATTR_BANDWIDTHS]);
> > +     if (tb[NL80211_PMSR_FTM_CAPA_ATTR_MAX_BURSTS_EXPONENT])
> > +             out->ftm.max_bursts_exponent =
> > +                     nla_get_u8(tb[NL80211_PMSR_FTM_CAPA_ATTR_MAX_BURSTS_EXPONENT]);
> > +     if (tb[NL80211_PMSR_FTM_CAPA_ATTR_MAX_FTMS_PER_BURST])
> > +             out->ftm.max_ftms_per_burst =
> > +                     nla_get_u8(tb[NL80211_PMSR_FTM_CAPA_ATTR_MAX_FTMS_PER_BURST]);
> > +     out->ftm.asap =
> > +             !!tb[NL80211_PMSR_FTM_CAPA_ATTR_ASAP];
> > +     out->ftm.non_asap =
> > +             !!tb[NL80211_PMSR_FTM_CAPA_ATTR_NON_ASAP];
> > +     out->ftm.request_lci =
> > +             !!tb[NL80211_PMSR_FTM_CAPA_ATTR_REQ_LCI];
> > +     out->ftm.request_civicloc =
> > +             !!tb[NL80211_PMSR_FTM_CAPA_ATTR_REQ_CIVICLOC];
> > +     out->ftm.trigger_based =
> > +             !!tb[NL80211_PMSR_FTM_CAPA_ATTR_TRIGGER_BASED];
> > +     out->ftm.non_trigger_based =
> > +             !!tb[NL80211_PMSR_FTM_CAPA_ATTR_NON_TRIGGER_BASED];
> > +
> > +     return 0;
> > +}
> > +
> > +static int parse_pmsr_capa(const struct nlattr *pmsr_capa,
> > +                        struct cfg80211_pmsr_capabilities *out)
> > +{
> > +     struct nlattr *tb[NL80211_PMSR_ATTR_MAX + 1];
> > +     struct nlattr *nla;
> > +     int size;
> > +     int ret = nla_parse_nested(tb, NL80211_PMSR_ATTR_MAX, pmsr_capa,
> > +                                hwsim_pmsr_capa_policy, NULL);
> > +     if (ret) {
> > +             pr_err("mac80211_hwsim: malformed PMSR capability");
> > +             return -EINVAL;
> > +     }
> > +
> > +     if (tb[NL80211_PMSR_ATTR_MAX_PEERS])
> > +             out->max_peers =
> > +                     nla_get_u32(tb[NL80211_PMSR_ATTR_MAX_PEERS]);
> > +     out->report_ap_tsf = !!tb[NL80211_PMSR_ATTR_REPORT_AP_TSF];
> > +     out->randomize_mac_addr =
> > +             !!tb[NL80211_PMSR_ATTR_RANDOMIZE_MAC_ADDR];
> > +
> > +     if (!tb[NL80211_PMSR_ATTR_TYPE_CAPA]) {
> > +             pr_err("mac80211_hwsim: malformed PMSR type");
> > +             return -EINVAL;
> > +     }
> > +
> > +     nla_for_each_nested(nla, tb[NL80211_PMSR_ATTR_TYPE_CAPA], size) {
> > +             switch (nla_type(nla)) {
> > +             case NL80211_PMSR_TYPE_FTM:
> > +                     parse_ftm_capa(nla, out);
> > +                     break;
> > +             default:
> > +                     pr_warn("mac80211_hwsim: Unknown PMSR type\n");
> > +             }
> > +     }
> > +     return 0;
> > +}
> > +
> >  static int hwsim_new_radio_nl(struct sk_buff *msg, struct genl_info *info)
> >  {
> >       struct hwsim_new_radio_params param = { 0 };
> > @@ -5173,8 +5309,26 @@ static int hwsim_new_radio_nl(struct sk_buff *msg, struct genl_info *info)
> >               param.hwname = hwname;
> >       }
> >
> > +     if (info->attrs[HWSIM_ATTR_PMSR_SUPPORT]) {
> > +             struct cfg80211_pmsr_capabilities *pmsr_capa =
> > +                     kmalloc(sizeof(struct cfg80211_pmsr_capabilities),
> > +                             GFP_KERNEL);
> > +             if (!pmsr_capa) {
> > +                     ret = -ENOMEM;
> > +                     goto out_free;
> > +             }
> > +             ret = parse_pmsr_capa(info->attrs[HWSIM_ATTR_PMSR_SUPPORT],
> > +                                   pmsr_capa);
> > +             if (ret)
> > +                     goto out_free;
> > +             param.pmsr_capa = pmsr_capa;
> > +     }
> > +
> >       ret = mac80211_hwsim_new_radio(info, &param);
> > +
> > +out_free:
> >       kfree(hwname);
> > +     kfree(param.pmsr_capa);
> >       return ret;
> >  }
> >
> > @@ -5419,7 +5573,6 @@ static struct notifier_block hwsim_netlink_notifier = {
> >  static int __init hwsim_init_netlink(void)
> >  {
> >       int rc;
> > -
> >       printk(KERN_INFO "mac80211_hwsim: initializing netlink\n");
> >
> >       rc = genl_register_family(&hwsim_genl_family);
>
> Why did you delete this line for no reason?
>
> You might want to take some time and get an internal-Google review
> before submitting this again to save the community some time and effort
> in reviewing these.  I'm sure there are developers there you can find to
> help you out.
>
> thanks,
>
> greg k-h

Thank you for the review. I'll get another round of internal reviews.

Re: [PATCH v6 1/2] mac80211_hwsim: add PMSR capability support

[Greg KH]

On Mon, Jan 30, 2023 at 12:48:37AM +0900, Jaewan Kim wrote:
> On Wed, Jan 25, 2023 at 12:55 AM Greg KH <gregkh@linuxfoundation.org> wrote:
> > > +static int parse_ftm_capa(const struct nlattr *ftm_capa,
> > > +                       struct cfg80211_pmsr_capabilities *out)
> > > +{
> > > +     struct nlattr *tb[NL80211_PMSR_FTM_CAPA_ATTR_MAX + 1];
> > > +     int ret = nla_parse_nested(tb, NL80211_PMSR_FTM_CAPA_ATTR_MAX,
> > > +                                ftm_capa, hwsim_ftm_capa_policy, NULL);
> > > +     if (ret) {
> > > +             pr_err("mac80211_hwsim: malformed FTM capability");
> >
> > dev_err()?
> 
> Is dev_err() the printing error for device code?

I am sorry, but I can not understand this question, can you rephrase it?

> If so, would it be better to propose another change for replacing all
> pr_err() with dev_err() in this file?

Odds are, yes, but that should be independent of your change to add a
new feature.

thanks,

greg k-h

Re: [PATCH v6 1/2] mac80211_hwsim: add PMSR capability support

[Jaewan Kim]

On Mon, Jan 30, 2023 at 2:34 PM Greg KH <gregkh@linuxfoundation.org> wrote:
>
> On Mon, Jan 30, 2023 at 12:48:37AM +0900, Jaewan Kim wrote:
> > On Wed, Jan 25, 2023 at 12:55 AM Greg KH <gregkh@linuxfoundation.org> wrote:
> > > > +static int parse_ftm_capa(const struct nlattr *ftm_capa,
> > > > +                       struct cfg80211_pmsr_capabilities *out)
> > > > +{
> > > > +     struct nlattr *tb[NL80211_PMSR_FTM_CAPA_ATTR_MAX + 1];
> > > > +     int ret = nla_parse_nested(tb, NL80211_PMSR_FTM_CAPA_ATTR_MAX,
> > > > +                                ftm_capa, hwsim_ftm_capa_policy, NULL);
> > > > +     if (ret) {
> > > > +             pr_err("mac80211_hwsim: malformed FTM capability");
> > >
> > > dev_err()?
> >
> > Is dev_err() the printing error for device code?
>
> I am sorry, but I can not understand this question, can you rephrase it?

I just wanted to know better about `dev_err()`,
because all existing code in this file uses `pr_err()`,
and there's no good documentation for `dev_err()`.

Given your answer below, it seems like that `pr_err()` isn't a good
choice in this file.
Am I correct?

>
> > If so, would it be better to propose another change for replacing all
> > pr_err() with dev_err() in this file?
>
> Odds are, yes, but that should be independent of your change to add a
> new feature.

Got it. Then I'll break the consistency in this file for my change,
and also propose another change for using `dev_err()` instead of `pr_err()`.

>
> thanks,
>
> greg k-h



-- 
Jaewan Kim (김재완) | Software Engineer in Google Korea |
jaewan@google.com | +82-10-2781-5078

Re: [PATCH v6 1/2] mac80211_hwsim: add PMSR capability support

[Greg KH]

On Mon, Jan 30, 2023 at 05:08:54PM +0900, Jaewan Kim wrote:
> On Mon, Jan 30, 2023 at 2:34 PM Greg KH <gregkh@linuxfoundation.org> wrote:
> >
> > On Mon, Jan 30, 2023 at 12:48:37AM +0900, Jaewan Kim wrote:
> > > On Wed, Jan 25, 2023 at 12:55 AM Greg KH <gregkh@linuxfoundation.org> wrote:
> > > > > +static int parse_ftm_capa(const struct nlattr *ftm_capa,
> > > > > +                       struct cfg80211_pmsr_capabilities *out)
> > > > > +{
> > > > > +     struct nlattr *tb[NL80211_PMSR_FTM_CAPA_ATTR_MAX + 1];
> > > > > +     int ret = nla_parse_nested(tb, NL80211_PMSR_FTM_CAPA_ATTR_MAX,
> > > > > +                                ftm_capa, hwsim_ftm_capa_policy, NULL);
> > > > > +     if (ret) {
> > > > > +             pr_err("mac80211_hwsim: malformed FTM capability");
> > > >
> > > > dev_err()?
> > >
> > > Is dev_err() the printing error for device code?
> >
> > I am sorry, but I can not understand this question, can you rephrase it?
> 
> I just wanted to know better about `dev_err()`,
> because all existing code in this file uses `pr_err()`,
> and there's no good documentation for `dev_err()`.
> 
> Given your answer below, it seems like that `pr_err()` isn't a good
> choice in this file.
> Am I correct?

Drivers should never be using "raw" pr_*() calls as userspace has no way
of matching up a device and driver to a kernel log message.  That is
what the dev_*() calls provide.

As you are working with a device here (it's in your call-chain
somewhere), then you should use dev_*() calls.  Or use the
networking-specific versions of these as this is part of the network
stack.  But don't use raw pr_() calls please, that doesn't help anyone
out.

thanks,

greg k-h

[PATCH v6 2/2] mac80211_hwsim: handle FTM requests with virtio

[Jaewan Kim]

This CL allows mac80211_hwsim to receive FTM request and send FTM response.
It passthrough request to wmediumd and gets response with virtio
to get the FTM information with another STA.

This CL adds following commands
 - HWSIM_CMD_START_PMSR: To send request to wmediumd
 - HWSIM_CMD_ABORT_PMSR: To send abort to wmediumd
 - HWSIM_CMD_REPORT_PMSR: To receive response from wmediumd

Request and response are formatted the same way as pmsr.c.
One exception is for sending rate_info -- hwsim_rate_info_attributes is
added to send rate_info as is.

Signed-off-by: Jaewan Kim <jaewan@google.com>
---
V5 -> V6: Added per change patch history.
V4 -> V5: N/A.
V3 -> V4: Added more comments about new commands in mac80211_hwsim.
V1 -> V3: Initial commit (includes resends).
---
 drivers/net/wireless/mac80211_hwsim.c | 679 +++++++++++++++++++++++-
 drivers/net/wireless/mac80211_hwsim.h |  54 +-
 include/net/cfg80211.h                |  10 +
 net/wireless/nl80211.c                |  11 +-
 4 files changed, 737 insertions(+), 17 deletions(-)

diff --git a/drivers/net/wireless/mac80211_hwsim.c b/drivers/net/wireless/mac80211_hwsim.c
index 84c9db9178c3..4191037f73b6 100644
--- a/drivers/net/wireless/mac80211_hwsim.c
+++ b/drivers/net/wireless/mac80211_hwsim.c
@@ -721,6 +721,8 @@ struct mac80211_hwsim_data {
 
 	/* only used when pmsr capability is supplied */
 	struct cfg80211_pmsr_capabilities pmsr_capa;
+	struct cfg80211_pmsr_request *pmsr_request;
+	struct wireless_dev *pmsr_request_wdev;
 
 	struct mac80211_hwsim_link_data link_data[IEEE80211_MLD_MAX_NUM_LINKS];
 };
@@ -750,6 +752,13 @@ struct hwsim_radiotap_ack_hdr {
 	__le16 rt_chbitmask;
 } __packed;
 
+static struct mac80211_hwsim_data *get_hwsim_data_ref_from_addr(const u8 *addr)
+{
+	return rhashtable_lookup_fast(&hwsim_radios_rht,
+				      addr,
+				      hwsim_rht_params);
+}
+
 /* MAC80211_HWSIM netlink family */
 static struct genl_family hwsim_genl_family;
 
@@ -763,6 +772,81 @@ static const struct genl_multicast_group hwsim_mcgrps[] = {
 
 /* MAC80211_HWSIM netlink policy */
 
+static const struct nla_policy
+hwsim_rate_info_policy[HWSIM_RATE_INFO_ATTR_MAX + 1] = {
+	[HWSIM_RATE_INFO_ATTR_FLAGS] = { .type = NLA_U8 },
+	[HWSIM_RATE_INFO_ATTR_MCS] = { .type = NLA_U8 },
+	[HWSIM_RATE_INFO_ATTR_LEGACY] = { .type = NLA_U16 },
+	[HWSIM_RATE_INFO_ATTR_NSS] = { .type = NLA_U8 },
+	[HWSIM_RATE_INFO_ATTR_BW] = { .type = NLA_U8 },
+	[HWSIM_RATE_INFO_ATTR_HE_GI] = { .type = NLA_U8 },
+	[HWSIM_RATE_INFO_ATTR_HE_DCM] = { .type = NLA_U8 },
+	[HWSIM_RATE_INFO_ATTR_HE_RU_ALLOC] = { .type = NLA_U8 },
+	[HWSIM_RATE_INFO_ATTR_N_BOUNDED_CH] = { .type = NLA_U8 },
+	[HWSIM_RATE_INFO_ATTR_EHT_GI] = { .type = NLA_U8 },
+	[HWSIM_RATE_INFO_ATTR_EHT_RU_ALLOC] = { .type = NLA_U8 },
+};
+
+static const struct nla_policy
+hwsim_ftm_result_policy[NL80211_PMSR_FTM_RESP_ATTR_MAX + 1] = {
+	[NL80211_PMSR_FTM_RESP_ATTR_FAIL_REASON] = { .type = NLA_U32 },
+	[NL80211_PMSR_FTM_RESP_ATTR_BURST_INDEX] = { .type = NLA_U16 },
+	[NL80211_PMSR_FTM_RESP_ATTR_NUM_FTMR_ATTEMPTS] = { .type = NLA_U32 },
+	[NL80211_PMSR_FTM_RESP_ATTR_NUM_FTMR_SUCCESSES] = { .type = NLA_U32 },
+	[NL80211_PMSR_FTM_RESP_ATTR_BUSY_RETRY_TIME] = { .type = NLA_U8 },
+	[NL80211_PMSR_FTM_RESP_ATTR_NUM_BURSTS_EXP] = { .type = NLA_U8 },
+	[NL80211_PMSR_FTM_RESP_ATTR_BURST_DURATION] = { .type = NLA_U8 },
+	[NL80211_PMSR_FTM_RESP_ATTR_FTMS_PER_BURST] = { .type = NLA_U8 },
+	[NL80211_PMSR_FTM_RESP_ATTR_RSSI_AVG] = { .type = NLA_U32 },
+	[NL80211_PMSR_FTM_RESP_ATTR_RSSI_SPREAD] = { .type = NLA_U32 },
+	[NL80211_PMSR_FTM_RESP_ATTR_TX_RATE] =
+		NLA_POLICY_NESTED(hwsim_rate_info_policy),
+	[NL80211_PMSR_FTM_RESP_ATTR_RX_RATE] =
+		NLA_POLICY_NESTED(hwsim_rate_info_policy),
+	[NL80211_PMSR_FTM_RESP_ATTR_RTT_AVG] = { .type = NLA_U64 },
+	[NL80211_PMSR_FTM_RESP_ATTR_RTT_VARIANCE] = { .type = NLA_U64 },
+	[NL80211_PMSR_FTM_RESP_ATTR_RTT_SPREAD] = { .type = NLA_U64 },
+	[NL80211_PMSR_FTM_RESP_ATTR_DIST_AVG] = { .type = NLA_U64 },
+	[NL80211_PMSR_FTM_RESP_ATTR_DIST_VARIANCE] = { .type = NLA_U64 },
+	[NL80211_PMSR_FTM_RESP_ATTR_DIST_SPREAD] = { .type = NLA_U64 },
+	[NL80211_PMSR_FTM_RESP_ATTR_LCI] = { .type = NLA_STRING },
+	[NL80211_PMSR_FTM_RESP_ATTR_CIVICLOC] = { .type = NLA_STRING },
+};
+
+static const struct nla_policy
+hwsim_pmsr_resp_type_policy[NL80211_PMSR_TYPE_MAX + 1] = {
+	[NL80211_PMSR_TYPE_FTM] = NLA_POLICY_NESTED(hwsim_ftm_result_policy),
+};
+
+static const struct nla_policy
+hwsim_pmsr_resp_policy[NL80211_PMSR_RESP_ATTR_MAX + 1] = {
+	[NL80211_PMSR_RESP_ATTR_STATUS] = { .type = NLA_U32 },
+	[NL80211_PMSR_RESP_ATTR_HOST_TIME] = { .type = NLA_U64 },
+	[NL80211_PMSR_RESP_ATTR_AP_TSF] = { .type = NLA_U64 },
+	[NL80211_PMSR_RESP_ATTR_FINAL] = { .type = NLA_FLAG },
+	[NL80211_PMSR_RESP_ATTR_DATA] =
+		NLA_POLICY_NESTED(hwsim_pmsr_resp_type_policy),
+};
+
+static const struct nla_policy
+hwsim_pmsr_peer_result_policy[NL80211_PMSR_PEER_ATTR_MAX + 1] = {
+	[NL80211_PMSR_PEER_ATTR_ADDR] = NLA_POLICY_ETH_ADDR_COMPAT,
+	[NL80211_PMSR_PEER_ATTR_CHAN] = { .type = NLA_REJECT },
+	[NL80211_PMSR_PEER_ATTR_REQ] = { .type = NLA_REJECT },
+	[NL80211_PMSR_PEER_ATTR_RESP] =
+		NLA_POLICY_NESTED(hwsim_pmsr_resp_policy),
+};
+
+static const struct nla_policy
+hwsim_pmsr_peers_result_policy[NL80211_PMSR_ATTR_MAX + 1] = {
+	[NL80211_PMSR_ATTR_MAX_PEERS] = { .type = NLA_REJECT },
+	[NL80211_PMSR_ATTR_REPORT_AP_TSF] = { .type = NLA_REJECT },
+	[NL80211_PMSR_ATTR_RANDOMIZE_MAC_ADDR] = { .type = NLA_REJECT },
+	[NL80211_PMSR_ATTR_TYPE_CAPA] = { .type = NLA_REJECT },
+	[NL80211_PMSR_ATTR_PEERS] =
+		NLA_POLICY_NESTED_ARRAY(hwsim_pmsr_peer_result_policy),
+};
+
 static const struct nla_policy
 hwsim_ftm_capa_policy[NL80211_PMSR_FTM_CAPA_ATTR_MAX + 1] = {
 	[NL80211_PMSR_FTM_CAPA_ATTR_ASAP] = { .type = NLA_FLAG },
@@ -780,7 +864,7 @@ hwsim_ftm_capa_policy[NL80211_PMSR_FTM_CAPA_ATTR_MAX + 1] = {
 };
 
 static const struct nla_policy
-hwsim_pmsr_type_policy[NL80211_PMSR_TYPE_MAX + 1] = {
+hwsim_pmsr_capa_type_policy[NL80211_PMSR_TYPE_MAX + 1] = {
 	[NL80211_PMSR_TYPE_FTM] = NLA_POLICY_NESTED(hwsim_ftm_capa_policy),
 };
 
@@ -790,7 +874,7 @@ hwsim_pmsr_capa_policy[NL80211_PMSR_ATTR_MAX + 1] = {
 	[NL80211_PMSR_ATTR_REPORT_AP_TSF] = { .type = NLA_FLAG },
 	[NL80211_PMSR_ATTR_RANDOMIZE_MAC_ADDR] = { .type = NLA_FLAG },
 	[NL80211_PMSR_ATTR_TYPE_CAPA] =
-		NLA_POLICY_NESTED(hwsim_pmsr_type_policy),
+		NLA_POLICY_NESTED(hwsim_pmsr_capa_type_policy),
 	[NL80211_PMSR_ATTR_PEERS] = { .type = NLA_REJECT }, // only for request.
 };
 
@@ -823,6 +907,7 @@ static const struct nla_policy hwsim_genl_policy[HWSIM_ATTR_MAX + 1] = {
 	[HWSIM_ATTR_CIPHER_SUPPORT] = { .type = NLA_BINARY },
 	[HWSIM_ATTR_MLO_SUPPORT] = { .type = NLA_FLAG },
 	[HWSIM_ATTR_PMSR_SUPPORT] = NLA_POLICY_NESTED(hwsim_pmsr_capa_policy),
+	[HWSIM_ATTR_PMSR_RESULT] = NLA_POLICY_NESTED(hwsim_pmsr_peers_result_policy),
 };
 
 #if IS_REACHABLE(CONFIG_VIRTIO)
@@ -3142,16 +3227,578 @@ static int mac80211_hwsim_change_sta_links(struct ieee80211_hw *hw,
 	return 0;
 }
 
-static int mac80211_hwsim_start_pmsr(struct ieee80211_hw *hw, struct ieee80211_vif *vif,
+static int mac80211_hwsim_send_pmsr_ftm_request_peer(struct sk_buff *msg,
+						     struct cfg80211_pmsr_ftm_request_peer *request)
+{
+	void *ftm;
+
+	if (!request || !request->requested)
+		return -EINVAL;
+
+	ftm = nla_nest_start(msg, NL80211_PMSR_TYPE_FTM);
+	if (!ftm)
+		return -ENOBUFS;
+
+	if (nla_put_u32(msg, NL80211_PMSR_FTM_REQ_ATTR_PREAMBLE,
+			request->preamble))
+		return -ENOBUFS;
+
+	if (nla_put_u16(msg, NL80211_PMSR_FTM_REQ_ATTR_BURST_PERIOD,
+			request->burst_period))
+		return -ENOBUFS;
+
+	if (request->asap &&
+	    nla_put_flag(msg, NL80211_PMSR_FTM_REQ_ATTR_ASAP))
+		return -ENOBUFS;
+
+	if (request->request_lci &&
+	    nla_put_flag(msg, NL80211_PMSR_FTM_REQ_ATTR_REQUEST_LCI))
+		return -ENOBUFS;
+
+	if (request->request_civicloc &&
+	    nla_put_flag(msg, NL80211_PMSR_FTM_REQ_ATTR_REQUEST_CIVICLOC))
+		return -ENOBUFS;
+
+	if (request->trigger_based &&
+	    nla_put_flag(msg, NL80211_PMSR_FTM_REQ_ATTR_TRIGGER_BASED))
+		return -ENOBUFS;
+
+	if (request->non_trigger_based &&
+	    nla_put_flag(msg, NL80211_PMSR_FTM_REQ_ATTR_NON_TRIGGER_BASED))
+		return -ENOBUFS;
+
+	if (request->lmr_feedback &&
+	    nla_put_flag(msg, NL80211_PMSR_FTM_REQ_ATTR_LMR_FEEDBACK))
+		return -ENOBUFS;
+
+	if (nla_put_u8(msg, NL80211_PMSR_FTM_REQ_ATTR_NUM_BURSTS_EXP,
+		       request->num_bursts_exp))
+		return -ENOBUFS;
+
+	if (nla_put_u8(msg, NL80211_PMSR_FTM_REQ_ATTR_BURST_DURATION,
+		       request->burst_duration))
+		return -ENOBUFS;
+
+	if (nla_put_u8(msg, NL80211_PMSR_FTM_REQ_ATTR_FTMS_PER_BURST,
+		       request->ftms_per_burst))
+		return -ENOBUFS;
+
+	if (nla_put_u8(msg, NL80211_PMSR_FTM_REQ_ATTR_NUM_FTMR_RETRIES,
+		       request->ftmr_retries))
+		return -ENOBUFS;
+
+	if (nla_put_u8(msg, NL80211_PMSR_FTM_REQ_ATTR_BSS_COLOR,
+		       request->bss_color))
+		return -ENOBUFS;
+
+	nla_nest_end(msg, ftm);
+
+	return 0;
+}
+
+static int mac80211_hwsim_send_pmsr_request_peer(struct sk_buff *msg,
+						 struct cfg80211_pmsr_request_peer *request)
+{
+	void *peer, *chandef, *req, *data;
+	int err;
+
+	peer = nla_nest_start(msg, NL80211_PMSR_ATTR_PEERS);
+	if (!peer)
+		return -ENOBUFS;
+
+	if (nla_put(msg, NL80211_PMSR_PEER_ATTR_ADDR, ETH_ALEN,
+		    request->addr))
+		return -ENOBUFS;
+
+	chandef = nla_nest_start(msg, NL80211_PMSR_PEER_ATTR_CHAN);
+	if (!chandef)
+		return -ENOBUFS;
+
+	err = cfg80211_send_chandef(msg, &request->chandef);
+	if (err)
+		return err;
+
+	nla_nest_end(msg, chandef);
+
+	req = nla_nest_start(msg, NL80211_PMSR_PEER_ATTR_REQ);
+	if (request->report_ap_tsf &&
+	    nla_put_flag(msg, NL80211_PMSR_REQ_ATTR_GET_AP_TSF))
+		return -ENOBUFS;
+
+	data = nla_nest_start(msg, NL80211_PMSR_REQ_ATTR_DATA);
+	if (!data)
+		return -ENOBUFS;
+
+	mac80211_hwsim_send_pmsr_ftm_request_peer(msg, &request->ftm);
+	nla_nest_end(msg, data);
+	nla_nest_end(msg, req);
+	nla_nest_end(msg, peer);
+
+	return 0;
+}
+
+static int mac80211_hwsim_send_pmsr_request(struct sk_buff *msg,
+					    struct cfg80211_pmsr_request *request)
+{
+	int err;
+	void *pmsr;
+
+	pmsr = nla_nest_start(msg, NL80211_ATTR_PEER_MEASUREMENTS);
+	if (!pmsr)
+		return -ENOBUFS;
+
+	if (nla_put_u32(msg, NL80211_ATTR_TIMEOUT, request->timeout))
+		return -ENOBUFS;
+
+	if (!is_zero_ether_addr(request->mac_addr)) {
+		if (nla_put(msg, NL80211_ATTR_MAC, ETH_ALEN, request->mac_addr))
+			return -ENOBUFS;
+		if (nla_put(msg, NL80211_ATTR_MAC_MASK, ETH_ALEN,
+			    request->mac_addr_mask))
+			return -ENOBUFS;
+	}
+
+	for (int i = 0; i < request->n_peers; i++) {
+		err = mac80211_hwsim_send_pmsr_request_peer(msg,
+							    &request->peers[i]);
+		if (err)
+			return err;
+	}
+
+	nla_nest_end(msg, pmsr);
+
+	return 0;
+}
+
+static int mac80211_hwsim_start_pmsr(struct ieee80211_hw *hw,
+				     struct ieee80211_vif *vif,
 				     struct cfg80211_pmsr_request *request)
 {
-	return -EOPNOTSUPP;
+	struct mac80211_hwsim_data *data = hw->priv;
+	u32 _portid = READ_ONCE(data->wmediumd);
+	int err = 0;
+	struct sk_buff *skb = NULL;
+	void *msg_head;
+	void *pmsr;
+
+	if (!_portid && !hwsim_virtio_enabled)
+		return -EOPNOTSUPP;
+
+	mutex_lock(&data->mutex);
+
+	if (data->pmsr_request) {
+		err = -EBUSY;
+		goto out_err;
+	}
+
+	skb = genlmsg_new(GENLMSG_DEFAULT_SIZE, GFP_KERNEL);
+
+	if (!skb) {
+		err = -ENOMEM;
+		goto out_err;
+	}
+
+	msg_head = genlmsg_put(skb, 0, 0, &hwsim_genl_family, 0,
+			       HWSIM_CMD_START_PMSR);
+
+	if (nla_put(skb, HWSIM_ATTR_ADDR_TRANSMITTER,
+		    ETH_ALEN, data->addresses[1].addr)) {
+		err = -ENOMEM;
+		goto out_err;
+	}
+
+	pmsr = nla_nest_start(skb, HWSIM_ATTR_PMSR_REQUEST);
+	if (!pmsr) {
+		err = -ENOMEM;
+		goto out_err;
+	}
+
+	err = mac80211_hwsim_send_pmsr_request(skb, request);
+	if (err)
+		goto out_err;
+
+	nla_nest_end(skb, pmsr);
+
+	genlmsg_end(skb, msg_head);
+	if (hwsim_virtio_enabled)
+		hwsim_tx_virtio(data, skb);
+	else
+		hwsim_unicast_netgroup(data, skb, _portid);
+
+out_err:
+	if (err && skb)
+		nlmsg_free(skb);
+
+	if (!err) {
+		data->pmsr_request = request;
+		data->pmsr_request_wdev = ieee80211_vif_to_wdev(vif);
+	}
+
+	mutex_unlock(&data->mutex);
+	return err;
 }
 
-static void mac80211_hwsim_abort_pmsr(struct ieee80211_hw *hw, struct ieee80211_vif *vif,
+static void mac80211_hwsim_abort_pmsr(struct ieee80211_hw *hw,
+				      struct ieee80211_vif *vif,
 				      struct cfg80211_pmsr_request *request)
 {
-	// Do nothing for now.
+	struct mac80211_hwsim_data *data = hw->priv;
+	u32 _portid = READ_ONCE(data->wmediumd);
+	struct sk_buff *skb = NULL;
+	int err = 0;
+	void *msg_head;
+	void *pmsr;
+
+	if (!_portid && !hwsim_virtio_enabled)
+		return;
+
+	mutex_lock(&data->mutex);
+
+	if (data->pmsr_request != request) {
+		err = -EINVAL;
+		goto out_err;
+	}
+
+	if (err)
+		goto out_err;
+
+	skb = genlmsg_new(GENLMSG_DEFAULT_SIZE, GFP_KERNEL);
+	if (!skb) {
+		err = -EINVAL;
+		goto out_err;
+	}
+
+	msg_head = genlmsg_put(skb, 0, 0, &hwsim_genl_family, 0,
+			       HWSIM_CMD_ABORT_PMSR);
+
+	if (nla_put(skb, HWSIM_ATTR_ADDR_TRANSMITTER,
+		    ETH_ALEN, data->addresses[1].addr)) {
+		err = -EINVAL;
+		goto out_err;
+	}
+
+	pmsr = nla_nest_start(skb, HWSIM_ATTR_PMSR_REQUEST);
+	if (!pmsr) {
+		err = -ENOMEM;
+		goto out_err;
+	}
+
+	err = mac80211_hwsim_send_pmsr_request(skb, request);
+	if (err)
+		goto out_err;
+
+	err = nla_nest_end(skb, pmsr);
+	if (err)
+		goto out_err;
+
+	genlmsg_end(skb, msg_head);
+	if (hwsim_virtio_enabled)
+		hwsim_tx_virtio(data, skb);
+	else
+		hwsim_unicast_netgroup(data, skb, _portid);
+
+out_err:
+	if (err && skb)
+		nlmsg_free(skb);
+
+	mutex_unlock(&data->mutex);
+}
+
+static int mac80211_hwsim_parse_rate_info(struct nlattr *rateattr,
+					  struct rate_info *rate_info,
+					  struct genl_info *info)
+{
+	struct nlattr *tb[HWSIM_RATE_INFO_ATTR_MAX + 1];
+	int ret;
+
+	ret = nla_parse_nested(tb, HWSIM_RATE_INFO_ATTR_MAX,
+			       rateattr, hwsim_rate_info_policy, info->extack);
+	if (ret)
+		return ret;
+
+	if (tb[HWSIM_RATE_INFO_ATTR_FLAGS])
+		rate_info->flags = nla_get_u8(tb[HWSIM_RATE_INFO_ATTR_FLAGS]);
+
+	if (tb[HWSIM_RATE_INFO_ATTR_MCS])
+		rate_info->mcs = nla_get_u8(tb[HWSIM_RATE_INFO_ATTR_MCS]);
+
+	if (tb[HWSIM_RATE_INFO_ATTR_LEGACY])
+		rate_info->legacy = nla_get_u16(tb[HWSIM_RATE_INFO_ATTR_LEGACY]);
+
+	if (tb[HWSIM_RATE_INFO_ATTR_NSS])
+		rate_info->nss = nla_get_u8(tb[HWSIM_RATE_INFO_ATTR_NSS]);
+
+	if (tb[HWSIM_RATE_INFO_ATTR_BW])
+		rate_info->bw = nla_get_u8(tb[HWSIM_RATE_INFO_ATTR_BW]);
+
+	if (tb[HWSIM_RATE_INFO_ATTR_HE_GI])
+		rate_info->he_gi = nla_get_u8(tb[HWSIM_RATE_INFO_ATTR_HE_GI]);
+
+	if (tb[HWSIM_RATE_INFO_ATTR_HE_DCM])
+		rate_info->he_dcm = nla_get_u8(tb[HWSIM_RATE_INFO_ATTR_HE_DCM]);
+
+	if (tb[HWSIM_RATE_INFO_ATTR_HE_RU_ALLOC])
+		rate_info->he_ru_alloc =
+			nla_get_u8(tb[HWSIM_RATE_INFO_ATTR_HE_RU_ALLOC]);
+
+	if (tb[HWSIM_RATE_INFO_ATTR_N_BOUNDED_CH])
+		rate_info->n_bonded_ch =
+			nla_get_u8(tb[HWSIM_RATE_INFO_ATTR_N_BOUNDED_CH]);
+
+	if (tb[HWSIM_RATE_INFO_ATTR_EHT_GI])
+		rate_info->eht_gi = nla_get_u8(tb[HWSIM_RATE_INFO_ATTR_EHT_GI]);
+
+	if (tb[HWSIM_RATE_INFO_ATTR_EHT_RU_ALLOC])
+		rate_info->eht_ru_alloc =
+			nla_get_u8(tb[HWSIM_RATE_INFO_ATTR_EHT_RU_ALLOC]);
+
+	return 0;
+}
+
+static int mac80211_hwsim_parse_ftm_result(struct nlattr *ftm,
+					   struct cfg80211_pmsr_ftm_result *result,
+					   struct genl_info *info)
+{
+	struct nlattr *tb[NL80211_PMSR_FTM_RESP_ATTR_MAX + 1];
+	int ret;
+
+	ret = nla_parse_nested(tb, NL80211_PMSR_FTM_RESP_ATTR_MAX,
+			       ftm, hwsim_ftm_result_policy, info->extack);
+	if (ret)
+		return ret;
+
+	if (tb[NL80211_PMSR_FTM_RESP_ATTR_FAIL_REASON])
+		result->failure_reason =
+			nla_get_u32(tb[NL80211_PMSR_FTM_RESP_ATTR_FAIL_REASON]);
+
+	if (tb[NL80211_PMSR_FTM_RESP_ATTR_BURST_INDEX])
+		result->burst_index =
+			nla_get_u16(tb[NL80211_PMSR_FTM_RESP_ATTR_BURST_INDEX]);
+
+	if (tb[NL80211_PMSR_FTM_RESP_ATTR_NUM_FTMR_ATTEMPTS]) {
+		result->num_ftmr_attempts_valid = 1;
+		result->num_ftmr_attempts =
+			nla_get_u32(tb[NL80211_PMSR_FTM_RESP_ATTR_NUM_FTMR_ATTEMPTS]);
+	}
+
+	if (tb[NL80211_PMSR_FTM_RESP_ATTR_NUM_FTMR_SUCCESSES]) {
+		result->num_ftmr_successes_valid = 1;
+		result->num_ftmr_successes =
+			nla_get_u32(tb[NL80211_PMSR_FTM_RESP_ATTR_NUM_FTMR_SUCCESSES]);
+	}
+
+	if (tb[NL80211_PMSR_FTM_RESP_ATTR_BUSY_RETRY_TIME])
+		result->busy_retry_time =
+			nla_get_u8(tb[NL80211_PMSR_FTM_RESP_ATTR_BUSY_RETRY_TIME]);
+
+	if (tb[NL80211_PMSR_FTM_RESP_ATTR_NUM_BURSTS_EXP])
+		result->num_bursts_exp =
+			nla_get_u8(tb[NL80211_PMSR_FTM_RESP_ATTR_NUM_BURSTS_EXP]);
+
+	if (tb[NL80211_PMSR_FTM_RESP_ATTR_BURST_DURATION])
+		result->burst_duration =
+			nla_get_u8(tb[NL80211_PMSR_FTM_RESP_ATTR_BURST_DURATION]);
+
+	if (tb[NL80211_PMSR_FTM_RESP_ATTR_FTMS_PER_BURST])
+		result->ftms_per_burst =
+			nla_get_u8(tb[NL80211_PMSR_FTM_RESP_ATTR_FTMS_PER_BURST]);
+
+	if (tb[NL80211_PMSR_FTM_RESP_ATTR_RSSI_AVG]) {
+		result->rssi_avg_valid = 1;
+		result->rssi_avg =
+			nla_get_s32(tb[NL80211_PMSR_FTM_RESP_ATTR_RSSI_AVG]);
+	}
+	if (tb[NL80211_PMSR_FTM_RESP_ATTR_RSSI_SPREAD]) {
+		result->rssi_spread_valid = 1;
+		result->rssi_spread =
+			nla_get_s32(tb[NL80211_PMSR_FTM_RESP_ATTR_RSSI_SPREAD]);
+	}
+
+	if (tb[NL80211_PMSR_FTM_RESP_ATTR_TX_RATE]) {
+		result->tx_rate_valid = 1;
+		ret = mac80211_hwsim_parse_rate_info(tb[NL80211_PMSR_FTM_RESP_ATTR_TX_RATE],
+						     &result->tx_rate, info);
+		if (ret)
+			return ret;
+	}
+
+	if (tb[NL80211_PMSR_FTM_RESP_ATTR_RX_RATE]) {
+		result->rx_rate_valid = 1;
+		ret = mac80211_hwsim_parse_rate_info(tb[NL80211_PMSR_FTM_RESP_ATTR_RX_RATE],
+						     &result->rx_rate, info);
+		if (ret)
+			return ret;
+	}
+
+	if (tb[NL80211_PMSR_FTM_RESP_ATTR_RTT_AVG]) {
+		result->rtt_avg_valid = 1;
+		result->rtt_avg =
+			nla_get_u64(tb[NL80211_PMSR_FTM_RESP_ATTR_RTT_AVG]);
+	}
+	if (tb[NL80211_PMSR_FTM_RESP_ATTR_RTT_VARIANCE]) {
+		result->rtt_variance_valid = 1;
+		result->rtt_variance =
+			nla_get_u64(tb[NL80211_PMSR_FTM_RESP_ATTR_RTT_VARIANCE]);
+	}
+	if (tb[NL80211_PMSR_FTM_RESP_ATTR_RTT_SPREAD]) {
+		result->rtt_spread_valid = 1;
+		result->rtt_spread =
+			nla_get_u64(tb[NL80211_PMSR_FTM_RESP_ATTR_RTT_SPREAD]);
+	}
+	if (tb[NL80211_PMSR_FTM_RESP_ATTR_DIST_AVG]) {
+		result->dist_avg_valid = 1;
+		result->dist_avg =
+			nla_get_u64(tb[NL80211_PMSR_FTM_RESP_ATTR_DIST_AVG]);
+	}
+	if (tb[NL80211_PMSR_FTM_RESP_ATTR_DIST_VARIANCE]) {
+		result->dist_variance_valid = 1;
+		result->dist_variance =
+			nla_get_u64(tb[NL80211_PMSR_FTM_RESP_ATTR_DIST_VARIANCE]);
+	}
+	if (tb[NL80211_PMSR_FTM_RESP_ATTR_DIST_SPREAD]) {
+		result->dist_spread_valid = 1;
+		result->dist_spread =
+			nla_get_u64(tb[NL80211_PMSR_FTM_RESP_ATTR_DIST_SPREAD]);
+	}
+
+	if (tb[NL80211_PMSR_FTM_RESP_ATTR_LCI]) {
+		result->lci = nla_data(tb[NL80211_PMSR_FTM_RESP_ATTR_LCI]);
+		result->lci_len = nla_len(tb[NL80211_PMSR_FTM_RESP_ATTR_LCI]);
+	}
+
+	if (tb[NL80211_PMSR_FTM_RESP_ATTR_CIVICLOC]) {
+		result->civicloc = nla_data(tb[NL80211_PMSR_FTM_RESP_ATTR_CIVICLOC]);
+		result->civicloc_len = nla_len(tb[NL80211_PMSR_FTM_RESP_ATTR_CIVICLOC]);
+	}
+
+	return 0;
+}
+
+static int mac80211_hwsim_parse_pmsr_resp(struct nlattr *resp,
+					  struct cfg80211_pmsr_result *result,
+					  struct genl_info *info)
+{
+	struct nlattr *tb[NL80211_PMSR_RESP_ATTR_MAX + 1];
+	struct nlattr *pmsr;
+	int rem;
+	int ret;
+
+	ret = nla_parse_nested(tb, NL80211_PMSR_RESP_ATTR_MAX, resp,
+			       hwsim_pmsr_resp_policy, info->extack);
+
+	if (tb[NL80211_PMSR_RESP_ATTR_STATUS])
+		result->status = nla_get_u32(tb[NL80211_PMSR_RESP_ATTR_STATUS]);
+
+	if (tb[NL80211_PMSR_RESP_ATTR_HOST_TIME])
+		result->host_time = nla_get_u64(tb[NL80211_PMSR_RESP_ATTR_HOST_TIME]);
+
+	if (tb[NL80211_PMSR_RESP_ATTR_AP_TSF]) {
+		result->ap_tsf_valid = 1;
+		result->ap_tsf = nla_get_u64(tb[NL80211_PMSR_RESP_ATTR_AP_TSF]);
+	}
+
+	result->final = !!tb[NL80211_PMSR_RESP_ATTR_FINAL];
+
+	if (tb[NL80211_PMSR_RESP_ATTR_DATA]) {
+		nla_for_each_nested(pmsr, tb[NL80211_PMSR_RESP_ATTR_DATA], rem) {
+			switch (nla_type(pmsr)) {
+			case NL80211_PMSR_TYPE_FTM:
+				result->type = NL80211_PMSR_TYPE_FTM;
+				ret = mac80211_hwsim_parse_ftm_result(pmsr, &result->ftm, info);
+				if (ret)
+					return ret;
+				break;
+			default:
+				NL_SET_ERR_MSG_ATTR(info->extack,
+						    pmsr, "Unknown pmsr resp type");
+				return -EINVAL;
+			}
+		}
+	}
+
+	return 0;
+}
+
+static int mac80211_hwsim_parse_pmsr_result(struct nlattr *peer,
+					    struct cfg80211_pmsr_result *result,
+					    struct genl_info *info)
+{
+	struct nlattr *tb[NL80211_PMSR_PEER_ATTR_MAX + 1];
+	int ret;
+
+	if (!peer)
+		return -EINVAL;
+
+	ret = nla_parse_nested(tb, NL80211_PMSR_PEER_ATTR_MAX, peer,
+			       hwsim_pmsr_peer_result_policy, info->extack);
+	if (ret)
+		return ret;
+
+	if (tb[NL80211_PMSR_PEER_ATTR_ADDR])
+		memcpy(result->addr, nla_data(tb[NL80211_PMSR_PEER_ATTR_ADDR]),
+		       ETH_ALEN);
+
+	if (tb[NL80211_PMSR_PEER_ATTR_RESP]) {
+		ret = mac80211_hwsim_parse_pmsr_resp(tb[NL80211_PMSR_PEER_ATTR_RESP], result, info);
+		if (ret)
+			return ret;
+	}
+
+	return 0;
+};
+
+static int hwsim_pmsr_report_nl(struct sk_buff *msg, struct genl_info *info)
+{
+	struct nlattr *reqattr;
+	const u8 *src;
+	int err = 0;
+	int rem;
+	struct nlattr *peers, *peer;
+	struct mac80211_hwsim_data *data;
+
+	src = nla_data(info->attrs[HWSIM_ATTR_ADDR_TRANSMITTER]);
+	data = get_hwsim_data_ref_from_addr(src);
+	if (!data)
+		return -EINVAL;
+
+	mutex_lock(&data->mutex);
+	if (!data->pmsr_request) {
+		err = -EINVAL;
+		goto out_err;
+	}
+
+	reqattr = info->attrs[HWSIM_ATTR_PMSR_RESULT];
+	if (!reqattr) {
+		err = -EINVAL;
+		goto out_err;
+	}
+
+	peers = nla_find_nested(reqattr, NL80211_PMSR_ATTR_PEERS);
+	if (!peers) {
+		err = -EINVAL;
+		goto out_err;
+	}
+
+	nla_for_each_nested(peer, peers, rem) {
+		struct cfg80211_pmsr_result result;
+
+		err = mac80211_hwsim_parse_pmsr_result(peer, &result, info);
+		if (err)
+			goto out_err;
+
+		cfg80211_pmsr_report(data->pmsr_request_wdev,
+				     data->pmsr_request, &result, GFP_KERNEL);
+	}
+
+	cfg80211_pmsr_complete(data->pmsr_request_wdev, data->pmsr_request,
+			       GFP_KERNEL);
+
+out_err:
+	data->pmsr_request = NULL;
+	data->pmsr_request_wdev = NULL;
+
+	mutex_unlock(&data->mutex);
+	return err;
 }
 
 #define HWSIM_COMMON_OPS					\
@@ -4825,13 +5472,6 @@ static void hwsim_mon_setup(struct net_device *dev)
 	eth_hw_addr_set(dev, addr);
 }
 
-static struct mac80211_hwsim_data *get_hwsim_data_ref_from_addr(const u8 *addr)
-{
-	return rhashtable_lookup_fast(&hwsim_radios_rht,
-				      addr,
-				      hwsim_rht_params);
-}
-
 static void hwsim_register_wmediumd(struct net *net, u32 portid)
 {
 	struct mac80211_hwsim_data *data;
@@ -5507,6 +6147,11 @@ static const struct genl_small_ops hwsim_ops[] = {
 		.doit = hwsim_get_radio_nl,
 		.dumpit = hwsim_dump_radio_nl,
 	},
+	{
+		.cmd = HWSIM_CMD_REPORT_PMSR,
+		.validate = GENL_DONT_VALIDATE_STRICT | GENL_DONT_VALIDATE_DUMP,
+		.doit = hwsim_pmsr_report_nl,
+	}
 };
 
 static struct genl_family hwsim_genl_family __ro_after_init = {
@@ -5518,7 +6163,7 @@ static struct genl_family hwsim_genl_family __ro_after_init = {
 	.module = THIS_MODULE,
 	.small_ops = hwsim_ops,
 	.n_small_ops = ARRAY_SIZE(hwsim_ops),
-	.resv_start_op = HWSIM_CMD_DEL_MAC_ADDR + 1,
+	.resv_start_op = __HWSIM_CMD_MAX,
 	.mcgrps = hwsim_mcgrps,
 	.n_mcgrps = ARRAY_SIZE(hwsim_mcgrps),
 };
@@ -5662,6 +6307,7 @@ static int hwsim_virtio_handle_cmd(struct sk_buff *skb)
 	struct genlmsghdr *gnlh;
 	struct nlattr *tb[HWSIM_ATTR_MAX + 1];
 	struct genl_info info = {};
+	struct netlink_ext_ack extack;
 	int err;
 
 	nlh = nlmsg_hdr(skb);
@@ -5678,6 +6324,7 @@ static int hwsim_virtio_handle_cmd(struct sk_buff *skb)
 	}
 
 	info.attrs = tb;
+	info.extack = &extack;
 
 	switch (gnlh->cmd) {
 	case HWSIM_CMD_FRAME:
@@ -5686,10 +6333,14 @@ static int hwsim_virtio_handle_cmd(struct sk_buff *skb)
 	case HWSIM_CMD_TX_INFO_FRAME:
 		hwsim_tx_info_frame_received_nl(skb, &info);
 		break;
+	case HWSIM_CMD_REPORT_PMSR:
+		hwsim_pmsr_report_nl(skb, &info);
+		break;
 	default:
 		pr_err_ratelimited("hwsim: invalid cmd: %d\n", gnlh->cmd);
 		return -EPROTO;
 	}
+
 	return 0;
 }
 
diff --git a/drivers/net/wireless/mac80211_hwsim.h b/drivers/net/wireless/mac80211_hwsim.h
index 81cd02d2555c..1d54fe4c402f 100644
--- a/drivers/net/wireless/mac80211_hwsim.h
+++ b/drivers/net/wireless/mac80211_hwsim.h
@@ -81,6 +81,9 @@ enum hwsim_tx_control_flags {
  *	to this receiver address for a given station.
  * @HWSIM_CMD_DEL_MAC_ADDR: remove the MAC address again, the attributes
  *	are the same as to @HWSIM_CMD_ADD_MAC_ADDR.
+ * @HWSIM_CMD_START_PMSR: start PMSR
+ * @HWSIM_CMD_ABORT_PMSR: abort PMSR
+ * @HWSIM_CMD_REPORT_PMSR: report PMSR results
  * @__HWSIM_CMD_MAX: enum limit
  */
 enum {
@@ -93,6 +96,9 @@ enum {
 	HWSIM_CMD_GET_RADIO,
 	HWSIM_CMD_ADD_MAC_ADDR,
 	HWSIM_CMD_DEL_MAC_ADDR,
+	HWSIM_CMD_START_PMSR,
+	HWSIM_CMD_ABORT_PMSR,
+	HWSIM_CMD_REPORT_PMSR,
 	__HWSIM_CMD_MAX,
 };
 #define HWSIM_CMD_MAX (_HWSIM_CMD_MAX - 1)
@@ -143,10 +149,11 @@ enum {
  * @HWSIM_ATTR_MLO_SUPPORT: claim MLO support (exact parameters TBD) for
  *	the new radio
  * @HWSIM_ATTR_PMSR_SUPPORT: claim peer measurement support
+ * @HWSIM_ATTR_PMSR_REQUEST: peer measurement request
+ * @HWSIM_ATTR_PMSR_RESULT: peer measurement result
  * @__HWSIM_ATTR_MAX: enum limit
  */
 
-
 enum {
 	HWSIM_ATTR_UNSPEC,
 	HWSIM_ATTR_ADDR_RECEIVER,
@@ -175,6 +182,8 @@ enum {
 	HWSIM_ATTR_CIPHER_SUPPORT,
 	HWSIM_ATTR_MLO_SUPPORT,
 	HWSIM_ATTR_PMSR_SUPPORT,
+	HWSIM_ATTR_PMSR_REQUEST,
+	HWSIM_ATTR_PMSR_RESULT,
 	__HWSIM_ATTR_MAX,
 };
 #define HWSIM_ATTR_MAX (__HWSIM_ATTR_MAX - 1)
@@ -279,4 +288,47 @@ enum {
 	HWSIM_VQ_RX,
 	HWSIM_NUM_VQS,
 };
+
+/**
+ * enum hwsim_rate_info -- bitrate information.
+ *
+ * Information about a receiving or transmitting bitrate
+ * that can be mapped to struct rate_info
+ *
+ * @HWSIM_RATE_INFO_ATTR_FLAGS: bitflag of flags from &enum rate_info_flags
+ * @HWSIM_RATE_INFO_ATTR_MCS: mcs index if struct describes an HT/VHT/HE rate
+ * @HWSIM_RATE_INFO_ATTR_LEGACY: bitrate in 100kbit/s for 802.11abg
+ * @HWSIM_RATE_INFO_ATTR_NSS: number of streams (VHT & HE only)
+ * @HWSIM_RATE_INFO_ATTR_BW: bandwidth (from &enum rate_info_bw)
+ * @HWSIM_RATE_INFO_ATTR_HE_GI: HE guard interval (from &enum nl80211_he_gi)
+ * @HWSIM_RATE_INFO_ATTR_HE_DCM: HE DCM value
+ * @HWSIM_RATE_INFO_ATTR_HE_RU_ALLOC:  HE RU allocation (from &enum nl80211_he_ru_alloc,
+ *	only valid if bw is %RATE_INFO_BW_HE_RU)
+ * @HWSIM_RATE_INFO_ATTR_N_BOUNDED_CH: In case of EDMG the number of bonded channels (1-4)
+ * @HWSIM_RATE_INFO_ATTR_EHT_GI: EHT guard interval (from &enum nl80211_eht_gi)
+ * @HWSIM_RATE_INFO_ATTR_EHT_RU_ALLOC: EHT RU allocation (from &enum nl80211_eht_ru_alloc,
+ *	only valid if bw is %RATE_INFO_BW_EHT_RU)
+ * @NUM_HWSIM_RATE_INFO_ATTRS: internal
+ * @HWSIM_RATE_INFO_ATTR_MAX: highest attribute number
+ */
+enum hwsim_rate_info_attributes {
+	__HWSIM_RATE_INFO_ATTR_INVALID,
+
+	HWSIM_RATE_INFO_ATTR_FLAGS,
+	HWSIM_RATE_INFO_ATTR_MCS,
+	HWSIM_RATE_INFO_ATTR_LEGACY,
+	HWSIM_RATE_INFO_ATTR_NSS,
+	HWSIM_RATE_INFO_ATTR_BW,
+	HWSIM_RATE_INFO_ATTR_HE_GI,
+	HWSIM_RATE_INFO_ATTR_HE_DCM,
+	HWSIM_RATE_INFO_ATTR_HE_RU_ALLOC,
+	HWSIM_RATE_INFO_ATTR_N_BOUNDED_CH,
+	HWSIM_RATE_INFO_ATTR_EHT_GI,
+	HWSIM_RATE_INFO_ATTR_EHT_RU_ALLOC,
+
+	/* keep last */
+	NUM_HWSIM_RATE_INFO_ATTRS,
+	HWSIM_RATE_INFO_ATTR_MAX = NUM_HWSIM_RATE_INFO_ATTRS - 1
+};
+
 #endif /* __MAC80211_HWSIM_H */
diff --git a/include/net/cfg80211.h b/include/net/cfg80211.h
index 33f775b0f0b0..4223e81e0d26 100644
--- a/include/net/cfg80211.h
+++ b/include/net/cfg80211.h
@@ -938,6 +938,16 @@ int cfg80211_chandef_dfs_required(struct wiphy *wiphy,
 				  const struct cfg80211_chan_def *chandef,
 				  enum nl80211_iftype iftype);
 
+/**
+ * cfg80211_send_chandef - sends the channel definition.
+ * @msg: the msg to send channel definition
+ * @chandef: the channel definition to check
+ *
+ * Returns: 0 if sent the channel definition to msg, < 0 on error
+ **/
+int cfg80211_send_chandef(struct sk_buff *msg,
+			  const struct cfg80211_chan_def *chandef);
+
 /**
  * ieee80211_chanwidth_rate_flags - return rate flags for channel width
  * @width: the channel width of the channel
diff --git a/net/wireless/nl80211.c b/net/wireless/nl80211.c
index b972a2135654..f9dda1801f50 100644
--- a/net/wireless/nl80211.c
+++ b/net/wireless/nl80211.c
@@ -3742,8 +3742,8 @@ static int nl80211_set_wiphy(struct sk_buff *skb, struct genl_info *info)
 	return result;
 }
 
-static int nl80211_send_chandef(struct sk_buff *msg,
-				const struct cfg80211_chan_def *chandef)
+int cfg80211_send_chandef(struct sk_buff *msg,
+			  const struct cfg80211_chan_def *chandef)
 {
 	if (WARN_ON(!cfg80211_chandef_valid(chandef)))
 		return -EINVAL;
@@ -3774,6 +3774,13 @@ static int nl80211_send_chandef(struct sk_buff *msg,
 		return -ENOBUFS;
 	return 0;
 }
+EXPORT_SYMBOL(cfg80211_send_chandef);
+
+static int nl80211_send_chandef(struct sk_buff *msg,
+				const struct cfg80211_chan_def *chandef)
+{
+	return cfg80211_send_chandef(msg, chandef);
+}
 
 static int nl80211_send_iface(struct sk_buff *msg, u32 portid, u32 seq, int flags,
 			      struct cfg80211_registered_device *rdev,
-- 
2.39.0.246.g2a6d74b583-goog

Re: [PATCH v6 2/2] mac80211_hwsim: handle FTM requests with virtio

[Greg KH]

On Tue, Jan 24, 2023 at 02:54:30PM +0000, Jaewan Kim wrote:
> This CL allows mac80211_hwsim to receive FTM request and send FTM response.

What is a "CL"?

What is "FTM"?

And why is this line not wrapped at 72 columns like your editor asked
you do when you committed it?  :)

> It passthrough request to wmediumd and gets response with virtio
> to get the FTM information with another STA.

What is "STA"?

> 
> This CL adds following commands

Again, what is "CL"?

>  - HWSIM_CMD_START_PMSR: To send request to wmediumd
>  - HWSIM_CMD_ABORT_PMSR: To send abort to wmediumd
>  - HWSIM_CMD_REPORT_PMSR: To receive response from wmediumd

Why isn't this 3 different patches?  One per thing you are adding?

> Request and response are formatted the same way as pmsr.c.
> One exception is for sending rate_info -- hwsim_rate_info_attributes is
> added to send rate_info as is.

I do not understand what this last sentence means, sorry.

> 
> Signed-off-by: Jaewan Kim <jaewan@google.com>
> ---
> V5 -> V6: Added per change patch history.
> V4 -> V5: N/A.
> V3 -> V4: Added more comments about new commands in mac80211_hwsim.
> V1 -> V3: Initial commit (includes resends).
> ---
>  drivers/net/wireless/mac80211_hwsim.c | 679 +++++++++++++++++++++++-
>  drivers/net/wireless/mac80211_hwsim.h |  54 +-
>  include/net/cfg80211.h                |  10 +
>  net/wireless/nl80211.c                |  11 +-
>  4 files changed, 737 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/net/wireless/mac80211_hwsim.c b/drivers/net/wireless/mac80211_hwsim.c
> index 84c9db9178c3..4191037f73b6 100644
> --- a/drivers/net/wireless/mac80211_hwsim.c
> +++ b/drivers/net/wireless/mac80211_hwsim.c
> @@ -721,6 +721,8 @@ struct mac80211_hwsim_data {
>  
>  	/* only used when pmsr capability is supplied */
>  	struct cfg80211_pmsr_capabilities pmsr_capa;
> +	struct cfg80211_pmsr_request *pmsr_request;
> +	struct wireless_dev *pmsr_request_wdev;
>  
>  	struct mac80211_hwsim_link_data link_data[IEEE80211_MLD_MAX_NUM_LINKS];
>  };
> @@ -750,6 +752,13 @@ struct hwsim_radiotap_ack_hdr {
>  	__le16 rt_chbitmask;
>  } __packed;
>  
> +static struct mac80211_hwsim_data *get_hwsim_data_ref_from_addr(const u8 *addr)
> +{
> +	return rhashtable_lookup_fast(&hwsim_radios_rht,
> +				      addr,
> +				      hwsim_rht_params);

Odd line wrapping :(

> +}
> +
>  /* MAC80211_HWSIM netlink family */
>  static struct genl_family hwsim_genl_family;
>  
> @@ -763,6 +772,81 @@ static const struct genl_multicast_group hwsim_mcgrps[] = {
>  
>  /* MAC80211_HWSIM netlink policy */
>  
> +static const struct nla_policy
> +hwsim_rate_info_policy[HWSIM_RATE_INFO_ATTR_MAX + 1] = {
> +	[HWSIM_RATE_INFO_ATTR_FLAGS] = { .type = NLA_U8 },
> +	[HWSIM_RATE_INFO_ATTR_MCS] = { .type = NLA_U8 },
> +	[HWSIM_RATE_INFO_ATTR_LEGACY] = { .type = NLA_U16 },
> +	[HWSIM_RATE_INFO_ATTR_NSS] = { .type = NLA_U8 },
> +	[HWSIM_RATE_INFO_ATTR_BW] = { .type = NLA_U8 },
> +	[HWSIM_RATE_INFO_ATTR_HE_GI] = { .type = NLA_U8 },
> +	[HWSIM_RATE_INFO_ATTR_HE_DCM] = { .type = NLA_U8 },
> +	[HWSIM_RATE_INFO_ATTR_HE_RU_ALLOC] = { .type = NLA_U8 },
> +	[HWSIM_RATE_INFO_ATTR_N_BOUNDED_CH] = { .type = NLA_U8 },
> +	[HWSIM_RATE_INFO_ATTR_EHT_GI] = { .type = NLA_U8 },
> +	[HWSIM_RATE_INFO_ATTR_EHT_RU_ALLOC] = { .type = NLA_U8 },
> +};
> +
> +static const struct nla_policy
> +hwsim_ftm_result_policy[NL80211_PMSR_FTM_RESP_ATTR_MAX + 1] = {
> +	[NL80211_PMSR_FTM_RESP_ATTR_FAIL_REASON] = { .type = NLA_U32 },
> +	[NL80211_PMSR_FTM_RESP_ATTR_BURST_INDEX] = { .type = NLA_U16 },
> +	[NL80211_PMSR_FTM_RESP_ATTR_NUM_FTMR_ATTEMPTS] = { .type = NLA_U32 },
> +	[NL80211_PMSR_FTM_RESP_ATTR_NUM_FTMR_SUCCESSES] = { .type = NLA_U32 },
> +	[NL80211_PMSR_FTM_RESP_ATTR_BUSY_RETRY_TIME] = { .type = NLA_U8 },
> +	[NL80211_PMSR_FTM_RESP_ATTR_NUM_BURSTS_EXP] = { .type = NLA_U8 },
> +	[NL80211_PMSR_FTM_RESP_ATTR_BURST_DURATION] = { .type = NLA_U8 },
> +	[NL80211_PMSR_FTM_RESP_ATTR_FTMS_PER_BURST] = { .type = NLA_U8 },
> +	[NL80211_PMSR_FTM_RESP_ATTR_RSSI_AVG] = { .type = NLA_U32 },
> +	[NL80211_PMSR_FTM_RESP_ATTR_RSSI_SPREAD] = { .type = NLA_U32 },
> +	[NL80211_PMSR_FTM_RESP_ATTR_TX_RATE] =
> +		NLA_POLICY_NESTED(hwsim_rate_info_policy),
> +	[NL80211_PMSR_FTM_RESP_ATTR_RX_RATE] =
> +		NLA_POLICY_NESTED(hwsim_rate_info_policy),
> +	[NL80211_PMSR_FTM_RESP_ATTR_RTT_AVG] = { .type = NLA_U64 },
> +	[NL80211_PMSR_FTM_RESP_ATTR_RTT_VARIANCE] = { .type = NLA_U64 },
> +	[NL80211_PMSR_FTM_RESP_ATTR_RTT_SPREAD] = { .type = NLA_U64 },
> +	[NL80211_PMSR_FTM_RESP_ATTR_DIST_AVG] = { .type = NLA_U64 },
> +	[NL80211_PMSR_FTM_RESP_ATTR_DIST_VARIANCE] = { .type = NLA_U64 },
> +	[NL80211_PMSR_FTM_RESP_ATTR_DIST_SPREAD] = { .type = NLA_U64 },
> +	[NL80211_PMSR_FTM_RESP_ATTR_LCI] = { .type = NLA_STRING },
> +	[NL80211_PMSR_FTM_RESP_ATTR_CIVICLOC] = { .type = NLA_STRING },
> +};
> +
> +static const struct nla_policy
> +hwsim_pmsr_resp_type_policy[NL80211_PMSR_TYPE_MAX + 1] = {
> +	[NL80211_PMSR_TYPE_FTM] = NLA_POLICY_NESTED(hwsim_ftm_result_policy),
> +};
> +
> +static const struct nla_policy
> +hwsim_pmsr_resp_policy[NL80211_PMSR_RESP_ATTR_MAX + 1] = {
> +	[NL80211_PMSR_RESP_ATTR_STATUS] = { .type = NLA_U32 },
> +	[NL80211_PMSR_RESP_ATTR_HOST_TIME] = { .type = NLA_U64 },
> +	[NL80211_PMSR_RESP_ATTR_AP_TSF] = { .type = NLA_U64 },
> +	[NL80211_PMSR_RESP_ATTR_FINAL] = { .type = NLA_FLAG },
> +	[NL80211_PMSR_RESP_ATTR_DATA] =
> +		NLA_POLICY_NESTED(hwsim_pmsr_resp_type_policy),

Are you sure these line-wraps are needed?  We can go to 100 columns now,
right?

> +};
> +
> +static const struct nla_policy
> +hwsim_pmsr_peer_result_policy[NL80211_PMSR_PEER_ATTR_MAX + 1] = {
> +	[NL80211_PMSR_PEER_ATTR_ADDR] = NLA_POLICY_ETH_ADDR_COMPAT,
> +	[NL80211_PMSR_PEER_ATTR_CHAN] = { .type = NLA_REJECT },
> +	[NL80211_PMSR_PEER_ATTR_REQ] = { .type = NLA_REJECT },
> +	[NL80211_PMSR_PEER_ATTR_RESP] =
> +		NLA_POLICY_NESTED(hwsim_pmsr_resp_policy),
> +};
> +
> +static const struct nla_policy
> +hwsim_pmsr_peers_result_policy[NL80211_PMSR_ATTR_MAX + 1] = {
> +	[NL80211_PMSR_ATTR_MAX_PEERS] = { .type = NLA_REJECT },
> +	[NL80211_PMSR_ATTR_REPORT_AP_TSF] = { .type = NLA_REJECT },
> +	[NL80211_PMSR_ATTR_RANDOMIZE_MAC_ADDR] = { .type = NLA_REJECT },
> +	[NL80211_PMSR_ATTR_TYPE_CAPA] = { .type = NLA_REJECT },
> +	[NL80211_PMSR_ATTR_PEERS] =
> +		NLA_POLICY_NESTED_ARRAY(hwsim_pmsr_peer_result_policy),
> +};
> +
>  static const struct nla_policy
>  hwsim_ftm_capa_policy[NL80211_PMSR_FTM_CAPA_ATTR_MAX + 1] = {
>  	[NL80211_PMSR_FTM_CAPA_ATTR_ASAP] = { .type = NLA_FLAG },
> @@ -780,7 +864,7 @@ hwsim_ftm_capa_policy[NL80211_PMSR_FTM_CAPA_ATTR_MAX + 1] = {
>  };
>  
>  static const struct nla_policy
> -hwsim_pmsr_type_policy[NL80211_PMSR_TYPE_MAX + 1] = {
> +hwsim_pmsr_capa_type_policy[NL80211_PMSR_TYPE_MAX + 1] = {
>  	[NL80211_PMSR_TYPE_FTM] = NLA_POLICY_NESTED(hwsim_ftm_capa_policy),
>  };
>  
> @@ -790,7 +874,7 @@ hwsim_pmsr_capa_policy[NL80211_PMSR_ATTR_MAX + 1] = {
>  	[NL80211_PMSR_ATTR_REPORT_AP_TSF] = { .type = NLA_FLAG },
>  	[NL80211_PMSR_ATTR_RANDOMIZE_MAC_ADDR] = { .type = NLA_FLAG },
>  	[NL80211_PMSR_ATTR_TYPE_CAPA] =
> -		NLA_POLICY_NESTED(hwsim_pmsr_type_policy),
> +		NLA_POLICY_NESTED(hwsim_pmsr_capa_type_policy),
>  	[NL80211_PMSR_ATTR_PEERS] = { .type = NLA_REJECT }, // only for request.
>  };
>  
> @@ -823,6 +907,7 @@ static const struct nla_policy hwsim_genl_policy[HWSIM_ATTR_MAX + 1] = {
>  	[HWSIM_ATTR_CIPHER_SUPPORT] = { .type = NLA_BINARY },
>  	[HWSIM_ATTR_MLO_SUPPORT] = { .type = NLA_FLAG },
>  	[HWSIM_ATTR_PMSR_SUPPORT] = NLA_POLICY_NESTED(hwsim_pmsr_capa_policy),
> +	[HWSIM_ATTR_PMSR_RESULT] = NLA_POLICY_NESTED(hwsim_pmsr_peers_result_policy),
>  };
>  
>  #if IS_REACHABLE(CONFIG_VIRTIO)
> @@ -3142,16 +3227,578 @@ static int mac80211_hwsim_change_sta_links(struct ieee80211_hw *hw,
>  	return 0;
>  }
>  
> -static int mac80211_hwsim_start_pmsr(struct ieee80211_hw *hw, struct ieee80211_vif *vif,
> +static int mac80211_hwsim_send_pmsr_ftm_request_peer(struct sk_buff *msg,
> +						     struct cfg80211_pmsr_ftm_request_peer *request)
> +{
> +	void *ftm;

Are you sure this is void?  Why isn't this a pointer to a real structure
that you are asking for?

> +
> +	if (!request || !request->requested)
> +		return -EINVAL;

How can these happen?

> +
> +	ftm = nla_nest_start(msg, NL80211_PMSR_TYPE_FTM);
> +	if (!ftm)
> +		return -ENOBUFS;
> +
> +	if (nla_put_u32(msg, NL80211_PMSR_FTM_REQ_ATTR_PREAMBLE,
> +			request->preamble))
> +		return -ENOBUFS;
> +
> +	if (nla_put_u16(msg, NL80211_PMSR_FTM_REQ_ATTR_BURST_PERIOD,
> +			request->burst_period))
> +		return -ENOBUFS;
> +
> +	if (request->asap &&
> +	    nla_put_flag(msg, NL80211_PMSR_FTM_REQ_ATTR_ASAP))
> +		return -ENOBUFS;
> +
> +	if (request->request_lci &&
> +	    nla_put_flag(msg, NL80211_PMSR_FTM_REQ_ATTR_REQUEST_LCI))
> +		return -ENOBUFS;
> +
> +	if (request->request_civicloc &&
> +	    nla_put_flag(msg, NL80211_PMSR_FTM_REQ_ATTR_REQUEST_CIVICLOC))
> +		return -ENOBUFS;
> +
> +	if (request->trigger_based &&
> +	    nla_put_flag(msg, NL80211_PMSR_FTM_REQ_ATTR_TRIGGER_BASED))
> +		return -ENOBUFS;
> +
> +	if (request->non_trigger_based &&
> +	    nla_put_flag(msg, NL80211_PMSR_FTM_REQ_ATTR_NON_TRIGGER_BASED))
> +		return -ENOBUFS;
> +
> +	if (request->lmr_feedback &&
> +	    nla_put_flag(msg, NL80211_PMSR_FTM_REQ_ATTR_LMR_FEEDBACK))
> +		return -ENOBUFS;
> +
> +	if (nla_put_u8(msg, NL80211_PMSR_FTM_REQ_ATTR_NUM_BURSTS_EXP,
> +		       request->num_bursts_exp))
> +		return -ENOBUFS;
> +
> +	if (nla_put_u8(msg, NL80211_PMSR_FTM_REQ_ATTR_BURST_DURATION,
> +		       request->burst_duration))
> +		return -ENOBUFS;
> +
> +	if (nla_put_u8(msg, NL80211_PMSR_FTM_REQ_ATTR_FTMS_PER_BURST,
> +		       request->ftms_per_burst))
> +		return -ENOBUFS;
> +
> +	if (nla_put_u8(msg, NL80211_PMSR_FTM_REQ_ATTR_NUM_FTMR_RETRIES,
> +		       request->ftmr_retries))
> +		return -ENOBUFS;
> +
> +	if (nla_put_u8(msg, NL80211_PMSR_FTM_REQ_ATTR_BSS_COLOR,
> +		       request->bss_color))
> +		return -ENOBUFS;
> +
> +	nla_nest_end(msg, ftm);
> +
> +	return 0;
> +}
> +
> +static int mac80211_hwsim_send_pmsr_request_peer(struct sk_buff *msg,
> +						 struct cfg80211_pmsr_request_peer *request)
> +{
> +	void *peer, *chandef, *req, *data;

Same here, why void * when you konw the types being used?

> +	int err;
> +
> +	peer = nla_nest_start(msg, NL80211_PMSR_ATTR_PEERS);
> +	if (!peer)
> +		return -ENOBUFS;
> +
> +	if (nla_put(msg, NL80211_PMSR_PEER_ATTR_ADDR, ETH_ALEN,
> +		    request->addr))
> +		return -ENOBUFS;
> +
> +	chandef = nla_nest_start(msg, NL80211_PMSR_PEER_ATTR_CHAN);
> +	if (!chandef)
> +		return -ENOBUFS;
> +
> +	err = cfg80211_send_chandef(msg, &request->chandef);
> +	if (err)
> +		return err;
> +
> +	nla_nest_end(msg, chandef);
> +
> +	req = nla_nest_start(msg, NL80211_PMSR_PEER_ATTR_REQ);
> +	if (request->report_ap_tsf &&
> +	    nla_put_flag(msg, NL80211_PMSR_REQ_ATTR_GET_AP_TSF))
> +		return -ENOBUFS;
> +
> +	data = nla_nest_start(msg, NL80211_PMSR_REQ_ATTR_DATA);
> +	if (!data)
> +		return -ENOBUFS;
> +
> +	mac80211_hwsim_send_pmsr_ftm_request_peer(msg, &request->ftm);
> +	nla_nest_end(msg, data);
> +	nla_nest_end(msg, req);
> +	nla_nest_end(msg, peer);
> +
> +	return 0;
> +}
> +
> +static int mac80211_hwsim_send_pmsr_request(struct sk_buff *msg,
> +					    struct cfg80211_pmsr_request *request)
> +{
> +	int err;
> +	void *pmsr;

and here (hint larger variables go before smaller ones usually, right?)

> +
> +	pmsr = nla_nest_start(msg, NL80211_ATTR_PEER_MEASUREMENTS);
> +	if (!pmsr)
> +		return -ENOBUFS;
> +
> +	if (nla_put_u32(msg, NL80211_ATTR_TIMEOUT, request->timeout))
> +		return -ENOBUFS;
> +
> +	if (!is_zero_ether_addr(request->mac_addr)) {
> +		if (nla_put(msg, NL80211_ATTR_MAC, ETH_ALEN, request->mac_addr))
> +			return -ENOBUFS;
> +		if (nla_put(msg, NL80211_ATTR_MAC_MASK, ETH_ALEN,
> +			    request->mac_addr_mask))
> +			return -ENOBUFS;
> +	}
> +
> +	for (int i = 0; i < request->n_peers; i++) {
> +		err = mac80211_hwsim_send_pmsr_request_peer(msg,
> +							    &request->peers[i]);

Is this line wrap needed?

> +		if (err)
> +			return err;
> +	}
> +
> +	nla_nest_end(msg, pmsr);
> +
> +	return 0;
> +}
> +
> +static int mac80211_hwsim_start_pmsr(struct ieee80211_hw *hw,
> +				     struct ieee80211_vif *vif,
>  				     struct cfg80211_pmsr_request *request)
>  {
> -	return -EOPNOTSUPP;
> +	struct mac80211_hwsim_data *data = hw->priv;
> +	u32 _portid = READ_ONCE(data->wmediumd);

Why READ_ONCE()?

Shouldn't you only access this after the lock is held?

thanks,

greg k-h

Re: [PATCH v6 2/2] mac80211_hwsim: handle FTM requests with virtio

[Jaewan Kim]

On Wed, Jan 25, 2023 at 12:51 AM Greg KH <gregkh@linuxfoundation.org> wrote:
>
> On Tue, Jan 24, 2023 at 02:54:30PM +0000, Jaewan Kim wrote:
> > This CL allows mac80211_hwsim to receive FTM request and send FTM response.
>
> What is a "CL"?
>
> What is "FTM"?
FTM (fine time measurement) is measuring time between two Wi-Fi devices with .
Added the description to this commit message, in addition to the cover ration.

>
> And why is this line not wrapped at 72 columns like your editor asked
> you do when you committed it?  :)
>
> > It passthrough request to wmediumd and gets response with virtio
> > to get the FTM information with another STA.
>
> What is "STA"?
It's an abbreviation of station, and it means Wi-Fi enabled device.
(term from IEEE 802.11 spec).
Changed to use simpler terms.

>
> >
> > This CL adds following commands
>
> Again, what is "CL"?
>
> >  - HWSIM_CMD_START_PMSR: To send request to wmediumd
> >  - HWSIM_CMD_ABORT_PMSR: To send abort to wmediumd
> >  - HWSIM_CMD_REPORT_PMSR: To receive response from wmediumd
>
> Why isn't this 3 different patches?  One per thing you are adding?

Let me split and upload the next patches.

>
> > Request and response are formatted the same way as pmsr.c.
> > One exception is for sending rate_info -- hwsim_rate_info_attributes is
> > added to send rate_info as is.
>
> I do not understand what this last sentence means, sorry.
>
> >
> > Signed-off-by: Jaewan Kim <jaewan@google.com>
> > ---
> > V5 -> V6: Added per change patch history.
> > V4 -> V5: N/A.
> > V3 -> V4: Added more comments about new commands in mac80211_hwsim.
> > V1 -> V3: Initial commit (includes resends).
> > ---
> >  drivers/net/wireless/mac80211_hwsim.c | 679 +++++++++++++++++++++++-
> >  drivers/net/wireless/mac80211_hwsim.h |  54 +-
> >  include/net/cfg80211.h                |  10 +
> >  net/wireless/nl80211.c                |  11 +-
> >  4 files changed, 737 insertions(+), 17 deletions(-)
> >
> > diff --git a/drivers/net/wireless/mac80211_hwsim.c b/drivers/net/wireless/mac80211_hwsim.c
> > index 84c9db9178c3..4191037f73b6 100644
> > --- a/drivers/net/wireless/mac80211_hwsim.c
> > +++ b/drivers/net/wireless/mac80211_hwsim.c
> > @@ -721,6 +721,8 @@ struct mac80211_hwsim_data {
> >
> >       /* only used when pmsr capability is supplied */
> >       struct cfg80211_pmsr_capabilities pmsr_capa;
> > +     struct cfg80211_pmsr_request *pmsr_request;
> > +     struct wireless_dev *pmsr_request_wdev;
> >
> >       struct mac80211_hwsim_link_data link_data[IEEE80211_MLD_MAX_NUM_LINKS];
> >  };
> > @@ -750,6 +752,13 @@ struct hwsim_radiotap_ack_hdr {
> >       __le16 rt_chbitmask;
> >  } __packed;
> >
> > +static struct mac80211_hwsim_data *get_hwsim_data_ref_from_addr(const u8 *addr)
> > +{
> > +     return rhashtable_lookup_fast(&hwsim_radios_rht,
> > +                                   addr,
> > +                                   hwsim_rht_params);
>
> Odd line wrapping :(

Done.

>
> > +}
> > +
> >  /* MAC80211_HWSIM netlink family */
> >  static struct genl_family hwsim_genl_family;
> >
> > @@ -763,6 +772,81 @@ static const struct genl_multicast_group hwsim_mcgrps[] = {
> >
> >  /* MAC80211_HWSIM netlink policy */
> >
> > +static const struct nla_policy
> > +hwsim_rate_info_policy[HWSIM_RATE_INFO_ATTR_MAX + 1] = {
> > +     [HWSIM_RATE_INFO_ATTR_FLAGS] = { .type = NLA_U8 },
> > +     [HWSIM_RATE_INFO_ATTR_MCS] = { .type = NLA_U8 },
> > +     [HWSIM_RATE_INFO_ATTR_LEGACY] = { .type = NLA_U16 },
> > +     [HWSIM_RATE_INFO_ATTR_NSS] = { .type = NLA_U8 },
> > +     [HWSIM_RATE_INFO_ATTR_BW] = { .type = NLA_U8 },
> > +     [HWSIM_RATE_INFO_ATTR_HE_GI] = { .type = NLA_U8 },
> > +     [HWSIM_RATE_INFO_ATTR_HE_DCM] = { .type = NLA_U8 },
> > +     [HWSIM_RATE_INFO_ATTR_HE_RU_ALLOC] = { .type = NLA_U8 },
> > +     [HWSIM_RATE_INFO_ATTR_N_BOUNDED_CH] = { .type = NLA_U8 },
> > +     [HWSIM_RATE_INFO_ATTR_EHT_GI] = { .type = NLA_U8 },
> > +     [HWSIM_RATE_INFO_ATTR_EHT_RU_ALLOC] = { .type = NLA_U8 },
> > +};
> > +
> > +static const struct nla_policy
> > +hwsim_ftm_result_policy[NL80211_PMSR_FTM_RESP_ATTR_MAX + 1] = {
> > +     [NL80211_PMSR_FTM_RESP_ATTR_FAIL_REASON] = { .type = NLA_U32 },
> > +     [NL80211_PMSR_FTM_RESP_ATTR_BURST_INDEX] = { .type = NLA_U16 },
> > +     [NL80211_PMSR_FTM_RESP_ATTR_NUM_FTMR_ATTEMPTS] = { .type = NLA_U32 },
> > +     [NL80211_PMSR_FTM_RESP_ATTR_NUM_FTMR_SUCCESSES] = { .type = NLA_U32 },
> > +     [NL80211_PMSR_FTM_RESP_ATTR_BUSY_RETRY_TIME] = { .type = NLA_U8 },
> > +     [NL80211_PMSR_FTM_RESP_ATTR_NUM_BURSTS_EXP] = { .type = NLA_U8 },
> > +     [NL80211_PMSR_FTM_RESP_ATTR_BURST_DURATION] = { .type = NLA_U8 },
> > +     [NL80211_PMSR_FTM_RESP_ATTR_FTMS_PER_BURST] = { .type = NLA_U8 },
> > +     [NL80211_PMSR_FTM_RESP_ATTR_RSSI_AVG] = { .type = NLA_U32 },
> > +     [NL80211_PMSR_FTM_RESP_ATTR_RSSI_SPREAD] = { .type = NLA_U32 },
> > +     [NL80211_PMSR_FTM_RESP_ATTR_TX_RATE] =
> > +             NLA_POLICY_NESTED(hwsim_rate_info_policy),
> > +     [NL80211_PMSR_FTM_RESP_ATTR_RX_RATE] =
> > +             NLA_POLICY_NESTED(hwsim_rate_info_policy),
> > +     [NL80211_PMSR_FTM_RESP_ATTR_RTT_AVG] = { .type = NLA_U64 },
> > +     [NL80211_PMSR_FTM_RESP_ATTR_RTT_VARIANCE] = { .type = NLA_U64 },
> > +     [NL80211_PMSR_FTM_RESP_ATTR_RTT_SPREAD] = { .type = NLA_U64 },
> > +     [NL80211_PMSR_FTM_RESP_ATTR_DIST_AVG] = { .type = NLA_U64 },
> > +     [NL80211_PMSR_FTM_RESP_ATTR_DIST_VARIANCE] = { .type = NLA_U64 },
> > +     [NL80211_PMSR_FTM_RESP_ATTR_DIST_SPREAD] = { .type = NLA_U64 },
> > +     [NL80211_PMSR_FTM_RESP_ATTR_LCI] = { .type = NLA_STRING },
> > +     [NL80211_PMSR_FTM_RESP_ATTR_CIVICLOC] = { .type = NLA_STRING },
> > +};
> > +
> > +static const struct nla_policy
> > +hwsim_pmsr_resp_type_policy[NL80211_PMSR_TYPE_MAX + 1] = {
> > +     [NL80211_PMSR_TYPE_FTM] = NLA_POLICY_NESTED(hwsim_ftm_result_policy),
> > +};
> > +
> > +static const struct nla_policy
> > +hwsim_pmsr_resp_policy[NL80211_PMSR_RESP_ATTR_MAX + 1] = {
> > +     [NL80211_PMSR_RESP_ATTR_STATUS] = { .type = NLA_U32 },
> > +     [NL80211_PMSR_RESP_ATTR_HOST_TIME] = { .type = NLA_U64 },
> > +     [NL80211_PMSR_RESP_ATTR_AP_TSF] = { .type = NLA_U64 },
> > +     [NL80211_PMSR_RESP_ATTR_FINAL] = { .type = NLA_FLAG },
> > +     [NL80211_PMSR_RESP_ATTR_DATA] =
> > +             NLA_POLICY_NESTED(hwsim_pmsr_resp_type_policy),
>
> Are you sure these line-wraps are needed?  We can go to 100 columns now,
> right?

Done for here and other places.

>
> > +};
> > +
> > +static const struct nla_policy
> > +hwsim_pmsr_peer_result_policy[NL80211_PMSR_PEER_ATTR_MAX + 1] = {
> > +     [NL80211_PMSR_PEER_ATTR_ADDR] = NLA_POLICY_ETH_ADDR_COMPAT,
> > +     [NL80211_PMSR_PEER_ATTR_CHAN] = { .type = NLA_REJECT },
> > +     [NL80211_PMSR_PEER_ATTR_REQ] = { .type = NLA_REJECT },
> > +     [NL80211_PMSR_PEER_ATTR_RESP] =
> > +             NLA_POLICY_NESTED(hwsim_pmsr_resp_policy),
> > +};
> > +
> > +static const struct nla_policy
> > +hwsim_pmsr_peers_result_policy[NL80211_PMSR_ATTR_MAX + 1] = {
> > +     [NL80211_PMSR_ATTR_MAX_PEERS] = { .type = NLA_REJECT },
> > +     [NL80211_PMSR_ATTR_REPORT_AP_TSF] = { .type = NLA_REJECT },
> > +     [NL80211_PMSR_ATTR_RANDOMIZE_MAC_ADDR] = { .type = NLA_REJECT },
> > +     [NL80211_PMSR_ATTR_TYPE_CAPA] = { .type = NLA_REJECT },
> > +     [NL80211_PMSR_ATTR_PEERS] =
> > +             NLA_POLICY_NESTED_ARRAY(hwsim_pmsr_peer_result_policy),
> > +};
> > +
> >  static const struct nla_policy
> >  hwsim_ftm_capa_policy[NL80211_PMSR_FTM_CAPA_ATTR_MAX + 1] = {
> >       [NL80211_PMSR_FTM_CAPA_ATTR_ASAP] = { .type = NLA_FLAG },
> > @@ -780,7 +864,7 @@ hwsim_ftm_capa_policy[NL80211_PMSR_FTM_CAPA_ATTR_MAX + 1] = {
> >  };
> >
> >  static const struct nla_policy
> > -hwsim_pmsr_type_policy[NL80211_PMSR_TYPE_MAX + 1] = {
> > +hwsim_pmsr_capa_type_policy[NL80211_PMSR_TYPE_MAX + 1] = {
> >       [NL80211_PMSR_TYPE_FTM] = NLA_POLICY_NESTED(hwsim_ftm_capa_policy),
> >  };
> >
> > @@ -790,7 +874,7 @@ hwsim_pmsr_capa_policy[NL80211_PMSR_ATTR_MAX + 1] = {
> >       [NL80211_PMSR_ATTR_REPORT_AP_TSF] = { .type = NLA_FLAG },
> >       [NL80211_PMSR_ATTR_RANDOMIZE_MAC_ADDR] = { .type = NLA_FLAG },
> >       [NL80211_PMSR_ATTR_TYPE_CAPA] =
> > -             NLA_POLICY_NESTED(hwsim_pmsr_type_policy),
> > +             NLA_POLICY_NESTED(hwsim_pmsr_capa_type_policy),
> >       [NL80211_PMSR_ATTR_PEERS] = { .type = NLA_REJECT }, // only for request.
> >  };
> >
> > @@ -823,6 +907,7 @@ static const struct nla_policy hwsim_genl_policy[HWSIM_ATTR_MAX + 1] = {
> >       [HWSIM_ATTR_CIPHER_SUPPORT] = { .type = NLA_BINARY },
> >       [HWSIM_ATTR_MLO_SUPPORT] = { .type = NLA_FLAG },
> >       [HWSIM_ATTR_PMSR_SUPPORT] = NLA_POLICY_NESTED(hwsim_pmsr_capa_policy),
> > +     [HWSIM_ATTR_PMSR_RESULT] = NLA_POLICY_NESTED(hwsim_pmsr_peers_result_policy),
> >  };
> >
> >  #if IS_REACHABLE(CONFIG_VIRTIO)
> > @@ -3142,16 +3227,578 @@ static int mac80211_hwsim_change_sta_links(struct ieee80211_hw *hw,
> >       return 0;
> >  }
> >
> > -static int mac80211_hwsim_start_pmsr(struct ieee80211_hw *hw, struct ieee80211_vif *vif,
> > +static int mac80211_hwsim_send_pmsr_ftm_request_peer(struct sk_buff *msg,
> > +                                                  struct cfg80211_pmsr_ftm_request_peer *request)
> > +{
> > +     void *ftm;
>
> Are you sure this is void?  Why isn't this a pointer to a real structure
> that you are asking for?

Done.

>
> > +
> > +     if (!request || !request->requested)
> > +             return -EINVAL;
>
> How can these happen?

I agree, and I think that the field `requested` seems useless.
But let me check the variable as long as it exists and existing code
sets the value to `true` in pmsr.c.

>
> > +
> > +     ftm = nla_nest_start(msg, NL80211_PMSR_TYPE_FTM);
> > +     if (!ftm)
> > +             return -ENOBUFS;
> > +
> > +     if (nla_put_u32(msg, NL80211_PMSR_FTM_REQ_ATTR_PREAMBLE,
> > +                     request->preamble))
> > +             return -ENOBUFS;
> > +
> > +     if (nla_put_u16(msg, NL80211_PMSR_FTM_REQ_ATTR_BURST_PERIOD,
> > +                     request->burst_period))
> > +             return -ENOBUFS;
> > +
> > +     if (request->asap &&
> > +         nla_put_flag(msg, NL80211_PMSR_FTM_REQ_ATTR_ASAP))
> > +             return -ENOBUFS;
> > +
> > +     if (request->request_lci &&
> > +         nla_put_flag(msg, NL80211_PMSR_FTM_REQ_ATTR_REQUEST_LCI))
> > +             return -ENOBUFS;
> > +
> > +     if (request->request_civicloc &&
> > +         nla_put_flag(msg, NL80211_PMSR_FTM_REQ_ATTR_REQUEST_CIVICLOC))
> > +             return -ENOBUFS;
> > +
> > +     if (request->trigger_based &&
> > +         nla_put_flag(msg, NL80211_PMSR_FTM_REQ_ATTR_TRIGGER_BASED))
> > +             return -ENOBUFS;
> > +
> > +     if (request->non_trigger_based &&
> > +         nla_put_flag(msg, NL80211_PMSR_FTM_REQ_ATTR_NON_TRIGGER_BASED))
> > +             return -ENOBUFS;
> > +
> > +     if (request->lmr_feedback &&
> > +         nla_put_flag(msg, NL80211_PMSR_FTM_REQ_ATTR_LMR_FEEDBACK))
> > +             return -ENOBUFS;
> > +
> > +     if (nla_put_u8(msg, NL80211_PMSR_FTM_REQ_ATTR_NUM_BURSTS_EXP,
> > +                    request->num_bursts_exp))
> > +             return -ENOBUFS;
> > +
> > +     if (nla_put_u8(msg, NL80211_PMSR_FTM_REQ_ATTR_BURST_DURATION,
> > +                    request->burst_duration))
> > +             return -ENOBUFS;
> > +
> > +     if (nla_put_u8(msg, NL80211_PMSR_FTM_REQ_ATTR_FTMS_PER_BURST,
> > +                    request->ftms_per_burst))
> > +             return -ENOBUFS;
> > +
> > +     if (nla_put_u8(msg, NL80211_PMSR_FTM_REQ_ATTR_NUM_FTMR_RETRIES,
> > +                    request->ftmr_retries))
> > +             return -ENOBUFS;
> > +
> > +     if (nla_put_u8(msg, NL80211_PMSR_FTM_REQ_ATTR_BSS_COLOR,
> > +                    request->bss_color))
> > +             return -ENOBUFS;
> > +
> > +     nla_nest_end(msg, ftm);
> > +
> > +     return 0;
> > +}
> > +
> > +static int mac80211_hwsim_send_pmsr_request_peer(struct sk_buff *msg,
> > +                                              struct cfg80211_pmsr_request_peer *request)
> > +{
> > +     void *peer, *chandef, *req, *data;
>
> Same here, why void * when you konw the types being used?
Done.
>
> > +     int err;
> > +
> > +     peer = nla_nest_start(msg, NL80211_PMSR_ATTR_PEERS);
> > +     if (!peer)
> > +             return -ENOBUFS;
> > +
> > +     if (nla_put(msg, NL80211_PMSR_PEER_ATTR_ADDR, ETH_ALEN,
> > +                 request->addr))
> > +             return -ENOBUFS;
> > +
> > +     chandef = nla_nest_start(msg, NL80211_PMSR_PEER_ATTR_CHAN);
> > +     if (!chandef)
> > +             return -ENOBUFS;
> > +
> > +     err = cfg80211_send_chandef(msg, &request->chandef);
> > +     if (err)
> > +             return err;
> > +
> > +     nla_nest_end(msg, chandef);
> > +
> > +     req = nla_nest_start(msg, NL80211_PMSR_PEER_ATTR_REQ);
> > +     if (request->report_ap_tsf &&
> > +         nla_put_flag(msg, NL80211_PMSR_REQ_ATTR_GET_AP_TSF))
> > +             return -ENOBUFS;
> > +
> > +     data = nla_nest_start(msg, NL80211_PMSR_REQ_ATTR_DATA);
> > +     if (!data)
> > +             return -ENOBUFS;
> > +
> > +     mac80211_hwsim_send_pmsr_ftm_request_peer(msg, &request->ftm);
> > +     nla_nest_end(msg, data);
> > +     nla_nest_end(msg, req);
> > +     nla_nest_end(msg, peer);
> > +
> > +     return 0;
> > +}
> > +
> > +static int mac80211_hwsim_send_pmsr_request(struct sk_buff *msg,
> > +                                         struct cfg80211_pmsr_request *request)
> > +{
> > +     int err;
> > +     void *pmsr;
>
> and here (hint larger variables go before smaller ones usually, right?)
Done.
>
> > +
> > +     pmsr = nla_nest_start(msg, NL80211_ATTR_PEER_MEASUREMENTS);
> > +     if (!pmsr)
> > +             return -ENOBUFS;
> > +
> > +     if (nla_put_u32(msg, NL80211_ATTR_TIMEOUT, request->timeout))
> > +             return -ENOBUFS;
> > +
> > +     if (!is_zero_ether_addr(request->mac_addr)) {
> > +             if (nla_put(msg, NL80211_ATTR_MAC, ETH_ALEN, request->mac_addr))
> > +                     return -ENOBUFS;
> > +             if (nla_put(msg, NL80211_ATTR_MAC_MASK, ETH_ALEN,
> > +                         request->mac_addr_mask))
> > +                     return -ENOBUFS;
> > +     }
> > +
> > +     for (int i = 0; i < request->n_peers; i++) {
> > +             err = mac80211_hwsim_send_pmsr_request_peer(msg,
> > +                                                         &request->peers[i]);
>
> Is this line wrap needed?
Unwrapped. Here and other places.

>
> > +             if (err)
> > +                     return err;
> > +     }
> > +
> > +     nla_nest_end(msg, pmsr);
> > +
> > +     return 0;
> > +}
> > +
> > +static int mac80211_hwsim_start_pmsr(struct ieee80211_hw *hw,
> > +                                  struct ieee80211_vif *vif,
> >                                    struct cfg80211_pmsr_request *request)
> >  {
> > -     return -EOPNOTSUPP;
> > +     struct mac80211_hwsim_data *data = hw->priv;
> > +     u32 _portid = READ_ONCE(data->wmediumd);
>
> Why READ_ONCE()?
>
> Shouldn't you only access this after the lock is held?
>
> thanks,
>
> greg k-h

It's intended behavior although I don't know details about the decision behind.
According to the commit a1910f9cad2b4b9cc0d5d326aa65632a23c16088
("mac80211_hwsim: fix wmediumd_pid"),
ACCESS_ONCE() is used instead of locking when synchronization was needed.

--
Jaewan Kim (김재완) | Software Engineer in Google Korea |
jaewan@google.com | +82-10-2781-5078