[PATCH v2 1/2] cfg80211/nl80211: rename packet pattern related structures and enums

[PATCH v2 1/2] cfg80211/nl80211: rename packet pattern related structures and enums

[Bing Zhao]

From: Amitkumar Karwar <akarwar@marvell.com>

Currently packet patterns and it's enum/structures are used only
for WoWLAN feature. As we intend to reuse them for new feature
packet coalesce, they are renamed in this patch.

Older names are kept for backward compatibility purpose.

Signed-off-by: Amitkumar Karwar <akarwar@marvell.com>
Signed-off-by: Bing Zhao <bzhao@marvell.com>
---
v2: more details about the renames in commit log (Luis R. Rodriguez)
    don't break the API with existing applications (Johannes Berg)

 drivers/net/wireless/ath/ath9k/main.c   |  2 +-
 drivers/net/wireless/mwifiex/cfg80211.c |  3 +--
 drivers/net/wireless/ti/wlcore/main.c   | 10 ++++----
 include/net/cfg80211.h                  |  6 ++---
 include/uapi/linux/nl80211.h            | 45 ++++++++++++++++++++-------------
 net/wireless/nl80211.c                  | 34 ++++++++++++-------------
 6 files changed, 53 insertions(+), 47 deletions(-)

diff --git a/drivers/net/wireless/ath/ath9k/main.c b/drivers/net/wireless/ath/ath9k/main.c
index 6963862..57e290e 100644
--- a/drivers/net/wireless/ath/ath9k/main.c
+++ b/drivers/net/wireless/ath/ath9k/main.c
@@ -2123,7 +2123,7 @@ static void ath9k_wow_add_pattern(struct ath_softc *sc,
 {
 	struct ath_hw *ah = sc->sc_ah;
 	struct ath9k_wow_pattern *wow_pattern = NULL;
-	struct cfg80211_wowlan_trig_pkt_pattern *patterns = wowlan->patterns;
+	struct cfg80211_pkt_pattern *patterns = wowlan->patterns;
 	int mask_len;
 	s8 i = 0;
 
diff --git a/drivers/net/wireless/mwifiex/cfg80211.c b/drivers/net/wireless/mwifiex/cfg80211.c
index d3c8ece..7a06f51 100644
--- a/drivers/net/wireless/mwifiex/cfg80211.c
+++ b/drivers/net/wireless/mwifiex/cfg80211.c
@@ -2250,8 +2250,7 @@ EXPORT_SYMBOL_GPL(mwifiex_del_virtual_intf);
 
 #ifdef CONFIG_PM
 static bool
-mwifiex_is_pattern_supported(struct cfg80211_wowlan_trig_pkt_pattern *pat,
-			     s8 *byte_seq)
+mwifiex_is_pattern_supported(struct cfg80211_pkt_pattern *pat, s8 *byte_seq)
 {
 	int j, k, valid_byte_cnt = 0;
 	bool dont_care_byte = false;
diff --git a/drivers/net/wireless/ti/wlcore/main.c b/drivers/net/wireless/ti/wlcore/main.c
index 953111a..db3653f 100644
--- a/drivers/net/wireless/ti/wlcore/main.c
+++ b/drivers/net/wireless/ti/wlcore/main.c
@@ -1328,7 +1328,7 @@ static struct sk_buff *wl12xx_alloc_dummy_packet(struct wl1271 *wl)
 
 #ifdef CONFIG_PM
 static int
-wl1271_validate_wowlan_pattern(struct cfg80211_wowlan_trig_pkt_pattern *p)
+wl1271_validate_wowlan_pattern(struct cfg80211_pkt_pattern *p)
 {
 	int num_fields = 0, in_field = 0, fields_size = 0;
 	int i, pattern_len = 0;
@@ -1471,9 +1471,9 @@ void wl1271_rx_filter_flatten_fields(struct wl12xx_rx_filter *filter,
  * Allocates an RX filter returned through f
  * which needs to be freed using rx_filter_free()
  */
-static int wl1271_convert_wowlan_pattern_to_rx_filter(
-	struct cfg80211_wowlan_trig_pkt_pattern *p,
-	struct wl12xx_rx_filter **f)
+static int
+wl1271_convert_wowlan_pattern_to_rx_filter(struct cfg80211_pkt_pattern *p,
+					   struct wl12xx_rx_filter **f)
 {
 	int i, j, ret = 0;
 	struct wl12xx_rx_filter *filter;
@@ -1575,7 +1575,7 @@ static int wl1271_configure_wowlan(struct wl1271 *wl,
 
 	/* Translate WoWLAN patterns into filters */
 	for (i = 0; i < wow->n_patterns; i++) {
-		struct cfg80211_wowlan_trig_pkt_pattern *p;
+		struct cfg80211_pkt_pattern *p;
 		struct wl12xx_rx_filter *filter = NULL;
 
 		p = &wow->patterns[i];
diff --git a/include/net/cfg80211.h b/include/net/cfg80211.h
index 32a2f1b..4585b63 100644
--- a/include/net/cfg80211.h
+++ b/include/net/cfg80211.h
@@ -1661,7 +1661,7 @@ struct cfg80211_pmksa {
 };
 
 /**
- * struct cfg80211_wowlan_trig_pkt_pattern - packet pattern
+ * struct cfg80211_pkt_pattern - packet pattern
  * @mask: bitmask where to match pattern and where to ignore bytes,
  *	one bit per byte, in same format as nl80211
  * @pattern: bytes to match where bitmask is 1
@@ -1671,7 +1671,7 @@ struct cfg80211_pmksa {
  * Internal note: @mask and @pattern are allocated in one chunk of
  * memory, free @mask only!
  */
-struct cfg80211_wowlan_trig_pkt_pattern {
+struct cfg80211_pkt_pattern {
 	u8 *mask, *pattern;
 	int pattern_len;
 	int pkt_offset;
@@ -1733,7 +1733,7 @@ struct cfg80211_wowlan {
 	bool any, disconnect, magic_pkt, gtk_rekey_failure,
 	     eap_identity_req, four_way_handshake,
 	     rfkill_release;
-	struct cfg80211_wowlan_trig_pkt_pattern *patterns;
+	struct cfg80211_pkt_pattern *patterns;
 	struct cfg80211_wowlan_tcp *tcp;
 	int n_patterns;
 };
diff --git a/include/uapi/linux/nl80211.h b/include/uapi/linux/nl80211.h
index 0632071..1fa2bf4 100644
--- a/include/uapi/linux/nl80211.h
+++ b/include/uapi/linux/nl80211.h
@@ -3041,11 +3041,11 @@ enum nl80211_tx_power_setting {
 };
 
 /**
- * enum nl80211_wowlan_packet_pattern_attr - WoWLAN packet pattern attribute
- * @__NL80211_WOWLAN_PKTPAT_INVALID: invalid number for nested attribute
- * @NL80211_WOWLAN_PKTPAT_PATTERN: the pattern, values where the mask has
+ * enum nl80211_packet_pattern_attr - packet pattern attribute
+ * @__NL80211_PKTPAT_INVALID: invalid number for nested attribute
+ * @NL80211_PKTPAT_PATTERN: the pattern, values where the mask has
  *	a zero bit are ignored
- * @NL80211_WOWLAN_PKTPAT_MASK: pattern mask, must be long enough to have
+ * @NL80211_PKTPAT_MASK: pattern mask, must be long enough to have
  *	a bit for each byte in the pattern. The lowest-order bit corresponds
  *	to the first byte of the pattern, but the bytes of the pattern are
  *	in a little-endian-like format, i.e. the 9th byte of the pattern
@@ -3056,23 +3056,23 @@ enum nl80211_tx_power_setting {
  *	Note that the pattern matching is done as though frames were not
  *	802.11 frames but 802.3 frames, i.e. the frame is fully unpacked
  *	first (including SNAP header unpacking) and then matched.
- * @NL80211_WOWLAN_PKTPAT_OFFSET: packet offset, pattern is matched after
+ * @NL80211_PKTPAT_OFFSET: packet offset, pattern is matched after
  *	these fixed number of bytes of received packet
- * @NUM_NL80211_WOWLAN_PKTPAT: number of attributes
- * @MAX_NL80211_WOWLAN_PKTPAT: max attribute number
+ * @NUM_NL80211_PKTPAT: number of attributes
+ * @MAX_NL80211_PKTPAT: max attribute number
  */
-enum nl80211_wowlan_packet_pattern_attr {
-	__NL80211_WOWLAN_PKTPAT_INVALID,
-	NL80211_WOWLAN_PKTPAT_MASK,
-	NL80211_WOWLAN_PKTPAT_PATTERN,
-	NL80211_WOWLAN_PKTPAT_OFFSET,
+enum nl80211_packet_pattern_attr {
+	__NL80211_PKTPAT_INVALID,
+	NL80211_PKTPAT_MASK,
+	NL80211_PKTPAT_PATTERN,
+	NL80211_PKTPAT_OFFSET,
 
-	NUM_NL80211_WOWLAN_PKTPAT,
-	MAX_NL80211_WOWLAN_PKTPAT = NUM_NL80211_WOWLAN_PKTPAT - 1,
+	NUM_NL80211_PKTPAT,
+	MAX_NL80211_PKTPAT = NUM_NL80211_PKTPAT - 1,
 };
 
 /**
- * struct nl80211_wowlan_pattern_support - pattern support information
+ * struct nl80211_pattern_support - packet pattern support information
  * @max_patterns: maximum number of patterns supported
  * @min_pattern_len: minimum length of each pattern
  * @max_pattern_len: maximum length of each pattern
@@ -3082,13 +3082,22 @@ enum nl80211_wowlan_packet_pattern_attr {
  * that is part of %NL80211_ATTR_WOWLAN_TRIGGERS_SUPPORTED in the
  * capability information given by the kernel to userspace.
  */
-struct nl80211_wowlan_pattern_support {
+struct nl80211_pattern_support {
 	__u32 max_patterns;
 	__u32 min_pattern_len;
 	__u32 max_pattern_len;
 	__u32 max_pkt_offset;
 } __attribute__((packed));
 
+/* only for backward compatibility */
+#define __NL80211_WOWLAN_PKTPAT_INVALID __NL80211_INVALID,
+#define NL80211_WOWLAN_PKTPAT_MASK NL80211_PKTPAT_MASK
+#define NL80211_WOWLAN_PKTPAT_PATTERN NL80211_PKTPAT_PATTERN
+#define NL80211_WOWLAN_PKTPAT_OFFSET NL80211_PKTPAT_OFFSET
+#define NUM_NL80211_WOWLAN_PKTPAT NUM_NL80211_PKTPAT
+#define MAX_NL80211_WOWLAN_PKTPAT MAX_NL80211_PKTPAT
+#define nl80211_wowlan_pattern_support nl80211_pattern_support
+
 /**
  * enum nl80211_wowlan_triggers - WoWLAN trigger definitions
  * @__NL80211_WOWLAN_TRIG_INVALID: invalid number for nested attributes
@@ -3108,7 +3117,7 @@ struct nl80211_wowlan_pattern_support {
  *	pattern matching is done after the packet is converted to the MSDU.
  *
  *	In %NL80211_ATTR_WOWLAN_TRIGGERS_SUPPORTED, it is a binary attribute
- *	carrying a &struct nl80211_wowlan_pattern_support.
+ *	carrying a &struct nl80211_pattern_support.
  *
  *	When reporting wakeup. it is a u32 attribute containing the 0-based
  *	index of the pattern that caused the wakeup, in the patterns passed
@@ -3265,7 +3274,7 @@ struct nl80211_wowlan_tcp_data_token_feature {
  * @NL80211_WOWLAN_TCP_WAKE_PAYLOAD: wake packet payload, for advertising a
  *	u32 attribute holding the maximum length
  * @NL80211_WOWLAN_TCP_WAKE_MASK: Wake packet payload mask, not used for
- *	feature advertising. The mask works like @NL80211_WOWLAN_PKTPAT_MASK
+ *	feature advertising. The mask works like @NL80211_PKTPAT_MASK
  *	but on the TCP payload only.
  * @NUM_NL80211_WOWLAN_TCP: number of TCP attributes
  * @MAX_NL80211_WOWLAN_TCP: highest attribute number
diff --git a/net/wireless/nl80211.c b/net/wireless/nl80211.c
index 5f10f7a..5f640de 100644
--- a/net/wireless/nl80211.c
+++ b/net/wireless/nl80211.c
@@ -1014,7 +1014,7 @@ static int nl80211_send_wowlan(struct sk_buff *msg,
 		return -ENOBUFS;
 
 	if (dev->wiphy.wowlan.n_patterns) {
-		struct nl80211_wowlan_pattern_support pat = {
+		struct nl80211_pattern_support pat = {
 			.max_patterns = dev->wiphy.wowlan.n_patterns,
 			.min_pattern_len = dev->wiphy.wowlan.pattern_min_len,
 			.max_pattern_len = dev->wiphy.wowlan.pattern_max_len,
@@ -7570,12 +7570,11 @@ static int nl80211_send_wowlan_patterns(struct sk_buff *msg,
 		if (!nl_pat)
 			return -ENOBUFS;
 		pat_len = rdev->wowlan->patterns[i].pattern_len;
-		if (nla_put(msg, NL80211_WOWLAN_PKTPAT_MASK,
-			    DIV_ROUND_UP(pat_len, 8),
+		if (nla_put(msg, NL80211_PKTPAT_MASK, DIV_ROUND_UP(pat_len, 8),
 			    rdev->wowlan->patterns[i].mask) ||
-		    nla_put(msg, NL80211_WOWLAN_PKTPAT_PATTERN,
-			    pat_len, rdev->wowlan->patterns[i].pattern) ||
-		    nla_put_u32(msg, NL80211_WOWLAN_PKTPAT_OFFSET,
+		    nla_put(msg, NL80211_PKTPAT_PATTERN, pat_len,
+			    rdev->wowlan->patterns[i].pattern) ||
+		    nla_put_u32(msg, NL80211_PKTPAT_OFFSET,
 				rdev->wowlan->patterns[i].pkt_offset))
 			return -ENOBUFS;
 		nla_nest_end(msg, nl_pat);
@@ -7915,7 +7914,7 @@ static int nl80211_set_wowlan(struct sk_buff *skb, struct genl_info *info)
 		struct nlattr *pat;
 		int n_patterns = 0;
 		int rem, pat_len, mask_len, pkt_offset;
-		struct nlattr *pat_tb[NUM_NL80211_WOWLAN_PKTPAT];
+		struct nlattr *pat_tb[NUM_NL80211_PKTPAT];
 
 		nla_for_each_nested(pat, tb[NL80211_WOWLAN_TRIG_PKT_PATTERN],
 				    rem)
@@ -7934,26 +7933,25 @@ static int nl80211_set_wowlan(struct sk_buff *skb, struct genl_info *info)
 
 		nla_for_each_nested(pat, tb[NL80211_WOWLAN_TRIG_PKT_PATTERN],
 				    rem) {
-			nla_parse(pat_tb, MAX_NL80211_WOWLAN_PKTPAT,
-				  nla_data(pat), nla_len(pat), NULL);
+			nla_parse(pat_tb, MAX_NL80211_PKTPAT, nla_data(pat),
+				  nla_len(pat), NULL);
 			err = -EINVAL;
-			if (!pat_tb[NL80211_WOWLAN_PKTPAT_MASK] ||
-			    !pat_tb[NL80211_WOWLAN_PKTPAT_PATTERN])
+			if (!pat_tb[NL80211_PKTPAT_MASK] ||
+			    !pat_tb[NL80211_PKTPAT_PATTERN])
 				goto error;
-			pat_len = nla_len(pat_tb[NL80211_WOWLAN_PKTPAT_PATTERN]);
+			pat_len = nla_len(pat_tb[NL80211_PKTPAT_PATTERN]);
 			mask_len = DIV_ROUND_UP(pat_len, 8);
-			if (nla_len(pat_tb[NL80211_WOWLAN_PKTPAT_MASK]) !=
-			    mask_len)
+			if (nla_len(pat_tb[NL80211_PKTPAT_MASK]) != mask_len)
 				goto error;
 			if (pat_len > wowlan->pattern_max_len ||
 			    pat_len < wowlan->pattern_min_len)
 				goto error;
 
-			if (!pat_tb[NL80211_WOWLAN_PKTPAT_OFFSET])
+			if (!pat_tb[NL80211_PKTPAT_OFFSET])
 				pkt_offset = 0;
 			else
 				pkt_offset = nla_get_u32(
-					pat_tb[NL80211_WOWLAN_PKTPAT_OFFSET]);
+					pat_tb[NL80211_PKTPAT_OFFSET]);
 			if (pkt_offset > wowlan->max_pkt_offset)
 				goto error;
 			new_triggers.patterns[i].pkt_offset = pkt_offset;
@@ -7967,11 +7965,11 @@ static int nl80211_set_wowlan(struct sk_buff *skb, struct genl_info *info)
 			new_triggers.patterns[i].pattern =
 				new_triggers.patterns[i].mask + mask_len;
 			memcpy(new_triggers.patterns[i].mask,
-			       nla_data(pat_tb[NL80211_WOWLAN_PKTPAT_MASK]),
+			       nla_data(pat_tb[NL80211_PKTPAT_MASK]),
 			       mask_len);
 			new_triggers.patterns[i].pattern_len = pat_len;
 			memcpy(new_triggers.patterns[i].pattern,
-			       nla_data(pat_tb[NL80211_WOWLAN_PKTPAT_PATTERN]),
+			       nla_data(pat_tb[NL80211_PKTPAT_PATTERN]),
 			       pat_len);
 			i++;
 		}
-- 
1.8.0

[PATCH v2 2/2] cfg80211/nl80211: Add packet coalesce support

[Bing Zhao]

From: Amitkumar Karwar <akarwar@marvell.com>

In most cases, host that receives IPv4 and IPv6 multicast/broadcast
packets does not do anything with these packets. Therefore the
reception of these unwanted packets causes unnecessary processing
and power consumption.

Packet coalesce feature helps to reduce number of received
interrupts to host by buffering these packets in firmware/hardware
for some predefined time. Received interrupt will be generated when
one of the following events occur.
a) Expiration of hardware timer whose expiration time is set to
maximum coalescing delay of matching coalesce rule.
b) Coalescing buffer in hardware reaches it's limit.
c) Packet doesn't match any of the configured coalesce rules.

This patch adds set/get configuration support for packet coalesce.
User needs to configure following parameters for creating a coalesce
rule.
a) Maximum coalescing delay
b) List of packet patterns which needs to be matched
c) Condition for coalescence. pattern 'match' or 'no match'
Multiple such rules can be created.

This feature needs to be advertised during driver initialization.
Drivers are supposed to do required firmware/hardware settings based
on user configuration.

Signed-off-by: Amitkumar Karwar <akarwar@marvell.com>
Signed-off-by: Bing Zhao <bzhao@marvell.com>
---
v2: add documentation in nl80211.h and elabiration of "coalesce"
    in commit log (Luis R. Rodriguez)

 include/net/cfg80211.h       |  51 +++++++++
 include/uapi/linux/nl80211.h |  82 ++++++++++++-
 net/wireless/core.c          |   2 +
 net/wireless/core.h          |  22 ++++
 net/wireless/nl80211.c       | 265 +++++++++++++++++++++++++++++++++++++++++++
 5 files changed, 419 insertions(+), 3 deletions(-)

diff --git a/include/net/cfg80211.h b/include/net/cfg80211.h
index 4585b63..dcbf125 100644
--- a/include/net/cfg80211.h
+++ b/include/net/cfg80211.h
@@ -1739,6 +1739,35 @@ struct cfg80211_wowlan {
 };
 
 /**
+ * struct cfg80211_coalesce_rules - Coalesce rule parameters
+ *
+ * This structure defines coalesce rule for the device.
+ * @delay: maximum coalescing delay in msecs.
+ * @condition: condition for packet coalescence.
+ *	i.e. pattern 'match' or 'no match'
+ * @patterns: array of packet patterns
+ * @n_patterns: number of patterns
+ */
+struct cfg80211_coalesce_rules {
+	int delay;
+	u8 condition;
+	struct cfg80211_pkt_pattern *patterns;
+	int n_patterns;
+};
+
+/**
+ * struct cfg80211_coalesce - Packet coalescing settings
+ *
+ * This structure defines coalescing settings.
+ * @rules: array of coalesce rules
+ * @n_rules: number of rules
+ */
+struct cfg80211_coalesce {
+	struct cfg80211_coalesce_rules **rules;
+	int n_rules;
+};
+
+/**
  * struct cfg80211_wowlan_wakeup - wakeup report
  * @disconnect: woke up by getting disconnected
  * @magic_pkt: woke up by receiving magic packet
@@ -2024,6 +2053,7 @@ struct cfg80211_update_ft_ies_params {
  *	driver can take the most appropriate actions.
  * @crit_proto_stop: Indicates critical protocol no longer needs increased link
  *	reliability. This operation can not fail.
+ * @set_coalesce: Set coalesce parameters.
  */
 struct cfg80211_ops {
 	int	(*suspend)(struct wiphy *wiphy, struct cfg80211_wowlan *wow);
@@ -2259,6 +2289,8 @@ struct cfg80211_ops {
 				    u16 duration);
 	void	(*crit_proto_stop)(struct wiphy *wiphy,
 				   struct wireless_dev *wdev);
+	int	(*set_coalesce)(struct wiphy *wiphy,
+				struct cfg80211_coalesce_rules *rule);
 };
 
 /*
@@ -2483,6 +2515,23 @@ struct wiphy_wowlan_support {
 };
 
 /**
+ * struct wiphy_coalesce_support - coalesce support data
+ * @n_rules: maximum number of coalesce rules
+ * @n_patterns: number of supported patterns
+ *	(see nl80211.h for the pattern definition)
+ * @pattern_max_len: maximum length of each pattern
+ * @pattern_min_len: minimum length of each pattern
+ * @max_pkt_offset: maximum Rx packet offset
+ */
+struct wiphy_coalesce_support {
+	int n_rules;
+	int n_patterns;
+	int pattern_max_len;
+	int pattern_min_len;
+	int max_pkt_offset;
+};
+
+/**
  * struct wiphy - wireless hardware description
  * @reg_notifier: the driver's regulatory notification callback,
  *	note that if your driver uses wiphy_apply_custom_regulatory()
@@ -2589,6 +2638,7 @@ struct wiphy_wowlan_support {
  *	802.11-2012 8.4.2.29 for the defined fields.
  * @extended_capabilities_mask: mask of the valid values
  * @extended_capabilities_len: length of the extended capabilities
+ * @wiphy_coalesce_support coalesce: coalesce support information
  */
 struct wiphy {
 	/* assign these fields before you register the wiphy */
@@ -2697,6 +2747,7 @@ struct wiphy {
 	const struct iw_handler_def *wext;
 #endif
 
+	struct wiphy_coalesce_support coalesce;
 	char priv[0] __aligned(NETDEV_ALIGN);
 };
 
diff --git a/include/uapi/linux/nl80211.h b/include/uapi/linux/nl80211.h
index 1fa2bf4..34401a1 100644
--- a/include/uapi/linux/nl80211.h
+++ b/include/uapi/linux/nl80211.h
@@ -126,6 +126,31 @@
  */
 
 /**
+ * DOC: packet coalesce support
+ *
+ * In most cases, host that receives IPv4 and IPv6 multicast/broadcast
+ * packets does not do anything with these packets. Therefore the
+ * reception of these unwanted packets causes unnecessary processing
+ * and power consumption.
+ *
+ * Packet coalesce feature helps to reduce number of received interrupts
+ * to host by buffering these packets in firmware/hardware for some
+ * predefined time. Received interrupt will be generated when one of the
+ * following events occur.
+ * a) Expiration of hardware timer whose expiration time is set to maximum
+ * coalescing delay of matching coalesce rule.
+ * b) Coalescing buffer in hardware reaches it's limit.
+ * c) Packet doesn't match any of the configured coalesce rules.
+ *
+ * User needs to configure following parameters for creating a coalesce
+ * rule.
+ * a) Maximum coalescing delay
+ * b) List of packet patterns which needs to be matched
+ * c) Condition for coalescence. pattern 'match' or 'no match'
+ * Multiple such rules can be created.
+ */
+
+/**
  * enum nl80211_commands - supported nl80211 commands
  *
  * @NL80211_CMD_UNSPEC: unspecified command to catch errors
@@ -648,6 +673,10 @@
  * @NL80211_CMD_CRIT_PROTOCOL_STOP: Indicates the connection reliability can
  *	return back to normal.
  *
+ * @NL80211_CMD_GET_COALESCE: Get currently supported coalesce rules.
+ *
+ * @NL80211_CMD_SET_COALESCE: Add new coalesce rule.
+ *
  * @NL80211_CMD_MAX: highest used command number
  * @__NL80211_CMD_AFTER_LAST: internal use
  */
@@ -810,6 +839,9 @@ enum nl80211_commands {
 	NL80211_CMD_CRIT_PROTOCOL_START,
 	NL80211_CMD_CRIT_PROTOCOL_STOP,
 
+	NL80211_CMD_GET_COALESCE,
+	NL80211_CMD_SET_COALESCE,
+
 	/* add new commands above here */
 
 	/* used to define NL80211_CMD_MAX below */
@@ -1431,6 +1463,8 @@ enum nl80211_commands {
  * @NL80211_ATTR_MAX_CRIT_PROT_DURATION: duration in milliseconds in which
  *      the connection should have increased reliability (u16).
  *
+ * @NL80211_ATTR_COALESCE_RULE: Coalesce rule information.
+ *
  * @NL80211_ATTR_MAX: highest attribute number currently defined
  * @__NL80211_ATTR_AFTER_LAST: internal use
  */
@@ -1729,6 +1763,8 @@ enum nl80211_attrs {
 	NL80211_ATTR_CRIT_PROT_ID,
 	NL80211_ATTR_MAX_CRIT_PROT_DURATION,
 
+	NL80211_ATTR_COALESCE_RULE,
+
 	/* add attributes here, update the policy in nl80211.c */
 
 	__NL80211_ATTR_AFTER_LAST,
@@ -3027,7 +3063,6 @@ enum nl80211_cqm_rssi_threshold_event {
 	NL80211_CQM_RSSI_BEACON_LOSS_EVENT,
 };
 
-
 /**
  * enum nl80211_tx_power_setting - TX power adjustment
  * @NL80211_TX_POWER_AUTOMATIC: automatically determine transmit power
@@ -3079,8 +3114,10 @@ enum nl80211_packet_pattern_attr {
  * @max_pkt_offset: maximum Rx packet offset
  *
  * This struct is carried in %NL80211_WOWLAN_TRIG_PKT_PATTERN when
- * that is part of %NL80211_ATTR_WOWLAN_TRIGGERS_SUPPORTED in the
- * capability information given by the kernel to userspace.
+ * that is part of %NL80211_ATTR_WOWLAN_TRIGGERS_SUPPORTED or in
+ * %NL80211_ATTR_COALESCE_RULE_PKT_PATTERN when that is part of
+ * %NL80211_ATTR_COALESCE_RULE in the capability information given
+ * by the kernel to userspace.
  */
 struct nl80211_pattern_support {
 	__u32 max_patterns;
@@ -3299,6 +3336,41 @@ enum nl80211_wowlan_tcp_attrs {
 };
 
 /**
+ * struct nl80211_coalesce_rule_support - coalesce rule support information
+ * @max_rules: maximum number of rules supported
+ * @pat: packet pattern support information
+ *
+ * This struct is carried in %NL80211_ATTR_COALESCE_RULE in the
+ * capability information given by the kernel to userspace.
+ */
+struct nl80211_coalesce_rule_support {
+	__u32 max_rules;
+	struct nl80211_pattern_support pat;
+} __attribute__((packed));
+
+/**
+ * enum nl80211_attr_coalesce_rule - coalesce rule attribute
+ * @__NL80211_COALESCE_RULE_INVALID: invalid number for nested attribute
+ * @NL80211_ATTR_COALESCE_RULE_DELAY: delay in msecs used for packet coalescing
+ * @NL80211_ATTR_COALESCE_RULE_CONDITION: condition for packet coalescence.
+ *	i.e. pattern 'match' or 'no match'
+ * @NL80211_ATTR_COALESCE_RULE_PKT_PATTERN: packet offset, pattern is matched
+ *	after these fixed number of bytes of received packet
+ * @NUM_NL80211_ATTR_COALESCE_RULE: number of attributes
+ * @NL80211_ATTR_COALESCE_RULE_MAX: max attribute number
+ */
+enum nl80211_attr_coalesce_rule {
+	__NL80211_COALESCE_RULE_INVALID,
+	NL80211_ATTR_COALESCE_RULE_DELAY,
+	NL80211_ATTR_COALESCE_RULE_CONDITION,
+	NL80211_ATTR_COALESCE_RULE_PKT_PATTERN,
+
+	/* keep last */
+	NUM_NL80211_ATTR_COALESCE_RULE,
+	NL80211_ATTR_COALESCE_RULE_MAX = NUM_NL80211_ATTR_COALESCE_RULE - 1
+};
+
+/**
  * enum nl80211_iface_limit_attrs - limit attributes
  * @NL80211_IFACE_LIMIT_UNSPEC: (reserved)
  * @NL80211_IFACE_LIMIT_MAX: maximum number of interfaces that
@@ -3578,6 +3650,9 @@ enum nl80211_ap_sme_features {
  *	Peering Management entity which may be implemented by registering for
  *	beacons or NL80211_CMD_NEW_PEER_CANDIDATE events. The mesh beacon is
  *	still generated by the driver.
+ * @NL80211_FEATURE_PACKET_COALESCE: This driver support packet coalescing
+ *	feature. Packets are buffered in firmware based on configured rules
+ *	to reduce unwanted packet or interrupt to host.
  */
 enum nl80211_feature_flags {
 	NL80211_FEATURE_SK_TX_STATUS			= 1 << 0,
@@ -3597,6 +3672,7 @@ enum nl80211_feature_flags {
 	NL80211_FEATURE_ADVERTISE_CHAN_LIMITS		= 1 << 14,
 	NL80211_FEATURE_FULL_AP_CLIENT_STATE		= 1 << 15,
 	NL80211_FEATURE_USERSPACE_MPM			= 1 << 16,
+	NL80211_FEATURE_PACKET_COALESCE			= 1 << 17,
 };
 
 /**
diff --git a/net/wireless/core.c b/net/wireless/core.c
index 68f0c96..72a41da 100644
--- a/net/wireless/core.c
+++ b/net/wireless/core.c
@@ -737,6 +737,8 @@ void wiphy_unregister(struct wiphy *wiphy)
 	if (rdev->wowlan && rdev->ops->set_wakeup)
 		rdev_set_wakeup(rdev, false);
 	cfg80211_rdev_free_wowlan(rdev);
+
+	cfg80211_rdev_free_coalesce(rdev);
 }
 EXPORT_SYMBOL(wiphy_unregister);
 
diff --git a/net/wireless/core.h b/net/wireless/core.h
index fd35dae..a0feff6 100644
--- a/net/wireless/core.h
+++ b/net/wireless/core.h
@@ -91,6 +91,8 @@ struct cfg80211_registered_device {
 	/* netlink port which started critical protocol (0 means not started) */
 	u32 crit_proto_nlportid;
 
+	struct cfg80211_coalesce *coalesce;
+
 	/* must be last because of the way we do wiphy_priv(),
 	 * and it should at least be aligned to NETDEV_ALIGN */
 	struct wiphy wiphy __aligned(NETDEV_ALIGN);
@@ -119,6 +121,26 @@ cfg80211_rdev_free_wowlan(struct cfg80211_registered_device *rdev)
 	kfree(rdev->wowlan);
 }
 
+static inline void
+cfg80211_rdev_free_coalesce(struct cfg80211_registered_device *rdev)
+{
+	int i, j;
+	struct cfg80211_coalesce_rules *rule;
+
+	if (!rdev->coalesce)
+		return;
+
+	for (i = 0; i < rdev->coalesce->n_rules; i++) {
+		rule = rdev->coalesce->rules[i];
+		for (j = 0; j < rule->n_patterns; j++)
+			kfree(rule->patterns[j].mask);
+		kfree(rule->patterns);
+		kfree(rule);
+	}
+	kfree(rdev->coalesce->rules);
+	kfree(rdev->coalesce);
+}
+
 extern struct workqueue_struct *cfg80211_wq;
 extern struct mutex cfg80211_mutex;
 extern struct list_head cfg80211_rdev_list;
diff --git a/net/wireless/nl80211.c b/net/wireless/nl80211.c
index 5f640de..8cfca0c 100644
--- a/net/wireless/nl80211.c
+++ b/net/wireless/nl80211.c
@@ -432,6 +432,14 @@ nl80211_wowlan_tcp_policy[NUM_NL80211_WOWLAN_TCP] = {
 	[NL80211_WOWLAN_TCP_WAKE_MASK] = { .len = 1 },
 };
 
+/* policy for coalesce rule attributes */
+static const struct nla_policy
+nl80211_coalesce_policy[NUM_NL80211_ATTR_COALESCE_RULE] = {
+	[NL80211_ATTR_COALESCE_RULE_DELAY] = { .type = NLA_U32 },
+	[NL80211_ATTR_COALESCE_RULE_CONDITION] = { .type = NLA_U8 },
+	[NL80211_ATTR_COALESCE_RULE_PKT_PATTERN] = { .type = NLA_NESTED },
+};
+
 /* policy for GTK rekey offload attributes */
 static const struct nla_policy
 nl80211_rekey_policy[NUM_NL80211_REKEY_DATA] = {
@@ -1035,6 +1043,27 @@ static int nl80211_send_wowlan(struct sk_buff *msg,
 }
 #endif
 
+static int nl80211_send_coalesce(struct sk_buff *msg,
+				 struct cfg80211_registered_device *dev,
+				 bool large)
+{
+	struct nl80211_coalesce_rule_support rule;
+
+	if (!dev->wiphy.coalesce.n_patterns || !dev->wiphy.coalesce.n_rules)
+		return 0;
+
+	rule.max_rules = dev->wiphy.coalesce.n_rules;
+	rule.pat.max_patterns = dev->wiphy.coalesce.n_patterns;
+	rule.pat.min_pattern_len = dev->wiphy.coalesce.pattern_min_len;
+	rule.pat.max_pattern_len = dev->wiphy.coalesce.pattern_max_len;
+	rule.pat.max_pkt_offset = dev->wiphy.coalesce.max_pkt_offset;
+
+	if (nla_put(msg, NL80211_ATTR_COALESCE_RULE, sizeof(rule), &rule))
+		return -ENOBUFS;
+
+	return 0;
+}
+
 static int nl80211_send_band_rateinfo(struct sk_buff *msg,
 				      struct ieee80211_supported_band *sband)
 {
@@ -1546,6 +1575,12 @@ static int nl80211_send_wiphy(struct cfg80211_registered_device *dev,
 			    dev->wiphy.vht_capa_mod_mask))
 			goto nla_put_failure;
 
+		(*split_start)++;
+		break;
+	case 10:
+		if (nl80211_send_coalesce(msg, dev, split))
+			goto nla_put_failure;
+
 		/* done */
 		*split_start = 0;
 		break;
@@ -8007,6 +8042,221 @@ static int nl80211_set_wowlan(struct sk_buff *skb, struct genl_info *info)
 }
 #endif
 
+static int nl80211_send_coalesce_rules(struct sk_buff *msg,
+				       struct cfg80211_registered_device *rdev)
+{
+	struct nlattr *nl_pats, *nl_pat, *nl_rule, *nl_rules;
+	int i, j, pat_len;
+	struct cfg80211_coalesce_rules *rule;
+
+	if (!rdev->coalesce->n_rules)
+		return 0;
+
+	nl_rules = nla_nest_start(msg, NL80211_ATTR_COALESCE_RULE);
+	if (!nl_rules)
+		return -ENOBUFS;
+
+	for (i = 0; i < rdev->coalesce->n_rules; i++) {
+		nl_rule = nla_nest_start(msg, i + 1);
+		if (!nl_rule)
+			return -ENOBUFS;
+
+		rule = rdev->coalesce->rules[i];
+		if (nla_put_u32(msg, NL80211_ATTR_COALESCE_RULE_DELAY,
+				rule->delay))
+			return -ENOBUFS;
+
+		if (nla_put_u8(msg, NL80211_ATTR_COALESCE_RULE_CONDITION,
+			       rule->condition))
+			return -ENOBUFS;
+
+		nl_pats = nla_nest_start(msg,
+				NL80211_ATTR_COALESCE_RULE_PKT_PATTERN);
+		if (!nl_pats)
+			return -ENOBUFS;
+
+		for (j = 0; j < rule->n_patterns; j++) {
+			nl_pat = nla_nest_start(msg, j + 1);
+			if (!nl_pat)
+				return -ENOBUFS;
+			pat_len = rule->patterns[j].pattern_len;
+			if (nla_put(msg, NL80211_PKTPAT_MASK,
+				    DIV_ROUND_UP(pat_len, 8),
+				    rule->patterns[j].mask) ||
+			    nla_put(msg, NL80211_PKTPAT_PATTERN, pat_len,
+				    rule->patterns[j].pattern) ||
+			    nla_put_u32(msg, NL80211_PKTPAT_OFFSET,
+					rule->patterns[j].pkt_offset))
+				return -ENOBUFS;
+			nla_nest_end(msg, nl_pat);
+		}
+		nla_nest_end(msg, nl_pats);
+		nla_nest_end(msg, nl_rule);
+	}
+	nla_nest_end(msg, nl_rules);
+
+	return 0;
+}
+
+static int nl80211_get_coalesce(struct sk_buff *skb, struct genl_info *info)
+{
+	struct cfg80211_registered_device *rdev = info->user_ptr[0];
+	struct sk_buff *msg;
+	void *hdr;
+
+	if (!rdev->wiphy.coalesce.n_patterns || !rdev->wiphy.coalesce.n_rules)
+		return -EOPNOTSUPP;
+
+	msg = nlmsg_new(NLMSG_DEFAULT_SIZE, GFP_KERNEL);
+	if (!msg)
+		return -ENOMEM;
+
+	hdr = nl80211hdr_put(msg, info->snd_portid, info->snd_seq, 0,
+			     NL80211_CMD_GET_COALESCE);
+	if (!hdr)
+		goto nla_put_failure;
+
+	if (rdev->coalesce && nl80211_send_coalesce_rules(msg, rdev))
+		goto nla_put_failure;
+
+	genlmsg_end(msg, hdr);
+	return genlmsg_reply(msg, info);
+
+nla_put_failure:
+	nlmsg_free(msg);
+	return -ENOBUFS;
+}
+
+static int nl80211_set_coalesce(struct sk_buff *skb, struct genl_info *info)
+{
+	struct cfg80211_registered_device *rdev = info->user_ptr[0];
+	struct nlattr *tb[NUM_NL80211_ATTR_COALESCE_RULE];
+	struct wiphy_coalesce_support *coalesce = &rdev->wiphy.coalesce;
+	struct cfg80211_coalesce_rules new_rule = {};
+	struct cfg80211_coalesce_rules *nrule;
+	int err, i;
+
+	if (!(rdev->wiphy.features & NL80211_FEATURE_PACKET_COALESCE))
+		return -EOPNOTSUPP;
+	if (!rdev->ops->set_coalesce)
+		return -EOPNOTSUPP;
+
+	if (!info->attrs[NL80211_ATTR_COALESCE_RULE]) {
+		cfg80211_rdev_free_coalesce(rdev);
+		rdev->coalesce = NULL;
+		rdev->ops->set_coalesce(&rdev->wiphy, NULL);
+		return 0;
+	}
+
+	if (!rdev->coalesce) {
+		rdev->coalesce = kzalloc(sizeof(*rdev->coalesce), GFP_KERNEL);
+		rdev->coalesce->rules = kcalloc(coalesce->n_rules,
+						sizeof(void *), GFP_KERNEL);
+	}
+
+	if (rdev->coalesce->n_rules >= coalesce->n_rules)
+		return -EOPNOTSUPP;
+
+	err = nla_parse(tb, NL80211_ATTR_COALESCE_RULE_MAX,
+			nla_data(info->attrs[NL80211_ATTR_COALESCE_RULE]),
+			nla_len(info->attrs[NL80211_ATTR_COALESCE_RULE]),
+			nl80211_coalesce_policy);
+	if (err)
+		return err;
+
+	new_rule.delay = nla_get_u32(tb[NL80211_ATTR_COALESCE_RULE_DELAY]);
+	new_rule.condition =
+		nla_get_u8(tb[NL80211_ATTR_COALESCE_RULE_CONDITION]);
+
+	if (tb[NL80211_ATTR_COALESCE_RULE_PKT_PATTERN]) {
+		struct nlattr *pat;
+		int n_patterns = 0;
+		int rem, pat_len, mask_len, pkt_offset;
+		struct nlattr *pat_tb[NUM_NL80211_PKTPAT];
+
+		nla_for_each_nested(pat,
+				    tb[NL80211_ATTR_COALESCE_RULE_PKT_PATTERN],
+				    rem)
+			n_patterns++;
+		if (n_patterns > coalesce->n_patterns)
+			return -EINVAL;
+
+		new_rule.patterns = kcalloc(n_patterns,
+						sizeof(new_rule.patterns[0]),
+						GFP_KERNEL);
+		if (!new_rule.patterns)
+			return -ENOMEM;
+
+		new_rule.n_patterns = n_patterns;
+		i = 0;
+
+		nla_for_each_nested(pat,
+				    tb[NL80211_ATTR_COALESCE_RULE_PKT_PATTERN],
+				    rem) {
+			nla_parse(pat_tb, MAX_NL80211_PKTPAT, nla_data(pat),
+				  nla_len(pat), NULL);
+			err = -EINVAL;
+			if (!pat_tb[NL80211_PKTPAT_MASK] ||
+			    !pat_tb[NL80211_PKTPAT_PATTERN])
+				goto error;
+			pat_len = nla_len(pat_tb[NL80211_PKTPAT_PATTERN]);
+			mask_len = DIV_ROUND_UP(pat_len, 8);
+			if (nla_len(pat_tb[NL80211_PKTPAT_MASK]) !=
+			    mask_len)
+				goto error;
+			if (pat_len > coalesce->pattern_max_len ||
+			    pat_len < coalesce->pattern_min_len)
+				goto error;
+
+			if (!pat_tb[NL80211_PKTPAT_OFFSET])
+				pkt_offset = 0;
+			else
+				pkt_offset = nla_get_u32(
+					pat_tb[NL80211_PKTPAT_OFFSET]);
+			if (pkt_offset > coalesce->max_pkt_offset)
+				goto error;
+			new_rule.patterns[i].pkt_offset = pkt_offset;
+
+			new_rule.patterns[i].mask =
+				kmalloc(mask_len + pat_len, GFP_KERNEL);
+			if (!new_rule.patterns[i].mask) {
+				err = -ENOMEM;
+				goto error;
+			}
+			new_rule.patterns[i].pattern =
+				new_rule.patterns[i].mask + mask_len;
+			memcpy(new_rule.patterns[i].mask,
+			       nla_data(pat_tb[NL80211_PKTPAT_MASK]),
+			       mask_len);
+			new_rule.patterns[i].pattern_len = pat_len;
+			memcpy(new_rule.patterns[i].pattern,
+			       nla_data(pat_tb[NL80211_PKTPAT_PATTERN]),
+			       pat_len);
+			i++;
+		}
+	}
+
+	nrule = kmemdup(&new_rule, sizeof(new_rule), GFP_KERNEL);
+	if (!nrule) {
+		err = -ENOMEM;
+		goto error;
+	}
+
+	err = rdev->ops->set_coalesce(&rdev->wiphy, nrule);
+	if (err)
+		goto error;
+
+	rdev->coalesce->rules[rdev->coalesce->n_rules++] = nrule;
+
+	return 0;
+
+error:
+	for (i = 0; i < new_rule.n_patterns; i++)
+		kfree(new_rule.patterns[i].mask);
+	kfree(new_rule.patterns);
+	return err;
+}
+
 static int nl80211_set_rekey_data(struct sk_buff *skb, struct genl_info *info)
 {
 	struct cfg80211_registered_device *rdev = info->user_ptr[0];
@@ -9026,6 +9276,21 @@ static struct genl_ops nl80211_ops[] = {
 		.flags = GENL_ADMIN_PERM,
 		.internal_flags = NL80211_FLAG_NEED_WDEV_UP |
 				  NL80211_FLAG_NEED_RTNL,
+	},
+	{
+		.cmd = NL80211_CMD_GET_COALESCE,
+		.doit = nl80211_get_coalesce,
+		.policy = nl80211_policy,
+		.internal_flags = NL80211_FLAG_NEED_WIPHY |
+				  NL80211_FLAG_NEED_RTNL,
+	},
+	{
+		.cmd = NL80211_CMD_SET_COALESCE,
+		.doit = nl80211_set_coalesce,
+		.policy = nl80211_policy,
+		.flags = GENL_ADMIN_PERM,
+		.internal_flags = NL80211_FLAG_NEED_WIPHY |
+				  NL80211_FLAG_NEED_RTNL,
 	}
 };
 
-- 
1.8.0

Re: [PATCH v2 1/2] cfg80211/nl80211: rename packet pattern related structures and enums

[Johannes Berg]

On Thu, 2013-05-23 at 14:35 -0700, Bing Zhao wrote:

> -enum nl80211_wowlan_packet_pattern_attr {

I think you missed a #define for this?

> +/* only for backward compatibility */
> +#define __NL80211_WOWLAN_PKTPAT_INVALID __NL80211_INVALID,

that , at the end looks like a copy/paste error, but in fact shouldn't
it be "__NL80211_PKTPAT_INVALID" anyway?


>   * that is part of %NL80211_ATTR_WOWLAN_TRIGGERS_SUPPORTED in the
>   * capability information given by the kernel to userspace.

Should this be updated?

johannes

Re: [PATCH v2 2/2] cfg80211/nl80211: Add packet coalesce support

[Johannes Berg]

Some smallish things.

> In most cases, host that receives IPv4 and IPv6 multicast/broadcast
> packets does not do anything with these packets. Therefore the
> reception of these unwanted packets causes unnecessary processing
> and power consumption.

This is curious, you already discard those that you don't care about by
way of multicast filtering, no? What's the added advantage here? Would
you coalesce interrupts for those packets that pass the filter(s)? But
those packets are packets that the host cares about, no?

>  /**
> + * struct cfg80211_coalesce_rules - Coalesce rule parameters
> + *
> + * This structure defines coalesce rule for the device.
> + * @delay: maximum coalescing delay in msecs.
> + * @condition: condition for packet coalescence.
> + *	i.e. pattern 'match' or 'no match'
> + * @patterns: array of packet patterns
> + * @n_patterns: number of patterns
> + */
> +struct cfg80211_coalesce_rules {
> +	int delay;
> +	u8 condition;

seems like "condition" should be of some enum type? presumably an
nl80211 enum type that userspace can use?

> +	struct cfg80211_pkt_pattern *patterns;

const?

> +struct cfg80211_coalesce {
> +	struct cfg80211_coalesce_rules **rules;
> +	int n_rules;
> +};

I think you can pass these as two function arguments rather than a
separate struct.

> @@ -3027,7 +3063,6 @@ enum nl80211_cqm_rssi_threshold_event {
>  	NL80211_CQM_RSSI_BEACON_LOSS_EVENT,
>  };
>  
> -
>  /**
>   * enum nl80211_tx_power_setting - TX power adjustment
>   * @NL80211_TX_POWER_AUTOMATIC: automatically determine transmit power

seems spurious :)

>   * This struct is carried in %NL80211_WOWLAN_TRIG_PKT_PATTERN when
> - * that is part of %NL80211_ATTR_WOWLAN_TRIGGERS_SUPPORTED in the
> - * capability information given by the kernel to userspace.
> + * that is part of %NL80211_ATTR_WOWLAN_TRIGGERS_SUPPORTED or in
> + * %NL80211_ATTR_COALESCE_RULE_PKT_PATTERN when that is part of
> + * %NL80211_ATTR_COALESCE_RULE in the capability information given
> + * by the kernel to userspace.

Ah, I think here you're updating what I asked about before.

> + * @NL80211_ATTR_COALESCE_RULE_CONDITION: condition for packet coalescence.
> + *	i.e. pattern 'match' or 'no match'

I'm sure the condition attribute isn't a string, so this should say
which enum to take the value from?

> + * @NL80211_FEATURE_PACKET_COALESCE: This driver support packet coalescing
> + *	feature. Packets are buffered in firmware based on configured rules
> + *	to reduce unwanted packet or interrupt to host.

I don't think you need this, since you have this:

> +       struct wiphy_coalesce_support coalesce;

Actually, you should probably make that a pointer, then it can be NULL
for drivers not supporting it, and static const for those that do. Means
you should make it a const pointer, of course.

Userspace can tell by checking if the support is advertised.


> +static inline void
> +cfg80211_rdev_free_coalesce(struct cfg80211_registered_device *rdev)

That's fairly big, would prefer not to inline it.

> +static int nl80211_set_coalesce(struct sk_buff *skb, struct genl_info *info)
> +{
> +	struct cfg80211_registered_device *rdev = info->user_ptr[0];
> +	struct nlattr *tb[NUM_NL80211_ATTR_COALESCE_RULE];
> +	struct wiphy_coalesce_support *coalesce = &rdev->wiphy.coalesce;
> +	struct cfg80211_coalesce_rules new_rule = {};
> +	struct cfg80211_coalesce_rules *nrule;
> +	int err, i;
> +
> +	if (!(rdev->wiphy.features & NL80211_FEATURE_PACKET_COALESCE))
> +		return -EOPNOTSUPP;

Then this should check the coalesce pointer of course, which can then be
NULL.

> +	if (!rdev->coalesce) {
> +		rdev->coalesce = kzalloc(sizeof(*rdev->coalesce), GFP_KERNEL);
> +		rdev->coalesce->rules = kcalloc(coalesce->n_rules,
> +						sizeof(void *), GFP_KERNEL);
> +	}
> +
> +	if (rdev->coalesce->n_rules >= coalesce->n_rules)
> +		return -EOPNOTSUPP;

This is bad. You leave rdev->coalesce assigned, but it's completely
invalid data. IMHO you should use a temporary variable and only assign
it when it's fully parsed, freeing it if not. That way, you also don't
kill old values when new invalid values are parsed.

> +	new_rule.delay = nla_get_u32(tb[NL80211_ATTR_COALESCE_RULE_DELAY]);
> +	new_rule.condition =
> +		nla_get_u8(tb[NL80211_ATTR_COALESCE_RULE_CONDITION]);

Needs sanity checking, what if userspace passes the value 17? Does that
mean anything? :)

> +		nla_for_each_nested(pat,
> +				    tb[NL80211_ATTR_COALESCE_RULE_PKT_PATTERN],
> +				    rem) {
> +			nla_parse(pat_tb, MAX_NL80211_PKTPAT, nla_data(pat),
> +				  nla_len(pat), NULL);
> +			err = -EINVAL;
> +			if (!pat_tb[NL80211_PKTPAT_MASK] ||
> +			    !pat_tb[NL80211_PKTPAT_PATTERN])
> +				goto error;
> +			pat_len = nla_len(pat_tb[NL80211_PKTPAT_PATTERN]);
> +			mask_len = DIV_ROUND_UP(pat_len, 8);
> +			if (nla_len(pat_tb[NL80211_PKTPAT_MASK]) !=
> +			    mask_len)
> +				goto error;
> +			if (pat_len > coalesce->pattern_max_len ||
> +			    pat_len < coalesce->pattern_min_len)
> +				goto error;

I wonder if any of this could be refactored with WoWLAN? Not really sure
though, maybe not.

> +	err = rdev->ops->set_coalesce(&rdev->wiphy, nrule);
> +	if (err)
> +		goto error;
> +
> +	rdev->coalesce->rules[rdev->coalesce->n_rules++] = nrule;

Wait ... you can't delete old rules, only add new ones? I think I'm
confused, I thought the SET command was going to overwrite all old
rules?

johannes

RE: [PATCH v2 1/2] cfg80211/nl80211: rename packet pattern related structures and enums

[Amitkumar Karwar]

Hi Johannes,

Thanks for your comments.

> 
> > -enum nl80211_wowlan_packet_pattern_attr {
> 
> I think you missed a #define for this?

It's not used anywhere in iw or kernel code.

> 
> > +/* only for backward compatibility */
> > +#define __NL80211_WOWLAN_PKTPAT_INVALID __NL80211_INVALID,
> 
> that , at the end looks like a copy/paste error, but in fact shouldn't
> it be "__NL80211_PKTPAT_INVALID" anyway?

Thanks for pointing this out. We will correct it in updated version.

> 
> 
> >   * that is part of %NL80211_ATTR_WOWLAN_TRIGGERS_SUPPORTED in the
> >   * capability information given by the kernel to userspace.
> 
> Should this be updated?

It has been updated after adding coalesce support in 2/2 patch.

Thanks,
Amitkumar Karwar

> 
> johannes

RE: [PATCH v2 2/2] cfg80211/nl80211: Add packet coalesce support

[Amitkumar Karwar]

Hi Johannes,

> 
> Some smallish things.
> 
> > In most cases, host that receives IPv4 and IPv6 multicast/broadcast
> > packets does not do anything with these packets. Therefore the
> > reception of these unwanted packets causes unnecessary processing
> > and power consumption.
> 
> This is curious, you already discard those that you don't care about by
> way of multicast filtering, no? What's the added advantage here? Would
> you coalesce interrupts for those packets that pass the filter(s)?

Yes. we don't want to simply discard those packets, because few of them may be useful for the host.
The advantage here is instead of getting random per packet receive interrupts, host receives multiple buffered packets with a single receive interrupt.

> But
> those packets are packets that the host cares about, no?

The assumption is filters are designed in such a way that specified delay in packet processing is acceptable for those packets.

> 
> >  /**
> > + * struct cfg80211_coalesce_rules - Coalesce rule parameters
> > + *
> > + * This structure defines coalesce rule for the device.
> > + * @delay: maximum coalescing delay in msecs.
> > + * @condition: condition for packet coalescence.
> > + *	i.e. pattern 'match' or 'no match'
> > + * @patterns: array of packet patterns
> > + * @n_patterns: number of patterns
> > + */
> > +struct cfg80211_coalesce_rules {
> > +	int delay;
> > +	u8 condition;
> 
> seems like "condition" should be of some enum type? presumably an
> nl80211 enum type that userspace can use?

Sure. We will add an enum for "condition".

> 
> > +	struct cfg80211_pkt_pattern *patterns;
> 
> const?

This can not be a const. It points to an array of patterns. Runtime while adding new rule, buffer is allocated based on number of patterns configured by user and information is filled.

> 
> > +struct cfg80211_coalesce {
> > +	struct cfg80211_coalesce_rules **rules;
> > +	int n_rules;
> > +};
> 
> I think you can pass these as two function arguments rather than a
> separate struct.

This is our global structure for coalesce configuration. We just save it's pointer in rdev structure. It is allocated when user adds first coalesce rule. rdev->coalesce is NULL when user clears the setting.

> 
> > @@ -3027,7 +3063,6 @@ enum nl80211_cqm_rssi_threshold_event {
> >  	NL80211_CQM_RSSI_BEACON_LOSS_EVENT,
> >  };
> >
> > -
> >  /**
> >   * enum nl80211_tx_power_setting - TX power adjustment
> >   * @NL80211_TX_POWER_AUTOMATIC: automatically determine transmit power
> 
> seems spurious :)

Ok. We will revert the change.

> 
> >   * This struct is carried in %NL80211_WOWLAN_TRIG_PKT_PATTERN when
> > - * that is part of %NL80211_ATTR_WOWLAN_TRIGGERS_SUPPORTED in the
> > - * capability information given by the kernel to userspace.
> > + * that is part of %NL80211_ATTR_WOWLAN_TRIGGERS_SUPPORTED or in
> > + * %NL80211_ATTR_COALESCE_RULE_PKT_PATTERN when that is part of
> > + * %NL80211_ATTR_COALESCE_RULE in the capability information given
> > + * by the kernel to userspace.
> 
> Ah, I think here you're updating what I asked about before.
> 
> > + * @NL80211_ATTR_COALESCE_RULE_CONDITION: condition for packet
> coalescence.
> > + *	i.e. pattern 'match' or 'no match'
> 
> I'm sure the condition attribute isn't a string, so this should say
> which enum to take the value from?

Sure. We will update the description using newly added enum for "condition".

> 
> > + * @NL80211_FEATURE_PACKET_COALESCE: This driver support packet
> coalescing
> > + *	feature. Packets are buffered in firmware based on configured rules
> > + *	to reduce unwanted packet or interrupt to host.
> 
> I don't think you need this, since you have this:
> 
> > +       struct wiphy_coalesce_support coalesce;

We will get rid of this flag.

> 
> Actually, you should probably make that a pointer, then it can be NULL
> for drivers not supporting it, and static const for those that do. Means
> you should make it a const pointer, of course.
> 
> Userspace can tell by checking if the support is advertised.

We will use following condition to check if driver is not supporting the feature. (wowlan code does the same thing)

"if (!dev->wiphy.coalesce.n_patterns || !dev->wiphy.coalesce.n_rules)"

> 
> 
> > +static inline void
> > +cfg80211_rdev_free_coalesce(struct cfg80211_registered_device *rdev)
> 
> That's fairly big, would prefer not to inline it.

Ack, thanks.

> 
> > +static int nl80211_set_coalesce(struct sk_buff *skb, struct genl_info
> *info)
> > +{
> > +	struct cfg80211_registered_device *rdev = info->user_ptr[0];
> > +	struct nlattr *tb[NUM_NL80211_ATTR_COALESCE_RULE];
> > +	struct wiphy_coalesce_support *coalesce = &rdev->wiphy.coalesce;
> > +	struct cfg80211_coalesce_rules new_rule = {};
> > +	struct cfg80211_coalesce_rules *nrule;
> > +	int err, i;
> > +
> > +	if (!(rdev->wiphy.features & NL80211_FEATURE_PACKET_COALESCE))
> > +		return -EOPNOTSUPP;
> 
> Then this should check the coalesce pointer of course, which can then be
> NULL.
> 
> > +	if (!rdev->coalesce) {
> > +		rdev->coalesce = kzalloc(sizeof(*rdev->coalesce), GFP_KERNEL);
> > +		rdev->coalesce->rules = kcalloc(coalesce->n_rules,
> > +						sizeof(void *), GFP_KERNEL);
> > +	}
> > +
> > +	if (rdev->coalesce->n_rules >= coalesce->n_rules)
> > +		return -EOPNOTSUPP;
> 
> This is bad. You leave rdev->coalesce assigned, but it's completely
> invalid data. IMHO you should use a temporary variable and only assign
> it when it's fully parsed, freeing it if not. That way, you also don't
> kill old values when new invalid values are parsed.

"iw coalesce add" command adds one coalesce rule at a time. "iw coalesce disable" command removes all configured rules.

"rdev->coalesce" will be created while adding first rule. "if (!rdev->coalesce)" condition will be false for further rules.

"if (rdev->coalesce->n_rules >= coalesce->n_rules)" condition becomes true when number of configured rules becomes equal to number of supported rules advertised by the driver. So we can't accommodate new rule in this case.

Therefore we don't leave rdev->coalesce assigned with invalid data.

We are already using 'new_rule' as a temporary variable for storing new rule information and freeing it in failure cases (similar to wowlan code).

> 
> > +	new_rule.delay = nla_get_u32(tb[NL80211_ATTR_COALESCE_RULE_DELAY]);
> > +	new_rule.condition =
> > +		nla_get_u8(tb[NL80211_ATTR_COALESCE_RULE_CONDITION]);
> 
> Needs sanity checking, what if userspace passes the value 17? Does that
> mean anything? :)

Ack. We will add sanity check to allow only valid enum values.

> 
> > +		nla_for_each_nested(pat,
> > +				    tb[NL80211_ATTR_COALESCE_RULE_PKT_PATTERN],
> > +				    rem) {
> > +			nla_parse(pat_tb, MAX_NL80211_PKTPAT, nla_data(pat),
> > +				  nla_len(pat), NULL);
> > +			err = -EINVAL;
> > +			if (!pat_tb[NL80211_PKTPAT_MASK] ||
> > +			    !pat_tb[NL80211_PKTPAT_PATTERN])
> > +				goto error;
> > +			pat_len = nla_len(pat_tb[NL80211_PKTPAT_PATTERN]);
> > +			mask_len = DIV_ROUND_UP(pat_len, 8);
> > +			if (nla_len(pat_tb[NL80211_PKTPAT_MASK]) !=
> > +			    mask_len)
> > +				goto error;
> > +			if (pat_len > coalesce->pattern_max_len ||
> > +			    pat_len < coalesce->pattern_min_len)
> > +				goto error;
> 
> I wonder if any of this could be refactored with WoWLAN? Not really sure
> though, maybe not.

Refactoring is little difficult here. We need to define new structure for pattern specific support variables (n_patterns, pattern_max_len, pattern_min_len, max_pkt_offset) which can be passed to a common routine created after refactoring. This will affect readability at many other places due to following replacements.
wiphy.wowlan.pattern_min_len -> wiphy.wowlan.pattern.pattern_min_len
wiphy.coalesce.pattern_min_len -> wiphy.coalesce.pattern.pattern_min_len
and so on.

> 
> > +	err = rdev->ops->set_coalesce(&rdev->wiphy, nrule);
> > +	if (err)
> > +		goto error;
> > +
> > +	rdev->coalesce->rules[rdev->coalesce->n_rules++] = nrule;
> 
> Wait ... you can't delete old rules, only add new ones? I think I'm
> confused, I thought the SET command was going to overwrite all old
> rules?

SET command either adds new coalesce rule or removes all existing rules.
Corresponding iw commands are 'add' and 'disable'

Please let me know if you want any specific modifications in the code/logic.

Thanks,
Amitkumar Karwar

> 
> johannes

Re: [PATCH v2 2/2] cfg80211/nl80211: Add packet coalesce support

[Johannes Berg]

Hi,

sorry for the late reply. I did see you sent another version, but wanted
to comment here anyway.

> > This is curious, you already discard those that you don't care about by
> > way of multicast filtering, no? What's the added advantage here? Would
> > you coalesce interrupts for those packets that pass the filter(s)?
> 
> Yes. we don't want to simply discard those packets, because few of
> them may be useful for the host.
> The advantage here is instead of getting random per packet receive
> interrupts, host receives multiple buffered packets with a single
> receive interrupt.

I guess that makes some sense.

> > Actually, you should probably make that a pointer, then it can be NULL
> > for drivers not supporting it, and static const for those that do. Means
> > you should make it a const pointer, of course.
> > 
> > Userspace can tell by checking if the support is advertised.
> 
> We will use following condition to check if driver is not supporting
> the feature. (wowlan code does the same thing)
> 
> "if (!dev->wiphy.coalesce.n_patterns || !dev->wiphy.coalesce.n_rules)"

Hmm. I think wowlan should probably be changed though, no need to make
the struct larger just for this, right? Not all drivers are going to
support it.

For WoWLAN I'm going to apply this:
http://p.sipsolutions.net/fa808e0722a7624a.txt

> > This is bad. You leave rdev->coalesce assigned, but it's completely
> > invalid data. IMHO you should use a temporary variable and only assign
> > it when it's fully parsed, freeing it if not. That way, you also don't
> > kill old values when new invalid values are parsed.
> 
> "iw coalesce add" command adds one coalesce rule at a time. "iw
> coalesce disable" command removes all configured rules.

Can you please change this to "iw coalesce set" and make it set all the
rules in one? Otherwise you're going to have very awkward races and need
to always clear etc. The code would also be easier, though obviously
you'd need to be able to specify multiple rules at the same time.

johannes

RE: [PATCH v2 2/2] cfg80211/nl80211: Add packet coalesce support

[Amitkumar Karwar]

Hi Johannes,

> Hi,
> 
> sorry for the late reply. I did see you sent another version, but wanted
> to comment here anyway.
> 
> > > This is curious, you already discard those that you don't care about
> by
> > > way of multicast filtering, no? What's the added advantage here? Would
> > > you coalesce interrupts for those packets that pass the filter(s)?
> >
> > Yes. we don't want to simply discard those packets, because few of
> > them may be useful for the host.
> > The advantage here is instead of getting random per packet receive
> > interrupts, host receives multiple buffered packets with a single
> > receive interrupt.
> 
> I guess that makes some sense.
> 
> > > Actually, you should probably make that a pointer, then it can be NULL
> > > for drivers not supporting it, and static const for those that do.
> Means
> > > you should make it a const pointer, of course.
> > >
> > > Userspace can tell by checking if the support is advertised.
> >
> > We will use following condition to check if driver is not supporting
> > the feature. (wowlan code does the same thing)
> >
> > "if (!dev->wiphy.coalesce.n_patterns || !dev->wiphy.coalesce.n_rules)"
> 
> Hmm. I think wowlan should probably be changed though, no need to make
> the struct larger just for this, right? Not all drivers are going to
> support it.
> 
> For WoWLAN I'm going to apply this:
> http://p.sipsolutions.net/fa808e0722a7624a.txt
> 

This looks good. We don't need to unnecessarily allocate space in wiphy. We will make similar changes for coalesce.

> > > This is bad. You leave rdev->coalesce assigned, but it's completely
> > > invalid data. IMHO you should use a temporary variable and only assign
> > > it when it's fully parsed, freeing it if not. That way, you also don't
> > > kill old values when new invalid values are parsed.
> >
> > "iw coalesce add" command adds one coalesce rule at a time. "iw
> > coalesce disable" command removes all configured rules.
> 
> Can you please change this to "iw coalesce set" and make it set all the
> rules in one?

"iw coalesce add" was used, because command for adding multiple rules at the same time will be a bit lengthy(user will need to enter multiple lists of packet patterns) and syntax check in iw will also need some efforts. 

> Otherwise you're going to have very awkward races and need
> to always clear etc. The code would also be easier, though obviously
> you'd need to be able to specify multiple rules at the same time.

For "iw coalesce set" also user needs to always clear the settings using "iw coalesce disable". Also similar to "coalesce set", for "coalesce add" we clear the settings and free allocations while unloading the driver.

Please let us know if you prefer "coalesce set" over "coalesce add".

Thanks,
Amit

> 
> johannes

RE: [PATCH v2 2/2] cfg80211/nl80211: Add packet coalesce support

[Bing Zhao]

Hi Johannes,

> > Can you please change this to "iw coalesce set" and make it set all the
> > rules in one?
> 
> "iw coalesce add" was used, because command for adding multiple rules at the same time will be a bit
> lengthy(user will need to enter multiple lists of packet patterns) and syntax check in iw will also
> need some efforts.
> 
> > Otherwise you're going to have very awkward races and need
> > to always clear etc. The code would also be easier, though obviously
> > you'd need to be able to specify multiple rules at the same time.
> 
> For "iw coalesce set" also user needs to always clear the settings using "iw coalesce disable". Also
> similar to "coalesce set", for "coalesce add" we clear the settings and free allocations while
> unloading the driver.
> 
> Please let us know if you prefer "coalesce set" over "coalesce add".

Could you please confirm your preference for "coalesce set" over "coalesce add"?

Thanks,
Bing

Re: [PATCH v2 2/2] cfg80211/nl80211: Add packet coalesce support

[Johannes Berg]

On Mon, 2013-06-03 at 09:47 -0700, Amitkumar Karwar wrote:

> "iw coalesce add" was used, because command for adding multiple rules
> at the same time will be a bit lengthy(user will need to enter
> multiple lists of packet patterns) and syntax check in iw will also
> need some efforts. 

I think that this is so much unlike all other nl80211 settings where
they're either refused or replace previous ones entirely that I would
rather not have it. Take connecting for instance, wext allowed you to
set all parameters one by one, etc. This is a bit similar. Also, this
disallows doing checks on the entire configuration easily.

Personally, I see iw as a bit of a test tool so I'm not too concerned
about its sometimes odd command line, but I know giving it lots of
things can be awkward. For TCP wakeup, I've made it parse a small file,
maybe that's an option here as well?

> > Otherwise you're going to have very awkward races and need
> > to always clear etc. The code would also be easier, though obviously
> > you'd need to be able to specify multiple rules at the same time.
> 
> For "iw coalesce set" also user needs to always clear the settings
> using "iw coalesce disable". Also similar to "coalesce set", for
> "coalesce add" we clear the settings and free allocations while
> unloading the driver.
> 
> Please let us know if you prefer "coalesce set" over "coalesce add".

I would much prefer just having set/clear over piecewise configuration.

johannes

RE: [PATCH v2 2/2] cfg80211/nl80211: Add packet coalesce support

[Bing Zhao]

SGkgSm9oYW5uZXMsDQoNCj4gUGVyc29uYWxseSwgSSBzZWUgaXcgYXMgYSBiaXQgb2YgYSB0ZXN0
IHRvb2wgc28gSSdtIG5vdCB0b28gY29uY2VybmVkDQo+IGFib3V0IGl0cyBzb21ldGltZXMgb2Rk
IGNvbW1hbmQgbGluZSwgYnV0IEkga25vdyBnaXZpbmcgaXQgbG90cyBvZg0KPiB0aGluZ3MgY2Fu
IGJlIGF3a3dhcmQuIEZvciBUQ1Agd2FrZXVwLCBJJ3ZlIG1hZGUgaXQgcGFyc2UgYSBzbWFsbCBm
aWxlLA0KPiBtYXliZSB0aGF0J3MgYW4gb3B0aW9uIGhlcmUgYXMgd2VsbD8NCg0KWWVzLCB3ZSBj
YW4gcGFyc2UgYSBjb25maWd1cmUgZmlsZSBhcyB3ZWxsLiBUaGFua3MgZm9yIHRoZSBoaW50Lg0K
DQo+ID4gUGxlYXNlIGxldCB1cyBrbm93IGlmIHlvdSBwcmVmZXIgImNvYWxlc2NlIHNldCIgb3Zl
ciAiY29hbGVzY2UgYWRkIi4NCj4gDQo+IEkgd291bGQgbXVjaCBwcmVmZXIganVzdCBoYXZpbmcg
c2V0L2NsZWFyIG92ZXIgcGllY2V3aXNlIGNvbmZpZ3VyYXRpb24uDQoNCk9LLiBXZSB3aWxsIHRh
a2Ugc2V0L2NsZWFyIGFwcHJvYWNoIGluIHYzLg0KDQpUaGFua3MsDQpCaW5nDQo=