fix and improvements

PATCH In review Zhao, Gang 2 participants 6 messages Apr 19, 2014 – Apr 22, 2014

Subsystems (2)

mac80211 802.11 (including cfg80211/NL80211)

lore

Share & tools

lore

Zhao, Gang

author

2014-04-19 16:33 UTC #0 lore raw

cover fix and improvements

I think the first patch should also be applied to 3.15.

Zhao, Gang (3):
cfg80211: fix incorrect checking of return value cfg80211/mac80211: rename cfg80211_chandef_dfs_required()
cfg80211: change variable name

 include/net/cfg80211.h |   6 +--
 net/mac80211/ibss.c    |  40 ++++++++---------
 net/mac80211/mesh.c    |  16 +++----
 net/wireless/chan.c    |  32 +++++++------
 net/wireless/mesh.c    |  22 ++++-----
 net/wireless/nl80211.c | 120 ++++++++++++++++++++++++-------------------------
 net/wireless/trace.h   |   2 +-
 7 files changed, 118 insertions(+), 120 deletions(-)

Signature · 1 line

1.9.0

Zhao, Gang

author

2014-04-19 16:33 UTC #1 lore raw

PATCH 1/3 cfg80211: fix incorrect checking of return value

Since cfg80211_chandef_dfs_required() can return error number(e.g., -EINVAL), it's not sufficient to just check if it didn't return zero.

Fixes 9e0e29615a20 ("cfg80211: consider existing DFS interfaces")

Signed-off-by Zhao, Gang <[email protected]>

---
 net/wireless/chan.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

net/wireless/chan.c +2 -2

index 9c9501a..2cfff9e 100644
@@ -744,7 +744,7 @@ cfg80211_get_chan_state(struct wireless_dev *wdev,
 			*chanmode = CHAN_MODE_SHARED;
 
 			if (cfg80211_chandef_dfs_required(wdev->wiphy,
-							  &wdev->chandef))
+							  &wdev->chandef) > 0)
 				*radar_detect |= BIT(wdev->chandef.width);
 		}
 		return;

@@ -754,7 +754,7 @@ cfg80211_get_chan_state(struct wireless_dev *wdev,
 			*chanmode = CHAN_MODE_SHARED;
 
 			if (cfg80211_chandef_dfs_required(wdev->wiphy,
-							  &wdev->chandef))
+							  &wdev->chandef) > 0)
 				*radar_detect |= BIT(wdev->chandef.width);
 		}
 		return;

Signature · 1 line

1.9.0

Zhao, Gang

author

2014-04-19 16:34 UTC #2 lore raw

PATCH 2/3 cfg80211/mac80211: rename cfg80211_chandef_dfs_required()

In reply to Zhao, Gang

Change the name to cfg80211_chandef_dfs_check to emphasize that the function isn't a yes/no function, its return value must be fully checked.

Also rename the helper function's name to
cfg80211_do_chandef_dfs_check for consistency.

Variable width needn't to be checked for sanity, since above cfg80211_chandef_valid() already checked the sanity of chandef->width.

Signed-off-by Zhao, Gang <[email protected]>

---
 include/net/cfg80211.h |  6 +++---
 net/mac80211/ibss.c    | 12 ++++++------
 net/mac80211/mesh.c    |  4 ++--
 net/wireless/chan.c    | 32 +++++++++++++++-----------------
 net/wireless/mesh.c    |  2 +-
 net/wireless/nl80211.c |  8 ++++----
 net/wireless/trace.h   |  2 +-
 7 files changed, 32 insertions(+), 34 deletions(-)

include/net/cfg80211.h +3 -3

index 6eb2420..54d4f67 100644
@@ -438,13 +438,13 @@ bool cfg80211_chandef_usable(struct wiphy *wiphy,
 			     u32 prohibited_flags);
 
 /**
- * cfg80211_chandef_dfs_required - checks if radar detection is required
+ * cfg80211_chandef_dfs_check - checks if radar detection is required
  * @wiphy: the wiphy to validate against
  * @chandef: the channel definition to check
  * Return: 1 if radar detection is required, 0 if it is not, < 0 on error
  */
