Skip to contentSkip to conversation

nl80211: Add HE UL MU settings used in UL Trigger

PATCH In review Muna Sinada 2 participants 2 messages Nov 4, 2021 – Dec 20, 2021 802.11 (including cfg80211/NL80211)

lore
Share & tools
lore

Muna Sinada

author
#0 lore raw

PATCH v3 1/1 nl80211: Add HE UL MU settings used in UL Trigger

This patch adds nl80211 definitions, policies and parsing code required to pass HE uplink MU fixed rate settings utiliized in UL trigger.

Signed-off-by Muna Sinada <[email protected]>

---
v3:
- restructured flow of passing uplink MU transmission settings. No longer utilizing existing bitrate structures and attributes, new ones are created for uplink MU transmission.
- added HE LTF/GI/LDPC/STBC along with HE fixed rate setting for uplink MU transmissions, which are part of the UL Trigger.
- modified commit message from "nl80211: Add HE UL MU fixed rate setting" in order to include all the rate settings related to UL Trigger.

---
 include/net/cfg80211.h       | 17 ++++++++
 include/uapi/linux/nl80211.h | 45 +++++++++++++++++++++
 net/wireless/nl80211.c       | 96 ++++++++++++++++++++++++++++++++++++++++++++
 net/wireless/rdev-ops.h      | 12 ++++++
 net/wireless/trace.h         | 18 +++++++++
 5 files changed, 188 insertions(+)
include/net/cfg80211.h +17 -0
index 362da9f6bf39..a05ecd067e08 100644
@@ -697,6 +697,18 @@ struct cfg80211_bitrate_mask {
 	} control[NUM_NL80211_BANDS];
 };
 
