[PATCH bluetooth-next 0/3] mac802154: remove pib/mib locks

[PATCH bluetooth-next 0/3] mac802154: remove pib/mib locks

[Alexander Aring]

This patch series removes the pib/mib lock inside the mac802154 subsystem.
The new locking mechanism will use the rtnl lock to ensure that other
operations like ifdown/ifup can't happend while setting pib/mib values.

Note: the new locking strategy depends on context, on most mib values while
interface up the mib attributes are readonly, this will allow us less
complexity and locking stuff while frame parsing.

I tried my best to change the old netlink interface according to that, but
I believe there is no real locking "strategy" in the old netlink calls
and it's hard to understand how it works.

- Alex

Alexander Aring (3):
  mac802154: fix hold rtnl while ioctl
  mac802154: remove pib lock
  mac802154: remove mib lock

 include/net/cfg802154.h         |  2 --
 include/net/ieee802154_netdev.h | 10 -------
 net/ieee802154/6lowpan/core.c   | 28 ------------------
 net/ieee802154/6lowpan/tx.c     |  2 +-
 net/ieee802154/core.c           |  2 --
 net/ieee802154/nl-mac.c         | 14 ++++++---
 net/ieee802154/nl-phy.c         |  6 ++--
 net/ieee802154/socket.c         | 12 ++++----
 net/mac802154/ieee802154_i.h    |  9 ------
 net/mac802154/iface.c           | 25 +++++-----------
 net/mac802154/mac_cmd.c         | 10 +++----
 net/mac802154/mib.c             | 63 ++---------------------------------------
 net/mac802154/rx.c              |  5 ----
 13 files changed, 32 insertions(+), 156 deletions(-)

-- 
2.3.7

[PATCH bluetooth-next 1/3] mac802154: fix hold rtnl while ioctl

[Alexander Aring]

This patch fixes an issue to set address configuration with ioctl.
Accessing the mib requires rtnl lock and the ndo_do_ioctl doesn't hold
the rtnl lock while this callback is called. This patch do that
manually.

Signed-off-by: Alexander Aring <alex.aring@gmail.com>
Reported-by: Matteo Petracca <matteo.petracca@sssup.it>
---
 net/mac802154/iface.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/net/mac802154/iface.c b/net/mac802154/iface.c
index 91b75ab..2a58788 100644
--- a/net/mac802154/iface.c
+++ b/net/mac802154/iface.c
@@ -62,8 +62,7 @@ mac802154_wpan_ioctl(struct net_device *dev, struct ifreq *ifr, int cmd)
 		(struct sockaddr_ieee802154 *)&ifr->ifr_addr;
 	int err = -ENOIOCTLCMD;
 
-	ASSERT_RTNL();
-
+	rtnl_lock();
 	spin_lock_bh(&sdata->mib_lock);
 
 	switch (cmd) {
@@ -90,6 +89,7 @@ mac802154_wpan_ioctl(struct net_device *dev, struct ifreq *ifr, int cmd)
 	case SIOCSIFADDR:
 		if (netif_running(dev)) {
 			spin_unlock_bh(&sdata->mib_lock);
+			rtnl_unlock();
 			return -EBUSY;
 		}
 
@@ -112,6 +112,7 @@ mac802154_wpan_ioctl(struct net_device *dev, struct ifreq *ifr, int cmd)
 	}
 
 	spin_unlock_bh(&sdata->mib_lock);
+	rtnl_unlock();
 	return err;
 }
 
-- 
2.3.7

[PATCH bluetooth-next 2/3] mac802154: remove pib lock

[Alexander Aring]

This patch removes the pib lock which is now replaced by rtnl lock. The
new interface already use the rtnl lock only. Nevertheless this patch
will fix issues while using new and old interface at the same time.

Signed-off-by: Alexander Aring <alex.aring@gmail.com>
---
 include/net/cfg802154.h | 2 --
 net/ieee802154/core.c   | 2 --
 net/ieee802154/nl-phy.c | 6 +++---
 net/mac802154/iface.c   | 7 -------
 net/mac802154/mib.c     | 4 ++--
 5 files changed, 5 insertions(+), 16 deletions(-)

