Search Linux Wireless

Re: [PATCH v9 3/5] mac80211_hwsim: add PMSR request support via virtio

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Tue, Mar 14, 2023 at 4:21 AM Michal Kubiak <michal.kubiak@xxxxxxxxx> wrote:
>
> On Mon, Mar 13, 2023 at 07:53:24AM +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 functionalities for mac80211_hwsim to start PMSR request by
> > passthrough the request to wmediumd via virtio. mac80211_hwsim can't
> > measure RTT for real because mac80211_hwsim the software simulator and
> > packets are sent almost immediately for real. This change expect wmediumd
> > to have all the location information of devices, so passthrough requests
> > to wmediumd.
> >
> > In detail, add new mac80211_hwsim command HWSIM_CMD_ABORT_PMSR. When
> > mac80211_hwsim receives the PMSR start request via
> > ieee80211_ops.start_pmsr, the received cfg80211_pmsr_request is resent to
> > the wmediumd with command HWSIM_CMD_START_PMSR and attribute
> > HWSIM_ATTR_PMSR_REQUEST. The attribute is formatted as the same way as
> > nl80211_pmsr_start() expects.
> >
> > Signed-off-by: Jaewan Kim <jaewan@xxxxxxxxxx>
> > ---
> > V7 -> V8: Exported nl80211_send_chandef directly instead of creating
> >           wrapper.
> > V7: Initial commit (split from previously large patch)
> > ---
> >  drivers/net/wireless/mac80211_hwsim.c | 207 +++++++++++++++++++++++++-
> >  drivers/net/wireless/mac80211_hwsim.h |   6 +
> >  2 files changed, 212 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/net/wireless/mac80211_hwsim.c b/drivers/net/wireless/mac80211_hwsim.c
> > index 65868f28a00f..a692d9c95566 100644
> > --- a/drivers/net/wireless/mac80211_hwsim.c
> > +++ b/drivers/net/wireless/mac80211_hwsim.c
> > @@ -721,6 +721,8 @@ struct mac80211_hwsim_data {
> >
> >       /* only used when pmsr capability is supplied */
> >       struct cfg80211_pmsr_capabilities pmsr_capa;
> > +     struct cfg80211_pmsr_request *pmsr_request;
> > +     struct wireless_dev *pmsr_request_wdev;
> >
> >       struct mac80211_hwsim_link_data link_data[IEEE80211_MLD_MAX_NUM_LINKS];
> >  };
> > @@ -3139,6 +3141,208 @@ static int mac80211_hwsim_change_sta_links(struct ieee80211_hw *hw,
> >       return 0;
> >  }
> >
> > +static int mac80211_hwsim_send_pmsr_ftm_request_peer(struct sk_buff *msg,
> > +                                                  struct cfg80211_pmsr_ftm_request_peer *request)
> > +{
> > +     struct nlattr *ftm;
> > +
> > +     if (!request->requested)
> > +             return -EINVAL;
> > +
> > +     ftm = 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, request->burst_period))
> > +             return -ENOBUFS;
> > +
> > +     if (request->asap && nla_put_flag(msg, NL80211_PMSR_FTM_REQ_ATTR_ASAP))
> > +             return -ENOBUFS;
> > +
> > +     if (request->request_lci && nla_put_flag(msg, NL80211_PMSR_FTM_REQ_ATTR_REQUEST_LCI))
> > +             return -ENOBUFS;
> > +
> > +     if (request->request_civicloc &&
> > +         nla_put_flag(msg, NL80211_PMSR_FTM_REQ_ATTR_REQUEST_CIVICLOC))
> > +             return -ENOBUFS;
> > +
> > +     if (request->trigger_based && nla_put_flag(msg, NL80211_PMSR_FTM_REQ_ATTR_TRIGGER_BASED))
> > +             return -ENOBUFS;
> > +
> > +     if (request->non_trigger_based &&
> > +         nla_put_flag(msg, NL80211_PMSR_FTM_REQ_ATTR_NON_TRIGGER_BASED))
> > +             return -ENOBUFS;
> > +
> > +     if (request->lmr_feedback && nla_put_flag(msg, NL80211_PMSR_FTM_REQ_ATTR_LMR_FEEDBACK))
> > +             return -ENOBUFS;
> > +
> > +     if (nla_put_u8(msg, NL80211_PMSR_FTM_REQ_ATTR_NUM_BURSTS_EXP, request->num_bursts_exp))
> > +             return -ENOBUFS;
> > +
> > +     if (nla_put_u8(msg, NL80211_PMSR_FTM_REQ_ATTR_BURST_DURATION, request->burst_duration))
> > +             return -ENOBUFS;
> > +
> > +     if (nla_put_u8(msg, NL80211_PMSR_FTM_REQ_ATTR_FTMS_PER_BURST, request->ftms_per_burst))
> > +             return -ENOBUFS;
> > +
> > +     if (nla_put_u8(msg, NL80211_PMSR_FTM_REQ_ATTR_NUM_FTMR_RETRIES, request->ftmr_retries))
> > +             return -ENOBUFS;
> > +
> > +     if (nla_put_u8(msg, NL80211_PMSR_FTM_REQ_ATTR_BURST_DURATION, request->burst_duration))
> > +             return -ENOBUFS;
> > +
> > +     if (nla_put_u8(msg, NL80211_PMSR_FTM_REQ_ATTR_BSS_COLOR, request->bss_color))
> > +             return -ENOBUFS;
> > +
> > +     nla_nest_end(msg, ftm);
> > +
> > +     return 0;
> > +}
>
> Lenght of lines in the function above exceeds 80 characters.
>
> > +
> > +static int mac80211_hwsim_send_pmsr_request_peer(struct sk_buff *msg,
> > +                                              struct cfg80211_pmsr_request_peer *request)
> > +{
> > +     struct nlattr *peer, *chandef, *req, *data;
> > +     int err;
> > +
> > +     peer = nla_nest_start(msg, NL80211_PMSR_ATTR_PEERS);
> > +     if (!peer)
> > +             return -ENOBUFS;
> > +
> > +     if (nla_put(msg, NL80211_PMSR_PEER_ATTR_ADDR, ETH_ALEN,
> > +                 request->addr))
> > +             return -ENOBUFS;
> > +
> > +     chandef = nla_nest_start(msg, NL80211_PMSR_PEER_ATTR_CHAN);
> > +     if (!chandef)
> > +             return -ENOBUFS;
> > +
> > +     err = nl80211_send_chandef(msg, &request->chandef);
> > +     if (err)
> > +             return err;
> > +
> > +     nla_nest_end(msg, chandef);
> > +
> > +     req = nla_nest_start(msg, NL80211_PMSR_PEER_ATTR_REQ);
>
> Don't you need to check "if (!req)" as you have done for other pointers
> returned by "nla_put_flag()"?
> Is it by mistake or intentional?

My mistake. Thank you for the review.

>
> > +     if (request->report_ap_tsf && nla_put_flag(msg, NL80211_PMSR_REQ_ATTR_GET_AP_TSF))
>
> Line length above 80 chars.
>
> > +             return -ENOBUFS;
> > +
> > +     data = nla_nest_start(msg, NL80211_PMSR_REQ_ATTR_DATA);
> > +     if (!data)
> > +             return -ENOBUFS;
> > +
> > +     err = mac80211_hwsim_send_pmsr_ftm_request_peer(msg, &request->ftm);
> > +     if (err)
> > +             return err;
> > +
> > +     nla_nest_end(msg, data);
> > +     nla_nest_end(msg, req);
> > +     nla_nest_end(msg, peer);
> > +
> > +     return 0;
> > +}
> > +
> > +static int mac80211_hwsim_send_pmsr_request(struct sk_buff *msg,
> > +                                         struct cfg80211_pmsr_request *request)
> > +{
> > +     int err;
> > +     struct nlattr *pmsr = nla_nest_start(msg, NL80211_ATTR_PEER_MEASUREMENTS);
>
> Please follow the RCT principle.
> Also, I would suggest to split declaration of "pmsr" and assignment because
> "nla_nest_start() is a call that is more in the action part of the code
> than the declaration part.
>
> > +
> > +     if (!pmsr)
> > +             return -ENOBUFS;
> > +
> > +     if (nla_put_u32(msg, NL80211_ATTR_TIMEOUT, request->timeout))
> > +             return -ENOBUFS;
> > +
> > +     if (!is_zero_ether_addr(request->mac_addr)) {
> > +             if (nla_put(msg, NL80211_ATTR_MAC, ETH_ALEN, request->mac_addr))
> > +                     return -ENOBUFS;
> > +             if (nla_put(msg, NL80211_ATTR_MAC_MASK, ETH_ALEN, request->mac_addr_mask))
> > +                     return -ENOBUFS;
> > +     }
> > +
> > +     for (int i = 0; i < request->n_peers; i++) {
> > +             err = mac80211_hwsim_send_pmsr_request_peer(msg, &request->peers[i]);
> > +             if (err)
> > +                     return err;
> > +     }
> > +
> > +     nla_nest_end(msg, pmsr);
> > +
> > +     return 0;
> > +}
> > +
> > +static int mac80211_hwsim_start_pmsr(struct ieee80211_hw *hw,
> > +                                  struct ieee80211_vif *vif,
> > +                                  struct cfg80211_pmsr_request *request)
> > +{
> > +     struct mac80211_hwsim_data *data = hw->priv;
> > +     u32 _portid = READ_ONCE(data->wmediumd);
> > +     int err = 0;
> > +     struct sk_buff *skb = NULL;
> > +     void *msg_head;
> > +     struct nlattr *pmsr;
>
> Please use RCT.
>
> > +
> > +     if (!_portid && !hwsim_virtio_enabled)
> > +             return -EOPNOTSUPP;
> > +
> > +     mutex_lock(&data->mutex);
> > +
> > +     if (data->pmsr_request) {
> > +             err = -EBUSY;
> > +             goto out_err;
> > +     }
> > +
> > +     skb = genlmsg_new(GENLMSG_DEFAULT_SIZE, GFP_KERNEL);
> > +
> > +     if (!skb) {
> > +             err = -ENOMEM;
> > +             goto out_err;
> > +     }
> > +
> > +     msg_head = genlmsg_put(skb, 0, 0, &hwsim_genl_family, 0,
> > +                            HWSIM_CMD_START_PMSR);
> > +
> > +     if (nla_put(skb, HWSIM_ATTR_ADDR_TRANSMITTER,
> > +                 ETH_ALEN, data->addresses[1].addr)) {
> > +             err = -ENOMEM;
> > +             goto out_err;
> > +     }
> > +
> > +     pmsr = nla_nest_start(skb, HWSIM_ATTR_PMSR_REQUEST);
> > +     if (!pmsr) {
> > +             err = -ENOMEM;
> > +             goto out_err;
> > +     }
> > +
> > +     err = mac80211_hwsim_send_pmsr_request(skb, request);
> > +     if (err)
> > +             goto out_err;
> > +
> > +     nla_nest_end(skb, pmsr);
> > +
> > +     genlmsg_end(skb, msg_head);
> > +     if (hwsim_virtio_enabled)
> > +             hwsim_tx_virtio(data, skb);
> > +     else
> > +             hwsim_unicast_netgroup(data, skb, _portid);
> > +
> > +out_err:
> > +     if (err && skb)
> > +             nlmsg_free(skb);
> > +
> > +     if (!err) {
> > +             data->pmsr_request = request;
> > +             data->pmsr_request_wdev = ieee80211_vif_to_wdev(vif);
> > +     }
>
> It looks confusing to have such a check under "out_err" label.
> I would expect error handling only under "out_err".

renamed out_err to out_free, to keep the mutex_unlock() code under the out_err.

> Please improve both checks above:
>  - handle normal cases e.g. "ieee80211_vif_to_wdev()" above the eror
>    label.

done

>  - try to avoid checking if error occured if you are already in the
>    error path.
>
> > +
> > +     mutex_unlock(&data->mutex);
> > +     return err;
> > +}
> > +
> >  #define HWSIM_COMMON_OPS                                     \
> >       .tx = mac80211_hwsim_tx,                                \
> >       .wake_tx_queue = ieee80211_handle_wake_tx_queue,        \
> > @@ -3161,7 +3365,8 @@ static int mac80211_hwsim_change_sta_links(struct ieee80211_hw *hw,
> >       .flush = mac80211_hwsim_flush,                          \
> >       .get_et_sset_count = mac80211_hwsim_get_et_sset_count,  \
> >       .get_et_stats = mac80211_hwsim_get_et_stats,            \
> > -     .get_et_strings = mac80211_hwsim_get_et_strings,
> > +     .get_et_strings = mac80211_hwsim_get_et_strings,        \
> > +     .start_pmsr = mac80211_hwsim_start_pmsr,                \
> >
> >  #define HWSIM_NON_MLO_OPS                                    \
> >       .sta_add = mac80211_hwsim_sta_add,                      \
> > diff --git a/drivers/net/wireless/mac80211_hwsim.h b/drivers/net/wireless/mac80211_hwsim.h
> > index d10fa7f4853b..98e586a56582 100644
> > --- a/drivers/net/wireless/mac80211_hwsim.h
> > +++ b/drivers/net/wireless/mac80211_hwsim.h
> > @@ -81,6 +81,8 @@ enum hwsim_tx_control_flags {
> >   *   to this receiver address for a given station.
> >   * @HWSIM_CMD_DEL_MAC_ADDR: remove the MAC address again, the attributes
> >   *   are the same as to @HWSIM_CMD_ADD_MAC_ADDR.
> > + * @HWSIM_CMD_START_PMSR: request to start peer measurement with the
> > + *   %HWSIM_ATTR_PMSR_REQUEST.
> >   * @__HWSIM_CMD_MAX: enum limit
> >   */
> >  enum {
> > @@ -93,6 +95,7 @@ enum {
> >       HWSIM_CMD_GET_RADIO,
> >       HWSIM_CMD_ADD_MAC_ADDR,
> >       HWSIM_CMD_DEL_MAC_ADDR,
> > +     HWSIM_CMD_START_PMSR,
> >       __HWSIM_CMD_MAX,
> >  };
> >  #define HWSIM_CMD_MAX (_HWSIM_CMD_MAX - 1)
> > @@ -144,6 +147,8 @@ enum {
> >   *   the new radio
> >   * @HWSIM_ATTR_PMSR_SUPPORT: nested attribute used with %HWSIM_CMD_CREATE_RADIO
> >   *   to provide peer measurement capabilities. (nl80211_peer_measurement_attrs)
> > + * @HWSIM_ATTR_PMSR_REQUEST: nested attribute used with %HWSIM_CMD_START_PMSR
> > + *   to provide details about peer measurement request (nl80211_peer_measurement_attrs)
> >   * @__HWSIM_ATTR_MAX: enum limit
> >   */
> >
> > @@ -176,6 +181,7 @@ enum {
> >       HWSIM_ATTR_CIPHER_SUPPORT,
> >       HWSIM_ATTR_MLO_SUPPORT,
> >       HWSIM_ATTR_PMSR_SUPPORT,
> > +     HWSIM_ATTR_PMSR_REQUEST,
> >       __HWSIM_ATTR_MAX,
> >  };
> >  #define HWSIM_ATTR_MAX (__HWSIM_ATTR_MAX - 1)
> > --
> > 2.40.0.rc1.284.g88254d51c5-goog
> >


Applied reverse xmas tree style and renamed goto label.
Line limits are unchanged because 80 is deprecated now.


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




[Index of Archives]     [Linux Host AP]     [ATH6KL]     [Linux Wireless Personal Area Network]     [Linux Bluetooth]     [Wireless Regulations]     [Linux Netdev]     [Kernel Newbies]     [Linux Kernel]     [IDE]     [Git]     [Netfilter]     [Bugtraq]     [Yosemite Hiking]     [MIPS Linux]     [ARM Linux]     [Linux RAID]

  Powered by Linux