[v9,5/5] mac80211_hwsim: add PMSR report support via virtio

PMSR (a.k.a. peer measurement) is generalized measurement between two
devices with Wi-Fi support. And currently FTM (a.k.a. fine time measurement
or flight time measurement) is the one and only measurement.

Add the necessary functionality to allow mac80211_hwsim to report PMSR
result. The result would come from the wmediumd, where other Wi-Fi
devices' information are kept. mac80211_hwsim only need to deliver the
result to the userspace.

In detail, add new mac80211_hwsim attributes HWSIM_CMD_REPORT_PMSR, and
HWSIM_ATTR_PMSR_RESULT. When mac80211_hwsim receives the PMSR result with
command HWSIM_CMD_REPORT_PMSR and detail with attribute
HWSIM_ATTR_PMSR_RESULT, received data is parsed to cfg80211_pmsr_result and
resent to the userspace by cfg80211_pmsr_report().

To help receive the details of PMSR result, hwsim_rate_info_attributes is
added to receive rate_info without complex bitrate calculation. (i.e. send
rate_info without adding inverse of nl80211_put_sta_rate()).

Signed-off-by: Jaewan Kim <jaewan@google.com>
---
V7 -> V8: Changed to specify calculated last HWSIM_CMD for resv_start_op
          instead of __HWSIM_CMD_MAX for adding new CMD more explicit.
V7: Initial commit (split from previously large patch)
---
 drivers/net/wireless/mac80211_hwsim.c | 379 +++++++++++++++++++++++++-
 drivers/net/wireless/mac80211_hwsim.h |  51 +++-
 2 files changed, 420 insertions(+), 10 deletions(-)

On Tue, Mar 14, 2023 at 5:26 AM Michal Kubiak <michal.kubiak@intel.com> wrote:
>
> On Mon, Mar 13, 2023 at 07:53:26AM +0000, Jaewan Kim wrote:
> > PMSR (a.k.a. peer measurement) is generalized measurement between two
> > devices with Wi-Fi support. And currently FTM (a.k.a. fine time measurement
> > or flight time measurement) is the one and only measurement.
> >
> > Add the necessary functionality to allow mac80211_hwsim to report PMSR
> > result. The result would come from the wmediumd, where other Wi-Fi
> > devices' information are kept. mac80211_hwsim only need to deliver the
> > result to the userspace.
> >
> > In detail, add new mac80211_hwsim attributes HWSIM_CMD_REPORT_PMSR, and
> > HWSIM_ATTR_PMSR_RESULT. When mac80211_hwsim receives the PMSR result with
> > command HWSIM_CMD_REPORT_PMSR and detail with attribute
> > HWSIM_ATTR_PMSR_RESULT, received data is parsed to cfg80211_pmsr_result and
> > resent to the userspace by cfg80211_pmsr_report().
> >
> > To help receive the details of PMSR result, hwsim_rate_info_attributes is
> > added to receive rate_info without complex bitrate calculation. (i.e. send
> > rate_info without adding inverse of nl80211_put_sta_rate()).
> >
> > Signed-off-by: Jaewan Kim <jaewan@google.com>
> > ---
> > V7 -> V8: Changed to specify calculated last HWSIM_CMD for resv_start_op
> >           instead of __HWSIM_CMD_MAX for adding new CMD more explicit.
> > V7: Initial commit (split from previously large patch)
> > ---
> >  drivers/net/wireless/mac80211_hwsim.c | 379 +++++++++++++++++++++++++-
> >  drivers/net/wireless/mac80211_hwsim.h |  51 +++-
> >  2 files changed, 420 insertions(+), 10 deletions(-)
> >
>
> General comment: there are many lines exceeding 80 characters (the limit
> for net).
> The rest of my comments - inline.

We can now using 100 columns
because 80 character limit is deprecated

Here's previous discussion thread:
https://patchwork.kernel.org/project/linux-wireless/patch/20230207085400.2232544-2-jaewan@google.com/#25217046

