Date:   Mon, 13 Mar 2023 20:21:02 +0100
From:   Michal Kubiak <michal.kubiak@intel.com>
To:     Jaewan Kim <jaewan@google.com>
CC:     <gregkh@linuxfoundation.org>, <johannes@sipsolutions.net>,
        <linux-wireless@vger.kernel.org>, <netdev@vger.kernel.org>,
        <kernel-team@android.com>, <adelva@google.com>
Subject: Re: [PATCH v9 3/5] mac80211_hwsim: add PMSR request support via
 virtio
Message-ID: <ZA93nupR04173j+h@localhost.localdomain>
References: <20230313075326.3594869-1-jaewan@google.com>
 <20230313075326.3594869-4-jaewan@google.com>
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <20230313075326.3594869-4-jaewan@google.com>
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?ggg4vYW4n405iClUpX8ks04AVzx9kbxhl2EtMNNpZ7mGTK+WHc4sp8mbvTNl?=
 =?us-ascii?Q?twDAk3VNYTLMfGSNZWsgE+WFoC4rdjvRKHuL36VeZVairBCHO4bp+C6jyVUp?=
 =?us-ascii?Q?jYfUfkSv5bk2SCJYje/k8iiMUcQmNO9ntoO4pslu1RnasDvVdIvRfShenCwJ?=
 =?us-ascii?Q?7QGaiyNCFSNhtIsumDSErwZCOu5IOVawOxiPqCyy/Kt+KSEAlx9oNt4w8ciI?=
 =?us-ascii?Q?NxJa04f972FVrHJacYTdzsdpYtOcvnrXF330yEwXAjkehz4VhF7y60fhiBpz?=
 =?us-ascii?Q?Vz3xkqsNzV5M2civ43PE1bNpxx3AUuPKjkE5zJQvPRrpibr2YlbPeq3dVb7i?=
 =?us-ascii?Q?fV1CI4JjPL01xt13nymNnhHgOVCwaXTGDl2T2I5+xHQMZFPnt2GtEawUhnxZ?=
 =?us-ascii?Q?5qDt17Eww2E/0IC7sY+w+N/MPuuzrqD4ikDhL3c1rz4/1H31LBcQ2wgFCyBb?=
 =?us-ascii?Q?TBjTqm5rbJux0mpZUSpKjW6Jtpw+MfheEmxpssMF+VlnXIMCbuAeQRr6vQR0?=
 =?us-ascii?Q?YUBKLstDMa1j5lqh2KXyaEn74Bjjf0jZicaTDwZB3ZKRsl3NL7QxCWPjvs8P?=
 =?us-ascii?Q?RYxtuS2EIi/9Bh3IFgm/7QXFMqB/p7bjwIzyJB2uAzpDK7sHGgQFH/HkI/xM?=
 =?us-ascii?Q?Ivu6yohg3dsuav7iTv2i3jsOYW3iT1a7HBkHh9LU0iWa6A7Qobs9mDbeGjnE?=
 =?us-ascii?Q?ezUBSESRjnTcLu9DpT4d6duR0uJtHFEJmBsIEd+pV4UFlsWgRFAQMtVs3ZMY?=
 =?us-ascii?Q?7JFA+8mWdkj84545HJ3KJ7DI18KMhNWAP7AIehaIgdmSyy2qXvzSkOUGTD/y?=
 =?us-ascii?Q?Ebcuh5gj12Oay2t78bczvM8/gmvPUxLT3fc6DgwHb5rQq7KC45yJjzZ1w+cp?=
 =?us-ascii?Q?orbX0xxq6F8xhb/QuyVmteQLMoFC72OvozIFeayRd7C9j1C8ENqg32ibelPd?=
 =?us-ascii?Q?XEQMc8QM+YSCexd2PuHCedmqdJY1HJFGtusn1H0m+BGYqCj2421g6h+nrN2+?=
 =?us-ascii?Q?SL1+1wC44Ez4nGVuna7z6HbGvWhV9Z6KLpQAm40RQ8l32bmtvKnsjkG1cvpU?=
 =?us-ascii?Q?Klfktnnv27KIhzpbr10eJdRaLN/tBvqanxLgB4FKUOv6vWDQ3Nv8eerWtCvx?=
 =?us-ascii?Q?UzH0qMjfPM3qfhv77eUcr7uBVKxAAzwAdXuKQOGWf7urpQfTMQ5h9uvtYIgY?=
 =?us-ascii?Q?bM0/1oyLHN5B1yVYq9mkFHikzgGL9yV/GQOqzPBI9kjoqNynmbWy/qcKluJg?=
 =?us-ascii?Q?cIxanIdLXg1cDw/jPMPrntRvjHbv0un8CyoosbiLIV+LFdX6oJje1LpxlRnK?=
 =?us-ascii?Q?5ysGw9aBdfRnjUuWCyNFk/F1A3AymRCXFlb+FlM/umH3uWCuSQZj56+JAHV2?=
 =?us-ascii?Q?7vweJHpPspYOO6irCZz9BwK4NtTnSYV9VoQaimmdhasTnETnQJMk0D8cm3K6?=
 =?us-ascii?Q?T3rXRewbWwpbtzAW96Y23bvaopROvVtx8+BwW++fMTbx6InKE2FMMGWwGKlh?=
 =?us-ascii?Q?jq9lnxlxNjGrb/Ltzom2Xv4dToaW69/N7vt+XJSk1ojKnlffdr1FOSJjd1Mj?=
 =?us-ascii?Q?eYD9pPq/820Tin3e7m1r2OsDBo6DGIp1kOsta6gLKm+TjI9vk+qV51yWQ0+c?=
 =?us-ascii?Q?9g=3D=3D?=
