[PATCH wpan-next v3 0/4] net: ieee802154: PAN management

[PATCH wpan-next v3 0/4] net: ieee802154: PAN management

[Miquel Raynal]

Hello,

Last step before adding scan support, we need to introduce a proper PAN
description (with its main properties) and PAN management helpers.

This series provides generic code to do simple operations on the list of
surrounding PANs, such as registering, listing, flushing... The scanning
code will soon make use of these additions.

Thanks,
Miquèl

Changes in v3:
* Dropped the device type (FFD, RFD, etc) enumeration, all the checks
  that were related to this parameter and of course the additional user
  attribute which allowed to check for that value.
* Reworded a few commit messages.

Changes in v2:
* The main change is related to the use of the COORD interface (instead
  of dropping it). Most of the actual diff is in the following series.

David Girault (1):
  net: ieee802154: Trace the registration of new PANs

Miquel Raynal (3):
  net: mac802154: Allow the creation of coordinator interfaces
  net: ieee802154: Add support for inter PAN management
  net: ieee802154: Give the user access to list of surrounding PANs

 include/net/cfg802154.h   |  31 +++++
 include/net/nl802154.h    |  49 ++++++++
 net/ieee802154/Makefile   |   2 +-
 net/ieee802154/core.c     |   2 +
 net/ieee802154/core.h     |  26 +++++
 net/ieee802154/nl802154.c | 199 +++++++++++++++++++++++++++++++-
 net/ieee802154/pan.c      | 234 ++++++++++++++++++++++++++++++++++++++
 net/ieee802154/trace.h    |  25 ++++
 net/mac802154/iface.c     |  14 ++-
 net/mac802154/rx.c        |   2 +-
 10 files changed, 574 insertions(+), 10 deletions(-)
 create mode 100644 net/ieee802154/pan.c

-- 
2.34.1

[PATCH wpan-next v3 1/4] net: mac802154: Allow the creation of coordinator interfaces

[Miquel Raynal]

As a first strep in introducing proper PAN management and association,
we need to be able to create coordinator interfaces which might act as
coordinator or PAN coordinator.

Hence, let's add the minimum support to allow the creation of these
interfaces. This might be restrained and improved later.

Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
 net/mac802154/iface.c | 14 ++++++++------
 net/mac802154/rx.c    |  2 +-
 2 files changed, 9 insertions(+), 7 deletions(-)

