MIME-Version: 1.0
References: <20230313075326.3594869-1-jaewan@google.com> <20230313075326.3594869-2-jaewan@google.com>
 <ZA9UrX1I6XXOfnYV@localhost.localdomain>
In-Reply-To: <ZA9UrX1I6XXOfnYV@localhost.localdomain>
From:   Jaewan Kim <jaewan@google.com>
Date:   Wed, 15 Mar 2023 01:36:02 +0900
Message-ID: <CABZjns40eHSBDn3BVg0+Wc6dBOZjvx9ewty4wyOzE0WGrH6Tjw@mail.gmail.com>
Subject: Re: [PATCH v9 1/5] mac80211_hwsim: add PMSR capability support
To:     Michal Kubiak <michal.kubiak@intel.com>
Cc:     gregkh@linuxfoundation.org, johannes@sipsolutions.net,
        linux-wireless@vger.kernel.org, netdev@vger.kernel.org,
        kernel-team@android.com, adelva@google.com
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Tue, Mar 14, 2023 at 1:52=E2=80=AFAM Michal Kubiak <michal.kubiak@intel.=
com> wrote:
>
> On Mon, Mar 13, 2023 at 07:53:22AM +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 fligh=
t
> > 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 wi=
th
> > PMSR capability. The capability is mandatory to accept incoming PMSR
> > request because nl80211_pmsr_start() ignores incoming the request witho=
ut
> > 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 ha=
ve
> > 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@google.com>
>
> Hi,
>
> Just a few style comments and suggestions.
>
> Thanks,
> Michal
>
> > ---
> > V8 -> V9: Changed to consider unknown PMSR type as error.
> > V7 -> V8: Changed not to send pmsr_capa when adding new radio to limit
> >           exporting cfg80211 function to driver.
> > V6 -> V7: Added terms definitions. Removed pr_*() uses.
> > V5 -> V6: Added per change patch history.
> > V4 -> V5: Fixed style for commit messages.
> > V3 -> V4: Added change details for new attribute, and fixed memory leak=
.
> > V1 -> V3: Initial commit (includes resends).
> > ---
> >  drivers/net/wireless/mac80211_hwsim.c | 129 +++++++++++++++++++++++++-
> >  drivers/net/wireless/mac80211_hwsim.h |   3 +
> >  2 files changed, 131 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/net/wireless/mac80211_hwsim.c b/drivers/net/wirele=
ss/mac80211_hwsim.c
> > index 4cc4eaf80b14..65868f28a00f 100644
> > --- a/drivers/net/wireless/mac80211_hwsim.c
> > +++ b/drivers/net/wireless/mac80211_hwsim.c
> > @@ -719,6 +719,9 @@ struct mac80211_hwsim_data {
> >       /* RSSI in rx status of the receiver */
> >       int rx_rssi;
> >
> > +     /* only used when pmsr capability is supplied */
> > +     struct cfg80211_pmsr_capabilities pmsr_capa;
> > +
> >       struct mac80211_hwsim_link_data link_data[IEEE80211_MLD_MAX_NUM_L=
INKS];
> >  };
> >
> > @@ -760,6 +763,34 @@ static const struct genl_multicast_group hwsim_mcg=
rps[] =3D {
> >
> >  /* MAC80211_HWSIM netlink policy */
> >
> > +static const struct nla_policy
> > +hwsim_ftm_capa_policy[NL80211_PMSR_FTM_CAPA_ATTR_MAX + 1] =3D {
> > +     [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_FLA=
G },
> > +     [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_M=
AX(NLA_U8, 15),
> > +     [NL80211_PMSR_FTM_CAPA_ATTR_MAX_FTMS_PER_BURST] =3D NLA_POLICY_MA=
X(NLA_U8, 31),
> > +     [NL80211_PMSR_FTM_CAPA_ATTR_TRIGGER_BASED] =3D { .type =3D NLA_FL=
AG },
> > +     [NL80211_PMSR_FTM_CAPA_ATTR_NON_TRIGGER_BASED] =3D { .type =3D NL=
A_FLAG },
> > +};
> > +
> > +static const struct nla_policy
> > +hwsim_pmsr_capa_type_policy[NL80211_PMSR_TYPE_MAX + 1] =3D {
> > +     [NL80211_PMSR_TYPE_FTM] =3D NLA_POLICY_NESTED(hwsim_ftm_capa_poli=
cy),
> > +};
> > +
> > +static const struct nla_policy
> > +hwsim_pmsr_capa_policy[NL80211_PMSR_ATTR_MAX + 1] =3D {
> > +     [NL80211_PMSR_ATTR_MAX_PEERS] =3D { .type =3D NLA_U32 },
> > +     [NL80211_PMSR_ATTR_REPORT_AP_TSF] =3D { .type =3D NLA_FLAG },
> > +     [NL80211_PMSR_ATTR_RANDOMIZE_MAC_ADDR] =3D { .type =3D NLA_FLAG }=
,
> > +     [NL80211_PMSR_ATTR_TYPE_CAPA] =3D NLA_POLICY_NESTED(hwsim_pmsr_ca=
pa_type_policy),
> > +     [NL80211_PMSR_ATTR_PEERS] =3D { .type =3D NLA_REJECT }, // only f=
or request.
> > +};
> > +
> >  static const struct nla_policy hwsim_genl_policy[HWSIM_ATTR_MAX + 1] =
=3D {
> >       [HWSIM_ATTR_ADDR_RECEIVER] =3D NLA_POLICY_ETH_ADDR_COMPAT,
> >       [HWSIM_ATTR_ADDR_TRANSMITTER] =3D NLA_POLICY_ETH_ADDR_COMPAT,
> > @@ -788,6 +819,7 @@ static const struct nla_policy hwsim_genl_policy[HW=
SIM_ATTR_MAX + 1] =3D {
> >       [HWSIM_ATTR_IFTYPE_SUPPORT] =3D { .type =3D NLA_U32 },
> >       [HWSIM_ATTR_CIPHER_SUPPORT] =3D { .type =3D NLA_BINARY },
> >       [HWSIM_ATTR_MLO_SUPPORT] =3D { .type =3D NLA_FLAG },
> > +     [HWSIM_ATTR_PMSR_SUPPORT] =3D NLA_POLICY_NESTED(hwsim_pmsr_capa_p=
olicy),
> >  };
> >
> >  #if IS_REACHABLE(CONFIG_VIRTIO)
> > @@ -3186,6 +3218,7 @@ struct hwsim_new_radio_params {
> >       u32 *ciphers;
> >       u8 n_ciphers;
> >       bool mlo;
> > +     const struct cfg80211_pmsr_capabilities *pmsr_capa;
> >  };
> >
> >  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;
> >  }
> >
> >  static void hwsim_mcast_new_radio(int id, struct genl_info *info,
> > @@ -4445,6 +4478,7 @@ static int mac80211_hwsim_new_radio(struct genl_i=
nfo *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_R=
ESPONDER);
> >
> >       hw->wiphy->interface_modes =3D param->iftypes;
> >
> > @@ -4606,6 +4640,11 @@ static int mac80211_hwsim_new_radio(struct genl_=
info *info,
> >                                   data->debugfs,
> >                                   data, &hwsim_simulate_radar);
> >
> > +     if (param->pmsr_capa) {
> > +             data->pmsr_capa =3D *param->pmsr_capa;
> > +             hw->wiphy->pmsr_capa =3D &data->pmsr_capa;
> > +     }
> > +
> >       spin_lock_bh(&hwsim_radio_lock);
> >       err =3D rhashtable_insert_fast(&hwsim_radios_rht, &data->rht,
> >                                    hwsim_rht_params);
> > @@ -4715,6 +4754,7 @@ static int mac80211_hwsim_get_radio(struct sk_buf=
f *skb,
> >       param.regd =3D data->regd;
> >       param.channels =3D data->channels;
> >       param.hwname =3D wiphy_name(data->hw->wiphy);
> > +     param.pmsr_capa =3D &data->pmsr_capa;
> >
> >       res =3D append_radio_msg(skb, data->idx, &param);
> >       if (res < 0)
> > @@ -5053,6 +5093,77 @@ static bool hwsim_known_ciphers(const u32 *ciphe=
rs, int n_ciphers)
> >       return true;
> >  }
> >
> > +static int parse_ftm_capa(const struct nlattr *ftm_capa, struct cfg802=
11_pmsr_capabilities *out,
> > +                       struct genl_info *info)
> > +{
> > +     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)=
;
>
> I would suggest to split declaration and assignment here. It breaks the
> RCT principle and it is more likely to overlook "nla_parse_nested" call.
> I think it would improve the readability when we know that the parsing
> function can return an error.

Thank you for the review, but what's the RCT principle?
I've searched Kernel documentation and also googled it but I couldn't
find a good match.
Could you elaborate on the details?
Most of your comments are related to the RCT, so I'd like to
understand what it is.

>
> > +
> > +     if (ret) {
> > +             NL_SET_ERR_MSG_ATTR(info->extack, ftm_capa, "malformed FT=
M capability");
> > +             return -EINVAL;
> > +     }
> > +
> > +     out->ftm.supported =3D 1;
> > +     if (tb[NL80211_PMSR_FTM_CAPA_ATTR_PREAMBLES])
> > +             out->ftm.preambles =3D nla_get_u32(tb[NL80211_PMSR_FTM_CA=
PA_ATTR_PREAMBLES]);
> > +     if (tb[NL80211_PMSR_FTM_CAPA_ATTR_BANDWIDTHS])
> > +             out->ftm.bandwidths =3D nla_get_u32(tb[NL80211_PMSR_FTM_C=
APA_ATTR_BANDWIDTHS]);
> > +     if (tb[NL80211_PMSR_FTM_CAPA_ATTR_MAX_BURSTS_EXPONENT])
> > +             out->ftm.max_bursts_exponent =3D
> > +                     nla_get_u8(tb[NL80211_PMSR_FTM_CAPA_ATTR_MAX_BURS=
TS_EXPONENT]);
> > +     if (tb[NL80211_PMSR_FTM_CAPA_ATTR_MAX_FTMS_PER_BURST])
> > +             out->ftm.max_ftms_per_burst =3D
> > +                     nla_get_u8(tb[NL80211_PMSR_FTM_CAPA_ATTR_MAX_FTMS=
_PER_BURST]);
> > +     out->ftm.asap =3D !!tb[NL80211_PMSR_FTM_CAPA_ATTR_ASAP];
> > +     out->ftm.non_asap =3D !!tb[NL80211_PMSR_FTM_CAPA_ATTR_NON_ASAP];
> > +     out->ftm.request_lci =3D !!tb[NL80211_PMSR_FTM_CAPA_ATTR_REQ_LCI]=
;
> > +     out->ftm.request_civicloc =3D !!tb[NL80211_PMSR_FTM_CAPA_ATTR_REQ=
_CIVICLOC];
> > +     out->ftm.trigger_based =3D !!tb[NL80211_PMSR_FTM_CAPA_ATTR_TRIGGE=
R_BASED];
> > +     out->ftm.non_trigger_based =3D !!tb[NL80211_PMSR_FTM_CAPA_ATTR_NO=
N_TRIGGER_BASED];
> > +
> > +     return 0;
> > +}
> > +
> > +static int parse_pmsr_capa(const struct nlattr *pmsr_capa, struct cfg8=
0211_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);
>
> Ditto.
>
> > +
> > +     if (ret) {
> > +             NL_SET_ERR_MSG_ATTR(info->extack, pmsr_capa, "malformed P=
MSR capability");
> > +             return -EINVAL;
> > +     }
> > +
> > +     if (tb[NL80211_PMSR_ATTR_MAX_PEERS])
> > +             out->max_peers =3D nla_get_u32(tb[NL80211_PMSR_ATTR_MAX_P=
EERS]);
> > +     out->report_ap_tsf =3D !!tb[NL80211_PMSR_ATTR_REPORT_AP_TSF];
> > +     out->randomize_mac_addr =3D !!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_TY=
PE_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:
> > +                     NL_SET_ERR_MSG_ATTR(info->extack, nla, "unsupport=
ed measurement type");
> > +                     return -EINVAL;
> > +             }
> > +     }
> > +     return 0;
> > +}
> > +
> >  static int hwsim_new_radio_nl(struct sk_buff *msg, struct genl_info *i=
nfo)
> >  {
> >       struct hwsim_new_radio_params param =3D { 0 };
> > @@ -5173,8 +5284,24 @@ static int hwsim_new_radio_nl(struct sk_buff *ms=
g, struct genl_info *info)
> >               param.hwname =3D hwname;
> >       }
> >
> > +     if (info->attrs[HWSIM_ATTR_PMSR_SUPPORT]) {
> > +             struct cfg80211_pmsr_capabilities *pmsr_capa =3D
> > +                     kmalloc(sizeof(*pmsr_capa), GFP_KERNEL);
>
> Missing empty line after variable definition.
> BTW, would it not be better to split "pmsr_capa" declaration and
> "kmalloc"? For example:
>
>                 struct cfg80211_pmsr_capabilities *pmsr_capa;
>
>                 pmsr_capa =3D kmalloc(sizeof(*pmsr_capa), GFP_KERNEL);
>                 if (!pmsr_capa) {
>
> I think it would be more readable and you would not have to break the
> line. Also, in the current version it seems more likely that the memory
> allocation will be overlooked.
>
> > +             if (!pmsr_capa) {
> > +                     ret =3D -ENOMEM;
> > +                     goto out_free;
> > +             }
> > +             ret =3D parse_pmsr_capa(info->attrs[HWSIM_ATTR_PMSR_SUPPO=
RT], pmsr_capa, info);
> > +             if (ret)
> > +                     goto out_free;
> > +             param.pmsr_capa =3D pmsr_capa;
> > +     }
> > +
> >       ret =3D mac80211_hwsim_new_radio(info, &param);
> > +
> > +out_free:
> >       kfree(hwname);
> > +     kfree(param.pmsr_capa);
> >       return ret;
> >  }
> >
> > diff --git a/drivers/net/wireless/mac80211_hwsim.h b/drivers/net/wirele=
ss/mac80211_hwsim.h
> > index 527799b2de0f..d10fa7f4853b 100644
> > --- a/drivers/net/wireless/mac80211_hwsim.h
> > +++ b/drivers/net/wireless/mac80211_hwsim.h
> > @@ -142,6 +142,8 @@ enum {
> >   * @HWSIM_ATTR_CIPHER_SUPPORT: u32 array of supported cipher types
> >   * @HWSIM_ATTR_MLO_SUPPORT: claim MLO support (exact parameters TBD) f=
or
> >   *   the new radio
> > + * @HWSIM_ATTR_PMSR_SUPPORT: nested attribute used with %HWSIM_CMD_CRE=
ATE_RADIO
> > + *   to provide peer measurement capabilities. (nl80211_peer_measureme=
nt_attrs)
> >   * @__HWSIM_ATTR_MAX: enum limit
> >   */
> >
> > @@ -173,6 +175,7 @@ enum {
> >       HWSIM_ATTR_IFTYPE_SUPPORT,
> >       HWSIM_ATTR_CIPHER_SUPPORT,
> >       HWSIM_ATTR_MLO_SUPPORT,
> > +     HWSIM_ATTR_PMSR_SUPPORT,
> >       __HWSIM_ATTR_MAX,
> >  };
> >  #define HWSIM_ATTR_MAX (__HWSIM_ATTR_MAX - 1)
> > --
> > 2.40.0.rc1.284.g88254d51c5-goog
> >


--=20
Jaewan Kim (=EA=B9=80=EC=9E=AC=EC=99=84) | Software Engineer in Google Kore=
a |
jaewan@google.com | +82-10-2781-5078