[RFC 0/3] RX handling consolidation

[RFC 0/3] RX handling consolidation

[Johannes Berg]

COMPLETELY UNTESTED (but it compiles and looks sane)

Please review, I'll test tomorrow.

johannes

[RFC 1/3] mac80211: remove prepare_for_handlers sdata argument

[Johannes Berg]

From: Johannes Berg <johannes.berg@intel.com>

The first argument to prepare_for_handlers is always
the sdata that can just be stored in rx data directly
(and even already is, in two of four code paths.)

Signed-off-by: Johannes Berg <johannes.berg@intel.com>
---
 net/mac80211/rx.c |   16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

--- wireless-testing.orig/net/mac80211/rx.c	2010-09-23 20:58:22.000000000 +0200
+++ wireless-testing/net/mac80211/rx.c	2010-09-23 21:27:20.000000000 +0200
@@ -2517,10 +2517,10 @@ void ieee80211_release_reorder_timeout(s
 
 /* main receive path */
 
-static int prepare_for_handlers(struct ieee80211_sub_if_data *sdata,
-				struct ieee80211_rx_data *rx,
+static int prepare_for_handlers(struct ieee80211_rx_data *rx,
 				struct ieee80211_hdr *hdr)
 {
+	struct ieee80211_sub_if_data *sdata = rx->sdata;
 	struct sk_buff *skb = rx->skb;
 	struct ieee80211_rx_status *status = IEEE80211_SKB_RXCB(skb);
 	u8 *bssid = ieee80211_get_bssid(hdr, skb->len, sdata->vif.type);
@@ -2660,7 +2660,7 @@ static void __ieee80211_rx_handle_packet
 			rx.sdata = prev_sta->sdata;
 
 			status->rx_flags |= IEEE80211_RX_RA_MATCH;
-			prepares = prepare_for_handlers(rx.sdata, &rx, hdr);
+			prepares = prepare_for_handlers(&rx, hdr);
 			if (!prepares)
 				goto next_sta;
 
@@ -2694,7 +2694,7 @@ next_sta:
 			rx.sdata = prev_sta->sdata;
 
 			status->rx_flags |= IEEE80211_RX_RA_MATCH;
-			prepares = prepare_for_handlers(rx.sdata, &rx, hdr);
+			prepares = prepare_for_handlers(&rx, hdr);
 			if (!prepares)
 				prev_sta = NULL;
 
@@ -2737,15 +2737,15 @@ next_sta:
 			}
 
 			rx.sta = sta_info_get_bss(prev, hdr->addr2);
+			rx.sdata = prev;
 
 			status->rx_flags |= IEEE80211_RX_RA_MATCH;
-			prepares = prepare_for_handlers(prev, &rx, hdr);
+			prepares = prepare_for_handlers(&rx, hdr);
 
 			if (!prepares)
 				goto next;
 
 			if (status->flag & RX_FLAG_MMIC_ERROR) {
-				rx.sdata = prev;
 				if (status->rx_flags & IEEE80211_RX_RA_MATCH)
 					ieee80211_rx_michael_mic_report(hdr,
 									&rx);
@@ -2772,15 +2772,15 @@ next:
 
 		if (prev) {
 			rx.sta = sta_info_get_bss(prev, hdr->addr2);
+			rx.sdata = prev;
 
 			status->rx_flags |= IEEE80211_RX_RA_MATCH;
-			prepares = prepare_for_handlers(prev, &rx, hdr);
+			prepares = prepare_for_handlers(&rx, hdr);
 
 			if (!prepares)
 				prev = NULL;
 
 			if (prev && status->flag & RX_FLAG_MMIC_ERROR) {
-				rx.sdata = prev;
 				if (status->rx_flags & IEEE80211_RX_RA_MATCH)
 					ieee80211_rx_michael_mic_report(hdr,
 									&rx);

[RFC 2/3] mac80211: consolidate packet processing

[Johannes Berg]

From: Johannes Berg <johannes.berg@intel.com>

There are now four instances of vaguely the same
code that does packet preparation, checking if
for MMIC errors and reporting them, and then
invoking packet processing. Consolidate all of
these.

Signed-off-by: Johannes Berg <johannes.berg@intel.com>
---
 net/mac80211/rx.c |  164 ++++++++++++++++++++----------------------------------
 1 file changed, 62 insertions(+), 102 deletions(-)

--- wireless-testing.orig/net/mac80211/rx.c	2010-09-23 21:27:20.000000000 +0200
+++ wireless-testing/net/mac80211/rx.c	2010-09-23 21:27:23.000000000 +0200
@@ -2453,18 +2453,13 @@ static void ieee80211_rx_handlers(struct
 	}
 }
 
-static void ieee80211_invoke_rx_handlers(struct ieee80211_sub_if_data *sdata,
-					 struct ieee80211_rx_data *rx,
-					 struct sk_buff *skb)
+static void ieee80211_invoke_rx_handlers(struct ieee80211_rx_data *rx)
 {
 	struct sk_buff_head reorder_release;
 	ieee80211_rx_result res = RX_DROP_MONITOR;
 
 	__skb_queue_head_init(&reorder_release);
 
-	rx->skb = skb;
-	rx->sdata = sdata;
-
 #define CALL_RXH(rxh)			\
 	do {				\
 		res = rxh(rx);		\
@@ -2602,6 +2597,51 @@ static int prepare_for_handlers(struct i
 }
 
 /*
+ * This function returns whether or not the SKB
+ * was destined for RX processing or not, which,
+ * if consume is true, is equivalent to whether
+ * or not the skb was consumed.
+ */
+static bool ieee80211_prepare_and_rx_handle(struct ieee80211_rx_data *rx,
+					    struct sk_buff *skb, bool consume)
+{
+	struct ieee80211_local *local = rx->local;
+	struct ieee80211_sub_if_data *sdata = rx->sdata;
+	struct ieee80211_rx_status *status = IEEE80211_SKB_RXCB(rx->skb);
+	struct ieee80211_hdr *hdr = (void *)skb->data;
+	int prepares;
+
+	rx->skb = skb;
+	status->rx_flags |= IEEE80211_RX_RA_MATCH;
+	prepares = prepare_for_handlers(rx, hdr);
+
+	if (!prepares)
+		return false;
+
+	if (status->flag & RX_FLAG_MMIC_ERROR) {
+		if (status->rx_flags & IEEE80211_RX_RA_MATCH)
+			ieee80211_rx_michael_mic_report(hdr, rx);
+		return false;
+	}
+
+	if (!consume) {
+		skb = skb_copy(skb, GFP_ATOMIC);
+		if (!skb) {
+			if (net_ratelimit())
+				wiphy_debug(local->hw.wiphy,
+					"failed to copy multicast frame for %s\n",
+					sdata->name);
+			return true;
+		}
+
+		rx->skb = skb;
+	}
+
+	ieee80211_invoke_rx_handlers(rx);
+	return true;
+}
+
+/*
  * This is the actual Rx frames handler. as it blongs to Rx path it must
  * be called with rcu_read_lock protection.
  */
@@ -2614,9 +2654,7 @@ static void __ieee80211_rx_handle_packet
 	struct ieee80211_hdr *hdr;
 	__le16 fc;
 	struct ieee80211_rx_data rx;
-	int prepares;
-	struct ieee80211_sub_if_data *prev = NULL;
-	struct sk_buff *skb_new;
+	struct ieee80211_sub_if_data *prev;
 	struct sta_info *sta, *tmp, *prev_sta;
 	bool found_sta = false;
 	int err = 0;
@@ -2649,8 +2687,10 @@ static void __ieee80211_rx_handle_packet
 
 	if (ieee80211_is_data(fc)) {
 		prev_sta = NULL;
+
 		for_each_sta_info(local, hdr->addr2, sta, tmp) {
 			found_sta = true;
+
 			if (!prev_sta) {
 				prev_sta = sta;
 				continue;
@@ -2658,65 +2698,23 @@ static void __ieee80211_rx_handle_packet
 
 			rx.sta = prev_sta;
 			rx.sdata = prev_sta->sdata;
+			ieee80211_prepare_and_rx_handle(&rx, skb, false);
 
-			status->rx_flags |= IEEE80211_RX_RA_MATCH;
-			prepares = prepare_for_handlers(&rx, hdr);
-			if (!prepares)
-				goto next_sta;
-
-			if (status->flag & RX_FLAG_MMIC_ERROR) {
-				if (status->rx_flags & IEEE80211_RX_RA_MATCH)
-					ieee80211_rx_michael_mic_report(hdr, &rx);
-				goto next_sta;
-			}
-
-			/*
-			 * frame was destined for the previous interface
-			 * so invoke RX handlers for it
-			 */
-			skb_new = skb_copy(skb, GFP_ATOMIC);
-			if (!skb_new) {
-				if (net_ratelimit())
-					wiphy_debug(local->hw.wiphy,
-						    "failed to copy multicast"
-						    " frame for %s\n",
-						    prev_sta->sdata->name);
-				goto next_sta;
-			}
-			ieee80211_invoke_rx_handlers(prev_sta->sdata, &rx,
-						     skb_new);
-next_sta:
 			prev_sta = sta;
-		} /* for all STA info */
+		}
 
 		if (prev_sta) {
 			rx.sta = prev_sta;
 			rx.sdata = prev_sta->sdata;
 
-			status->rx_flags |= IEEE80211_RX_RA_MATCH;
-			prepares = prepare_for_handlers(&rx, hdr);
-			if (!prepares)
-				prev_sta = NULL;
-
-			if (prev_sta && status->flag & RX_FLAG_MMIC_ERROR) {
-				if (status->rx_flags & IEEE80211_RX_RA_MATCH)
-					ieee80211_rx_michael_mic_report(hdr, &rx);
-				prev_sta = NULL;
-			}
-		}
-
-
-		if (prev_sta) {
-			ieee80211_invoke_rx_handlers(prev_sta->sdata, &rx, skb);
-			return;
-		} else {
-			if (found_sta) {
-				dev_kfree_skb(skb);
+			if (ieee80211_prepare_and_rx_handle(&rx, skb, true))
 				return;
-			}
 		}
-	} /* if data frame */
+	}
+
 	if (!found_sta) {
+		prev = NULL;
+
 		list_for_each_entry_rcu(sdata, &local->interfaces, list) {
 			if (!ieee80211_sdata_running(sdata))
 				continue;
@@ -2738,35 +2736,8 @@ next_sta:
 
 			rx.sta = sta_info_get_bss(prev, hdr->addr2);
 			rx.sdata = prev;
+			ieee80211_prepare_and_rx_handle(&rx, skb, false);
 
-			status->rx_flags |= IEEE80211_RX_RA_MATCH;
-			prepares = prepare_for_handlers(&rx, hdr);
-
-			if (!prepares)
-				goto next;
-
-			if (status->flag & RX_FLAG_MMIC_ERROR) {
-				if (status->rx_flags & IEEE80211_RX_RA_MATCH)
-					ieee80211_rx_michael_mic_report(hdr,
-									&rx);
-				goto next;
-			}
-
-			/*
-			 * frame was destined for the previous interface
-			 * so invoke RX handlers for it
-			 */
-
-			skb_new = skb_copy(skb, GFP_ATOMIC);
-			if (!skb_new) {
-				if (net_ratelimit())
-					wiphy_debug(local->hw.wiphy,
-						    "failed to copy multicast frame for %s\n",
-						    prev->name);
-				goto next;
-			}
-			ieee80211_invoke_rx_handlers(prev, &rx, skb_new);
-next:
 			prev = sdata;
 		}
 
@@ -2774,24 +2745,13 @@ next:
 			rx.sta = sta_info_get_bss(prev, hdr->addr2);
 			rx.sdata = prev;
 
-			status->rx_flags |= IEEE80211_RX_RA_MATCH;
-			prepares = prepare_for_handlers(&rx, hdr);
-
-			if (!prepares)
-				prev = NULL;
-
-			if (prev && status->flag & RX_FLAG_MMIC_ERROR) {
-				if (status->rx_flags & IEEE80211_RX_RA_MATCH)
-					ieee80211_rx_michael_mic_report(hdr,
-									&rx);
-				prev = NULL;
-			}
+			if (ieee80211_prepare_and_rx_handle(&rx, skb, true))
+				return;
 		}
+
 	}
-	if (prev)
-		ieee80211_invoke_rx_handlers(prev, &rx, skb);
-	else
-		dev_kfree_skb(skb);
+
+	dev_kfree_skb(skb);
 }
 
 /*

[RFC 3/3] mac80211: clean up rx handling wrt. found_sta

[Johannes Berg]

From: Johannes Berg <johannes.berg@intel.com>

If a station was found, then we'll have exited
the function already, so it is not necessary to
have a variable keeping track of it.

Signed-off-by: Johannes Berg <johannes.berg@intel.com>
---
 net/mac80211/rx.c |   60 ++++++++++++++++++++++++------------------------------
 1 file changed, 27 insertions(+), 33 deletions(-)

--- wireless-testing.orig/net/mac80211/rx.c	2010-09-23 21:27:23.000000000 +0200
+++ wireless-testing/net/mac80211/rx.c	2010-09-23 21:27:25.000000000 +0200
@@ -2656,7 +2656,6 @@ static void __ieee80211_rx_handle_packet
 	struct ieee80211_rx_data rx;
 	struct ieee80211_sub_if_data *prev;
 	struct sta_info *sta, *tmp, *prev_sta;
-	bool found_sta = false;
 	int err = 0;
 
 	fc = ((struct ieee80211_hdr *)skb->data)->frame_control;
@@ -2689,8 +2688,6 @@ static void __ieee80211_rx_handle_packet
 		prev_sta = NULL;
 
 		for_each_sta_info(local, hdr->addr2, sta, tmp) {
-			found_sta = true;
-
 			if (!prev_sta) {
 				prev_sta = sta;
 				continue;
@@ -2712,43 +2709,40 @@ static void __ieee80211_rx_handle_packet
 		}
 	}
 
-	if (!found_sta) {
-		prev = NULL;
-
-		list_for_each_entry_rcu(sdata, &local->interfaces, list) {
-			if (!ieee80211_sdata_running(sdata))
-				continue;
-
-			if (sdata->vif.type == NL80211_IFTYPE_MONITOR ||
-			    sdata->vif.type == NL80211_IFTYPE_AP_VLAN)
-				continue;
-
-			/*
-			 * frame is destined for this interface, but if it's
-			 * not also for the previous one we handle that after
-			 * the loop to avoid copying the SKB once too much
-			 */
+	prev = NULL;
 
-			if (!prev) {
-				prev = sdata;
-				continue;
-			}
-
-			rx.sta = sta_info_get_bss(prev, hdr->addr2);
-			rx.sdata = prev;
-			ieee80211_prepare_and_rx_handle(&rx, skb, false);
+	list_for_each_entry_rcu(sdata, &local->interfaces, list) {
+		if (!ieee80211_sdata_running(sdata))
+			continue;
+
+		if (sdata->vif.type == NL80211_IFTYPE_MONITOR ||
+		    sdata->vif.type == NL80211_IFTYPE_AP_VLAN)
+			continue;
+
+		/*
+		 * frame is destined for this interface, but if it's
+		 * not also for the previous one we handle that after
+		 * the loop to avoid copying the SKB once too much
+		 */
 
+		if (!prev) {
 			prev = sdata;
+			continue;
 		}
 
-		if (prev) {
-			rx.sta = sta_info_get_bss(prev, hdr->addr2);
-			rx.sdata = prev;
+		rx.sta = sta_info_get_bss(prev, hdr->addr2);
+		rx.sdata = prev;
+		ieee80211_prepare_and_rx_handle(&rx, skb, false);
 
-			if (ieee80211_prepare_and_rx_handle(&rx, skb, true))
-				return;
-		}
+		prev = sdata;
+	}
+
+	if (prev) {
+		rx.sta = sta_info_get_bss(prev, hdr->addr2);
+		rx.sdata = prev;
 
+		if (ieee80211_prepare_and_rx_handle(&rx, skb, true))
+			return;
 	}
 
 	dev_kfree_skb(skb);

Re: [RFC 0/3] RX handling consolidation

[Johannes Berg]

On Thu, 2010-09-23 at 21:30 +0200, Johannes Berg wrote:
> COMPLETELY UNTESTED (but it compiles and looks sane)
> 
> Please review, I'll test tomorrow.

Oh silly me, I did this on top of the other patches I'm working on,
sorry.

johannes