diff --git a/net/mac802154/iface.c b/net/mac802154/iface.c
index 500ed1b81250..7ac0c5685d3f 100644
--- a/net/mac802154/iface.c
+++ b/net/mac802154/iface.c
@@ -273,13 +273,13 @@ ieee802154_check_concurrent_iface(struct ieee802154_sub_if_data *sdata,
 		if (nsdata != sdata && ieee802154_sdata_running(nsdata)) {
 			int ret;
 
-			/* TODO currently we don't support multiple node types
-			 * we need to run skb_clone at rx path. Check if there
-			 * exist really an use case if we need to support
-			 * multiple node types at the same time.
+			/* TODO currently we don't support multiple node/coord
+			 * types we need to run skb_clone at rx path. Check if
+			 * there exist really an use case if we need to support
+			 * multiple node/coord types at the same time.
 			 */
-			if (wpan_dev->iftype == NL802154_IFTYPE_NODE &&
-			    nsdata->wpan_dev.iftype == NL802154_IFTYPE_NODE)
+			if (wpan_dev->iftype != NL802154_IFTYPE_MONITOR &&
+			    nsdata->wpan_dev.iftype != NL802154_IFTYPE_MONITOR)
 				return -EBUSY;
 
 			/* check all phy mac sublayer settings are the same.
@@ -577,6 +577,7 @@ ieee802154_setup_sdata(struct ieee802154_sub_if_data *sdata,
 	wpan_dev->short_addr = cpu_to_le16(IEEE802154_ADDR_BROADCAST);
 
 	switch (type) {
+	case NL802154_IFTYPE_COORD:
 	case NL802154_IFTYPE_NODE:
 		ieee802154_be64_to_le64(&wpan_dev->extended_addr,
 					sdata->dev->dev_addr);
@@ -636,6 +637,7 @@ ieee802154_if_add(struct ieee802154_local *local, const char *name,
 	ieee802154_le64_to_be64(ndev->perm_addr,
 				&local->hw.phy->perm_extended_addr);
 	switch (type) {
+	case NL802154_IFTYPE_COORD:
 	case NL802154_IFTYPE_NODE:
 		ndev->type = ARPHRD_IEEE802154;
 		if (ieee802154_is_valid_extended_unicast_addr(extended_addr)) {
diff --git a/net/mac802154/rx.c b/net/mac802154/rx.c
index b8ce84618a55..39459d8d787a 100644
--- a/net/mac802154/rx.c
+++ b/net/mac802154/rx.c
@@ -203,7 +203,7 @@ __ieee802154_rx_handle_packet(struct ieee802154_local *local,
 	}
 
 	list_for_each_entry_rcu(sdata, &local->interfaces, list) {
-		if (sdata->wpan_dev.iftype != NL802154_IFTYPE_NODE)
+		if (sdata->wpan_dev.iftype == NL802154_IFTYPE_MONITOR)
 			continue;
 
 		if (!ieee802154_sdata_running(sdata))
-- 
2.34.1

[PATCH wpan-next v3 2/4] net: ieee802154: Add support for inter PAN management

[Miquel Raynal]

Let's introduce the basics for defining PANs:
- structures defining a PAN
- helpers for PAN registration
- helpers discarding old PANs

Co-developed-by: David Girault <david.girault@qorvo.com>
Signed-off-by: David Girault <david.girault@qorvo.com>
Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
 include/net/cfg802154.h |  31 ++++++
 net/ieee802154/Makefile |   2 +-
 net/ieee802154/core.c   |   2 +
 net/ieee802154/core.h   |  26 +++++
 net/ieee802154/pan.c    | 231 ++++++++++++++++++++++++++++++++++++++++
 5 files changed, 291 insertions(+), 1 deletion(-)
 create mode 100644 net/ieee802154/pan.c

diff --git a/include/net/cfg802154.h b/include/net/cfg802154.h
index 04b996895fc1..9838eca3c41e 100644
--- a/include/net/cfg802154.h
+++ b/include/net/cfg802154.h
@@ -255,6 +255,24 @@ struct ieee802154_addr {
 	};
 };
 
+/**
+ * struct ieee802154_pan_desc - PAN descriptor information
+ * @coord: PAN ID and coordinator address
+ * @page: page this PAN is on
+ * @channel: channel this PAN is on
+ * @superframe_spec: SuperFrame specification as received
+ * @link_quality: link quality indicator at which the beacon was received
+ * @gts_permit: the PAN coordinator accepts GTS requests
+ */
+struct ieee802154_pan_desc {
+	struct ieee802154_addr *coord;
+	u8 page;
+	u8 channel;
+	u16 superframe_spec;
+	u8 link_quality;
+	bool gts_permit;
+};
+
 struct ieee802154_llsec_key_id {
 	u8 mode;
 	u8 id;
@@ -426,4 +444,17 @@ static inline const char *wpan_phy_name(struct wpan_phy *phy)
 
 void ieee802154_configure_durations(struct wpan_phy *phy);
 
+/**
+ * cfg802154_record_pan - Advertize a new PAN following a beacon's reception
+ * @wpan_phy: PHY receiving the beacon
+ * @pan: PAN descriptor
+ *
+ * Tells the internal pan management layer to either register this PAN if it is
+ * new or at least update its entry if already discovered.
+ *
+ * Returns 0 on success, a negative error code otherwise.
+ */
+int cfg802154_record_pan(struct wpan_phy *wpan_phy,
+			 struct ieee802154_pan_desc *pan);
+
 #endif /* __NET_CFG802154_H */
diff --git a/net/ieee802154/Makefile b/net/ieee802154/Makefile
index f05b7bdae2aa..6b7c66de730d 100644
--- a/net/ieee802154/Makefile
+++ b/net/ieee802154/Makefile
@@ -4,7 +4,7 @@ obj-$(CONFIG_IEEE802154_SOCKET) += ieee802154_socket.o
 obj-y += 6lowpan/
 
 ieee802154-y := netlink.o nl-mac.o nl-phy.o nl_policy.o core.o \
-                header_ops.o sysfs.o nl802154.o trace.o
+                header_ops.o sysfs.o nl802154.o pan.o trace.o
 ieee802154_socket-y := socket.o
 
 CFLAGS_trace.o := -I$(src)
diff --git a/net/ieee802154/core.c b/net/ieee802154/core.c
index 57546e07e06a..f642db35d62e 100644
--- a/net/ieee802154/core.c
+++ b/net/ieee802154/core.c
@@ -115,6 +115,8 @@ wpan_phy_new(const struct cfg802154_ops *ops, size_t priv_size)
 		kfree(rdev);
 		return NULL;
 	}
+	spin_lock_init(&rdev->pan_lock);
+	INIT_LIST_HEAD(&rdev->pan_list);
 
 	/* atomic_inc_return makes it start at 1, make it start at 0 */
 	rdev->wpan_phy_idx--;
diff --git a/net/ieee802154/core.h b/net/ieee802154/core.h
index 1c19f575d574..cae071bede37 100644
--- a/net/ieee802154/core.h
+++ b/net/ieee802154/core.h
@@ -22,6 +22,14 @@ struct cfg802154_registered_device {
 	struct list_head wpan_dev_list;
 	int devlist_generation, wpan_dev_id;
 
+	/* PAN management */
+	spinlock_t pan_lock;
+	struct list_head pan_list;
+	unsigned int max_pan_entries;
+	unsigned int pan_expiration;
+	unsigned int pan_entries;
+	unsigned int pan_generation;
+
 	/* must be last because of the way we do wpan_phy_priv(),
 	 * and it should at least be aligned to NETDEV_ALIGN
 	 */
@@ -39,6 +47,17 @@ wpan_phy_to_rdev(struct wpan_phy *wpan_phy)
 extern struct list_head cfg802154_rdev_list;
 extern int cfg802154_rdev_list_generation;
 
+struct cfg802154_internal_pan {
+	struct list_head list;
+	unsigned long discovery_ts;
+	struct ieee802154_pan_desc desc;
+};
+
+/* Always update the list by dropping the expired PANs before iterating */
+#define ieee802154_for_each_pan(pan, rdev)				\
+	cfg802154_expire_pans(rdev);					\
+	list_for_each_entry((pan), &(rdev)->pan_list, list)
+
 int cfg802154_switch_netns(struct cfg802154_registered_device *rdev,
 			   struct net *net);
 /* free object */
@@ -47,4 +66,11 @@ struct cfg802154_registered_device *
 cfg802154_rdev_by_wpan_phy_idx(int wpan_phy_idx);
 struct wpan_phy *wpan_phy_idx_to_wpan_phy(int wpan_phy_idx);
 
+void cfg802154_set_max_pan_entries(struct cfg802154_registered_device *rdev,
+				   unsigned int max);
+void cfg802154_set_pans_expiration(struct cfg802154_registered_device *rdev,
+				   unsigned int exp_time_s);
+void cfg802154_expire_pans(struct cfg802154_registered_device *rdev);
+void cfg802154_flush_pans(struct cfg802154_registered_device *rdev);
+
 #endif /* __IEEE802154_CORE_H */
diff --git a/net/ieee802154/pan.c b/net/ieee802154/pan.c
new file mode 100644
index 000000000000..b9f50f785960
--- /dev/null
+++ b/net/ieee802154/pan.c
@@ -0,0 +1,231 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * IEEE 802.15.4 PAN management
+ *
+ * Copyright (C) Qorvo, 2021
+ * Authors:
+ *   - David Girault <david.girault@qorvo.com>
+ *   - Miquel Raynal <miquel.raynal@bootlin.com>
+ */
+
+#include <linux/slab.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/device.h>
+
+#include <net/cfg802154.h>
+#include <net/af_ieee802154.h>
+
+#include "ieee802154.h"
+#include "core.h"
+
+static struct cfg802154_internal_pan *
+cfg802154_alloc_pan(struct ieee802154_pan_desc *desc)
+{
+	struct cfg802154_internal_pan *new;
+	struct ieee802154_addr *coord;
+
+	new = kzalloc(sizeof(*new), GFP_ATOMIC);
+	if (!new)
+		return ERR_PTR(-ENOMEM);
+
+	coord = kzalloc(sizeof(*coord), GFP_ATOMIC);
+	if (!coord) {
+		kfree(new);
+		return ERR_PTR(-ENOMEM);
+	}
+
+	new->discovery_ts = jiffies;
+	new->desc = *desc;
+
+	*coord = *desc->coord;
+	new->desc.coord = coord;
+
+	return new;
+}
+
+static void cfg802154_free_pan(struct cfg802154_internal_pan *pan)
+{
+	kfree(pan->desc.coord);
+	kfree(pan);
+}
+
+static void cfg802154_unlink_pan(struct cfg802154_registered_device *rdev,
+				 struct cfg802154_internal_pan *pan)
+{
+	lockdep_assert_held(&rdev->pan_lock);
+
+	list_del(&pan->list);
+	cfg802154_free_pan(pan);
+	rdev->pan_entries--;
+	rdev->pan_generation++;
+}
+
+static void cfg802154_link_pan(struct cfg802154_registered_device *rdev,
+			       struct cfg802154_internal_pan *pan)
+{
+	lockdep_assert_held(&rdev->pan_lock);
+
+	list_add_tail(&pan->list, &rdev->pan_list);
+	rdev->pan_entries++;
+	rdev->pan_generation++;
+}
+
+void cfg802154_set_max_pan_entries(struct cfg802154_registered_device *rdev,
+				   unsigned int max)
+{
+	lockdep_assert_held(&rdev->pan_lock);
+
+	rdev->max_pan_entries = max;
+}
+EXPORT_SYMBOL(cfg802154_set_max_pan_entries);
+
+static bool
+cfg802154_need_to_expire_pans(struct cfg802154_registered_device *rdev)
+{
+	if (!rdev->max_pan_entries)
+		return false;
+
+	if (rdev->pan_entries > rdev->max_pan_entries)
+		return true;
+
+	return false;
+}
+
+void cfg802154_set_pans_expiration(struct cfg802154_registered_device *rdev,
+				   unsigned int exp_time_s)
+{
+	lockdep_assert_held(&rdev->pan_lock);
+
+	rdev->pan_expiration = exp_time_s * HZ;
+}
+EXPORT_SYMBOL(cfg802154_set_pans_expiration);
+
+void cfg802154_expire_pans(struct cfg802154_registered_device *rdev)
+{
+	struct cfg802154_internal_pan *pan, *tmp;
+	unsigned long expiration_time;
+
+	lockdep_assert_held(&rdev->pan_lock);
+
+	if (!rdev->pan_expiration)
+		return;
+
+	expiration_time = jiffies - rdev->pan_expiration;
+	list_for_each_entry_safe(pan, tmp, &rdev->pan_list, list) {
+		if (!time_after(expiration_time, pan->discovery_ts))
+			continue;
+
+		cfg802154_unlink_pan(rdev, pan);
+	}
+}
+EXPORT_SYMBOL(cfg802154_expire_pans);
+
+static void cfg802154_expire_oldest_pan(struct cfg802154_registered_device *rdev)
+{
+	struct cfg802154_internal_pan *pan, *oldest;
+
+	lockdep_assert_held(&rdev->pan_lock);
+
+	if (WARN_ON(list_empty(&rdev->pan_list)))
+		return;
+
+	oldest = list_first_entry(&rdev->pan_list,
+				  struct cfg802154_internal_pan, list);
+
+	list_for_each_entry(pan, &rdev->pan_list, list) {
+		if (!time_before(oldest->discovery_ts, pan->discovery_ts))
+			oldest = pan;
+	}
+
+	cfg802154_unlink_pan(rdev, oldest);
+}
+
+void cfg802154_flush_pans(struct cfg802154_registered_device *rdev)
+{
+	struct cfg802154_internal_pan *pan, *tmp;
+
+	lockdep_assert_held(&rdev->pan_lock);
+
+	list_for_each_entry_safe(pan, tmp, &rdev->pan_list, list)
+		cfg802154_unlink_pan(rdev, pan);
+}
+EXPORT_SYMBOL(cfg802154_flush_pans);
+
+static bool cfg802154_same_pan(struct ieee802154_pan_desc *a,
+			       struct ieee802154_pan_desc *b)
+{
+	int ret;
+
+	if (a->page != b->page)
+		return false;
+
+	if (a->channel != b->channel)
+		return false;
+
+	ret = memcmp(&a->coord->pan_id, &b->coord->pan_id,
+		     sizeof(a->coord->pan_id));
+	if (ret)
+		return false;
+
+	if (a->coord->mode != b->coord->mode)
+		return false;
+
+	if (a->coord->mode == IEEE802154_ADDR_SHORT)
+		ret = memcmp(&a->coord->short_addr, &b->coord->short_addr,
+			     IEEE802154_SHORT_ADDR_LEN);
+	else
+		ret = memcmp(&a->coord->extended_addr, &b->coord->extended_addr,
+			     IEEE802154_EXTENDED_ADDR_LEN);
+
+	return true;
+}
+
+static struct cfg802154_internal_pan *
+cfg802154_find_matching_pan(struct cfg802154_registered_device *rdev,
+			    struct cfg802154_internal_pan *tmp)
+{
+	struct cfg802154_internal_pan *pan;
+
+	list_for_each_entry(pan, &rdev->pan_list, list) {
+		if (cfg802154_same_pan(&pan->desc, &tmp->desc))
+			return pan;
+	}
+
+	return NULL;
+}
+
+static void cfg802154_pan_update(struct cfg802154_registered_device *rdev,
+				 struct cfg802154_internal_pan *new)
+{
+	struct cfg802154_internal_pan *found;
+
+	spin_lock_bh(&rdev->pan_lock);
+
+	found = cfg802154_find_matching_pan(rdev, new);
+	if (found)
+		cfg802154_unlink_pan(rdev, found);
+
+	if (unlikely(cfg802154_need_to_expire_pans(rdev)))
+		cfg802154_expire_oldest_pan(rdev);
+
+	cfg802154_link_pan(rdev, new);
+
+	spin_unlock_bh(&rdev->pan_lock);
+}
+
+int cfg802154_record_pan(struct wpan_phy *wpan_phy,
+			 struct ieee802154_pan_desc *desc)
+{
+	struct cfg802154_registered_device *rdev = wpan_phy_to_rdev(wpan_phy);
+	struct cfg802154_internal_pan *new;
+
+	new = cfg802154_alloc_pan(desc);
+	if (IS_ERR(new))
+		return (PTR_ERR(new));
+
+	cfg802154_pan_update(rdev, new);
+
+	return 0;
+}
+EXPORT_SYMBOL(cfg802154_record_pan);
-- 
2.34.1

Re: [PATCH wpan-next v3 2/4] net: ieee802154: Add support for inter PAN management

[Alexander Aring]

Hi,

On Mon, Jun 20, 2022 at 10:26 AM Miquel Raynal
<miquel.raynal@bootlin.com> wrote:
>
> Let's introduce the basics for defining PANs:
> - structures defining a PAN
> - helpers for PAN registration
> - helpers discarding old PANs
>

I think the whole pan management can/should be stored in user space by
a daemon running in background. This can be a network manager as it
listens to netlink events as "detect PAN xy" and stores it and offers
it in their list to associate with it.

We need somewhere to draw a line and I guess the line is "Is this
information used e.g. as any lookup or something in the hot path", I
don't see this currently...

- Alex

Re: [PATCH wpan-next v3 2/4] net: ieee802154: Add support for inter PAN management

[Miquel Raynal]

Hi Alexander,

aahringo@redhat.com wrote on Sat, 25 Jun 2022 22:29:08 -0400:

> Hi,
> 
> On Mon, Jun 20, 2022 at 10:26 AM Miquel Raynal
> <miquel.raynal@bootlin.com> wrote:
> >
> > Let's introduce the basics for defining PANs:
> > - structures defining a PAN
> > - helpers for PAN registration
> > - helpers discarding old PANs
> >  
> 
> I think the whole pan management can/should be stored in user space by
> a daemon running in background.

We need both, and currently:
- while the scan is happening, the kernel saves all the discovered PANs
- the kernel PAN list can be dumped (and also flushed) asynchronously by
  the userspace

IOW the userspace is responsible of keeping its own list of PANs in
sync with what the kernel discovers, so at any moment it can ask the
kernel what it has in memory, it can be done during a scan or after. It
can request a new scan to update the entries, or flush the kernel list.
The scan operation is always requested by the user anyway, it's not
something happening in the background.

> This can be a network manager as it
> listens to netlink events as "detect PAN xy" and stores it and offers
> it in their list to associate with it.

There are events produced, yes. But really, this is not something we
actually need. The user requests a scan over a given range, when the
scan is over it looks at the list and decides which PAN it
wants to associate with, and through which coordinator (95% of the
scenarii).

> We need somewhere to draw a line and I guess the line is "Is this
> information used e.g. as any lookup or something in the hot path", I
> don't see this currently...

Each PAN descriptor is like 20 bytes, so that's why I don't feel back
keeping them, I think it's easier to be able to serve the list of PANs
upon request rather than only forwarding events and not being able to
retrieve the list a second time (at least during the development).

Overall I feel like this part is still a little bit blurry because it
has currently no user, perhaps I should send the next series which
actually makes the current series useful.

Thanks,
Miquèl

Re: [PATCH wpan-next v3 2/4] net: ieee802154: Add support for inter PAN management

[Alexander Aring]

Hi,

On Mon, Jun 27, 2022 at 4:43 AM Miquel Raynal <miquel.raynal@bootlin.com> wrote:
>
> Hi Alexander,
>
> aahringo@redhat.com wrote on Sat, 25 Jun 2022 22:29:08 -0400:
>
> > Hi,
> >
> > On Mon, Jun 20, 2022 at 10:26 AM Miquel Raynal
> > <miquel.raynal@bootlin.com> wrote:
> > >
> > > Let's introduce the basics for defining PANs:
> > > - structures defining a PAN
> > > - helpers for PAN registration
> > > - helpers discarding old PANs
> > >
> >
> > I think the whole pan management can/should be stored in user space by
> > a daemon running in background.
>
> We need both, and currently:
> - while the scan is happening, the kernel saves all the discovered PANs
> - the kernel PAN list can be dumped (and also flushed) asynchronously by
>   the userspace
>
> IOW the userspace is responsible of keeping its own list of PANs in
> sync with what the kernel discovers, so at any moment it can ask the
> kernel what it has in memory, it can be done during a scan or after. It
> can request a new scan to update the entries, or flush the kernel list.
> The scan operation is always requested by the user anyway, it's not
> something happening in the background.
>

I don't see what advantage it has to keep the discovered pan in the
kernel. You can do everything with a start/stop/pan discovered event.
It also has more advantages as you can look for a specific pan and
stop afterwards.
At the end the daemon has everything that the kernel also has, as you
said it's in sync.

> > This can be a network manager as it
> > listens to netlink events as "detect PAN xy" and stores it and offers
> > it in their list to associate with it.
>
> There are events produced, yes. But really, this is not something we
> actually need. The user requests a scan over a given range, when the
> scan is over it looks at the list and decides which PAN it
> wants to associate with, and through which coordinator (95% of the
> scenarii).
>

This isn't either a kernel job to decide which pan it will be associated with.

> > We need somewhere to draw a line and I guess the line is "Is this
> > information used e.g. as any lookup or something in the hot path", I
> > don't see this currently...
>
> Each PAN descriptor is like 20 bytes, so that's why I don't feel back
> keeping them, I think it's easier to be able to serve the list of PANs
> upon request rather than only forwarding events and not being able to
> retrieve the list a second time (at least during the development).
>

This has nothing to do with memory.

> Overall I feel like this part is still a little bit blurry because it
> has currently no user, perhaps I should send the next series which
> actually makes the current series useful.
>

Will it get more used than caching entries in the kernel for user
space? Please also no in-kernel association feature.

We can maybe agree to that point to put it under
IEEE802154_NL802154_EXPERIMENTAL config, as soon as we have some
_open_ user space program ready we will drop this feature again...
this program will show that there is no magic about it.

- Alex

Re: [PATCH wpan-next v3 2/4] net: ieee802154: Add support for inter PAN management

[Miquel Raynal]

Hi Alexander,

aahringo@redhat.com wrote on Mon, 27 Jun 2022 21:32:08 -0400:

> Hi,
> 
> On Mon, Jun 27, 2022 at 4:43 AM Miquel Raynal <miquel.raynal@bootlin.com> wrote:
> >
> > Hi Alexander,
> >
> > aahringo@redhat.com wrote on Sat, 25 Jun 2022 22:29:08 -0400:
> >  
> > > Hi,
> > >
> > > On Mon, Jun 20, 2022 at 10:26 AM Miquel Raynal
> > > <miquel.raynal@bootlin.com> wrote:  
> > > >
> > > > Let's introduce the basics for defining PANs:
> > > > - structures defining a PAN
> > > > - helpers for PAN registration
> > > > - helpers discarding old PANs
> > > >  
> > >
> > > I think the whole pan management can/should be stored in user space by
> > > a daemon running in background.  
> >
> > We need both, and currently:
> > - while the scan is happening, the kernel saves all the discovered PANs
> > - the kernel PAN list can be dumped (and also flushed) asynchronously by
> >   the userspace
> >
> > IOW the userspace is responsible of keeping its own list of PANs in
> > sync with what the kernel discovers, so at any moment it can ask the
> > kernel what it has in memory, it can be done during a scan or after. It
> > can request a new scan to update the entries, or flush the kernel list.
> > The scan operation is always requested by the user anyway, it's not
> > something happening in the background.
> >  
> 
> I don't see what advantage it has to keep the discovered pan in the
> kernel. You can do everything with a start/stop/pan discovered event.

I think the main reason is to be much more user friendly. Keeping track
of the known PANs in the kernel matters because when you start working
with 802.15.4 you won't blindly use a daemon (if there is any) and will
use test apps like iwpan which are stateless. Re-doing a scan on demand
just takes ages (from seconds to minutes, depending on the beacon
order).

Aside from this non technical reason, I also had in mind to retrieve
values gathered from the beacons (and stored in the PAN descriptors) to
know more about the devices when eg. listing associations, like
registering the short address of a coordinator. I don't yet know how
useful this is TBH.

> It also has more advantages as you can look for a specific pan and
> stop afterwards. At the end the daemon has everything that the kernel
> also has, as you said it's in sync.
> 
> > > This can be a network manager as it
> > > listens to netlink events as "detect PAN xy" and stores it and
> > > offers it in their list to associate with it.  
> >
> > There are events produced, yes. But really, this is not something we
> > actually need. The user requests a scan over a given range, when the
> > scan is over it looks at the list and decides which PAN it
> > wants to associate with, and through which coordinator (95% of the
> > scenarii).
> >  
> 
> This isn't either a kernel job to decide which pan it will be
> associated with.

Yes, "it looks at the list and decides" referred to "the user".

> > > We need somewhere to draw a line and I guess the line is "Is this
> > > information used e.g. as any lookup or something in the hot path", I
> > > don't see this currently...  
> >
> > Each PAN descriptor is like 20 bytes, so that's why I don't feel back
> > keeping them, I think it's easier to be able to serve the list of PANs
> > upon request rather than only forwarding events and not being able to
> > retrieve the list a second time (at least during the development).
> >  
> 
> This has nothing to do with memory.
> 
> > Overall I feel like this part is still a little bit blurry because it
> > has currently no user, perhaps I should send the next series which
> > actually makes the current series useful.
> >  
> 
> Will it get more used than caching entries in the kernel for user
> space? Please also no in-kernel association feature.

I am aligned on this.

> We can maybe agree to that point to put it under
> IEEE802154_NL802154_EXPERIMENTAL config, as soon as we have some
> _open_ user space program ready we will drop this feature again...
> this program will show that there is no magic about it.

Yeah, do you want to move all the code scan/beacon/pan/association code
under EXPERIMENTAL sections? Or is it just the PAN management logic?

Thanks,
Miquèl

Re: [PATCH wpan-next v3 2/4] net: ieee802154: Add support for inter PAN management

[Alexander Aring]

Hi,

On Tue, Jun 28, 2022 at 3:58 AM Miquel Raynal <miquel.raynal@bootlin.com> wrote:
>
> Hi Alexander,
>
> aahringo@redhat.com wrote on Mon, 27 Jun 2022 21:32:08 -0400:
>
> > Hi,
> >
> > On Mon, Jun 27, 2022 at 4:43 AM Miquel Raynal <miquel.raynal@bootlin.com> wrote:
> > >
> > > Hi Alexander,
> > >
> > > aahringo@redhat.com wrote on Sat, 25 Jun 2022 22:29:08 -0400:
> > >
> > > > Hi,
> > > >
> > > > On Mon, Jun 20, 2022 at 10:26 AM Miquel Raynal
> > > > <miquel.raynal@bootlin.com> wrote:
> > > > >
> > > > > Let's introduce the basics for defining PANs:
> > > > > - structures defining a PAN
> > > > > - helpers for PAN registration
> > > > > - helpers discarding old PANs
> > > > >
> > > >
> > > > I think the whole pan management can/should be stored in user space by
> > > > a daemon running in background.
> > >
> > > We need both, and currently:
> > > - while the scan is happening, the kernel saves all the discovered PANs
> > > - the kernel PAN list can be dumped (and also flushed) asynchronously by
> > >   the userspace
> > >
> > > IOW the userspace is responsible of keeping its own list of PANs in
> > > sync with what the kernel discovers, so at any moment it can ask the
> > > kernel what it has in memory, it can be done during a scan or after. It
> > > can request a new scan to update the entries, or flush the kernel list.
> > > The scan operation is always requested by the user anyway, it's not
> > > something happening in the background.
> > >
> >
> > I don't see what advantage it has to keep the discovered pan in the
> > kernel. You can do everything with a start/stop/pan discovered event.
>
> I think the main reason is to be much more user friendly. Keeping track
> of the known PANs in the kernel matters because when you start working
> with 802.15.4 you won't blindly use a daemon (if there is any) and will
> use test apps like iwpan which are stateless. Re-doing a scan on demand
> just takes ages (from seconds to minutes, depending on the beacon
> order).
>

I can see that things should work "out-of the box" and we are already
doing it by manual setting pan_id, etc. However, doing it in an
automatic way there exists a lot of "interpretation" about how you
want to handle it (doesn't matter if this is what the spec says or
not)... moving it to user space will offload it to the user.

> Aside from this non technical reason, I also had in mind to retrieve
> values gathered from the beacons (and stored in the PAN descriptors) to
> know more about the devices when eg. listing associations, like
> registering the short address of a coordinator. I don't yet know how
> useful this is TBH.
>
> > It also has more advantages as you can look for a specific pan and
> > stop afterwards. At the end the daemon has everything that the kernel
> > also has, as you said it's in sync.
> >
> > > > This can be a network manager as it
> > > > listens to netlink events as "detect PAN xy" and stores it and
> > > > offers it in their list to associate with it.
> > >
> > > There are events produced, yes. But really, this is not something we
> > > actually need. The user requests a scan over a given range, when the
> > > scan is over it looks at the list and decides which PAN it
> > > wants to associate with, and through which coordinator (95% of the
> > > scenarii).
> > >
> >
> > This isn't either a kernel job to decide which pan it will be
> > associated with.
>
> Yes, "it looks at the list and decides" referred to "the user".
>
> > > > We need somewhere to draw a line and I guess the line is "Is this
> > > > information used e.g. as any lookup or something in the hot path", I
> > > > don't see this currently...
> > >
> > > Each PAN descriptor is like 20 bytes, so that's why I don't feel back
> > > keeping them, I think it's easier to be able to serve the list of PANs
> > > upon request rather than only forwarding events and not being able to
> > > retrieve the list a second time (at least during the development).
> > >
> >
> > This has nothing to do with memory.
> >
> > > Overall I feel like this part is still a little bit blurry because it
> > > has currently no user, perhaps I should send the next series which
> > > actually makes the current series useful.
> > >
> >
> > Will it get more used than caching entries in the kernel for user
> > space? Please also no in-kernel association feature.
>
> I am aligned on this.
>

I am sorry I am not sure what that means.

> > We can maybe agree to that point to put it under
> > IEEE802154_NL802154_EXPERIMENTAL config, as soon as we have some
> > _open_ user space program ready we will drop this feature again...
> > this program will show that there is no magic about it.
>
> Yeah, do you want to move all the code scan/beacon/pan/association code
> under EXPERIMENTAL sections? Or is it just the PAN management logic?

Yes, why not. But as I can see there exists two categories of
introducing your netlink api:

1. API candidates which are very likely to become stable
2. API candidates which we want to remove when we have a user
replacement for it (will probably never go stable)

The 2. should be defining _after_ the 1. In the "big" netlink API
enums of EXPERIMENTAL sections.

Also you should provide for 2. some kind of ifdef/functions dummy/etc.
that it's easy to remove from the kernel when we have a user
replacement for it.
I hope that is fine for everybody.

I try to find solutions here, I don't see a reason for putting this
pan management into the kernel... whereas I appreciate the effort
which is done here and will not force you to write some user space
software that does this job. From my point of view I can't accept this
functionality in the kernel "yet".

- Alex

Re: [PATCH wpan-next v3 2/4] net: ieee802154: Add support for inter PAN management

[Miquel Raynal]

Hi Alexander,

aahringo@redhat.com wrote on Wed, 29 Jun 2022 21:40:14 -0400:

> Hi,
> 
> On Tue, Jun 28, 2022 at 3:58 AM Miquel Raynal <miquel.raynal@bootlin.com> wrote:
> >
> > Hi Alexander,
> >
> > aahringo@redhat.com wrote on Mon, 27 Jun 2022 21:32:08 -0400:
> >  
> > > Hi,
> > >
> > > On Mon, Jun 27, 2022 at 4:43 AM Miquel Raynal <miquel.raynal@bootlin.com> wrote:  
> > > >
> > > > Hi Alexander,
> > > >
> > > > aahringo@redhat.com wrote on Sat, 25 Jun 2022 22:29:08 -0400:
> > > >  
> > > > > Hi,
> > > > >
> > > > > On Mon, Jun 20, 2022 at 10:26 AM Miquel Raynal
> > > > > <miquel.raynal@bootlin.com> wrote:  
> > > > > >
> > > > > > Let's introduce the basics for defining PANs:
> > > > > > - structures defining a PAN
> > > > > > - helpers for PAN registration
> > > > > > - helpers discarding old PANs
> > > > > >  
> > > > >
> > > > > I think the whole pan management can/should be stored in user space by
> > > > > a daemon running in background.  
> > > >
> > > > We need both, and currently:
> > > > - while the scan is happening, the kernel saves all the discovered PANs
> > > > - the kernel PAN list can be dumped (and also flushed) asynchronously by
> > > >   the userspace
> > > >
> > > > IOW the userspace is responsible of keeping its own list of PANs in
> > > > sync with what the kernel discovers, so at any moment it can ask the
> > > > kernel what it has in memory, it can be done during a scan or after. It
> > > > can request a new scan to update the entries, or flush the kernel list.
> > > > The scan operation is always requested by the user anyway, it's not
> > > > something happening in the background.
> > > >  
> > >
> > > I don't see what advantage it has to keep the discovered pan in the
> > > kernel. You can do everything with a start/stop/pan discovered event.  
> >
> > I think the main reason is to be much more user friendly. Keeping track
> > of the known PANs in the kernel matters because when you start working
> > with 802.15.4 you won't blindly use a daemon (if there is any) and will
> > use test apps like iwpan which are stateless. Re-doing a scan on demand
> > just takes ages (from seconds to minutes, depending on the beacon
> > order).
> >  
> 
> I can see that things should work "out-of the box" and we are already
> doing it by manual setting pan_id, etc. However, doing it in an
> automatic way there exists a lot of "interpretation" about how you
> want to handle it (doesn't matter if this is what the spec says or
> not)... moving it to user space will offload it to the user.
> 
> > Aside from this non technical reason, I also had in mind to retrieve
> > values gathered from the beacons (and stored in the PAN descriptors) to
> > know more about the devices when eg. listing associations, like
> > registering the short address of a coordinator. I don't yet know how
> > useful this is TBH.
> >  
> > > It also has more advantages as you can look for a specific pan and
> > > stop afterwards. At the end the daemon has everything that the kernel
> > > also has, as you said it's in sync.
> > >  
> > > > > This can be a network manager as it
> > > > > listens to netlink events as "detect PAN xy" and stores it and
> > > > > offers it in their list to associate with it.  
> > > >
> > > > There are events produced, yes. But really, this is not something we
> > > > actually need. The user requests a scan over a given range, when the
> > > > scan is over it looks at the list and decides which PAN it
> > > > wants to associate with, and through which coordinator (95% of the
> > > > scenarii).
> > > >  
> > >
> > > This isn't either a kernel job to decide which pan it will be
> > > associated with.  
> >
> > Yes, "it looks at the list and decides" referred to "the user".
> >  
> > > > > We need somewhere to draw a line and I guess the line is "Is this
> > > > > information used e.g. as any lookup or something in the hot path", I
> > > > > don't see this currently...  
> > > >
> > > > Each PAN descriptor is like 20 bytes, so that's why I don't feel back
> > > > keeping them, I think it's easier to be able to serve the list of PANs
> > > > upon request rather than only forwarding events and not being able to
> > > > retrieve the list a second time (at least during the development).
> > > >  
> > >
> > > This has nothing to do with memory.
> > >  
> > > > Overall I feel like this part is still a little bit blurry because it
> > > > has currently no user, perhaps I should send the next series which
> > > > actually makes the current series useful.
> > > >  
> > >
> > > Will it get more used than caching entries in the kernel for user
> > > space? Please also no in-kernel association feature.  
> >
> > I am aligned on this.
> >  
> 
> I am sorry I am not sure what that means.

I was referring to the "no in-kernel association feature".

There is however one situation which I _had_ to be handled in the
kernel: other devices asking for being associated or disassociated. In
the case of the disassociation, the receiving device is only notified
and cannot refuse the disassociation. For the association however,
the device receiving the association request has to make a decision.
There are three possible outcomes:
- accepting
- refusing because the PAN is at capacity
- refusing because the device is blacklisted
For now I've only implemented the first reason, because it's much
easier and only requires a maximum device number variable, set by the
user. For the second reason, it requires handling a
whitelist/blacklist, I don't plan to implement this for now, but that
should not impact the rest of the code. I'll let that to other
developers, or future-me, perhaps :-). Anyhow, you can kick-out devices
at any time anyway if needed with a disassociation notification
controlled by the user.

> > > We can maybe agree to that point to put it under
> > > IEEE802154_NL802154_EXPERIMENTAL config, as soon as we have some
> > > _open_ user space program ready we will drop this feature again...
> > > this program will show that there is no magic about it.  
> >
> > Yeah, do you want to move all the code scan/beacon/pan/association code
> > under EXPERIMENTAL sections? Or is it just the PAN management logic?  
> 
> Yes, why not. But as I can see there exists two categories of
> introducing your netlink api:
> 
> 1. API candidates which are very likely to become stable
> 2. API candidates which we want to remove when we have a user
> replacement for it (will probably never go stable)
> 
> The 2. should be defining _after_ the 1. In the "big" netlink API
> enums of EXPERIMENTAL sections.

Yeah, got it.

> Also you should provide for 2. some kind of ifdef/functions dummy/etc.
> that it's easy to remove from the kernel when we have a user
> replacement for it.
> I hope that is fine for everybody.
> 
> I try to find solutions here, I don't see a reason for putting this
> pan management into the kernel... whereas I appreciate the effort
> which is done here and will not force you to write some user space
> software that does this job. From my point of view I can't accept this
> functionality in the kernel "yet".

I've already spent a couple of days reworking all that part, I've
dropped most of the in-kernel PAN management, which means:
- when a new coordinator gets discovered (beacon received), if the mac
  was scanning then it calls a generic function from the cfg layer to
  advertise this pan.
- the cfg layer will send a NL message to the user with all the
  important information
- BUT the cfg layer will also keep in memory the beacon information for
  the time of the scan (only), to avoid polluting the user with the same
  information over and over again, this seems a necessary step to me,
  because otherwise if you track on the same channel two coordinators
  not emitting at the same pace, you might end up with 100 user
  notifications, for just 2 devices. I think this is the kernel duty to
  filter out identical beacons.

I _will_ send a v4, including the scanning part this time by the end of
the week, I need to settle everything down, ensure it still works and
clean the branch.

Thanks,
Miquèl

Re: [PATCH wpan-next v3 2/4] net: ieee802154: Add support for inter PAN management

[Alexander Aring]

Hi,

On Thu, Jun 30, 2022 at 4:14 AM Miquel Raynal <miquel.raynal@bootlin.com> wrote:
>
> Hi Alexander,
>
> aahringo@redhat.com wrote on Wed, 29 Jun 2022 21:40:14 -0400:
>
> > Hi,
> >
> > On Tue, Jun 28, 2022 at 3:58 AM Miquel Raynal <miquel.raynal@bootlin.com> wrote:
> > >
> > > Hi Alexander,
> > >
> > > aahringo@redhat.com wrote on Mon, 27 Jun 2022 21:32:08 -0400:
> > >
> > > > Hi,
> > > >
> > > > On Mon, Jun 27, 2022 at 4:43 AM Miquel Raynal <miquel.raynal@bootlin.com> wrote:
> > > > >
> > > > > Hi Alexander,
> > > > >
> > > > > aahringo@redhat.com wrote on Sat, 25 Jun 2022 22:29:08 -0400:
> > > > >
> > > > > > Hi,
> > > > > >
> > > > > > On Mon, Jun 20, 2022 at 10:26 AM Miquel Raynal
> > > > > > <miquel.raynal@bootlin.com> wrote:
> > > > > > >
> > > > > > > Let's introduce the basics for defining PANs:
> > > > > > > - structures defining a PAN
> > > > > > > - helpers for PAN registration
> > > > > > > - helpers discarding old PANs
> > > > > > >
> > > > > >
> > > > > > I think the whole pan management can/should be stored in user space by
> > > > > > a daemon running in background.
> > > > >
> > > > > We need both, and currently:
> > > > > - while the scan is happening, the kernel saves all the discovered PANs
> > > > > - the kernel PAN list can be dumped (and also flushed) asynchronously by
> > > > >   the userspace
> > > > >
> > > > > IOW the userspace is responsible of keeping its own list of PANs in
> > > > > sync with what the kernel discovers, so at any moment it can ask the
> > > > > kernel what it has in memory, it can be done during a scan or after. It
> > > > > can request a new scan to update the entries, or flush the kernel list.
> > > > > The scan operation is always requested by the user anyway, it's not
> > > > > something happening in the background.
> > > > >
> > > >
> > > > I don't see what advantage it has to keep the discovered pan in the
> > > > kernel. You can do everything with a start/stop/pan discovered event.
> > >
> > > I think the main reason is to be much more user friendly. Keeping track
> > > of the known PANs in the kernel matters because when you start working
> > > with 802.15.4 you won't blindly use a daemon (if there is any) and will
> > > use test apps like iwpan which are stateless. Re-doing a scan on demand
> > > just takes ages (from seconds to minutes, depending on the beacon
> > > order).
> > >
> >
> > I can see that things should work "out-of the box" and we are already
> > doing it by manual setting pan_id, etc. However, doing it in an
> > automatic way there exists a lot of "interpretation" about how you
> > want to handle it (doesn't matter if this is what the spec says or
> > not)... moving it to user space will offload it to the user.
> >
> > > Aside from this non technical reason, I also had in mind to retrieve
> > > values gathered from the beacons (and stored in the PAN descriptors) to
> > > know more about the devices when eg. listing associations, like
> > > registering the short address of a coordinator. I don't yet know how
> > > useful this is TBH.
> > >
> > > > It also has more advantages as you can look for a specific pan and
> > > > stop afterwards. At the end the daemon has everything that the kernel
> > > > also has, as you said it's in sync.
> > > >
> > > > > > This can be a network manager as it
> > > > > > listens to netlink events as "detect PAN xy" and stores it and
> > > > > > offers it in their list to associate with it.
> > > > >
> > > > > There are events produced, yes. But really, this is not something we
> > > > > actually need. The user requests a scan over a given range, when the
> > > > > scan is over it looks at the list and decides which PAN it
> > > > > wants to associate with, and through which coordinator (95% of the
> > > > > scenarii).
> > > > >
> > > >
> > > > This isn't either a kernel job to decide which pan it will be
> > > > associated with.
> > >
> > > Yes, "it looks at the list and decides" referred to "the user".
> > >
> > > > > > We need somewhere to draw a line and I guess the line is "Is this
> > > > > > information used e.g. as any lookup or something in the hot path", I
> > > > > > don't see this currently...
> > > > >
> > > > > Each PAN descriptor is like 20 bytes, so that's why I don't feel back
> > > > > keeping them, I think it's easier to be able to serve the list of PANs
> > > > > upon request rather than only forwarding events and not being able to
> > > > > retrieve the list a second time (at least during the development).
> > > > >
> > > >
> > > > This has nothing to do with memory.
> > > >
> > > > > Overall I feel like this part is still a little bit blurry because it
> > > > > has currently no user, perhaps I should send the next series which
> > > > > actually makes the current series useful.
> > > > >
> > > >
> > > > Will it get more used than caching entries in the kernel for user
> > > > space? Please also no in-kernel association feature.
> > >
> > > I am aligned on this.
> > >
> >
> > I am sorry I am not sure what that means.
>
> I was referring to the "no in-kernel association feature".
>
> There is however one situation which I _had_ to be handled in the
> kernel: other devices asking for being associated or disassociated. In
> the case of the disassociation, the receiving device is only notified
> and cannot refuse the disassociation. For the association however,
> the device receiving the association request has to make a decision.
> There are three possible outcomes:
> - accepting
> - refusing because the PAN is at capacity
> - refusing because the device is blacklisted

Why not move this decision to the user as well? The kernel will wait
for the reason? This isn't required to be fast and the decision may
depend on the current pan management...

> For now I've only implemented the first reason, because it's much
> easier and only requires a maximum device number variable, set by the
> user. For the second reason, it requires handling a
> whitelist/blacklist, I don't plan to implement this for now, but that
> should not impact the rest of the code. I'll let that to other
> developers, or future-me, perhaps :-). Anyhow, you can kick-out devices
> at any time anyway if needed with a disassociation notification
> controlled by the user.
>
> > > > We can maybe agree to that point to put it under
> > > > IEEE802154_NL802154_EXPERIMENTAL config, as soon as we have some
> > > > _open_ user space program ready we will drop this feature again...
> > > > this program will show that there is no magic about it.
> > >
> > > Yeah, do you want to move all the code scan/beacon/pan/association code
> > > under EXPERIMENTAL sections? Or is it just the PAN management logic?
> >
> > Yes, why not. But as I can see there exists two categories of
> > introducing your netlink api:
> >
> > 1. API candidates which are very likely to become stable
> > 2. API candidates which we want to remove when we have a user
> > replacement for it (will probably never go stable)
> >
> > The 2. should be defining _after_ the 1. In the "big" netlink API
> > enums of EXPERIMENTAL sections.
>
> Yeah, got it.
>
> > Also you should provide for 2. some kind of ifdef/functions dummy/etc.
> > that it's easy to remove from the kernel when we have a user
> > replacement for it.
> > I hope that is fine for everybody.
> >
> > I try to find solutions here, I don't see a reason for putting this
> > pan management into the kernel... whereas I appreciate the effort
> > which is done here and will not force you to write some user space
> > software that does this job. From my point of view I can't accept this
> > functionality in the kernel "yet".
>
> I've already spent a couple of days reworking all that part, I've
> dropped most of the in-kernel PAN management, which means:
> - when a new coordinator gets discovered (beacon received), if the mac
>   was scanning then it calls a generic function from the cfg layer to
>   advertise this pan.
> - the cfg layer will send a NL message to the user with all the
>   important information
> - BUT the cfg layer will also keep in memory the beacon information for
>   the time of the scan (only), to avoid polluting the user with the same
>   information over and over again, this seems a necessary step to me,
>   because otherwise if you track on the same channel two coordinators
>   not emitting at the same pace, you might end up with 100 user
>   notifications, for just 2 devices. I think this is the kernel duty to
>   filter out identical beacons.
>

Okay, I am sure if somebody complains about such kernel behaviour and
has a good argument to switch back... we still can do it.

> I _will_ send a v4, including the scanning part this time by the end of
> the week, I need to settle everything down, ensure it still works and
> clean the branch.
>

ok.

- Alex

Re: [PATCH wpan-next v3 2/4] net: ieee802154: Add support for inter PAN management

[Alexander Aring]

Hi,

On Thu, Jun 30, 2022 at 7:27 PM Alexander Aring <aahringo@redhat.com> wrote:
...
>
> Why not move this decision to the user as well? The kernel will wait
> for the reason? This isn't required to be fast and the decision may
> depend on the current pan management...

to be clear here, that this will then and on some coordinator only use
a user space daemon which manages whatever is needed for assoc/deassoc
management e.g. how short-addresses are allocated, etc. That should
also not be part of the kernel, if so then same strategy as we have a
user space replacement for it?

- Alex

Re: [PATCH wpan-next v3 2/4] net: ieee802154: Add support for inter PAN management

[Miquel Raynal]

Hi Alexander,

aahringo@redhat.com wrote on Thu, 30 Jun 2022 19:27:49 -0400:

> Hi,
> 
> On Thu, Jun 30, 2022 at 4:14 AM Miquel Raynal <miquel.raynal@bootlin.com> wrote:
> >
> > Hi Alexander,
> >
> > aahringo@redhat.com wrote on Wed, 29 Jun 2022 21:40:14 -0400:
> >  
> > > Hi,
> > >
> > > On Tue, Jun 28, 2022 at 3:58 AM Miquel Raynal <miquel.raynal@bootlin.com> wrote:  
> > > >
> > > > Hi Alexander,
> > > >
> > > > aahringo@redhat.com wrote on Mon, 27 Jun 2022 21:32:08 -0400:
> > > >  
> > > > > Hi,
> > > > >
> > > > > On Mon, Jun 27, 2022 at 4:43 AM Miquel Raynal <miquel.raynal@bootlin.com> wrote:  
> > > > > >
> > > > > > Hi Alexander,
> > > > > >
> > > > > > aahringo@redhat.com wrote on Sat, 25 Jun 2022 22:29:08 -0400:
> > > > > >  
> > > > > > > Hi,
> > > > > > >
> > > > > > > On Mon, Jun 20, 2022 at 10:26 AM Miquel Raynal
> > > > > > > <miquel.raynal@bootlin.com> wrote:  
> > > > > > > >
> > > > > > > > Let's introduce the basics for defining PANs:
> > > > > > > > - structures defining a PAN
> > > > > > > > - helpers for PAN registration
> > > > > > > > - helpers discarding old PANs
> > > > > > > >  
> > > > > > >
> > > > > > > I think the whole pan management can/should be stored in user space by
> > > > > > > a daemon running in background.  
> > > > > >
> > > > > > We need both, and currently:
> > > > > > - while the scan is happening, the kernel saves all the discovered PANs
> > > > > > - the kernel PAN list can be dumped (and also flushed) asynchronously by
> > > > > >   the userspace
> > > > > >
> > > > > > IOW the userspace is responsible of keeping its own list of PANs in
> > > > > > sync with what the kernel discovers, so at any moment it can ask the
> > > > > > kernel what it has in memory, it can be done during a scan or after. It
> > > > > > can request a new scan to update the entries, or flush the kernel list.
> > > > > > The scan operation is always requested by the user anyway, it's not
> > > > > > something happening in the background.
> > > > > >  
> > > > >
> > > > > I don't see what advantage it has to keep the discovered pan in the
> > > > > kernel. You can do everything with a start/stop/pan discovered event.  
> > > >
> > > > I think the main reason is to be much more user friendly. Keeping track
> > > > of the known PANs in the kernel matters because when you start working
> > > > with 802.15.4 you won't blindly use a daemon (if there is any) and will
> > > > use test apps like iwpan which are stateless. Re-doing a scan on demand
> > > > just takes ages (from seconds to minutes, depending on the beacon
> > > > order).
> > > >  
> > >
> > > I can see that things should work "out-of the box" and we are already
> > > doing it by manual setting pan_id, etc. However, doing it in an
> > > automatic way there exists a lot of "interpretation" about how you
> > > want to handle it (doesn't matter if this is what the spec says or
> > > not)... moving it to user space will offload it to the user.
> > >  
> > > > Aside from this non technical reason, I also had in mind to retrieve
> > > > values gathered from the beacons (and stored in the PAN descriptors) to
> > > > know more about the devices when eg. listing associations, like
> > > > registering the short address of a coordinator. I don't yet know how
> > > > useful this is TBH.
> > > >  
> > > > > It also has more advantages as you can look for a specific pan and
> > > > > stop afterwards. At the end the daemon has everything that the kernel
> > > > > also has, as you said it's in sync.
> > > > >  
> > > > > > > This can be a network manager as it
> > > > > > > listens to netlink events as "detect PAN xy" and stores it and
> > > > > > > offers it in their list to associate with it.  
> > > > > >
> > > > > > There are events produced, yes. But really, this is not something we
> > > > > > actually need. The user requests a scan over a given range, when the
> > > > > > scan is over it looks at the list and decides which PAN it
> > > > > > wants to associate with, and through which coordinator (95% of the
> > > > > > scenarii).
> > > > > >  
> > > > >
> > > > > This isn't either a kernel job to decide which pan it will be
> > > > > associated with.  
> > > >
> > > > Yes, "it looks at the list and decides" referred to "the user".
> > > >  
> > > > > > > We need somewhere to draw a line and I guess the line is "Is this
> > > > > > > information used e.g. as any lookup or something in the hot path", I
> > > > > > > don't see this currently...  
> > > > > >
> > > > > > Each PAN descriptor is like 20 bytes, so that's why I don't feel back
> > > > > > keeping them, I think it's easier to be able to serve the list of PANs
> > > > > > upon request rather than only forwarding events and not being able to
> > > > > > retrieve the list a second time (at least during the development).
> > > > > >  
> > > > >
> > > > > This has nothing to do with memory.
> > > > >  
> > > > > > Overall I feel like this part is still a little bit blurry because it
> > > > > > has currently no user, perhaps I should send the next series which
> > > > > > actually makes the current series useful.
> > > > > >  
> > > > >
> > > > > Will it get more used than caching entries in the kernel for user
> > > > > space? Please also no in-kernel association feature.  
> > > >
> > > > I am aligned on this.
> > > >  
> > >
> > > I am sorry I am not sure what that means.  
> >
> > I was referring to the "no in-kernel association feature".
> >
> > There is however one situation which I _had_ to be handled in the
> > kernel: other devices asking for being associated or disassociated. In
> > the case of the disassociation, the receiving device is only notified
> > and cannot refuse the disassociation. For the association however,
> > the device receiving the association request has to make a decision.
> > There are three possible outcomes:
> > - accepting
> > - refusing because the PAN is at capacity
> > - refusing because the device is blacklisted  
> 
> Why not move this decision to the user as well? The kernel will wait
> for the reason? This isn't required to be fast and the decision may
> depend on the current pan management...

I've opted out for the simplest option, which is allowing X devices
being associated, X being manageable by the user. For now I'll keep
this very simple approach, I propose we add this filtering feature
later?

> > For now I've only implemented the first reason, because it's much
> > easier and only requires a maximum device number variable, set by the
> > user. For the second reason, it requires handling a
> > whitelist/blacklist, I don't plan to implement this for now, but that
> > should not impact the rest of the code. I'll let that to other
> > developers, or future-me, perhaps :-). Anyhow, you can kick-out devices
> > at any time anyway if needed with a disassociation notification
> > controlled by the user.
> >  
> > > > > We can maybe agree to that point to put it under
> > > > > IEEE802154_NL802154_EXPERIMENTAL config, as soon as we have some
> > > > > _open_ user space program ready we will drop this feature again...
> > > > > this program will show that there is no magic about it.  
> > > >
> > > > Yeah, do you want to move all the code scan/beacon/pan/association code
> > > > under EXPERIMENTAL sections? Or is it just the PAN management logic?  
> > >
> > > Yes, why not. But as I can see there exists two categories of
> > > introducing your netlink api:
> > >
> > > 1. API candidates which are very likely to become stable
> > > 2. API candidates which we want to remove when we have a user
> > > replacement for it (will probably never go stable)
> > >
> > > The 2. should be defining _after_ the 1. In the "big" netlink API
> > > enums of EXPERIMENTAL sections.  
> >
> > Yeah, got it.
> >  
> > > Also you should provide for 2. some kind of ifdef/functions dummy/etc.
> > > that it's easy to remove from the kernel when we have a user
> > > replacement for it.
> > > I hope that is fine for everybody.
> > >
> > > I try to find solutions here, I don't see a reason for putting this
> > > pan management into the kernel... whereas I appreciate the effort
> > > which is done here and will not force you to write some user space
> > > software that does this job. From my point of view I can't accept this
> > > functionality in the kernel "yet".  
> >
> > I've already spent a couple of days reworking all that part, I've
> > dropped most of the in-kernel PAN management, which means:
> > - when a new coordinator gets discovered (beacon received), if the mac
> >   was scanning then it calls a generic function from the cfg layer to
> >   advertise this pan.
> > - the cfg layer will send a NL message to the user with all the
> >   important information
> > - BUT the cfg layer will also keep in memory the beacon information for
> >   the time of the scan (only), to avoid polluting the user with the same
> >   information over and over again, this seems a necessary step to me,
> >   because otherwise if you track on the same channel two coordinators
> >   not emitting at the same pace, you might end up with 100 user
> >   notifications, for just 2 devices. I think this is the kernel duty to
> >   filter out identical beacons.
> >  
> 
> Okay, I am sure if somebody complains about such kernel behaviour and
> has a good argument to switch back... we still can do it.

