Message-ID: <fbe6f8eb820b29f0cc932a63ad84253d0cef93c3.camel@sipsolutions.net>
Subject: Re: [PATCH v7 2/4] mac80211_hwsim: add PMSR request support via
 virtio
From:   Johannes Berg <johannes@sipsolutions.net>
To:     Jaewan Kim <jaewan@google.com>, gregkh@linuxfoundation.org,
        linux-wireless@vger.kernel.org, netdev@vger.kernel.org
Cc:     kernel-team@android.com, adelva@google.com
Date:   Wed, 15 Feb 2023 19:07:47 +0100
In-Reply-To: <20230207085400.2232544-3-jaewan@google.com>
References: <20230207085400.2232544-1-jaewan@google.com>
         <20230207085400.2232544-3-jaewan@google.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
User-Agent: Evolution 3.46.3 (3.46.3-1.fc37) 
MIME-Version: 1.0
Precedence: bulk

On Tue, 2023-02-07 at 08:53 +0000, Jaewan Kim wrote:
>=20
> =20
> +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 =3D nla_nest_start(msg, NL80211_PMSR_TYPE_FTM);
> +	if (!ftm)
> +		return -ENOBUFS;
> +
> +	if (nla_put_u32(msg, NL80211_PMSR_FTM_REQ_ATTR_PREAMBLE, request->pream=
ble))
> +		return -ENOBUFS;
> +
> +	if (nla_put_u16(msg, NL80211_PMSR_FTM_REQ_ATTR_BURST_PERIOD, request->b=
urst_period))
> +		return -ENOBUFS;

and this ... etc ...

also got some really long lines that could easily be broken

> +	chandef =3D nla_nest_start(msg, NL80211_PMSR_PEER_ATTR_CHAN);
> +	if (!chandef)
> +		return -ENOBUFS;
> +
> +	err =3D 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 =3D nla_nest_start(msg, NL80211_ATTR_PEER_MEASUREME=
NTS);

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 *wiph=
y,
>  				  const struct cfg80211_chan_def *chandef,
>  				  enum nl80211_iftype iftype);
> =20
> +/**
> + * 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_cha=
n_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