Message-ID: <6ad6708b124b50ff9ea64771b31d09e9168bfa17.camel@sipsolutions.net>
Subject: Re: [PATCH v7 1/4] mac80211_hwsim: add PMSR capability support
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:01:11 +0100
In-Reply-To: <20230207085400.2232544-2-jaewan@google.com>
References: <20230207085400.2232544-1-jaewan@google.com>
         <20230207085400.2232544-2-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:
> 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.
>=20
> This change allows mac80211_hwsim to be configured with PMSR capability.
> The capability is mandatory to accept incoming PMSR request because
> nl80211_pmsr_start() ignores incoming the request without the PMSR
> capability.
>=20
> This change adds HWSIM_ATTR_PMSR_SUPPORT, which is used to set PMSR
> capability when creating a new radio. To send extra details,
> HWSIM_ATTR_PMSR_SUPPORT can have nested PMSR capability attributes define=
d
> in the nl80211.h. Data format is the same as cfg80211_pmsr_capabilities.
>=20
> If HWSIM_ATTR_PMSR_SUPPORT is specified, mac80211_hwsim builds
> cfg80211_pmsr_capabilities and sets wiphy.pmsr_capa.

I feel kind of bad for even still commenting on v7 already ... :)

Sorry I didn't pay much attention to this before.


> +static const struct nla_policy
> +hwsim_ftm_capa_policy[NL80211_PMSR_FTM_CAPA_ATTR_MAX + 1] =3D {

This feels a bit iffy to have here, but I guess it's better that
defining new attributes for all this over and over again.

> +	[NL80211_PMSR_FTM_CAPA_ATTR_ASAP] =3D { .type =3D NLA_FLAG },
> +	[NL80211_PMSR_FTM_CAPA_ATTR_NON_ASAP] =3D { .type =3D NLA_FLAG },
> +	[NL80211_PMSR_FTM_CAPA_ATTR_REQ_LCI] =3D { .type =3D NLA_FLAG },
> +	[NL80211_PMSR_FTM_CAPA_ATTR_REQ_CIVICLOC] =3D { .type =3D NLA_FLAG },
> +	[NL80211_PMSR_FTM_CAPA_ATTR_PREAMBLES] =3D { .type =3D NLA_U32 },
> +	[NL80211_PMSR_FTM_CAPA_ATTR_BANDWIDTHS] =3D { .type =3D NLA_U32 },
> +	[NL80211_PMSR_FTM_CAPA_ATTR_MAX_BURSTS_EXPONENT] =3D NLA_POLICY_MAX(NLA=
_U8, 15),
> +	[NL80211_PMSR_FTM_CAPA_ATTR_MAX_FTMS_PER_BURST] =3D NLA_POLICY_MAX(NLA_=
U8, 31),

Could add some line-breaks where it's easy to stay below 80 columns. Not
a hard rule, but still reads nicer.

> +	if (param->pmsr_capa)
> +		ret =3D cfg80211_send_pmsr_capa(param->pmsr_capa, skb);

I'm not a fan of exporting this function to drivers - it feels odd. It's
also not really needed, since once the new radio exists the user can
query it through cfg80211. I'd just remove this part, along with the
changes in include/ and net/

> @@ -4445,6 +4481,8 @@ static int mac80211_hwsim_new_radio(struct genl_inf=
o *info,
>  			      NL80211_EXT_FEATURE_MULTICAST_REGISTRATIONS);
>  	wiphy_ext_feature_set(hw->wiphy,
>  			      NL80211_EXT_FEATURE_BEACON_RATE_LEGACY);
> +	wiphy_ext_feature_set(hw->wiphy, NL80211_EXT_FEATURE_ENABLE_FTM_RESPOND=
ER);
> +
>=20

no need for the extra blank line.

> +static int parse_ftm_capa(const struct nlattr *ftm_capa, struct cfg80211=
_pmsr_capabilities *out,
> +			  struct genl_info *info)

That line also got really long, unnecessarily.

> +{
> +	struct nlattr *tb[NL80211_PMSR_FTM_CAPA_ATTR_MAX + 1];
> +	int ret =3D nla_parse_nested(tb, NL80211_PMSR_FTM_CAPA_ATTR_MAX,
> +				   ftm_capa, hwsim_ftm_capa_policy, NULL);

should have a blank line here I guess.

> +static int parse_pmsr_capa(const struct nlattr *pmsr_capa, struct cfg802=
11_pmsr_capabilities *out,
> +			   struct genl_info *info)
> +{
> +	struct nlattr *tb[NL80211_PMSR_ATTR_MAX + 1];
> +	struct nlattr *nla;
> +	int size;
> +	int ret =3D nla_parse_nested(tb, NL80211_PMSR_ATTR_MAX, pmsr_capa,
> +				   hwsim_pmsr_capa_policy, NULL);
> +	if (ret) {

same here for both of those comments

> =20
> +	if (info->attrs[HWSIM_ATTR_PMSR_SUPPORT]) {
> +		struct cfg80211_pmsr_capabilities *pmsr_capa =3D
> +			kmalloc(sizeof(struct cfg80211_pmsr_capabilities), GFP_KERNEL);

sizeof(*pmsr_capa), also makes that line a lot shorter

> + * @HWSIM_ATTR_PMSR_SUPPORT: claim peer measurement support

This should probably explain that it's nested, and what should be inside
of it.

johannes