Great!

> 
> > I _will_ send a v4, including the scanning part this time by the end of
> > the week, I need to settle everything down, ensure it still works and
> > clean the branch.
> >  
> 
> ok.
> 
> - Alex
> 


Thanks,
Miquèl

Re: [PATCH wpan-next v3 2/4] net: ieee802154: Add support for inter PAN management

[Alexander Aring]

Hi,

On Thu, Jun 30, 2022 at 8:50 PM Miquel Raynal <miquel.raynal@bootlin.com> wrote:
>
> Hi Alexander,
>
> aahringo@redhat.com wrote on Thu, 30 Jun 2022 19:27:49 -0400:
>
> > Hi,
> >
> > On Thu, Jun 30, 2022 at 4:14 AM Miquel Raynal <miquel.raynal@bootlin.com> wrote:
> > >
> > > Hi Alexander,
> > >
> > > aahringo@redhat.com wrote on Wed, 29 Jun 2022 21:40:14 -0400:
> > >
> > > > Hi,
> > > >
> > > > On Tue, Jun 28, 2022 at 3:58 AM Miquel Raynal <miquel.raynal@bootlin.com> wrote:
> > > > >
> > > > > Hi Alexander,
> > > > >
> > > > > aahringo@redhat.com wrote on Mon, 27 Jun 2022 21:32:08 -0400:
> > > > >
> > > > > > Hi,
> > > > > >
> > > > > > On Mon, Jun 27, 2022 at 4:43 AM Miquel Raynal <miquel.raynal@bootlin.com> wrote:
> > > > > > >
> > > > > > > Hi Alexander,
> > > > > > >
> > > > > > > aahringo@redhat.com wrote on Sat, 25 Jun 2022 22:29:08 -0400:
> > > > > > >
> > > > > > > > Hi,
> > > > > > > >
> > > > > > > > On Mon, Jun 20, 2022 at 10:26 AM Miquel Raynal
> > > > > > > > <miquel.raynal@bootlin.com> wrote:
> > > > > > > > >
> > > > > > > > > Let's introduce the basics for defining PANs:
> > > > > > > > > - structures defining a PAN
> > > > > > > > > - helpers for PAN registration
> > > > > > > > > - helpers discarding old PANs
> > > > > > > > >
> > > > > > > >
> > > > > > > > I think the whole pan management can/should be stored in user space by
> > > > > > > > a daemon running in background.
> > > > > > >
> > > > > > > We need both, and currently:
> > > > > > > - while the scan is happening, the kernel saves all the discovered PANs
> > > > > > > - the kernel PAN list can be dumped (and also flushed) asynchronously by
> > > > > > >   the userspace
> > > > > > >
> > > > > > > IOW the userspace is responsible of keeping its own list of PANs in
> > > > > > > sync with what the kernel discovers, so at any moment it can ask the
> > > > > > > kernel what it has in memory, it can be done during a scan or after. It
> > > > > > > can request a new scan to update the entries, or flush the kernel list.
> > > > > > > The scan operation is always requested by the user anyway, it's not
> > > > > > > something happening in the background.
> > > > > > >
> > > > > >
> > > > > > I don't see what advantage it has to keep the discovered pan in the
> > > > > > kernel. You can do everything with a start/stop/pan discovered event.
> > > > >
> > > > > I think the main reason is to be much more user friendly. Keeping track
> > > > > of the known PANs in the kernel matters because when you start working
> > > > > with 802.15.4 you won't blindly use a daemon (if there is any) and will
> > > > > use test apps like iwpan which are stateless. Re-doing a scan on demand
> > > > > just takes ages (from seconds to minutes, depending on the beacon
> > > > > order).
> > > > >
> > > >
> > > > I can see that things should work "out-of the box" and we are already
> > > > doing it by manual setting pan_id, etc. However, doing it in an
> > > > automatic way there exists a lot of "interpretation" about how you
> > > > want to handle it (doesn't matter if this is what the spec says or
> > > > not)... moving it to user space will offload it to the user.
> > > >
> > > > > Aside from this non technical reason, I also had in mind to retrieve
> > > > > values gathered from the beacons (and stored in the PAN descriptors) to
> > > > > know more about the devices when eg. listing associations, like
> > > > > registering the short address of a coordinator. I don't yet know how
> > > > > useful this is TBH.
> > > > >
> > > > > > It also has more advantages as you can look for a specific pan and
> > > > > > stop afterwards. At the end the daemon has everything that the kernel
> > > > > > also has, as you said it's in sync.
> > > > > >
> > > > > > > > This can be a network manager as it
> > > > > > > > listens to netlink events as "detect PAN xy" and stores it and
> > > > > > > > offers it in their list to associate with it.
> > > > > > >
> > > > > > > There are events produced, yes. But really, this is not something we
> > > > > > > actually need. The user requests a scan over a given range, when the
> > > > > > > scan is over it looks at the list and decides which PAN it
> > > > > > > wants to associate with, and through which coordinator (95% of the
> > > > > > > scenarii).
> > > > > > >
> > > > > >
> > > > > > This isn't either a kernel job to decide which pan it will be
> > > > > > associated with.
> > > > >
> > > > > Yes, "it looks at the list and decides" referred to "the user".
> > > > >
> > > > > > > > We need somewhere to draw a line and I guess the line is "Is this
> > > > > > > > information used e.g. as any lookup or something in the hot path", I
> > > > > > > > don't see this currently...
> > > > > > >
> > > > > > > Each PAN descriptor is like 20 bytes, so that's why I don't feel back
> > > > > > > keeping them, I think it's easier to be able to serve the list of PANs
> > > > > > > upon request rather than only forwarding events and not being able to
> > > > > > > retrieve the list a second time (at least during the development).
> > > > > > >
> > > > > >
> > > > > > This has nothing to do with memory.
> > > > > >
> > > > > > > Overall I feel like this part is still a little bit blurry because it
> > > > > > > has currently no user, perhaps I should send the next series which
> > > > > > > actually makes the current series useful.
> > > > > > >
> > > > > >
> > > > > > Will it get more used than caching entries in the kernel for user
> > > > > > space? Please also no in-kernel association feature.
> > > > >
> > > > > I am aligned on this.
> > > > >
> > > >
> > > > I am sorry I am not sure what that means.
> > >
> > > I was referring to the "no in-kernel association feature".
> > >
> > > There is however one situation which I _had_ to be handled in the
> > > kernel: other devices asking for being associated or disassociated. In
> > > the case of the disassociation, the receiving device is only notified
> > > and cannot refuse the disassociation. For the association however,
> > > the device receiving the association request has to make a decision.
> > > There are three possible outcomes:
> > > - accepting
> > > - refusing because the PAN is at capacity
> > > - refusing because the device is blacklisted
> >
> > Why not move this decision to the user as well? The kernel will wait
> > for the reason? This isn't required to be fast and the decision may
> > depend on the current pan management...
>
> I've opted out for the simplest option, which is allowing X devices
> being associated, X being manageable by the user. For now I'll keep
> this very simple approach, I propose we add this filtering feature
> later?
>

