From: Stefan Schmidt <stefan@osg.samsung.com>
To: Alexander Aring <alex.aring@gmail.com>, linux-wpan@vger.kernel.org
Cc: kernel@pengutronix.de
Subject: Re: [PATCH bluetooth-next 3/4] mac802154: remove mib lock
Date: Fri, 22 May 2015 14:25:12 +0200 [thread overview]
Message-ID: <555F2028.1080808@osg.samsung.com> (raw)
In-Reply-To: <1432285031-3360-4-git-send-email-alex.aring@gmail.com>
Hello.
On 22/05/15 10:57, Alexander Aring wrote:
> 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 | 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(-)
>
> 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 cdc1cc3..ada58a8 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 7aaaf96..e5cc253 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..eec668f 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,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);
> 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++;
OK, given this comment I think swapping 3 and 4 would really make sense.
>
> 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 6dcbb3b..5220c2b2 100644
> --- a/net/mac802154/mac_cmd.c
> +++ b/net/mac802154/mac_cmd.c
> @@ -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,
>
> diff --git a/net/mac802154/mib.c b/net/mac802154/mib.c
> index 033dfc7..73f94fb 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);
After swapping 3 and for you can add:
Reviewed-by: Stefan Schmidt <stefan@osg.samsung.com>
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
next prev parent reply other threads:[~2015-05-22 12:25 UTC|newest]
Thread overview: 17+ messages / expand[flat|nested] mbox.gz Atom feed top
2015-05-22 8:57 [PATCH bluetooth-next 0/4] mac802154: remove pib/mib locks and locking fixes Alexander Aring
2015-05-22 8:57 ` [PATCH bluetooth-next 1/4] mac802154: fix hold rtnl while ioctl Alexander Aring
2015-05-22 12:15 ` Stefan Schmidt
2015-05-22 8:57 ` [PATCH bluetooth-next 2/4] mac802154: remove pib lock Alexander Aring
2015-05-22 12:19 ` Stefan Schmidt
2015-05-22 12:41 ` Alexander Aring
2015-05-22 12:48 ` Stefan Schmidt
2015-05-22 8:57 ` [PATCH bluetooth-next 3/4] mac802154: remove mib lock Alexander Aring
2015-05-22 12:25 ` Stefan Schmidt [this message]
2015-05-22 8:57 ` [PATCH bluetooth-next 4/4] mac802154: use atomic ops for sequence incrementation Alexander Aring
2015-05-22 8:59 ` Marc Kleine-Budde
2015-05-22 10:30 ` Stefan Schmidt
2015-05-22 10:34 ` Marc Kleine-Budde
2015-05-22 10:40 ` Stefan Schmidt
2015-05-22 12:12 ` Alexander Aring
2015-05-22 12:33 ` Stefan Schmidt
2015-05-22 12:27 ` Stefan Schmidt
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=555F2028.1080808@osg.samsung.com \
--to=stefan@osg.samsung.com \
--cc=alex.aring@gmail.com \
--cc=kernel@pengutronix.de \
--cc=linux-wpan@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox