[PATCH 0/4 v1] Refactoring ieee80211_iface_work

[PATCH 0/4 v1] Refactoring ieee80211_iface_work

[Alex Briskin]

Hi All,
This is my first patch(s).

I've decided to refactor ieee80211_iface_work function and break it down
to smaller better defined function.

I think these changes make the code much more readable and do not impose
no overhead.

I've tested these patches with sparse and checkpatch.pl 

Function names might not be descriptive enough.
Hope you find this useful.

Alex Briskin (4):
  0) [28e464b19aaaba90c8946fb979b58709d55dffcf] 
	Added new function ieee80211_is_skb_handled_by_pkt_type and moved
	some code from ieee80211_iface_work to reduce complexity and 
	improve readability

  1) [486e3d5abb4dc6361cdd923254a2b68d43dcdaba]
	Refactored code in ieee80211_is_skb_handled_by_pkt_type.
	"if () {} else if ()" replaced by switch case. 

  2) [9ef2eab8e831420bc6748a4466ffa6b7a99bf447]
	Added new function ieee80211_is_handled_by_frame_control and moved
	some code from ieee80211_iface_work to it.

  3) [1de8cdf9a0c05c6a21d9e43e5b55862f6efcf450] 
	Added new function ieee80211_handle_by_vif_type with code from
	ieee80211_iface_work.

	At this point ieee80211_iface_work seems to me much more readable
	and better understood. 

 net/mac80211/iface.c | 264 +++++++++++++++++++++++++++++----------------------
 1 file changed, 150 insertions(+), 114 deletions(-)

-- 
2.5.0

[PATCH 1/4 v1] Refactoring ieee80211_iface_work to reduce complexity and improve readability.

[Alex Briskin]

No performance impact/overhead expected. The (gcc) compiler optimizes the
added function out.

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..a68cbac 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 bool ieee80211_is_skb_handled_by_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 false;
+	}
+	/*will return true if pkt_type found and handled */
+	return true;
+}
+
 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_is_skb_handled_by_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.5.0

[PATCH 2/4 v1] even better readability

[Alex Briskin]

Further improve readability by replacing if else with switch case

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

diff --git a/net/mac80211/iface.c b/net/mac80211/iface.c
index a68cbac..1793ad2 100644
--- a/net/mac80211/iface.c
+++ b/net/mac80211/iface.c
@@ -1207,13 +1207,16 @@ static bool ieee80211_is_skb_handled_by_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 bool ieee80211_is_skb_handled_by_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,7 +1237,8 @@ static bool ieee80211_is_skb_handled_by_pkt_type(struct sk_buff *skb,
 						       WLAN_BACK_RECIPIENT, 0,
 						       false);
 		mutex_unlock(&local->sta_mtx);
-	} else {
+		break;
+	default:
 		return false;
 	}
 	/*will return true if pkt_type found and handled */
-- 
2.5.0

[PATCH 3/4 v1] Continue breaking down to smaller functions

[Alex Briskin]

This commit contains unhandled checkpatch warnings in code moved from
ieee80211_iface_work

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

diff --git a/net/mac80211/iface.c b/net/mac80211/iface.c
index 1793ad2..41c3bd0 100644
--- a/net/mac80211/iface.c
+++ b/net/mac80211/iface.c
@@ -1245,6 +1245,85 @@ static bool ieee80211_is_skb_handled_by_pkt_type(struct sk_buff *skb,
 	return true;
 }
 
+static bool ieee80211_is_handled_by_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 false;
+	}
+	return true;
+}
+
 static void ieee80211_iface_work(struct work_struct *work)
 {
 	struct ieee80211_sub_if_data *sdata =
@@ -1264,77 +1343,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_is_skb_handled_by_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_is_handled_by_frame_control(skb, sdata)) {
+			goto free_skb;
 		} else switch (sdata->vif.type) {
 		case NL80211_IFTYPE_STATION:
 			ieee80211_sta_rx_queued_mgmt(sdata, skb);
-- 
2.5.0

Re: [PATCH 3/4 v1] Continue breaking down to smaller functions

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 1963 bytes --]