What I suggest here is to move the filtering logic into the user
space. If the interface is a coordinator it will trigger an event for
the user and waits for an upper layer user space logic to get an
answer back what to do as answer.

However as I said I don't force you to program a user space software
which does that job but you code should be prepared to be get replaced
by such handling.

> > > For now I've only implemented the first reason, because it's much
> > > easier and only requires a maximum device number variable, set by the
> > > user. For the second reason, it requires handling a
> > > whitelist/blacklist, I don't plan to implement this for now, but that
> > > should not impact the rest of the code. I'll let that to other
> > > developers, or future-me, perhaps :-). Anyhow, you can kick-out devices
> > > at any time anyway if needed with a disassociation notification
> > > controlled by the user.
> > >
> > > > > > We can maybe agree to that point to put it under
> > > > > > IEEE802154_NL802154_EXPERIMENTAL config, as soon as we have some
> > > > > > _open_ user space program ready we will drop this feature again...
> > > > > > this program will show that there is no magic about it.
> > > > >
> > > > > Yeah, do you want to move all the code scan/beacon/pan/association code
> > > > > under EXPERIMENTAL sections? Or is it just the PAN management logic?
> > > >
> > > > Yes, why not. But as I can see there exists two categories of
> > > > introducing your netlink api:
> > > >
> > > > 1. API candidates which are very likely to become stable
> > > > 2. API candidates which we want to remove when we have a user
> > > > replacement for it (will probably never go stable)
> > > >
> > > > The 2. should be defining _after_ the 1. In the "big" netlink API
> > > > enums of EXPERIMENTAL sections.
> > >
> > > Yeah, got it.
> > >
> > > > Also you should provide for 2. some kind of ifdef/functions dummy/etc.
> > > > that it's easy to remove from the kernel when we have a user
> > > > replacement for it.
> > > > I hope that is fine for everybody.
> > > >
> > > > I try to find solutions here, I don't see a reason for putting this
> > > > pan management into the kernel... whereas I appreciate the effort
> > > > which is done here and will not force you to write some user space
> > > > software that does this job. From my point of view I can't accept this
> > > > functionality in the kernel "yet".
> > >
> > > I've already spent a couple of days reworking all that part, I've
> > > dropped most of the in-kernel PAN management, which means:
> > > - when a new coordinator gets discovered (beacon received), if the mac
> > >   was scanning then it calls a generic function from the cfg layer to
> > >   advertise this pan.
> > > - the cfg layer will send a NL message to the user with all the
> > >   important information
> > > - BUT the cfg layer will also keep in memory the beacon information for
> > >   the time of the scan (only), to avoid polluting the user with the same
> > >   information over and over again, this seems a necessary step to me,
> > >   because otherwise if you track on the same channel two coordinators
> > >   not emitting at the same pace, you might end up with 100 user
> > >   notifications, for just 2 devices. I think this is the kernel duty to
> > >   filter out identical beacons.
> > >
> >
> > Okay, I am sure if somebody complains about such kernel behaviour and
> > has a good argument to switch back... we still can do it.
>
> Great!
>

