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
