Skip to contentSkip to conversation

mac802154: iface: better concurrent iface handling

PATCH Superseded Alexander Aring 2 participants 6 messages Dec 19, 2014

Subsystems (2)
Networking IEEE 802.15.4

v1 of 2 Superseded Latest is v2 →

lore
Share & tools
lore

Alexander Aring

author maintainer
#0 lore raw

cover mac802154: iface: better concurrent iface handling

This patch series contains changes for a better multiple iface handling. We have only one phy with several mac parameters like csma, aret, etc. Currently it could be that one interface can overwrite these parameters while ifup when another interface is currently running. To avoid that we compare each parameter and check if differs, if not we allow to ifup the interface.

Alexander Aring (2):
mac802154: iface: check concurrent ifaces
ieee802154: iface: move multiple node type check

 net/mac802154/iface.c | 101 +++++++++++++++++++++++++++++++++++++++++++-------
 1 file changed, 87 insertions(+), 14 deletions(-)
Signature · 1 line
2.2.0
author maintainer
#1 lore raw

PATCH 1/2 mac802154: iface: check concurrent ifaces

This patch adds a check for concurrent interfaces while calling interface up. This avoids to have different mac parameters on one phy. Otherwise it could be that a interface can overwrite current phy mac settings which is set by an another interface.

Signed-off-by Alexander Aring <[email protected]>
---
 net/mac802154/iface.c | 77 +++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 77 insertions(+)
net/mac802154/iface.c +77 -0
index 9ae8930..509621a 100644
@@ -175,6 +175,79 @@ err:
 	return res;
 }
 
+static int
+ieee802154_check_mac_settings(struct ieee802154_local *local,
+			      struct wpan_dev *wpan_dev,
+			      struct wpan_dev *nwpan_dev)
+{
+	ASSERT_RTNL();
+
+	if (local->hw.flags & IEEE802154_HW_PROMISCUOUS) {
+		if (wpan_dev->promiscuous_mode != nwpan_dev->promiscuous_mode)
+			return -EBUSY;
+	}
+
+	if (local->hw.flags & IEEE802154_HW_AFILT) {
+		if (wpan_dev->pan_id != nwpan_dev->pan_id)
+			return -EBUSY;
+
+		if (wpan_dev->short_addr != nwpan_dev->short_addr)
+			return -EBUSY;
+
+		if (wpan_dev->extended_addr != nwpan_dev->extended_addr)
+			return -EBUSY;
+	}
+
+	if (local->hw.flags & IEEE802154_HW_CSMA_PARAMS) {
+		if (wpan_dev->min_be != nwpan_dev->min_be)
+			return -EBUSY;
+
+		if (wpan_dev->max_be != nwpan_dev->max_be)
+			return -EBUSY;
+
+		if (wpan_dev->csma_retries != nwpan_dev->csma_retries)
+			return -EBUSY;
+	}
+
+	if (local->hw.flags & IEEE802154_HW_FRAME_RETRIES) {
+		if (wpan_dev->frame_retries != nwpan_dev->frame_retries)
+			return -EBUSY;
+	}
+
+	if (local->hw.flags & IEEE802154_HW_LBT) {
+		if (wpan_dev->lbt != nwpan_dev->lbt)
+			return -EBUSY;
+	}
+
+	return 0;
+}
+
+static int
+ieee802154_check_concurrent_iface(struct ieee802154_sub_if_data *sdata,
+				  enum nl802154_iftype iftype)
+{
+	struct ieee802154_local *local = sdata->local;
+	struct wpan_dev *wpan_dev = &sdata->wpan_dev;
+	struct ieee802154_sub_if_data *nsdata;
+
+	/* we hold the RTNL here so can safely walk the list */
+	list_for_each_entry(nsdata, &local->interfaces, list) {
+		if (nsdata != sdata && ieee802154_sdata_running(nsdata)) {
+			int ret;
+			/* check all phy mac sublayer settings are the same.
+			 * We have only one phy, different values makes trouble.
+			 */
+
+			ret = ieee802154_check_mac_settings(local, wpan_dev,
+							    &nsdata->wpan_dev);
+			if (ret < 0)
+				return ret;
+		}
+	}
+
+	return 0;
+}
+
 static int mac802154_wpan_open(struct net_device *dev)
 {
 	int rc;

@@ -183,6 +256,10 @@ static int mac802154_wpan_open(struct net_device *dev)
 	struct wpan_dev *wpan_dev = &sdata->wpan_dev;
 	struct wpan_phy *phy = sdata->local->phy;
 
+	rc = ieee802154_check_concurrent_iface(sdata, sdata->vif.type);
+	if (rc < 0)
+		return rc;
+
 	rc = mac802154_slave_open(dev);
 	if (rc < 0)
 		return rc;
Signature · 1 line
2.2.0
#2 lore raw

Re: [PATCH bluetooth-next 1/2] mac802154: iface: check concurrent ifaces

Hi Alex,

Quoted 71 lines

This patch adds a check for concurrent interfaces while calling interface up. This avoids to have different mac parameters on one phy. Otherwise it could be that a interface can overwrite current phy mac settings which is set by an another interface.

Signed-off-by Alexander Aring <[email protected]>

---
net/mac802154/iface.c | 77 +++++++++++++++++++++++++++++++++++++++++++++++++++

1 file changed, 77 insertions(+)
net/mac802154/iface.c +53 -0
index 9ae8930..509621a 100644
@@ -175,6 +175,79 @@ err:
 	return res;
 }
 