I would say more here... there might be some API documentation where
you cannot expect anything from the kernel but it tries to avoid
stupid things (Whatever that means). As the API is experimental it can
be easily changed, otherwise some additional flag is required to
enable this feature or not. However I can say more about this when I
see code and we have some user experience about whatever the default
behaviour should be or such flag is really necessary.

We probably will find some solution...

- Alex

Re: [PATCH wpan-next v3 2/4] net: ieee802154: Add support for inter PAN management

[Miquel Raynal]

Hi Alexander,

aahringo@redhat.com wrote on Fri, 1 Jul 2022 08:23:32 -0400:

> Hi,
> 
> On Thu, Jun 30, 2022 at 8:50 PM Miquel Raynal <miquel.raynal@bootlin.com> wrote:
> >
> > Hi Alexander,
> >
> > aahringo@redhat.com wrote on Thu, 30 Jun 2022 19:27:49 -0400:
> >  
> > > Hi,
> > >
> > > On Thu, Jun 30, 2022 at 4:14 AM Miquel Raynal <miquel.raynal@bootlin.com> wrote:  
> > > >
> > > > Hi Alexander,
> > > >
> > > > aahringo@redhat.com wrote on Wed, 29 Jun 2022 21:40:14 -0400:
> > > >  
> > > > > Hi,
> > > > >
> > > > > On Tue, Jun 28, 2022 at 3:58 AM Miquel Raynal <miquel.raynal@bootlin.com> wrote:  
> > > > > >
> > > > > > Hi Alexander,
> > > > > >
> > > > > > aahringo@redhat.com wrote on Mon, 27 Jun 2022 21:32:08 -0400:
> > > > > >  
> > > > > > > Hi,
> > > > > > >
> > > > > > > On Mon, Jun 27, 2022 at 4:43 AM Miquel Raynal <miquel.raynal@bootlin.com> wrote:  
> > > > > > > >
> > > > > > > > Hi Alexander,
> > > > > > > >
> > > > > > > > aahringo@redhat.com wrote on Sat, 25 Jun 2022 22:29:08 -0400:
> > > > > > > >  
> > > > > > > > > Hi,
> > > > > > > > >
> > > > > > > > > On Mon, Jun 20, 2022 at 10:26 AM Miquel Raynal
> > > > > > > > > <miquel.raynal@bootlin.com> wrote:  
> > > > > > > > > >
> > > > > > > > > > Let's introduce the basics for defining PANs:
> > > > > > > > > > - structures defining a PAN
> > > > > > > > > > - helpers for PAN registration
> > > > > > > > > > - helpers discarding old PANs
> > > > > > > > > >  
> > > > > > > > >
> > > > > > > > > I think the whole pan management can/should be stored in user space by
> > > > > > > > > a daemon running in background.  
> > > > > > > >
> > > > > > > > We need both, and currently:
> > > > > > > > - while the scan is happening, the kernel saves all the discovered PANs
> > > > > > > > - the kernel PAN list can be dumped (and also flushed) asynchronously by
> > > > > > > >   the userspace
> > > > > > > >
> > > > > > > > IOW the userspace is responsible of keeping its own list of PANs in
> > > > > > > > sync with what the kernel discovers, so at any moment it can ask the
> > > > > > > > kernel what it has in memory, it can be done during a scan or after. It
> > > > > > > > can request a new scan to update the entries, or flush the kernel list.
> > > > > > > > The scan operation is always requested by the user anyway, it's not
> > > > > > > > something happening in the background.
> > > > > > > >  
> > > > > > >
> > > > > > > I don't see what advantage it has to keep the discovered pan in the
> > > > > > > kernel. You can do everything with a start/stop/pan discovered event.  
> > > > > >
> > > > > > I think the main reason is to be much more user friendly. Keeping track
> > > > > > of the known PANs in the kernel matters because when you start working
> > > > > > with 802.15.4 you won't blindly use a daemon (if there is any) and will
> > > > > > use test apps like iwpan which are stateless. Re-doing a scan on demand
> > > > > > just takes ages (from seconds to minutes, depending on the beacon
> > > > > > order).
> > > > > >  
> > > > >
> > > > > I can see that things should work "out-of the box" and we are already
> > > > > doing it by manual setting pan_id, etc. However, doing it in an
> > > > > automatic way there exists a lot of "interpretation" about how you
> > > > > want to handle it (doesn't matter if this is what the spec says or
> > > > > not)... moving it to user space will offload it to the user.
> > > > >  
> > > > > > Aside from this non technical reason, I also had in mind to retrieve
> > > > > > values gathered from the beacons (and stored in the PAN descriptors) to
> > > > > > know more about the devices when eg. listing associations, like
> > > > > > registering the short address of a coordinator. I don't yet know how
> > > > > > useful this is TBH.
> > > > > >  
> > > > > > > It also has more advantages as you can look for a specific pan and
> > > > > > > stop afterwards. At the end the daemon has everything that the kernel
> > > > > > > also has, as you said it's in sync.
> > > > > > >  
> > > > > > > > > This can be a network manager as it
> > > > > > > > > listens to netlink events as "detect PAN xy" and stores it and
> > > > > > > > > offers it in their list to associate with it.  
> > > > > > > >
> > > > > > > > There are events produced, yes. But really, this is not something we
> > > > > > > > actually need. The user requests a scan over a given range, when the
> > > > > > > > scan is over it looks at the list and decides which PAN it
> > > > > > > > wants to associate with, and through which coordinator (95% of the
> > > > > > > > scenarii).
> > > > > > > >  
> > > > > > >
> > > > > > > This isn't either a kernel job to decide which pan it will be
> > > > > > > associated with.  
> > > > > >
> > > > > > Yes, "it looks at the list and decides" referred to "the user".
> > > > > >  
> > > > > > > > > We need somewhere to draw a line and I guess the line is "Is this
> > > > > > > > > information used e.g. as any lookup or something in the hot path", I
> > > > > > > > > don't see this currently...  
> > > > > > > >
> > > > > > > > Each PAN descriptor is like 20 bytes, so that's why I don't feel back
> > > > > > > > keeping them, I think it's easier to be able to serve the list of PANs
> > > > > > > > upon request rather than only forwarding events and not being able to
> > > > > > > > retrieve the list a second time (at least during the development).
> > > > > > > >  
> > > > > > >
> > > > > > > This has nothing to do with memory.
> > > > > > >  
> > > > > > > > Overall I feel like this part is still a little bit blurry because it
> > > > > > > > has currently no user, perhaps I should send the next series which
> > > > > > > > actually makes the current series useful.
> > > > > > > >  
> > > > > > >
> > > > > > > Will it get more used than caching entries in the kernel for user
> > > > > > > space? Please also no in-kernel association feature.  
> > > > > >
> > > > > > I am aligned on this.
> > > > > >  
> > > > >
> > > > > I am sorry I am not sure what that means.  
> > > >
> > > > I was referring to the "no in-kernel association feature".
> > > >
> > > > There is however one situation which I _had_ to be handled in the
> > > > kernel: other devices asking for being associated or disassociated. In
> > > > the case of the disassociation, the receiving device is only notified
> > > > and cannot refuse the disassociation. For the association however,
> > > > the device receiving the association request has to make a decision.
> > > > There are three possible outcomes:
> > > > - accepting
> > > > - refusing because the PAN is at capacity
> > > > - refusing because the device is blacklisted  
> > >
> > > Why not move this decision to the user as well? The kernel will wait
> > > for the reason? This isn't required to be fast and the decision may
> > > depend on the current pan management...  
> >
> > I've opted out for the simplest option, which is allowing X devices
> > being associated, X being manageable by the user. For now I'll keep
> > this very simple approach, I propose we add this filtering feature
> > later?
> >  
> 
> What I suggest here is to move the filtering logic into the user
> space. If the interface is a coordinator it will trigger an event for
> the user and waits for an upper layer user space logic to get an
> answer back what to do as answer.
> 
> However as I said I don't force you to program a user space software
> which does that job but you code should be prepared to be get replaced
> by such handling.

