On Tue, Mar 14, 2023 at 4:45 AM Michal Kubiak <[email protected]> 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 <[email protected]>
> > ---
> > 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 |
[email protected] | +82-10-2781-5078