+/*
+ * cfg80211_ul_bitrate_mask - masks for MU uplink bitrate control
+ */
+struct cfg80211_ul_bitrate_mask {
+	struct {
+		u16 he_ul_mcs[NL80211_HE_NSS_MAX];
+		enum nl80211_he_gi he_ul_gi;
+		enum nl80211_he_ltf he_ul_ltf;
+		bool he_ul_ldpc;
+		bool he_ul_stbc;
+	} control[NUM_NL80211_BANDS];
+};
 
 /**
  * struct cfg80211_tid_cfg - TID specific configuration

@@ -4233,6 +4245,11 @@ struct cfg80211_ops {
 				    const u8 *peer,
 				    const struct cfg80211_bitrate_mask *mask);
 
+	int	(*set_ul_bitrate_mask)(struct wiphy *wiphy,
+				       struct net_device *dev,
+				       const u8 *peer,
+				       const struct cfg80211_ul_bitrate_mask *mask);
+
 	int	(*dump_survey)(struct wiphy *wiphy, struct net_device *netdev,
 			int idx, struct survey_info *info);
include/uapi/linux/nl80211.h +45 -0

75 lines not on this page · raw message

net/wireless/nl80211.c +96 -0

120 lines not on this page · raw message

net/wireless/rdev-ops.h +12 -0
index 8672b3ef99e4..418cb37c99a2 100644
@@ -661,6 +661,18 @@ rdev_set_bitrate_mask(struct cfg80211_registered_device *rdev,
 	return ret;
 }
 
+static inline int
+rdev_set_ul_bitrate_mask(struct cfg80211_registered_device *rdev,
+			 struct net_device *dev, const u8 *peer,
+			 const struct cfg80211_ul_bitrate_mask *mask)
+{
+	int ret;
+	trace_rdev_set_ul_bitrate_mask(&rdev->wiphy, dev, peer, mask);
+	ret = rdev->ops->set_ul_bitrate_mask(&rdev->wiphy, dev, peer, mask);
+	trace_rdev_return_int(&rdev->wiphy, ret);
+	return ret;
+}
+
 static inline int rdev_dump_survey(struct cfg80211_registered_device *rdev,
 				   struct net_device *netdev, int idx,
 				   struct survey_info *info)
net/wireless/trace.h +18 -0
index 0b27eaa14a18..eb9a6e2641a1 100644
@@ -1635,6 +1635,24 @@ TRACE_EVENT(rdev_set_bitrate_mask,
 		  WIPHY_PR_ARG, NETDEV_PR_ARG, MAC_PR_ARG(peer))
 );
 
+TRACE_EVENT(rdev_set_ul_bitrate_mask,
+	TP_PROTO(struct wiphy *wiphy, struct net_device *netdev,
+		 const u8 *peer, const struct cfg80211_ul_bitrate_mask *mask),
+	TP_ARGS(wiphy, netdev, peer, mask),
+	TP_STRUCT__entry(
+		WIPHY_ENTRY
+		NETDEV_ENTRY
+		MAC_ENTRY(peer)
+	),
+	TP_fast_assign(
+		WIPHY_ASSIGN;
+		NETDEV_ASSIGN;
+		MAC_ASSIGN(peer, peer);
+	),
+	TP_printk(WIPHY_PR_FMT ", " NETDEV_PR_FMT ", peer: " MAC_PR_FMT,
+		  WIPHY_PR_ARG, NETDEV_PR_ARG, MAC_PR_ARG(peer))
+);
+
 TRACE_EVENT(rdev_update_mgmt_frame_registrations,
 	TP_PROTO(struct wiphy *wiphy, struct wireless_dev *wdev,
 		 struct mgmt_frame_regs *upd),
Signature · 1 line
2.7.4
maintainer
#1 lore raw

Hi,

Sorry for the long delay reviewing this here...

Quoted 2 lines

+/*
+ * cfg80211_ul_bitrate_mask - masks for MU uplink bitrate control

Can you add kernel-doc?

Also maybe we should think about EHT already - I guess we can use the same structure here for EHT in the future?

include/net/cfg80211.h struct cfg80211_ops { · line 4251

Quoted 10 lines

/**
* struct cfg80211_tid_cfg - TID specific configuration

include/net/cfg80211.h +4 -0
@@ -4233,6 +4245,11 @@ struct cfg80211_ops {
 				    const u8 *peer,
 				    const struct cfg80211_bitrate_mask *mask);
 
+	int	(*set_ul_bitrate_mask)(struct wiphy *wiphy,
+				       struct net_device *dev,
+				       const u8 *peer,
+				       const struct cfg80211_ul_bitrate_mask *mask);

Missing documentation.

include/uapi/linux/nl80211.h enum nl80211_commands { · line 1482

Quoted 4 lines
include/uapi/linux/nl80211.h +1 -0
 	NL80211_CMD_SET_FILS_AAD,
 
+	NL80211_CMD_SET_UL_BITRATE_MASK,
 	/* add new commands above here */

Please keep a newline after.

