MIME-Version: 1.0
References: <20230207085400.2232544-1-jaewan@google.com> <20230207085400.2232544-3-jaewan@google.com>
 <fbe6f8eb820b29f0cc932a63ad84253d0cef93c3.camel@sipsolutions.net>
In-Reply-To: <fbe6f8eb820b29f0cc932a63ad84253d0cef93c3.camel@sipsolutions.net>
From:   Jaewan Kim <jaewan@google.com>
Date:   Fri, 24 Feb 2023 00:38:23 +0900
Message-ID: <CABZjns4r_CJ-paj1FQ-SMFJMQW7rkXnvx5w98zYRgf6UQSnfkw@mail.gmail.com>
Subject: Re: [PATCH v7 2/4] mac80211_hwsim: add PMSR request support via virtio
To:     Johannes Berg <johannes@sipsolutions.net>
Cc:     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 Thu, Feb 16, 2023 at 3:07 AM Johannes Berg <johannes@sipsolutions.net> w=
rote:
>
> On Tue, 2023-02-07 at 08:53 +0000, Jaewan Kim wrote:
> >
> >
> > +static int mac80211_hwsim_send_pmsr_ftm_request_peer(struct sk_buff *m=
sg,
> > +                                                  struct cfg80211_pmsr=
_ftm_request_peer *request)
>
> this ...
>
> > +{
> > +     struct nlattr *ftm;
> > +
> > +     if (!request->requested)
> > +             return -EINVAL;
> > +
> > +     ftm =3D nla_nest_start(msg, NL80211_PMSR_TYPE_FTM);
> > +     if (!ftm)
> > +             return -ENOBUFS;
> > +
> > +     if (nla_put_u32(msg, NL80211_PMSR_FTM_REQ_ATTR_PREAMBLE, request-=
>preamble))
> > +             return -ENOBUFS;
> > +
> > +     if (nla_put_u16(msg, NL80211_PMSR_FTM_REQ_ATTR_BURST_PERIOD, requ=
est->burst_period))
> > +             return -ENOBUFS;
>
> and this ... etc ...
>
> also got some really long lines that could easily be broken
>
> > +     chandef =3D nla_nest_start(msg, NL80211_PMSR_PEER_ATTR_CHAN);
> > +     if (!chandef)
> > +             return -ENOBUFS;
> > +
> > +     err =3D cfg80211_send_chandef(msg, &request->chandef);
> > +     if (err)
> > +             return err;
>
> So this one I think I'll let you do with the export and all, because
> that's way nicer than duplicating the code, and it's clearly necessary.
>
> > +static int mac80211_hwsim_send_pmsr_request(struct sk_buff *msg,
> > +                                         struct cfg80211_pmsr_request =
*request)
> > +{
> > +     int err;
> > +     struct nlattr *pmsr =3D nla_nest_start(msg, NL80211_ATTR_PEER_MEA=
SUREMENTS);
>
> I'm not going to complain _too_ much about this, but all this use of
> nl80211 attributes better be thoroughly documented in the header file.
>
> > + * @HWSIM_CMD_START_PMSR: start PMSR
>
> That sounds almost like it's a command ("start PMSR") but really it's a
> notification/event as far as hwsim is concerned, so please document
> that.
>
> > + * @HWSIM_ATTR_PMSR_REQUEST: peer measurement request
>
> and please document the structure of the request that userspace will get
> (and how it should respond?)
>
> > +++ b/include/net/cfg80211.h
> > @@ -938,6 +938,16 @@ int cfg80211_chandef_dfs_required(struct wiphy *wi=
phy,
> >                                 const struct cfg80211_chan_def *chandef=
,
> >                                 enum nl80211_iftype iftype);
> >
> > +/**
> > + * cfg80211_send_chandef - sends the channel definition.
> > + * @msg: the msg to send channel definition
> > + * @chandef: the channel definition to check
> > + *
> > + * Returns: 0 if sent the channel definition to msg, < 0 on error
> > + **/
>
> That last line should just be */
>
> > +int cfg80211_send_chandef(struct sk_buff *msg, const struct cfg80211_c=
han_def *chandef);
>
> I think it'd be better if you exported it as nl80211_..., since it
> really is just a netlink thing, not cfg80211 functionality.

Sorry about the late response but could you elaborate to me in more
detail on this?
Where header file would be the good place if it's exporting it as nl80211_.=
..?

Here are some places that I've considered but don't seem like a good candid=
ate.

- include/net/cfg80211.h: proposed by current patch with name
cfg80211_send_chandef.
- include/uapi/linux/nl80211.h: only contains enums. doesn't seem like
a good place.

net/wireless/nl80211.h seems like your suggestion, but I can't find
how to include this from mac80211_hwsim.c.

I may need to EXPORT_SYMBOL(nl80211_send_chandef) and also declare it
to the cfg80211.h,
but I'm not sure because I can't find any existing example.

>
> It would also be good, IMHO, to split this part out into a separate
> patch saying that e.g. hwsim might use it like you do here, or even that
> vendor netlink could use it where needed.
>
> johannes


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