mac802154: remove pib/mib locks and locking fixes

PATCH Superseded Alexander Aring 3 participants 17 messages May 22, 2015

Subsystems (2)

Networking IEEE 802.15.4

lore

Share & tools

lore

Alexander Aring

author maintainer

2015-05-22 08:57 UTC #0 lore raw

cover mac802154: remove pib/mib locks and locking fixes

Hi,

this patch series contains patches for remove pib and mib lock. We have now a other locking strategie inside this stack which requires at the moment the rtnl lock only. The first patch will hold the rtnl lock which is required to access the mib attributes while calling socket ioctl. The last patch will use atomic operations for incremet the dsn value for dataframes, currently it could be that this value is the same in two contiguous frames which could be occur a wrong ack frame handling in ARET mode.

- Alex

Alexander Aring (4):
mac802154: fix hold rtnl while ioctl
mac802154: remove pib lock
mac802154: remove mib lock
mac802154: use atomic ops for sequence incrementation

 include/net/cfg802154.h         |  6 ++--
 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    |  7 -----
 net/mac802154/iface.c           | 29 ++++++-------------
 net/mac802154/mac_cmd.c         |  7 ++---
 net/mac802154/mib.c             | 63 ++---------------------------------------
 net/mac802154/rx.c              |  5 ----
 13 files changed, 34 insertions(+), 157 deletions(-)

Signature · 1 line

2.4.1

Alexander Aring

author maintainer

2015-05-22 08:57 UTC #1 lore raw

PATCH 1/4 mac802154: fix hold rtnl while ioctl

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 <[email protected]>

Reported-by Matteo Petracca <[email protected]>

---
 net/mac802154/iface.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

net/mac802154/iface.c +3 -2

index 91b75ab..2a58788 100644
@@ -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;
 }

Signature · 1 line

2.4.1

Stefan Schmidt

Reviewed-by on v1 1/4

2015-05-22 12:15 UTC #2 lore raw

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

Hello.

Quoted 34 lines · 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 <[email protected]>
Reported-by Matteo Petracca <[email protected]>
---
  net/mac802154/iface.c | 5 +++--
  1 file changed, 3 insertions(+), 2 deletions(-)
net/mac802154/iface.c +2 -2
index 91b75ab..2a58788 100644
@@ -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;
  		}
 

net/mac802154/iface.c mac802154_wpan_ioctl(struct net_device *dev, struct ifreq *ifr, int cmd) · line 118

Quoted 7 lines

net/mac802154/iface.c +1 -0

@@ -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;
  }

Reviewed-by Stefan Schmidt <[email protected]>

This includes some smoke testing between two nodes (atusb and at86rf233), Basically ping6 with various sizes and a count of 1000 pings. No problem showed up with these patches during the testing.

regards
Stefan Schmidt

Alexander Aring

author maintainer

2015-05-22 08:57 UTC #3 lore raw

PATCH 2/4 mac802154: remove pib lock

In reply to 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 <[email protected]>

---
 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(-)

include/net/cfg802154.h +0 -2

index 11bbf17..c6aa1d2 100644
@@ -121,8 +121,6 @@ enum wpan_phy_flags {
 };
 
 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

net/ieee802154/core.c +0 -2

index 2ee00e8..b0248e9 100644
@@ -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);

net/ieee802154/nl-phy.c +3 -3

index cbc0d09..77d7301 100644
@@ -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);

net/mac802154/iface.c +0 -7

index 2a58788..22f478b 100644
@@ -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;
 }

net/mac802154/mib.c +2 -2

index 5cf019a..033dfc7 100644
@@ -91,16 +91,16 @@ void mac802154_dev_set_page_channel(struct net_device *dev, u8 page, u8 chan)
 	struct ieee802154_local *local = sdata->local;
 	int res;
 
+	ASSERT_RTNL();
+
 	BUG_ON(dev->type != ARPHRD_IEEE802154);
 
 	res = drv_set_channel(local, page, chan);
 	if (res) {
 		pr_debug("set_channel failed\n");
 	} else {
-		mutex_lock(&local->phy->pib_lock);
 		local->phy->current_channel = chan;
 		local->phy->current_page = page;
-		mutex_unlock(&local->phy->pib_lock);
 	}
 }

Signature · 1 line

2.4.1

Stefan Schmidt

2015-05-22 12:19 UTC #4 lore raw

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

Hello.

Quoted 90 lines · Alexander Aring

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

---
  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(-)

include/net/cfg802154.h +0 -2