X-MS-Exchange-CrossTenant-Network-Message-Id: f98dcb4c-cc42-4978-87b8-08db23f81e9e
X-MS-Exchange-CrossTenant-AuthSource: DM6PR11MB2937.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 13 Mar 2023 19:21:17.8124
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: TlATQRO4pasq7HDtCcVBlFIvwg0cbT5EaD684i2MOiX6L64c4KO8gEAy/6uIjeJ2iX8xyxb81WLzk5PNzmEITg==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DM4PR11MB8092
X-OriginatorOrg: intel.com
Precedence: bulk
List-ID: <linux-wireless.vger.kernel.org>
X-Mailing-List: linux-wireless@vger.kernel.org

On Mon, Mar 13, 2023 at 07:53:24AM +0000, Jaewan Kim wrote:
> PMSR (a.k.a. peer measurement) is generalized measurement between two
> Wi-Fi devices. And currently FTM (a.k.a. fine time measurement or flight
> time measurement) is the one and only measurement. FTM is measured by
> RTT (a.k.a. round trip time) of packets between two Wi-Fi devices.
> 
> Add necessary functionalities for mac80211_hwsim to start PMSR request by
> passthrough the request to wmediumd via virtio. mac80211_hwsim can't
> measure RTT for real because mac80211_hwsim the software simulator and
> packets are sent almost immediately for real. This change expect wmediumd
> to have all the location information of devices, so passthrough requests
> to wmediumd.
> 
> In detail, add new mac80211_hwsim command HWSIM_CMD_ABORT_PMSR. When
> mac80211_hwsim receives the PMSR start request via
> ieee80211_ops.start_pmsr, the received cfg80211_pmsr_request is resent to
> the wmediumd with command HWSIM_CMD_START_PMSR and attribute
> HWSIM_ATTR_PMSR_REQUEST. The attribute is formatted as the same way as
> nl80211_pmsr_start() expects.
> 
> Signed-off-by: Jaewan Kim <jaewan@google.com>
> ---
> V7 -> V8: Exported nl80211_send_chandef directly instead of creating
>           wrapper.
> V7: Initial commit (split from previously large patch)
> ---
>  drivers/net/wireless/mac80211_hwsim.c | 207 +++++++++++++++++++++++++-
>  drivers/net/wireless/mac80211_hwsim.h |   6 +
>  2 files changed, 212 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/wireless/mac80211_hwsim.c b/drivers/net/wireless/mac80211_hwsim.c
> index 65868f28a00f..a692d9c95566 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];
>  };
> @@ -3139,6 +3141,208 @@ static int mac80211_hwsim_change_sta_links(struct ieee80211_hw *hw,
>  	return 0;
>  }
>  
> +static int mac80211_hwsim_send_pmsr_ftm_request_peer(struct sk_buff *msg,
> +						     struct cfg80211_pmsr_ftm_request_peer *request)
> +{
> +	struct nlattr *ftm;
> +
> +	if (!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_BURST_DURATION, request->burst_duration))
> +		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;
> +}

Lenght of lines in the function above exceeds 80 characters.