Actually I really think we should keep the maximum value which returns
the 802.15.4 "PAN AT CAPACITY" error status. The filtering thing is an
additional feature, I don't think it will replace what I currently
provide, but it will rather complement it.

> > > > For now I've only implemented the first reason, because it's much
> > > > easier and only requires a maximum device number variable, set by the
> > > > user. For the second reason, it requires handling a
> > > > whitelist/blacklist, I don't plan to implement this for now, but that
> > > > should not impact the rest of the code. I'll let that to other
> > > > developers, or future-me, perhaps :-). Anyhow, you can kick-out devices
> > > > at any time anyway if needed with a disassociation notification
> > > > controlled by the user.
> > > >  
> > > > > > > We can maybe agree to that point to put it under
> > > > > > > IEEE802154_NL802154_EXPERIMENTAL config, as soon as we have some
> > > > > > > _open_ user space program ready we will drop this feature again...
> > > > > > > this program will show that there is no magic about it.  
> > > > > >
> > > > > > Yeah, do you want to move all the code scan/beacon/pan/association code
> > > > > > under EXPERIMENTAL sections? Or is it just the PAN management logic?  
> > > > >
> > > > > Yes, why not. But as I can see there exists two categories of
> > > > > introducing your netlink api:
> > > > >
> > > > > 1. API candidates which are very likely to become stable
> > > > > 2. API candidates which we want to remove when we have a user
> > > > > replacement for it (will probably never go stable)
> > > > >
> > > > > The 2. should be defining _after_ the 1. In the "big" netlink API
> > > > > enums of EXPERIMENTAL sections.  
> > > >
> > > > Yeah, got it.
> > > >  
> > > > > Also you should provide for 2. some kind of ifdef/functions dummy/etc.
> > > > > that it's easy to remove from the kernel when we have a user
> > > > > replacement for it.
> > > > > I hope that is fine for everybody.
> > > > >
> > > > > I try to find solutions here, I don't see a reason for putting this
> > > > > pan management into the kernel... whereas I appreciate the effort
> > > > > which is done here and will not force you to write some user space
> > > > > software that does this job. From my point of view I can't accept this
> > > > > functionality in the kernel "yet".  
> > > >
> > > > I've already spent a couple of days reworking all that part, I've
> > > > dropped most of the in-kernel PAN management, which means:
> > > > - when a new coordinator gets discovered (beacon received), if the mac
> > > >   was scanning then it calls a generic function from the cfg layer to
> > > >   advertise this pan.
> > > > - the cfg layer will send a NL message to the user with all the
> > > >   important information
> > > > - BUT the cfg layer will also keep in memory the beacon information for
> > > >   the time of the scan (only), to avoid polluting the user with the same
> > > >   information over and over again, this seems a necessary step to me,
> > > >   because otherwise if you track on the same channel two coordinators
> > > >   not emitting at the same pace, you might end up with 100 user
> > > >   notifications, for just 2 devices. I think this is the kernel duty to
> > > >   filter out identical beacons.
> > > >  
> > >
> > > Okay, I am sure if somebody complains about such kernel behaviour and
> > > has a good argument to switch back... we still can do it.  
> >
> > Great!
> >  
> 
> I would say more here... there might be some API documentation where
> you cannot expect anything from the kernel but it tries to avoid
> stupid things (Whatever that means). As the API is experimental it can
> be easily changed, otherwise some additional flag is required to
> enable this feature or not. However I can say more about this when I
> see code and we have some user experience about whatever the default
> behaviour should be or such flag is really necessary.