index 11bbf17..c6aa1d2 100644
@@ -121,8 +121,6 @@ enum wpan_phy_flags {
  };
 
  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

net/ieee802154/core.c +0 -2

index 2ee00e8..b0248e9 100644
@@ -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);

net/ieee802154/nl-phy.c +3 -3

index cbc0d09..77d7301 100644
@@ -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);

net/mac802154/iface.c +0 -3

index 2a58788..22f478b 100644
@@ -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);

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

Quoted 5 lines

net/mac802154/iface.c +0 -2

@@ -295,11 +292,7 @@ static int mac802154_wpan_open(struct net_device *dev)
  			goto out;
  	}
 
-	mutex_unlock(&phy->pib_lock);
-	return 0;

Hmm, why did you remove the return 0; here? Is this supposed to fall through to out: and use the return rc now?

Quoted 18 lines

net/mac802154/iface.c +0 -1

  out:
-	mutex_unlock(&phy->pib_lock);
  	return rc;
  }

net/mac802154/mib.c +2 -0

index 5cf019a..033dfc7 100644
@@ -91,16 +91,16 @@ void mac802154_dev_set_page_channel(struct net_device *dev, u8 page, u8 chan)
  	struct ieee802154_local *local = sdata->local;
  	int res;
 
+	ASSERT_RTNL();
+
  	BUG_ON(dev->type != ARPHRD_IEEE802154);
 
  	res = drv_set_channel(local, page, chan);

net/mac802154/mib.c void mac802154_dev_set_page_channel(struct net_device *dev, u8 page, u8 chan) · line 106

Quoted 10 lines

net/mac802154/mib.c +0 -2

  	if (res) {
  		pr_debug("set_channel failed\n");
  	} else {
-		mutex_lock(&local->phy->pib_lock);
  		local->phy->current_channel = chan;
  		local->phy->current_page = page;
-		mutex_unlock(&local->phy->pib_lock);
  	}
  }

regards
Stefan Schmidt

Alexander Aring

author maintainer

2015-05-22 12:41 UTC #5 lore raw

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

On Fri, May 22, 2015 at 02:19:05PM +0200, Stefan Schmidt wrote:

...

Quoted 12 lines

net/mac802154/iface.c static int mac802154_wpan_open(struct net_device *dev) · line 298
Quoted 8 lines
net/mac802154/iface.c +0 -2
 	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;
Hmm, why did you remove the return 0; here? Is this supposed to fall through to out: and use the return rc now?

This function is the netdev open call of an wpan interface. In this call we do at the moment all MAC dependent settings which are done by phy. e.g. address filtering, frame reties, etc...

When this call failed the interface is still down. This callback is usually called on an ifup. If it's successful it's up, if not still down.

The last call is:

        if (local->hw.flags & IEEE802154_HW_FRAME_RETRIES) {
                rc = drv_set_max_frame_retries(local, wpan_dev->frame_retries);
                if (rc < 0)
                        goto out;
        }

Which is the driver callback which have an identically return value indicator, means -ERRNO if failed, according to the netdev open call.

We can still use the return 0 on successful or the rc. It's a silent cleanup which I did here (hope that was okay).

btw:
Also I don't know why there is a pib hold, when some of them are mib attributes. This all makes no sense for me.

To hold the pib_lock will represent the documentation of driver_ops, but this makes no sense when you hold the pib lock for mib attributes in this case of callback and when accessing mib attributes over netlink or such else to hold the mib lock and not the pib lock, something is wrong there.

Now, we doing everything over rtnl, which can be indicated by [0] flag to hold this lock while netlink command and the _most_ mib attributes are readonly while interface up. This will occur that we need don't care about locking if the interface is up. For attributes like address filtering this is also "impossible" to set address filtering registers while the interface is running. The best option here is to set address filtering while ifup and then don't allo the change the addresses while having the interface up. This is what we have now.

The dsn/bsn values are special here, this need to be writeable while ifup. Also we need to think about handling setting of short_address while assoc with coordinators, but I think this will take some time to support such feature. I am sure we will find some solution.

- Alex

[0] http://lxr.free-electrons.com/source/net/ieee802154/nl802154.c#L799

Stefan Schmidt

Reviewed-by

2015-05-22 12:48 UTC #6 lore raw

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

Hello.

Quoted 30 lines · Alexander Aring

