MIME-Version: 1.0
References: <20230302160310.923349-1-jaewan@google.com> <20230302160310.923349-2-jaewan@google.com>
 <ZAYa4oteaDVPGOLp@corigine.com> <CABZjns6=CM7qYPEDnhP=ZpJqMaA=yWw6vSMPOTRnk87PsYY4yg@mail.gmail.com>
 <51c2b615d848c227edae52cc07df334695c7f856.camel@sipsolutions.net>
In-Reply-To: <51c2b615d848c227edae52cc07df334695c7f856.camel@sipsolutions.net>
From:   Jaewan Kim <jaewan@google.com>
Date:   Mon, 13 Mar 2023 16:54:48 +0900
Message-ID: <CABZjns4feHMHtoR5+AYBRpP-Jo=BMaG4Wa2UF2ouWoKFQeuuBg@mail.gmail.com>
Subject: Re: [PATCH v8 1/5] mac80211_hwsim: add PMSR capability support
To:     Johannes Berg <johannes@sipsolutions.net>
Cc:     Simon Horman <simon.horman@corigine.com>,
        gregkh@linuxfoundation.org, 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 Wed, Mar 8, 2023 at 5:07=E2=80=AFPM Johannes Berg <johannes@sipsolutions=
.net> wrote:
>
> On Wed, 2023-03-08 at 08:00 +0000, Jaewan Kim wrote:
> > >
> > > > +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 =3D 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, "malform=
ed PMSR capability");
> > > > +             return -EINVAL;
> > > > +     }
> > > > +
> > > > +     if (tb[NL80211_PMSR_ATTR_MAX_PEERS])
> > > > +             out->max_peers =3D nla_get_u32(tb[NL80211_PMSR_ATTR_M=
AX_PEERS]);
> > > > +     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_ATT=
R_TYPE_CAPA],
> > > > +                                 "malformed PMSR type");
> > > > +             return -EINVAL;
> > > > +     }
> > > > +
> > > > +     nla_for_each_nested(nla, tb[NL80211_PMSR_ATTR_TYPE_CAPA], siz=
e) {
> > > > +             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.
>
> NL80211_PMSR_ATTR_TYPE_CAPA is normally NLA_REJECT (not sent by
> userspace), you just use it here for the hwsim capabilities which makes
> sense, but it feels better to just reject unknown types.
>
> If you're thinking of actually using it we still have in pmsr.c this
> code:
>
>         nla_for_each_nested(treq, req[NL80211_PMSR_REQ_ATTR_DATA], rem) {
>                 switch (nla_type(treq)) {
>                 case NL80211_PMSR_TYPE_FTM:
>                         err =3D pmsr_parse_ftm(rdev, treq, out, info);
>                         break;
>                 default:
>                         NL_SET_ERR_MSG_ATTR(info->extack, treq,
>                                             "unsupported measurement type=
");
>                         err =3D -EINVAL;
>                 }
>
>
> johannes

Done, and uploaded the next patchset for this. Thank you for the review.

--=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