As I've dropped the entire internal PAN management handling thing with
userspace I have not set anything within the EXPERIMENTAL section, but
I believe there will be other versions, so we can decide what else
should go in there and I'll do it in the next version.

Thanks,
Miquèl

[PATCH wpan-next v3 3/4] net: ieee802154: Give the user access to list of surrounding PANs

[Miquel Raynal]

Now that we have support for registering PANs, give certain rights to
the user, such as listing asynchronously the PANs as well as flushing
the list.

The maximum number of PANs to list and their delay before expiration can
be configured. By default there is no limit. When these parameters are
set, PANs are automatically dropped from the list.

This change has the side effect of moving the following helpers out of
the experimental zone as they are now used by non-experimental security
functions:
- nl802154_prepare_wpan_dev_dump()
- nl802154_finish_wpan_dev_dump()

Co-developed-by: David Girault <david.girault@qorvo.com>
Signed-off-by: David Girault <david.girault@qorvo.com>
Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
 include/net/nl802154.h    |  49 ++++++++++
 net/ieee802154/nl802154.c | 199 +++++++++++++++++++++++++++++++++++++-
 2 files changed, 246 insertions(+), 2 deletions(-)

diff --git a/include/net/nl802154.h b/include/net/nl802154.h
index 145acb8f2509..5e2ccedd3826 100644
--- a/include/net/nl802154.h
+++ b/include/net/nl802154.h
@@ -58,6 +58,11 @@ enum nl802154_commands {
 
 	NL802154_CMD_SET_WPAN_PHY_NETNS,
 
+	NL802154_CMD_DUMP_PANS,
+	NL802154_CMD_FLUSH_PANS,
+	NL802154_CMD_SET_MAX_PAN_ENTRIES,
+	NL802154_CMD_SET_PANS_EXPIRATION,
+
 	/* add new commands above here */
 
 #ifdef CONFIG_IEEE802154_NL802154_EXPERIMENTAL
@@ -133,6 +138,10 @@ enum nl802154_attrs {
 	NL802154_ATTR_PID,
 	NL802154_ATTR_NETNS_FD,
 
+	NL802154_ATTR_PAN,
+	NL802154_ATTR_MAX_PAN_ENTRIES,
+	NL802154_ATTR_PANS_EXPIRATION,
+
 	/* add attributes here, update the policy in nl802154.c */
 
 #ifdef CONFIG_IEEE802154_NL802154_EXPERIMENTAL
@@ -218,6 +227,46 @@ enum nl802154_wpan_phy_capability_attr {
 	NL802154_CAP_ATTR_MAX = __NL802154_CAP_ATTR_AFTER_LAST - 1
 };
 
+/**
+ * enum nl802154_pan - Netlink attributes for a PAN
+ *
+ * @__NL802154_PAN_INVALID: invalid
+ * @NL802154_PAN_PANID: PANID of the PAN (2 bytes)
+ * @NL802154_PAN_COORD_ADDR: Coordinator address, (8 bytes or 2 bytes)
+ * @NL802154_PAN_CHANNEL: channel number, related to @NL802154_PAN_PAGE (u8)
+ * @NL802154_PAN_PAGE: channel page, related to @NL802154_PAN_CHANNEL (u8)
+ * @NL802154_PAN_PREAMBLE_CODE: Preamble code while the beacon was received,
+ *	this is PHY dependent and optional (4 bytes)
+ * @NL802154_PAN_SUPERFRAME_SPEC: superframe specification of the PAN (u16)
+ * @NL802154_PAN_LINK_QUALITY: signal quality of beacon in unspecified units,
+ *	scaled to 0..255 (u8)
+ * @NL802154_PAN_GTS_PERMIT: set to true if GTS is permitted on this PAN
+ * @NL802154_PAN_PAYLOAD_DATA: binary data containing the raw data from the
+ *	frame payload, (only if beacon or probe response had data)
+ * @NL802154_PAN_STATUS: status, if this PAN is "used"
+ * @NL802154_PAN_SEEN_MS_AGO: age of this PAN entry in ms
+ * @NL802154_PAN_PAD: attribute used for padding for 64-bit alignment
+ * @NL802154_PAN_MAX: highest PAN attribute
+ */
+enum nl802154_pan {
+	__NL802154_PAN_INVALID,
+	NL802154_PAN_PANID,
+	NL802154_PAN_COORD_ADDR,
+	NL802154_PAN_CHANNEL,
+	NL802154_PAN_PAGE,
+	NL802154_PAN_PREAMBLE_CODE,
+	NL802154_PAN_SUPERFRAME_SPEC,
+	NL802154_PAN_LINK_QUALITY,
+	NL802154_PAN_GTS_PERMIT,
+	NL802154_PAN_PAYLOAD_DATA,
+	NL802154_PAN_STATUS,
+	NL802154_PAN_SEEN_MS_AGO,
+	NL802154_PAN_PAD,
+
+	/* keep last */
+	NL802154_PAN_MAX,
+};
+
 /**
  * enum nl802154_cca_modes - cca modes
  *
diff --git a/net/ieee802154/nl802154.c b/net/ieee802154/nl802154.c
index e0b072aecf0f..2356e6893398 100644
--- a/net/ieee802154/nl802154.c
+++ b/net/ieee802154/nl802154.c
@@ -216,6 +216,11 @@ static const struct nla_policy nl802154_policy[NL802154_ATTR_MAX+1] = {
 
 	[NL802154_ATTR_PID] = { .type = NLA_U32 },
 	[NL802154_ATTR_NETNS_FD] = { .type = NLA_U32 },
+
+	[NL802154_ATTR_PAN] = { .type = NLA_NESTED },
+	[NL802154_ATTR_MAX_PAN_ENTRIES] = { .type = NLA_U32 },
+	[NL802154_ATTR_PANS_EXPIRATION] = { .type = NLA_U32 },
+
 #ifdef CONFIG_IEEE802154_NL802154_EXPERIMENTAL
 	[NL802154_ATTR_SEC_ENABLED] = { .type = NLA_U8, },
 	[NL802154_ATTR_SEC_OUT_LEVEL] = { .type = NLA_U32, },
@@ -229,7 +234,6 @@ static const struct nla_policy nl802154_policy[NL802154_ATTR_MAX+1] = {
 #endif /* CONFIG_IEEE802154_NL802154_EXPERIMENTAL */
 };
 
-#ifdef CONFIG_IEEE802154_NL802154_EXPERIMENTAL
 static int
 nl802154_prepare_wpan_dev_dump(struct sk_buff *skb,
 			       struct netlink_callback *cb,
@@ -288,7 +292,6 @@ nl802154_finish_wpan_dev_dump(struct cfg802154_registered_device *rdev)
 {
 	rtnl_unlock();
 }
-#endif /* CONFIG_IEEE802154_NL802154_EXPERIMENTAL */
 
 /* message building helper */
 static inline void *nl802154hdr_put(struct sk_buff *skb, u32 portid, u32 seq,
@@ -1281,6 +1284,172 @@ static int nl802154_wpan_phy_netns(struct sk_buff *skb, struct genl_info *info)
 	return err;
 }
 
+static int nl802154_send_pan_info(struct sk_buff *msg,
+				  struct netlink_callback *cb,
+				  u32 seq, int flags,
+				  struct cfg802154_registered_device *rdev,
+				  struct wpan_dev *wpan_dev,
+				  struct cfg802154_internal_pan *intpan)
+{
+	struct ieee802154_pan_desc *pan = &intpan->desc;
+	struct nlattr *nla;
+	void *hdr;
+
+	ASSERT_RTNL();
+
+	hdr = nl802154hdr_put(msg, NETLINK_CB(cb->skb).portid, seq, flags,
+			      NL802154_CMD_DUMP_PANS);
+	if (!hdr)
+		return -ENOBUFS;
+
+	genl_dump_check_consistent(cb, hdr);
+
+	if (nla_put_u32(msg, NL802154_ATTR_GENERATION, rdev->pan_generation))
+		goto nla_put_failure;
+
+	if (wpan_dev->netdev &&
+	    nla_put_u32(msg, NL802154_ATTR_IFINDEX, wpan_dev->netdev->ifindex))
+		goto nla_put_failure;
+
+	if (nla_put_u64_64bit(msg, NL802154_ATTR_WPAN_DEV, wpan_dev_id(wpan_dev),
+			      NL802154_ATTR_PAD))
+		goto nla_put_failure;
+
+	nla = nla_nest_start_noflag(msg, NL802154_ATTR_PAN);
+	if (!nla)
+		goto nla_put_failure;
+
+	if (nla_put(msg, NL802154_PAN_PANID, IEEE802154_PAN_ID_LEN,
+		    &pan->coord->pan_id))
+		goto nla_put_failure;
+
+	if (pan->coord->mode == IEEE802154_ADDR_SHORT) {
+		if (nla_put(msg, NL802154_PAN_COORD_ADDR,
+			    IEEE802154_SHORT_ADDR_LEN,
+			    &pan->coord->short_addr))
+			goto nla_put_failure;
+	} else {
+		if (nla_put(msg, NL802154_PAN_COORD_ADDR,
+			    IEEE802154_EXTENDED_ADDR_LEN,
+			    &pan->coord->extended_addr))
+			goto nla_put_failure;
+	}
+
+	if (nla_put_u8(msg, NL802154_PAN_CHANNEL, pan->channel))
+		goto nla_put_failure;
+
+	if (nla_put_u8(msg, NL802154_PAN_PAGE, pan->page))
+		goto nla_put_failure;
+
+	if (nla_put_u16(msg, NL802154_PAN_SUPERFRAME_SPEC,
+			pan->superframe_spec))
+		goto nla_put_failure;
+
+	if (nla_put_u8(msg, NL802154_PAN_LINK_QUALITY, pan->link_quality))
+		goto nla_put_failure;
+
+	if (nla_put_u32(msg, NL802154_PAN_SEEN_MS_AGO,
+			jiffies_to_msecs(jiffies - intpan->discovery_ts)))
+		goto nla_put_failure;
+
+	if (pan->gts_permit && nla_put_flag(msg, NL802154_PAN_GTS_PERMIT))
+		goto nla_put_failure;
+
+	/* TODO: NL802154_PAN_PAYLOAD_DATA if any */
+
+	nla_nest_end(msg, nla);
+	genlmsg_end(msg, hdr);
+
+	return 0;
+
+ nla_put_failure:
+	genlmsg_cancel(msg, hdr);
+	return -EMSGSIZE;
+}
+
+static int nl802154_dump_pans(struct sk_buff *skb, struct netlink_callback *cb)
+{
+	struct cfg802154_registered_device *rdev;
+	struct cfg802154_internal_pan *pan;
+	struct wpan_dev *wpan_dev;
+	int err;
+
+	err = nl802154_prepare_wpan_dev_dump(skb, cb, &rdev, &wpan_dev);
+	if (err)
+		return err;
+
+	spin_lock_bh(&rdev->pan_lock);
+
+	if (cb->args[2])
+		goto out;
+
+	cb->seq = rdev->pan_generation;
+
+	ieee802154_for_each_pan(pan, rdev) {
+		err = nl802154_send_pan_info(skb, cb, cb->nlh->nlmsg_seq,
+					     NLM_F_MULTI, rdev, wpan_dev, pan);
+		if (err < 0)
+			goto out_err;
+	}
+
+	cb->args[2] = 1;
+out:
+	err = skb->len;
+out_err:
+	spin_unlock_bh(&rdev->pan_lock);
+
+	nl802154_finish_wpan_dev_dump(rdev);
+
+	return err;
+}
+
+static int nl802154_flush_pans(struct sk_buff *skb, struct genl_info *info)
+{
+	struct cfg802154_registered_device *rdev = info->user_ptr[0];
+
+	spin_lock_bh(&rdev->pan_lock);
+	cfg802154_flush_pans(rdev);
+	spin_unlock_bh(&rdev->pan_lock);
+
+	return 0;
+}
+
+static int nl802154_set_max_pan_entries(struct sk_buff *skb,
+					struct genl_info *info)
+{
+	struct cfg802154_registered_device *rdev = info->user_ptr[0];
+	unsigned int max_entries;
+
+	if (!info->attrs[NL802154_ATTR_MAX_PAN_ENTRIES])
+		return -EINVAL;
+
+	max_entries = nla_get_u32(info->attrs[NL802154_ATTR_MAX_PAN_ENTRIES]);
+
+	spin_lock_bh(&rdev->pan_lock);
+	cfg802154_set_max_pan_entries(rdev, max_entries);
+	spin_unlock_bh(&rdev->pan_lock);
+
+	return 0;
+}
+
+static int nl802154_set_pans_expiration(struct sk_buff *skb,
+					struct genl_info *info)
+{
+	struct cfg802154_registered_device *rdev = info->user_ptr[0];
+	unsigned int exp_time_s;
+
+	if (!info->attrs[NL802154_ATTR_PANS_EXPIRATION])
+		return -EINVAL;
+
+	exp_time_s = nla_get_u32(info->attrs[NL802154_ATTR_PANS_EXPIRATION]);
+
+	spin_lock_bh(&rdev->pan_lock);
+	cfg802154_set_pans_expiration(rdev, exp_time_s);
+	spin_unlock_bh(&rdev->pan_lock);
+
+	return 0;
+}
+
 #ifdef CONFIG_IEEE802154_NL802154_EXPERIMENTAL
 static const struct nla_policy nl802154_dev_addr_policy[NL802154_DEV_ADDR_ATTR_MAX + 1] = {
 	[NL802154_DEV_ADDR_ATTR_PAN_ID] = { .type = NLA_U16 },
@@ -2369,6 +2538,32 @@ static const struct genl_ops nl802154_ops[] = {
 		.internal_flags = NL802154_FLAG_NEED_NETDEV |
 				  NL802154_FLAG_NEED_RTNL,
 	},
+	{
+		.cmd = NL802154_CMD_DUMP_PANS,
+		.dumpit = nl802154_dump_pans,
+		/* can be retrieved by unprivileged users */
+	},
+	{
+		.cmd = NL802154_CMD_FLUSH_PANS,
+		.doit = nl802154_flush_pans,
+		.flags = GENL_ADMIN_PERM,
+		.internal_flags = NL802154_FLAG_NEED_NETDEV |
+				  NL802154_FLAG_NEED_RTNL,
+	},
+	{
+		.cmd = NL802154_CMD_SET_MAX_PAN_ENTRIES,
+		.doit = nl802154_set_max_pan_entries,
+		.flags = GENL_ADMIN_PERM,
+		.internal_flags = NL802154_FLAG_NEED_NETDEV |
+				  NL802154_FLAG_NEED_RTNL,
+	},
+	{
+		.cmd = NL802154_CMD_SET_PANS_EXPIRATION,
+		.doit = nl802154_set_pans_expiration,
+		.flags = GENL_ADMIN_PERM,
+		.internal_flags = NL802154_FLAG_NEED_NETDEV |
+				  NL802154_FLAG_NEED_RTNL,
+	},
 #ifdef CONFIG_IEEE802154_NL802154_EXPERIMENTAL
 	{
 		.cmd = NL802154_CMD_SET_SEC_PARAMS,
-- 
2.34.1

[PATCH wpan-next v3 4/4] net: ieee802154: Trace the registration of new PANs

[Miquel Raynal]

From: David Girault <david.girault@qorvo.com>

Add an internal trace when new PANs get discovered.

Signed-off-by: David Girault <david.girault@qorvo.com>
Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
 net/ieee802154/pan.c   |  3 +++
 net/ieee802154/trace.h | 25 +++++++++++++++++++++++++
 2 files changed, 28 insertions(+)

diff --git a/net/ieee802154/pan.c b/net/ieee802154/pan.c
index b9f50f785960..0dd30c19c3a2 100644
--- a/net/ieee802154/pan.c
+++ b/net/ieee802154/pan.c
@@ -18,6 +18,7 @@
 
 #include "ieee802154.h"
 #include "core.h"
+#include "trace.h"
 
 static struct cfg802154_internal_pan *
 cfg802154_alloc_pan(struct ieee802154_pan_desc *desc)
@@ -205,6 +206,8 @@ static void cfg802154_pan_update(struct cfg802154_registered_device *rdev,
 	found = cfg802154_find_matching_pan(rdev, new);
 	if (found)
 		cfg802154_unlink_pan(rdev, found);
+	else
+		trace_802154_new_pan(&new->desc);
 
 	if (unlikely(cfg802154_need_to_expire_pans(rdev)))
 		cfg802154_expire_oldest_pan(rdev);
diff --git a/net/ieee802154/trace.h b/net/ieee802154/trace.h
index 19c2e5d60e76..fa989dac090d 100644
--- a/net/ieee802154/trace.h
+++ b/net/ieee802154/trace.h
@@ -295,6 +295,31 @@ TRACE_EVENT(802154_rdev_set_ackreq_default,
 		WPAN_DEV_PR_ARG, BOOL_TO_STR(__entry->ackreq))
 );
 
+DECLARE_EVENT_CLASS(802154_pan_evt,
+	TP_PROTO(struct ieee802154_pan_desc *desc),
+	TP_ARGS(desc),
+	TP_STRUCT__entry(
+		__field(u16, pan_id)
+		__field(__le64, coord_addr)
+		__field(u8, channel)
+		__field(u8, page)
+	),
+	TP_fast_assign(
+		__entry->page = desc->page;
+		__entry->channel = desc->channel;
+		memcpy(&__entry->pan_id, &desc->coord->pan_id, 2);
+		memcpy(&__entry->coord_addr, &desc->coord->extended_addr, 8);
+	),
+	TP_printk("panid: %u, coord_addr: 0x%llx, page: %u, channel: %u",
+		  __entry->pan_id, __le64_to_cpu(__entry->coord_addr),
+		  __entry->page, __entry->channel)
+);
+
+DEFINE_EVENT(802154_pan_evt, 802154_new_pan,
+	TP_PROTO(struct ieee802154_pan_desc *desc),
+	TP_ARGS(desc)
+);
+
 TRACE_EVENT(802154_rdev_return_int,
 	TP_PROTO(struct wpan_phy *wpan_phy, int ret),
 	TP_ARGS(wpan_phy, ret),
-- 
2.34.1