> +
> +static int mac80211_hwsim_send_pmsr_request_peer(struct sk_buff *msg,
> +						 struct cfg80211_pmsr_request_peer *request)
> +{
> +	struct nlattr *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 = nl80211_send_chandef(msg, &request->chandef);
> +	if (err)
> +		return err;
> +
> +	nla_nest_end(msg, chandef);
> +
> +	req = nla_nest_start(msg, NL80211_PMSR_PEER_ATTR_REQ);

Don't you need to check "if (!req)" as you have done for other pointers
returned by "nla_put_flag()"?
Is it by mistake or intentional?

> +	if (request->report_ap_tsf && nla_put_flag(msg, NL80211_PMSR_REQ_ATTR_GET_AP_TSF))

Line length above 80 chars.

> +		return -ENOBUFS;
> +
> +	data = nla_nest_start(msg, NL80211_PMSR_REQ_ATTR_DATA);
> +	if (!data)
> +		return -ENOBUFS;
> +
> +	err = mac80211_hwsim_send_pmsr_ftm_request_peer(msg, &request->ftm);
> +	if (err)
> +		return err;
> +
> +	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;
> +	struct nlattr *pmsr = nla_nest_start(msg, NL80211_ATTR_PEER_MEASUREMENTS);

Please follow the RCT principle.
Also, I would suggest to split declaration of "pmsr" and assignment because
"nla_nest_start() is a call that is more in the action part of the code
than the declaration part.

> +
> +	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)
> +{
> +	struct mac80211_hwsim_data *data = hw->priv;
> +	u32 _portid = READ_ONCE(data->wmediumd);
> +	int err = 0;
> +	struct sk_buff *skb = NULL;
> +	void *msg_head;
> +	struct nlattr *pmsr;

Please use RCT.

> +
> +	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);
> +	}

It looks confusing to have such a check under "out_err" label.
I would expect error handling only under "out_err".
Please improve both checks above:
 - handle normal cases e.g. "ieee80211_vif_to_wdev()" above the eror
   label.
 - try to avoid checking if error occured if you are already in the
   error path.

> +
> +	mutex_unlock(&data->mutex);
> +	return err;
> +}
> +
>  #define HWSIM_COMMON_OPS					\
>  	.tx = mac80211_hwsim_tx,				\
>  	.wake_tx_queue = ieee80211_handle_wake_tx_queue,	\
> @@ -3161,7 +3365,8 @@ 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,		\
>  
>  #define HWSIM_NON_MLO_OPS					\
>  	.sta_add = mac80211_hwsim_sta_add,			\
> diff --git a/drivers/net/wireless/mac80211_hwsim.h b/drivers/net/wireless/mac80211_hwsim.h
> index d10fa7f4853b..98e586a56582 100644
> --- a/drivers/net/wireless/mac80211_hwsim.h
> +++ b/drivers/net/wireless/mac80211_hwsim.h
> @@ -81,6 +81,8 @@ 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: request to start peer measurement with the
> + *	%HWSIM_ATTR_PMSR_REQUEST.
>   * @__HWSIM_CMD_MAX: enum limit
>   */
>  enum {
> @@ -93,6 +95,7 @@ enum {
>  	HWSIM_CMD_GET_RADIO,
>  	HWSIM_CMD_ADD_MAC_ADDR,
>  	HWSIM_CMD_DEL_MAC_ADDR,
> +	HWSIM_CMD_START_PMSR,
>  	__HWSIM_CMD_MAX,
>  };
>  #define HWSIM_CMD_MAX (_HWSIM_CMD_MAX - 1)
> @@ -144,6 +147,8 @@ enum {
>   *	the new radio
>   * @HWSIM_ATTR_PMSR_SUPPORT: nested attribute used with %HWSIM_CMD_CREATE_RADIO
>   *	to provide peer measurement capabilities. (nl80211_peer_measurement_attrs)
> + * @HWSIM_ATTR_PMSR_REQUEST: nested attribute used with %HWSIM_CMD_START_PMSR
> + *	to provide details about peer measurement request (nl80211_peer_measurement_attrs)
>   * @__HWSIM_ATTR_MAX: enum limit
>   */
>  
> @@ -176,6 +181,7 @@ enum {
>  	HWSIM_ATTR_CIPHER_SUPPORT,
>  	HWSIM_ATTR_MLO_SUPPORT,
>  	HWSIM_ATTR_PMSR_SUPPORT,
> +	HWSIM_ATTR_PMSR_REQUEST,
>  	__HWSIM_ATTR_MAX,
>  };
>  #define HWSIM_ATTR_MAX (__HWSIM_ATTR_MAX - 1)
> -- 
> 2.40.0.rc1.284.g88254d51c5-goog
>