Hi,

[auto build test WARNING on mac80211/master]
[also build test WARNING on v4.7-rc7]
[cannot apply to mac80211-next/master next-20160712]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Alex-Briskin/Refactoring-ieee80211_iface_work/20160714-042301
base:   https://git.kernel.org/pub/scm/linux/kernel/git/jberg/mac80211.git master
config: sparc64-allmodconfig (attached as .config)
compiler: sparc64-linux-gnu-gcc (Debian 5.3.1-8) 5.3.1 20160205
reproduce:
        wget https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        make.cross ARCH=sparc64 

All warnings (new ones prefixed by >>):

   net/mac80211/iface.c: In function 'ieee80211_iface_work':
>> net/mac80211/iface.c:1333:19: warning: unused variable 'sta' [-Wunused-variable]
     struct sta_info *sta;
                      ^

vim +/sta +1333 net/mac80211/iface.c

  1317							       WLAN_REASON_QSTA_REQUIRE_SETUP,
  1318							       true);
  1319			}
  1320			mutex_unlock(&local->sta_mtx);
  1321		} else {
  1322			return false;
  1323		}
  1324		return true;
  1325	}
  1326	
  1327	static void ieee80211_iface_work(struct work_struct *work)
  1328	{
  1329		struct ieee80211_sub_if_data *sdata =
  1330			container_of(work, struct ieee80211_sub_if_data, work);
  1331		struct ieee80211_local *local = sdata->local;
  1332		struct sk_buff *skb;
> 1333		struct sta_info *sta;
  1334	
  1335		if (!ieee80211_sdata_running(sdata))
  1336			return;
  1337	
  1338		if (test_bit(SCAN_SW_SCANNING, &local->scanning))
  1339			return;
  1340	
  1341		if (!ieee80211_can_run_worker(local))

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/octet-stream, Size: 46212 bytes --]

[PATCH 4/4 v1] Simple and well understood logic

[Alex Briskin]

Now the logic is obvious - if skb not handled by pkt type and not handled
by frame control it is handled by vif type and then released

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 41c3bd0..76e8c6a 100644
--- a/net/mac80211/iface.c
+++ b/net/mac80211/iface.c
@@ -1324,6 +1324,27 @@ static bool ieee80211_is_handled_by_frame_control(struct sk_buff *skb,
 	return true;
 }
 
+static void ieee80211_handle_by_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 =
@@ -1343,28 +1364,11 @@ static void ieee80211_iface_work(struct work_struct *work)
 
 	/* first process frames */
 	while ((skb = skb_dequeue(&sdata->skb_queue))) {
-		if (ieee80211_is_skb_handled_by_pkt_type(skb, sdata)) {
-			goto free_skb;
-		} else if (ieee80211_is_handled_by_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_is_skb_handled_by_pkt_type(skb, sdata) &&
+		    !ieee80211_is_handled_by_frame_control(skb, sdata)) {
+			ieee80211_handle_by_vif_type(skb, sdata);
 		}
 
-free_skb:
 		kfree_skb(skb);
 	}
 
-- 
2.5.0

Re: [PATCH 0/4 v1] Refactoring ieee80211_iface_work

[Arend Van Spriel]

On 13-7-2016 22:19, Alex Briskin wrote:
> Hi All,
> This is my first patch(s).

Hi Alex,

As these patches are touching mac80211 it would be good to use
'mac80211: ' prefix.

> I've decided to refactor ieee80211_iface_work function and break it down
> to smaller better defined function.
> 
> I think these changes make the code much more readable and do not impose
> no overhead.
> 
> I've tested these patches with sparse and checkpatch.pl 
> 
> Function names might not be descriptive enough.
> Hope you find this useful.
> 
> Alex Briskin (4):
>   0) [28e464b19aaaba90c8946fb979b58709d55dffcf] 
> 	Added new function ieee80211_is_skb_handled_by_pkt_type and moved
> 	some code from ieee80211_iface_work to reduce complexity and 
> 	improve readability
> 
>   1) [486e3d5abb4dc6361cdd923254a2b68d43dcdaba]
> 	Refactored code in ieee80211_is_skb_handled_by_pkt_type.
> 	"if () {} else if ()" replaced by switch case. 