On Fri, May 22, 2015 at 02:19:05PM +0200, Stefan Schmidt wrote:
...
Quoted 11 lines
net/mac802154/iface.c static int mac802154_wpan_open(struct net_device *dev) · line 298
Quoted 8 lines
  	if (local->hw.flags & IEEE802154_HW_PROMISCUOUS) {
  		rc = drv_set_promiscuous_mode(local,
  					      wpan_dev->promiscuous_mode);
net/mac802154/iface.c +0 -2
@@ -295,11 +292,7 @@ static int mac802154_wpan_open(struct net_device *dev)
  			goto out;
  	}
-	mutex_unlock(&phy->pib_lock);
-	return 0;
Hmm, why did you remove the return 0; here? Is this supposed to fall through to out: and use the return rc now?
This function is the netdev open call of an wpan interface. In this call we do at the moment all MAC dependent settings which are done by phy. e.g. address filtering, frame reties, etc...
When this call failed the interface is still down. This callback is usually called on an ifup. If it's successful it's up, if not still down.
The last call is:
         if (local->hw.flags & IEEE802154_HW_FRAME_RETRIES) {
                 rc = drv_set_max_frame_retries(local, wpan_dev->frame_retries);
                 if (rc < 0)
                         goto out;
         }

Quoted 6 lines

Which is the driver callback which have an identically return value indicator, means -ERRNO if failed, according to the netdev open call.
We can still use the return 0 on successful or the rc. It's a silent cleanup which I did here (hope that was okay).

OK, so this change does not change the return value in any condition. That was the part I was worried about.

A silent cleanup is fine but it made me suspicious during the review. :)

Quoted 21 lines

btw:
Also I don't know why there is a pib hold, when some of them are mib attributes. This all makes no sense for me.
To hold the pib_lock will represent the documentation of driver_ops, but this makes no sense when you hold the pib lock for mib attributes in this case of callback and when accessing mib attributes over netlink or such else to hold the mib lock and not the pib lock, something is wrong there.
Now, we doing everything over rtnl, which can be indicated by [0] flag to hold this lock while netlink command and the _most_ mib attributes are readonly while interface up. This will occur that we need don't care about locking if the interface is up. For attributes like address filtering this is also "impossible" to set address filtering registers while the interface is running. The best option here is to set address filtering while ifup and then don't allo the change the addresses while having the interface up. This is what we have now.

Quoted 4 lines

The dsn/bsn values are special here, this need to be writeable while ifup. Also we need to think about handling setting of short_address while assoc with coordinators, but I think this will take some time to support such feature. I am sure we will find some solution.

Yeah, we need to revive the coordinator functionality at all for this to be a problem. :)

With the above explained you can add my:

Reviewed-by Stefan Schmidt <[email protected]>

regards
Stefan Schmidt

Alexander Aring

author maintainer

2015-05-22 08:57 UTC #7 lore raw

PATCH 3/4 mac802154: remove mib lock

In reply to 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 <[email protected]>

---
 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    |  7 -----
 net/mac802154/iface.c           | 13 +++------
 net/mac802154/mac_cmd.c         |  7 ++---
 net/mac802154/mib.c             | 59 -----------------------------------------
 net/mac802154/rx.c              |  5 ----
 10 files changed, 22 insertions(+), 135 deletions(-)

include/net/ieee802154_netdev.h +0 ~~-10~~

index 94a2970..84a72a1 100644
@@ -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 *

net/ieee802154/6lowpan/core.c +0 ~~-28~~

index 0ae5822..f20a387 100644
@@ -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;
 }

net/ieee802154/6lowpan/tx.c +1 -1

index 2349070..98acf73 100644
@@ -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 */

net/ieee802154/nl-mac.c +10 -4

index cdc1cc3..ada58a8 100644
@@ -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]));

net/ieee802154/socket.c +5 -7

index 7aaaf96..e5cc253 100644
@@ -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) {

net/mac802154/ieee802154_i.h +0 -7

index 127ba18..eec668f 100644
@@ -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,12 +134,7 @@ 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);

net/mac802154/iface.c +4 -9

index 22f478b..866d27f 100644
@@ -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);

net/mac802154/mac_cmd.c +2 -5

index 6dcbb3b..5220c2b2 100644
@@ -43,8 +43,8 @@ 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);
+	dev->ieee802154_ptr->pan_id = addr->pan_id;
+	dev->ieee802154_ptr->short_addr = addr->short_addr;
 	mac802154_dev_set_page_channel(dev, page, channel);
 
 	if (ops->llsec) {

@@ -151,9 +151,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,

net/mac802154/mib.c +0 ~~-59~~

65 lines not on this page · raw message

net/mac802154/rx.c +0 -5

index c0d67b2..e0f1006 100644
@@ -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);