diff --git a/include/net/cfg802154.h b/include/net/cfg802154.h
index 6ea16c8..c867d4f 100644
--- a/include/net/cfg802154.h
+++ b/include/net/cfg802154.h
@@ -67,8 +67,6 @@ struct wpan_phy_cca {
 };
 
 struct wpan_phy {
-	struct mutex pib_lock;
-
 	/* If multiple wpan_phys are registered and you're handed e.g.
 	 * a regular netdev with assigned ieee802154_ptr, you won't
 	 * know whether it points to a wpan_phy your driver has registered
diff --git a/net/ieee802154/core.c b/net/ieee802154/core.c
index 2ee00e8..b0248e9 100644
--- a/net/ieee802154/core.c
+++ b/net/ieee802154/core.c
@@ -121,8 +121,6 @@ wpan_phy_new(const struct cfg802154_ops *ops, size_t priv_size)
 	/* atomic_inc_return makes it start at 1, make it start at 0 */
 	rdev->wpan_phy_idx--;
 
-	mutex_init(&rdev->wpan_phy.pib_lock);
-
 	INIT_LIST_HEAD(&rdev->wpan_dev_list);
 	device_initialize(&rdev->wpan_phy.dev);
 	dev_set_name(&rdev->wpan_phy.dev, PHY_NAME "%d", rdev->wpan_phy_idx);
diff --git a/net/ieee802154/nl-phy.c b/net/ieee802154/nl-phy.c
index 346c666..543bd09 100644
--- a/net/ieee802154/nl-phy.c
+++ b/net/ieee802154/nl-phy.c
@@ -50,7 +50,7 @@ static int ieee802154_nl_fill_phy(struct sk_buff *msg, u32 portid,
 	if (!hdr)
 		goto out;
 
-	mutex_lock(&phy->pib_lock);
+	rtnl_lock();
 	if (nla_put_string(msg, IEEE802154_ATTR_PHY_NAME, wpan_phy_name(phy)) ||
 	    nla_put_u8(msg, IEEE802154_ATTR_PAGE, phy->current_page) ||
 	    nla_put_u8(msg, IEEE802154_ATTR_CHANNEL, phy->current_channel))
@@ -63,13 +63,13 @@ static int ieee802154_nl_fill_phy(struct sk_buff *msg, u32 portid,
 	    nla_put(msg, IEEE802154_ATTR_CHANNEL_PAGE_LIST,
 		    pages * sizeof(uint32_t), buf))
 		goto nla_put_failure;
-	mutex_unlock(&phy->pib_lock);
+	rtnl_unlock();
 	kfree(buf);
 	genlmsg_end(msg, hdr);
 	return 0;
 
 nla_put_failure:
-	mutex_unlock(&phy->pib_lock);
+	rtnl_unlock();
 	genlmsg_cancel(msg, hdr);
 out:
 	kfree(buf);
diff --git a/net/mac802154/iface.c b/net/mac802154/iface.c
index 2a58788..22f478b 100644
--- a/net/mac802154/iface.c
+++ b/net/mac802154/iface.c
@@ -242,7 +242,6 @@ static int mac802154_wpan_open(struct net_device *dev)
 	struct ieee802154_sub_if_data *sdata = IEEE802154_DEV_TO_SUB_IF(dev);
 	struct ieee802154_local *local = sdata->local;
 	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)
@@ -252,8 +251,6 @@ static int mac802154_wpan_open(struct net_device *dev)
 	if (rc < 0)
 		return rc;
 
-	mutex_lock(&phy->pib_lock);
-
 	if (local->hw.flags & IEEE802154_HW_PROMISCUOUS) {
 		rc = drv_set_promiscuous_mode(local,
 					      wpan_dev->promiscuous_mode);
@@ -295,11 +292,7 @@ static int mac802154_wpan_open(struct net_device *dev)
 			goto out;
 	}
 
-	mutex_unlock(&phy->pib_lock);
-	return 0;
-
 out:
-	mutex_unlock(&phy->pib_lock);
 	return rc;
 }
 
diff --git a/net/mac802154/mib.c b/net/mac802154/mib.c
index 5cf019a..437ed44 100644
--- a/net/mac802154/mib.c
+++ b/net/mac802154/mib.c
@@ -97,10 +97,10 @@ void mac802154_dev_set_page_channel(struct net_device *dev, u8 page, u8 chan)
 	if (res) {
 		pr_debug("set_channel failed\n");
 	} else {
-		mutex_lock(&local->phy->pib_lock);
+		rtnl_lock();
 		local->phy->current_channel = chan;
 		local->phy->current_page = page;
-		mutex_unlock(&local->phy->pib_lock);
+		rtnl_unlock();
 	}
 }
 
-- 
2.3.7

[PATCH bluetooth-next 3/3] mac802154: remove mib lock

[Alexander Aring]

This patch removes the mib lock. The new locking mechanism is to protect
the mib values with the rtnl lock. Note that this isn't always necessary
if we have an interface up the most mib values are readonly (e.g.
address settings). With this behaviour we can remove locking in
hotpath like frame parsing completely. It depends on context if we need
to hold the rtnl lock or not, this makes the callbacks of
ieee802154_mlme_ops unnecessary because these callbacks hols always the
locks.

Signed-off-by: Alexander Aring <alex.aring@gmail.com>
---
 include/net/ieee802154_netdev.h | 10 -------
 net/ieee802154/6lowpan/core.c   | 28 -------------------
 net/ieee802154/6lowpan/tx.c     |  2 +-
 net/ieee802154/nl-mac.c         | 14 +++++++---
 net/ieee802154/socket.c         | 12 ++++-----
 net/mac802154/ieee802154_i.h    |  9 -------
 net/mac802154/iface.c           | 13 +++------
 net/mac802154/mac_cmd.c         | 10 +++----
 net/mac802154/mib.c             | 59 -----------------------------------------
 net/mac802154/rx.c              |  5 ----
 10 files changed, 24 insertions(+), 138 deletions(-)

diff --git a/include/net/ieee802154_netdev.h b/include/net/ieee802154_netdev.h
index 94a2970..84a72a1 100644
--- a/include/net/ieee802154_netdev.h
+++ b/include/net/ieee802154_netdev.h
@@ -422,16 +422,6 @@ struct ieee802154_mlme_ops {
 			       struct ieee802154_mac_params *params);
 
 	struct ieee802154_llsec_ops *llsec;
-
-	/* The fields below are required. */
-
-	/*
-	 * FIXME: these should become the part of PIB/MIB interface.
-	 * However we still don't have IB interface of any kind
-	 */
-	__le16 (*get_pan_id)(const struct net_device *dev);
-	__le16 (*get_short_addr)(const struct net_device *dev);
-	u8 (*get_dsn)(const struct net_device *dev);
 };
 
 static inline struct ieee802154_mlme_ops *
