On Mon, Mar 6, 2023 at 4:55 PM Simon Horman <simon.horman@xxxxxxxxxxxx> wrote:
>
> On Thu, Mar 02, 2023 at 04:03:06PM +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 functionality to allow 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.
> >
> > In detail, add new mac80211_hwsim attribute HWSIM_ATTR_PMSR_SUPPORT.
> > HWSIM_ATTR_PMSR_SUPPORT is used to set PMSR capability when creating a new
> > radio. To send extra capability details, HWSIM_ATTR_PMSR_SUPPORT can have
> > nested PMSR capability attributes defined in the nl80211.h. Data format is
> > the same as cfg80211_pmsr_capabilities.
> >
> > If HWSIM_ATTR_PMSR_SUPPORT is specified, mac80211_hwsim builds
> > cfg80211_pmsr_capabilities and sets wiphy.pmsr_capa.
> >
> > Signed-off-by: Jaewan Kim <jaewan@xxxxxxxxxx>
>
> Thanks for your patch, a few comments below.
>
> > diff --git a/drivers/net/wireless/mac80211_hwsim.c b/drivers/net/wireless/mac80211_hwsim.c
> > index 4cc4eaf80b14..79476d55c1ca 100644
> > --- a/drivers/net/wireless/mac80211_hwsim.c
> > +++ b/drivers/net/wireless/mac80211_hwsim.c
>
> ...
>
> > @@ -3186,6 +3218,7 @@ struct hwsim_new_radio_params {
> > u32 *ciphers;
> > u8 n_ciphers;
> > bool mlo;
> > + const struct cfg80211_pmsr_capabilities *pmsr_capa;
>
> nit: not related to this patch,
> but there are lots of holes in hwsim_new_radio_params.
> And, I think that all fields, other than the new pmsr_capa field,
> could fit into one cacheline on x86_64.
>
> I'm unsure if it is worth cleaning up or not.
>
> > };
> >
> > static void hwsim_mcast_config_msg(struct sk_buff *mcast_skb,
> > @@ -3260,7 +3293,7 @@ static int append_radio_msg(struct sk_buff *skb, int id,
> > return ret;
> > }
> >
> > - return 0;
> > + return ret;
>
> This change seems unrelated to the rest of the patch.
I asked to change this in the prior patch v3.
>
> > }
> >
> > static void hwsim_mcast_new_radio(int id, struct genl_info *info,
>
> ...
>
> > +static int parse_pmsr_capa(const struct nlattr *pmsr_capa, struct cfg80211_pmsr_capabilities *out,
> > + struct genl_info *info)
> > +{
> > + struct nlattr *tb[NL80211_PMSR_ATTR_MAX + 1];
> > + struct nlattr *nla;
> > + int size;
> + int ret = nla_parse_nested(tb, NL80211_PMSR_ATTR_MAX, pmsr_capa,
> > + hwsim_pmsr_capa_policy, NULL);
> > +
> > + if (ret) {
> > + NL_SET_ERR_MSG_ATTR(info->extack, pmsr_capa, "malformed PMSR capability");
> > + return -EINVAL;
> > + }
> > +
> > + if (tb[NL80211_PMSR_ATTR_MAX_PEERS])
> > + out->max_peers = nla_get_u32(tb[NL80211_PMSR_ATTR_MAX_PEERS]);
> > + out->report_ap_tsf = !!tb[NL80211_PMSR_ATTR_REPORT_AP_TSF];
> > + out->randomize_mac_addr = !!tb[NL80211_PMSR_ATTR_RANDOMIZE_MAC_ADDR];
> > +
> > + if (!tb[NL80211_PMSR_ATTR_TYPE_CAPA]) {
> > + NL_SET_ERR_MSG_ATTR(info->extack, tb[NL80211_PMSR_ATTR_TYPE_CAPA],
> > + "malformed PMSR type");
> > + return -EINVAL;
> > + }
> > +
> > + nla_for_each_nested(nla, tb[NL80211_PMSR_ATTR_TYPE_CAPA], size) {
> > + switch (nla_type(nla)) {
> > + case NL80211_PMSR_TYPE_FTM:
> > + parse_ftm_capa(nla, out, info);
> > + break;
> > + default:
> > + WARN_ON(1);
>
> WARN_ON doesn't seem right here. I suspect that the following is more fitting.
>
> NL_SET_ERR_MSG_ATTR(...);
> return -EINVAL;
>
Not using NL_SET_ERR_MSG_ATTR(...) is intended to follow the pattern
of net/wireless/pmsr.c,
where unknown type isn't considered as an error.
--
Jaewan Kim (김재완) | Software Engineer in Google Korea |
jaewan@xxxxxxxxxx | +82-10-2781-5078
