Re: [PATCH v7 2/4] mac80211_hwsim: add PMSR request support via virtio

[Johannes Berg <johannes@sipsolutions.net>]

On Tue, 2023-02-07 at 08:53 +0000, Jaewan Kim wrote:
> 
>  
> +static int mac80211_hwsim_send_pmsr_ftm_request_peer(struct sk_buff *msg,
> +						     struct cfg80211_pmsr_ftm_request_peer *request)

this ...

> +{
> +	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;

and this ... etc ...

also got some really long lines that could easily be broken

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

So this one I think I'll let you do with the export and all, because
that's way nicer than duplicating the code, and it's clearly necessary.

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

I'm not going to complain _too_ much about this, but all this use of
nl80211 attributes better be thoroughly documented in the header file.

> + * @HWSIM_CMD_START_PMSR: start PMSR

That sounds almost like it's a command ("start PMSR") but really it's a
notification/event as far as hwsim is concerned, so please document
that.

> + * @HWSIM_ATTR_PMSR_REQUEST: peer measurement request

and please document the structure of the request that userspace will get
(and how it should respond?)

> +++ 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
> + **/

That last line should just be */

> +int cfg80211_send_chandef(struct sk_buff *msg, const struct cfg80211_chan_def *chandef);

I think it'd be better if you exported it as nl80211_..., since it
really is just a netlink thing, not cfg80211 functionality.

It would also be good, IMHO, to split this part out into a separate
patch saying that e.g. hwsim might use it like you do here, or even that
vendor netlink could use it where needed.

johannes