include/uapi/linux/nl80211.h enum nl80211_attrs { · line 3170

Quoted 4 lines
include/uapi/linux/nl80211.h +1 -0
 	NL80211_ATTR_RADAR_OFFCHAN,
 
+	NL80211_ATTR_UL_RATES,
 	/* add attributes here, update the policy in nl80211.c */

same here

Quoted 4 lines

/**
+ * enum nl80211_ul_rate_attributes - MU uplink rate set attributes
+ * @NL80211_UL_RATE_HE: HE MU UL MCS rates for MU uplink traffic,
+ * see &struct nl80211_ul_rate_he

Generally, this should be called e.g. NL80211_UL_RATE_ATTR_*, i.e. have "ATTR" somewhere.

Given my earlier comment about EHT I _think_ it should be

NL80211_UL_RATE_ATTR_HE_GI

so we can add

NL80211_UL_RATE_ATTR_EHT_GI

if needed (though I think in that particuilar it's really not needed and we can use the same GI attribute for both since they both have the same values.

net/wireless/nl80211.c static const struct nla_policy nl80211_txattr_policy[NL80211_TXRATE_MAX + 1] = { · line 399

Quoted 11 lines
net/wireless/nl80211.c +11 -0
+static const struct nla_policy nl80211_ul_rate_attr_policy[NL80211_UL_RATE_MAX + 1] = {
+	[NL80211_UL_RATE_HE] = NLA_POLICY_EXACT_LEN(sizeof(struct nl80211_ul_rate_he)),
+	[NL80211_UL_RATE_HE_GI] =  NLA_POLICY_RANGE(NLA_U8,
+						   NL80211_RATE_INFO_HE_GI_0_8,
+						   NL80211_RATE_INFO_HE_GI_3_2),
+	[NL80211_UL_RATE_HE_LTF] = NLA_POLICY_RANGE(NLA_U8,
+						   NL80211_RATE_INFO_HE_1XLTF,
+						   NL80211_RATE_INFO_HE_4XLTF),
+	[NL80211_UL_RATE_HE_LDPC] = { .type = NLA_FLAG },
+	[NL80211_UL_RATE_HE_STBC] = { .type = NLA_FLAG },
+};

Nice!

But please link it into the overall policy:

+ [NL80211_ATTR_UL_RATES] =
+ NLA_POLICY_NESTED_ARRAY(nl80211_ul_rate_attr_policy),

I think? The index is the band enum value, but we can still validate it already?

net/wireless/nl80211.c static int nl80211_parse_tx_bitrate_mask(struct genl_info *info, · line 4993

Quoted 9 lines
net/wireless/nl80211.c +9 -0
+	/* The nested attribute uses enum nl80211_band as the index. This maps
+	 * directly to the enum nl80211_band values used in cfg80211.
+	 */
+	nla_for_each_nested(tx_rates, attrs[attr], rem) {
+		enum nl80211_band band = nla_type(tx_rates);
+		int err;
+
+		if (band < 0 || band >= NUM_NL80211_BANDS)
+			return -EINVAL;

NUM_NL80211_BANDS can change, maybe we should just ignore additional bands?

net/wireless/nl80211.c static int nl80211_parse_tx_bitrate_mask(struct genl_info *info, · line 5000

Quoted 7 lines
net/wireless/nl80211.c +7 -0
+		sband = rdev->wiphy.bands[band];
+		if (!sband)
+			return -EINVAL;
+		err = nla_parse_nested_deprecated(tb, NL80211_UL_RATE_MAX,
+						  tx_rates,
+						  nl80211_ul_rate_attr_policy,
+						  info->extack);

Please don't use _deprecated() in new code. And if you link the policy properly, you don't need it here.

net/wireless/nl80211.c static int nl80211_parse_tx_bitrate_mask(struct genl_info *info, · line 5020

Quoted 6 lines
net/wireless/nl80211.c +6 -0
+		if (tb[NL80211_UL_RATE_HE_LDPC])
+			mask->control[band].he_ul_ldpc =
+				!nla_get_flag(tb[NL80211_UL_RATE_HE_LDPC]);
+		if (tb[NL80211_UL_RATE_HE_STBC])
+			mask->control[band].he_ul_stbc =
+				!nla_get_flag(tb[NL80211_UL_RATE_HE_STBC]);

This doesn't make sense - see how nla_get_flag() works and then remove the if statements :)

Quoted 8 lines
net/wireless/trace.h +7 -0
+TRACE_EVENT(rdev_set_ul_bitrate_mask,
+	TP_PROTO(struct wiphy *wiphy, struct net_device *netdev,
+		 const u8 *peer, const struct cfg80211_ul_bitrate_mask *mask),
+	TP_ARGS(wiphy, netdev, peer, mask),
+	TP_STRUCT__entry(
+		WIPHY_ENTRY
+		NETDEV_ENTRY

net/wireless/trace.h TRACE_EVENT(rdev_set_bitrate_mask, · line 1654

Quoted 10 lines
net/wireless/trace.h +10 -0
+		MAC_ENTRY(peer)
+	),
+	TP_fast_assign(
+		WIPHY_ASSIGN;
+		NETDEV_ASSIGN;
+		MAC_ASSIGN(peer, peer);
+	),
+	TP_printk(WIPHY_PR_FMT ", " NETDEV_PR_FMT ", peer: " MAC_PR_FMT,
+		  WIPHY_PR_ARG, NETDEV_PR_ARG, MAC_PR_ARG(peer))
+);

A bit more information would be useful, IMHO.

johannes