+static int
+ieee802154_check_mac_settings(struct ieee802154_local *local,
+			      struct wpan_dev *wpan_dev,
+			      struct wpan_dev *nwpan_dev)
+{
+	ASSERT_RTNL();
+
+	if (local->hw.flags & IEEE802154_HW_PROMISCUOUS) {
+		if (wpan_dev->promiscuous_mode != nwpan_dev->promiscuous_mode)
+			return -EBUSY;
+	}
+
+	if (local->hw.flags & IEEE802154_HW_AFILT) {
+		if (wpan_dev->pan_id != nwpan_dev->pan_id)
+			return -EBUSY;
+
+		if (wpan_dev->short_addr != nwpan_dev->short_addr)
+			return -EBUSY;
+
+		if (wpan_dev->extended_addr != nwpan_dev->extended_addr)
+			return -EBUSY;
+	}
+
+	if (local->hw.flags & IEEE802154_HW_CSMA_PARAMS) {
+		if (wpan_dev->min_be != nwpan_dev->min_be)
+			return -EBUSY;
+
+		if (wpan_dev->max_be != nwpan_dev->max_be)
+			return -EBUSY;
+
+		if (wpan_dev->csma_retries != nwpan_dev->csma_retries)
+			return -EBUSY;
+	}
+
+	if (local->hw.flags & IEEE802154_HW_FRAME_RETRIES) {
+		if (wpan_dev->frame_retries != nwpan_dev->frame_retries)
+			return -EBUSY;
+	}
+
+	if (local->hw.flags & IEEE802154_HW_LBT) {
+		if (wpan_dev->lbt != nwpan_dev->lbt)
+			return -EBUSY;
+	}
+
+	return 0;
+}
+
+static int
+ieee802154_check_concurrent_iface(struct ieee802154_sub_if_data *sdata,
+				  enum nl802154_iftype iftype)
+{
+	struct ieee802154_local *local = sdata->local;
+	struct wpan_dev *wpan_dev = &sdata->wpan_dev;

net/mac802154/iface.c err: · line 240

Quoted 10 lines
net/mac802154/iface.c +10 -0
+	struct ieee802154_sub_if_data *nsdata;
+
+	/* we hold the RTNL here so can safely walk the list */
+	list_for_each_entry(nsdata, &local->interfaces, list) {
+		if (nsdata != sdata && ieee802154_sdata_running(nsdata)) {
+			int ret;
+			/* check all phy mac sublayer settings are the same.
+			 * We have only one phy, different values makes trouble.
+			 */
+

I think this empty line was meant to be above the comment. So that the variable declaration is separated from the code.

Quoted 14 lines
net/mac802154/iface.c +10 -0
+			ret = ieee802154_check_mac_settings(local, wpan_dev,
+							    &nsdata->wpan_dev);
+			if (ret < 0)
+				return ret;
+		}
+	}
+
+	return 0;
+}
+
 static int mac802154_wpan_open(struct net_device *dev)
 {
 	int rc;

net/mac802154/iface.c static int mac802154_wpan_open(struct net_device *dev) · line 265

Quoted 10 lines
net/mac802154/iface.c +4 -0
@@ -183,6 +256,10 @@ static int mac802154_wpan_open(struct net_device *dev)
 	struct wpan_dev *wpan_dev = &sdata->wpan_dev;
 	struct wpan_phy *phy = sdata->local->phy;
 
+	rc = ieee802154_check_concurrent_iface(sdata, sdata->vif.type);
+	if (rc < 0)
+		return rc;
+
 	rc = mac802154_slave_open(dev);
 	if (rc < 0)
 		return rc;

Regards

Marcel

author maintainer
#3 lore raw

PATCH 2/2 ieee802154: iface: move multiple node type check

In reply to Alexander Aring

This patch moves the handling for checking on multiple node type interface to the corresponding concurrent iface check function.

Signed-off-by Alexander Aring <[email protected]>
---
 net/mac802154/iface.c | 24 ++++++++++--------------
 1 file changed, 10 insertions(+), 14 deletions(-)
net/mac802154/iface.c +10 -14
index 509621a..b5aab5a 100644
@@ -137,25 +137,11 @@ static int mac802154_wpan_mac_addr(struct net_device *dev, void *p)
 static int mac802154_slave_open(struct net_device *dev)
 {
 	struct ieee802154_sub_if_data *sdata = IEEE802154_DEV_TO_SUB_IF(dev);
-	struct ieee802154_sub_if_data *subif;
 	struct ieee802154_local *local = sdata->local;
 	int res = 0;
 
 	ASSERT_RTNL();
 
-	if (sdata->vif.type == NL802154_IFTYPE_NODE) {
-		mutex_lock(&sdata->local->iflist_mtx);
-		list_for_each_entry(subif, &sdata->local->interfaces, list) {
-			if (subif != sdata &&
-			    subif->vif.type == sdata->vif.type &&
-			    ieee802154_sdata_running(subif)) {
-				mutex_unlock(&sdata->local->iflist_mtx);
-				return -EBUSY;
-			}
-		}
-		mutex_unlock(&sdata->local->iflist_mtx);
-	}
-
 	set_bit(SDATA_STATE_RUNNING, &sdata->state);
 
 	if (!local->open_count) {

@@ -234,6 +220,16 @@ ieee802154_check_concurrent_iface(struct ieee802154_sub_if_data *sdata,
 	list_for_each_entry(nsdata, &local->interfaces, list) {
 		if (nsdata != sdata && ieee802154_sdata_running(nsdata)) {
 			int ret;
+
+			/* TODO currently we don't support multiple node types
+			 * we need to run skb_clone at rx path. Check if there
+			 * exist really an use case if we need to support
+			 * multiple node types at the same time.
+			 */
+			if (sdata->vif.type == NL802154_IFTYPE_NODE &&
+			    nsdata->vif.type == NL802154_IFTYPE_NODE)
+				return -EBUSY;
+
 			/* check all phy mac sublayer settings are the same.
 			 * We have only one phy, different values makes trouble.
 			 */
Signature · 1 line
2.2.0
#4 lore raw

Re: [PATCH bluetooth-next 2/2] ieee802154: iface: move multiple node type check

Hi Alex,

Quoted 42 lines

This patch moves the handling for checking on multiple node type interface to the corresponding concurrent iface check function.

Signed-off-by Alexander Aring <[email protected]>

---
net/mac802154/iface.c | 24 ++++++++++--------------

1 file changed, 10 insertions(+), 14 deletions(-)
net/mac802154/iface.c +0 -14
index 509621a..b5aab5a 100644
@@ -137,25 +137,11 @@ static int mac802154_wpan_mac_addr(struct net_device *dev, void *p)
 static int mac802154_slave_open(struct net_device *dev)
 {
 	struct ieee802154_sub_if_data *sdata = IEEE802154_DEV_TO_SUB_IF(dev);
-	struct ieee802154_sub_if_data *subif;
 	struct ieee802154_local *local = sdata->local;
 	int res = 0;
 
 	ASSERT_RTNL();
 
-	if (sdata->vif.type == NL802154_IFTYPE_NODE) {
-		mutex_lock(&sdata->local->iflist_mtx);
-		list_for_each_entry(subif, &sdata->local->interfaces, list) {
-			if (subif != sdata &&
-			    subif->vif.type == sdata->vif.type &&
-			    ieee802154_sdata_running(subif)) {
-				mutex_unlock(&sdata->local->iflist_mtx);
-				return -EBUSY;
-			}
-		}
-		mutex_unlock(&sdata->local->iflist_mtx);
-	}
-
 	set_bit(SDATA_STATE_RUNNING, &sdata->state);
 
 	if (!local->open_count) {

@@ -234,6 +220,16 @@ ieee802154_check_concurrent_iface(struct ieee802154_sub_if_data *sdata,
 	list_for_each_entry(nsdata, &local->interfaces, list) {
 		if (nsdata != sdata && ieee802154_sdata_running(nsdata)) {
 			int ret;

net/mac802154/iface.c ieee802154_check_concurrent_iface(struct ieee802154_sub_if_data *sdata, · line 232

Quoted 10 lines
net/mac802154/iface.c +10 -0
+
+			/* TODO currently we don't support multiple node types
+			 * we need to run skb_clone at rx path. Check if there
+			 * exist really an use case if we need to support
+			 * multiple node types at the same time.
+			 */
+			if (sdata->vif.type == NL802154_IFTYPE_NODE &&
+			    nsdata->vif.type == NL802154_IFTYPE_NODE)
+				return -EBUSY;
+

here you end up doing it correctly and fixing it, ..

net/mac802154/iface.c ieee802154_check_concurrent_iface(struct ieee802154_sub_if_data *sdata, · line 235

Quoted 3 lines
net/mac802154/iface.c +0 -0
 			/* check all phy mac sublayer settings are the same.
 			 * We have only one phy, different values makes trouble.
 			 */

.. but that still leaves the extra empty line after this comment. You need to redo these two patches.

Regards

Marcel

author maintainer
#5 lore raw

Re: [PATCH bluetooth-next 2/2] ieee802154: iface: move multiple node type check

Hi Marcel,

Quoted 57 lines · Marcel Holtmann

Hi Alex,

Quoted 42 lines

This patch moves the handling for checking on multiple node type interface to the corresponding concurrent iface check function.

Signed-off-by Alexander Aring <[email protected]>

---
net/mac802154/iface.c | 24 ++++++++++--------------

1 file changed, 10 insertions(+), 14 deletions(-)
net/mac802154/iface.c +0 -14
index 509621a..b5aab5a 100644
@@ -137,25 +137,11 @@ static int mac802154_wpan_mac_addr(struct net_device *dev, void *p)
 static int mac802154_slave_open(struct net_device *dev)
 {
 	struct ieee802154_sub_if_data *sdata = IEEE802154_DEV_TO_SUB_IF(dev);
-	struct ieee802154_sub_if_data *subif;
 	struct ieee802154_local *local = sdata->local;
 	int res = 0;
 
 	ASSERT_RTNL();
 
-	if (sdata->vif.type == NL802154_IFTYPE_NODE) {
-		mutex_lock(&sdata->local->iflist_mtx);
-		list_for_each_entry(subif, &sdata->local->interfaces, list) {
-			if (subif != sdata &&
-			    subif->vif.type == sdata->vif.type &&
-			    ieee802154_sdata_running(subif)) {
-				mutex_unlock(&sdata->local->iflist_mtx);
-				return -EBUSY;
-			}
-		}
-		mutex_unlock(&sdata->local->iflist_mtx);
-	}
-
 	set_bit(SDATA_STATE_RUNNING, &sdata->state);
 
 	if (!local->open_count) {

@@ -234,6 +220,16 @@ ieee802154_check_concurrent_iface(struct ieee802154_sub_if_data *sdata,
 	list_for_each_entry(nsdata, &local->interfaces, list) {
 		if (nsdata != sdata && ieee802154_sdata_running(nsdata)) {
 			int ret;

net/mac802154/iface.c ieee802154_check_concurrent_iface(struct ieee802154_sub_if_data *sdata, · line 232

Quoted 10 lines
net/mac802154/iface.c +10 -0
+
+			/* TODO currently we don't support multiple node types
+			 * we need to run skb_clone at rx path. Check if there
+			 * exist really an use case if we need to support
+			 * multiple node types at the same time.
+			 */
+			if (sdata->vif.type == NL802154_IFTYPE_NODE &&
+			    nsdata->vif.type == NL802154_IFTYPE_NODE)
+				return -EBUSY;
+

here you end up doing it correctly and fixing it, ..

Quoted 6 lines

net/mac802154/iface.c ieee802154_check_concurrent_iface(struct ieee802154_sub_if_data *sdata, · line 235

Quoted 3 lines
net/mac802154/iface.c +0 -0
 			/* check all phy mac sublayer settings are the same.
 			 * We have only one phy, different values makes trouble.
 			 */

.. but that still leaves the extra empty line after this comment. You need to redo these two patches.

ok.

- Alex