-int cfg80211_chandef_dfs_required(struct wiphy *wiphy,
-				  const struct cfg80211_chan_def *chandef);
+int cfg80211_chandef_dfs_check(struct wiphy *wiphy,
+			       const struct cfg80211_chan_def *chandef);
 
 /**
  * ieee80211_chandef_rate_flags - returns rate flags for a channel

net/mac80211/ibss.c +6 -6

index 06d2878..18dd2e2 100644
@@ -281,8 +281,8 @@ static void __ieee80211_sta_join_ibss(struct ieee80211_sub_if_data *sdata,
 		}
 	}
 
-	err = cfg80211_chandef_dfs_required(sdata->local->hw.wiphy,
-					    &chandef);
+	err = cfg80211_chandef_dfs_check(sdata->local->hw.wiphy,
+					 &chandef);
 	if (err < 0) {
 		sdata_info(sdata,
 			   "Failed to join IBSS, invalid chandef\n");

@@ -774,8 +774,8 @@ static void ieee80211_ibss_csa_mark_radar(struct ieee80211_sub_if_data *sdata)
 	/* if the current channel is a DFS channel, mark the channel as
 	 * unavailable.
 	 */
-	err = cfg80211_chandef_dfs_required(sdata->local->hw.wiphy,
-					    &ifibss->chandef);
+	err = cfg80211_chandef_dfs_check(sdata->local->hw.wiphy,
+					 &ifibss->chandef);
 	if (err > 0)
 		cfg80211_radar_event(sdata->local->hw.wiphy, &ifibss->chandef,
 				     GFP_ATOMIC);

@@ -872,8 +872,8 @@ ieee80211_ibss_process_chanswitch(struct ieee80211_sub_if_data *sdata,
 		goto disconnect;
 	}
 
