[PATCH 1/3 v6] mac80211: Refactor ieee80211_iface_work

[PATCH 1/3 v6] mac80211: Refactor ieee80211_iface_work

[Alex Briskin]

Added ieee80211_iface_work_handle_pkt_type function. Moved part of the
code that checks skb->pkt_type from ieee80211_iface_work to
ieee80211_iface_work_handle_pkt_type.

Signed-off-by: Alex Briskin <br.shurik@gmail.com>
---
 net/mac80211/iface.c | 75 +++++++++++++++++++++++++++++++---------------------
 1 file changed, 45 insertions(+), 30 deletions(-)

diff --git a/net/mac80211/iface.c b/net/mac80211/iface.c
index c59af3e..0fc973a 100644
--- a/net/mac80211/iface.c
+++ b/net/mac80211/iface.c
@@ -1198,6 +1198,48 @@ static void ieee80211_if_setup(struct net_device *dev)
 	dev->destructor = ieee80211_if_free;
 }
 
+static int ieee80211_iface_work_handle_pkt_type(struct sk_buff *skb,
+						struct ieee80211_sub_if_data
+						*sdata)
+{
+	struct ieee80211_ra_tid *ra_tid;
+	struct ieee80211_rx_agg *rx_agg;
+	struct ieee80211_local *local = sdata->local;
+	struct sta_info *sta;
+
+	if (skb->pkt_type == IEEE80211_SDATA_QUEUE_AGG_START) {
+		ra_tid = (void *)&skb->cb;
+		ieee80211_start_tx_ba_cb(&sdata->vif, ra_tid->ra, ra_tid->tid);
+	} else if (skb->pkt_type == IEEE80211_SDATA_QUEUE_AGG_STOP) {
+		ra_tid = (void *)&skb->cb;
+		ieee80211_stop_tx_ba_cb(&sdata->vif, ra_tid->ra, ra_tid->tid);
+	} else if (skb->pkt_type == IEEE80211_SDATA_QUEUE_RX_AGG_START) {
+		rx_agg = (void *)&skb->cb;
+		mutex_lock(&local->sta_mtx);
+		sta = sta_info_get_bss(sdata, rx_agg->addr);
+		if (sta)
+			__ieee80211_start_rx_ba_session(sta,
+							0, 0, 0, 1, rx_agg->tid,
+							IEEE80211_MAX_AMPDU_BUF,
+							false, true);
+		mutex_unlock(&local->sta_mtx);
+	} else if (skb->pkt_type == IEEE80211_SDATA_QUEUE_RX_AGG_STOP) {
+		rx_agg = (void *)&skb->cb;
+		mutex_lock(&local->sta_mtx);
+		sta = sta_info_get_bss(sdata, rx_agg->addr);
+		if (sta)
+			__ieee80211_stop_rx_ba_session(sta,
+						       rx_agg->tid,
+						       WLAN_BACK_RECIPIENT, 0,
+						       false);
+		mutex_unlock(&local->sta_mtx);
+	} else {
+		return -EINVAL;
+	}
+	/*will return 0 if pkt_type found and handled */
+	return 0;
+}
+
 static void ieee80211_iface_work(struct work_struct *work)
 {
 	struct ieee80211_sub_if_data *sdata =
@@ -1205,8 +1247,6 @@ static void ieee80211_iface_work(struct work_struct *work)
 	struct ieee80211_local *local = sdata->local;
 	struct sk_buff *skb;
 	struct sta_info *sta;
-	struct ieee80211_ra_tid *ra_tid;
-	struct ieee80211_rx_agg *rx_agg;
 
 	if (!ieee80211_sdata_running(sdata))
 		return;
@@ -1221,34 +1261,8 @@ static void ieee80211_iface_work(struct work_struct *work)
 	while ((skb = skb_dequeue(&sdata->skb_queue))) {
 		struct ieee80211_mgmt *mgmt = (void *)skb->data;
 
-		if (skb->pkt_type == IEEE80211_SDATA_QUEUE_AGG_START) {
-			ra_tid = (void *)&skb->cb;
-			ieee80211_start_tx_ba_cb(&sdata->vif, ra_tid->ra,
-						 ra_tid->tid);
-		} else if (skb->pkt_type == IEEE80211_SDATA_QUEUE_AGG_STOP) {
-			ra_tid = (void *)&skb->cb;
-			ieee80211_stop_tx_ba_cb(&sdata->vif, ra_tid->ra,
-						ra_tid->tid);
-		} else if (skb->pkt_type == IEEE80211_SDATA_QUEUE_RX_AGG_START) {
-			rx_agg = (void *)&skb->cb;
-			mutex_lock(&local->sta_mtx);
-			sta = sta_info_get_bss(sdata, rx_agg->addr);
-			if (sta)
-				__ieee80211_start_rx_ba_session(sta,
-						0, 0, 0, 1, rx_agg->tid,
-						IEEE80211_MAX_AMPDU_BUF,
-						false, true);
-			mutex_unlock(&local->sta_mtx);
-		} else if (skb->pkt_type == IEEE80211_SDATA_QUEUE_RX_AGG_STOP) {
-			rx_agg = (void *)&skb->cb;
-			mutex_lock(&local->sta_mtx);
-			sta = sta_info_get_bss(sdata, rx_agg->addr);
-			if (sta)
-				__ieee80211_stop_rx_ba_session(sta,
-							rx_agg->tid,
-							WLAN_BACK_RECIPIENT, 0,
-							false);
-			mutex_unlock(&local->sta_mtx);
+		if (!ieee80211_iface_work_handle_pkt_type(skb, sdata)) {
+			goto free_skb;
 		} else if (ieee80211_is_action(mgmt->frame_control) &&
 			   mgmt->u.action.category == WLAN_CATEGORY_BACK) {
 			int len = skb->len;
@@ -1333,6 +1347,7 @@ static void ieee80211_iface_work(struct work_struct *work)
 			break;
 		}
 
+free_skb:
 		kfree_skb(skb);
 	}
 
-- 
2.7.4

[PATCH 2/3 v6] mac80211: Refactor ieee80211_iface_work

[Alex Briskin]

Refactored ieee80211_iface_work_handle_pkt_type - if else if replaced by
switch case.

Added ieee80211_iface_work_handle_frame_control.
Moved the code that looks in skb->data (ieee80211_mgmt/ieee80211_hdr) at
frame_control member(field) from ieee80211_iface_work to the function.
Signed-off-by: Alex Briskin <br.shurik@gmail.com>
---
 net/mac80211/iface.c | 165 ++++++++++++++++++++++++++++-----------------------
 1 file changed, 90 insertions(+), 75 deletions(-)

diff --git a/net/mac80211/iface.c b/net/mac80211/iface.c
index 0fc973a..c185801 100644
--- a/net/mac80211/iface.c
+++ b/net/mac80211/iface.c
@@ -1207,13 +1207,16 @@ static int ieee80211_iface_work_handle_pkt_type(struct sk_buff *skb,
 	struct ieee80211_local *local = sdata->local;
 	struct sta_info *sta;
 
-	if (skb->pkt_type == IEEE80211_SDATA_QUEUE_AGG_START) {
+	switch (skb->pkt_type) {
+	case IEEE80211_SDATA_QUEUE_AGG_START:
 		ra_tid = (void *)&skb->cb;
 		ieee80211_start_tx_ba_cb(&sdata->vif, ra_tid->ra, ra_tid->tid);
-	} else if (skb->pkt_type == IEEE80211_SDATA_QUEUE_AGG_STOP) {
+		break;
+	case IEEE80211_SDATA_QUEUE_AGG_STOP:
 		ra_tid = (void *)&skb->cb;
 		ieee80211_stop_tx_ba_cb(&sdata->vif, ra_tid->ra, ra_tid->tid);
-	} else if (skb->pkt_type == IEEE80211_SDATA_QUEUE_RX_AGG_START) {
+		break;
+	case IEEE80211_SDATA_QUEUE_RX_AGG_START:
 		rx_agg = (void *)&skb->cb;
 		mutex_lock(&local->sta_mtx);
 		sta = sta_info_get_bss(sdata, rx_agg->addr);
@@ -1223,7 +1226,8 @@ static int ieee80211_iface_work_handle_pkt_type(struct sk_buff *skb,
 							IEEE80211_MAX_AMPDU_BUF,
 							false, true);
 		mutex_unlock(&local->sta_mtx);
-	} else if (skb->pkt_type == IEEE80211_SDATA_QUEUE_RX_AGG_STOP) {
+		break;
+	case IEEE80211_SDATA_QUEUE_RX_AGG_STOP:
 		rx_agg = (void *)&skb->cb;
 		mutex_lock(&local->sta_mtx);
 		sta = sta_info_get_bss(sdata, rx_agg->addr);
@@ -1233,20 +1237,98 @@ static int ieee80211_iface_work_handle_pkt_type(struct sk_buff *skb,
 						       WLAN_BACK_RECIPIENT, 0,
 						       false);
 		mutex_unlock(&local->sta_mtx);
-	} else {
+		break;
+	default:
 		return -EINVAL;
 	}
 	/*will return 0 if pkt_type found and handled */
 	return 0;
 }
 
+static int ieee80211_iface_work_handle_frame_control(struct sk_buff *skb,
+				struct ieee80211_sub_if_data  *sdata)
+{
+	struct ieee80211_local *local = sdata->local;
+	struct sta_info *sta;
+	struct ieee80211_mgmt *mgmt = (void *)skb->data;
+
+	if (ieee80211_is_action(mgmt->frame_control) &&
+	    mgmt->u.action.category == WLAN_CATEGORY_BACK) {
+		int len = skb->len;
+
+		mutex_lock(&local->sta_mtx);
+		sta = sta_info_get_bss(sdata, mgmt->sa);
+		if (sta) {
+			switch (mgmt->u.action.u.addba_req.action_code) {
+			case WLAN_ACTION_ADDBA_REQ:
+				ieee80211_process_addba_request(local, sta,
+								mgmt, len);
+				break;
+			case WLAN_ACTION_ADDBA_RESP:
+				ieee80211_process_addba_resp(local, sta,
+							     mgmt, len);
+				break;
+			case WLAN_ACTION_DELBA:
+				ieee80211_process_delba(sdata, sta, mgmt, len);
+				break;
+			default:
+				WARN_ON(1);
+				break;
+			}
+		}
+		mutex_unlock(&local->sta_mtx);
+	} else if (ieee80211_is_action(mgmt->frame_control) &&
+		   mgmt->u.action.category == WLAN_CATEGORY_VHT) {
+		switch (mgmt->u.action.u.vht_group_notif.action_code) {
+		case WLAN_VHT_ACTION_GROUPID_MGMT:
+			ieee80211_process_mu_groups(sdata, mgmt);
+			break;
+		default:
+			WARN_ON(1);
+			break;
+		}
+	} else if (ieee80211_is_data_qos(mgmt->frame_control)) {
+		struct ieee80211_hdr *hdr = (void *)mgmt;
+		/*
+		 * So the frame isn't mgmt, but frame_control
+		 * is at the right place anyway, of course, so
+		 * the if statement is correct.
+		 *
+		 * Warn if we have other data frame types here,
+		 * they must not get here.
+		 */
+		WARN_ON(hdr->frame_control &
+			cpu_to_le16(IEEE80211_STYPE_NULLFUNC));
+		WARN_ON(!(hdr->seq_ctrl & cpu_to_le16(IEEE80211_SCTL_FRAG)));
+		/*
+		 * This was a fragment of a frame, received while
+		 * a block-ack session was active. That cannot be
+		 * right, so terminate the session.
+		 */
+		mutex_lock(&local->sta_mtx);
+		sta = sta_info_get_bss(sdata, mgmt->sa);
+		if (sta) {
+			u16 tid = *ieee80211_get_qos_ctl(hdr) &
+			    IEEE80211_QOS_CTL_TID_MASK;
+
+			__ieee80211_stop_rx_ba_session(sta, tid,
+						       WLAN_BACK_RECIPIENT,
+						       WLAN_REASON_QSTA_REQUIRE_SETUP,
+						       true);
+		}
+		mutex_unlock(&local->sta_mtx);
+	} else {
+		return -EINVAL;
+	}
+	return 0;
+}
+
 static void ieee80211_iface_work(struct work_struct *work)
 {
 	struct ieee80211_sub_if_data *sdata =
 		container_of(work, struct ieee80211_sub_if_data, work);
 	struct ieee80211_local *local = sdata->local;
 	struct sk_buff *skb;
-	struct sta_info *sta;
 
 	if (!ieee80211_sdata_running(sdata))
 		return;
@@ -1259,77 +1341,10 @@ static void ieee80211_iface_work(struct work_struct *work)
 
 	/* first process frames */
 	while ((skb = skb_dequeue(&sdata->skb_queue))) {
-		struct ieee80211_mgmt *mgmt = (void *)skb->data;
-
 		if (!ieee80211_iface_work_handle_pkt_type(skb, sdata)) {
 			goto free_skb;
-		} else if (ieee80211_is_action(mgmt->frame_control) &&
-			   mgmt->u.action.category == WLAN_CATEGORY_BACK) {
-			int len = skb->len;
-
-			mutex_lock(&local->sta_mtx);
-			sta = sta_info_get_bss(sdata, mgmt->sa);
-			if (sta) {
-				switch (mgmt->u.action.u.addba_req.action_code) {
-				case WLAN_ACTION_ADDBA_REQ:
-					ieee80211_process_addba_request(
-							local, sta, mgmt, len);
-					break;
-				case WLAN_ACTION_ADDBA_RESP:
-					ieee80211_process_addba_resp(local, sta,
-								     mgmt, len);
-					break;
-				case WLAN_ACTION_DELBA:
-					ieee80211_process_delba(sdata, sta,
-								mgmt, len);
-					break;
-				default:
-					WARN_ON(1);
-					break;
-				}
-			}
-			mutex_unlock(&local->sta_mtx);
-		} else if (ieee80211_is_action(mgmt->frame_control) &&
-			   mgmt->u.action.category == WLAN_CATEGORY_VHT) {
-			switch (mgmt->u.action.u.vht_group_notif.action_code) {
-			case WLAN_VHT_ACTION_GROUPID_MGMT:
-				ieee80211_process_mu_groups(sdata, mgmt);
-				break;
-			default:
-				WARN_ON(1);
-				break;
-			}
-		} else if (ieee80211_is_data_qos(mgmt->frame_control)) {
-			struct ieee80211_hdr *hdr = (void *)mgmt;
-			/*
-			 * So the frame isn't mgmt, but frame_control
-			 * is at the right place anyway, of course, so
-			 * the if statement is correct.
-			 *
-			 * Warn if we have other data frame types here,
-			 * they must not get here.
-			 */
-			WARN_ON(hdr->frame_control &
-					cpu_to_le16(IEEE80211_STYPE_NULLFUNC));
-			WARN_ON(!(hdr->seq_ctrl &
-					cpu_to_le16(IEEE80211_SCTL_FRAG)));
-			/*
-			 * This was a fragment of a frame, received while
-			 * a block-ack session was active. That cannot be
-			 * right, so terminate the session.
-			 */
-			mutex_lock(&local->sta_mtx);
-			sta = sta_info_get_bss(sdata, mgmt->sa);
-			if (sta) {
-				u16 tid = *ieee80211_get_qos_ctl(hdr) &
-						IEEE80211_QOS_CTL_TID_MASK;
-
-				__ieee80211_stop_rx_ba_session(
-					sta, tid, WLAN_BACK_RECIPIENT,
-					WLAN_REASON_QSTA_REQUIRE_SETUP,
-					true);
-			}
-			mutex_unlock(&local->sta_mtx);
+		} else if (!ieee80211_iface_work_handle_frame_control(skb, sdata)) {
+			goto free_skb;
 		} else switch (sdata->vif.type) {
 		case NL80211_IFTYPE_STATION:
 			ieee80211_sta_rx_queued_mgmt(sdata, skb);
-- 
2.7.4

[PATCH 3/3 v6] mac80211: Refactor ieee80211_iface_work

[Alex Briskin]

Added ieee80211_iface_work_handle_vif_type function. Moved the code that
handles sdata->vif.type from ieee80211_iface_work to the function.

Signed-off-by: Alex Briskin <br.shurik@gmail.com>
---
 net/mac80211/iface.c | 44 ++++++++++++++++++++++++--------------------
 1 file changed, 24 insertions(+), 20 deletions(-)

diff --git a/net/mac80211/iface.c b/net/mac80211/iface.c
index c185801..f8f7363 100644
--- a/net/mac80211/iface.c
+++ b/net/mac80211/iface.c
@@ -1323,6 +1323,27 @@ static int ieee80211_iface_work_handle_frame_control(struct sk_buff *skb,
 	return 0;
 }
 
+static void ieee80211_iface_work_handle_vif_type(struct sk_buff *skb,
+					 struct ieee80211_sub_if_data *sdata)
+{
+	switch (sdata->vif.type) {
+	case NL80211_IFTYPE_STATION:
+		ieee80211_sta_rx_queued_mgmt(sdata, skb);
+		break;
+	case NL80211_IFTYPE_ADHOC:
+		ieee80211_ibss_rx_queued_mgmt(sdata, skb);
+		break;
+	case NL80211_IFTYPE_MESH_POINT:
+		if (!ieee80211_vif_is_mesh(&sdata->vif))
+			break;
+		ieee80211_mesh_rx_queued_mgmt(sdata, skb);
+		break;
+	default:
+		WARN(1, "frame for unexpected interface type");
+		break;
+	}
+}
+
 static void ieee80211_iface_work(struct work_struct *work)
 {
 	struct ieee80211_sub_if_data *sdata =
@@ -1341,28 +1362,11 @@ static void ieee80211_iface_work(struct work_struct *work)
 
 	/* first process frames */
 	while ((skb = skb_dequeue(&sdata->skb_queue))) {
-		if (!ieee80211_iface_work_handle_pkt_type(skb, sdata)) {
-			goto free_skb;
-		} else if (!ieee80211_iface_work_handle_frame_control(skb, sdata)) {
-			goto free_skb;
-		} else switch (sdata->vif.type) {
-		case NL80211_IFTYPE_STATION:
-			ieee80211_sta_rx_queued_mgmt(sdata, skb);
-			break;
-		case NL80211_IFTYPE_ADHOC:
-			ieee80211_ibss_rx_queued_mgmt(sdata, skb);
-			break;
-		case NL80211_IFTYPE_MESH_POINT:
-			if (!ieee80211_vif_is_mesh(&sdata->vif))
-				break;
-			ieee80211_mesh_rx_queued_mgmt(sdata, skb);
-			break;
-		default:
-			WARN(1, "frame for unexpected interface type");
-			break;
+		if (ieee80211_iface_work_handle_pkt_type(skb, sdata) &&
+		    ieee80211_iface_work_handle_frame_control(skb, sdata)) {
+			ieee80211_iface_work_handle_vif_type(skb, sdata);
 		}
 
-free_skb:
 		kfree_skb(skb);
 	}
 
-- 
2.7.4

Re: [PATCH 1/3 v6] mac80211: Refactor ieee80211_iface_work

[Johannes Berg]

Please don't submit three patches with identical subject.

> Added ieee80211_iface_work_handle_pkt_type function. Moved part of

Should also use present tense here - "Add ... function, move [...]".

This also applies to the other patches.

The if/else if/... -> switch change should also be part of this patch,
or separate, but not squashed together with a different unrelated
change. Might be worth moving "return 0" into each of the cases and
return an error in the fall-through, but it doesn't really matter.


johannes

Re: [PATCH 2/3 v6] mac80211: Refactor ieee80211_iface_work

[Johannes Berg]

> Added ieee80211_iface_work_handle_frame_control.

That function name doesn't really make sense - handle_mgmt() perhaps?

johannes

Re: [PATCH 3/3 v6] mac80211: Refactor ieee80211_iface_work

[Johannes Berg]

> +		if (ieee80211_iface_work_handle_pkt_type(skb, sdata) &&
> +		    ieee80211_iface_work_handle_frame_control(skb, sdata)) {
> +			ieee80211_iface_work_handle_vif_type(skb, sdata);
>  		}
> 
This ends up being pretty strange IMHO, using boolean short-circuit
evaluation to avoid doing the next thing...

Not sure how to better do this though.

(You also don't need braces here for a single-statement if branch
contents)

johannes