diff --git a/net/ieee802154/6lowpan/core.c b/net/ieee802154/6lowpan/core.c
index 0ae5822..f20a387 100644
--- a/net/ieee802154/6lowpan/core.c
+++ b/net/ieee802154/6lowpan/core.c
@@ -55,27 +55,6 @@
 LIST_HEAD(lowpan_devices);
 static int lowpan_open_count;
 
-static __le16 lowpan_get_pan_id(const struct net_device *dev)
-{
-	struct net_device *real_dev = lowpan_dev_info(dev)->real_dev;
-
-	return ieee802154_mlme_ops(real_dev)->get_pan_id(real_dev);
-}
-
-static __le16 lowpan_get_short_addr(const struct net_device *dev)
-{
-	struct net_device *real_dev = lowpan_dev_info(dev)->real_dev;
-
-	return ieee802154_mlme_ops(real_dev)->get_short_addr(real_dev);
-}
-
-static u8 lowpan_get_dsn(const struct net_device *dev)
-{
-	struct net_device *real_dev = lowpan_dev_info(dev)->real_dev;
-
-	return ieee802154_mlme_ops(real_dev)->get_dsn(real_dev);
-}
-
 static struct header_ops lowpan_header_ops = {
 	.create	= lowpan_header_create,
 };
@@ -103,12 +82,6 @@ static const struct net_device_ops lowpan_netdev_ops = {
 	.ndo_start_xmit		= lowpan_xmit,
 };
 
-static struct ieee802154_mlme_ops lowpan_mlme = {
-	.get_pan_id = lowpan_get_pan_id,
-	.get_short_addr = lowpan_get_short_addr,
-	.get_dsn = lowpan_get_dsn,
-};
-
 static void lowpan_setup(struct net_device *dev)
 {
 	dev->addr_len		= IEEE802154_ADDR_LEN;
@@ -124,7 +97,6 @@ static void lowpan_setup(struct net_device *dev)
 
 	dev->netdev_ops		= &lowpan_netdev_ops;
 	dev->header_ops		= &lowpan_header_ops;
-	dev->ml_priv		= &lowpan_mlme;
 	dev->destructor		= free_netdev;
 	dev->features		|= NETIF_F_NETNS_LOCAL;
 }
diff --git a/net/ieee802154/6lowpan/tx.c b/net/ieee802154/6lowpan/tx.c
index 2349070..98acf73 100644
--- a/net/ieee802154/6lowpan/tx.c
+++ b/net/ieee802154/6lowpan/tx.c
@@ -207,7 +207,7 @@ static int lowpan_header(struct sk_buff *skb, struct net_device *dev)
 
 	/* prepare wpan address data */
 	sa.mode = IEEE802154_ADDR_LONG;
-	sa.pan_id = ieee802154_mlme_ops(dev)->get_pan_id(dev);
+	sa.pan_id = lowpan_dev_info(dev)->real_dev->ieee802154_ptr->pan_id;
 	sa.extended_addr = ieee802154_devaddr_from_raw(saddr);
 
 	/* intra-PAN communications */