Signature · 1 line

2.4.1

Stefan Schmidt

Reviewed-by on v1 3/4

2015-05-22 12:25 UTC #8 lore raw

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

Hello.

Quoted 234 lines · Alexander Aring

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

---
  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    |  7 -----
  net/mac802154/iface.c           | 13 +++------
  net/mac802154/mac_cmd.c         |  7 ++---
  net/mac802154/mib.c             | 59 -----------------------------------------
  net/mac802154/rx.c              |  5 ----
  10 files changed, 22 insertions(+), 135 deletions(-)

include/net/ieee802154_netdev.h +0 ~~-10~~

index 94a2970..84a72a1 100644
@@ -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 *

net/ieee802154/6lowpan/core.c +0 ~~-28~~

index 0ae5822..f20a387 100644
@@ -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;
  }

net/ieee802154/6lowpan/tx.c +1 -1

index 2349070..98acf73 100644
@@ -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 */

net/ieee802154/nl-mac.c +10 -4

index cdc1cc3..ada58a8 100644
@@ -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]));

net/ieee802154/socket.c +5 -7

index 7aaaf96..e5cc253 100644
@@ -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) {

net/mac802154/ieee802154_i.h +0 -7

index 127ba18..eec668f 100644
@@ -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,12 +134,7 @@ 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);

net/mac802154/iface.c +0 -3

index 22f478b..866d27f 100644
@@ -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;
  }

net/mac802154/iface.c static int mac802154_header_create(struct sk_buff *skb, · line 371

Quoted 8 lines

net/mac802154/iface.c +4 -1

@@ -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++;

OK, given this comment I think swapping 3 and 4 would really make sense.

Quoted 139 lines

net/mac802154/iface.c +0 -5

  	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);

net/mac802154/mac_cmd.c +2 -5

index 6dcbb3b..5220c2b2 100644
@@ -43,8 +43,8 @@ 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);
+	dev->ieee802154_ptr->pan_id = addr->pan_id;
+	dev->ieee802154_ptr->short_addr = addr->short_addr;
  	mac802154_dev_set_page_channel(dev, page, channel);
 
  	if (ops->llsec) {

@@ -151,9 +151,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,

net/mac802154/mib.c +0 ~~-59~~

index 033dfc7..73f94fb 100644
@@ -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);

net/mac802154/rx.c +0 -2

index c0d67b2..e0f1006 100644
@@ -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:

net/mac802154/rx.c ieee802154_subif_frame(struct ieee802154_sub_if_data *sdata, · line 90

Quoted 10 lines

net/mac802154/rx.c +0 -3

-		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);

After swapping 3 and for you can add:

Reviewed-by Stefan Schmidt <[email protected]>

This includes some smoke testing between two nodes (atusb and at86rf233), Basically ping6 with various sizes and a count of 1000 pings. No problem showed up with these patches during the testing.

regards
Stefan Schmidt

Alexander Aring

author maintainer

2015-05-22 08:57 UTC #9 lore raw

PATCH 4/4 mac802154: use atomic ops for sequence incrementation

In reply to Alexander Aring

This patch will use atomic operations for sequence number incrementation while MAC header generation. Upper layers like af_802154 or 6LoWPAN could call this function in a parallel context while generating 802.15.4 MAC header before queuing into wpan interfaces transmit queue.

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

---
 include/net/cfg802154.h |  4 ++--
 net/mac802154/iface.c   | 12 ++++++------
 2 files changed, 8 insertions(+), 8 deletions(-)

include/net/cfg802154.h +2 -2

index c6aa1d2..4de59aa 100644
@@ -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;

net/mac802154/iface.c +6 -6

index 866d27f..f30788d 100644
@@ -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, sizeof(tmp));
+	atomic_set(&wpan_dev->bsn, tmp);
+	get_random_bytes(&tmp, sizeof(tmp));
+	atomic_set(&wpan_dev->dsn, tmp);
 
 	/* defaults per 802.15.4-2011 */
 	wpan_dev->min_be = 3;

Signature · 1 line

2.4.1

Marc Kleine-Budde

2015-05-22 08:59 UTC #10 lore raw

Re: [PATCH bluetooth-next 4/4] mac802154: use atomic ops for sequence incrementation

Quoted 4 lines · Alexander Aring

This patch will use atomic operations for sequence number incrementation while MAC header generation. Upper layers like af_802154 or 6LoWPAN could call this function in a parallel context while generating 802.15.4 MAC header before queuing into wpan interfaces transmit queue.