I would collapse these two patches in one patch.

>   2) [9ef2eab8e831420bc6748a4466ffa6b7a99bf447]
> 	Added new function ieee80211_is_handled_by_frame_control and moved
> 	some code from ieee80211_iface_work to it.
> 
>   3) [1de8cdf9a0c05c6a21d9e43e5b55862f6efcf450] 
> 	Added new function ieee80211_handle_by_vif_type with code from
> 	ieee80211_iface_work.
> 
> 	At this point ieee80211_iface_work seems to me much more readable
> 	and better understood. 

You are allowed to have an opinion :-) The function naming of the three
functions could be more consistent as you seem to drop a bit in every
patch, ie. is_skb_handled_by -> is_handled_by -> handle_by.

Regards,
Arend

>  net/mac80211/iface.c | 264 +++++++++++++++++++++++++++++----------------------
>  1 file changed, 150 insertions(+), 114 deletions(-)
> 

Re: [PATCH 0/4 v1] Refactoring ieee80211_iface_work

[Alex Briskin]

Hi Arend,

Thank you for suggestions!
I will definitely collapse the two commits you suggested.

Could you please elaborate on your naming suggestion.
In your opinion, should it be something like 
mac80211_is handled_by_pkt_type? Or something entirely different?

Also, I have two more concerns about the suggested changes:
1. Necessity - whether these changes really improve readability of the code.
2. Precision - whether the code was split in logical manner.


On Thursday, July 14, 2016 09:56:19 AM Arend Van Spriel wrote:
> On 13-7-2016 22:19, Alex Briskin wrote:
> > Hi All,
> > This is my first patch(s).
> 
> Hi Alex,
> 
> As these patches are touching mac80211 it would be good to use
> 'mac80211: ' prefix.
> 
> > I've decided to refactor ieee80211_iface_work function and break it down
> > to smaller better defined function.
> > 
> > I think these changes make the code much more readable and do not impose
> > no overhead.
> > 
> > I've tested these patches with sparse and checkpatch.pl
> > 
> > Function names might not be descriptive enough.
> > Hope you find this useful.
> > 
> > Alex Briskin (4):
> >   0) [28e464b19aaaba90c8946fb979b58709d55dffcf]
> > 	
> > 	Added new function ieee80211_is_skb_handled_by_pkt_type and moved
> > 	some code from ieee80211_iface_work to reduce complexity and
> > 	improve readability
> > 	
> >   1) [486e3d5abb4dc6361cdd923254a2b68d43dcdaba]
> > 	
> > 	Refactored code in ieee80211_is_skb_handled_by_pkt_type.
> > 	"if () {} else if ()" replaced by switch case.
> 
> I would collapse these two patches in one patch.
> 
> >   2) [9ef2eab8e831420bc6748a4466ffa6b7a99bf447]
> > 	
> > 	Added new function ieee80211_is_handled_by_frame_control and moved
> > 	some code from ieee80211_iface_work to it.
> > 	
> >   3) [1de8cdf9a0c05c6a21d9e43e5b55862f6efcf450]
> > 	
> > 	Added new function ieee80211_handle_by_vif_type with code from
> > 	ieee80211_iface_work.
> > 	
> > 	At this point ieee80211_iface_work seems to me much more readable
> > 	and better understood.
> 
> You are allowed to have an opinion :-) The function naming of the three
> functions could be more consistent as you seem to drop a bit in every
> patch, ie. is_skb_handled_by -> is_handled_by -> handle_by.
> 
> Regards,
> Arend
> 
> >  net/mac80211/iface.c | 264
> >  +++++++++++++++++++++++++++++---------------------- 1 file changed, 150
> >  insertions(+), 114 deletions(-)