>
> Thanks,
> Michal
>
> > diff --git a/drivers/net/wireless/mac80211_hwsim.c b/drivers/net/wireless/mac80211_hwsim.c
> > index 8f699dfab77a..d1218c1efba4 100644
> > --- a/drivers/net/wireless/mac80211_hwsim.c
> > +++ b/drivers/net/wireless/mac80211_hwsim.c
> > @@ -752,6 +752,11 @@ struct hwsim_radiotap_ack_hdr {
> >       __le16 rt_chbitmask;
> >  } __packed;
> >
> > +static struct mac80211_hwsim_data *get_hwsim_data_ref_from_addr(const u8 *addr)
> > +{
> > +     return rhashtable_lookup_fast(&hwsim_radios_rht, addr, hwsim_rht_params);
> > +}
> > +
> >  /* MAC80211_HWSIM netlink family */
> >  static struct genl_family hwsim_genl_family;
> >
> > @@ -765,6 +770,76 @@ static const struct genl_multicast_group hwsim_mcgrps[] = {
> >
> >  /* MAC80211_HWSIM netlink policy */
> >
> > +static const struct nla_policy
> > +hwsim_rate_info_policy[HWSIM_RATE_INFO_ATTR_MAX + 1] = {
> > +     [HWSIM_RATE_INFO_ATTR_FLAGS] = { .type = NLA_U8 },
> > +     [HWSIM_RATE_INFO_ATTR_MCS] = { .type = NLA_U8 },
> > +     [HWSIM_RATE_INFO_ATTR_LEGACY] = { .type = NLA_U16 },
> > +     [HWSIM_RATE_INFO_ATTR_NSS] = { .type = NLA_U8 },
> > +     [HWSIM_RATE_INFO_ATTR_BW] = { .type = NLA_U8 },
> > +     [HWSIM_RATE_INFO_ATTR_HE_GI] = { .type = NLA_U8 },
> > +     [HWSIM_RATE_INFO_ATTR_HE_DCM] = { .type = NLA_U8 },
> > +     [HWSIM_RATE_INFO_ATTR_HE_RU_ALLOC] = { .type = NLA_U8 },
> > +     [HWSIM_RATE_INFO_ATTR_N_BOUNDED_CH] = { .type = NLA_U8 },
> > +     [HWSIM_RATE_INFO_ATTR_EHT_GI] = { .type = NLA_U8 },
> > +     [HWSIM_RATE_INFO_ATTR_EHT_RU_ALLOC] = { .type = NLA_U8 },
> > +};
> > +
> > +static const struct nla_policy
> > +hwsim_ftm_result_policy[NL80211_PMSR_FTM_RESP_ATTR_MAX + 1] = {
> > +     [NL80211_PMSR_FTM_RESP_ATTR_FAIL_REASON] = { .type = NLA_U32 },
> > +     [NL80211_PMSR_FTM_RESP_ATTR_BURST_INDEX] = { .type = NLA_U16 },
> > +     [NL80211_PMSR_FTM_RESP_ATTR_NUM_FTMR_ATTEMPTS] = { .type = NLA_U32 },
> > +     [NL80211_PMSR_FTM_RESP_ATTR_NUM_FTMR_SUCCESSES] = { .type = NLA_U32 },
> > +     [NL80211_PMSR_FTM_RESP_ATTR_BUSY_RETRY_TIME] = { .type = NLA_U8 },
> > +     [NL80211_PMSR_FTM_RESP_ATTR_NUM_BURSTS_EXP] = { .type = NLA_U8 },
> > +     [NL80211_PMSR_FTM_RESP_ATTR_BURST_DURATION] = { .type = NLA_U8 },
> > +     [NL80211_PMSR_FTM_RESP_ATTR_FTMS_PER_BURST] = { .type = NLA_U8 },
> > +     [NL80211_PMSR_FTM_RESP_ATTR_RSSI_AVG] = { .type = NLA_U32 },
> > +     [NL80211_PMSR_FTM_RESP_ATTR_RSSI_SPREAD] = { .type = NLA_U32 },
> > +     [NL80211_PMSR_FTM_RESP_ATTR_TX_RATE] = NLA_POLICY_NESTED(hwsim_rate_info_policy),
> > +     [NL80211_PMSR_FTM_RESP_ATTR_RX_RATE] = NLA_POLICY_NESTED(hwsim_rate_info_policy),
> > +     [NL80211_PMSR_FTM_RESP_ATTR_RTT_AVG] = { .type = NLA_U64 },
> > +     [NL80211_PMSR_FTM_RESP_ATTR_RTT_VARIANCE] = { .type = NLA_U64 },
> > +     [NL80211_PMSR_FTM_RESP_ATTR_RTT_SPREAD] = { .type = NLA_U64 },
> > +     [NL80211_PMSR_FTM_RESP_ATTR_DIST_AVG] = { .type = NLA_U64 },
> > +     [NL80211_PMSR_FTM_RESP_ATTR_DIST_VARIANCE] = { .type = NLA_U64 },
> > +     [NL80211_PMSR_FTM_RESP_ATTR_DIST_SPREAD] = { .type = NLA_U64 },
> > +     [NL80211_PMSR_FTM_RESP_ATTR_LCI] = { .type = NLA_STRING },
> > +     [NL80211_PMSR_FTM_RESP_ATTR_CIVICLOC] = { .type = NLA_STRING },
> > +};
> > +
> > +static const struct nla_policy
> > +hwsim_pmsr_resp_type_policy[NL80211_PMSR_TYPE_MAX + 1] = {
> > +     [NL80211_PMSR_TYPE_FTM] = NLA_POLICY_NESTED(hwsim_ftm_result_policy),
> > +};
> > +
> > +static const struct nla_policy
> > +hwsim_pmsr_resp_policy[NL80211_PMSR_RESP_ATTR_MAX + 1] = {
> > +     [NL80211_PMSR_RESP_ATTR_STATUS] = { .type = NLA_U32 },
> > +     [NL80211_PMSR_RESP_ATTR_HOST_TIME] = { .type = NLA_U64 },
> > +     [NL80211_PMSR_RESP_ATTR_AP_TSF] = { .type = NLA_U64 },
> > +     [NL80211_PMSR_RESP_ATTR_FINAL] = { .type = NLA_FLAG },
> > +     [NL80211_PMSR_RESP_ATTR_DATA] = NLA_POLICY_NESTED(hwsim_pmsr_resp_type_policy),
> > +};
> > +
> > +static const struct nla_policy
> > +hwsim_pmsr_peer_result_policy[NL80211_PMSR_PEER_ATTR_MAX + 1] = {
> > +     [NL80211_PMSR_PEER_ATTR_ADDR] = NLA_POLICY_ETH_ADDR_COMPAT,
> > +     [NL80211_PMSR_PEER_ATTR_CHAN] = { .type = NLA_REJECT },
> > +     [NL80211_PMSR_PEER_ATTR_REQ] = { .type = NLA_REJECT },
> > +     [NL80211_PMSR_PEER_ATTR_RESP] = NLA_POLICY_NESTED(hwsim_pmsr_resp_policy),
> > +};
> > +
> > +static const struct nla_policy
> > +hwsim_pmsr_peers_result_policy[NL80211_PMSR_ATTR_MAX + 1] = {
> > +     [NL80211_PMSR_ATTR_MAX_PEERS] = { .type = NLA_REJECT },
> > +     [NL80211_PMSR_ATTR_REPORT_AP_TSF] = { .type = NLA_REJECT },
> > +     [NL80211_PMSR_ATTR_RANDOMIZE_MAC_ADDR] = { .type = NLA_REJECT },
> > +     [NL80211_PMSR_ATTR_TYPE_CAPA] = { .type = NLA_REJECT },
> > +     [NL80211_PMSR_ATTR_PEERS] = NLA_POLICY_NESTED_ARRAY(hwsim_pmsr_peer_result_policy),
> > +};
> > +
> >  static const struct nla_policy
> >  hwsim_ftm_capa_policy[NL80211_PMSR_FTM_CAPA_ATTR_MAX + 1] = {
> >       [NL80211_PMSR_FTM_CAPA_ATTR_ASAP] = { .type = NLA_FLAG },
> > @@ -822,6 +897,7 @@ static const struct nla_policy hwsim_genl_policy[HWSIM_ATTR_MAX + 1] = {
> >       [HWSIM_ATTR_CIPHER_SUPPORT] = { .type = NLA_BINARY },
> >       [HWSIM_ATTR_MLO_SUPPORT] = { .type = NLA_FLAG },
> >       [HWSIM_ATTR_PMSR_SUPPORT] = NLA_POLICY_NESTED(hwsim_pmsr_capa_policy),
> > +     [HWSIM_ATTR_PMSR_RESULT] = NLA_POLICY_NESTED(hwsim_pmsr_peers_result_policy),
> >  };
> >
> >  #if IS_REACHABLE(CONFIG_VIRTIO)
> > @@ -3403,6 +3479,292 @@ static void mac80211_hwsim_abort_pmsr(struct ieee80211_hw *hw,
> >       mutex_unlock(&data->mutex);
> >  }
> >
> > +static int mac80211_hwsim_parse_rate_info(struct nlattr *rateattr,
> > +                                       struct rate_info *rate_info,
> > +                                       struct genl_info *info)
> > +{
> > +     struct nlattr *tb[HWSIM_RATE_INFO_ATTR_MAX + 1];
> > +     int ret;
> > +
> > +     ret = nla_parse_nested(tb, HWSIM_RATE_INFO_ATTR_MAX,
> > +                            rateattr, hwsim_rate_info_policy, info->extack);
> > +     if (ret)
> > +             return ret;
> > +
> > +     if (tb[HWSIM_RATE_INFO_ATTR_FLAGS])
> > +             rate_info->flags = nla_get_u8(tb[HWSIM_RATE_INFO_ATTR_FLAGS]);
> > +
> > +     if (tb[HWSIM_RATE_INFO_ATTR_MCS])
> > +             rate_info->mcs = nla_get_u8(tb[HWSIM_RATE_INFO_ATTR_MCS]);
> > +
> > +     if (tb[HWSIM_RATE_INFO_ATTR_LEGACY])
> > +             rate_info->legacy = nla_get_u16(tb[HWSIM_RATE_INFO_ATTR_LEGACY]);
> > +
> > +     if (tb[HWSIM_RATE_INFO_ATTR_NSS])
> > +             rate_info->nss = nla_get_u8(tb[HWSIM_RATE_INFO_ATTR_NSS]);
> > +
> > +     if (tb[HWSIM_RATE_INFO_ATTR_BW])
> > +             rate_info->bw = nla_get_u8(tb[HWSIM_RATE_INFO_ATTR_BW]);
> > +
> > +     if (tb[HWSIM_RATE_INFO_ATTR_HE_GI])
> > +             rate_info->he_gi = nla_get_u8(tb[HWSIM_RATE_INFO_ATTR_HE_GI]);
> > +
> > +     if (tb[HWSIM_RATE_INFO_ATTR_HE_DCM])
> > +             rate_info->he_dcm = nla_get_u8(tb[HWSIM_RATE_INFO_ATTR_HE_DCM]);
> > +
> > +     if (tb[HWSIM_RATE_INFO_ATTR_HE_RU_ALLOC])
> > +             rate_info->he_ru_alloc =
> > +                     nla_get_u8(tb[HWSIM_RATE_INFO_ATTR_HE_RU_ALLOC]);
> > +
> > +     if (tb[HWSIM_RATE_INFO_ATTR_N_BOUNDED_CH])
> > +             rate_info->n_bonded_ch = nla_get_u8(tb[HWSIM_RATE_INFO_ATTR_N_BOUNDED_CH]);
> > +
> > +     if (tb[HWSIM_RATE_INFO_ATTR_EHT_GI])
> > +             rate_info->eht_gi = nla_get_u8(tb[HWSIM_RATE_INFO_ATTR_EHT_GI]);
> > +
> > +     if (tb[HWSIM_RATE_INFO_ATTR_EHT_RU_ALLOC])
> > +             rate_info->eht_ru_alloc = nla_get_u8(tb[HWSIM_RATE_INFO_ATTR_EHT_RU_ALLOC]);
> > +
> > +     return 0;
> > +}
>
> Lines in the function above often exceed 80 chars.
>
> > +
> > +static int mac80211_hwsim_parse_ftm_result(struct nlattr *ftm,
> > +                                        struct cfg80211_pmsr_ftm_result *result,
> > +                                        struct genl_info *info)
> > +{
> > +     struct nlattr *tb[NL80211_PMSR_FTM_RESP_ATTR_MAX + 1];
> > +     int ret;
> > +
> > +     ret = nla_parse_nested(tb, NL80211_PMSR_FTM_RESP_ATTR_MAX,
> > +                            ftm, hwsim_ftm_result_policy, info->extack);
> > +     if (ret)
> > +             return ret;
> > +
> > +     if (tb[NL80211_PMSR_FTM_RESP_ATTR_FAIL_REASON])
> > +             result->failure_reason = nla_get_u32(tb[NL80211_PMSR_FTM_RESP_ATTR_FAIL_REASON]);
> > +
> > +     if (tb[NL80211_PMSR_FTM_RESP_ATTR_BURST_INDEX])
> > +             result->burst_index = nla_get_u16(tb[NL80211_PMSR_FTM_RESP_ATTR_BURST_INDEX]);
> > +
> > +     if (tb[NL80211_PMSR_FTM_RESP_ATTR_NUM_FTMR_ATTEMPTS]) {
> > +             result->num_ftmr_attempts_valid = 1;
> > +             result->num_ftmr_attempts =
> > +                     nla_get_u32(tb[NL80211_PMSR_FTM_RESP_ATTR_NUM_FTMR_ATTEMPTS]);
> > +     }
> > +
> > +     if (tb[NL80211_PMSR_FTM_RESP_ATTR_NUM_FTMR_SUCCESSES]) {
> > +             result->num_ftmr_successes_valid = 1;
> > +             result->num_ftmr_successes =
> > +                     nla_get_u32(tb[NL80211_PMSR_FTM_RESP_ATTR_NUM_FTMR_SUCCESSES]);
> > +     }
> > +
> > +     if (tb[NL80211_PMSR_FTM_RESP_ATTR_BUSY_RETRY_TIME])
> > +             result->busy_retry_time =
> > +                     nla_get_u8(tb[NL80211_PMSR_FTM_RESP_ATTR_BUSY_RETRY_TIME]);
> > +
> > +     if (tb[NL80211_PMSR_FTM_RESP_ATTR_NUM_BURSTS_EXP])
> > +             result->num_bursts_exp = nla_get_u8(tb[NL80211_PMSR_FTM_RESP_ATTR_NUM_BURSTS_EXP]);
> > +
> > +     if (tb[NL80211_PMSR_FTM_RESP_ATTR_BURST_DURATION])
> > +             result->burst_duration = nla_get_u8(tb[NL80211_PMSR_FTM_RESP_ATTR_BURST_DURATION]);
> > +
> > +     if (tb[NL80211_PMSR_FTM_RESP_ATTR_FTMS_PER_BURST])
> > +             result->ftms_per_burst = nla_get_u8(tb[NL80211_PMSR_FTM_RESP_ATTR_FTMS_PER_BURST]);
> > +
> > +     if (tb[NL80211_PMSR_FTM_RESP_ATTR_RSSI_AVG]) {
> > +             result->rssi_avg_valid = 1;
> > +             result->rssi_avg = nla_get_s32(tb[NL80211_PMSR_FTM_RESP_ATTR_RSSI_AVG]);
> > +     }
> > +     if (tb[NL80211_PMSR_FTM_RESP_ATTR_RSSI_SPREAD]) {
> > +             result->rssi_spread_valid = 1;
> > +             result->rssi_spread =
> > +                     nla_get_s32(tb[NL80211_PMSR_FTM_RESP_ATTR_RSSI_SPREAD]);
> > +     }
> > +
> > +     if (tb[NL80211_PMSR_FTM_RESP_ATTR_TX_RATE]) {
> > +             result->tx_rate_valid = 1;
> > +             ret = mac80211_hwsim_parse_rate_info(tb[NL80211_PMSR_FTM_RESP_ATTR_TX_RATE],
> > +                                                  &result->tx_rate, info);
> > +             if (ret)
> > +                     return ret;
> > +     }
> > +
> > +     if (tb[NL80211_PMSR_FTM_RESP_ATTR_RX_RATE]) {
> > +             result->rx_rate_valid = 1;
> > +             ret = mac80211_hwsim_parse_rate_info(tb[NL80211_PMSR_FTM_RESP_ATTR_RX_RATE],
> > +                                                  &result->rx_rate, info);
> > +             if (ret)
> > +                     return ret;
> > +     }
> > +
> > +     if (tb[NL80211_PMSR_FTM_RESP_ATTR_RTT_AVG]) {
> > +             result->rtt_avg_valid = 1;
> > +             result->rtt_avg =
> > +                     nla_get_u64(tb[NL80211_PMSR_FTM_RESP_ATTR_RTT_AVG]);
> > +     }
> > +     if (tb[NL80211_PMSR_FTM_RESP_ATTR_RTT_VARIANCE]) {
> > +             result->rtt_variance_valid = 1;
> > +             result->rtt_variance =
> > +                     nla_get_u64(tb[NL80211_PMSR_FTM_RESP_ATTR_RTT_VARIANCE]);
> > +     }
> > +     if (tb[NL80211_PMSR_FTM_RESP_ATTR_RTT_SPREAD]) {
> > +             result->rtt_spread_valid = 1;
> > +             result->rtt_spread =
> > +                     nla_get_u64(tb[NL80211_PMSR_FTM_RESP_ATTR_RTT_SPREAD]);
> > +     }
> > +     if (tb[NL80211_PMSR_FTM_RESP_ATTR_DIST_AVG]) {
> > +             result->dist_avg_valid = 1;
> > +             result->dist_avg =
> > +                     nla_get_u64(tb[NL80211_PMSR_FTM_RESP_ATTR_DIST_AVG]);
> > +     }
> > +     if (tb[NL80211_PMSR_FTM_RESP_ATTR_DIST_VARIANCE]) {
> > +             result->dist_variance_valid = 1;
> > +             result->dist_variance =
> > +                     nla_get_u64(tb[NL80211_PMSR_FTM_RESP_ATTR_DIST_VARIANCE]);
> > +     }
> > +     if (tb[NL80211_PMSR_FTM_RESP_ATTR_DIST_SPREAD]) {
> > +             result->dist_spread_valid = 1;
> > +             result->dist_spread =
> > +                     nla_get_u64(tb[NL80211_PMSR_FTM_RESP_ATTR_DIST_SPREAD]);
> > +     }
> > +
> > +     if (tb[NL80211_PMSR_FTM_RESP_ATTR_LCI]) {
> > +             result->lci = nla_data(tb[NL80211_PMSR_FTM_RESP_ATTR_LCI]);
> > +             result->lci_len = nla_len(tb[NL80211_PMSR_FTM_RESP_ATTR_LCI]);
> > +     }
> > +
> > +     if (tb[NL80211_PMSR_FTM_RESP_ATTR_CIVICLOC]) {
> > +             result->civicloc = nla_data(tb[NL80211_PMSR_FTM_RESP_ATTR_CIVICLOC]);
> > +             result->civicloc_len = nla_len(tb[NL80211_PMSR_FTM_RESP_ATTR_CIVICLOC]);
> > +     }
> > +
> > +     return 0;
> > +}
> > +
> > +static int mac80211_hwsim_parse_pmsr_resp(struct nlattr *resp,
> > +                                       struct cfg80211_pmsr_result *result,
> > +                                       struct genl_info *info)
> > +{
> > +     struct nlattr *tb[NL80211_PMSR_RESP_ATTR_MAX + 1];
> > +     struct nlattr *pmsr;
> > +     int rem;
> > +     int ret;
> > +
> > +     ret = nla_parse_nested(tb, NL80211_PMSR_RESP_ATTR_MAX, resp,
> > +                            hwsim_pmsr_resp_policy, info->extack);
>
> You are assigning the value to "ret" variable but you are never using
> it. Is the check for "ret" missing?
>
> > +
> > +     if (tb[NL80211_PMSR_RESP_ATTR_STATUS])
> > +             result->status = nla_get_u32(tb[NL80211_PMSR_RESP_ATTR_STATUS]);
> > +
> > +     if (tb[NL80211_PMSR_RESP_ATTR_HOST_TIME])
> > +             result->host_time = nla_get_u64(tb[NL80211_PMSR_RESP_ATTR_HOST_TIME]);
> > +
> > +     if (tb[NL80211_PMSR_RESP_ATTR_AP_TSF]) {
> > +             result->ap_tsf_valid = 1;
> > +             result->ap_tsf = nla_get_u64(tb[NL80211_PMSR_RESP_ATTR_AP_TSF]);
> > +     }
> > +
> > +     result->final = !!tb[NL80211_PMSR_RESP_ATTR_FINAL];
> > +
> > +     if (tb[NL80211_PMSR_RESP_ATTR_DATA]) {
>
> How about using a negative logic in here to decrease indentation?
> For example:
>
>         if (!tb[NL80211_PMSR_RESP_ATTR_DATA])
>                 return ret;
>
> > +             nla_for_each_nested(pmsr, tb[NL80211_PMSR_RESP_ATTR_DATA], rem) {
> > +                     switch (nla_type(pmsr)) {
> > +                     case NL80211_PMSR_TYPE_FTM:
> > +                             result->type = NL80211_PMSR_TYPE_FTM;
> > +                             ret = mac80211_hwsim_parse_ftm_result(pmsr, &result->ftm, info);
> > +                             if (ret)
> > +                                     return ret;
> > +                             break;
> > +                     default:
> > +                             NL_SET_ERR_MSG_ATTR(info->extack, pmsr, "Unknown pmsr resp type");
> > +                             return -EINVAL;
> > +                     }
> > +             }
> > +     }
> > +
> > +     return 0;
> > +}
> > +
> > +static int mac80211_hwsim_parse_pmsr_result(struct nlattr *peer,
> > +                                         struct cfg80211_pmsr_result *result,
> > +                                         struct genl_info *info)
> > +{
> > +     struct nlattr *tb[NL80211_PMSR_PEER_ATTR_MAX + 1];
> > +     int ret;
> > +
> > +     if (!peer)
> > +             return -EINVAL;
> > +
> > +     ret = nla_parse_nested(tb, NL80211_PMSR_PEER_ATTR_MAX, peer,
> > +                            hwsim_pmsr_peer_result_policy, info->extack);
> > +     if (ret)
> > +             return ret;
> > +
> > +     if (tb[NL80211_PMSR_PEER_ATTR_ADDR])
> > +             memcpy(result->addr, nla_data(tb[NL80211_PMSR_PEER_ATTR_ADDR]),
> > +                    ETH_ALEN);
> > +
> > +     if (tb[NL80211_PMSR_PEER_ATTR_RESP]) {
> > +             ret = mac80211_hwsim_parse_pmsr_resp(tb[NL80211_PMSR_PEER_ATTR_RESP], result, info);
> > +             if (ret)
> > +                     return ret;
> > +     }
> > +
> > +     return 0;
> > +};
> > +
> > +static int hwsim_pmsr_report_nl(struct sk_buff *msg, struct genl_info *info)
> > +{
> > +     struct nlattr *reqattr;
> > +     const u8 *src;
> > +     int err, rem;
> > +     struct nlattr *peers, *peer;
> > +     struct mac80211_hwsim_data *data;
>
> Please use RCT formatting.
>
> > +
> > +     src = nla_data(info->attrs[HWSIM_ATTR_ADDR_TRANSMITTER]);
> > +     data = get_hwsim_data_ref_from_addr(src);
> > +     if (!data)
> > +             return -EINVAL;
> > +
> > +     mutex_lock(&data->mutex);
> > +     if (!data->pmsr_request) {
> > +             err = -EINVAL;
> > +             goto out_err;
> > +     }
> > +
> > +     reqattr = info->attrs[HWSIM_ATTR_PMSR_RESULT];
> > +     if (!reqattr) {
> > +             err = -EINVAL;
> > +             goto out_err;
> > +     }
> > +
> > +     peers = nla_find_nested(reqattr, NL80211_PMSR_ATTR_PEERS);
> > +     if (!peers) {
> > +             err = -EINVAL;
> > +             goto out_err;
> > +     }
> > +
> > +     nla_for_each_nested(peer, peers, rem) {
> > +             struct cfg80211_pmsr_result result;
> > +
> > +             err = mac80211_hwsim_parse_pmsr_result(peer, &result, info);
> > +             if (err)
> > +                     goto out_err;
> > +
> > +             cfg80211_pmsr_report(data->pmsr_request_wdev,
> > +                                  data->pmsr_request, &result, GFP_KERNEL);
> > +     }
> > +
> > +     cfg80211_pmsr_complete(data->pmsr_request_wdev, data->pmsr_request, GFP_KERNEL);
> > +
> > +out_err:
>
> How about renaming this label to "out" or "exit"?
> The code below is used for error path as well as for a normal path.
>
> > +     data->pmsr_request = NULL;
> > +     data->pmsr_request_wdev = NULL;
> > +
> > +     mutex_unlock(&data->mutex);
> > +     return err;
> > +}
> > +
> >  #define HWSIM_COMMON_OPS                                     \
> >       .tx = mac80211_hwsim_tx,                                \
> >       .wake_tx_queue = ieee80211_handle_wake_tx_queue,        \
> > @@ -5072,13 +5434,6 @@ static void hwsim_mon_setup(struct net_device *dev)
> >       eth_hw_addr_set(dev, addr);
> >  }
> >
> > -static struct mac80211_hwsim_data *get_hwsim_data_ref_from_addr(const u8 *addr)
> > -{
> > -     return rhashtable_lookup_fast(&hwsim_radios_rht,
> > -                                   addr,
> > -                                   hwsim_rht_params);
> > -}
> > -
> >  static void hwsim_register_wmediumd(struct net *net, u32 portid)
> >  {
> >       struct mac80211_hwsim_data *data;
> > @@ -5746,6 +6101,11 @@ static const struct genl_small_ops hwsim_ops[] = {
> >               .doit = hwsim_get_radio_nl,
> >               .dumpit = hwsim_dump_radio_nl,
> >       },
> > +     {
> > +             .cmd = HWSIM_CMD_REPORT_PMSR,
> > +             .validate = GENL_DONT_VALIDATE_STRICT | GENL_DONT_VALIDATE_DUMP,
> > +             .doit = hwsim_pmsr_report_nl,
> > +     },
> >  };
> >
> >  static struct genl_family hwsim_genl_family __ro_after_init = {
> > @@ -5757,7 +6117,7 @@ static struct genl_family hwsim_genl_family __ro_after_init = {
> >       .module = THIS_MODULE,
> >       .small_ops = hwsim_ops,
> >       .n_small_ops = ARRAY_SIZE(hwsim_ops),
> > -     .resv_start_op = HWSIM_CMD_DEL_MAC_ADDR + 1,
> > +     .resv_start_op = HWSIM_CMD_REPORT_PMSR + 1, // match with __HWSIM_CMD_MAX
>
>
> >       .mcgrps = hwsim_mcgrps,
> >       .n_mcgrps = ARRAY_SIZE(hwsim_mcgrps),
> >  };
> > @@ -5926,6 +6286,9 @@ static int hwsim_virtio_handle_cmd(struct sk_buff *skb)
> >       case HWSIM_CMD_TX_INFO_FRAME:
> >               hwsim_tx_info_frame_received_nl(skb, &info);
> >               break;
> > +     case HWSIM_CMD_REPORT_PMSR:
> > +             hwsim_pmsr_report_nl(skb, &info);
> > +             break;
> >       default:
> >               pr_err_ratelimited("hwsim: invalid cmd: %d\n", gnlh->cmd);
> >               return -EPROTO;
> > diff --git a/drivers/net/wireless/mac80211_hwsim.h b/drivers/net/wireless/mac80211_hwsim.h
> > index 383f3e39c911..92126f02c58f 100644
> > --- a/drivers/net/wireless/mac80211_hwsim.h
> > +++ b/drivers/net/wireless/mac80211_hwsim.h
> > @@ -82,8 +82,8 @@ enum hwsim_tx_control_flags {
> >   * @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_ABORT_PMSR: abort previously sent peer measurement
> > + *   %HWSIM_ATTR_PMSR_REQUEST. Result will be sent back asynchronously
> > + *   with %HWSIM_CMD_REPORT_PMSR.
> >   * @__HWSIM_CMD_MAX: enum limit
> >   */
> >  enum {
> > @@ -98,6 +98,7 @@ enum {
> >       HWSIM_CMD_DEL_MAC_ADDR,
> >       HWSIM_CMD_START_PMSR,
> >       HWSIM_CMD_ABORT_PMSR,
> > +     HWSIM_CMD_REPORT_PMSR,
> >       __HWSIM_CMD_MAX,
> >  };
> >  #define HWSIM_CMD_MAX (_HWSIM_CMD_MAX - 1)
> > @@ -151,6 +152,8 @@ enum {
> >   *   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_PMSR_RESULT: nested attributed used with %HWSIM_CMD_REPORT_PMSR
> > + *   to provide peer measurement result (nl80211_peer_measurement_attrs)
> >   * @__HWSIM_ATTR_MAX: enum limit
> >   */
> >
> > @@ -184,6 +187,7 @@ enum {
> >       HWSIM_ATTR_MLO_SUPPORT,
> >       HWSIM_ATTR_PMSR_SUPPORT,
> >       HWSIM_ATTR_PMSR_REQUEST,
> > +     HWSIM_ATTR_PMSR_RESULT,
> >       __HWSIM_ATTR_MAX,
> >  };
> >  #define HWSIM_ATTR_MAX (__HWSIM_ATTR_MAX - 1)
> > @@ -288,4 +292,47 @@ enum {
> >       HWSIM_VQ_RX,
> >       HWSIM_NUM_VQS,
> >  };
> > +
> > +/**
> > + * enum hwsim_rate_info -- bitrate information.
> > + *
> > + * Information about a receiving or transmitting bitrate
> > + * that can be mapped to struct rate_info
> > + *
> > + * @HWSIM_RATE_INFO_ATTR_FLAGS: bitflag of flags from &enum rate_info_flags
> > + * @HWSIM_RATE_INFO_ATTR_MCS: mcs index if struct describes an HT/VHT/HE rate
> > + * @HWSIM_RATE_INFO_ATTR_LEGACY: bitrate in 100kbit/s for 802.11abg
> > + * @HWSIM_RATE_INFO_ATTR_NSS: number of streams (VHT & HE only)
> > + * @HWSIM_RATE_INFO_ATTR_BW: bandwidth (from &enum rate_info_bw)
> > + * @HWSIM_RATE_INFO_ATTR_HE_GI: HE guard interval (from &enum nl80211_he_gi)
> > + * @HWSIM_RATE_INFO_ATTR_HE_DCM: HE DCM value
> > + * @HWSIM_RATE_INFO_ATTR_HE_RU_ALLOC:  HE RU allocation (from &enum nl80211_he_ru_alloc,
> > + *   only valid if bw is %RATE_INFO_BW_HE_RU)
> > + * @HWSIM_RATE_INFO_ATTR_N_BOUNDED_CH: In case of EDMG the number of bonded channels (1-4)
> > + * @HWSIM_RATE_INFO_ATTR_EHT_GI: EHT guard interval (from &enum nl80211_eht_gi)
> > + * @HWSIM_RATE_INFO_ATTR_EHT_RU_ALLOC: EHT RU allocation (from &enum nl80211_eht_ru_alloc,
> > + *   only valid if bw is %RATE_INFO_BW_EHT_RU)
> > + * @NUM_HWSIM_RATE_INFO_ATTRS: internal
> > + * @HWSIM_RATE_INFO_ATTR_MAX: highest attribute number
> > + */
> > +enum hwsim_rate_info_attributes {
> > +     __HWSIM_RATE_INFO_ATTR_INVALID,
> > +
> > +     HWSIM_RATE_INFO_ATTR_FLAGS,
> > +     HWSIM_RATE_INFO_ATTR_MCS,
> > +     HWSIM_RATE_INFO_ATTR_LEGACY,
> > +     HWSIM_RATE_INFO_ATTR_NSS,
> > +     HWSIM_RATE_INFO_ATTR_BW,
> > +     HWSIM_RATE_INFO_ATTR_HE_GI,
> > +     HWSIM_RATE_INFO_ATTR_HE_DCM,
> > +     HWSIM_RATE_INFO_ATTR_HE_RU_ALLOC,
> > +     HWSIM_RATE_INFO_ATTR_N_BOUNDED_CH,
> > +     HWSIM_RATE_INFO_ATTR_EHT_GI,
> > +     HWSIM_RATE_INFO_ATTR_EHT_RU_ALLOC,
> > +
> > +     /* keep last */
> > +     NUM_HWSIM_RATE_INFO_ATTRS,
> > +     HWSIM_RATE_INFO_ATTR_MAX = NUM_HWSIM_RATE_INFO_ATTRS - 1
> > +};
> > +
> >  #endif /* __MAC80211_HWSIM_H */
> > --
> > 2.40.0.rc1.284.g88254d51c5-goog
> >

Many Thanks,

On Wed, Mar 15, 2023 at 01:31:38AM +0900, Jaewan Kim wrote:
> On Tue, Mar 14, 2023 at 5:26 AM Michal Kubiak <michal.kubiak@intel.com> wrote:
> >
> > On Mon, Mar 13, 2023 at 07:53:26AM +0000, Jaewan Kim wrote:
> > > PMSR (a.k.a. peer measurement) is generalized measurement between two
> > > devices with Wi-Fi support. And currently FTM (a.k.a. fine time measurement
> > > or flight time measurement) is the one and only measurement.
> > >
> > > Add the necessary functionality to allow mac80211_hwsim to report PMSR
> > > result. The result would come from the wmediumd, where other Wi-Fi
> > > devices' information are kept. mac80211_hwsim only need to deliver the
> > > result to the userspace.
> > >
> > > In detail, add new mac80211_hwsim attributes HWSIM_CMD_REPORT_PMSR, and
> > > HWSIM_ATTR_PMSR_RESULT. When mac80211_hwsim receives the PMSR result with
> > > command HWSIM_CMD_REPORT_PMSR and detail with attribute
> > > HWSIM_ATTR_PMSR_RESULT, received data is parsed to cfg80211_pmsr_result and
> > > resent to the userspace by cfg80211_pmsr_report().
> > >
> > > To help receive the details of PMSR result, hwsim_rate_info_attributes is
> > > added to receive rate_info without complex bitrate calculation. (i.e. send
> > > rate_info without adding inverse of nl80211_put_sta_rate()).
> > >
> > > Signed-off-by: Jaewan Kim <jaewan@google.com>
> > > ---
> > > V7 -> V8: Changed to specify calculated last HWSIM_CMD for resv_start_op
> > >           instead of __HWSIM_CMD_MAX for adding new CMD more explicit.
> > > V7: Initial commit (split from previously large patch)
> > > ---
> > >  drivers/net/wireless/mac80211_hwsim.c | 379 +++++++++++++++++++++++++-
> > >  drivers/net/wireless/mac80211_hwsim.h |  51 +++-
> > >  2 files changed, 420 insertions(+), 10 deletions(-)
> > >
> >
> > General comment: there are many lines exceeding 80 characters (the limit
> > for net).
> > The rest of my comments - inline.
> 
> We can now using 100 columns
> because 80 character limit is deprecated
> 
> Here's previous discussion thread:
> https://patchwork.kernel.org/project/linux-wireless/patch/20230207085400.2232544-2-jaewan@google.com/#25217046

Oh, sorry, so it's my mistake.
I mainly had an experience with ethernet drivers, where we still check
the limit 80 characters.

Thanks,
Michal

> 
> >
> > Thanks,
> > Michal
> >
> > > diff --git a/drivers/net/wireless/mac80211_hwsim.c b/drivers/net/wireless/mac80211_hwsim.c
> > > index 8f699dfab77a..d1218c1efba4 100644
> > > --- a/drivers/net/wireless/mac80211_hwsim.c
> > > +++ b/drivers/net/wireless/mac80211_hwsim.c
> > > @@ -752,6 +752,11 @@ struct hwsim_radiotap_ack_hdr {
> > >       __le16 rt_chbitmask;
> > >  } __packed;
> > >
> > > +static struct mac80211_hwsim_data *get_hwsim_data_ref_from_addr(const u8 *addr)
> > > +{
> > > +     return rhashtable_lookup_fast(&hwsim_radios_rht, addr, hwsim_rht_params);
> > > +}
> > > +
> > >  /* MAC80211_HWSIM netlink family */
> > >  static struct genl_family hwsim_genl_family;
> > >
> > > @@ -765,6 +770,76 @@ static const struct genl_multicast_group hwsim_mcgrps[] = {
> > >
> > >  /* MAC80211_HWSIM netlink policy */
> > >
> > > +static const struct nla_policy
> > > +hwsim_rate_info_policy[HWSIM_RATE_INFO_ATTR_MAX + 1] = {
> > > +     [HWSIM_RATE_INFO_ATTR_FLAGS] = { .type = NLA_U8 },
> > > +     [HWSIM_RATE_INFO_ATTR_MCS] = { .type = NLA_U8 },
> > > +     [HWSIM_RATE_INFO_ATTR_LEGACY] = { .type = NLA_U16 },
> > > +     [HWSIM_RATE_INFO_ATTR_NSS] = { .type = NLA_U8 },
> > > +     [HWSIM_RATE_INFO_ATTR_BW] = { .type = NLA_U8 },
> > > +     [HWSIM_RATE_INFO_ATTR_HE_GI] = { .type = NLA_U8 },
> > > +     [HWSIM_RATE_INFO_ATTR_HE_DCM] = { .type = NLA_U8 },
> > > +     [HWSIM_RATE_INFO_ATTR_HE_RU_ALLOC] = { .type = NLA_U8 },
> > > +     [HWSIM_RATE_INFO_ATTR_N_BOUNDED_CH] = { .type = NLA_U8 },
> > > +     [HWSIM_RATE_INFO_ATTR_EHT_GI] = { .type = NLA_U8 },
> > > +     [HWSIM_RATE_INFO_ATTR_EHT_RU_ALLOC] = { .type = NLA_U8 },
> > > +};
> > > +
> > > +static const struct nla_policy
> > > +hwsim_ftm_result_policy[NL80211_PMSR_FTM_RESP_ATTR_MAX + 1] = {
> > > +     [NL80211_PMSR_FTM_RESP_ATTR_FAIL_REASON] = { .type = NLA_U32 },
> > > +     [NL80211_PMSR_FTM_RESP_ATTR_BURST_INDEX] = { .type = NLA_U16 },
> > > +     [NL80211_PMSR_FTM_RESP_ATTR_NUM_FTMR_ATTEMPTS] = { .type = NLA_U32 },
> > > +     [NL80211_PMSR_FTM_RESP_ATTR_NUM_FTMR_SUCCESSES] = { .type = NLA_U32 },
> > > +     [NL80211_PMSR_FTM_RESP_ATTR_BUSY_RETRY_TIME] = { .type = NLA_U8 },
> > > +     [NL80211_PMSR_FTM_RESP_ATTR_NUM_BURSTS_EXP] = { .type = NLA_U8 },
> > > +     [NL80211_PMSR_FTM_RESP_ATTR_BURST_DURATION] = { .type = NLA_U8 },
> > > +     [NL80211_PMSR_FTM_RESP_ATTR_FTMS_PER_BURST] = { .type = NLA_U8 },
> > > +     [NL80211_PMSR_FTM_RESP_ATTR_RSSI_AVG] = { .type = NLA_U32 },
> > > +     [NL80211_PMSR_FTM_RESP_ATTR_RSSI_SPREAD] = { .type = NLA_U32 },
> > > +     [NL80211_PMSR_FTM_RESP_ATTR_TX_RATE] = NLA_POLICY_NESTED(hwsim_rate_info_policy),
> > > +     [NL80211_PMSR_FTM_RESP_ATTR_RX_RATE] = NLA_POLICY_NESTED(hwsim_rate_info_policy),
> > > +     [NL80211_PMSR_FTM_RESP_ATTR_RTT_AVG] = { .type = NLA_U64 },
> > > +     [NL80211_PMSR_FTM_RESP_ATTR_RTT_VARIANCE] = { .type = NLA_U64 },
> > > +     [NL80211_PMSR_FTM_RESP_ATTR_RTT_SPREAD] = { .type = NLA_U64 },
> > > +     [NL80211_PMSR_FTM_RESP_ATTR_DIST_AVG] = { .type = NLA_U64 },
> > > +     [NL80211_PMSR_FTM_RESP_ATTR_DIST_VARIANCE] = { .type = NLA_U64 },
> > > +     [NL80211_PMSR_FTM_RESP_ATTR_DIST_SPREAD] = { .type = NLA_U64 },
> > > +     [NL80211_PMSR_FTM_RESP_ATTR_LCI] = { .type = NLA_STRING },
> > > +     [NL80211_PMSR_FTM_RESP_ATTR_CIVICLOC] = { .type = NLA_STRING },
> > > +};
> > > +
> > > +static const struct nla_policy
> > > +hwsim_pmsr_resp_type_policy[NL80211_PMSR_TYPE_MAX + 1] = {
> > > +     [NL80211_PMSR_TYPE_FTM] = NLA_POLICY_NESTED(hwsim_ftm_result_policy),
> > > +};
> > > +
> > > +static const struct nla_policy
> > > +hwsim_pmsr_resp_policy[NL80211_PMSR_RESP_ATTR_MAX + 1] = {
> > > +     [NL80211_PMSR_RESP_ATTR_STATUS] = { .type = NLA_U32 },
> > > +     [NL80211_PMSR_RESP_ATTR_HOST_TIME] = { .type = NLA_U64 },
> > > +     [NL80211_PMSR_RESP_ATTR_AP_TSF] = { .type = NLA_U64 },
> > > +     [NL80211_PMSR_RESP_ATTR_FINAL] = { .type = NLA_FLAG },
> > > +     [NL80211_PMSR_RESP_ATTR_DATA] = NLA_POLICY_NESTED(hwsim_pmsr_resp_type_policy),
> > > +};
> > > +
> > > +static const struct nla_policy
> > > +hwsim_pmsr_peer_result_policy[NL80211_PMSR_PEER_ATTR_MAX + 1] = {
> > > +     [NL80211_PMSR_PEER_ATTR_ADDR] = NLA_POLICY_ETH_ADDR_COMPAT,
> > > +     [NL80211_PMSR_PEER_ATTR_CHAN] = { .type = NLA_REJECT },
> > > +     [NL80211_PMSR_PEER_ATTR_REQ] = { .type = NLA_REJECT },
> > > +     [NL80211_PMSR_PEER_ATTR_RESP] = NLA_POLICY_NESTED(hwsim_pmsr_resp_policy),
> > > +};
> > > +
> > > +static const struct nla_policy
> > > +hwsim_pmsr_peers_result_policy[NL80211_PMSR_ATTR_MAX + 1] = {
> > > +     [NL80211_PMSR_ATTR_MAX_PEERS] = { .type = NLA_REJECT },
> > > +     [NL80211_PMSR_ATTR_REPORT_AP_TSF] = { .type = NLA_REJECT },
> > > +     [NL80211_PMSR_ATTR_RANDOMIZE_MAC_ADDR] = { .type = NLA_REJECT },
> > > +     [NL80211_PMSR_ATTR_TYPE_CAPA] = { .type = NLA_REJECT },
> > > +     [NL80211_PMSR_ATTR_PEERS] = NLA_POLICY_NESTED_ARRAY(hwsim_pmsr_peer_result_policy),
> > > +};
> > > +
> > >  static const struct nla_policy
> > >  hwsim_ftm_capa_policy[NL80211_PMSR_FTM_CAPA_ATTR_MAX + 1] = {
> > >       [NL80211_PMSR_FTM_CAPA_ATTR_ASAP] = { .type = NLA_FLAG },
> > > @@ -822,6 +897,7 @@ static const struct nla_policy hwsim_genl_policy[HWSIM_ATTR_MAX + 1] = {
> > >       [HWSIM_ATTR_CIPHER_SUPPORT] = { .type = NLA_BINARY },
> > >       [HWSIM_ATTR_MLO_SUPPORT] = { .type = NLA_FLAG },
> > >       [HWSIM_ATTR_PMSR_SUPPORT] = NLA_POLICY_NESTED(hwsim_pmsr_capa_policy),
> > > +     [HWSIM_ATTR_PMSR_RESULT] = NLA_POLICY_NESTED(hwsim_pmsr_peers_result_policy),
> > >  };
> > >
> > >  #if IS_REACHABLE(CONFIG_VIRTIO)
> > > @@ -3403,6 +3479,292 @@ static void mac80211_hwsim_abort_pmsr(struct ieee80211_hw *hw,
> > >       mutex_unlock(&data->mutex);
> > >  }
> > >
> > > +static int mac80211_hwsim_parse_rate_info(struct nlattr *rateattr,
> > > +                                       struct rate_info *rate_info,
> > > +                                       struct genl_info *info)
> > > +{
> > > +     struct nlattr *tb[HWSIM_RATE_INFO_ATTR_MAX + 1];
> > > +     int ret;
> > > +
> > > +     ret = nla_parse_nested(tb, HWSIM_RATE_INFO_ATTR_MAX,
> > > +                            rateattr, hwsim_rate_info_policy, info->extack);
> > > +     if (ret)
> > > +             return ret;
> > > +
> > > +     if (tb[HWSIM_RATE_INFO_ATTR_FLAGS])
> > > +             rate_info->flags = nla_get_u8(tb[HWSIM_RATE_INFO_ATTR_FLAGS]);
> > > +
> > > +     if (tb[HWSIM_RATE_INFO_ATTR_MCS])
> > > +             rate_info->mcs = nla_get_u8(tb[HWSIM_RATE_INFO_ATTR_MCS]);
> > > +
> > > +     if (tb[HWSIM_RATE_INFO_ATTR_LEGACY])
> > > +             rate_info->legacy = nla_get_u16(tb[HWSIM_RATE_INFO_ATTR_LEGACY]);
> > > +
> > > +     if (tb[HWSIM_RATE_INFO_ATTR_NSS])
> > > +             rate_info->nss = nla_get_u8(tb[HWSIM_RATE_INFO_ATTR_NSS]);
> > > +
> > > +     if (tb[HWSIM_RATE_INFO_ATTR_BW])
> > > +             rate_info->bw = nla_get_u8(tb[HWSIM_RATE_INFO_ATTR_BW]);
> > > +
> > > +     if (tb[HWSIM_RATE_INFO_ATTR_HE_GI])
> > > +             rate_info->he_gi = nla_get_u8(tb[HWSIM_RATE_INFO_ATTR_HE_GI]);
> > > +
> > > +     if (tb[HWSIM_RATE_INFO_ATTR_HE_DCM])
> > > +             rate_info->he_dcm = nla_get_u8(tb[HWSIM_RATE_INFO_ATTR_HE_DCM]);
> > > +
> > > +     if (tb[HWSIM_RATE_INFO_ATTR_HE_RU_ALLOC])
> > > +             rate_info->he_ru_alloc =
> > > +                     nla_get_u8(tb[HWSIM_RATE_INFO_ATTR_HE_RU_ALLOC]);
> > > +
> > > +     if (tb[HWSIM_RATE_INFO_ATTR_N_BOUNDED_CH])
> > > +             rate_info->n_bonded_ch = nla_get_u8(tb[HWSIM_RATE_INFO_ATTR_N_BOUNDED_CH]);
> > > +
> > > +     if (tb[HWSIM_RATE_INFO_ATTR_EHT_GI])
> > > +             rate_info->eht_gi = nla_get_u8(tb[HWSIM_RATE_INFO_ATTR_EHT_GI]);
> > > +
> > > +     if (tb[HWSIM_RATE_INFO_ATTR_EHT_RU_ALLOC])
> > > +             rate_info->eht_ru_alloc = nla_get_u8(tb[HWSIM_RATE_INFO_ATTR_EHT_RU_ALLOC]);
> > > +
> > > +     return 0;
> > > +}
> >
> > Lines in the function above often exceed 80 chars.
> >
> > > +
> > > +static int mac80211_hwsim_parse_ftm_result(struct nlattr *ftm,
> > > +                                        struct cfg80211_pmsr_ftm_result *result,
> > > +                                        struct genl_info *info)
> > > +{
> > > +     struct nlattr *tb[NL80211_PMSR_FTM_RESP_ATTR_MAX + 1];
> > > +     int ret;
> > > +
> > > +     ret = nla_parse_nested(tb, NL80211_PMSR_FTM_RESP_ATTR_MAX,
> > > +                            ftm, hwsim_ftm_result_policy, info->extack);
> > > +     if (ret)
> > > +             return ret;
> > > +
> > > +     if (tb[NL80211_PMSR_FTM_RESP_ATTR_FAIL_REASON])
> > > +             result->failure_reason = nla_get_u32(tb[NL80211_PMSR_FTM_RESP_ATTR_FAIL_REASON]);
> > > +
> > > +     if (tb[NL80211_PMSR_FTM_RESP_ATTR_BURST_INDEX])
> > > +             result->burst_index = nla_get_u16(tb[NL80211_PMSR_FTM_RESP_ATTR_BURST_INDEX]);
> > > +
> > > +     if (tb[NL80211_PMSR_FTM_RESP_ATTR_NUM_FTMR_ATTEMPTS]) {
> > > +             result->num_ftmr_attempts_valid = 1;
> > > +             result->num_ftmr_attempts =
> > > +                     nla_get_u32(tb[NL80211_PMSR_FTM_RESP_ATTR_NUM_FTMR_ATTEMPTS]);
> > > +     }
> > > +
> > > +     if (tb[NL80211_PMSR_FTM_RESP_ATTR_NUM_FTMR_SUCCESSES]) {
> > > +             result->num_ftmr_successes_valid = 1;
> > > +             result->num_ftmr_successes =
> > > +                     nla_get_u32(tb[NL80211_PMSR_FTM_RESP_ATTR_NUM_FTMR_SUCCESSES]);
> > > +     }
> > > +
> > > +     if (tb[NL80211_PMSR_FTM_RESP_ATTR_BUSY_RETRY_TIME])
> > > +             result->busy_retry_time =
> > > +                     nla_get_u8(tb[NL80211_PMSR_FTM_RESP_ATTR_BUSY_RETRY_TIME]);
> > > +
> > > +     if (tb[NL80211_PMSR_FTM_RESP_ATTR_NUM_BURSTS_EXP])
> > > +             result->num_bursts_exp = nla_get_u8(tb[NL80211_PMSR_FTM_RESP_ATTR_NUM_BURSTS_EXP]);
> > > +
> > > +     if (tb[NL80211_PMSR_FTM_RESP_ATTR_BURST_DURATION])
> > > +             result->burst_duration = nla_get_u8(tb[NL80211_PMSR_FTM_RESP_ATTR_BURST_DURATION]);
> > > +
> > > +     if (tb[NL80211_PMSR_FTM_RESP_ATTR_FTMS_PER_BURST])
> > > +             result->ftms_per_burst = nla_get_u8(tb[NL80211_PMSR_FTM_RESP_ATTR_FTMS_PER_BURST]);
> > > +
> > > +     if (tb[NL80211_PMSR_FTM_RESP_ATTR_RSSI_AVG]) {
> > > +             result->rssi_avg_valid = 1;
> > > +             result->rssi_avg = nla_get_s32(tb[NL80211_PMSR_FTM_RESP_ATTR_RSSI_AVG]);
> > > +     }
> > > +     if (tb[NL80211_PMSR_FTM_RESP_ATTR_RSSI_SPREAD]) {
> > > +             result->rssi_spread_valid = 1;
> > > +             result->rssi_spread =
> > > +                     nla_get_s32(tb[NL80211_PMSR_FTM_RESP_ATTR_RSSI_SPREAD]);
> > > +     }
> > > +
> > > +     if (tb[NL80211_PMSR_FTM_RESP_ATTR_TX_RATE]) {
> > > +             result->tx_rate_valid = 1;
> > > +             ret = mac80211_hwsim_parse_rate_info(tb[NL80211_PMSR_FTM_RESP_ATTR_TX_RATE],
> > > +                                                  &result->tx_rate, info);
> > > +             if (ret)
> > > +                     return ret;
> > > +     }
> > > +
> > > +     if (tb[NL80211_PMSR_FTM_RESP_ATTR_RX_RATE]) {
> > > +             result->rx_rate_valid = 1;
> > > +             ret = mac80211_hwsim_parse_rate_info(tb[NL80211_PMSR_FTM_RESP_ATTR_RX_RATE],
> > > +                                                  &result->rx_rate, info);
> > > +             if (ret)
> > > +                     return ret;
> > > +     }
> > > +
> > > +     if (tb[NL80211_PMSR_FTM_RESP_ATTR_RTT_AVG]) {
> > > +             result->rtt_avg_valid = 1;
> > > +             result->rtt_avg =
> > > +                     nla_get_u64(tb[NL80211_PMSR_FTM_RESP_ATTR_RTT_AVG]);
> > > +     }
> > > +     if (tb[NL80211_PMSR_FTM_RESP_ATTR_RTT_VARIANCE]) {
> > > +             result->rtt_variance_valid = 1;
> > > +             result->rtt_variance =
> > > +                     nla_get_u64(tb[NL80211_PMSR_FTM_RESP_ATTR_RTT_VARIANCE]);
> > > +     }
> > > +     if (tb[NL80211_PMSR_FTM_RESP_ATTR_RTT_SPREAD]) {
> > > +             result->rtt_spread_valid = 1;
> > > +             result->rtt_spread =
> > > +                     nla_get_u64(tb[NL80211_PMSR_FTM_RESP_ATTR_RTT_SPREAD]);
> > > +     }
> > > +     if (tb[NL80211_PMSR_FTM_RESP_ATTR_DIST_AVG]) {
> > > +             result->dist_avg_valid = 1;
> > > +             result->dist_avg =
> > > +                     nla_get_u64(tb[NL80211_PMSR_FTM_RESP_ATTR_DIST_AVG]);
> > > +     }
> > > +     if (tb[NL80211_PMSR_FTM_RESP_ATTR_DIST_VARIANCE]) {
> > > +             result->dist_variance_valid = 1;
> > > +             result->dist_variance =
> > > +                     nla_get_u64(tb[NL80211_PMSR_FTM_RESP_ATTR_DIST_VARIANCE]);
> > > +     }
> > > +     if (tb[NL80211_PMSR_FTM_RESP_ATTR_DIST_SPREAD]) {
> > > +             result->dist_spread_valid = 1;
> > > +             result->dist_spread =
> > > +                     nla_get_u64(tb[NL80211_PMSR_FTM_RESP_ATTR_DIST_SPREAD]);
> > > +     }
> > > +
> > > +     if (tb[NL80211_PMSR_FTM_RESP_ATTR_LCI]) {
> > > +             result->lci = nla_data(tb[NL80211_PMSR_FTM_RESP_ATTR_LCI]);
> > > +             result->lci_len = nla_len(tb[NL80211_PMSR_FTM_RESP_ATTR_LCI]);
> > > +     }
> > > +
> > > +     if (tb[NL80211_PMSR_FTM_RESP_ATTR_CIVICLOC]) {
> > > +             result->civicloc = nla_data(tb[NL80211_PMSR_FTM_RESP_ATTR_CIVICLOC]);
> > > +             result->civicloc_len = nla_len(tb[NL80211_PMSR_FTM_RESP_ATTR_CIVICLOC]);
> > > +     }
> > > +
> > > +     return 0;
> > > +}
> > > +
> > > +static int mac80211_hwsim_parse_pmsr_resp(struct nlattr *resp,
> > > +                                       struct cfg80211_pmsr_result *result,
> > > +                                       struct genl_info *info)
> > > +{
> > > +     struct nlattr *tb[NL80211_PMSR_RESP_ATTR_MAX + 1];
> > > +     struct nlattr *pmsr;
> > > +     int rem;
> > > +     int ret;
> > > +
> > > +     ret = nla_parse_nested(tb, NL80211_PMSR_RESP_ATTR_MAX, resp,
> > > +                            hwsim_pmsr_resp_policy, info->extack);
> >
> > You are assigning the value to "ret" variable but you are never using
> > it. Is the check for "ret" missing?
> >
> > > +
> > > +     if (tb[NL80211_PMSR_RESP_ATTR_STATUS])
> > > +             result->status = nla_get_u32(tb[NL80211_PMSR_RESP_ATTR_STATUS]);
> > > +
> > > +     if (tb[NL80211_PMSR_RESP_ATTR_HOST_TIME])
> > > +             result->host_time = nla_get_u64(tb[NL80211_PMSR_RESP_ATTR_HOST_TIME]);
> > > +
> > > +     if (tb[NL80211_PMSR_RESP_ATTR_AP_TSF]) {
> > > +             result->ap_tsf_valid = 1;
> > > +             result->ap_tsf = nla_get_u64(tb[NL80211_PMSR_RESP_ATTR_AP_TSF]);
> > > +     }
> > > +
> > > +     result->final = !!tb[NL80211_PMSR_RESP_ATTR_FINAL];
> > > +
> > > +     if (tb[NL80211_PMSR_RESP_ATTR_DATA]) {
> >
> > How about using a negative logic in here to decrease indentation?
> > For example:
> >
> >         if (!tb[NL80211_PMSR_RESP_ATTR_DATA])
> >                 return ret;
> >
> > > +             nla_for_each_nested(pmsr, tb[NL80211_PMSR_RESP_ATTR_DATA], rem) {
> > > +                     switch (nla_type(pmsr)) {
> > > +                     case NL80211_PMSR_TYPE_FTM:
> > > +                             result->type = NL80211_PMSR_TYPE_FTM;
> > > +                             ret = mac80211_hwsim_parse_ftm_result(pmsr, &result->ftm, info);
> > > +                             if (ret)
> > > +                                     return ret;
> > > +                             break;
> > > +                     default:
> > > +                             NL_SET_ERR_MSG_ATTR(info->extack, pmsr, "Unknown pmsr resp type");
> > > +                             return -EINVAL;
> > > +                     }
> > > +             }
> > > +     }
> > > +
> > > +     return 0;
> > > +}
> > > +
> > > +static int mac80211_hwsim_parse_pmsr_result(struct nlattr *peer,
> > > +                                         struct cfg80211_pmsr_result *result,
> > > +                                         struct genl_info *info)
> > > +{
> > > +     struct nlattr *tb[NL80211_PMSR_PEER_ATTR_MAX + 1];
> > > +     int ret;
> > > +
> > > +     if (!peer)
> > > +             return -EINVAL;
> > > +
> > > +     ret = nla_parse_nested(tb, NL80211_PMSR_PEER_ATTR_MAX, peer,
> > > +                            hwsim_pmsr_peer_result_policy, info->extack);
> > > +     if (ret)
> > > +             return ret;
> > > +
> > > +     if (tb[NL80211_PMSR_PEER_ATTR_ADDR])
> > > +             memcpy(result->addr, nla_data(tb[NL80211_PMSR_PEER_ATTR_ADDR]),
> > > +                    ETH_ALEN);
> > > +
> > > +     if (tb[NL80211_PMSR_PEER_ATTR_RESP]) {
> > > +             ret = mac80211_hwsim_parse_pmsr_resp(tb[NL80211_PMSR_PEER_ATTR_RESP], result, info);
> > > +             if (ret)
> > > +                     return ret;
> > > +     }
> > > +
> > > +     return 0;
> > > +};
> > > +
> > > +static int hwsim_pmsr_report_nl(struct sk_buff *msg, struct genl_info *info)
> > > +{
> > > +     struct nlattr *reqattr;
> > > +     const u8 *src;
> > > +     int err, rem;
> > > +     struct nlattr *peers, *peer;
> > > +     struct mac80211_hwsim_data *data;
> >
> > Please use RCT formatting.
> >
> > > +
> > > +     src = nla_data(info->attrs[HWSIM_ATTR_ADDR_TRANSMITTER]);
> > > +     data = get_hwsim_data_ref_from_addr(src);
> > > +     if (!data)
> > > +             return -EINVAL;
> > > +
> > > +     mutex_lock(&data->mutex);
> > > +     if (!data->pmsr_request) {
> > > +             err = -EINVAL;
> > > +             goto out_err;
> > > +     }
> > > +
> > > +     reqattr = info->attrs[HWSIM_ATTR_PMSR_RESULT];
> > > +     if (!reqattr) {
> > > +             err = -EINVAL;
> > > +             goto out_err;
> > > +     }
> > > +
> > > +     peers = nla_find_nested(reqattr, NL80211_PMSR_ATTR_PEERS);
> > > +     if (!peers) {
> > > +             err = -EINVAL;
> > > +             goto out_err;
> > > +     }
> > > +
> > > +     nla_for_each_nested(peer, peers, rem) {
> > > +             struct cfg80211_pmsr_result result;
> > > +
> > > +             err = mac80211_hwsim_parse_pmsr_result(peer, &result, info);
> > > +             if (err)
> > > +                     goto out_err;
> > > +
> > > +             cfg80211_pmsr_report(data->pmsr_request_wdev,
> > > +                                  data->pmsr_request, &result, GFP_KERNEL);
> > > +     }
> > > +
> > > +     cfg80211_pmsr_complete(data->pmsr_request_wdev, data->pmsr_request, GFP_KERNEL);
> > > +
> > > +out_err:
> >
> > How about renaming this label to "out" or "exit"?
> > The code below is used for error path as well as for a normal path.
> >
> > > +     data->pmsr_request = NULL;
> > > +     data->pmsr_request_wdev = NULL;
> > > +
> > > +     mutex_unlock(&data->mutex);
> > > +     return err;
> > > +}
> > > +
> > >  #define HWSIM_COMMON_OPS                                     \
> > >       .tx = mac80211_hwsim_tx,                                \
> > >       .wake_tx_queue = ieee80211_handle_wake_tx_queue,        \
> > > @@ -5072,13 +5434,6 @@ static void hwsim_mon_setup(struct net_device *dev)
> > >       eth_hw_addr_set(dev, addr);
> > >  }
> > >
> > > -static struct mac80211_hwsim_data *get_hwsim_data_ref_from_addr(const u8 *addr)
> > > -{
> > > -     return rhashtable_lookup_fast(&hwsim_radios_rht,
> > > -                                   addr,
> > > -                                   hwsim_rht_params);
> > > -}
> > > -
> > >  static void hwsim_register_wmediumd(struct net *net, u32 portid)
> > >  {
> > >       struct mac80211_hwsim_data *data;
> > > @@ -5746,6 +6101,11 @@ static const struct genl_small_ops hwsim_ops[] = {
> > >               .doit = hwsim_get_radio_nl,
> > >               .dumpit = hwsim_dump_radio_nl,
> > >       },
> > > +     {
> > > +             .cmd = HWSIM_CMD_REPORT_PMSR,
> > > +             .validate = GENL_DONT_VALIDATE_STRICT | GENL_DONT_VALIDATE_DUMP,
> > > +             .doit = hwsim_pmsr_report_nl,
> > > +     },
> > >  };
> > >
> > >  static struct genl_family hwsim_genl_family __ro_after_init = {
> > > @@ -5757,7 +6117,7 @@ static struct genl_family hwsim_genl_family __ro_after_init = {
> > >       .module = THIS_MODULE,
> > >       .small_ops = hwsim_ops,
> > >       .n_small_ops = ARRAY_SIZE(hwsim_ops),
> > > -     .resv_start_op = HWSIM_CMD_DEL_MAC_ADDR + 1,
> > > +     .resv_start_op = HWSIM_CMD_REPORT_PMSR + 1, // match with __HWSIM_CMD_MAX
> >
> >
> > >       .mcgrps = hwsim_mcgrps,
> > >       .n_mcgrps = ARRAY_SIZE(hwsim_mcgrps),
> > >  };
> > > @@ -5926,6 +6286,9 @@ static int hwsim_virtio_handle_cmd(struct sk_buff *skb)
> > >       case HWSIM_CMD_TX_INFO_FRAME:
> > >               hwsim_tx_info_frame_received_nl(skb, &info);
> > >               break;
> > > +     case HWSIM_CMD_REPORT_PMSR:
> > > +             hwsim_pmsr_report_nl(skb, &info);
> > > +             break;
> > >       default:
> > >               pr_err_ratelimited("hwsim: invalid cmd: %d\n", gnlh->cmd);
> > >               return -EPROTO;
> > > diff --git a/drivers/net/wireless/mac80211_hwsim.h b/drivers/net/wireless/mac80211_hwsim.h
> > > index 383f3e39c911..92126f02c58f 100644
> > > --- a/drivers/net/wireless/mac80211_hwsim.h
> > > +++ b/drivers/net/wireless/mac80211_hwsim.h
> > > @@ -82,8 +82,8 @@ enum hwsim_tx_control_flags {
> > >   * @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_ABORT_PMSR: abort previously sent peer measurement
> > > + *   %HWSIM_ATTR_PMSR_REQUEST. Result will be sent back asynchronously
> > > + *   with %HWSIM_CMD_REPORT_PMSR.
> > >   * @__HWSIM_CMD_MAX: enum limit
> > >   */
> > >  enum {
> > > @@ -98,6 +98,7 @@ enum {
> > >       HWSIM_CMD_DEL_MAC_ADDR,
> > >       HWSIM_CMD_START_PMSR,
> > >       HWSIM_CMD_ABORT_PMSR,
> > > +     HWSIM_CMD_REPORT_PMSR,
> > >       __HWSIM_CMD_MAX,
> > >  };
> > >  #define HWSIM_CMD_MAX (_HWSIM_CMD_MAX - 1)
> > > @@ -151,6 +152,8 @@ enum {
> > >   *   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_PMSR_RESULT: nested attributed used with %HWSIM_CMD_REPORT_PMSR
> > > + *   to provide peer measurement result (nl80211_peer_measurement_attrs)
> > >   * @__HWSIM_ATTR_MAX: enum limit
> > >   */
> > >
> > > @@ -184,6 +187,7 @@ enum {
> > >       HWSIM_ATTR_MLO_SUPPORT,
> > >       HWSIM_ATTR_PMSR_SUPPORT,
> > >       HWSIM_ATTR_PMSR_REQUEST,
> > > +     HWSIM_ATTR_PMSR_RESULT,
> > >       __HWSIM_ATTR_MAX,
> > >  };
> > >  #define HWSIM_ATTR_MAX (__HWSIM_ATTR_MAX - 1)
> > > @@ -288,4 +292,47 @@ enum {
> > >       HWSIM_VQ_RX,
> > >       HWSIM_NUM_VQS,
> > >  };
> > > +
> > > +/**
> > > + * enum hwsim_rate_info -- bitrate information.
> > > + *
> > > + * Information about a receiving or transmitting bitrate
> > > + * that can be mapped to struct rate_info
> > > + *
> > > + * @HWSIM_RATE_INFO_ATTR_FLAGS: bitflag of flags from &enum rate_info_flags
> > > + * @HWSIM_RATE_INFO_ATTR_MCS: mcs index if struct describes an HT/VHT/HE rate
> > > + * @HWSIM_RATE_INFO_ATTR_LEGACY: bitrate in 100kbit/s for 802.11abg
> > > + * @HWSIM_RATE_INFO_ATTR_NSS: number of streams (VHT & HE only)
> > > + * @HWSIM_RATE_INFO_ATTR_BW: bandwidth (from &enum rate_info_bw)
> > > + * @HWSIM_RATE_INFO_ATTR_HE_GI: HE guard interval (from &enum nl80211_he_gi)
> > > + * @HWSIM_RATE_INFO_ATTR_HE_DCM: HE DCM value
> > > + * @HWSIM_RATE_INFO_ATTR_HE_RU_ALLOC:  HE RU allocation (from &enum nl80211_he_ru_alloc,
> > > + *   only valid if bw is %RATE_INFO_BW_HE_RU)
> > > + * @HWSIM_RATE_INFO_ATTR_N_BOUNDED_CH: In case of EDMG the number of bonded channels (1-4)
> > > + * @HWSIM_RATE_INFO_ATTR_EHT_GI: EHT guard interval (from &enum nl80211_eht_gi)
> > > + * @HWSIM_RATE_INFO_ATTR_EHT_RU_ALLOC: EHT RU allocation (from &enum nl80211_eht_ru_alloc,
> > > + *   only valid if bw is %RATE_INFO_BW_EHT_RU)
> > > + * @NUM_HWSIM_RATE_INFO_ATTRS: internal
> > > + * @HWSIM_RATE_INFO_ATTR_MAX: highest attribute number
> > > + */
> > > +enum hwsim_rate_info_attributes {
> > > +     __HWSIM_RATE_INFO_ATTR_INVALID,
> > > +
> > > +     HWSIM_RATE_INFO_ATTR_FLAGS,
> > > +     HWSIM_RATE_INFO_ATTR_MCS,
> > > +     HWSIM_RATE_INFO_ATTR_LEGACY,
> > > +     HWSIM_RATE_INFO_ATTR_NSS,
> > > +     HWSIM_RATE_INFO_ATTR_BW,
> > > +     HWSIM_RATE_INFO_ATTR_HE_GI,
> > > +     HWSIM_RATE_INFO_ATTR_HE_DCM,
> > > +     HWSIM_RATE_INFO_ATTR_HE_RU_ALLOC,
> > > +     HWSIM_RATE_INFO_ATTR_N_BOUNDED_CH,
> > > +     HWSIM_RATE_INFO_ATTR_EHT_GI,
> > > +     HWSIM_RATE_INFO_ATTR_EHT_RU_ALLOC,
> > > +
> > > +     /* keep last */
> > > +     NUM_HWSIM_RATE_INFO_ATTRS,
> > > +     HWSIM_RATE_INFO_ATTR_MAX = NUM_HWSIM_RATE_INFO_ATTRS - 1
> > > +};
> > > +
> > >  #endif /* __MAC80211_HWSIM_H */
> > > --
> > > 2.40.0.rc1.284.g88254d51c5-goog
> > >
> 
> Many Thanks,
> 
> -- 
> Jaewan Kim (김재완) | Software Engineer in Google Korea |
> jaewan@google.com | +82-10-2781-5078

On Wed, Mar 15, 2023 at 3:48 AM Michal Kubiak <michal.kubiak@intel.com> wrote:
>
> On Wed, Mar 15, 2023 at 01:31:38AM +0900, Jaewan Kim wrote:
> > On Tue, Mar 14, 2023 at 5:26 AM Michal Kubiak <michal.kubiak@intel.com> wrote:
> > >
> > > On Mon, Mar 13, 2023 at 07:53:26AM +0000, Jaewan Kim wrote:
> > > > PMSR (a.k.a. peer measurement) is generalized measurement between two
> > > > devices with Wi-Fi support. And currently FTM (a.k.a. fine time measurement
> > > > or flight time measurement) is the one and only measurement.
> > > >
> > > > Add the necessary functionality to allow mac80211_hwsim to report PMSR
> > > > result. The result would come from the wmediumd, where other Wi-Fi
> > > > devices' information are kept. mac80211_hwsim only need to deliver the
> > > > result to the userspace.
> > > >
> > > > In detail, add new mac80211_hwsim attributes HWSIM_CMD_REPORT_PMSR, and
> > > > HWSIM_ATTR_PMSR_RESULT. When mac80211_hwsim receives the PMSR result with
> > > > command HWSIM_CMD_REPORT_PMSR and detail with attribute
> > > > HWSIM_ATTR_PMSR_RESULT, received data is parsed to cfg80211_pmsr_result and
> > > > resent to the userspace by cfg80211_pmsr_report().
> > > >
> > > > To help receive the details of PMSR result, hwsim_rate_info_attributes is
> > > > added to receive rate_info without complex bitrate calculation. (i.e. send
> > > > rate_info without adding inverse of nl80211_put_sta_rate()).
> > > >
> > > > Signed-off-by: Jaewan Kim <jaewan@google.com>
> > > > ---
> > > > V7 -> V8: Changed to specify calculated last HWSIM_CMD for resv_start_op
> > > >           instead of __HWSIM_CMD_MAX for adding new CMD more explicit.
> > > > V7: Initial commit (split from previously large patch)
> > > > ---
> > > >  drivers/net/wireless/mac80211_hwsim.c | 379 +++++++++++++++++++++++++-
> > > >  drivers/net/wireless/mac80211_hwsim.h |  51 +++-
> > > >  2 files changed, 420 insertions(+), 10 deletions(-)
> > > >
> > >
> > > General comment: there are many lines exceeding 80 characters (the limit
> > > for net).
> > > The rest of my comments - inline.
> >
> > We can now using 100 columns
> > because 80 character limit is deprecated
> >
> > Here's previous discussion thread:
> > https://patchwork.kernel.org/project/linux-wireless/patch/20230207085400.2232544-2-jaewan@google.com/#25217046
>
> Oh, sorry, so it's my mistake.
> I mainly had an experience with ethernet drivers, where we still check
> the limit 80 characters.
>
> Thanks,
> Michal
>
> >
> > >
> > > Thanks,
> > > Michal
> > >
> > > > diff --git a/drivers/net/wireless/mac80211_hwsim.c b/drivers/net/wireless/mac80211_hwsim.c
> > > > index 8f699dfab77a..d1218c1efba4 100644
> > > > --- a/drivers/net/wireless/mac80211_hwsim.c
> > > > +++ b/drivers/net/wireless/mac80211_hwsim.c
> > > > @@ -752,6 +752,11 @@ struct hwsim_radiotap_ack_hdr {
> > > >       __le16 rt_chbitmask;
> > > >  } __packed;
> > > >
> > > > +static struct mac80211_hwsim_data *get_hwsim_data_ref_from_addr(const u8 *addr)
> > > > +{
> > > > +     return rhashtable_lookup_fast(&hwsim_radios_rht, addr, hwsim_rht_params);
> > > > +}
> > > > +
> > > >  /* MAC80211_HWSIM netlink family */
> > > >  static struct genl_family hwsim_genl_family;
> > > >
> > > > @@ -765,6 +770,76 @@ static const struct genl_multicast_group hwsim_mcgrps[] = {
> > > >
> > > >  /* MAC80211_HWSIM netlink policy */
> > > >
> > > > +static const struct nla_policy
> > > > +hwsim_rate_info_policy[HWSIM_RATE_INFO_ATTR_MAX + 1] = {
> > > > +     [HWSIM_RATE_INFO_ATTR_FLAGS] = { .type = NLA_U8 },
> > > > +     [HWSIM_RATE_INFO_ATTR_MCS] = { .type = NLA_U8 },
> > > > +     [HWSIM_RATE_INFO_ATTR_LEGACY] = { .type = NLA_U16 },
> > > > +     [HWSIM_RATE_INFO_ATTR_NSS] = { .type = NLA_U8 },
> > > > +     [HWSIM_RATE_INFO_ATTR_BW] = { .type = NLA_U8 },
> > > > +     [HWSIM_RATE_INFO_ATTR_HE_GI] = { .type = NLA_U8 },
> > > > +     [HWSIM_RATE_INFO_ATTR_HE_DCM] = { .type = NLA_U8 },
> > > > +     [HWSIM_RATE_INFO_ATTR_HE_RU_ALLOC] = { .type = NLA_U8 },
> > > > +     [HWSIM_RATE_INFO_ATTR_N_BOUNDED_CH] = { .type = NLA_U8 },
> > > > +     [HWSIM_RATE_INFO_ATTR_EHT_GI] = { .type = NLA_U8 },
> > > > +     [HWSIM_RATE_INFO_ATTR_EHT_RU_ALLOC] = { .type = NLA_U8 },
> > > > +};
> > > > +
> > > > +static const struct nla_policy
> > > > +hwsim_ftm_result_policy[NL80211_PMSR_FTM_RESP_ATTR_MAX + 1] = {
> > > > +     [NL80211_PMSR_FTM_RESP_ATTR_FAIL_REASON] = { .type = NLA_U32 },
> > > > +     [NL80211_PMSR_FTM_RESP_ATTR_BURST_INDEX] = { .type = NLA_U16 },
> > > > +     [NL80211_PMSR_FTM_RESP_ATTR_NUM_FTMR_ATTEMPTS] = { .type = NLA_U32 },
> > > > +     [NL80211_PMSR_FTM_RESP_ATTR_NUM_FTMR_SUCCESSES] = { .type = NLA_U32 },
> > > > +     [NL80211_PMSR_FTM_RESP_ATTR_BUSY_RETRY_TIME] = { .type = NLA_U8 },
> > > > +     [NL80211_PMSR_FTM_RESP_ATTR_NUM_BURSTS_EXP] = { .type = NLA_U8 },
> > > > +     [NL80211_PMSR_FTM_RESP_ATTR_BURST_DURATION] = { .type = NLA_U8 },
> > > > +     [NL80211_PMSR_FTM_RESP_ATTR_FTMS_PER_BURST] = { .type = NLA_U8 },
> > > > +     [NL80211_PMSR_FTM_RESP_ATTR_RSSI_AVG] = { .type = NLA_U32 },
> > > > +     [NL80211_PMSR_FTM_RESP_ATTR_RSSI_SPREAD] = { .type = NLA_U32 },
> > > > +     [NL80211_PMSR_FTM_RESP_ATTR_TX_RATE] = NLA_POLICY_NESTED(hwsim_rate_info_policy),
> > > > +     [NL80211_PMSR_FTM_RESP_ATTR_RX_RATE] = NLA_POLICY_NESTED(hwsim_rate_info_policy),
> > > > +     [NL80211_PMSR_FTM_RESP_ATTR_RTT_AVG] = { .type = NLA_U64 },
> > > > +     [NL80211_PMSR_FTM_RESP_ATTR_RTT_VARIANCE] = { .type = NLA_U64 },
> > > > +     [NL80211_PMSR_FTM_RESP_ATTR_RTT_SPREAD] = { .type = NLA_U64 },
> > > > +     [NL80211_PMSR_FTM_RESP_ATTR_DIST_AVG] = { .type = NLA_U64 },
> > > > +     [NL80211_PMSR_FTM_RESP_ATTR_DIST_VARIANCE] = { .type = NLA_U64 },
> > > > +     [NL80211_PMSR_FTM_RESP_ATTR_DIST_SPREAD] = { .type = NLA_U64 },
> > > > +     [NL80211_PMSR_FTM_RESP_ATTR_LCI] = { .type = NLA_STRING },
> > > > +     [NL80211_PMSR_FTM_RESP_ATTR_CIVICLOC] = { .type = NLA_STRING },
> > > > +};
> > > > +
> > > > +static const struct nla_policy
> > > > +hwsim_pmsr_resp_type_policy[NL80211_PMSR_TYPE_MAX + 1] = {
> > > > +     [NL80211_PMSR_TYPE_FTM] = NLA_POLICY_NESTED(hwsim_ftm_result_policy),
> > > > +};
> > > > +
> > > > +static const struct nla_policy
> > > > +hwsim_pmsr_resp_policy[NL80211_PMSR_RESP_ATTR_MAX + 1] = {
> > > > +     [NL80211_PMSR_RESP_ATTR_STATUS] = { .type = NLA_U32 },
> > > > +     [NL80211_PMSR_RESP_ATTR_HOST_TIME] = { .type = NLA_U64 },
> > > > +     [NL80211_PMSR_RESP_ATTR_AP_TSF] = { .type = NLA_U64 },
> > > > +     [NL80211_PMSR_RESP_ATTR_FINAL] = { .type = NLA_FLAG },
> > > > +     [NL80211_PMSR_RESP_ATTR_DATA] = NLA_POLICY_NESTED(hwsim_pmsr_resp_type_policy),
> > > > +};
> > > > +
> > > > +static const struct nla_policy
> > > > +hwsim_pmsr_peer_result_policy[NL80211_PMSR_PEER_ATTR_MAX + 1] = {
> > > > +     [NL80211_PMSR_PEER_ATTR_ADDR] = NLA_POLICY_ETH_ADDR_COMPAT,
> > > > +     [NL80211_PMSR_PEER_ATTR_CHAN] = { .type = NLA_REJECT },
> > > > +     [NL80211_PMSR_PEER_ATTR_REQ] = { .type = NLA_REJECT },
> > > > +     [NL80211_PMSR_PEER_ATTR_RESP] = NLA_POLICY_NESTED(hwsim_pmsr_resp_policy),
> > > > +};
> > > > +
> > > > +static const struct nla_policy
> > > > +hwsim_pmsr_peers_result_policy[NL80211_PMSR_ATTR_MAX + 1] = {
> > > > +     [NL80211_PMSR_ATTR_MAX_PEERS] = { .type = NLA_REJECT },
> > > > +     [NL80211_PMSR_ATTR_REPORT_AP_TSF] = { .type = NLA_REJECT },
> > > > +     [NL80211_PMSR_ATTR_RANDOMIZE_MAC_ADDR] = { .type = NLA_REJECT },
> > > > +     [NL80211_PMSR_ATTR_TYPE_CAPA] = { .type = NLA_REJECT },
> > > > +     [NL80211_PMSR_ATTR_PEERS] = NLA_POLICY_NESTED_ARRAY(hwsim_pmsr_peer_result_policy),
> > > > +};
> > > > +
> > > >  static const struct nla_policy
> > > >  hwsim_ftm_capa_policy[NL80211_PMSR_FTM_CAPA_ATTR_MAX + 1] = {
> > > >       [NL80211_PMSR_FTM_CAPA_ATTR_ASAP] = { .type = NLA_FLAG },
> > > > @@ -822,6 +897,7 @@ static const struct nla_policy hwsim_genl_policy[HWSIM_ATTR_MAX + 1] = {
> > > >       [HWSIM_ATTR_CIPHER_SUPPORT] = { .type = NLA_BINARY },
> > > >       [HWSIM_ATTR_MLO_SUPPORT] = { .type = NLA_FLAG },
> > > >       [HWSIM_ATTR_PMSR_SUPPORT] = NLA_POLICY_NESTED(hwsim_pmsr_capa_policy),
> > > > +     [HWSIM_ATTR_PMSR_RESULT] = NLA_POLICY_NESTED(hwsim_pmsr_peers_result_policy),
> > > >  };
> > > >
> > > >  #if IS_REACHABLE(CONFIG_VIRTIO)
> > > > @@ -3403,6 +3479,292 @@ static void mac80211_hwsim_abort_pmsr(struct ieee80211_hw *hw,
> > > >       mutex_unlock(&data->mutex);
> > > >  }
> > > >
> > > > +static int mac80211_hwsim_parse_rate_info(struct nlattr *rateattr,
> > > > +                                       struct rate_info *rate_info,
> > > > +                                       struct genl_info *info)
> > > > +{
> > > > +     struct nlattr *tb[HWSIM_RATE_INFO_ATTR_MAX + 1];
> > > > +     int ret;
> > > > +
> > > > +     ret = nla_parse_nested(tb, HWSIM_RATE_INFO_ATTR_MAX,
> > > > +                            rateattr, hwsim_rate_info_policy, info->extack);
> > > > +     if (ret)
> > > > +             return ret;
> > > > +
> > > > +     if (tb[HWSIM_RATE_INFO_ATTR_FLAGS])
> > > > +             rate_info->flags = nla_get_u8(tb[HWSIM_RATE_INFO_ATTR_FLAGS]);
> > > > +
> > > > +     if (tb[HWSIM_RATE_INFO_ATTR_MCS])
> > > > +             rate_info->mcs = nla_get_u8(tb[HWSIM_RATE_INFO_ATTR_MCS]);
> > > > +
> > > > +     if (tb[HWSIM_RATE_INFO_ATTR_LEGACY])
> > > > +             rate_info->legacy = nla_get_u16(tb[HWSIM_RATE_INFO_ATTR_LEGACY]);
> > > > +
> > > > +     if (tb[HWSIM_RATE_INFO_ATTR_NSS])
> > > > +             rate_info->nss = nla_get_u8(tb[HWSIM_RATE_INFO_ATTR_NSS]);
> > > > +
> > > > +     if (tb[HWSIM_RATE_INFO_ATTR_BW])
> > > > +             rate_info->bw = nla_get_u8(tb[HWSIM_RATE_INFO_ATTR_BW]);
> > > > +
> > > > +     if (tb[HWSIM_RATE_INFO_ATTR_HE_GI])
> > > > +             rate_info->he_gi = nla_get_u8(tb[HWSIM_RATE_INFO_ATTR_HE_GI]);
> > > > +
> > > > +     if (tb[HWSIM_RATE_INFO_ATTR_HE_DCM])
> > > > +             rate_info->he_dcm = nla_get_u8(tb[HWSIM_RATE_INFO_ATTR_HE_DCM]);
> > > > +
> > > > +     if (tb[HWSIM_RATE_INFO_ATTR_HE_RU_ALLOC])
> > > > +             rate_info->he_ru_alloc =
> > > > +                     nla_get_u8(tb[HWSIM_RATE_INFO_ATTR_HE_RU_ALLOC]);
> > > > +
> > > > +     if (tb[HWSIM_RATE_INFO_ATTR_N_BOUNDED_CH])
> > > > +             rate_info->n_bonded_ch = nla_get_u8(tb[HWSIM_RATE_INFO_ATTR_N_BOUNDED_CH]);
> > > > +
> > > > +     if (tb[HWSIM_RATE_INFO_ATTR_EHT_GI])
> > > > +             rate_info->eht_gi = nla_get_u8(tb[HWSIM_RATE_INFO_ATTR_EHT_GI]);
> > > > +
> > > > +     if (tb[HWSIM_RATE_INFO_ATTR_EHT_RU_ALLOC])
> > > > +             rate_info->eht_ru_alloc = nla_get_u8(tb[HWSIM_RATE_INFO_ATTR_EHT_RU_ALLOC]);
> > > > +
> > > > +     return 0;
> > > > +}
> > >
> > > Lines in the function above often exceed 80 chars.
> > >
> > > > +
> > > > +static int mac80211_hwsim_parse_ftm_result(struct nlattr *ftm,
> > > > +                                        struct cfg80211_pmsr_ftm_result *result,
> > > > +                                        struct genl_info *info)
> > > > +{
> > > > +     struct nlattr *tb[NL80211_PMSR_FTM_RESP_ATTR_MAX + 1];
> > > > +     int ret;
> > > > +
> > > > +     ret = nla_parse_nested(tb, NL80211_PMSR_FTM_RESP_ATTR_MAX,
> > > > +                            ftm, hwsim_ftm_result_policy, info->extack);
> > > > +     if (ret)
> > > > +             return ret;
> > > > +
> > > > +     if (tb[NL80211_PMSR_FTM_RESP_ATTR_FAIL_REASON])
> > > > +             result->failure_reason = nla_get_u32(tb[NL80211_PMSR_FTM_RESP_ATTR_FAIL_REASON]);
> > > > +
> > > > +     if (tb[NL80211_PMSR_FTM_RESP_ATTR_BURST_INDEX])
> > > > +             result->burst_index = nla_get_u16(tb[NL80211_PMSR_FTM_RESP_ATTR_BURST_INDEX]);
> > > > +
> > > > +     if (tb[NL80211_PMSR_FTM_RESP_ATTR_NUM_FTMR_ATTEMPTS]) {
> > > > +             result->num_ftmr_attempts_valid = 1;
> > > > +             result->num_ftmr_attempts =
> > > > +                     nla_get_u32(tb[NL80211_PMSR_FTM_RESP_ATTR_NUM_FTMR_ATTEMPTS]);
> > > > +     }
> > > > +
> > > > +     if (tb[NL80211_PMSR_FTM_RESP_ATTR_NUM_FTMR_SUCCESSES]) {
> > > > +             result->num_ftmr_successes_valid = 1;
> > > > +             result->num_ftmr_successes =
> > > > +                     nla_get_u32(tb[NL80211_PMSR_FTM_RESP_ATTR_NUM_FTMR_SUCCESSES]);
> > > > +     }
> > > > +
> > > > +     if (tb[NL80211_PMSR_FTM_RESP_ATTR_BUSY_RETRY_TIME])
> > > > +             result->busy_retry_time =
> > > > +                     nla_get_u8(tb[NL80211_PMSR_FTM_RESP_ATTR_BUSY_RETRY_TIME]);
> > > > +
> > > > +     if (tb[NL80211_PMSR_FTM_RESP_ATTR_NUM_BURSTS_EXP])
> > > > +             result->num_bursts_exp = nla_get_u8(tb[NL80211_PMSR_FTM_RESP_ATTR_NUM_BURSTS_EXP]);
> > > > +
> > > > +     if (tb[NL80211_PMSR_FTM_RESP_ATTR_BURST_DURATION])
> > > > +             result->burst_duration = nla_get_u8(tb[NL80211_PMSR_FTM_RESP_ATTR_BURST_DURATION]);
> > > > +
> > > > +     if (tb[NL80211_PMSR_FTM_RESP_ATTR_FTMS_PER_BURST])
> > > > +             result->ftms_per_burst = nla_get_u8(tb[NL80211_PMSR_FTM_RESP_ATTR_FTMS_PER_BURST]);
> > > > +
> > > > +     if (tb[NL80211_PMSR_FTM_RESP_ATTR_RSSI_AVG]) {
> > > > +             result->rssi_avg_valid = 1;
> > > > +             result->rssi_avg = nla_get_s32(tb[NL80211_PMSR_FTM_RESP_ATTR_RSSI_AVG]);
> > > > +     }
> > > > +     if (tb[NL80211_PMSR_FTM_RESP_ATTR_RSSI_SPREAD]) {
> > > > +             result->rssi_spread_valid = 1;
> > > > +             result->rssi_spread =
> > > > +                     nla_get_s32(tb[NL80211_PMSR_FTM_RESP_ATTR_RSSI_SPREAD]);
> > > > +     }
> > > > +
> > > > +     if (tb[NL80211_PMSR_FTM_RESP_ATTR_TX_RATE]) {
> > > > +             result->tx_rate_valid = 1;
> > > > +             ret = mac80211_hwsim_parse_rate_info(tb[NL80211_PMSR_FTM_RESP_ATTR_TX_RATE],
> > > > +                                                  &result->tx_rate, info);
> > > > +             if (ret)
> > > > +                     return ret;
> > > > +     }
> > > > +
> > > > +     if (tb[NL80211_PMSR_FTM_RESP_ATTR_RX_RATE]) {
> > > > +             result->rx_rate_valid = 1;
> > > > +             ret = mac80211_hwsim_parse_rate_info(tb[NL80211_PMSR_FTM_RESP_ATTR_RX_RATE],
> > > > +                                                  &result->rx_rate, info);
> > > > +             if (ret)
> > > > +                     return ret;
> > > > +     }
> > > > +
> > > > +     if (tb[NL80211_PMSR_FTM_RESP_ATTR_RTT_AVG]) {
> > > > +             result->rtt_avg_valid = 1;
> > > > +             result->rtt_avg =
> > > > +                     nla_get_u64(tb[NL80211_PMSR_FTM_RESP_ATTR_RTT_AVG]);
> > > > +     }
> > > > +     if (tb[NL80211_PMSR_FTM_RESP_ATTR_RTT_VARIANCE]) {
> > > > +             result->rtt_variance_valid = 1;
> > > > +             result->rtt_variance =
> > > > +                     nla_get_u64(tb[NL80211_PMSR_FTM_RESP_ATTR_RTT_VARIANCE]);
> > > > +     }
> > > > +     if (tb[NL80211_PMSR_FTM_RESP_ATTR_RTT_SPREAD]) {
> > > > +             result->rtt_spread_valid = 1;
> > > > +             result->rtt_spread =
> > > > +                     nla_get_u64(tb[NL80211_PMSR_FTM_RESP_ATTR_RTT_SPREAD]);
> > > > +     }
> > > > +     if (tb[NL80211_PMSR_FTM_RESP_ATTR_DIST_AVG]) {
> > > > +             result->dist_avg_valid = 1;
> > > > +             result->dist_avg =
> > > > +                     nla_get_u64(tb[NL80211_PMSR_FTM_RESP_ATTR_DIST_AVG]);
> > > > +     }
> > > > +     if (tb[NL80211_PMSR_FTM_RESP_ATTR_DIST_VARIANCE]) {
> > > > +             result->dist_variance_valid = 1;
> > > > +             result->dist_variance =
> > > > +                     nla_get_u64(tb[NL80211_PMSR_FTM_RESP_ATTR_DIST_VARIANCE]);
> > > > +     }
> > > > +     if (tb[NL80211_PMSR_FTM_RESP_ATTR_DIST_SPREAD]) {
> > > > +             result->dist_spread_valid = 1;
> > > > +             result->dist_spread =
> > > > +                     nla_get_u64(tb[NL80211_PMSR_FTM_RESP_ATTR_DIST_SPREAD]);
> > > > +     }
> > > > +
> > > > +     if (tb[NL80211_PMSR_FTM_RESP_ATTR_LCI]) {
> > > > +             result->lci = nla_data(tb[NL80211_PMSR_FTM_RESP_ATTR_LCI]);
> > > > +             result->lci_len = nla_len(tb[NL80211_PMSR_FTM_RESP_ATTR_LCI]);
> > > > +     }
> > > > +
> > > > +     if (tb[NL80211_PMSR_FTM_RESP_ATTR_CIVICLOC]) {
> > > > +             result->civicloc = nla_data(tb[NL80211_PMSR_FTM_RESP_ATTR_CIVICLOC]);
> > > > +             result->civicloc_len = nla_len(tb[NL80211_PMSR_FTM_RESP_ATTR_CIVICLOC]);
> > > > +     }
> > > > +
> > > > +     return 0;
> > > > +}
> > > > +
> > > > +static int mac80211_hwsim_parse_pmsr_resp(struct nlattr *resp,
> > > > +                                       struct cfg80211_pmsr_result *result,
> > > > +                                       struct genl_info *info)
> > > > +{
> > > > +     struct nlattr *tb[NL80211_PMSR_RESP_ATTR_MAX + 1];
> > > > +     struct nlattr *pmsr;
> > > > +     int rem;
> > > > +     int ret;
> > > > +
> > > > +     ret = nla_parse_nested(tb, NL80211_PMSR_RESP_ATTR_MAX, resp,
> > > > +                            hwsim_pmsr_resp_policy, info->extack);
> > >
> > > You are assigning the value to "ret" variable but you are never using
> > > it. Is the check for "ret" missing?
> > >
> > > > +
> > > > +     if (tb[NL80211_PMSR_RESP_ATTR_STATUS])
> > > > +             result->status = nla_get_u32(tb[NL80211_PMSR_RESP_ATTR_STATUS]);
> > > > +
> > > > +     if (tb[NL80211_PMSR_RESP_ATTR_HOST_TIME])
> > > > +             result->host_time = nla_get_u64(tb[NL80211_PMSR_RESP_ATTR_HOST_TIME]);
> > > > +
> > > > +     if (tb[NL80211_PMSR_RESP_ATTR_AP_TSF]) {
> > > > +             result->ap_tsf_valid = 1;
> > > > +             result->ap_tsf = nla_get_u64(tb[NL80211_PMSR_RESP_ATTR_AP_TSF]);
> > > > +     }
> > > > +
> > > > +     result->final = !!tb[NL80211_PMSR_RESP_ATTR_FINAL];
> > > > +
> > > > +     if (tb[NL80211_PMSR_RESP_ATTR_DATA]) {
> > >
> > > How about using a negative logic in here to decrease indentation?
> > > For example:
> > >
> > >         if (!tb[NL80211_PMSR_RESP_ATTR_DATA])
> > >                 return ret;
> > >
> > > > +             nla_for_each_nested(pmsr, tb[NL80211_PMSR_RESP_ATTR_DATA], rem) {
> > > > +                     switch (nla_type(pmsr)) {
> > > > +                     case NL80211_PMSR_TYPE_FTM:
> > > > +                             result->type = NL80211_PMSR_TYPE_FTM;
> > > > +                             ret = mac80211_hwsim_parse_ftm_result(pmsr, &result->ftm, info);
> > > > +                             if (ret)
> > > > +                                     return ret;
> > > > +                             break;
> > > > +                     default:
> > > > +                             NL_SET_ERR_MSG_ATTR(info->extack, pmsr, "Unknown pmsr resp type");
> > > > +                             return -EINVAL;
> > > > +                     }
> > > > +             }
> > > > +     }
> > > > +
> > > > +     return 0;
> > > > +}
> > > > +
> > > > +static int mac80211_hwsim_parse_pmsr_result(struct nlattr *peer,
> > > > +                                         struct cfg80211_pmsr_result *result,
> > > > +                                         struct genl_info *info)
> > > > +{
> > > > +     struct nlattr *tb[NL80211_PMSR_PEER_ATTR_MAX + 1];
> > > > +     int ret;
> > > > +
> > > > +     if (!peer)
> > > > +             return -EINVAL;
> > > > +
> > > > +     ret = nla_parse_nested(tb, NL80211_PMSR_PEER_ATTR_MAX, peer,
> > > > +                            hwsim_pmsr_peer_result_policy, info->extack);
> > > > +     if (ret)
> > > > +             return ret;
> > > > +
> > > > +     if (tb[NL80211_PMSR_PEER_ATTR_ADDR])
> > > > +             memcpy(result->addr, nla_data(tb[NL80211_PMSR_PEER_ATTR_ADDR]),
> > > > +                    ETH_ALEN);
> > > > +
> > > > +     if (tb[NL80211_PMSR_PEER_ATTR_RESP]) {
> > > > +             ret = mac80211_hwsim_parse_pmsr_resp(tb[NL80211_PMSR_PEER_ATTR_RESP], result, info);
> > > > +             if (ret)
> > > > +                     return ret;
> > > > +     }
> > > > +
> > > > +     return 0;
> > > > +};
> > > > +
> > > > +static int hwsim_pmsr_report_nl(struct sk_buff *msg, struct genl_info *info)
> > > > +{
> > > > +     struct nlattr *reqattr;
> > > > +     const u8 *src;
> > > > +     int err, rem;
> > > > +     struct nlattr *peers, *peer;
> > > > +     struct mac80211_hwsim_data *data;
> > >
> > > Please use RCT formatting.
> > >
> > > > +
> > > > +     src = nla_data(info->attrs[HWSIM_ATTR_ADDR_TRANSMITTER]);
> > > > +     data = get_hwsim_data_ref_from_addr(src);
> > > > +     if (!data)
> > > > +             return -EINVAL;
> > > > +
> > > > +     mutex_lock(&data->mutex);
> > > > +     if (!data->pmsr_request) {
> > > > +             err = -EINVAL;
> > > > +             goto out_err;
> > > > +     }
> > > > +
> > > > +     reqattr = info->attrs[HWSIM_ATTR_PMSR_RESULT];
> > > > +     if (!reqattr) {
> > > > +             err = -EINVAL;
> > > > +             goto out_err;
> > > > +     }
> > > > +
> > > > +     peers = nla_find_nested(reqattr, NL80211_PMSR_ATTR_PEERS);
> > > > +     if (!peers) {
> > > > +             err = -EINVAL;
> > > > +             goto out_err;
> > > > +     }
> > > > +
> > > > +     nla_for_each_nested(peer, peers, rem) {
> > > > +             struct cfg80211_pmsr_result result;
> > > > +
> > > > +             err = mac80211_hwsim_parse_pmsr_result(peer, &result, info);
> > > > +             if (err)
> > > > +                     goto out_err;
> > > > +
> > > > +             cfg80211_pmsr_report(data->pmsr_request_wdev,
> > > > +                                  data->pmsr_request, &result, GFP_KERNEL);
> > > > +     }
> > > > +
> > > > +     cfg80211_pmsr_complete(data->pmsr_request_wdev, data->pmsr_request, GFP_KERNEL);
> > > > +
> > > > +out_err:
> > >
> > > How about renaming this label to "out" or "exit"?
> > > The code below is used for error path as well as for a normal path.
> > >
> > > > +     data->pmsr_request = NULL;
> > > > +     data->pmsr_request_wdev = NULL;
> > > > +
> > > > +     mutex_unlock(&data->mutex);
> > > > +     return err;
> > > > +}
> > > > +
> > > >  #define HWSIM_COMMON_OPS                                     \
> > > >       .tx = mac80211_hwsim_tx,                                \
> > > >       .wake_tx_queue = ieee80211_handle_wake_tx_queue,        \
> > > > @@ -5072,13 +5434,6 @@ static void hwsim_mon_setup(struct net_device *dev)
> > > >       eth_hw_addr_set(dev, addr);
> > > >  }
> > > >
> > > > -static struct mac80211_hwsim_data *get_hwsim_data_ref_from_addr(const u8 *addr)
> > > > -{
> > > > -     return rhashtable_lookup_fast(&hwsim_radios_rht,
> > > > -                                   addr,
> > > > -                                   hwsim_rht_params);
> > > > -}
> > > > -
> > > >  static void hwsim_register_wmediumd(struct net *net, u32 portid)
> > > >  {
> > > >       struct mac80211_hwsim_data *data;
> > > > @@ -5746,6 +6101,11 @@ static const struct genl_small_ops hwsim_ops[] = {
> > > >               .doit = hwsim_get_radio_nl,
> > > >               .dumpit = hwsim_dump_radio_nl,
> > > >       },
> > > > +     {
> > > > +             .cmd = HWSIM_CMD_REPORT_PMSR,
> > > > +             .validate = GENL_DONT_VALIDATE_STRICT | GENL_DONT_VALIDATE_DUMP,
> > > > +             .doit = hwsim_pmsr_report_nl,
> > > > +     },
> > > >  };
> > > >
> > > >  static struct genl_family hwsim_genl_family __ro_after_init = {
> > > > @@ -5757,7 +6117,7 @@ static struct genl_family hwsim_genl_family __ro_after_init = {
> > > >       .module = THIS_MODULE,
> > > >       .small_ops = hwsim_ops,
> > > >       .n_small_ops = ARRAY_SIZE(hwsim_ops),
> > > > -     .resv_start_op = HWSIM_CMD_DEL_MAC_ADDR + 1,
> > > > +     .resv_start_op = HWSIM_CMD_REPORT_PMSR + 1, // match with __HWSIM_CMD_MAX
> > >
> > >
> > > >       .mcgrps = hwsim_mcgrps,
> > > >       .n_mcgrps = ARRAY_SIZE(hwsim_mcgrps),
> > > >  };
> > > > @@ -5926,6 +6286,9 @@ static int hwsim_virtio_handle_cmd(struct sk_buff *skb)
> > > >       case HWSIM_CMD_TX_INFO_FRAME:
> > > >               hwsim_tx_info_frame_received_nl(skb, &info);
> > > >               break;
> > > > +     case HWSIM_CMD_REPORT_PMSR:
> > > > +             hwsim_pmsr_report_nl(skb, &info);
> > > > +             break;
> > > >       default:
> > > >               pr_err_ratelimited("hwsim: invalid cmd: %d\n", gnlh->cmd);
> > > >               return -EPROTO;
> > > > diff --git a/drivers/net/wireless/mac80211_hwsim.h b/drivers/net/wireless/mac80211_hwsim.h
> > > > index 383f3e39c911..92126f02c58f 100644
> > > > --- a/drivers/net/wireless/mac80211_hwsim.h
> > > > +++ b/drivers/net/wireless/mac80211_hwsim.h
> > > > @@ -82,8 +82,8 @@ enum hwsim_tx_control_flags {
> > > >   * @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_ABORT_PMSR: abort previously sent peer measurement
> > > > + *   %HWSIM_ATTR_PMSR_REQUEST. Result will be sent back asynchronously
> > > > + *   with %HWSIM_CMD_REPORT_PMSR.
> > > >   * @__HWSIM_CMD_MAX: enum limit
> > > >   */
> > > >  enum {
> > > > @@ -98,6 +98,7 @@ enum {
> > > >       HWSIM_CMD_DEL_MAC_ADDR,
> > > >       HWSIM_CMD_START_PMSR,
> > > >       HWSIM_CMD_ABORT_PMSR,
> > > > +     HWSIM_CMD_REPORT_PMSR,
> > > >       __HWSIM_CMD_MAX,
> > > >  };
> > > >  #define HWSIM_CMD_MAX (_HWSIM_CMD_MAX - 1)
> > > > @@ -151,6 +152,8 @@ enum {
> > > >   *   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_PMSR_RESULT: nested attributed used with %HWSIM_CMD_REPORT_PMSR
> > > > + *   to provide peer measurement result (nl80211_peer_measurement_attrs)
> > > >   * @__HWSIM_ATTR_MAX: enum limit
> > > >   */
> > > >
> > > > @@ -184,6 +187,7 @@ enum {
> > > >       HWSIM_ATTR_MLO_SUPPORT,
> > > >       HWSIM_ATTR_PMSR_SUPPORT,
> > > >       HWSIM_ATTR_PMSR_REQUEST,
> > > > +     HWSIM_ATTR_PMSR_RESULT,
> > > >       __HWSIM_ATTR_MAX,
> > > >  };
> > > >  #define HWSIM_ATTR_MAX (__HWSIM_ATTR_MAX - 1)
> > > > @@ -288,4 +292,47 @@ enum {
> > > >       HWSIM_VQ_RX,
> > > >       HWSIM_NUM_VQS,
> > > >  };
> > > > +
> > > > +/**
> > > > + * enum hwsim_rate_info -- bitrate information.
> > > > + *
> > > > + * Information about a receiving or transmitting bitrate
> > > > + * that can be mapped to struct rate_info
> > > > + *
> > > > + * @HWSIM_RATE_INFO_ATTR_FLAGS: bitflag of flags from &enum rate_info_flags
> > > > + * @HWSIM_RATE_INFO_ATTR_MCS: mcs index if struct describes an HT/VHT/HE rate
> > > > + * @HWSIM_RATE_INFO_ATTR_LEGACY: bitrate in 100kbit/s for 802.11abg
> > > > + * @HWSIM_RATE_INFO_ATTR_NSS: number of streams (VHT & HE only)
> > > > + * @HWSIM_RATE_INFO_ATTR_BW: bandwidth (from &enum rate_info_bw)
> > > > + * @HWSIM_RATE_INFO_ATTR_HE_GI: HE guard interval (from &enum nl80211_he_gi)
> > > > + * @HWSIM_RATE_INFO_ATTR_HE_DCM: HE DCM value
> > > > + * @HWSIM_RATE_INFO_ATTR_HE_RU_ALLOC:  HE RU allocation (from &enum nl80211_he_ru_alloc,
> > > > + *   only valid if bw is %RATE_INFO_BW_HE_RU)
> > > > + * @HWSIM_RATE_INFO_ATTR_N_BOUNDED_CH: In case of EDMG the number of bonded channels (1-4)
> > > > + * @HWSIM_RATE_INFO_ATTR_EHT_GI: EHT guard interval (from &enum nl80211_eht_gi)
> > > > + * @HWSIM_RATE_INFO_ATTR_EHT_RU_ALLOC: EHT RU allocation (from &enum nl80211_eht_ru_alloc,
> > > > + *   only valid if bw is %RATE_INFO_BW_EHT_RU)
> > > > + * @NUM_HWSIM_RATE_INFO_ATTRS: internal
> > > > + * @HWSIM_RATE_INFO_ATTR_MAX: highest attribute number
> > > > + */
> > > > +enum hwsim_rate_info_attributes {
> > > > +     __HWSIM_RATE_INFO_ATTR_INVALID,
> > > > +
> > > > +     HWSIM_RATE_INFO_ATTR_FLAGS,
> > > > +     HWSIM_RATE_INFO_ATTR_MCS,
> > > > +     HWSIM_RATE_INFO_ATTR_LEGACY,
> > > > +     HWSIM_RATE_INFO_ATTR_NSS,
> > > > +     HWSIM_RATE_INFO_ATTR_BW,
> > > > +     HWSIM_RATE_INFO_ATTR_HE_GI,
> > > > +     HWSIM_RATE_INFO_ATTR_HE_DCM,
> > > > +     HWSIM_RATE_INFO_ATTR_HE_RU_ALLOC,
> > > > +     HWSIM_RATE_INFO_ATTR_N_BOUNDED_CH,
> > > > +     HWSIM_RATE_INFO_ATTR_EHT_GI,
> > > > +     HWSIM_RATE_INFO_ATTR_EHT_RU_ALLOC,
> > > > +
> > > > +     /* keep last */
> > > > +     NUM_HWSIM_RATE_INFO_ATTRS,
> > > > +     HWSIM_RATE_INFO_ATTR_MAX = NUM_HWSIM_RATE_INFO_ATTRS - 1
> > > > +};
> > > > +
> > > >  #endif /* __MAC80211_HWSIM_H */
> > > > --
> > > > 2.40.0.rc1.284.g88254d51c5-goog
> > > >
> >
> > Many Thanks,
> >
> > --
> > Jaewan Kim (김재완) | Software Engineer in Google Korea |
> > jaewan@google.com | +82-10-2781-5078

Done for everything except for line limits.
Thank you for the reviews.


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