what about swapping patch 3 and 4?

Marc

Signature · 4 lines

Pengutronix e.K.                  | Marc Kleine-Budde           |
Industrial Linux Solutions        | Phone: +49-231-2826-924     |
Vertretung West/Dortmund          | Fax:   +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686  | http://www.pengutronix.de   |

Stefan Schmidt

2015-05-22 10:31 UTC #11 lore raw

Re: [PATCH bluetooth-next 4/4] mac802154: use atomic ops for sequence incrementation

Hello.

Quoted 6 lines · Marc Kleine-Budde

Quoted 4 lines · Alexander Aring
This patch will use atomic operations for sequence number incrementation while MAC header generation. Upper layers like af_802154 or 6LoWPAN could call this function in a parallel context while generating 802.15.4 MAC header before queuing into wpan interfaces transmit queue.
what about swapping patch 3 and 4?

To avoid having problems during a git bisect later one? E.g. having the lock removed but no atomic in place?

regards
Stefan Schmidt

Marc Kleine-Budde

2015-05-22 10:34 UTC #12 lore raw

Re: [PATCH bluetooth-next 4/4] mac802154: use atomic ops for sequence incrementation

Quoted 4 lines · Stefan Schmidt

Hello.
Quoted 1 line · Marc Kleine-Budde
On 05/22/2015 10:57 AM, Alexander Aring wrote:

Quoted 8 lines

Quoted 5 lines
Quoted 4 lines
This patch will use atomic operations for sequence number incrementation while MAC header generation. Upper layers like af_802154 or 6LoWPAN could call this function in a parallel context while generating 802.15.4 MAC header before queuing into wpan interfaces transmit queue.
what about swapping patch 3 and 4?
To avoid having problems during a git bisect later one? E.g. having the lock removed but no atomic in place?

Yes, that's what I was thinking about. I don't know the code to tell if this is an issue here.

Marc

Signature · 4 lines

Pengutronix e.K.                  | Marc Kleine-Budde           |
Industrial Linux Solutions        | Phone: +49-231-2826-924     |
Vertretung West/Dortmund          | Fax:   +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686  | http://www.pengutronix.de   |

Stefan Schmidt

2015-05-22 10:40 UTC #13 lore raw

Re: [PATCH bluetooth-next 4/4] mac802154: use atomic ops for sequence incrementation

Hello.

Quoted 9 lines · Marc Kleine-Budde

Quoted 8 lines · Stefan Schmidt
Hello.
Quoted 5 lines · Marc Kleine-Budde
Quoted 4 lines · Alexander Aring
This patch will use atomic operations for sequence number incrementation while MAC header generation. Upper layers like af_802154 or 6LoWPAN could call this function in a parallel context while generating 802.15.4 MAC header before queuing into wpan interfaces transmit queue.

Quoted 5 lines

Quoted 3 lines
Quoted 1 line
what about swapping patch 3 and 4?
To avoid having problems during a git bisect later one? E.g. having the lock removed but no atomic in place?
Yes, that's what I was thinking about. I don't know the code to tell if this is an issue here.

A good point. I'm not sure either. I would say we could play safe and swap them. Alex, guess its your final call as you know the code best from us. Could we have problems with the locks removed but the atomic patch not applied (as it could happen with git bisect right now)? If yes, swapping them makes sense to avoid this.

Btw, I'm running some light testing with 6lowpan pings with these four patches applied right now. Looks good so far. Will send another mail once its done.

regards
Stefan Schmidt

Alexander Aring

author maintainer

2015-05-22 12:12 UTC #14 lore raw

Re: [PATCH bluetooth-next 4/4] mac802154: use atomic ops for sequence incrementation

In reply to Marc Kleine-Budde

Quoted 9 lines · Marc Kleine-Budde

Quoted 8 lines · Stefan Schmidt
Hello.
Quoted 5 lines · Marc Kleine-Budde
Quoted 4 lines · Alexander Aring
This patch will use atomic operations for sequence number incrementation while MAC header generation. Upper layers like af_802154 or 6LoWPAN could call this function in a parallel context while generating 802.15.4 MAC header before queuing into wpan interfaces transmit queue.

Quoted 8 lines

Quoted 4 lines
Quoted 1 line
what about swapping patch 3 and 4?
To avoid having problems during a git bisect later one? E.g. having the lock removed but no atomic in place?
Yes, that's what I was thinking about. I don't know the code to tell if this is an issue here.

