Message-ID: <CABZjns42zm8Xi-BU0pvT3edNHuJZoh-xshgUk3Oc=nMbxbiY8w@mail.gmail.com>
Date:   Fri, 17 Feb 2023 14:11:38 +0900
From:   Jaewan Kim <jaewan@...gle.com>
To:     Johannes Berg <johannes@...solutions.net>
Cc:     gregkh@...uxfoundation.org, linux-wireless@...r.kernel.org,
        netdev@...r.kernel.org, kernel-team@...roid.com, adelva@...gle.com
Subject: Re: [PATCH v7 1/4] mac80211_hwsim: add PMSR capability support

On Thu, Feb 16, 2023 at 3:01 AM Johannes Berg <johannes@...solutions.net> wrote:
>
> 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.
> >
> > 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.
> >
> > 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 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.
>
> 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] = {
>
> 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.

I'm sorry but could you rephrase what you expect here?
Are you suggesting to define new sets of HWSIM_PMSR_* enums
instead of using existing enums NL80211_PMSR_*?

>
> > +     [NL80211_PMSR_FTM_CAPA_ATTR_ASAP] = { .type = NLA_FLAG },
> > +     [NL80211_PMSR_FTM_CAPA_ATTR_NON_ASAP] = { .type = NLA_FLAG },
> > +     [NL80211_PMSR_FTM_CAPA_ATTR_REQ_LCI] = { .type = NLA_FLAG },
> > +     [NL80211_PMSR_FTM_CAPA_ATTR_REQ_CIVICLOC] = { .type = NLA_FLAG },
> > +     [NL80211_PMSR_FTM_CAPA_ATTR_PREAMBLES] = { .type = NLA_U32 },
> > +     [NL80211_PMSR_FTM_CAPA_ATTR_BANDWIDTHS] = { .type = NLA_U32 },
> > +     [NL80211_PMSR_FTM_CAPA_ATTR_MAX_BURSTS_EXPONENT] = NLA_POLICY_MAX(NLA_U8, 15),
> > +     [NL80211_PMSR_FTM_CAPA_ATTR_MAX_FTMS_PER_BURST] = 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 = 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_info *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_RESPONDER);
> > +
> >
>
> 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 = 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 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) {
>
> same here for both of those comments
>
> >
> > +     if (info->attrs[HWSIM_ATTR_PMSR_SUPPORT]) {
> > +             struct cfg80211_pmsr_capabilities *pmsr_capa =
> > +                     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


Dear Johannes Berg,

First of all, thank you for the review.

I left a question for clarification. I'll send another patchset when I
address your feedback correctly.

BTW,  can I expect you to review my changes for further patchsets?
I sometimes get conflicting opinions (e.g. line limits)
so it would be a great help if you take a look at my changes.


--
Jaewan Kim (김재완) | Software Engineer in Google Korea |
jaewan@...gle.com | +82-10-2781-5078

Powered by blists - more mailing lists