diff --git a/net/ieee802154/nl-mac.c b/net/ieee802154/nl-mac.c
index 2b4955d..ecff7cb 100644
--- a/net/ieee802154/nl-mac.c
+++ b/net/ieee802154/nl-mac.c
@@ -97,8 +97,10 @@ static int ieee802154_nl_fill_iface(struct sk_buff *msg, u32 portid,
 	BUG_ON(!phy);
 	get_device(&phy->dev);
 
-	short_addr = ops->get_short_addr(dev);
-	pan_id = ops->get_pan_id(dev);
+	rtnl_lock();
+	short_addr = dev->ieee802154_ptr->short_addr;
+	pan_id = dev->ieee802154_ptr->pan_id;
+	rtnl_unlock();
 
 	if (nla_put_string(msg, IEEE802154_ATTR_DEV_NAME, dev->name) ||
 	    nla_put_string(msg, IEEE802154_ATTR_PHY_NAME, wpan_phy_name(phy)) ||
@@ -244,7 +246,9 @@ int ieee802154_associate_resp(struct sk_buff *skb, struct genl_info *info)
 	addr.mode = IEEE802154_ADDR_LONG;
 	addr.extended_addr = nla_get_hwaddr(
 			info->attrs[IEEE802154_ATTR_DEST_HW_ADDR]);
-	addr.pan_id = ieee802154_mlme_ops(dev)->get_pan_id(dev);
+	rtnl_lock();
+	addr.pan_id = dev->ieee802154_ptr->pan_id;
+	rtnl_unlock();
 
 	ret = ieee802154_mlme_ops(dev)->assoc_resp(dev, &addr,
 		nla_get_shortaddr(info->attrs[IEEE802154_ATTR_DEST_SHORT_ADDR]),
@@ -281,7 +285,9 @@ int ieee802154_disassociate_req(struct sk_buff *skb, struct genl_info *info)
 		addr.short_addr = nla_get_shortaddr(
 				info->attrs[IEEE802154_ATTR_DEST_SHORT_ADDR]);
 	}
-	addr.pan_id = ieee802154_mlme_ops(dev)->get_pan_id(dev);
+	rtnl_lock();
+	addr.pan_id = dev->ieee802154_ptr->pan_id;
+	rtnl_unlock();
 
 	ret = ieee802154_mlme_ops(dev)->disassoc_req(dev, &addr,
 			nla_get_u8(info->attrs[IEEE802154_ATTR_REASON]));
diff --git a/net/ieee802154/socket.c b/net/ieee802154/socket.c
index b60c65f..ffd7018 100644
--- a/net/ieee802154/socket.c
+++ b/net/ieee802154/socket.c
@@ -64,10 +64,8 @@ ieee802154_get_dev(struct net *net, const struct ieee802154_addr *addr)
 			if (tmp->type != ARPHRD_IEEE802154)
 				continue;
 
-			pan_id = ieee802154_mlme_ops(tmp)->get_pan_id(tmp);
-			short_addr =
-				ieee802154_mlme_ops(tmp)->get_short_addr(tmp);
-
+			pan_id = tmp->ieee802154_ptr->pan_id;
+			short_addr = tmp->ieee802154_ptr->short_addr;
 			if (pan_id == addr->pan_id &&
 			    short_addr == addr->short_addr) {
 				dev = tmp;
@@ -797,9 +795,9 @@ static int ieee802154_dgram_deliver(struct net_device *dev, struct sk_buff *skb)
 	/* Data frame processing */
 	BUG_ON(dev->type != ARPHRD_IEEE802154);
 
-	pan_id = ieee802154_mlme_ops(dev)->get_pan_id(dev);
-	short_addr = ieee802154_mlme_ops(dev)->get_short_addr(dev);
-	hw_addr = ieee802154_devaddr_from_raw(dev->dev_addr);
+	pan_id = dev->ieee802154_ptr->pan_id;
+	short_addr = dev->ieee802154_ptr->short_addr;
+	hw_addr = dev->ieee802154_ptr->extended_addr;
 
 	read_lock(&dgram_lock);
 	sk_for_each(sk, &dgram_head) {
diff --git a/net/mac802154/ieee802154_i.h b/net/mac802154/ieee802154_i.h
index 127ba18..4204024 100644
--- a/net/mac802154/ieee802154_i.h
+++ b/net/mac802154/ieee802154_i.h
@@ -86,8 +86,6 @@ struct ieee802154_sub_if_data {
 	unsigned long state;
 	char name[IFNAMSIZ];
 
-	spinlock_t mib_lock;
-
 	/* protects sec from concurrent access by netlink. access by
 	 * encrypt/decrypt/header_create safe without additional protection.
 	 */
@@ -136,13 +134,6 @@ ieee802154_subif_start_xmit(struct sk_buff *skb, struct net_device *dev);
 enum hrtimer_restart ieee802154_xmit_ifs_timer(struct hrtimer *timer);
 
 /* MIB callbacks */
-void mac802154_dev_set_short_addr(struct net_device *dev, __le16 val);
-__le16 mac802154_dev_get_short_addr(const struct net_device *dev);
-__le16 mac802154_dev_get_pan_id(const struct net_device *dev);
-void mac802154_dev_set_pan_id(struct net_device *dev, __le16 val);
-void mac802154_dev_set_page_channel(struct net_device *dev, u8 page, u8 chan);
-u8 mac802154_dev_get_dsn(const struct net_device *dev);
-
 int mac802154_get_params(struct net_device *dev,
 			 struct ieee802154_llsec_params *params);
 int mac802154_set_params(struct net_device *dev,
diff --git a/net/mac802154/iface.c b/net/mac802154/iface.c
index 22f478b..866d27f 100644
--- a/net/mac802154/iface.c
+++ b/net/mac802154/iface.c
@@ -63,7 +63,6 @@ mac802154_wpan_ioctl(struct net_device *dev, struct ifreq *ifr, int cmd)
 	int err = -ENOIOCTLCMD;
 
 	rtnl_lock();
-	spin_lock_bh(&sdata->mib_lock);
 
 	switch (cmd) {
 	case SIOCGIFADDR:
@@ -88,7 +87,6 @@ mac802154_wpan_ioctl(struct net_device *dev, struct ifreq *ifr, int cmd)
 	}
 	case SIOCSIFADDR:
 		if (netif_running(dev)) {
-			spin_unlock_bh(&sdata->mib_lock);
 			rtnl_unlock();
 			return -EBUSY;
 		}
@@ -111,7 +109,6 @@ mac802154_wpan_ioctl(struct net_device *dev, struct ifreq *ifr, int cmd)
 		break;
 	}
 
-	spin_unlock_bh(&sdata->mib_lock);
 	rtnl_unlock();
 	return err;
 }
@@ -368,14 +365,15 @@ static int mac802154_header_create(struct sk_buff *skb,
 	hdr.fc.type = cb->type;
 	hdr.fc.security_enabled = cb->secen;
 	hdr.fc.ack_request = cb->ackreq;
-	hdr.seq = ieee802154_mlme_ops(dev)->get_dsn(dev);
+	/* TODO: use atomic_t as dsn, dsn need to be locked when AF_IEEE802154
+	 * and IEEE802154 6LoWPAN call this at the same time.
+	 */
+	hdr.seq = dev->ieee802154_ptr->dsn++;
 
 	if (mac802154_set_header_security(sdata, &hdr, cb) < 0)
 		return -EINVAL;
 
 	if (!saddr) {
-		spin_lock_bh(&sdata->mib_lock);
-
 		if (wpan_dev->short_addr == cpu_to_le16(IEEE802154_ADDR_BROADCAST) ||
 		    wpan_dev->short_addr == cpu_to_le16(IEEE802154_ADDR_UNDEF) ||
 		    wpan_dev->pan_id == cpu_to_le16(IEEE802154_PANID_BROADCAST)) {
@@ -387,8 +385,6 @@ static int mac802154_header_create(struct sk_buff *skb,
 		}
 
 		hdr.source.pan_id = wpan_dev->pan_id;
-
-		spin_unlock_bh(&sdata->mib_lock);
 	} else {
 		hdr.source = *(const struct ieee802154_addr *)saddr;
 	}
@@ -497,7 +493,6 @@ ieee802154_setup_sdata(struct ieee802154_sub_if_data *sdata,
 		sdata->dev->ml_priv = &mac802154_mlme_wpan;
 		wpan_dev->promiscuous_mode = false;
 
-		spin_lock_init(&sdata->mib_lock);
 		mutex_init(&sdata->sec_mtx);
 
 		mac802154_llsec_init(&sdata->sec);
diff --git a/net/mac802154/mac_cmd.c b/net/mac802154/mac_cmd.c
index bdccb4e..faae5f29 100644
--- a/net/mac802154/mac_cmd.c
+++ b/net/mac802154/mac_cmd.c
@@ -43,9 +43,10 @@ static int mac802154_mlme_start_req(struct net_device *dev,
 
 	BUG_ON(addr->mode != IEEE802154_ADDR_SHORT);
 
-	mac802154_dev_set_pan_id(dev, addr->pan_id);
-	mac802154_dev_set_short_addr(dev, addr->short_addr);
-	mac802154_dev_set_page_channel(dev, page, channel);
+	dev->ieee802154_ptr->pan_id = addr->pan_id;
+	dev->ieee802154_ptr->short_addr = addr->short_addr;
+	dev->ieee802154_ptr->wpan_phy->current_channel = channel;
+	dev->ieee802154_ptr->wpan_phy->current_page = page;
 
 	if (ops->llsec) {
 		struct ieee802154_llsec_params params;
@@ -151,9 +152,6 @@ static struct ieee802154_llsec_ops mac802154_llsec_ops = {
 
 struct ieee802154_mlme_ops mac802154_mlme_wpan = {
 	.start_req = mac802154_mlme_start_req,
-	.get_pan_id = mac802154_dev_get_pan_id,
-	.get_short_addr = mac802154_dev_get_short_addr,
-	.get_dsn = mac802154_dev_get_dsn,
 
 	.llsec = &mac802154_llsec_ops,
 
diff --git a/net/mac802154/mib.c b/net/mac802154/mib.c
index 437ed44..c3a6a6a 100644
--- a/net/mac802154/mib.c
+++ b/net/mac802154/mib.c
@@ -26,65 +26,6 @@
 #include "ieee802154_i.h"
 #include "driver-ops.h"
 
-void mac802154_dev_set_short_addr(struct net_device *dev, __le16 val)
-{
-	struct ieee802154_sub_if_data *sdata = IEEE802154_DEV_TO_SUB_IF(dev);
-
-	BUG_ON(dev->type != ARPHRD_IEEE802154);
-
-	spin_lock_bh(&sdata->mib_lock);
-	sdata->wpan_dev.short_addr = val;
-	spin_unlock_bh(&sdata->mib_lock);
-}
-
-__le16 mac802154_dev_get_short_addr(const struct net_device *dev)
-{
-	struct ieee802154_sub_if_data *sdata = IEEE802154_DEV_TO_SUB_IF(dev);
-	__le16 ret;
-
-	BUG_ON(dev->type != ARPHRD_IEEE802154);
-
-	spin_lock_bh(&sdata->mib_lock);
-	ret = sdata->wpan_dev.short_addr;
-	spin_unlock_bh(&sdata->mib_lock);
-
-	return ret;
-}
-
-__le16 mac802154_dev_get_pan_id(const struct net_device *dev)
-{
-	struct ieee802154_sub_if_data *sdata = IEEE802154_DEV_TO_SUB_IF(dev);
-	__le16 ret;
-
-	BUG_ON(dev->type != ARPHRD_IEEE802154);
-
-	spin_lock_bh(&sdata->mib_lock);
-	ret = sdata->wpan_dev.pan_id;
-	spin_unlock_bh(&sdata->mib_lock);
-
-	return ret;
-}
-
-void mac802154_dev_set_pan_id(struct net_device *dev, __le16 val)
-{
-	struct ieee802154_sub_if_data *sdata = IEEE802154_DEV_TO_SUB_IF(dev);
-
-	BUG_ON(dev->type != ARPHRD_IEEE802154);
-
-	spin_lock_bh(&sdata->mib_lock);
-	sdata->wpan_dev.pan_id = val;
-	spin_unlock_bh(&sdata->mib_lock);
-}
-
-u8 mac802154_dev_get_dsn(const struct net_device *dev)
-{
-	struct ieee802154_sub_if_data *sdata = IEEE802154_DEV_TO_SUB_IF(dev);
-
-	BUG_ON(dev->type != ARPHRD_IEEE802154);
-
-	return sdata->wpan_dev.dsn++;
-}
-
 void mac802154_dev_set_page_channel(struct net_device *dev, u8 page, u8 chan)
 {
 	struct ieee802154_sub_if_data *sdata = IEEE802154_DEV_TO_SUB_IF(dev);
diff --git a/net/mac802154/rx.c b/net/mac802154/rx.c
index c0d67b2..e0f1006 100644
--- a/net/mac802154/rx.c
+++ b/net/mac802154/rx.c
@@ -47,8 +47,6 @@ ieee802154_subif_frame(struct ieee802154_sub_if_data *sdata,
 
 	pr_debug("getting packet via slave interface %s\n", sdata->dev->name);
 
-	spin_lock_bh(&sdata->mib_lock);
-
 	span = wpan_dev->pan_id;
 	sshort = wpan_dev->short_addr;
 
@@ -83,13 +81,10 @@ ieee802154_subif_frame(struct ieee802154_sub_if_data *sdata,
 			skb->pkt_type = PACKET_OTHERHOST;
 		break;
 	default:
-		spin_unlock_bh(&sdata->mib_lock);
 		pr_debug("invalid dest mode\n");
 		goto fail;
 	}
 
-	spin_unlock_bh(&sdata->mib_lock);
-
 	skb->dev = sdata->dev;
 
 	rc = mac802154_llsec_decrypt(&sdata->sec, skb);
-- 
2.3.7

Re: [PATCH bluetooth-next 3/3] mac802154: remove mib lock

[Alexander Aring]

Hi,

On Sun, May 10, 2015 at 10:40:56PM +0200, Alexander Aring wrote:
>  	hdr.fc.type = cb->type;
>  	hdr.fc.security_enabled = cb->secen;
>  	hdr.fc.ack_request = cb->ackreq;
> -	hdr.seq = ieee802154_mlme_ops(dev)->get_dsn(dev);
> +	/* TODO: use atomic_t as dsn, dsn need to be locked when AF_IEEE802154
> +	 * and IEEE802154 6LoWPAN call this at the same time.
> +	 */
> +	hdr.seq = dev->ieee802154_ptr->dsn++;

I am thinking that a good solution would be:

diff --git a/include/net/cfg802154.h b/include/net/cfg802154.h
index c6aa1d2..91550c4 100644
--- a/include/net/cfg802154.h
+++ b/include/net/cfg802154.h
@@ -177,9 +177,9 @@ struct wpan_dev {
        __le64 extended_addr;
 
        /* MAC BSN field */
-       u8 bsn;
+       u8 __percpu *bsn;
        /* MAC DSN field */
-       u8 dsn;
+       u8 __percpu *dsn;
 
        u8 min_be;
        u8 max_be;
diff --git a/net/mac802154/iface.c b/net/mac802154/iface.c
index 866d27f..343ef29 100644
--- a/net/mac802154/iface.c
+++ b/net/mac802154/iface.c
@@ -365,10 +365,7 @@ static int mac802154_header_create(struct sk_buff *skb,
        hdr.fc.type = cb->type;
        hdr.fc.security_enabled = cb->secen;
        hdr.fc.ack_request = cb->ackreq;
-       /* TODO: use atomic_t as dsn, dsn need to be locked when AF_IEEE802154
-        * and IEEE802154 6LoWPAN call this at the same time.
-        */
-       hdr.seq = dev->ieee802154_ptr->dsn++;
+       hdr.seq = this_cpu_inc_return(*wpan_dev->dsn);
 
        if (mac802154_set_header_security(sdata, &hdr, cb) < 0)
                return -EINVAL;
@@ -442,6 +439,8 @@ static void mac802154_wpan_free(struct net_device *dev)
 {
        struct ieee802154_sub_if_data *sdata = IEEE802154_DEV_TO_SUB_IF(dev);
 
+       free_percpu(sdata->wpan_dev.dsn);
+       free_percpu(sdata->wpan_dev.bsn);
        mac802154_llsec_destroy(&sdata->sec);
 
        free_netdev(dev);
@@ -464,13 +463,25 @@ ieee802154_setup_sdata(struct ieee802154_sub_if_data *sdata,
                       enum nl802154_iftype type)
 {
        struct wpan_dev *wpan_dev = &sdata->wpan_dev;
+       u8 tmp;
 
        /* set some type-dependent values */
        sdata->vif.type = type;
        sdata->wpan_dev.iftype = type;
 
-       get_random_bytes(&wpan_dev->bsn, 1);
-       get_random_bytes(&wpan_dev->dsn, 1);
+       wpan_dev->bsn = alloc_percpu(u8);
+       if (!wpan_dev->bsn)
+               return -ENOMEM;
+
+       get_random_bytes(&tmp, 1);
+       this_cpu_write(*wpan_dev->bsn, tmp);
+
+       wpan_dev->dsn = alloc_percpu(u8);
+       if (!wpan_dev->dsn)
+               return -ENOMEM;
+
+       get_random_bytes(&tmp, 1);
+       this_cpu_write(*wpan_dev->dsn, tmp);
 
        /* defaults per 802.15.4-2011 */
        wpan_dev->min_be = 3;


Introduce a percpu counter for the sequence numbers, incrementation of
this counter is an atomic operation then and we are sure that we don't
sending the same sequence number when calling this function at the  same
time.

- Alex

Re: [PATCH bluetooth-next 3/3] mac802154: remove mib lock

[Phoebe Buckheister]

On Tue, 12 May 2015 09:50:19 +0200
Alexander Aring <alex.aring@gmail.com> wrote:

> Hi,
> 
> On Sun, May 10, 2015 at 10:40:56PM +0200, Alexander Aring wrote:
> >  	hdr.fc.type = cb->type;
> >  	hdr.fc.security_enabled = cb->secen;
> >  	hdr.fc.ack_request = cb->ackreq;
> > -	hdr.seq = ieee802154_mlme_ops(dev)->get_dsn(dev);
> > +	/* TODO: use atomic_t as dsn, dsn need to be locked when
> > AF_IEEE802154
> > +	 * and IEEE802154 6LoWPAN call this at the same time.
> > +	 */
> > +	hdr.seq = dev->ieee802154_ptr->dsn++;
> 
> I am thinking that a good solution would be:
> 
> diff --git a/include/net/cfg802154.h b/include/net/cfg802154.h
> index c6aa1d2..91550c4 100644
> --- a/include/net/cfg802154.h
> +++ b/include/net/cfg802154.h
> @@ -177,9 +177,9 @@ struct wpan_dev {
>         __le64 extended_addr;
>  
>         /* MAC BSN field */
> -       u8 bsn;
> +       u8 __percpu *bsn;
>         /* MAC DSN field */
> -       u8 dsn;
> +       u8 __percpu *dsn;
>  
>         u8 min_be;
>         u8 max_be;
> diff --git a/net/mac802154/iface.c b/net/mac802154/iface.c
> index 866d27f..343ef29 100644
> --- a/net/mac802154/iface.c
> +++ b/net/mac802154/iface.c
> @@ -365,10 +365,7 @@ static int mac802154_header_create(struct
> sk_buff *skb, hdr.fc.type = cb->type;
>         hdr.fc.security_enabled = cb->secen;
>         hdr.fc.ack_request = cb->ackreq;
> -       /* TODO: use atomic_t as dsn, dsn need to be locked when
> AF_IEEE802154
> -        * and IEEE802154 6LoWPAN call this at the same time.
> -        */
> -       hdr.seq = dev->ieee802154_ptr->dsn++;
> +       hdr.seq = this_cpu_inc_return(*wpan_dev->dsn);
>  
>         if (mac802154_set_header_security(sdata, &hdr, cb) < 0)
>                 return -EINVAL;
> @@ -442,6 +439,8 @@ static void mac802154_wpan_free(struct net_device
> *dev) {
>         struct ieee802154_sub_if_data *sdata =
> IEEE802154_DEV_TO_SUB_IF(dev); 
> +       free_percpu(sdata->wpan_dev.dsn);
> +       free_percpu(sdata->wpan_dev.bsn);
>         mac802154_llsec_destroy(&sdata->sec);
>  
>         free_netdev(dev);
> @@ -464,13 +463,25 @@ ieee802154_setup_sdata(struct
> ieee802154_sub_if_data *sdata, enum nl802154_iftype type)
>  {
>         struct wpan_dev *wpan_dev = &sdata->wpan_dev;
> +       u8 tmp;
>  
>         /* set some type-dependent values */
>         sdata->vif.type = type;
>         sdata->wpan_dev.iftype = type;
>  
> -       get_random_bytes(&wpan_dev->bsn, 1);
> -       get_random_bytes(&wpan_dev->dsn, 1);
> +       wpan_dev->bsn = alloc_percpu(u8);
> +       if (!wpan_dev->bsn)
> +               return -ENOMEM;
> +
> +       get_random_bytes(&tmp, 1);
> +       this_cpu_write(*wpan_dev->bsn, tmp);
> +
> +       wpan_dev->dsn = alloc_percpu(u8);
> +       if (!wpan_dev->dsn)
> +               return -ENOMEM;
> +
> +       get_random_bytes(&tmp, 1);
> +       this_cpu_write(*wpan_dev->dsn, tmp);
>  
>         /* defaults per 802.15.4-2011 */
>         wpan_dev->min_be = 3;
> 
> 
> Introduce a percpu counter for the sequence numbers, incrementation of
> this counter is an atomic operation then and we are sure that we don't
> sending the same sequence number when calling this function at the
> same time.

With this, two threads running on the same interface can send different
packets with the same sequence number back to back. Maybe better make
it atomic instead of percpu instead to avoid that?

> - Alex
> --
> To unsubscribe from this list: send the line "unsubscribe linux-wpan"
> in the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH bluetooth-next 3/3] mac802154: remove mib lock

[Alexander Aring]

Hi,

...
> > 
> > Introduce a percpu counter for the sequence numbers, incrementation of
> > this counter is an atomic operation then and we are sure that we don't
> > sending the same sequence number when calling this function at the
> > same time.
> 
> With this, two threads running on the same interface can send different
> packets with the same sequence number back to back. Maybe better make
> it atomic instead of percpu instead to avoid that?
> 

Yes you are right, that's not correct. Because per_cpu is a local
variable what's the name said _per_ _cpu_. For this kind of very global
mib value which needs to be incremented after each transmit a atomic_t
should be correct here and that's also what the comment said.

Damn, why I thought that a percpu variable should be correct here.

- Alex

Re: [PATCH bluetooth-next 3/3] mac802154: remove mib lock

[Alexander Aring]

On Tue, May 12, 2015 at 01:52:58PM +0200, Alexander Aring wrote:
> Hi,
> 
> ...
> > > 
> > > Introduce a percpu counter for the sequence numbers, incrementation of
> > > this counter is an atomic operation then and we are sure that we don't
> > > sending the same sequence number when calling this function at the
> > > same time.
> > 
> > With this, two threads running on the same interface can send different
> > packets with the same sequence number back to back. Maybe better make
> > it atomic instead of percpu instead to avoid that?
> > 
> 
> Yes you are right, that's not correct. Because per_cpu is a local
> variable what's the name said _per_ _cpu_. For this kind of very global
> mib value which needs to be incremented after each transmit a atomic_t
> should be correct here and that's also what the comment said.
> 
> Damn, why I thought that a percpu variable should be correct here.
> 

So I updated the draft with this, I hope that is more correct than the
previous one, which really makes no sense:

diff --git a/include/net/cfg802154.h b/include/net/cfg802154.h
index c6aa1d2..4de59aa 100644
--- a/include/net/cfg802154.h
+++ b/include/net/cfg802154.h
@@ -177,9 +177,9 @@ struct wpan_dev {
        __le64 extended_addr;
 
        /* MAC BSN field */
-       u8 bsn;
+       atomic_t bsn;
        /* MAC DSN field */
-       u8 dsn;
+       atomic_t dsn;
 
        u8 min_be;
        u8 max_be;
diff --git a/net/mac802154/iface.c b/net/mac802154/iface.c
index 866d27f..b99a6f6 100644
--- a/net/mac802154/iface.c
+++ b/net/mac802154/iface.c
@@ -365,10 +365,7 @@ static int mac802154_header_create(struct sk_buff *skb,
        hdr.fc.type = cb->type;
        hdr.fc.security_enabled = cb->secen;
        hdr.fc.ack_request = cb->ackreq;
-       /* TODO: use atomic_t as dsn, dsn need to be locked when AF_IEEE802154
-        * and IEEE802154 6LoWPAN call this at the same time.
-        */
-       hdr.seq = dev->ieee802154_ptr->dsn++;
+       hdr.seq = atomic_inc_return(&dev->ieee802154_ptr->dsn) & 0xFF;
 
        if (mac802154_set_header_security(sdata, &hdr, cb) < 0)
                return -EINVAL;
@@ -464,13 +461,16 @@ ieee802154_setup_sdata(struct ieee802154_sub_if_data *sdata,
                       enum nl802154_iftype type)
 {
        struct wpan_dev *wpan_dev = &sdata->wpan_dev;
+       u8 tmp;
 
        /* set some type-dependent values */
        sdata->vif.type = type;
        sdata->wpan_dev.iftype = type;
 
-       get_random_bytes(&wpan_dev->bsn, 1);
-       get_random_bytes(&wpan_dev->dsn, 1);
+       get_random_bytes(&tmp, 1);
+       atomic_set(&wpan_dev->bsn, tmp);
+       get_random_bytes(&tmp, 1);
+       atomic_set(&wpan_dev->dsn, tmp);
 
        /* defaults per 802.15.4-2011 */
        wpan_dev->min_be = 3;

- Alex

Re: [PATCH bluetooth-next 3/3] mac802154: remove mib lock

[Phoebe Buckheister]

On Tue, 12 May 2015 14:22:56 +0200
Alexander Aring <alex.aring@gmail.com> wrote:

> On Tue, May 12, 2015 at 01:52:58PM +0200, Alexander Aring wrote:
> > Hi,
> > 
> > ...
> > > > 
> > > > Introduce a percpu counter for the sequence numbers,
> > > > incrementation of this counter is an atomic operation then and
> > > > we are sure that we don't sending the same sequence number when
> > > > calling this function at the same time.
> > > 
> > > With this, two threads running on the same interface can send
> > > different packets with the same sequence number back to back.
> > > Maybe better make it atomic instead of percpu instead to avoid
> > > that?
> > > 
> > 
> > Yes you are right, that's not correct. Because per_cpu is a local
> > variable what's the name said _per_ _cpu_. For this kind of very
> > global mib value which needs to be incremented after each transmit
> > a atomic_t should be correct here and that's also what the comment
> > said.
> > 
> > Damn, why I thought that a percpu variable should be correct here.
> > 
> 
> So I updated the draft with this, I hope that is more correct than the
> previous one, which really makes no sense:
> 
> diff --git a/include/net/cfg802154.h b/include/net/cfg802154.h
> index c6aa1d2..4de59aa 100644
> --- a/include/net/cfg802154.h
> +++ b/include/net/cfg802154.h
> @@ -177,9 +177,9 @@ struct wpan_dev {
>         __le64 extended_addr;
>  
>         /* MAC BSN field */
> -       u8 bsn;
> +       atomic_t bsn;
>         /* MAC DSN field */
> -       u8 dsn;
> +       atomic_t dsn;
>  
>         u8 min_be;
>         u8 max_be;
> diff --git a/net/mac802154/iface.c b/net/mac802154/iface.c
> index 866d27f..b99a6f6 100644
> --- a/net/mac802154/iface.c
> +++ b/net/mac802154/iface.c
> @@ -365,10 +365,7 @@ static int mac802154_header_create(struct
> sk_buff *skb, hdr.fc.type = cb->type;
>         hdr.fc.security_enabled = cb->secen;
>         hdr.fc.ack_request = cb->ackreq;
> -       /* TODO: use atomic_t as dsn, dsn need to be locked when
> AF_IEEE802154
> -        * and IEEE802154 6LoWPAN call this at the same time.
> -        */
> -       hdr.seq = dev->ieee802154_ptr->dsn++;
> +       hdr.seq = atomic_inc_return(&dev->ieee802154_ptr->dsn) & 0xFF;
>  
>         if (mac802154_set_header_security(sdata, &hdr, cb) < 0)
>                 return -EINVAL;
> @@ -464,13 +461,16 @@ ieee802154_setup_sdata(struct
> ieee802154_sub_if_data *sdata, enum nl802154_iftype type)
>  {
>         struct wpan_dev *wpan_dev = &sdata->wpan_dev;
> +       u8 tmp;
>  
>         /* set some type-dependent values */
>         sdata->vif.type = type;
>         sdata->wpan_dev.iftype = type;
>  
> -       get_random_bytes(&wpan_dev->bsn, 1);
> -       get_random_bytes(&wpan_dev->dsn, 1);
> +       get_random_bytes(&tmp, 1);
> +       atomic_set(&wpan_dev->bsn, tmp);
> +       get_random_bytes(&tmp, 1);
> +       atomic_set(&wpan_dev->dsn, tmp);
>  
>         /* defaults per 802.15.4-2011 */
>         wpan_dev->min_be = 3;
> 
> - Alex

LGTM