The problem is more difficult because the dsn incrementation which I do atomic now had never a locking mechanism. So this was always not working correctly. Somebody need to scream now "hey fix that in net, not next". I do at the moment only critical things fixed in net, like [0].

But we should swap it because I first wrote some "TODO we should use atomic here" and then later I decide to implement this TODO. At the end this results in some cherry-pick orgy and I did not change it.

- Alex

[0] ("mac802154: tx: fix sync xmit handling") can't find them in the archive

Stefan Schmidt

2015-05-22 12:33 UTC #15 lore raw

Re: [PATCH bluetooth-next 4/4] mac802154: use atomic ops for sequence incrementation

Hello.

Quoted 11 lines · Alexander Aring

Quoted 10 lines · Marc Kleine-Budde
Quoted 9 lines · Stefan Schmidt
Hello.
Quoted 6 lines · Marc Kleine-Budde
Quoted 4 lines · Alexander Aring
This patch will use atomic operations for sequence number incrementation while MAC header generation. Upper layers like af_802154 or 6LoWPAN could call this function in a parallel context while generating 802.15.4 MAC header before queuing into wpan interfaces transmit queue.
what about swapping patch 3 and 4?

Quoted 8 lines

Quoted 5 lines
Quoted 2 lines
To avoid having problems during a git bisect later one? E.g. having the lock removed but no atomic in place?
Yes, that's what I was thinking about. I don't know the code to tell if this is an issue here.
The problem is more difficult because the dsn incrementation which I do atomic now had never a locking mechanism. So this was always not working correctly. Somebody need to scream now "hey fix that in net, not next".

Hey, fix that in net not next! :P

If we know it causes problems in current net we should indeed try to submit it there.
Given it is only a few lines it might be worth the effort even now. Some people might be using kernel releases for their work and thus they could get this fix one release (3 months) earlier.

Quoted 2 lines

I do at the moment only critical things fixed in net, like [0].

Sure, these are the most important ones. I leave it to you to decide if you think this one is worth the extra effort.

Quoted 4 lines

But we should swap it because I first wrote some "TODO we should use atomic here" and then later I decide to implement this TODO. At the end this results in some cherry-pick orgy and I did not change it.

ok, cool.

regards
Stefan Schmidt

Stefan Schmidt

Reviewed-by on v1 4/4

2015-05-22 12:27 UTC #16 lore raw

Re: [PATCH bluetooth-next 4/4] mac802154: use atomic ops for sequence incrementation

In reply to Alexander Aring

Hello.

Quoted 32 lines · Alexander Aring

This patch will use atomic operations for sequence number incrementation while MAC header generation. Upper layers like af_802154 or 6LoWPAN could call this function in a parallel context while generating 802.15.4 MAC header before queuing into wpan interfaces transmit queue.
Signed-off-by Alexander Aring <[email protected]>
---
  include/net/cfg802154.h |  4 ++--
  net/mac802154/iface.c   | 12 ++++++------
  2 files changed, 8 insertions(+), 8 deletions(-)
include/net/cfg802154.h +2 -2
index c6aa1d2..4de59aa 100644
@@ -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;
net/mac802154/iface.c +0 -0
index 866d27f..f30788d 100644

net/mac802154/iface.c static int mac802154_header_create(struct sk_buff *skb, · line 368

Quoted 8 lines

net/mac802154/iface.c +1 -4

@@ -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;

After reviewing this in its full glory I also think the order needs to change an patch number 4 should come before 3.

Quoted 12 lines

net/mac802154/iface.c +1 -0

  	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;

net/mac802154/iface.c ieee802154_setup_sdata(struct ieee802154_sub_if_data *sdata, · line 476

Quoted 10 lines

net/mac802154/iface.c +4 -2

 
-	get_random_bytes(&wpan_dev->bsn, 1);
-	get_random_bytes(&wpan_dev->dsn, 1);
+	get_random_bytes(&tmp, sizeof(tmp));
+	atomic_set(&wpan_dev->bsn, tmp);
+	get_random_bytes(&tmp, sizeof(tmp));
+	atomic_set(&wpan_dev->dsn, tmp);
 
  	/* defaults per 802.15.4-2011 */
  	wpan_dev->min_be = 3;

After swapping you can add:

Reviewed-by Stefan Schmidt <[email protected]>

This includes some smoke testing between two nodes (atusb and at86rf233), Basically ping6 with various sizes and a count of 1000 pings. No problem showed up with these patches during the testing.

regards
Stefan Schmidt