Re: [PATCH v7 2/4] mac80211_hwsim: add PMSR request support via virtio
From: Johannes Berg <johannes@sipsolutions.net>
Date: 2023-02-15 18:07:53
Also in:
linux-wireless
On Tue, 2023-02-07 at 08:53 +0000, Jaewan Kim wrote:
+static int mac80211_hwsim_send_pmsr_ftm_request_peer(struct sk_buff *msg, + struct cfg80211_pmsr_ftm_request_peer *request)
this ...
+{
+ struct nlattr *ftm;
+
+ if (!request->requested)
+ return -EINVAL;
+
+ ftm = nla_nest_start(msg, NL80211_PMSR_TYPE_FTM);
+ if (!ftm)
+ return -ENOBUFS;
+
+ if (nla_put_u32(msg, NL80211_PMSR_FTM_REQ_ATTR_PREAMBLE, request->preamble))
+ return -ENOBUFS;
+
+ if (nla_put_u16(msg, NL80211_PMSR_FTM_REQ_ATTR_BURST_PERIOD, request->burst_period))
+ return -ENOBUFS;and this ... etc ... also got some really long lines that could easily be broken
+ chandef = nla_nest_start(msg, NL80211_PMSR_PEER_ATTR_CHAN); + if (!chandef) + return -ENOBUFS; + + err = cfg80211_send_chandef(msg, &request->chandef); + if (err) + return err;
So this one I think I'll let you do with the export and all, because that's way nicer than duplicating the code, and it's clearly necessary.
+static int mac80211_hwsim_send_pmsr_request(struct sk_buff *msg,
+ struct cfg80211_pmsr_request *request)
+{
+ int err;
+ struct nlattr *pmsr = nla_nest_start(msg, NL80211_ATTR_PEER_MEASUREMENTS);I'm not going to complain _too_ much about this, but all this use of nl80211 attributes better be thoroughly documented in the header file.
+ * @HWSIM_CMD_START_PMSR: start PMSR
That sounds almost like it's a command ("start PMSR") but really it's a
notification/event as far as hwsim is concerned, so please document
that.
+ * @HWSIM_ATTR_PMSR_REQUEST: peer measurement request
and please document the structure of the request that userspace will get (and how it should respond?)
quoted hunk ↗ jump to hunk
+++ b/include/net/cfg80211.h@@ -938,6 +938,16 @@ int cfg80211_chandef_dfs_required(struct wiphy *wiphy, const struct cfg80211_chan_def *chandef, enum nl80211_iftype iftype); +/** + * cfg80211_send_chandef - sends the channel definition. + * @msg: the msg to send channel definition + * @chandef: the channel definition to check + * + * Returns: 0 if sent the channel definition to msg, < 0 on error + **/
That last line should just be */
+int cfg80211_send_chandef(struct sk_buff *msg, const struct cfg80211_chan_def *chandef);
I think it'd be better if you exported it as nl80211_..., since it really is just a netlink thing, not cfg80211 functionality. It would also be good, IMHO, to split this part out into a separate patch saying that e.g. hwsim might use it like you do here, or even that vendor netlink could use it where needed. johannes