| Message ID | 20230313075326.3594869-5-jaewan@google.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | mac80211_hwsim: Add PMSR support | expand |
On Mon, Mar 13, 2023 at 07:53:25AM +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 necessary functionalities for mac80211_hwsim to abort previous PMSR > request. The abortion request is sent to the wmedium where the PMSR request > is actually handled. > > In detail, add new mac80211_hwsim command HWSIM_CMD_ABORT_PMSR. When > mac80211_hwsim receives the PMSR abortion request via > ieee80211_ops.abort_pmsr, the received cfg80211_pmsr_request is resent to > the wmediumd with command HWSIM_CMD_ABORT_PMSR and attribute > HWSIM_ATTR_PMSR_REQUEST. The attribute is formatted as the same way as > nl80211_pmsr_start() expects. > > Signed-off-by: Jaewan Kim <jaewan@google.com> > --- > V7 -> V8: Rewrote commit msg > V7: Initial commit (split from previously large patch) > --- > drivers/net/wireless/mac80211_hwsim.c | 61 +++++++++++++++++++++++++++ > drivers/net/wireless/mac80211_hwsim.h | 2 + > 2 files changed, 63 insertions(+) > > diff --git a/drivers/net/wireless/mac80211_hwsim.c b/drivers/net/wireless/mac80211_hwsim.c > index a692d9c95566..8f699dfab77a 100644 > --- a/drivers/net/wireless/mac80211_hwsim.c > +++ b/drivers/net/wireless/mac80211_hwsim.c > @@ -3343,6 +3343,66 @@ static int mac80211_hwsim_start_pmsr(struct ieee80211_hw *hw, > return err; > } > > +static void mac80211_hwsim_abort_pmsr(struct ieee80211_hw *hw, > + struct ieee80211_vif *vif, > + struct cfg80211_pmsr_request *request) > +{ > + struct mac80211_hwsim_data *data = hw->priv; > + u32 _portid = READ_ONCE(data->wmediumd); > + struct sk_buff *skb = NULL; > + int err = 0; > + void *msg_head; > + struct nlattr *pmsr; Please use RCT style. > + > + if (!_portid && !hwsim_virtio_enabled) > + return; > + > + mutex_lock(&data->mutex); > + > + if (data->pmsr_request != request) { > + err = -EINVAL; > + goto out_err; > + } > + > + if (err) > + return; Redundant code - err is always zero in this place, isn't it? > + > + skb = genlmsg_new(GENLMSG_DEFAULT_SIZE, GFP_KERNEL); > + if (!skb) > + return; Return from the function while the mutex is still locked. I guess 'goto' should be used here like for other checks in this function. > + > + msg_head = genlmsg_put(skb, 0, 0, &hwsim_genl_family, 0, HWSIM_CMD_ABORT_PMSR); > + > + if (nla_put(skb, HWSIM_ATTR_ADDR_TRANSMITTER, ETH_ALEN, data->addresses[1].addr)) > + goto out_err; > + > + pmsr = nla_nest_start(skb, HWSIM_ATTR_PMSR_REQUEST); > + if (!pmsr) { > + err = -ENOMEM; > + goto out_err; > + } > + > + err = mac80211_hwsim_send_pmsr_request(skb, request); > + if (err) > + goto out_err; > + > + err = nla_nest_end(skb, pmsr); > + if (err) > + goto out_err; > + > + genlmsg_end(skb, msg_head); > + if (hwsim_virtio_enabled) > + hwsim_tx_virtio(data, skb); > + else > + hwsim_unicast_netgroup(data, skb, _portid); > + > +out_err: > + if (err && skb) > + nlmsg_free(skb); I suggest to reorganize that code (or at least rename the label "out_err" to "out" maybe?) to separate error path and normal path more clearly. > + > + mutex_unlock(&data->mutex); > +} > + > #define HWSIM_COMMON_OPS \ > .tx = mac80211_hwsim_tx, \ > .wake_tx_queue = ieee80211_handle_wake_tx_queue, \ > @@ -3367,6 +3427,7 @@ static int mac80211_hwsim_start_pmsr(struct ieee80211_hw *hw, > .get_et_stats = mac80211_hwsim_get_et_stats, \ > .get_et_strings = mac80211_hwsim_get_et_strings, \ > .start_pmsr = mac80211_hwsim_start_pmsr, \ > + .abort_pmsr = mac80211_hwsim_abort_pmsr, > > #define HWSIM_NON_MLO_OPS \ > .sta_add = mac80211_hwsim_sta_add, \ > diff --git a/drivers/net/wireless/mac80211_hwsim.h b/drivers/net/wireless/mac80211_hwsim.h > index 98e586a56582..383f3e39c911 100644 > --- a/drivers/net/wireless/mac80211_hwsim.h > +++ b/drivers/net/wireless/mac80211_hwsim.h > @@ -83,6 +83,7 @@ enum hwsim_tx_control_flags { > * 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_CMD_MAX: enum limit > */ > enum { > @@ -96,6 +97,7 @@ enum { > HWSIM_CMD_ADD_MAC_ADDR, > HWSIM_CMD_DEL_MAC_ADDR, > HWSIM_CMD_START_PMSR, > + HWSIM_CMD_ABORT_PMSR, > __HWSIM_CMD_MAX, > }; > #define HWSIM_CMD_MAX (_HWSIM_CMD_MAX - 1) > -- > 2.40.0.rc1.284.g88254d51c5-goog >
On Tue, Mar 14, 2023 at 4:45 AM Michal Kubiak <michal.kubiak@intel.com> wrote: > > On Mon, Mar 13, 2023 at 07:53:25AM +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 necessary functionalities for mac80211_hwsim to abort previous PMSR > > request. The abortion request is sent to the wmedium where the PMSR request > > is actually handled. > > > > In detail, add new mac80211_hwsim command HWSIM_CMD_ABORT_PMSR. When > > mac80211_hwsim receives the PMSR abortion request via > > ieee80211_ops.abort_pmsr, the received cfg80211_pmsr_request is resent to > > the wmediumd with command HWSIM_CMD_ABORT_PMSR and attribute > > HWSIM_ATTR_PMSR_REQUEST. The attribute is formatted as the same way as > > nl80211_pmsr_start() expects. > > > > Signed-off-by: Jaewan Kim <jaewan@google.com> > > --- > > V7 -> V8: Rewrote commit msg > > V7: Initial commit (split from previously large patch) > > --- > > drivers/net/wireless/mac80211_hwsim.c | 61 +++++++++++++++++++++++++++ > > drivers/net/wireless/mac80211_hwsim.h | 2 + > > 2 files changed, 63 insertions(+) > > > > diff --git a/drivers/net/wireless/mac80211_hwsim.c b/drivers/net/wireless/mac80211_hwsim.c > > index a692d9c95566..8f699dfab77a 100644 > > --- a/drivers/net/wireless/mac80211_hwsim.c > > +++ b/drivers/net/wireless/mac80211_hwsim.c > > @@ -3343,6 +3343,66 @@ static int mac80211_hwsim_start_pmsr(struct ieee80211_hw *hw, > > return err; > > } > > > > +static void mac80211_hwsim_abort_pmsr(struct ieee80211_hw *hw, > > + struct ieee80211_vif *vif, > > + struct cfg80211_pmsr_request *request) > > +{ > > + struct mac80211_hwsim_data *data = hw->priv; > > + u32 _portid = READ_ONCE(data->wmediumd); > > + struct sk_buff *skb = NULL; > > + int err = 0; > > + void *msg_head; > > + struct nlattr *pmsr; > > Please use RCT style. > > > + > > + if (!_portid && !hwsim_virtio_enabled) > > + return; > > + > > + mutex_lock(&data->mutex); > > + > > + if (data->pmsr_request != request) { > > + err = -EINVAL; > > + goto out_err; > > + } > > + > > + if (err) > > + return; > > Redundant code - err is always zero in this place, isn't it? removed. > > > + > > + skb = genlmsg_new(GENLMSG_DEFAULT_SIZE, GFP_KERNEL); > > + if (!skb) > > + return; > > Return from the function while the mutex is still locked. > I guess 'goto' should be used here like for other checks in this > function. great catch. Thank you so much. > > > + > > + msg_head = genlmsg_put(skb, 0, 0, &hwsim_genl_family, 0, HWSIM_CMD_ABORT_PMSR); > > + > > + if (nla_put(skb, HWSIM_ATTR_ADDR_TRANSMITTER, ETH_ALEN, data->addresses[1].addr)) > > + goto out_err; > > + > > + pmsr = nla_nest_start(skb, HWSIM_ATTR_PMSR_REQUEST); > > + if (!pmsr) { > > + err = -ENOMEM; > > + goto out_err; > > + } > > + > > + err = mac80211_hwsim_send_pmsr_request(skb, request); > > + if (err) > > + goto out_err; > > + > > + err = nla_nest_end(skb, pmsr); > > + if (err) > > + goto out_err; > > + > > + genlmsg_end(skb, msg_head); > > + if (hwsim_virtio_enabled) > > + hwsim_tx_virtio(data, skb); > > + else > > + hwsim_unicast_netgroup(data, skb, _portid); > > + > > +out_err: > > + if (err && skb) > > + nlmsg_free(skb); > > I suggest to reorganize that code (or at least rename the label "out_err" > to "out" maybe?) to separate error path and normal path more clearly. renamed to out. > > > + > > + mutex_unlock(&data->mutex); > > +} > > + > > #define HWSIM_COMMON_OPS \ > > .tx = mac80211_hwsim_tx, \ > > .wake_tx_queue = ieee80211_handle_wake_tx_queue, \ > > @@ -3367,6 +3427,7 @@ static int mac80211_hwsim_start_pmsr(struct ieee80211_hw *hw, > > .get_et_stats = mac80211_hwsim_get_et_stats, \ > > .get_et_strings = mac80211_hwsim_get_et_strings, \ > > .start_pmsr = mac80211_hwsim_start_pmsr, \ > > + .abort_pmsr = mac80211_hwsim_abort_pmsr, > > > > #define HWSIM_NON_MLO_OPS \ > > .sta_add = mac80211_hwsim_sta_add, \ > > diff --git a/drivers/net/wireless/mac80211_hwsim.h b/drivers/net/wireless/mac80211_hwsim.h > > index 98e586a56582..383f3e39c911 100644 > > --- a/drivers/net/wireless/mac80211_hwsim.h > > +++ b/drivers/net/wireless/mac80211_hwsim.h > > @@ -83,6 +83,7 @@ enum hwsim_tx_control_flags { > > * 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_CMD_MAX: enum limit > > */ > > enum { > > @@ -96,6 +97,7 @@ enum { > > HWSIM_CMD_ADD_MAC_ADDR, > > HWSIM_CMD_DEL_MAC_ADDR, > > HWSIM_CMD_START_PMSR, > > + HWSIM_CMD_ABORT_PMSR, > > __HWSIM_CMD_MAX, > > }; > > #define HWSIM_CMD_MAX (_HWSIM_CMD_MAX - 1) > > -- > > 2.40.0.rc1.284.g88254d51c5-goog > > Done for reverse christmas tree style. -- Jaewan Kim (김재완) | Software Engineer in Google Korea | jaewan@google.com | +82-10-2781-5078
diff --git a/drivers/net/wireless/mac80211_hwsim.c b/drivers/net/wireless/mac80211_hwsim.c index a692d9c95566..8f699dfab77a 100644 --- a/drivers/net/wireless/mac80211_hwsim.c +++ b/drivers/net/wireless/mac80211_hwsim.c @@ -3343,6 +3343,66 @@ static int mac80211_hwsim_start_pmsr(struct ieee80211_hw *hw, return err; } +static void mac80211_hwsim_abort_pmsr(struct ieee80211_hw *hw, + struct ieee80211_vif *vif, + struct cfg80211_pmsr_request *request) +{ + struct mac80211_hwsim_data *data = hw->priv; + u32 _portid = READ_ONCE(data->wmediumd); + struct sk_buff *skb = NULL; + int err = 0; + void *msg_head; + struct nlattr *pmsr; + + if (!_portid && !hwsim_virtio_enabled) + return; + + mutex_lock(&data->mutex); + + if (data->pmsr_request != request) { + err = -EINVAL; + goto out_err; + } + + if (err) + return; + + skb = genlmsg_new(GENLMSG_DEFAULT_SIZE, GFP_KERNEL); + if (!skb) + return; + + msg_head = genlmsg_put(skb, 0, 0, &hwsim_genl_family, 0, HWSIM_CMD_ABORT_PMSR); + + if (nla_put(skb, HWSIM_ATTR_ADDR_TRANSMITTER, ETH_ALEN, data->addresses[1].addr)) + goto out_err; + + pmsr = nla_nest_start(skb, HWSIM_ATTR_PMSR_REQUEST); + if (!pmsr) { + err = -ENOMEM; + goto out_err; + } + + err = mac80211_hwsim_send_pmsr_request(skb, request); + if (err) + goto out_err; + + err = nla_nest_end(skb, pmsr); + if (err) + goto out_err; + + genlmsg_end(skb, msg_head); + if (hwsim_virtio_enabled) + hwsim_tx_virtio(data, skb); + else + hwsim_unicast_netgroup(data, skb, _portid); + +out_err: + if (err && skb) + nlmsg_free(skb); + + mutex_unlock(&data->mutex); +} + #define HWSIM_COMMON_OPS \ .tx = mac80211_hwsim_tx, \ .wake_tx_queue = ieee80211_handle_wake_tx_queue, \ @@ -3367,6 +3427,7 @@ static int mac80211_hwsim_start_pmsr(struct ieee80211_hw *hw, .get_et_stats = mac80211_hwsim_get_et_stats, \ .get_et_strings = mac80211_hwsim_get_et_strings, \ .start_pmsr = mac80211_hwsim_start_pmsr, \ + .abort_pmsr = mac80211_hwsim_abort_pmsr, #define HWSIM_NON_MLO_OPS \ .sta_add = mac80211_hwsim_sta_add, \ diff --git a/drivers/net/wireless/mac80211_hwsim.h b/drivers/net/wireless/mac80211_hwsim.h index 98e586a56582..383f3e39c911 100644 --- a/drivers/net/wireless/mac80211_hwsim.h +++ b/drivers/net/wireless/mac80211_hwsim.h @@ -83,6 +83,7 @@ enum hwsim_tx_control_flags { * 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_CMD_MAX: enum limit */ enum { @@ -96,6 +97,7 @@ enum { HWSIM_CMD_ADD_MAC_ADDR, HWSIM_CMD_DEL_MAC_ADDR, HWSIM_CMD_START_PMSR, + HWSIM_CMD_ABORT_PMSR, __HWSIM_CMD_MAX, }; #define HWSIM_CMD_MAX (_HWSIM_CMD_MAX - 1)
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 necessary functionalities for mac80211_hwsim to abort previous PMSR request. The abortion request is sent to the wmedium where the PMSR request is actually handled. In detail, add new mac80211_hwsim command HWSIM_CMD_ABORT_PMSR. When mac80211_hwsim receives the PMSR abortion request via ieee80211_ops.abort_pmsr, the received cfg80211_pmsr_request is resent to the wmediumd with command HWSIM_CMD_ABORT_PMSR and attribute HWSIM_ATTR_PMSR_REQUEST. The attribute is formatted as the same way as nl80211_pmsr_start() expects. Signed-off-by: Jaewan Kim <jaewan@google.com> --- V7 -> V8: Rewrote commit msg V7: Initial commit (split from previously large patch) --- drivers/net/wireless/mac80211_hwsim.c | 61 +++++++++++++++++++++++++++ drivers/net/wireless/mac80211_hwsim.h | 2 + 2 files changed, 63 insertions(+)