| Message ID | 20230313075326.3594869-1-jaewan@google.com |
|---|---|
| Headers | show |
| Series | mac80211_hwsim: Add PMSR support | expand |
On Wed, Mar 15, 2023 at 01:36:02AM +0900, Jaewan Kim wrote: > On Tue, Mar 14, 2023 at 1:52 AM Michal Kubiak <michal.kubiak@intel.com> wrote: > > > > On Mon, Mar 13, 2023 at 07:53:22AM +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 functionality to allow mac80211_hwsim to be configured with > > > PMSR capability. The capability is mandatory to accept incoming PMSR > > > request because nl80211_pmsr_start() ignores incoming the request without > > > the PMSR capability. > > > > > > In detail, add new mac80211_hwsim attribute HWSIM_ATTR_PMSR_SUPPORT. > > > HWSIM_ATTR_PMSR_SUPPORT is used to set PMSR capability when creating a new > > > radio. To send extra capability details, HWSIM_ATTR_PMSR_SUPPORT can have > > > nested PMSR capability attributes defined in the nl80211.h. Data format is > > > the same as cfg80211_pmsr_capabilities. > > > > > > If HWSIM_ATTR_PMSR_SUPPORT is specified, mac80211_hwsim builds > > > cfg80211_pmsr_capabilities and sets wiphy.pmsr_capa. > > > > > > Signed-off-by: Jaewan Kim <jaewan@google.com> > > > > Hi, > > > > Just a few style comments and suggestions. > > > > Thanks, > > Michal > > > > > --- > > > V8 -> V9: Changed to consider unknown PMSR type as error. > > > V7 -> V8: Changed not to send pmsr_capa when adding new radio to limit > > > exporting cfg80211 function to driver. > > > V6 -> V7: Added terms definitions. Removed pr_*() uses. > > > V5 -> V6: Added per change patch history. > > > V4 -> V5: Fixed style for commit messages. > > > V3 -> V4: Added change details for new attribute, and fixed memory leak. > > > V1 -> V3: Initial commit (includes resends). > > > --- > > > drivers/net/wireless/mac80211_hwsim.c | 129 +++++++++++++++++++++++++- > > > drivers/net/wireless/mac80211_hwsim.h | 3 + > > > 2 files changed, 131 insertions(+), 1 deletion(-) > > > > > > diff --git a/drivers/net/wireless/mac80211_hwsim.c b/drivers/net/wireless/mac80211_hwsim.c > > > index 4cc4eaf80b14..65868f28a00f 100644 > > > --- a/drivers/net/wireless/mac80211_hwsim.c > > > +++ b/drivers/net/wireless/mac80211_hwsim.c > > > @@ -719,6 +719,9 @@ struct mac80211_hwsim_data { > > > /* RSSI in rx status of the receiver */ > > > int rx_rssi; > > > > > > + /* only used when pmsr capability is supplied */ > > > + struct cfg80211_pmsr_capabilities pmsr_capa; > > > + > > > struct mac80211_hwsim_link_data link_data[IEEE80211_MLD_MAX_NUM_LINKS]; > > > }; > > > > > > @@ -760,6 +763,34 @@ static const struct genl_multicast_group hwsim_mcgrps[] = { > > > > > > /* MAC80211_HWSIM netlink 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 }, > > > + [NL80211_PMSR_FTM_CAPA_ATTR_NON_ASAP] = { .type = NLA_FLAG }, > > > + [NL80211_PMSR_FTM_CAPA_ATTR_REQ_LCI] = { .type = NLA_FLAG }, > > > + [NL80211_PMSR_FTM_CAPA_ATTR_REQ_CIVICLOC] = { .type = NLA_FLAG }, > > > + [NL80211_PMSR_FTM_CAPA_ATTR_PREAMBLES] = { .type = NLA_U32 }, > > > + [NL80211_PMSR_FTM_CAPA_ATTR_BANDWIDTHS] = { .type = NLA_U32 }, > > > + [NL80211_PMSR_FTM_CAPA_ATTR_MAX_BURSTS_EXPONENT] = NLA_POLICY_MAX(NLA_U8, 15), > > > + [NL80211_PMSR_FTM_CAPA_ATTR_MAX_FTMS_PER_BURST] = NLA_POLICY_MAX(NLA_U8, 31), > > > + [NL80211_PMSR_FTM_CAPA_ATTR_TRIGGER_BASED] = { .type = NLA_FLAG }, > > > + [NL80211_PMSR_FTM_CAPA_ATTR_NON_TRIGGER_BASED] = { .type = NLA_FLAG }, > > > +}; > > > + > > > +static const struct nla_policy > > > +hwsim_pmsr_capa_type_policy[NL80211_PMSR_TYPE_MAX + 1] = { > > > + [NL80211_PMSR_TYPE_FTM] = NLA_POLICY_NESTED(hwsim_ftm_capa_policy), > > > +}; > > > + > > > +static const struct nla_policy > > > +hwsim_pmsr_capa_policy[NL80211_PMSR_ATTR_MAX + 1] = { > > > + [NL80211_PMSR_ATTR_MAX_PEERS] = { .type = NLA_U32 }, > > > + [NL80211_PMSR_ATTR_REPORT_AP_TSF] = { .type = NLA_FLAG }, > > > + [NL80211_PMSR_ATTR_RANDOMIZE_MAC_ADDR] = { .type = NLA_FLAG }, > > > + [NL80211_PMSR_ATTR_TYPE_CAPA] = NLA_POLICY_NESTED(hwsim_pmsr_capa_type_policy), > > > + [NL80211_PMSR_ATTR_PEERS] = { .type = NLA_REJECT }, // only for request. > > > +}; > > > + > > > static const struct nla_policy hwsim_genl_policy[HWSIM_ATTR_MAX + 1] = { > > > [HWSIM_ATTR_ADDR_RECEIVER] = NLA_POLICY_ETH_ADDR_COMPAT, > > > [HWSIM_ATTR_ADDR_TRANSMITTER] = NLA_POLICY_ETH_ADDR_COMPAT, > > > @@ -788,6 +819,7 @@ static const struct nla_policy hwsim_genl_policy[HWSIM_ATTR_MAX + 1] = { > > > [HWSIM_ATTR_IFTYPE_SUPPORT] = { .type = NLA_U32 }, > > > [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), > > > }; > > > > > > #if IS_REACHABLE(CONFIG_VIRTIO) > > > @@ -3186,6 +3218,7 @@ struct hwsim_new_radio_params { > > > u32 *ciphers; > > > u8 n_ciphers; > > > bool mlo; > > > + const struct cfg80211_pmsr_capabilities *pmsr_capa; > > > }; > > > > > > static void hwsim_mcast_config_msg(struct sk_buff *mcast_skb, > > > @@ -3260,7 +3293,7 @@ static int append_radio_msg(struct sk_buff *skb, int id, > > > return ret; > > > } > > > > > > - return 0; > > > + return ret; > > > } > > > > > > static void hwsim_mcast_new_radio(int id, struct genl_info *info, > > > @@ -4445,6 +4478,7 @@ static int mac80211_hwsim_new_radio(struct genl_info *info, > > > NL80211_EXT_FEATURE_MULTICAST_REGISTRATIONS); > > > wiphy_ext_feature_set(hw->wiphy, > > > NL80211_EXT_FEATURE_BEACON_RATE_LEGACY); > > > + wiphy_ext_feature_set(hw->wiphy, NL80211_EXT_FEATURE_ENABLE_FTM_RESPONDER); > > > > > > hw->wiphy->interface_modes = param->iftypes; > > > > > > @@ -4606,6 +4640,11 @@ static int mac80211_hwsim_new_radio(struct genl_info *info, > > > data->debugfs, > > > data, &hwsim_simulate_radar); > > > > > > + if (param->pmsr_capa) { > > > + data->pmsr_capa = *param->pmsr_capa; > > > + hw->wiphy->pmsr_capa = &data->pmsr_capa; > > > + } > > > + > > > spin_lock_bh(&hwsim_radio_lock); > > > err = rhashtable_insert_fast(&hwsim_radios_rht, &data->rht, > > > hwsim_rht_params); > > > @@ -4715,6 +4754,7 @@ static int mac80211_hwsim_get_radio(struct sk_buff *skb, > > > param.regd = data->regd; > > > param.channels = data->channels; > > > param.hwname = wiphy_name(data->hw->wiphy); > > > + param.pmsr_capa = &data->pmsr_capa; > > > > > > res = append_radio_msg(skb, data->idx, ¶m); > > > if (res < 0) > > > @@ -5053,6 +5093,77 @@ static bool hwsim_known_ciphers(const u32 *ciphers, int n_ciphers) > > > return true; > > > } > > > > > > +static int parse_ftm_capa(const struct nlattr *ftm_capa, struct cfg80211_pmsr_capabilities *out, > > > + struct genl_info *info) > > > +{ > > > + struct nlattr *tb[NL80211_PMSR_FTM_CAPA_ATTR_MAX + 1]; > > > + int ret = nla_parse_nested(tb, NL80211_PMSR_FTM_CAPA_ATTR_MAX, > > > + ftm_capa, hwsim_ftm_capa_policy, NULL); > > > > I would suggest to split declaration and assignment here. It breaks the > > RCT principle and it is more likely to overlook "nla_parse_nested" call. > > I think it would improve the readability when we know that the parsing > > function can return an error. > > Thank you for the review, but what's the RCT principle? > I've searched Kernel documentation and also googled it but I couldn't > find a good match. > Could you elaborate on the details? > Most of your comments are related to the RCT, so I'd like to > understand what it is. > RCT stands for "reverse christmas tree" order of declaration. That means the longest declaration should go first and the shortest last. For example: struct very_long_structure_name *ptr; int abc, defgh, othername; long ret_code = 0; u32 a, b, c; u8 i; As far as I know, it is a good practice of coding style in networking. Thanks, Michal > > > > > + > > > + if (ret) { > > > + NL_SET_ERR_MSG_ATTR(info->extack, ftm_capa, "malformed FTM capability"); > > > + return -EINVAL; > > > + } > > > + > > > + out->ftm.supported = 1; > > > + if (tb[NL80211_PMSR_FTM_CAPA_ATTR_PREAMBLES]) > > > + out->ftm.preambles = nla_get_u32(tb[NL80211_PMSR_FTM_CAPA_ATTR_PREAMBLES]); > > > + if (tb[NL80211_PMSR_FTM_CAPA_ATTR_BANDWIDTHS]) > > > + out->ftm.bandwidths = nla_get_u32(tb[NL80211_PMSR_FTM_CAPA_ATTR_BANDWIDTHS]); > > > + if (tb[NL80211_PMSR_FTM_CAPA_ATTR_MAX_BURSTS_EXPONENT]) > > > + out->ftm.max_bursts_exponent = > > > + nla_get_u8(tb[NL80211_PMSR_FTM_CAPA_ATTR_MAX_BURSTS_EXPONENT]); > > > + if (tb[NL80211_PMSR_FTM_CAPA_ATTR_MAX_FTMS_PER_BURST]) > > > + out->ftm.max_ftms_per_burst = > > > + nla_get_u8(tb[NL80211_PMSR_FTM_CAPA_ATTR_MAX_FTMS_PER_BURST]); > > > + out->ftm.asap = !!tb[NL80211_PMSR_FTM_CAPA_ATTR_ASAP]; > > > + out->ftm.non_asap = !!tb[NL80211_PMSR_FTM_CAPA_ATTR_NON_ASAP]; > > > + out->ftm.request_lci = !!tb[NL80211_PMSR_FTM_CAPA_ATTR_REQ_LCI]; > > > + out->ftm.request_civicloc = !!tb[NL80211_PMSR_FTM_CAPA_ATTR_REQ_CIVICLOC]; > > > + out->ftm.trigger_based = !!tb[NL80211_PMSR_FTM_CAPA_ATTR_TRIGGER_BASED]; > > > + out->ftm.non_trigger_based = !!tb[NL80211_PMSR_FTM_CAPA_ATTR_NON_TRIGGER_BASED]; > > > + > > > + return 0; > > > +} > > > + > > > +static int parse_pmsr_capa(const struct nlattr *pmsr_capa, struct cfg80211_pmsr_capabilities *out, > > > + struct genl_info *info) > > > +{ > > > + struct nlattr *tb[NL80211_PMSR_ATTR_MAX + 1]; > > > + struct nlattr *nla; > > > + int size; > > > + int ret = nla_parse_nested(tb, NL80211_PMSR_ATTR_MAX, pmsr_capa, > > > + hwsim_pmsr_capa_policy, NULL); > > > > Ditto. > > > > > + > > > + if (ret) { > > > + NL_SET_ERR_MSG_ATTR(info->extack, pmsr_capa, "malformed PMSR capability"); > > > + return -EINVAL; > > > + } > > > + > > > + if (tb[NL80211_PMSR_ATTR_MAX_PEERS]) > > > + out->max_peers = nla_get_u32(tb[NL80211_PMSR_ATTR_MAX_PEERS]); > > > + out->report_ap_tsf = !!tb[NL80211_PMSR_ATTR_REPORT_AP_TSF]; > > > + out->randomize_mac_addr = !!tb[NL80211_PMSR_ATTR_RANDOMIZE_MAC_ADDR]; > > > + > > > + if (!tb[NL80211_PMSR_ATTR_TYPE_CAPA]) { > > > + NL_SET_ERR_MSG_ATTR(info->extack, tb[NL80211_PMSR_ATTR_TYPE_CAPA], > > > + "malformed PMSR type"); > > > + return -EINVAL; > > > + } > > > + > > > + nla_for_each_nested(nla, tb[NL80211_PMSR_ATTR_TYPE_CAPA], size) { > > > + switch (nla_type(nla)) { > > > + case NL80211_PMSR_TYPE_FTM: > > > + parse_ftm_capa(nla, out, info); > > > + break; > > > + default: > > > + NL_SET_ERR_MSG_ATTR(info->extack, nla, "unsupported measurement type"); > > > + return -EINVAL; > > > + } > > > + } > > > + return 0; > > > +} > > > + > > > static int hwsim_new_radio_nl(struct sk_buff *msg, struct genl_info *info) > > > { > > > struct hwsim_new_radio_params param = { 0 }; > > > @@ -5173,8 +5284,24 @@ static int hwsim_new_radio_nl(struct sk_buff *msg, struct genl_info *info) > > > param.hwname = hwname; > > > } > > > > > > + if (info->attrs[HWSIM_ATTR_PMSR_SUPPORT]) { > > > + struct cfg80211_pmsr_capabilities *pmsr_capa = > > > + kmalloc(sizeof(*pmsr_capa), GFP_KERNEL); > > > > Missing empty line after variable definition. > > BTW, would it not be better to split "pmsr_capa" declaration and > > "kmalloc"? For example: > > > > struct cfg80211_pmsr_capabilities *pmsr_capa; > > > > pmsr_capa = kmalloc(sizeof(*pmsr_capa), GFP_KERNEL); > > if (!pmsr_capa) { > > > > I think it would be more readable and you would not have to break the > > line. Also, in the current version it seems more likely that the memory > > allocation will be overlooked. > > > > > + if (!pmsr_capa) { > > > + ret = -ENOMEM; > > > + goto out_free; > > > + } > > > + ret = parse_pmsr_capa(info->attrs[HWSIM_ATTR_PMSR_SUPPORT], pmsr_capa, info); > > > + if (ret) > > > + goto out_free; > > > + param.pmsr_capa = pmsr_capa; > > > + } > > > + > > > ret = mac80211_hwsim_new_radio(info, ¶m); > > > + > > > +out_free: > > > kfree(hwname); > > > + kfree(param.pmsr_capa); > > > return ret; > > > } > > > > > > diff --git a/drivers/net/wireless/mac80211_hwsim.h b/drivers/net/wireless/mac80211_hwsim.h > > > index 527799b2de0f..d10fa7f4853b 100644 > > > --- a/drivers/net/wireless/mac80211_hwsim.h > > > +++ b/drivers/net/wireless/mac80211_hwsim.h > > > @@ -142,6 +142,8 @@ enum { > > > * @HWSIM_ATTR_CIPHER_SUPPORT: u32 array of supported cipher types > > > * @HWSIM_ATTR_MLO_SUPPORT: claim MLO support (exact parameters TBD) for > > > * 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_MAX: enum limit > > > */ > > > > > > @@ -173,6 +175,7 @@ enum { > > > HWSIM_ATTR_IFTYPE_SUPPORT, > > > HWSIM_ATTR_CIPHER_SUPPORT, > > > HWSIM_ATTR_MLO_SUPPORT, > > > + HWSIM_ATTR_PMSR_SUPPORT, > > > __HWSIM_ATTR_MAX, > > > }; > > > #define HWSIM_ATTR_MAX (__HWSIM_ATTR_MAX - 1) > > > -- > > > 2.40.0.rc1.284.g88254d51c5-goog > > > > > > > -- > Jaewan Kim (김재완) | Software Engineer in Google Korea | > jaewan@google.com | +82-10-2781-5078
On Wed, Mar 15, 2023 at 4:02 AM Michal Kubiak <michal.kubiak@intel.com> wrote: > > On Wed, Mar 15, 2023 at 01:36:02AM +0900, Jaewan Kim wrote: > > On Tue, Mar 14, 2023 at 1:52 AM Michal Kubiak <michal.kubiak@intel.com> wrote: > > > > > > On Mon, Mar 13, 2023 at 07:53:22AM +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 functionality to allow mac80211_hwsim to be configured with > > > > PMSR capability. The capability is mandatory to accept incoming PMSR > > > > request because nl80211_pmsr_start() ignores incoming the request without > > > > the PMSR capability. > > > > > > > > In detail, add new mac80211_hwsim attribute HWSIM_ATTR_PMSR_SUPPORT. > > > > HWSIM_ATTR_PMSR_SUPPORT is used to set PMSR capability when creating a new > > > > radio. To send extra capability details, HWSIM_ATTR_PMSR_SUPPORT can have > > > > nested PMSR capability attributes defined in the nl80211.h. Data format is > > > > the same as cfg80211_pmsr_capabilities. > > > > > > > > If HWSIM_ATTR_PMSR_SUPPORT is specified, mac80211_hwsim builds > > > > cfg80211_pmsr_capabilities and sets wiphy.pmsr_capa. > > > > > > > > Signed-off-by: Jaewan Kim <jaewan@google.com> > > > > > > Hi, > > > > > > Just a few style comments and suggestions. > > > > > > Thanks, > > > Michal > > > > > > > --- > > > > V8 -> V9: Changed to consider unknown PMSR type as error. > > > > V7 -> V8: Changed not to send pmsr_capa when adding new radio to limit > > > > exporting cfg80211 function to driver. > > > > V6 -> V7: Added terms definitions. Removed pr_*() uses. > > > > V5 -> V6: Added per change patch history. > > > > V4 -> V5: Fixed style for commit messages. > > > > V3 -> V4: Added change details for new attribute, and fixed memory leak. > > > > V1 -> V3: Initial commit (includes resends). > > > > --- > > > > drivers/net/wireless/mac80211_hwsim.c | 129 +++++++++++++++++++++++++- > > > > drivers/net/wireless/mac80211_hwsim.h | 3 + > > > > 2 files changed, 131 insertions(+), 1 deletion(-) > > > > > > > > diff --git a/drivers/net/wireless/mac80211_hwsim.c b/drivers/net/wireless/mac80211_hwsim.c > > > > index 4cc4eaf80b14..65868f28a00f 100644 > > > > --- a/drivers/net/wireless/mac80211_hwsim.c > > > > +++ b/drivers/net/wireless/mac80211_hwsim.c > > > > @@ -719,6 +719,9 @@ struct mac80211_hwsim_data { > > > > /* RSSI in rx status of the receiver */ > > > > int rx_rssi; > > > > > > > > + /* only used when pmsr capability is supplied */ > > > > + struct cfg80211_pmsr_capabilities pmsr_capa; > > > > + > > > > struct mac80211_hwsim_link_data link_data[IEEE80211_MLD_MAX_NUM_LINKS]; > > > > }; > > > > > > > > @@ -760,6 +763,34 @@ static const struct genl_multicast_group hwsim_mcgrps[] = { > > > > > > > > /* MAC80211_HWSIM netlink 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 }, > > > > + [NL80211_PMSR_FTM_CAPA_ATTR_NON_ASAP] = { .type = NLA_FLAG }, > > > > + [NL80211_PMSR_FTM_CAPA_ATTR_REQ_LCI] = { .type = NLA_FLAG }, > > > > + [NL80211_PMSR_FTM_CAPA_ATTR_REQ_CIVICLOC] = { .type = NLA_FLAG }, > > > > + [NL80211_PMSR_FTM_CAPA_ATTR_PREAMBLES] = { .type = NLA_U32 }, > > > > + [NL80211_PMSR_FTM_CAPA_ATTR_BANDWIDTHS] = { .type = NLA_U32 }, > > > > + [NL80211_PMSR_FTM_CAPA_ATTR_MAX_BURSTS_EXPONENT] = NLA_POLICY_MAX(NLA_U8, 15), > > > > + [NL80211_PMSR_FTM_CAPA_ATTR_MAX_FTMS_PER_BURST] = NLA_POLICY_MAX(NLA_U8, 31), > > > > + [NL80211_PMSR_FTM_CAPA_ATTR_TRIGGER_BASED] = { .type = NLA_FLAG }, > > > > + [NL80211_PMSR_FTM_CAPA_ATTR_NON_TRIGGER_BASED] = { .type = NLA_FLAG }, > > > > +}; > > > > + > > > > +static const struct nla_policy > > > > +hwsim_pmsr_capa_type_policy[NL80211_PMSR_TYPE_MAX + 1] = { > > > > + [NL80211_PMSR_TYPE_FTM] = NLA_POLICY_NESTED(hwsim_ftm_capa_policy), > > > > +}; > > > > + > > > > +static const struct nla_policy > > > > +hwsim_pmsr_capa_policy[NL80211_PMSR_ATTR_MAX + 1] = { > > > > + [NL80211_PMSR_ATTR_MAX_PEERS] = { .type = NLA_U32 }, > > > > + [NL80211_PMSR_ATTR_REPORT_AP_TSF] = { .type = NLA_FLAG }, > > > > + [NL80211_PMSR_ATTR_RANDOMIZE_MAC_ADDR] = { .type = NLA_FLAG }, > > > > + [NL80211_PMSR_ATTR_TYPE_CAPA] = NLA_POLICY_NESTED(hwsim_pmsr_capa_type_policy), > > > > + [NL80211_PMSR_ATTR_PEERS] = { .type = NLA_REJECT }, // only for request. > > > > +}; > > > > + > > > > static const struct nla_policy hwsim_genl_policy[HWSIM_ATTR_MAX + 1] = { > > > > [HWSIM_ATTR_ADDR_RECEIVER] = NLA_POLICY_ETH_ADDR_COMPAT, > > > > [HWSIM_ATTR_ADDR_TRANSMITTER] = NLA_POLICY_ETH_ADDR_COMPAT, > > > > @@ -788,6 +819,7 @@ static const struct nla_policy hwsim_genl_policy[HWSIM_ATTR_MAX + 1] = { > > > > [HWSIM_ATTR_IFTYPE_SUPPORT] = { .type = NLA_U32 }, > > > > [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), > > > > }; > > > > > > > > #if IS_REACHABLE(CONFIG_VIRTIO) > > > > @@ -3186,6 +3218,7 @@ struct hwsim_new_radio_params { > > > > u32 *ciphers; > > > > u8 n_ciphers; > > > > bool mlo; > > > > + const struct cfg80211_pmsr_capabilities *pmsr_capa; > > > > }; > > > > > > > > static void hwsim_mcast_config_msg(struct sk_buff *mcast_skb, > > > > @@ -3260,7 +3293,7 @@ static int append_radio_msg(struct sk_buff *skb, int id, > > > > return ret; > > > > } > > > > > > > > - return 0; > > > > + return ret; > > > > } > > > > > > > > static void hwsim_mcast_new_radio(int id, struct genl_info *info, > > > > @@ -4445,6 +4478,7 @@ static int mac80211_hwsim_new_radio(struct genl_info *info, > > > > NL80211_EXT_FEATURE_MULTICAST_REGISTRATIONS); > > > > wiphy_ext_feature_set(hw->wiphy, > > > > NL80211_EXT_FEATURE_BEACON_RATE_LEGACY); > > > > + wiphy_ext_feature_set(hw->wiphy, NL80211_EXT_FEATURE_ENABLE_FTM_RESPONDER); > > > > > > > > hw->wiphy->interface_modes = param->iftypes; > > > > > > > > @@ -4606,6 +4640,11 @@ static int mac80211_hwsim_new_radio(struct genl_info *info, > > > > data->debugfs, > > > > data, &hwsim_simulate_radar); > > > > > > > > + if (param->pmsr_capa) { > > > > + data->pmsr_capa = *param->pmsr_capa; > > > > + hw->wiphy->pmsr_capa = &data->pmsr_capa; > > > > + } > > > > + > > > > spin_lock_bh(&hwsim_radio_lock); > > > > err = rhashtable_insert_fast(&hwsim_radios_rht, &data->rht, > > > > hwsim_rht_params); > > > > @@ -4715,6 +4754,7 @@ static int mac80211_hwsim_get_radio(struct sk_buff *skb, > > > > param.regd = data->regd; > > > > param.channels = data->channels; > > > > param.hwname = wiphy_name(data->hw->wiphy); > > > > + param.pmsr_capa = &data->pmsr_capa; > > > > > > > > res = append_radio_msg(skb, data->idx, ¶m); > > > > if (res < 0) > > > > @@ -5053,6 +5093,77 @@ static bool hwsim_known_ciphers(const u32 *ciphers, int n_ciphers) > > > > return true; > > > > } > > > > > > > > +static int parse_ftm_capa(const struct nlattr *ftm_capa, struct cfg80211_pmsr_capabilities *out, > > > > + struct genl_info *info) > > > > +{ > > > > + struct nlattr *tb[NL80211_PMSR_FTM_CAPA_ATTR_MAX + 1]; > > > > + int ret = nla_parse_nested(tb, NL80211_PMSR_FTM_CAPA_ATTR_MAX, > > > > + ftm_capa, hwsim_ftm_capa_policy, NULL); > > > > > > I would suggest to split declaration and assignment here. It breaks the > > > RCT principle and it is more likely to overlook "nla_parse_nested" call. > > > I think it would improve the readability when we know that the parsing > > > function can return an error. > > > > Thank you for the review, but what's the RCT principle? > > I've searched Kernel documentation and also googled it but I couldn't > > find a good match. > > Could you elaborate on the details? > > Most of your comments are related to the RCT, so I'd like to > > understand what it is. > > > > RCT stands for "reverse christmas tree" order of declaration. > That means the longest declaration should go first and the shortest last. > For example: > > struct very_long_structure_name *ptr; > int abc, defgh, othername; > long ret_code = 0; > u32 a, b, c; > u8 i; > > As far as I know, it is a good practice of coding style in networking. > > Thanks, > Michal > Thank you for the info. I managed to find the relevant information from netdev doc with the name RCS. https://www.kernel.org/doc/html//latest/process/maintainer-netdev.html Let me follow your suggestion to inherit style guide from netdev, although there isn't a lint check nor existing RCS style code in mac80211_hwsim. > > > > > > > + > > > > + if (ret) { > > > > + NL_SET_ERR_MSG_ATTR(info->extack, ftm_capa, "malformed FTM capability"); > > > > + return -EINVAL; > > > > + } > > > > + > > > > + out->ftm.supported = 1; > > > > + if (tb[NL80211_PMSR_FTM_CAPA_ATTR_PREAMBLES]) > > > > + out->ftm.preambles = nla_get_u32(tb[NL80211_PMSR_FTM_CAPA_ATTR_PREAMBLES]); > > > > + if (tb[NL80211_PMSR_FTM_CAPA_ATTR_BANDWIDTHS]) > > > > + out->ftm.bandwidths = nla_get_u32(tb[NL80211_PMSR_FTM_CAPA_ATTR_BANDWIDTHS]); > > > > + if (tb[NL80211_PMSR_FTM_CAPA_ATTR_MAX_BURSTS_EXPONENT]) > > > > + out->ftm.max_bursts_exponent = > > > > + nla_get_u8(tb[NL80211_PMSR_FTM_CAPA_ATTR_MAX_BURSTS_EXPONENT]); > > > > + if (tb[NL80211_PMSR_FTM_CAPA_ATTR_MAX_FTMS_PER_BURST]) > > > > + out->ftm.max_ftms_per_burst = > > > > + nla_get_u8(tb[NL80211_PMSR_FTM_CAPA_ATTR_MAX_FTMS_PER_BURST]); > > > > + out->ftm.asap = !!tb[NL80211_PMSR_FTM_CAPA_ATTR_ASAP]; > > > > + out->ftm.non_asap = !!tb[NL80211_PMSR_FTM_CAPA_ATTR_NON_ASAP]; > > > > + out->ftm.request_lci = !!tb[NL80211_PMSR_FTM_CAPA_ATTR_REQ_LCI]; > > > > + out->ftm.request_civicloc = !!tb[NL80211_PMSR_FTM_CAPA_ATTR_REQ_CIVICLOC]; > > > > + out->ftm.trigger_based = !!tb[NL80211_PMSR_FTM_CAPA_ATTR_TRIGGER_BASED]; > > > > + out->ftm.non_trigger_based = !!tb[NL80211_PMSR_FTM_CAPA_ATTR_NON_TRIGGER_BASED]; > > > > + > > > > + return 0; > > > > +} > > > > + > > > > +static int parse_pmsr_capa(const struct nlattr *pmsr_capa, struct cfg80211_pmsr_capabilities *out, > > > > + struct genl_info *info) > > > > +{ > > > > + struct nlattr *tb[NL80211_PMSR_ATTR_MAX + 1]; > > > > + struct nlattr *nla; > > > > + int size; > > > > + int ret = nla_parse_nested(tb, NL80211_PMSR_ATTR_MAX, pmsr_capa, > > > > + hwsim_pmsr_capa_policy, NULL); > > > > > > Ditto. > > > > > > > + > > > > + if (ret) { > > > > + NL_SET_ERR_MSG_ATTR(info->extack, pmsr_capa, "malformed PMSR capability"); > > > > + return -EINVAL; > > > > + } > > > > + > > > > + if (tb[NL80211_PMSR_ATTR_MAX_PEERS]) > > > > + out->max_peers = nla_get_u32(tb[NL80211_PMSR_ATTR_MAX_PEERS]); > > > > + out->report_ap_tsf = !!tb[NL80211_PMSR_ATTR_REPORT_AP_TSF]; > > > > + out->randomize_mac_addr = !!tb[NL80211_PMSR_ATTR_RANDOMIZE_MAC_ADDR]; > > > > + > > > > + if (!tb[NL80211_PMSR_ATTR_TYPE_CAPA]) { > > > > + NL_SET_ERR_MSG_ATTR(info->extack, tb[NL80211_PMSR_ATTR_TYPE_CAPA], > > > > + "malformed PMSR type"); > > > > + return -EINVAL; > > > > + } > > > > + > > > > + nla_for_each_nested(nla, tb[NL80211_PMSR_ATTR_TYPE_CAPA], size) { > > > > + switch (nla_type(nla)) { > > > > + case NL80211_PMSR_TYPE_FTM: > > > > + parse_ftm_capa(nla, out, info); > > > > + break; > > > > + default: > > > > + NL_SET_ERR_MSG_ATTR(info->extack, nla, "unsupported measurement type"); > > > > + return -EINVAL; > > > > + } > > > > + } > > > > + return 0; > > > > +} > > > > + > > > > static int hwsim_new_radio_nl(struct sk_buff *msg, struct genl_info *info) > > > > { > > > > struct hwsim_new_radio_params param = { 0 }; > > > > @@ -5173,8 +5284,24 @@ static int hwsim_new_radio_nl(struct sk_buff *msg, struct genl_info *info) > > > > param.hwname = hwname; > > > > } > > > > > > > > + if (info->attrs[HWSIM_ATTR_PMSR_SUPPORT]) { > > > > + struct cfg80211_pmsr_capabilities *pmsr_capa = > > > > + kmalloc(sizeof(*pmsr_capa), GFP_KERNEL); > > > > > > Missing empty line after variable definition. > > > BTW, would it not be better to split "pmsr_capa" declaration and > > > "kmalloc"? For example: > > > > > > struct cfg80211_pmsr_capabilities *pmsr_capa; > > > > > > pmsr_capa = kmalloc(sizeof(*pmsr_capa), GFP_KERNEL); > > > if (!pmsr_capa) { > > > > > > I think it would be more readable and you would not have to break the > > > line. Also, in the current version it seems more likely that the memory > > > allocation will be overlooked. > > > > > > > + if (!pmsr_capa) { > > > > + ret = -ENOMEM; > > > > + goto out_free; > > > > + } > > > > + ret = parse_pmsr_capa(info->attrs[HWSIM_ATTR_PMSR_SUPPORT], pmsr_capa, info); > > > > + if (ret) > > > > + goto out_free; > > > > + param.pmsr_capa = pmsr_capa; > > > > + } > > > > + > > > > ret = mac80211_hwsim_new_radio(info, ¶m); > > > > + > > > > +out_free: > > > > kfree(hwname); > > > > + kfree(param.pmsr_capa); > > > > return ret; > > > > } > > > > > > > > diff --git a/drivers/net/wireless/mac80211_hwsim.h b/drivers/net/wireless/mac80211_hwsim.h > > > > index 527799b2de0f..d10fa7f4853b 100644 > > > > --- a/drivers/net/wireless/mac80211_hwsim.h > > > > +++ b/drivers/net/wireless/mac80211_hwsim.h > > > > @@ -142,6 +142,8 @@ enum { > > > > * @HWSIM_ATTR_CIPHER_SUPPORT: u32 array of supported cipher types > > > > * @HWSIM_ATTR_MLO_SUPPORT: claim MLO support (exact parameters TBD) for > > > > * 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_MAX: enum limit > > > > */ > > > > > > > > @@ -173,6 +175,7 @@ enum { > > > > HWSIM_ATTR_IFTYPE_SUPPORT, > > > > HWSIM_ATTR_CIPHER_SUPPORT, > > > > HWSIM_ATTR_MLO_SUPPORT, > > > > + HWSIM_ATTR_PMSR_SUPPORT, > > > > __HWSIM_ATTR_MAX, > > > > }; > > > > #define HWSIM_ATTR_MAX (__HWSIM_ATTR_MAX - 1) > > > > -- > > > > 2.40.0.rc1.284.g88254d51c5-goog > > > > -- Jaewan Kim (김재완) | Software Engineer in Google Korea | jaewan@google.com | +82-10-2781-5078
Dear Kernel maintainers, First of all, thank you for spending your precious time for reviewing my changes. Let me propose series of patches for adding PMSR support in the mac80211_hwsim. PMSR (peer measurement) is generalized measurement between STAs, and currently FTM (fine time measurement or flight time measurement) is the one and only measurement. FTM measures the RTT (round trip time) and FTM can be used to measure distances between two STAs. RTT is often referred as 'measuring distance' as well. Kernel had already defined protocols for PMSR in the include/uapi/linux/nl80211.h and relevant parsing/sending code are in the net/wireless/pmsr.c, but they are only used in intel's iwlwifi driver. Patchset is tested with iw tool on Virtual Android device (a.k.a. Cuttlefish). Hope this explains my changes. Many Thanks, -- V8 -> V9: Removed any wrong words for patch. Changed to reject unknown PMSR types. V7 -> V8: Separated patch for exporting nl80211_send_chandef V6 -> V7: Split 'mac80211_hwsim: handle FTM requests with virtio' with three pieces V5 -> V6: Added per patch change history. V4 -> V5: Fixed style V3 -> V4: Added detailed explanation to cover letter and per patch commit messages, includes explanation of PMSR and FTM. Also fixed memory leak. V1 -> V3: Initial commits (include resends) Jaewan Kim (5): mac80211_hwsim: add PMSR capability support wifi: nl80211: make nl80211_send_chandef non-static mac80211_hwsim: add PMSR request support via virtio mac80211_hwsim: add PMSR abort support via virtio mac80211_hwsim: add PMSR report support via virtio drivers/net/wireless/mac80211_hwsim.c | 776 +++++++++++++++++++++++++- drivers/net/wireless/mac80211_hwsim.h | 58 ++ include/net/cfg80211.h | 9 + net/wireless/nl80211.c | 4 +- 4 files changed, 835 insertions(+), 12 deletions(-)