-	err = cfg80211_chandef_dfs_required(sdata->local->hw.wiphy,
-					    &params.chandef);
+	err = cfg80211_chandef_dfs_check(sdata->local->hw.wiphy,
+					 &params.chandef);
 	if (err < 0)
 		goto disconnect;
 	if (err) {

net/mac80211/mesh.c +2 -2

index f70e9cd..33cf125 100644
@@ -902,8 +902,8 @@ ieee80211_mesh_process_chnswitch(struct ieee80211_sub_if_data *sdata,
 		return false;
 	}
 
-	err = cfg80211_chandef_dfs_required(sdata->local->hw.wiphy,
-					    &params.chandef);
+	err = cfg80211_chandef_dfs_check(sdata->local->hw.wiphy,
+					 &params.chandef);
 	if (err < 0)
 		return false;
 	if (err) {

net/wireless/chan.c +15 ~~-17~~

index 2cfff9e..aaf24fc 100644
@@ -303,9 +303,9 @@ static u32 cfg80211_get_end_freq(u32 center_freq,
 	return end_freq;
 }
 
-static int cfg80211_get_chans_dfs_required(struct wiphy *wiphy,
-					    u32 center_freq,
-					    u32 bandwidth)
+static int cfg80211_do_chandef_dfs_check(struct wiphy *wiphy,
+					 u32 center_freq,
+					 u32 bandwidth)
 {
 	struct ieee80211_channel *c;
 	u32 freq, start_freq, end_freq;

@@ -325,8 +325,8 @@ static int cfg80211_get_chans_dfs_required(struct wiphy *wiphy,
 }
 
 
-int cfg80211_chandef_dfs_required(struct wiphy *wiphy,
-				  const struct cfg80211_chan_def *chandef)
+int cfg80211_chandef_dfs_check(struct wiphy *wiphy,
+			       const struct cfg80211_chan_def *chandef)
 {
 	int width;
 	int r;

@@ -335,21 +335,19 @@ int cfg80211_chandef_dfs_required(struct wiphy *wiphy,
 		return -EINVAL;
 
 	width = cfg80211_chandef_get_width(chandef);
-	if (width < 0)
-		return -EINVAL;
 
-	r = cfg80211_get_chans_dfs_required(wiphy, chandef->center_freq1,
-					    width);
+	r = cfg80211_do_chandef_dfs_check(wiphy, chandef->center_freq1,
+					  width);
 	if (r)
 		return r;
 
 	if (!chandef->center_freq2)
 		return 0;
 
-	return cfg80211_get_chans_dfs_required(wiphy, chandef->center_freq2,
-					       width);
+	return cfg80211_do_chandef_dfs_check(wiphy, chandef->center_freq2,
+					     width);
 }
-EXPORT_SYMBOL(cfg80211_chandef_dfs_required);
+EXPORT_SYMBOL(cfg80211_chandef_dfs_check);
 
 static int cfg80211_get_chans_dfs_usable(struct wiphy *wiphy,
 					 u32 center_freq,

@@ -671,7 +669,7 @@ bool cfg80211_reg_can_beacon(struct wiphy *wiphy,
 
 	trace_cfg80211_reg_can_beacon(wiphy, chandef);
 
-	if (cfg80211_chandef_dfs_required(wiphy, chandef) > 0 &&
+	if (cfg80211_chandef_dfs_check(wiphy, chandef) > 0 &&
 	    cfg80211_chandef_dfs_available(wiphy, chandef)) {
 		/* We can skip IEEE80211_CHAN_NO_IR if chandef dfs available */
 		prohibited_flags = IEEE80211_CHAN_DISABLED;

@@ -743,8 +741,8 @@ cfg80211_get_chan_state(struct wireless_dev *wdev,
 			*chan = wdev->chandef.chan;
 			*chanmode = CHAN_MODE_SHARED;
 
-			if (cfg80211_chandef_dfs_required(wdev->wiphy,
-							  &wdev->chandef) > 0)
+			if (cfg80211_chandef_dfs_check(wdev->wiphy,
+						       &wdev->chandef) > 0)
 				*radar_detect |= BIT(wdev->chandef.width);
 		}
 		return;

@@ -753,8 +751,8 @@ cfg80211_get_chan_state(struct wireless_dev *wdev,
 			*chan = wdev->chandef.chan;
 			*chanmode = CHAN_MODE_SHARED;
 
-			if (cfg80211_chandef_dfs_required(wdev->wiphy,
-							  &wdev->chandef) > 0)
+			if (cfg80211_chandef_dfs_check(wdev->wiphy,
+						       &wdev->chandef) > 0)
 				*radar_detect |= BIT(wdev->chandef.width);
 		}
 		return;

net/wireless/mesh.c +1 -1

index 5af5cc6..d196a7c 100644
@@ -178,7 +178,7 @@ int __cfg80211_join_mesh(struct cfg80211_registered_device *rdev,
 	if (!cfg80211_reg_can_beacon(&rdev->wiphy, &setup->chandef))
 		return -EINVAL;
 
-	err = cfg80211_chandef_dfs_required(wdev->wiphy, &setup->chandef);
+	err = cfg80211_chandef_dfs_check(wdev->wiphy, &setup->chandef);
 	if (err < 0)
 		return err;
 	if (err)

net/wireless/nl80211.c +4 -4

index b6b2f6f..305ede8 100644
@@ -3261,7 +3261,7 @@ static int nl80211_start_ap(struct sk_buff *skb, struct genl_info *info)
 	if (!cfg80211_reg_can_beacon(&rdev->wiphy, &params.chandef))
 		return -EINVAL;
 
-	err = cfg80211_chandef_dfs_required(wdev->wiphy, &params.chandef);
+	err = cfg80211_chandef_dfs_check(wdev->wiphy, &params.chandef);
 	if (err < 0)
 		return err;
 	if (err) {

@@ -5796,7 +5796,7 @@ static int nl80211_start_radar_detection(struct sk_buff *skb,
 	if (wdev->cac_started)
 		return -EBUSY;
 
-	err = cfg80211_chandef_dfs_required(wdev->wiphy, &chandef);
+	err = cfg80211_chandef_dfs_check(wdev->wiphy, &chandef);
 	if (err < 0)
 		return err;

@@ -5936,8 +5936,8 @@ skip_beacons:
 	case NL80211_IFTYPE_P2P_GO:
 	case NL80211_IFTYPE_ADHOC:
 	case NL80211_IFTYPE_MESH_POINT:
-		err = cfg80211_chandef_dfs_required(wdev->wiphy,
-						    &params.chandef);
+		err = cfg80211_chandef_dfs_check(wdev->wiphy,
+						 &params.chandef);
 		if (err < 0)
 			return err;
 		if (err) {

net/wireless/trace.h +1 -1

index aabccf1..80770f4 100644
@@ -2207,7 +2207,7 @@ TRACE_EVENT(cfg80211_reg_can_beacon,
 		  WIPHY_PR_ARG, CHAN_DEF_PR_ARG)
 );
 
-TRACE_EVENT(cfg80211_chandef_dfs_required,
+TRACE_EVENT(cfg80211_chandef_dfs_check,
 	TP_PROTO(struct wiphy *wiphy, struct cfg80211_chan_def *chandef),
 	TP_ARGS(wiphy, chandef),
 	TP_STRUCT__entry(

Signature · 1 line

1.9.0

Zhao, Gang

author

2014-04-19 16:34 UTC #3 lore raw

PATCH 3/3 cfg80211: change variable name

In reply to Zhao, Gang

Change "err" to "ret", since return value isn't zero doesn't always mean error occured, sometimes it means success.

if (err)

        /* do things if function successfully returned */

looks strange to me, so change it.

Signed-off-by Zhao, Gang <[email protected]>

---
 net/mac80211/ibss.c    |  34 +++++++-------
 net/mac80211/mesh.c    |  14 +++---
 net/wireless/mesh.c    |  22 ++++-----
 net/wireless/nl80211.c | 118 ++++++++++++++++++++++++-------------------------
 4 files changed, 94 insertions(+), 94 deletions(-)

net/mac80211/ibss.c +17 ~~-17~~

index 18dd2e2..6c08424 100644
@@ -229,7 +229,7 @@ static void __ieee80211_sta_join_ibss(struct ieee80211_sub_if_data *sdata,
 	enum nl80211_bss_scan_width scan_width;
 	bool have_higher_than_11mbit;
 	bool radar_required = false;
-	int err;
+	int ret;
 
 	sdata_assert_lock(sdata);

@@ -281,14 +281,14 @@ static void __ieee80211_sta_join_ibss(struct ieee80211_sub_if_data *sdata,
 		}
 	}
 
-	err = cfg80211_chandef_dfs_check(sdata->local->hw.wiphy,
+	ret = cfg80211_chandef_dfs_check(sdata->local->hw.wiphy,
 					 &chandef);
-	if (err < 0) {
+	if (ret < 0) {
 		sdata_info(sdata,
 			   "Failed to join IBSS, invalid chandef\n");
 		return;
 	}
-	if (err > 0) {
+	if (ret > 0) {
 		if (!ifibss->userspace_handles_dfs) {
 			sdata_info(sdata,
 				   "Failed to join IBSS, DFS channel without control program\n");

@@ -359,8 +359,8 @@ static void __ieee80211_sta_join_ibss(struct ieee80211_sub_if_data *sdata,
 	sdata->vif.bss_conf.ibss_joined = true;
 	sdata->vif.bss_conf.ibss_creator = creator;
 
-	err = drv_join_ibss(local, sdata);
-	if (err) {
+	ret = drv_join_ibss(local, sdata);
+	if (ret) {
 		sdata->vif.bss_conf.ibss_joined = false;
 		sdata->vif.bss_conf.ibss_creator = false;
 		sdata->vif.bss_conf.enable_beacon = false;

@@ -371,7 +371,7 @@ static void __ieee80211_sta_join_ibss(struct ieee80211_sub_if_data *sdata,
 		ieee80211_vif_release_channel(sdata);
 		mutex_unlock(&local->mtx);
 		sdata_info(sdata, "Failed to join IBSS, driver failure: %d\n",
-			   err);
+			   ret);
 		return;
 	}

@@ -769,14 +769,14 @@ static void ieee80211_csa_connection_drop_work(struct work_struct *work)
 static void ieee80211_ibss_csa_mark_radar(struct ieee80211_sub_if_data *sdata)
 {
 	struct ieee80211_if_ibss *ifibss = &sdata->u.ibss;
-	int err;
+	int ret;
 
 	/* if the current channel is a DFS channel, mark the channel as
 	 * unavailable.
 	 */
-	err = cfg80211_chandef_dfs_check(sdata->local->hw.wiphy,
+	ret = cfg80211_chandef_dfs_check(sdata->local->hw.wiphy,
 					 &ifibss->chandef);
-	if (err > 0)
+	if (ret > 0)
 		cfg80211_radar_event(sdata->local->hw.wiphy, &ifibss->chandef,
 				     GFP_ATOMIC);
 }

@@ -790,7 +790,7 @@ ieee80211_ibss_process_chanswitch(struct ieee80211_sub_if_data *sdata,
 	struct ieee80211_csa_ie csa_ie;
 	struct ieee80211_if_ibss *ifibss = &sdata->u.ibss;
 	enum nl80211_channel_type ch_type;
-	int err;
+	int ret;
 	u32 sta_flags;
 
 	sdata_assert_lock(sdata);

@@ -811,15 +811,15 @@ ieee80211_ibss_process_chanswitch(struct ieee80211_sub_if_data *sdata,
 
 	memset(&params, 0, sizeof(params));
 	memset(&csa_ie, 0, sizeof(csa_ie));
-	err = ieee80211_parse_ch_switch_ie(sdata, elems, beacon,
+	ret = ieee80211_parse_ch_switch_ie(sdata, elems, beacon,
 					   ifibss->chandef.chan->band,
 					   sta_flags, ifibss->bssid, &csa_ie);
 	/* can't switch to destination channel, fail */
-	if (err < 0)
+	if (ret < 0)
 		goto disconnect;
 
 	/* did not contain a CSA */
-	if (err)
+	if (ret)
 		return false;
 
 	/* channel switch is not supported, disconnect */

@@ -872,11 +872,11 @@ ieee80211_ibss_process_chanswitch(struct ieee80211_sub_if_data *sdata,
 		goto disconnect;
 	}
 
-	err = cfg80211_chandef_dfs_check(sdata->local->hw.wiphy,
+	ret = cfg80211_chandef_dfs_check(sdata->local->hw.wiphy,
 					 &params.chandef);
-	if (err < 0)
+	if (ret < 0)
 		goto disconnect;
-	if (err) {
+	if (ret) {
 		/* IBSS-DFS only allowed with a control program */
 		if (!ifibss->userspace_handles_dfs)
 			goto disconnect;

net/mac80211/mesh.c +7 -7

index 33cf125..336b963 100644
@@ -861,7 +861,7 @@ ieee80211_mesh_process_chnswitch(struct ieee80211_sub_if_data *sdata,
 	struct ieee80211_csa_ie csa_ie;
 	struct ieee80211_if_mesh *ifmsh = &sdata->u.mesh;
 	enum ieee80211_band band = ieee80211_get_sdata_band(sdata);
-	int err;
+	int ret;
 	u32 sta_flags;
 
 	sdata_assert_lock(sdata);

@@ -879,12 +879,12 @@ ieee80211_mesh_process_chnswitch(struct ieee80211_sub_if_data *sdata,
 
 	memset(&params, 0, sizeof(params));
 	memset(&csa_ie, 0, sizeof(csa_ie));
-	err = ieee80211_parse_ch_switch_ie(sdata, elems, beacon, band,
+	ret = ieee80211_parse_ch_switch_ie(sdata, elems, beacon, band,
 					   sta_flags, sdata->vif.addr,
 					   &csa_ie);
-	if (err < 0)
+	if (ret < 0)
 		return false;
-	if (err)
+	if (ret)
 		return false;
 
 	params.chandef = csa_ie.chandef;

@@ -902,11 +902,11 @@ ieee80211_mesh_process_chnswitch(struct ieee80211_sub_if_data *sdata,
 		return false;
 	}
 
-	err = cfg80211_chandef_dfs_check(sdata->local->hw.wiphy,
+	ret = cfg80211_chandef_dfs_check(sdata->local->hw.wiphy,
 					 &params.chandef);
-	if (err < 0)
+	if (ret < 0)
 		return false;
-	if (err) {
+	if (ret) {
 		params.radar_required = true;
 		/* TODO: DFS not (yet) supported */
 		return false;

net/wireless/mesh.c +11 ~~-11~~

index d196a7c..1e74e5d 100644
@@ -100,7 +100,7 @@ int __cfg80211_join_mesh(struct cfg80211_registered_device *rdev,
 {
 	struct wireless_dev *wdev = dev->ieee80211_ptr;
 	u8 radar_detect_width = 0;
-	int err;
+	int ret;
 
 	BUILD_BUG_ON(IEEE80211_MAX_SSID_LEN != IEEE80211_MAX_MESH_ID_LEN);

@@ -178,27 +178,27 @@ int __cfg80211_join_mesh(struct cfg80211_registered_device *rdev,
 	if (!cfg80211_reg_can_beacon(&rdev->wiphy, &setup->chandef))
 		return -EINVAL;
 
-	err = cfg80211_chandef_dfs_check(wdev->wiphy, &setup->chandef);
-	if (err < 0)
-		return err;
-	if (err)
+	ret = cfg80211_chandef_dfs_check(wdev->wiphy, &setup->chandef);
+	if (ret < 0)
+		return ret;
+	if (ret)
 		radar_detect_width = BIT(setup->chandef.width);
 
-	err = cfg80211_can_use_iftype_chan(rdev, wdev, wdev->iftype,
+	ret = cfg80211_can_use_iftype_chan(rdev, wdev, wdev->iftype,
 					   setup->chandef.chan,
 					   CHAN_MODE_SHARED,
 					   radar_detect_width);
-	if (err)
-		return err;
+	if (ret)
+		return ret;
 
-	err = rdev_join_mesh(rdev, dev, conf, setup);
-	if (!err) {
+	ret = rdev_join_mesh(rdev, dev, conf, setup);
+	if (!ret) {
 		memcpy(wdev->ssid, setup->mesh_id, setup->mesh_id_len);
 		wdev->mesh_id_len = setup->mesh_id_len;
 		wdev->chandef = setup->chandef;
 	}
 
-	return err;
+	return ret;
 }
 
 int cfg80211_join_mesh(struct cfg80211_registered_device *rdev,

net/wireless/nl80211.c +59 ~~-59~~

256 lines not on this page · raw message

Signature · 1 line

1.9.0

Zhao, Gang

author

2014-04-20 03:39 UTC #4 lore raw

In reply to Zhao, Gang

Quoted 7 lines · Gang

I think the first patch should also be applied to 3.15.
Zhao, Gang (3):
cfg80211: fix incorrect checking of return value cfg80211/mac80211: rename cfg80211_chandef_dfs_required()
cfg80211: change variable name

I found that the incorrect checking of return value problem had already been fixed in mac80211-next 2beb6dab2d79 ("cfg80211/mac80211: refactor cfg80211_chandef_dfs_required()"). But not on mac80211 tree.

I think the fix may worth going to mac80211 and also stable-3.14. Johannes, you can decide whether to apply this patch or it needs to be modified, since if it's applied, it will surely cause conflict in future time.

The following two improvement patches should be ignored, since I found they need to be rebased to be applied on mac80211-next.

Johannes Berg

maintainer

2014-04-22 14:43 UTC #5 lore raw

Quoted 8 lines · Gang

I found that the incorrect checking of return value problem had already been fixed in mac80211-next 2beb6dab2d79 ("cfg80211/mac80211: refactor cfg80211_chandef_dfs_required()"). But not on mac80211 tree.
I think the fix may worth going to mac80211 and also stable-3.14. Johannes, you can decide whether to apply this patch or it needs to be modified, since if it's applied, it will surely cause conflict in future time.

I'll let the DFS folk worry about that, I don't use that myself ... if they need it then they might find the issue or whatever. Chances are, since CSA isn't there, they won't care much.

Quoted 2 lines

The following two improvement patches should be ignored, since I found they need to be rebased to be applied on mac80211-next.

OK.

johannes