[PATCH wpan-next 0/3] IEEE 802.15.4 PAN discovery handling

[PATCH wpan-next 0/3] IEEE 802.15.4 PAN discovery handling

[Miquel Raynal]

Hello,

Last preparation step before the introduction of the scanning feature
(really): generic helpers to handle PAN discovery upon beacon
reception. We need to tell user space about the discoveries.

In all the past, current and future submissions, David and Romuald from
Qorvo are credited in various ways (main author, co-author,
suggested-by) depending of the amount of rework that was involved on
each patch, reflecting as much as possible the open-source guidelines we
follow in the kernel. All this effort is made possible thanks to Qorvo
Inc which is pushing towards a featureful upstream WPAN support.

Cheers,
Miquèl

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

Miquel Raynal (2):
  ieee802154: Advertize coordinators discovery
  ieee802154: Handle coordinators discovery

 include/net/cfg802154.h   |  32 ++++++++++
 include/net/nl802154.h    |  44 ++++++++++++++
 net/ieee802154/Makefile   |   2 +-
 net/ieee802154/core.c     |   2 +
 net/ieee802154/nl802154.c | 123 ++++++++++++++++++++++++++++++++++++++
 net/ieee802154/nl802154.h |   6 ++
 net/ieee802154/pan.c      | 116 +++++++++++++++++++++++++++++++++++
 net/ieee802154/trace.h    |  25 ++++++++
 8 files changed, 349 insertions(+), 1 deletion(-)
 create mode 100644 net/ieee802154/pan.c

-- 
2.34.1

[PATCH wpan-next 1/3] ieee802154: Advertize coordinators discovery

[Miquel Raynal]

Let's introduce the basics for advertizing discovered PANs and
coordinators, which is:
- A new "scan" netlink message group.
- A couple of netlink command/attribute.
- The main netlink helper to send a netlink message with all the
  necessary information to forward the main information to the user.

Two netlink attributes are proactively added to support future UWB
complex channels, but are not actually used yet.

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   |  20 +++++++
 include/net/nl802154.h    |  44 ++++++++++++++
 net/ieee802154/nl802154.c | 121 ++++++++++++++++++++++++++++++++++++++
 net/ieee802154/nl802154.h |   6 ++
 4 files changed, 191 insertions(+)

diff --git a/include/net/cfg802154.h b/include/net/cfg802154.h
index e1481f9cf049..8d67d9ed438d 100644
--- a/include/net/cfg802154.h
+++ b/include/net/cfg802154.h
@@ -260,6 +260,26 @@ struct ieee802154_addr {
 	};
 };
 
+/**
+ * struct ieee802154_coord_desc - Coordinator descriptor
+ * @coord: PAN ID and coordinator address
+ * @page: page this coordinator is using
+ * @channel: channel this coordinator is using
+ * @superframe_spec: SuperFrame specification as received
+ * @link_quality: link quality indicator at which the beacon was received
+ * @gts_permit: the coordinator accepts GTS requests
+ * @node: list item
+ */
+struct ieee802154_coord_desc {
+	struct ieee802154_addr *addr;
+	u8 page;
+	u8 channel;
+	u16 superframe_spec;
+	u8 link_quality;
+	bool gts_permit;
+	struct list_head node;
+};
+
 struct ieee802154_llsec_key_id {
 	u8 mode;
 	u8 id;
diff --git a/include/net/nl802154.h b/include/net/nl802154.h
index 145acb8f2509..cfe462288695 100644
--- a/include/net/nl802154.h
+++ b/include/net/nl802154.h
@@ -58,6 +58,9 @@ enum nl802154_commands {
 
 	NL802154_CMD_SET_WPAN_PHY_NETNS,
 
+	NL802154_CMD_NEW_COORDINATOR,
+	NL802154_CMD_KNOWN_COORDINATOR,
+
 	/* add new commands above here */
 
 #ifdef CONFIG_IEEE802154_NL802154_EXPERIMENTAL
@@ -133,6 +136,8 @@ enum nl802154_attrs {
 	NL802154_ATTR_PID,
 	NL802154_ATTR_NETNS_FD,
 
+	NL802154_ATTR_COORDINATOR,
+
 	/* add attributes here, update the policy in nl802154.c */
 
 #ifdef CONFIG_IEEE802154_NL802154_EXPERIMENTAL
@@ -218,6 +223,45 @@ enum nl802154_wpan_phy_capability_attr {
 	NL802154_CAP_ATTR_MAX = __NL802154_CAP_ATTR_AFTER_LAST - 1
 };
 
+/**
+ * enum nl802154_coord - Netlink attributes for a coord
+ *
+ * @__NL802154_COORD_INVALID: invalid
+ * @NL802154_COORD_PANID: PANID of the coordinator (2 bytes)
+ * @NL802154_COORD_ADDR: coordinator address, (8 bytes or 2 bytes)
+ * @NL802154_COORD_CHANNEL: channel number, related to @NL802154_COORD_PAGE (u8)
+ * @NL802154_COORD_PAGE: channel page, related to @NL802154_COORD_CHANNEL (u8)
+ * @NL802154_COORD_PREAMBLE_CODE: Preamble code used when the beacon was received,
+ *	this is PHY dependent and optional (u8)
+ * @NL802154_COORD_MEAN_PRF: Mean PRF used when the beacon was received,
+ *     this is PHY dependent and optional (u8)
+ * @NL802154_COORD_SUPERFRAME_SPEC: superframe specification of the PAN (u16)
+ * @NL802154_COORD_LINK_QUALITY: signal quality of beacon in unspecified units,
+ *	scaled to 0..255 (u8)
+ * @NL802154_COORD_GTS_PERMIT: set to true if GTS is permitted on this PAN
+ * @NL802154_COORD_PAYLOAD_DATA: binary data containing the raw data from the
+ *	frame payload, (only if beacon or probe response had data)
+ * @NL802154_COORD_PAD: attribute used for padding for 64-bit alignment
+ * @NL802154_COORD_MAX: highest coordinator attribute
+ */
+enum nl802154_coord {
+	__NL802154_COORD_INVALID,
+	NL802154_COORD_PANID,
+	NL802154_COORD_ADDR,
+	NL802154_COORD_CHANNEL,
+	NL802154_COORD_PAGE,
+	NL802154_COORD_PREAMBLE_CODE,
+	NL802154_COORD_MEAN_PRF,
+	NL802154_COORD_SUPERFRAME_SPEC,
+	NL802154_COORD_LINK_QUALITY,
+	NL802154_COORD_GTS_PERMIT,
+	NL802154_COORD_PAYLOAD_DATA,
+	NL802154_COORD_PAD,
+
+	/* keep last */
+	NL802154_COORD_MAX,
+};
+
 /**
  * enum nl802154_cca_modes - cca modes
  *
diff --git a/net/ieee802154/nl802154.c b/net/ieee802154/nl802154.c
index e0b072aecf0f..f6fb7a228747 100644
--- a/net/ieee802154/nl802154.c
+++ b/net/ieee802154/nl802154.c
@@ -26,10 +26,12 @@ static struct genl_family nl802154_fam;
 /* multicast groups */
 enum nl802154_multicast_groups {
 	NL802154_MCGRP_CONFIG,
+	NL802154_MCGRP_SCAN,
 };
 
 static const struct genl_multicast_group nl802154_mcgrps[] = {
 	[NL802154_MCGRP_CONFIG] = { .name = "config", },
+	[NL802154_MCGRP_SCAN] = { .name = "scan", },
 };
 
 /* returns ERR_PTR values */
@@ -216,6 +218,9 @@ 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_COORDINATOR] = { .type = NLA_NESTED },
+
 #ifdef CONFIG_IEEE802154_NL802154_EXPERIMENTAL
 	[NL802154_ATTR_SEC_ENABLED] = { .type = NLA_U8, },
 	[NL802154_ATTR_SEC_OUT_LEVEL] = { .type = NLA_U32, },
@@ -1281,6 +1286,122 @@ static int nl802154_wpan_phy_netns(struct sk_buff *skb, struct genl_info *info)
 	return err;
 }
 
+static int nl802154_prep_new_coord_msg(struct sk_buff *msg,
+				       struct cfg802154_registered_device *rdev,
+				       struct wpan_dev *wpan_dev,
+				       u32 portid, u32 seq, int flags, u8 cmd,
+				       struct ieee802154_coord_desc *desc)
+{
+	struct nlattr *nla;
+	void *hdr;
+
+	hdr = nl802154hdr_put(msg, portid, seq, flags, cmd);
+	if (!hdr)
+		return -ENOBUFS;
+
+	if (nla_put_u32(msg, NL802154_ATTR_WPAN_PHY, rdev->wpan_phy_idx))
+		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_COORDINATOR);
+	if (!nla)
+		goto nla_put_failure;
+
+	if (nla_put(msg, NL802154_COORD_PANID, IEEE802154_PAN_ID_LEN,
+		    &desc->addr->pan_id))
+		goto nla_put_failure;
+
+	if (desc->addr->mode == IEEE802154_ADDR_SHORT) {
+		if (nla_put(msg, NL802154_COORD_ADDR,
+			    IEEE802154_SHORT_ADDR_LEN,
+			    &desc->addr->short_addr))
+			goto nla_put_failure;
+	} else {
+		if (nla_put(msg, NL802154_COORD_ADDR,
+			    IEEE802154_EXTENDED_ADDR_LEN,
+			    &desc->addr->extended_addr))
+			goto nla_put_failure;
+	}
+
+	if (nla_put_u8(msg, NL802154_COORD_CHANNEL, desc->channel))
+		goto nla_put_failure;
+
+	if (nla_put_u8(msg, NL802154_COORD_PAGE, desc->page))
+		goto nla_put_failure;
+
+	if (nla_put_u16(msg, NL802154_COORD_SUPERFRAME_SPEC,
+			desc->superframe_spec))
+		goto nla_put_failure;
+
+	if (nla_put_u8(msg, NL802154_COORD_LINK_QUALITY, desc->link_quality))
+		goto nla_put_failure;
+
+	if (desc->gts_permit && nla_put_flag(msg, NL802154_COORD_GTS_PERMIT))
+		goto nla_put_failure;
+
+	/* TODO: NL802154_COORD_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_advertise_coordinator(struct wpan_phy *wpan_phy,
+					  struct wpan_dev *wpan_dev, u8 cmd,
+					  struct ieee802154_coord_desc *desc)
+{
+	struct cfg802154_registered_device *rdev = wpan_phy_to_rdev(wpan_phy);
+	struct sk_buff *msg;
+	int ret;
+
+	msg = nlmsg_new(NLMSG_DEFAULT_SIZE, GFP_ATOMIC);
+	if (!msg)
+		return -ENOMEM;
+
+	ret = nl802154_prep_new_coord_msg(msg, rdev, wpan_dev, 0, 0, 0, cmd, desc);
+	if (ret < 0) {
+		nlmsg_free(msg);
+		return ret;
+	}
+
+	return genlmsg_multicast_netns(&nl802154_fam, wpan_phy_net(wpan_phy),
+				       msg, 0, NL802154_MCGRP_SCAN, GFP_ATOMIC);
+}
+
+int nl802154_advertise_new_coordinator(struct wpan_phy *wpan_phy,
+				       struct wpan_dev *wpan_dev,
+				       struct ieee802154_coord_desc *desc)
+{
+	return nl802154_advertise_coordinator(wpan_phy, wpan_dev,
+					      NL802154_CMD_NEW_COORDINATOR,
+					      desc);
+}
+EXPORT_SYMBOL_GPL(nl802154_advertise_new_coordinator);
+
+int nl802154_advertise_known_coordinator(struct wpan_phy *wpan_phy,
+					 struct wpan_dev *wpan_dev,
+					 struct ieee802154_coord_desc *desc)
+{
+	return nl802154_advertise_coordinator(wpan_phy, wpan_dev,
+					      NL802154_CMD_KNOWN_COORDINATOR,
+					      desc);
+}
+EXPORT_SYMBOL_GPL(nl802154_advertise_known_coordinator);
+
 #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 },
diff --git a/net/ieee802154/nl802154.h b/net/ieee802154/nl802154.h
index 8c4b6d08954c..607af197fa0a 100644
--- a/net/ieee802154/nl802154.h
+++ b/net/ieee802154/nl802154.h
@@ -4,5 +4,11 @@
 
 int nl802154_init(void);
 void nl802154_exit(void);
+int nl802154_advertise_new_coordinator(struct wpan_phy *wpan_phy,
+				       struct wpan_dev *wpan_dev,
+				       struct ieee802154_coord_desc *desc);
+int nl802154_advertise_known_coordinator(struct wpan_phy *wpan_phy,
+					 struct wpan_dev *wpan_dev,
+					 struct ieee802154_coord_desc *desc);
 
 #endif /* __IEEE802154_NL802154_H */
-- 
2.34.1

[PATCH wpan-next 2/3] ieee802154: Handle coordinators discovery

[Miquel Raynal]

Let's introduce helpers for giving the MAC layer a generic interface for
advertising discovered coordinators/PANs upon beacon reception. This
support requires the MAC layers to:
- Allocate a coordinator/PAN descriptor and fill it.
- Register this structure, giving the generic ieee802154 layer the
  necessary information about the coordinator/PAN the beacon originates
  from.
- To flush all the allocated structures once the scan is done.

The generic layer keeps a temporary list of the discovered coordinators
to tell the user whether or not the beacon comes from a new device or
not, so stateless userspace applications might not spam the user with
identical information if not required.

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   |  12 ++++
 net/ieee802154/Makefile   |   2 +-
 net/ieee802154/core.c     |   2 +
 net/ieee802154/nl802154.c |   2 +
 net/ieee802154/pan.c      | 114 ++++++++++++++++++++++++++++++++++++++
 5 files changed, 131 insertions(+), 1 deletion(-)
 create mode 100644 net/ieee802154/pan.c

diff --git a/include/net/cfg802154.h b/include/net/cfg802154.h
index 8d67d9ed438d..3057b4e0726c 100644
--- a/include/net/cfg802154.h
+++ b/include/net/cfg802154.h
@@ -401,6 +401,10 @@ struct wpan_dev {
 
 	/* fallback for acknowledgment bit setting */
 	bool ackreq;
+
+	/* Coordinators management during scans */
+	spinlock_t coord_list_lock;
+	struct list_head coord_list;
 };
 
 #define to_phy(_dev)	container_of(_dev, struct wpan_phy, dev)
@@ -451,4 +455,12 @@ static inline const char *wpan_phy_name(struct wpan_phy *phy)
 
 void ieee802154_configure_durations(struct wpan_phy *phy);
 
+struct ieee802154_coord_desc *
+cfg802154_alloc_coordinator(struct ieee802154_addr *coord, gfp_t gfp);
+void cfg802154_free_coordinator_desc(struct ieee802154_coord_desc *desc);
+void cfg802154_record_coordinator(struct wpan_phy *wpan_phy,
+				  struct wpan_dev *wpan_dev,
+				  struct ieee802154_coord_desc *desc);
+void cfg802154_flush_known_coordinators(struct wpan_dev *wpan_dev);
+
 #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..091eb467fde6 100644
--- a/net/ieee802154/core.c
+++ b/net/ieee802154/core.c
@@ -276,6 +276,8 @@ static int cfg802154_netdev_notifier_call(struct notifier_block *nb,
 		wpan_dev->identifier = ++rdev->wpan_dev_id;
 		list_add_rcu(&wpan_dev->list, &rdev->wpan_dev_list);
 		rdev->devlist_generation++;
+		spin_lock_init(&wpan_dev->coord_list_lock);
+		INIT_LIST_HEAD(&wpan_dev->coord_list);
 
 		wpan_dev->netdev = dev;
 		break;
diff --git a/net/ieee802154/nl802154.c b/net/ieee802154/nl802154.c
index f6fb7a228747..b6bd04fe160b 100644
--- a/net/ieee802154/nl802154.c
+++ b/net/ieee802154/nl802154.c
@@ -1368,6 +1368,8 @@ static int nl802154_advertise_coordinator(struct wpan_phy *wpan_phy,
 	struct sk_buff *msg;
 	int ret;
 
+	lockdep_assert(&wpan_dev->coord_list_lock);
+
 	msg = nlmsg_new(NLMSG_DEFAULT_SIZE, GFP_ATOMIC);
 	if (!msg)
 		return -ENOMEM;
diff --git a/net/ieee802154/pan.c b/net/ieee802154/pan.c
new file mode 100644
index 000000000000..0d4f752a090a
--- /dev/null
+++ b/net/ieee802154/pan.c
@@ -0,0 +1,114 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * IEEE 802.15.4 PAN management
+ *
+ * Copyright (C) 2021 Qorvo US, Inc
+ * 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 "../ieee802154/nl802154.h"
+
+struct ieee802154_coord_desc *
+cfg802154_alloc_coordinator(struct ieee802154_addr *coord, gfp_t gfp)
+{
+	struct ieee802154_coord_desc *desc;
+
+	desc = kzalloc(sizeof(*desc), gfp);
+	if (!desc)
+		return ERR_PTR(-ENOMEM);
+
+	desc->addr = kzalloc(sizeof(*coord), gfp);
+	if (!desc->addr) {
+		kfree(desc);
+		return ERR_PTR(-ENOMEM);
+	}
+
+	memcpy(desc->addr, coord, sizeof(*coord));
+
+	return desc;
+}
+EXPORT_SYMBOL_GPL(cfg802154_alloc_coordinator);
+
+void cfg802154_free_coordinator_desc(struct ieee802154_coord_desc *desc)
+{
+	kfree(desc->addr);
+	kfree(desc);
+}
+EXPORT_SYMBOL_GPL(cfg802154_free_coordinator_desc);
+
+static bool
+cfg802154_is_same_coordinator(struct ieee802154_coord_desc *a,
+			      struct ieee802154_coord_desc *b)
+{
+	if (a->addr->pan_id != b->addr->pan_id)
+		return false;
+
+	if (a->addr->mode != b->addr->mode)
+		return false;
+
+	if (a->addr->mode == IEEE802154_ADDR_SHORT &&
+	    a->addr->short_addr == b->addr->short_addr)
+		return true;
+	else if (a->addr->mode == IEEE802154_ADDR_LONG &&
+		 a->addr->extended_addr == b->addr->extended_addr)
+		return true;
+
+	return false;
+}
+
+static bool
+cfg802154_coordinator_is_known(struct wpan_dev *wpan_dev,
+			       struct ieee802154_coord_desc *desc)
+{
+	struct ieee802154_coord_desc *item;
+
+	list_for_each_entry(item, &wpan_dev->coord_list, node)
+		if (cfg802154_is_same_coordinator(item, desc))
+			return true;
+
+	return false;
+}
+
+void cfg802154_record_coordinator(struct wpan_phy *wpan_phy,
+				  struct wpan_dev *wpan_dev,
+				  struct ieee802154_coord_desc *desc)
+{
+	spin_lock_bh(&wpan_dev->coord_list_lock);
+
+	if (cfg802154_coordinator_is_known(wpan_dev, desc)) {
+		nl802154_advertise_known_coordinator(wpan_phy, wpan_dev, desc);
+		cfg802154_free_coordinator_desc(desc);
+	} else {
+		list_add_tail(&desc->node, &wpan_dev->coord_list);
+		nl802154_advertise_new_coordinator(wpan_phy, wpan_dev, desc);
+	}
+
+	spin_unlock_bh(&wpan_dev->coord_list_lock);
+}
+EXPORT_SYMBOL_GPL(cfg802154_record_coordinator);
+
+void cfg802154_flush_known_coordinators(struct wpan_dev *wpan_dev)
+{
+	struct ieee802154_coord_desc *desc, *tmp;
+
+	spin_lock_bh(&wpan_dev->coord_list_lock);
+
+	list_for_each_entry_safe(desc, tmp, &wpan_dev->coord_list, node) {
+		list_del(&desc->node);
+		cfg802154_free_coordinator_desc(desc);
+	}
+
+	spin_unlock_bh(&wpan_dev->coord_list_lock);
+}
+EXPORT_SYMBOL_GPL(cfg802154_flush_known_coordinators);
-- 
2.34.1

[PATCH wpan-next 3/3] 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   |  2 ++
 net/ieee802154/trace.h | 25 +++++++++++++++++++++++++
 2 files changed, 27 insertions(+)

diff --git a/net/ieee802154/pan.c b/net/ieee802154/pan.c
index 0d4f752a090a..b41fe173edd3 100644
--- a/net/ieee802154/pan.c
+++ b/net/ieee802154/pan.c
@@ -18,6 +18,7 @@
 
 #include "ieee802154.h"
 #include "../ieee802154/nl802154.h"
+#include "trace.h"
 
 struct ieee802154_coord_desc *
 cfg802154_alloc_coordinator(struct ieee802154_addr *coord, gfp_t gfp)
@@ -91,6 +92,7 @@ void cfg802154_record_coordinator(struct wpan_phy *wpan_phy,
 		cfg802154_free_coordinator_desc(desc);
 	} else {
 		list_add_tail(&desc->node, &wpan_dev->coord_list);
+		trace_802154_new_coordinator(desc);
 		nl802154_advertise_new_coordinator(wpan_phy, wpan_dev, desc);
 	}
 
diff --git a/net/ieee802154/trace.h b/net/ieee802154/trace.h
index 19c2e5d60e76..03b3817c34ad 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_new_coordinator_evt,
+	TP_PROTO(struct ieee802154_coord_desc *desc),
+	TP_ARGS(desc),
+	TP_STRUCT__entry(
+		__field(__le16, pan_id)
+		__field(__le64, addr)
+		__field(u8, channel)
+		__field(u8, page)
+	),
+	TP_fast_assign(
+		__entry->page = desc->page;
+		__entry->channel = desc->channel;
+		__entry->pan_id = desc->addr->pan_id;
+		__entry->addr = desc->addr->extended_addr;
+	),
+	TP_printk("panid: %u, coord_addr: 0x%llx, page: %u, channel: %u",
+		  __le16_to_cpu(__entry->pan_id), __le64_to_cpu(__entry->addr),
+		  __entry->page, __entry->channel)
+);
+
+DEFINE_EVENT(802154_new_coordinator_evt, 802154_new_coordinator,
+	TP_PROTO(struct ieee802154_coord_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

Re: [PATCH wpan-next 3/3] ieee802154: Trace the registration of new PANs

[Alexander Aring]

Hi,

On Wed, Nov 2, 2022 at 11:20 AM Miquel Raynal <miquel.raynal@bootlin.com> wrote:
>
> From: David Girault <david.girault@qorvo.com>
>
> Add an internal trace when new PANs get discovered.

I guess this will not be the API for the user that there is a PAN
discovered? Is it only for debugging purposes? There will be an
nl802154 event for this? As we discussed previously with additional
commands for start, discovered, etc.?

I am sorry, maybe I can read that somewhere on your previous patch
series, just want to be sure.

- Alex

Re: [PATCH wpan-next 1/3] ieee802154: Advertize coordinators discovery

[Alexander Aring]

Hi,

On Wed, Nov 2, 2022 at 11:20 AM Miquel Raynal <miquel.raynal@bootlin.com> wrote:
>
> Let's introduce the basics for advertizing discovered PANs and
> coordinators, which is:
> - A new "scan" netlink message group.
> - A couple of netlink command/attribute.
> - The main netlink helper to send a netlink message with all the
>   necessary information to forward the main information to the user.
>
> Two netlink attributes are proactively added to support future UWB
> complex channels, but are not actually used yet.
>
> 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   |  20 +++++++
>  include/net/nl802154.h    |  44 ++++++++++++++
>  net/ieee802154/nl802154.c | 121 ++++++++++++++++++++++++++++++++++++++
>  net/ieee802154/nl802154.h |   6 ++
>  4 files changed, 191 insertions(+)
>
> diff --git a/include/net/cfg802154.h b/include/net/cfg802154.h
> index e1481f9cf049..8d67d9ed438d 100644
> --- a/include/net/cfg802154.h
> +++ b/include/net/cfg802154.h
> @@ -260,6 +260,26 @@ struct ieee802154_addr {
>         };
>  };
>
> +/**
> + * struct ieee802154_coord_desc - Coordinator descriptor
> + * @coord: PAN ID and coordinator address
> + * @page: page this coordinator is using
> + * @channel: channel this coordinator is using
> + * @superframe_spec: SuperFrame specification as received
> + * @link_quality: link quality indicator at which the beacon was received
> + * @gts_permit: the coordinator accepts GTS requests
> + * @node: list item
> + */
> +struct ieee802154_coord_desc {
> +       struct ieee802154_addr *addr;

Why is this a pointer?

> +       u8 page;
> +       u8 channel;
> +       u16 superframe_spec;
> +       u8 link_quality;
> +       bool gts_permit;
> +       struct list_head node;
> +};
> +
>  struct ieee802154_llsec_key_id {
>         u8 mode;
>         u8 id;
> diff --git a/include/net/nl802154.h b/include/net/nl802154.h
> index 145acb8f2509..cfe462288695 100644
> --- a/include/net/nl802154.h
> +++ b/include/net/nl802154.h
> @@ -58,6 +58,9 @@ enum nl802154_commands {
>
>         NL802154_CMD_SET_WPAN_PHY_NETNS,
>
> +       NL802154_CMD_NEW_COORDINATOR,
> +       NL802154_CMD_KNOWN_COORDINATOR,
> +

NEW is something we never saw before and KNOWN we already saw before?
I am not getting that when I just want to maintain a list in the user
space and keep them updated, but I think we had this discussion
already or? Currently they do the same thing, just the command is
different. The user can use it to filter NEW and KNOWN? Still I am not
getting it why there is not just a start ... event, event, event ....
end. and let the user decide if it knows that it's new or old from its
perspective.

>         /* add new commands above here */
>
>  #ifdef CONFIG_IEEE802154_NL802154_EXPERIMENTAL
> @@ -133,6 +136,8 @@ enum nl802154_attrs {
>         NL802154_ATTR_PID,
>         NL802154_ATTR_NETNS_FD,
>
> +       NL802154_ATTR_COORDINATOR,
> +
>         /* add attributes here, update the policy in nl802154.c */
>
>  #ifdef CONFIG_IEEE802154_NL802154_EXPERIMENTAL
> @@ -218,6 +223,45 @@ enum nl802154_wpan_phy_capability_attr {
>         NL802154_CAP_ATTR_MAX = __NL802154_CAP_ATTR_AFTER_LAST - 1
>  };
>
> +/**
> + * enum nl802154_coord - Netlink attributes for a coord
> + *
> + * @__NL802154_COORD_INVALID: invalid
> + * @NL802154_COORD_PANID: PANID of the coordinator (2 bytes)
> + * @NL802154_COORD_ADDR: coordinator address, (8 bytes or 2 bytes)
> + * @NL802154_COORD_CHANNEL: channel number, related to @NL802154_COORD_PAGE (u8)
> + * @NL802154_COORD_PAGE: channel page, related to @NL802154_COORD_CHANNEL (u8)
> + * @NL802154_COORD_PREAMBLE_CODE: Preamble code used when the beacon was received,
> + *     this is PHY dependent and optional (u8)
> + * @NL802154_COORD_MEAN_PRF: Mean PRF used when the beacon was received,
> + *     this is PHY dependent and optional (u8)
> + * @NL802154_COORD_SUPERFRAME_SPEC: superframe specification of the PAN (u16)
> + * @NL802154_COORD_LINK_QUALITY: signal quality of beacon in unspecified units,
> + *     scaled to 0..255 (u8)
> + * @NL802154_COORD_GTS_PERMIT: set to true if GTS is permitted on this PAN
> + * @NL802154_COORD_PAYLOAD_DATA: binary data containing the raw data from the
> + *     frame payload, (only if beacon or probe response had data)
> + * @NL802154_COORD_PAD: attribute used for padding for 64-bit alignment
> + * @NL802154_COORD_MAX: highest coordinator attribute
> + */
> +enum nl802154_coord {
> +       __NL802154_COORD_INVALID,
> +       NL802154_COORD_PANID,
> +       NL802154_COORD_ADDR,
> +       NL802154_COORD_CHANNEL,
> +       NL802154_COORD_PAGE,
> +       NL802154_COORD_PREAMBLE_CODE,

Interesting, if you do a scan and discover pans and others answers I
would think you would see only pans on the same preamble. How is this
working?

> +       NL802154_COORD_MEAN_PRF,
> +       NL802154_COORD_SUPERFRAME_SPEC,
> +       NL802154_COORD_LINK_QUALITY,

not against it to have it, it's fine. I just think it is not very
useful. A way to dump all LQI values with some timestamp and having
something in user space to collect stats and do some heuristic may be
better?

> +       NL802154_COORD_GTS_PERMIT,
> +       NL802154_COORD_PAYLOAD_DATA,
> +       NL802154_COORD_PAD,
> +
> +       /* keep last */
> +       NL802154_COORD_MAX,
> +};
> +
>  /**
>   * enum nl802154_cca_modes - cca modes
>   *
> diff --git a/net/ieee802154/nl802154.c b/net/ieee802154/nl802154.c
> index e0b072aecf0f..f6fb7a228747 100644
> --- a/net/ieee802154/nl802154.c
> +++ b/net/ieee802154/nl802154.c
> @@ -26,10 +26,12 @@ static struct genl_family nl802154_fam;
>  /* multicast groups */
>  enum nl802154_multicast_groups {
>         NL802154_MCGRP_CONFIG,
> +       NL802154_MCGRP_SCAN,
>  };
>
>  static const struct genl_multicast_group nl802154_mcgrps[] = {
>         [NL802154_MCGRP_CONFIG] = { .name = "config", },
> +       [NL802154_MCGRP_SCAN] = { .name = "scan", },
>  };
>
>  /* returns ERR_PTR values */
> @@ -216,6 +218,9 @@ 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_COORDINATOR] = { .type = NLA_NESTED },
> +
>  #ifdef CONFIG_IEEE802154_NL802154_EXPERIMENTAL
>         [NL802154_ATTR_SEC_ENABLED] = { .type = NLA_U8, },
>         [NL802154_ATTR_SEC_OUT_LEVEL] = { .type = NLA_U32, },
> @@ -1281,6 +1286,122 @@ static int nl802154_wpan_phy_netns(struct sk_buff *skb, struct genl_info *info)
>         return err;
>  }
>
> +static int nl802154_prep_new_coord_msg(struct sk_buff *msg,
> +                                      struct cfg802154_registered_device *rdev,
> +                                      struct wpan_dev *wpan_dev,
> +                                      u32 portid, u32 seq, int flags, u8 cmd,
> +                                      struct ieee802154_coord_desc *desc)
> +{
> +       struct nlattr *nla;
> +       void *hdr;
> +
> +       hdr = nl802154hdr_put(msg, portid, seq, flags, cmd);
> +       if (!hdr)
> +               return -ENOBUFS;
> +
> +       if (nla_put_u32(msg, NL802154_ATTR_WPAN_PHY, rdev->wpan_phy_idx))
> +               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_COORDINATOR);
> +       if (!nla)
> +               goto nla_put_failure;
> +
> +       if (nla_put(msg, NL802154_COORD_PANID, IEEE802154_PAN_ID_LEN,
> +                   &desc->addr->pan_id))
> +               goto nla_put_failure;
> +
> +       if (desc->addr->mode == IEEE802154_ADDR_SHORT) {
> +               if (nla_put(msg, NL802154_COORD_ADDR,
> +                           IEEE802154_SHORT_ADDR_LEN,
> +                           &desc->addr->short_addr))
> +                       goto nla_put_failure;
> +       } else {
> +               if (nla_put(msg, NL802154_COORD_ADDR,
> +                           IEEE802154_EXTENDED_ADDR_LEN,
> +                           &desc->addr->extended_addr))
> +                       goto nla_put_failure;
> +       }
> +
> +       if (nla_put_u8(msg, NL802154_COORD_CHANNEL, desc->channel))
> +               goto nla_put_failure;
> +
> +       if (nla_put_u8(msg, NL802154_COORD_PAGE, desc->page))
> +               goto nla_put_failure;
> +
> +       if (nla_put_u16(msg, NL802154_COORD_SUPERFRAME_SPEC,
> +                       desc->superframe_spec))
> +               goto nla_put_failure;
> +
> +       if (nla_put_u8(msg, NL802154_COORD_LINK_QUALITY, desc->link_quality))
> +               goto nla_put_failure;
> +
> +       if (desc->gts_permit && nla_put_flag(msg, NL802154_COORD_GTS_PERMIT))
> +               goto nla_put_failure;
> +
> +       /* TODO: NL802154_COORD_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_advertise_coordinator(struct wpan_phy *wpan_phy,
> +                                         struct wpan_dev *wpan_dev, u8 cmd,
> +                                         struct ieee802154_coord_desc *desc)
> +{
> +       struct cfg802154_registered_device *rdev = wpan_phy_to_rdev(wpan_phy);
> +       struct sk_buff *msg;
> +       int ret;
> +
> +       msg = nlmsg_new(NLMSG_DEFAULT_SIZE, GFP_ATOMIC);
> +       if (!msg)
> +               return -ENOMEM;
> +
> +       ret = nl802154_prep_new_coord_msg(msg, rdev, wpan_dev, 0, 0, 0, cmd, desc);
> +       if (ret < 0) {
> +               nlmsg_free(msg);
> +               return ret;
> +       }
> +
> +       return genlmsg_multicast_netns(&nl802154_fam, wpan_phy_net(wpan_phy),
> +                                      msg, 0, NL802154_MCGRP_SCAN, GFP_ATOMIC);
> +}

ah, okay that answers my previous question... regarding the trace event.

- Alex

Re: [PATCH wpan-next 2/3] ieee802154: Handle coordinators discovery

[Alexander Aring]

Hi,

On Wed, Nov 2, 2022 at 11:20 AM Miquel Raynal <miquel.raynal@bootlin.com> wrote:
>
> Let's introduce helpers for giving the MAC layer a generic interface for
> advertising discovered coordinators/PANs upon beacon reception. This
> support requires the MAC layers to:
> - Allocate a coordinator/PAN descriptor and fill it.
> - Register this structure, giving the generic ieee802154 layer the
>   necessary information about the coordinator/PAN the beacon originates
>   from.
> - To flush all the allocated structures once the scan is done.
>
> The generic layer keeps a temporary list of the discovered coordinators
> to tell the user whether or not the beacon comes from a new device or
> not, so stateless userspace applications might not spam the user with
> identical information if not required.
>
> 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   |  12 ++++
>  net/ieee802154/Makefile   |   2 +-
>  net/ieee802154/core.c     |   2 +
>  net/ieee802154/nl802154.c |   2 +
>  net/ieee802154/pan.c      | 114 ++++++++++++++++++++++++++++++++++++++
>  5 files changed, 131 insertions(+), 1 deletion(-)
>  create mode 100644 net/ieee802154/pan.c
>
> diff --git a/include/net/cfg802154.h b/include/net/cfg802154.h
> index 8d67d9ed438d..3057b4e0726c 100644
> --- a/include/net/cfg802154.h
> +++ b/include/net/cfg802154.h
> @@ -401,6 +401,10 @@ struct wpan_dev {
>
>         /* fallback for acknowledgment bit setting */
>         bool ackreq;
> +
> +       /* Coordinators management during scans */
> +       spinlock_t coord_list_lock;
> +       struct list_head coord_list;
>  };
>
>  #define to_phy(_dev)   container_of(_dev, struct wpan_phy, dev)
> @@ -451,4 +455,12 @@ static inline const char *wpan_phy_name(struct wpan_phy *phy)
>
>  void ieee802154_configure_durations(struct wpan_phy *phy);
>
> +struct ieee802154_coord_desc *
> +cfg802154_alloc_coordinator(struct ieee802154_addr *coord, gfp_t gfp);
> +void cfg802154_free_coordinator_desc(struct ieee802154_coord_desc *desc);
> +void cfg802154_record_coordinator(struct wpan_phy *wpan_phy,
> +                                 struct wpan_dev *wpan_dev,
> +                                 struct ieee802154_coord_desc *desc);
> +void cfg802154_flush_known_coordinators(struct wpan_dev *wpan_dev);
> +
>  #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..091eb467fde6 100644
> --- a/net/ieee802154/core.c
> +++ b/net/ieee802154/core.c
> @@ -276,6 +276,8 @@ static int cfg802154_netdev_notifier_call(struct notifier_block *nb,
>                 wpan_dev->identifier = ++rdev->wpan_dev_id;
>                 list_add_rcu(&wpan_dev->list, &rdev->wpan_dev_list);
>                 rdev->devlist_generation++;
> +               spin_lock_init(&wpan_dev->coord_list_lock);
> +               INIT_LIST_HEAD(&wpan_dev->coord_list);
>
>                 wpan_dev->netdev = dev;
>                 break;
> diff --git a/net/ieee802154/nl802154.c b/net/ieee802154/nl802154.c
> index f6fb7a228747..b6bd04fe160b 100644
> --- a/net/ieee802154/nl802154.c
> +++ b/net/ieee802154/nl802154.c
> @@ -1368,6 +1368,8 @@ static int nl802154_advertise_coordinator(struct wpan_phy *wpan_phy,
>         struct sk_buff *msg;
>         int ret;
>
> +       lockdep_assert(&wpan_dev->coord_list_lock);
> +
>         msg = nlmsg_new(NLMSG_DEFAULT_SIZE, GFP_ATOMIC);
>         if (!msg)
>                 return -ENOMEM;
> diff --git a/net/ieee802154/pan.c b/net/ieee802154/pan.c
> new file mode 100644
> index 000000000000..0d4f752a090a
> --- /dev/null
> +++ b/net/ieee802154/pan.c
> @@ -0,0 +1,114 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * IEEE 802.15.4 PAN management
> + *
> + * Copyright (C) 2021 Qorvo US, Inc
> + * 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 "../ieee802154/nl802154.h"
> +
> +struct ieee802154_coord_desc *
> +cfg802154_alloc_coordinator(struct ieee802154_addr *coord, gfp_t gfp)
> +{
> +       struct ieee802154_coord_desc *desc;
> +
> +       desc = kzalloc(sizeof(*desc), gfp);
> +       if (!desc)
> +               return ERR_PTR(-ENOMEM);
> +
> +       desc->addr = kzalloc(sizeof(*coord), gfp);
> +       if (!desc->addr) {
> +               kfree(desc);
> +               return ERR_PTR(-ENOMEM);
> +       }
> +
> +       memcpy(desc->addr, coord, sizeof(*coord));
> +
> +       return desc;
> +}
> +EXPORT_SYMBOL_GPL(cfg802154_alloc_coordinator);
> +
> +void cfg802154_free_coordinator_desc(struct ieee802154_coord_desc *desc)
> +{
> +       kfree(desc->addr);
> +       kfree(desc);
> +}
> +EXPORT_SYMBOL_GPL(cfg802154_free_coordinator_desc);
> +
> +static bool
> +cfg802154_is_same_coordinator(struct ieee802154_coord_desc *a,
> +                             struct ieee802154_coord_desc *b)
> +{
> +       if (a->addr->pan_id != b->addr->pan_id)
> +               return false;
> +
> +       if (a->addr->mode != b->addr->mode)
> +               return false;
> +
> +       if (a->addr->mode == IEEE802154_ADDR_SHORT &&
> +           a->addr->short_addr == b->addr->short_addr)
> +               return true;
> +       else if (a->addr->mode == IEEE802154_ADDR_LONG &&
> +                a->addr->extended_addr == b->addr->extended_addr)
> +               return true;
> +
> +       return false;

semantic is a little bit different, can we use "ieee802154_addr_equal()" here?

- Alex

Re: [PATCH wpan-next 2/3] ieee802154: Handle coordinators discovery

[Miquel Raynal]

Hi Alexander,

> > +static bool
> > +cfg802154_is_same_coordinator(struct ieee802154_coord_desc *a,
> > +                             struct ieee802154_coord_desc *b)
> > +{
> > +       if (a->addr->pan_id != b->addr->pan_id)
> > +               return false;
> > +
> > +       if (a->addr->mode != b->addr->mode)
> > +               return false;
> > +
> > +       if (a->addr->mode == IEEE802154_ADDR_SHORT &&
> > +           a->addr->short_addr == b->addr->short_addr)
> > +               return true;
> > +       else if (a->addr->mode == IEEE802154_ADDR_LONG &&
> > +                a->addr->extended_addr == b->addr->extended_addr)
> > +               return true;
> > +
> > +       return false;  
> 
> semantic is a little bit different, can we use "ieee802154_addr_equal()" here?

No problem, I will.

Thanks,
Miquèl

Re: [PATCH wpan-next 3/3] ieee802154: Trace the registration of new PANs

[Miquel Raynal]

Hi Alexander,

aahringo@redhat.com wrote on Sun, 6 Nov 2022 20:36:21 -0500:

> Hi,
> 
> On Wed, Nov 2, 2022 at 11:20 AM Miquel Raynal <miquel.raynal@bootlin.com> wrote:
> >
> > From: David Girault <david.girault@qorvo.com>
> >
> > Add an internal trace when new PANs get discovered.  
> 
> I guess this will not be the API for the user that there is a PAN
> discovered? Is it only for debugging purposes? There will be an
> nl802154 event for this? As we discussed previously with additional
> commands for start, discovered, etc.?
> 
> I am sorry, maybe I can read that somewhere on your previous patch
> series, just want to be sure.

Yeah no problem, so yes, as you eventually saw in patch 1/3, the
internal tracing is just here for in-kernel debugging purposes, it's in
no way related to the user interface. It's a 2015 feature, we're just
adding support for tracing the new commands.

Let's discuss the nl user interface in the other thread.

Thanks,
Miquèl

Re: [PATCH wpan-next 1/3] ieee802154: Advertize coordinators discovery

[Miquel Raynal]

Hi Alexander,

aahringo@redhat.com wrote on Sun, 6 Nov 2022 21:01:35 -0500:

> Hi,
> 
> On Wed, Nov 2, 2022 at 11:20 AM Miquel Raynal <miquel.raynal@bootlin.com> wrote:
> >
> > Let's introduce the basics for advertizing discovered PANs and
> > coordinators, which is:
> > - A new "scan" netlink message group.
> > - A couple of netlink command/attribute.
> > - The main netlink helper to send a netlink message with all the
> >   necessary information to forward the main information to the user.
> >
> > Two netlink attributes are proactively added to support future UWB
> > complex channels, but are not actually used yet.
> >
> > 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   |  20 +++++++
> >  include/net/nl802154.h    |  44 ++++++++++++++
> >  net/ieee802154/nl802154.c | 121 ++++++++++++++++++++++++++++++++++++++
> >  net/ieee802154/nl802154.h |   6 ++
> >  4 files changed, 191 insertions(+)
> >
> > diff --git a/include/net/cfg802154.h b/include/net/cfg802154.h
> > index e1481f9cf049..8d67d9ed438d 100644
> > --- a/include/net/cfg802154.h
> > +++ b/include/net/cfg802154.h
> > @@ -260,6 +260,26 @@ struct ieee802154_addr {
> >         };
> >  };
> >
> > +/**
> > + * struct ieee802154_coord_desc - Coordinator descriptor
> > + * @coord: PAN ID and coordinator address
> > + * @page: page this coordinator is using
> > + * @channel: channel this coordinator is using
> > + * @superframe_spec: SuperFrame specification as received
> > + * @link_quality: link quality indicator at which the beacon was received
> > + * @gts_permit: the coordinator accepts GTS requests
> > + * @node: list item
> > + */
> > +struct ieee802154_coord_desc {
> > +       struct ieee802154_addr *addr;  
> 
> Why is this a pointer?

No reason anymore, I've changed this member to be a regular structure.

> 
> > +       u8 page;
> > +       u8 channel;
> > +       u16 superframe_spec;
> > +       u8 link_quality;
> > +       bool gts_permit;
> > +       struct list_head node;
> > +};
> > +
> >  struct ieee802154_llsec_key_id {
> >         u8 mode;
> >         u8 id;
> > diff --git a/include/net/nl802154.h b/include/net/nl802154.h
> > index 145acb8f2509..cfe462288695 100644
> > --- a/include/net/nl802154.h
> > +++ b/include/net/nl802154.h
> > @@ -58,6 +58,9 @@ enum nl802154_commands {
> >
> >         NL802154_CMD_SET_WPAN_PHY_NETNS,
> >
> > +       NL802154_CMD_NEW_COORDINATOR,
> > +       NL802154_CMD_KNOWN_COORDINATOR,
> > +  
> 
> NEW is something we never saw before and KNOWN we already saw before?
> I am not getting that when I just want to maintain a list in the user
> space and keep them updated, but I think we had this discussion
> already or? Currently they do the same thing, just the command is
> different. The user can use it to filter NEW and KNOWN? Still I am not
> getting it why there is not just a start ... event, event, event ....
> end. and let the user decide if it knows that it's new or old from its
> perspective.

Actually we already discussed this once and I personally liked more to
handle this in the kernel, but you seem to really prefer letting the
user space device whether or not the beacon is a new one or not, so
I've updated both the kernel side and the userspace side to act like
this.

> 
> >         /* add new commands above here */
> >
> >  #ifdef CONFIG_IEEE802154_NL802154_EXPERIMENTAL
> > @@ -133,6 +136,8 @@ enum nl802154_attrs {
> >         NL802154_ATTR_PID,
> >         NL802154_ATTR_NETNS_FD,
> >
> > +       NL802154_ATTR_COORDINATOR,
> > +
> >         /* add attributes here, update the policy in nl802154.c */
> >
> >  #ifdef CONFIG_IEEE802154_NL802154_EXPERIMENTAL
> > @@ -218,6 +223,45 @@ enum nl802154_wpan_phy_capability_attr {
> >         NL802154_CAP_ATTR_MAX = __NL802154_CAP_ATTR_AFTER_LAST - 1
> >  };
> >
> > +/**
> > + * enum nl802154_coord - Netlink attributes for a coord
> > + *
> > + * @__NL802154_COORD_INVALID: invalid
> > + * @NL802154_COORD_PANID: PANID of the coordinator (2 bytes)
> > + * @NL802154_COORD_ADDR: coordinator address, (8 bytes or 2 bytes)
> > + * @NL802154_COORD_CHANNEL: channel number, related to @NL802154_COORD_PAGE (u8)
> > + * @NL802154_COORD_PAGE: channel page, related to @NL802154_COORD_CHANNEL (u8)
> > + * @NL802154_COORD_PREAMBLE_CODE: Preamble code used when the beacon was received,
> > + *     this is PHY dependent and optional (u8)
> > + * @NL802154_COORD_MEAN_PRF: Mean PRF used when the beacon was received,
> > + *     this is PHY dependent and optional (u8)
> > + * @NL802154_COORD_SUPERFRAME_SPEC: superframe specification of the PAN (u16)
> > + * @NL802154_COORD_LINK_QUALITY: signal quality of beacon in unspecified units,
> > + *     scaled to 0..255 (u8)
> > + * @NL802154_COORD_GTS_PERMIT: set to true if GTS is permitted on this PAN
> > + * @NL802154_COORD_PAYLOAD_DATA: binary data containing the raw data from the
> > + *     frame payload, (only if beacon or probe response had data)
> > + * @NL802154_COORD_PAD: attribute used for padding for 64-bit alignment
> > + * @NL802154_COORD_MAX: highest coordinator attribute
> > + */
> > +enum nl802154_coord {
> > +       __NL802154_COORD_INVALID,
> > +       NL802154_COORD_PANID,
> > +       NL802154_COORD_ADDR,
> > +       NL802154_COORD_CHANNEL,
> > +       NL802154_COORD_PAGE,
> > +       NL802154_COORD_PREAMBLE_CODE,  
> 
> Interesting, if you do a scan and discover pans and others answers I
> would think you would see only pans on the same preamble. How is this
> working?

Yes this is how it is working, you only see PANs on one preamble at a
time. That's why we need to tell on which preamble we received the
beacon.

> 
> > +       NL802154_COORD_MEAN_PRF,
> > +       NL802154_COORD_SUPERFRAME_SPEC,
> > +       NL802154_COORD_LINK_QUALITY,  
> 
> not against it to have it, it's fine. I just think it is not very
> useful. A way to dump all LQI values with some timestamp and having
> something in user space to collect stats and do some heuristic may be
> better?

Actually I really like seeing this in the event logs in userspace, so if
you don't mind I'll keep this parameter. It can safely be ignored by the
userspace anyway, so I guess it does not hurt.

> 
> > +       NL802154_COORD_GTS_PERMIT,
> > +       NL802154_COORD_PAYLOAD_DATA,
> > +       NL802154_COORD_PAD,
> > +
> > +       /* keep last */
> > +       NL802154_COORD_MAX,
> > +};
> > +
> >  /**
> >   * enum nl802154_cca_modes - cca modes
> >   *
> > diff --git a/net/ieee802154/nl802154.c b/net/ieee802154/nl802154.c
> > index e0b072aecf0f..f6fb7a228747 100644
> > --- a/net/ieee802154/nl802154.c
> > +++ b/net/ieee802154/nl802154.c
> > @@ -26,10 +26,12 @@ static struct genl_family nl802154_fam;
> >  /* multicast groups */
> >  enum nl802154_multicast_groups {
> >         NL802154_MCGRP_CONFIG,
> > +       NL802154_MCGRP_SCAN,
> >  };
> >
> >  static const struct genl_multicast_group nl802154_mcgrps[] = {
> >         [NL802154_MCGRP_CONFIG] = { .name = "config", },
> > +       [NL802154_MCGRP_SCAN] = { .name = "scan", },
> >  };
> >
> >  /* returns ERR_PTR values */
> > @@ -216,6 +218,9 @@ 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_COORDINATOR] = { .type = NLA_NESTED },
> > +
> >  #ifdef CONFIG_IEEE802154_NL802154_EXPERIMENTAL
> >         [NL802154_ATTR_SEC_ENABLED] = { .type = NLA_U8, },
> >         [NL802154_ATTR_SEC_OUT_LEVEL] = { .type = NLA_U32, },
> > @@ -1281,6 +1286,122 @@ static int nl802154_wpan_phy_netns(struct sk_buff *skb, struct genl_info *info)
> >         return err;
> >  }
> >
> > +static int nl802154_prep_new_coord_msg(struct sk_buff *msg,
> > +                                      struct cfg802154_registered_device *rdev,
> > +                                      struct wpan_dev *wpan_dev,
> > +                                      u32 portid, u32 seq, int flags, u8 cmd,
> > +                                      struct ieee802154_coord_desc *desc)
> > +{
> > +       struct nlattr *nla;
> > +       void *hdr;
> > +
> > +       hdr = nl802154hdr_put(msg, portid, seq, flags, cmd);
> > +       if (!hdr)
> > +               return -ENOBUFS;
> > +
> > +       if (nla_put_u32(msg, NL802154_ATTR_WPAN_PHY, rdev->wpan_phy_idx))
> > +               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_COORDINATOR);
> > +       if (!nla)
> > +               goto nla_put_failure;
> > +
> > +       if (nla_put(msg, NL802154_COORD_PANID, IEEE802154_PAN_ID_LEN,
> > +                   &desc->addr->pan_id))
> > +               goto nla_put_failure;
> > +
> > +       if (desc->addr->mode == IEEE802154_ADDR_SHORT) {
> > +               if (nla_put(msg, NL802154_COORD_ADDR,
> > +                           IEEE802154_SHORT_ADDR_LEN,
> > +                           &desc->addr->short_addr))
> > +                       goto nla_put_failure;
> > +       } else {
> > +               if (nla_put(msg, NL802154_COORD_ADDR,
> > +                           IEEE802154_EXTENDED_ADDR_LEN,
> > +                           &desc->addr->extended_addr))
> > +                       goto nla_put_failure;
> > +       }
> > +
> > +       if (nla_put_u8(msg, NL802154_COORD_CHANNEL, desc->channel))
> > +               goto nla_put_failure;
> > +
> > +       if (nla_put_u8(msg, NL802154_COORD_PAGE, desc->page))
> > +               goto nla_put_failure;
> > +
> > +       if (nla_put_u16(msg, NL802154_COORD_SUPERFRAME_SPEC,
> > +                       desc->superframe_spec))
> > +               goto nla_put_failure;
> > +
> > +       if (nla_put_u8(msg, NL802154_COORD_LINK_QUALITY, desc->link_quality))
> > +               goto nla_put_failure;
> > +
> > +       if (desc->gts_permit && nla_put_flag(msg, NL802154_COORD_GTS_PERMIT))
> > +               goto nla_put_failure;
> > +
> > +       /* TODO: NL802154_COORD_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_advertise_coordinator(struct wpan_phy *wpan_phy,
> > +                                         struct wpan_dev *wpan_dev, u8 cmd,
> > +                                         struct ieee802154_coord_desc *desc)
> > +{
> > +       struct cfg802154_registered_device *rdev = wpan_phy_to_rdev(wpan_phy);
> > +       struct sk_buff *msg;
> > +       int ret;
> > +
> > +       msg = nlmsg_new(NLMSG_DEFAULT_SIZE, GFP_ATOMIC);
> > +       if (!msg)
> > +               return -ENOMEM;
> > +
> > +       ret = nl802154_prep_new_coord_msg(msg, rdev, wpan_dev, 0, 0, 0, cmd, desc);
> > +       if (ret < 0) {
> > +               nlmsg_free(msg);
> > +               return ret;
> > +       }
> > +
> > +       return genlmsg_multicast_netns(&nl802154_fam, wpan_phy_net(wpan_phy),
> > +                                      msg, 0, NL802154_MCGRP_SCAN, GFP_ATOMIC);
> > +}  
> 
> ah, okay that answers my previous question... regarding the trace event.
> 
> - Alex
> 


Thanks,
Miquèl

Re: [PATCH wpan-next 1/3] ieee802154: Advertize coordinators discovery

[Alexander Aring]

Hi,

On Fri, Nov 18, 2022 at 5:04 PM Miquel Raynal <miquel.raynal@bootlin.com> wrote:
>
> Hi Alexander,
>
> aahringo@redhat.com wrote on Sun, 6 Nov 2022 21:01:35 -0500:
>
> > Hi,
> >
> > On Wed, Nov 2, 2022 at 11:20 AM Miquel Raynal <miquel.raynal@bootlin.com> wrote:
> > >
> > > Let's introduce the basics for advertizing discovered PANs and
> > > coordinators, which is:
> > > - A new "scan" netlink message group.
> > > - A couple of netlink command/attribute.
> > > - The main netlink helper to send a netlink message with all the
> > >   necessary information to forward the main information to the user.
> > >
> > > Two netlink attributes are proactively added to support future UWB
> > > complex channels, but are not actually used yet.
> > >
> > > 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   |  20 +++++++
> > >  include/net/nl802154.h    |  44 ++++++++++++++
> > >  net/ieee802154/nl802154.c | 121 ++++++++++++++++++++++++++++++++++++++
> > >  net/ieee802154/nl802154.h |   6 ++
> > >  4 files changed, 191 insertions(+)
> > >
> > > diff --git a/include/net/cfg802154.h b/include/net/cfg802154.h
> > > index e1481f9cf049..8d67d9ed438d 100644
> > > --- a/include/net/cfg802154.h
> > > +++ b/include/net/cfg802154.h
> > > @@ -260,6 +260,26 @@ struct ieee802154_addr {
> > >         };
> > >  };
> > >
> > > +/**
> > > + * struct ieee802154_coord_desc - Coordinator descriptor
> > > + * @coord: PAN ID and coordinator address
> > > + * @page: page this coordinator is using
> > > + * @channel: channel this coordinator is using
> > > + * @superframe_spec: SuperFrame specification as received
> > > + * @link_quality: link quality indicator at which the beacon was received
> > > + * @gts_permit: the coordinator accepts GTS requests
> > > + * @node: list item
> > > + */
> > > +struct ieee802154_coord_desc {
> > > +       struct ieee802154_addr *addr;
> >
> > Why is this a pointer?
>
> No reason anymore, I've changed this member to be a regular structure.
>

ok.

> >
> > > +       u8 page;
> > > +       u8 channel;
> > > +       u16 superframe_spec;
> > > +       u8 link_quality;
> > > +       bool gts_permit;
> > > +       struct list_head node;
> > > +};
> > > +
> > >  struct ieee802154_llsec_key_id {
> > >         u8 mode;
> > >         u8 id;
> > > diff --git a/include/net/nl802154.h b/include/net/nl802154.h
> > > index 145acb8f2509..cfe462288695 100644
> > > --- a/include/net/nl802154.h
> > > +++ b/include/net/nl802154.h
> > > @@ -58,6 +58,9 @@ enum nl802154_commands {
> > >
> > >         NL802154_CMD_SET_WPAN_PHY_NETNS,
> > >
> > > +       NL802154_CMD_NEW_COORDINATOR,
> > > +       NL802154_CMD_KNOWN_COORDINATOR,
> > > +
> >
> > NEW is something we never saw before and KNOWN we already saw before?
> > I am not getting that when I just want to maintain a list in the user
> > space and keep them updated, but I think we had this discussion
> > already or? Currently they do the same thing, just the command is
> > different. The user can use it to filter NEW and KNOWN? Still I am not
> > getting it why there is not just a start ... event, event, event ....
> > end. and let the user decide if it knows that it's new or old from its
> > perspective.
>
> Actually we already discussed this once and I personally liked more to
> handle this in the kernel, but you seem to really prefer letting the
> user space device whether or not the beacon is a new one or not, so
> I've updated both the kernel side and the userspace side to act like
> this.
>

I thought there was some problem about when the "scan-op" is running
and there could be the case that the discovered PANs are twice there,
but this looks more like handling UAPI features as separate new and
old ones? I can see that there can be a need for the first case?

> >
> > >         /* add new commands above here */
> > >
> > >  #ifdef CONFIG_IEEE802154_NL802154_EXPERIMENTAL
> > > @@ -133,6 +136,8 @@ enum nl802154_attrs {
> > >         NL802154_ATTR_PID,
> > >         NL802154_ATTR_NETNS_FD,
> > >
> > > +       NL802154_ATTR_COORDINATOR,
> > > +
> > >         /* add attributes here, update the policy in nl802154.c */
> > >
> > >  #ifdef CONFIG_IEEE802154_NL802154_EXPERIMENTAL
> > > @@ -218,6 +223,45 @@ enum nl802154_wpan_phy_capability_attr {
> > >         NL802154_CAP_ATTR_MAX = __NL802154_CAP_ATTR_AFTER_LAST - 1
> > >  };
> > >
> > > +/**
> > > + * enum nl802154_coord - Netlink attributes for a coord
> > > + *
> > > + * @__NL802154_COORD_INVALID: invalid
> > > + * @NL802154_COORD_PANID: PANID of the coordinator (2 bytes)
> > > + * @NL802154_COORD_ADDR: coordinator address, (8 bytes or 2 bytes)
> > > + * @NL802154_COORD_CHANNEL: channel number, related to @NL802154_COORD_PAGE (u8)
> > > + * @NL802154_COORD_PAGE: channel page, related to @NL802154_COORD_CHANNEL (u8)
> > > + * @NL802154_COORD_PREAMBLE_CODE: Preamble code used when the beacon was received,
> > > + *     this is PHY dependent and optional (u8)
> > > + * @NL802154_COORD_MEAN_PRF: Mean PRF used when the beacon was received,
> > > + *     this is PHY dependent and optional (u8)
> > > + * @NL802154_COORD_SUPERFRAME_SPEC: superframe specification of the PAN (u16)
> > > + * @NL802154_COORD_LINK_QUALITY: signal quality of beacon in unspecified units,
> > > + *     scaled to 0..255 (u8)
> > > + * @NL802154_COORD_GTS_PERMIT: set to true if GTS is permitted on this PAN
> > > + * @NL802154_COORD_PAYLOAD_DATA: binary data containing the raw data from the
> > > + *     frame payload, (only if beacon or probe response had data)
> > > + * @NL802154_COORD_PAD: attribute used for padding for 64-bit alignment
> > > + * @NL802154_COORD_MAX: highest coordinator attribute
> > > + */
> > > +enum nl802154_coord {
> > > +       __NL802154_COORD_INVALID,
> > > +       NL802154_COORD_PANID,
> > > +       NL802154_COORD_ADDR,
> > > +       NL802154_COORD_CHANNEL,
> > > +       NL802154_COORD_PAGE,
> > > +       NL802154_COORD_PREAMBLE_CODE,
> >
> > Interesting, if you do a scan and discover pans and others answers I
> > would think you would see only pans on the same preamble. How is this
> > working?
>
> Yes this is how it is working, you only see PANs on one preamble at a
> time. That's why we need to tell on which preamble we received the
> beacon.
>

But then I don't know how you want to change the preamble while
scanning? I know there are registers for changing the preamble and I
thought that is a vendor specific option. However I am not an expert
to judge if it's needed or not, but somehow curious how it's working.

NOTE: that the preamble is so far I know (and makes sense for me)
_always_ filtered on PHY side.

> >
> > > +       NL802154_COORD_MEAN_PRF,
> > > +       NL802154_COORD_SUPERFRAME_SPEC,
> > > +       NL802154_COORD_LINK_QUALITY,
> >
> > not against it to have it, it's fine. I just think it is not very
> > useful. A way to dump all LQI values with some timestamp and having
> > something in user space to collect stats and do some heuristic may be
> > better?
>
> Actually I really like seeing this in the event logs in userspace, so if
> you don't mind I'll keep this parameter. It can safely be ignored by the
> userspace anyway, so I guess it does not hurt.
>

ok.

- Alex

Re: [PATCH wpan-next 1/3] ieee802154: Advertize coordinators discovery

[Alexander Aring]

Hi,

On Sun, Nov 20, 2022 at 7:57 PM Alexander Aring <aahringo@redhat.com> wrote:
...
> >
> > Yes this is how it is working, you only see PANs on one preamble at a
> > time. That's why we need to tell on which preamble we received the
> > beacon.
> >
>
> But then I don't know how you want to change the preamble while
> scanning? I know there are registers for changing the preamble and I
> thought that is a vendor specific option. However I am not an expert
> to judge if it's needed or not, but somehow curious how it's working.
>
> NOTE: that the preamble is so far I know (and makes sense for me)
> _always_ filtered on PHY side.

*I hope we are talking here about the same preamble.

- Alex

Re: [PATCH wpan-next 1/3] ieee802154: Advertize coordinators discovery

[Miquel Raynal]

Hi Alexander,

aahringo@redhat.com wrote on Sun, 20 Nov 2022 19:57:31 -0500:

> Hi,
> 
> On Fri, Nov 18, 2022 at 5:04 PM Miquel Raynal <miquel.raynal@bootlin.com> wrote:
> >
> > Hi Alexander,
> >
> > aahringo@redhat.com wrote on Sun, 6 Nov 2022 21:01:35 -0500:
> >  
> > > Hi,
> > >
> > > On Wed, Nov 2, 2022 at 11:20 AM Miquel Raynal <miquel.raynal@bootlin.com> wrote:  
> > > >
> > > > Let's introduce the basics for advertizing discovered PANs and
> > > > coordinators, which is:
> > > > - A new "scan" netlink message group.
> > > > - A couple of netlink command/attribute.
> > > > - The main netlink helper to send a netlink message with all the
> > > >   necessary information to forward the main information to the user.
> > > >
> > > > Two netlink attributes are proactively added to support future UWB
> > > > complex channels, but are not actually used yet.
> > > >
> > > > 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   |  20 +++++++
> > > >  include/net/nl802154.h    |  44 ++++++++++++++
> > > >  net/ieee802154/nl802154.c | 121 ++++++++++++++++++++++++++++++++++++++
> > > >  net/ieee802154/nl802154.h |   6 ++
> > > >  4 files changed, 191 insertions(+)
> > > >
> > > > diff --git a/include/net/cfg802154.h b/include/net/cfg802154.h
> > > > index e1481f9cf049..8d67d9ed438d 100644
> > > > --- a/include/net/cfg802154.h
> > > > +++ b/include/net/cfg802154.h
> > > > @@ -260,6 +260,26 @@ struct ieee802154_addr {
> > > >         };
> > > >  };
> > > >
> > > > +/**
> > > > + * struct ieee802154_coord_desc - Coordinator descriptor
> > > > + * @coord: PAN ID and coordinator address
> > > > + * @page: page this coordinator is using
> > > > + * @channel: channel this coordinator is using
> > > > + * @superframe_spec: SuperFrame specification as received
> > > > + * @link_quality: link quality indicator at which the beacon was received
> > > > + * @gts_permit: the coordinator accepts GTS requests
> > > > + * @node: list item
> > > > + */
> > > > +struct ieee802154_coord_desc {
> > > > +       struct ieee802154_addr *addr;  
> > >
> > > Why is this a pointer?  
> >
> > No reason anymore, I've changed this member to be a regular structure.
> >  
> 
> ok.
> 
> > >  
> > > > +       u8 page;
> > > > +       u8 channel;
> > > > +       u16 superframe_spec;
> > > > +       u8 link_quality;
> > > > +       bool gts_permit;
> > > > +       struct list_head node;
> > > > +};
> > > > +
> > > >  struct ieee802154_llsec_key_id {
> > > >         u8 mode;
> > > >         u8 id;
> > > > diff --git a/include/net/nl802154.h b/include/net/nl802154.h
> > > > index 145acb8f2509..cfe462288695 100644
> > > > --- a/include/net/nl802154.h
> > > > +++ b/include/net/nl802154.h
> > > > @@ -58,6 +58,9 @@ enum nl802154_commands {
> > > >
> > > >         NL802154_CMD_SET_WPAN_PHY_NETNS,
> > > >
> > > > +       NL802154_CMD_NEW_COORDINATOR,
> > > > +       NL802154_CMD_KNOWN_COORDINATOR,
> > > > +  
> > >
> > > NEW is something we never saw before and KNOWN we already saw before?
> > > I am not getting that when I just want to maintain a list in the user
> > > space and keep them updated, but I think we had this discussion
> > > already or? Currently they do the same thing, just the command is
> > > different. The user can use it to filter NEW and KNOWN? Still I am not
> > > getting it why there is not just a start ... event, event, event ....
> > > end. and let the user decide if it knows that it's new or old from its
> > > perspective.  
> >
> > Actually we already discussed this once and I personally liked more to
> > handle this in the kernel, but you seem to really prefer letting the
> > user space device whether or not the beacon is a new one or not, so
> > I've updated both the kernel side and the userspace side to act like
> > this.
> >  
> 
> I thought there was some problem about when the "scan-op" is running
> and there could be the case that the discovered PANs are twice there,
> but this looks more like handling UAPI features as separate new and
> old ones? I can see that there can be a need for the first case?

I don't think there is a problem handling this on one side or the
other, both should work identically. I've done the change anyway in v2
:)

> > > >         /* add new commands above here */
> > > >
> > > >  #ifdef CONFIG_IEEE802154_NL802154_EXPERIMENTAL
> > > > @@ -133,6 +136,8 @@ enum nl802154_attrs {
> > > >         NL802154_ATTR_PID,
> > > >         NL802154_ATTR_NETNS_FD,
> > > >
> > > > +       NL802154_ATTR_COORDINATOR,
> > > > +
> > > >         /* add attributes here, update the policy in nl802154.c */
> > > >
> > > >  #ifdef CONFIG_IEEE802154_NL802154_EXPERIMENTAL
> > > > @@ -218,6 +223,45 @@ enum nl802154_wpan_phy_capability_attr {
> > > >         NL802154_CAP_ATTR_MAX = __NL802154_CAP_ATTR_AFTER_LAST - 1
> > > >  };
> > > >
> > > > +/**
> > > > + * enum nl802154_coord - Netlink attributes for a coord
> > > > + *
> > > > + * @__NL802154_COORD_INVALID: invalid
> > > > + * @NL802154_COORD_PANID: PANID of the coordinator (2 bytes)
> > > > + * @NL802154_COORD_ADDR: coordinator address, (8 bytes or 2 bytes)
> > > > + * @NL802154_COORD_CHANNEL: channel number, related to @NL802154_COORD_PAGE (u8)
> > > > + * @NL802154_COORD_PAGE: channel page, related to @NL802154_COORD_CHANNEL (u8)
> > > > + * @NL802154_COORD_PREAMBLE_CODE: Preamble code used when the beacon was received,
> > > > + *     this is PHY dependent and optional (u8)
> > > > + * @NL802154_COORD_MEAN_PRF: Mean PRF used when the beacon was received,
> > > > + *     this is PHY dependent and optional (u8)
> > > > + * @NL802154_COORD_SUPERFRAME_SPEC: superframe specification of the PAN (u16)
> > > > + * @NL802154_COORD_LINK_QUALITY: signal quality of beacon in unspecified units,
> > > > + *     scaled to 0..255 (u8)
> > > > + * @NL802154_COORD_GTS_PERMIT: set to true if GTS is permitted on this PAN
> > > > + * @NL802154_COORD_PAYLOAD_DATA: binary data containing the raw data from the
> > > > + *     frame payload, (only if beacon or probe response had data)
> > > > + * @NL802154_COORD_PAD: attribute used for padding for 64-bit alignment
> > > > + * @NL802154_COORD_MAX: highest coordinator attribute
> > > > + */
> > > > +enum nl802154_coord {
> > > > +       __NL802154_COORD_INVALID,
> > > > +       NL802154_COORD_PANID,
> > > > +       NL802154_COORD_ADDR,
> > > > +       NL802154_COORD_CHANNEL,
> > > > +       NL802154_COORD_PAGE,
> > > > +       NL802154_COORD_PREAMBLE_CODE,  
> > >
> > > Interesting, if you do a scan and discover pans and others answers I
> > > would think you would see only pans on the same preamble. How is this
> > > working?  
> >
> > Yes this is how it is working, you only see PANs on one preamble at a
> > time. That's why we need to tell on which preamble we received the
> > beacon.
> >  
> 
> But then I don't know how you want to change the preamble while
> scanning?

Just to be sure: here we are talking about reporting the beacons that
were received and the coordinators they advertise. Which means we
_need_ to tell the user on which preamble code it was, but we don't yet
consider any preamble code changes here on the PHY.

> I know there are registers for changing the preamble and I
> thought that is a vendor specific option. However I am not an expert
> to judge if it's needed or not, but somehow curious how it's working.

I guess this is a problem that we must delegate to the drivers, very
much like channel changes, no?

> NOTE: that the preamble is so far I know (and makes sense for me)
> _always_ filtered on PHY side.

Yes, I guess so.

> 
> > >  
> > > > +       NL802154_COORD_MEAN_PRF,
> > > > +       NL802154_COORD_SUPERFRAME_SPEC,
> > > > +       NL802154_COORD_LINK_QUALITY,  
> > >
> > > not against it to have it, it's fine. I just think it is not very
> > > useful. A way to dump all LQI values with some timestamp and having
> > > something in user space to collect stats and do some heuristic may be
> > > better?  
> >
> > Actually I really like seeing this in the event logs in userspace, so if
> > you don't mind I'll keep this parameter. It can safely be ignored by the
> > userspace anyway, so I guess it does not hurt.
> >  
> 
> ok.
> 
> - Alex
> 


Thanks,
Miquèl

Re: [PATCH wpan-next 1/3] ieee802154: Advertize coordinators discovery

[Alexander Aring]

Hi,

On Mon, Nov 21, 2022 at 4:05 AM Miquel Raynal <miquel.raynal@bootlin.com> wrote:
>
> Hi Alexander,
>
> aahringo@redhat.com wrote on Sun, 20 Nov 2022 19:57:31 -0500:
>
> > Hi,
> >
> > On Fri, Nov 18, 2022 at 5:04 PM Miquel Raynal <miquel.raynal@bootlin.com> wrote:
> > >
> > > Hi Alexander,
> > >
> > > aahringo@redhat.com wrote on Sun, 6 Nov 2022 21:01:35 -0500:
> > >
> > > > Hi,
> > > >
> > > > On Wed, Nov 2, 2022 at 11:20 AM Miquel Raynal <miquel.raynal@bootlin.com> wrote:
> > > > >
> > > > > Let's introduce the basics for advertizing discovered PANs and
> > > > > coordinators, which is:
> > > > > - A new "scan" netlink message group.
> > > > > - A couple of netlink command/attribute.
> > > > > - The main netlink helper to send a netlink message with all the
> > > > >   necessary information to forward the main information to the user.
> > > > >
> > > > > Two netlink attributes are proactively added to support future UWB
> > > > > complex channels, but are not actually used yet.
> > > > >
> > > > > 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   |  20 +++++++
> > > > >  include/net/nl802154.h    |  44 ++++++++++++++
> > > > >  net/ieee802154/nl802154.c | 121 ++++++++++++++++++++++++++++++++++++++
> > > > >  net/ieee802154/nl802154.h |   6 ++
> > > > >  4 files changed, 191 insertions(+)
> > > > >
> > > > > diff --git a/include/net/cfg802154.h b/include/net/cfg802154.h
> > > > > index e1481f9cf049..8d67d9ed438d 100644
> > > > > --- a/include/net/cfg802154.h
> > > > > +++ b/include/net/cfg802154.h
> > > > > @@ -260,6 +260,26 @@ struct ieee802154_addr {
> > > > >         };
> > > > >  };
> > > > >
> > > > > +/**
> > > > > + * struct ieee802154_coord_desc - Coordinator descriptor
> > > > > + * @coord: PAN ID and coordinator address
> > > > > + * @page: page this coordinator is using
> > > > > + * @channel: channel this coordinator is using
> > > > > + * @superframe_spec: SuperFrame specification as received
> > > > > + * @link_quality: link quality indicator at which the beacon was received
> > > > > + * @gts_permit: the coordinator accepts GTS requests
> > > > > + * @node: list item
> > > > > + */
> > > > > +struct ieee802154_coord_desc {
> > > > > +       struct ieee802154_addr *addr;
> > > >
> > > > Why is this a pointer?
> > >
> > > No reason anymore, I've changed this member to be a regular structure.
> > >
> >
> > ok.
> >
> > > >
> > > > > +       u8 page;
> > > > > +       u8 channel;
> > > > > +       u16 superframe_spec;
> > > > > +       u8 link_quality;
> > > > > +       bool gts_permit;
> > > > > +       struct list_head node;
> > > > > +};
> > > > > +
> > > > >  struct ieee802154_llsec_key_id {
> > > > >         u8 mode;
> > > > >         u8 id;
> > > > > diff --git a/include/net/nl802154.h b/include/net/nl802154.h
> > > > > index 145acb8f2509..cfe462288695 100644
> > > > > --- a/include/net/nl802154.h
> > > > > +++ b/include/net/nl802154.h
> > > > > @@ -58,6 +58,9 @@ enum nl802154_commands {
> > > > >
> > > > >         NL802154_CMD_SET_WPAN_PHY_NETNS,
> > > > >
> > > > > +       NL802154_CMD_NEW_COORDINATOR,
> > > > > +       NL802154_CMD_KNOWN_COORDINATOR,
> > > > > +
> > > >
> > > > NEW is something we never saw before and KNOWN we already saw before?
> > > > I am not getting that when I just want to maintain a list in the user
> > > > space and keep them updated, but I think we had this discussion
> > > > already or? Currently they do the same thing, just the command is
> > > > different. The user can use it to filter NEW and KNOWN? Still I am not
> > > > getting it why there is not just a start ... event, event, event ....
> > > > end. and let the user decide if it knows that it's new or old from its
> > > > perspective.
> > >
> > > Actually we already discussed this once and I personally liked more to
> > > handle this in the kernel, but you seem to really prefer letting the
> > > user space device whether or not the beacon is a new one or not, so
> > > I've updated both the kernel side and the userspace side to act like
> > > this.
> > >
> >
> > I thought there was some problem about when the "scan-op" is running
> > and there could be the case that the discovered PANs are twice there,
> > but this looks more like handling UAPI features as separate new and
> > old ones? I can see that there can be a need for the first case?
>
> I don't think there is a problem handling this on one side or the
> other, both should work identically. I've done the change anyway in v2
> :)
>

ok.

> > > > >         /* add new commands above here */
> > > > >
> > > > >  #ifdef CONFIG_IEEE802154_NL802154_EXPERIMENTAL
> > > > > @@ -133,6 +136,8 @@ enum nl802154_attrs {
> > > > >         NL802154_ATTR_PID,
> > > > >         NL802154_ATTR_NETNS_FD,
> > > > >
> > > > > +       NL802154_ATTR_COORDINATOR,
> > > > > +
> > > > >         /* add attributes here, update the policy in nl802154.c */
> > > > >
> > > > >  #ifdef CONFIG_IEEE802154_NL802154_EXPERIMENTAL
> > > > > @@ -218,6 +223,45 @@ enum nl802154_wpan_phy_capability_attr {
> > > > >         NL802154_CAP_ATTR_MAX = __NL802154_CAP_ATTR_AFTER_LAST - 1
> > > > >  };
> > > > >
> > > > > +/**
> > > > > + * enum nl802154_coord - Netlink attributes for a coord
> > > > > + *
> > > > > + * @__NL802154_COORD_INVALID: invalid
> > > > > + * @NL802154_COORD_PANID: PANID of the coordinator (2 bytes)
> > > > > + * @NL802154_COORD_ADDR: coordinator address, (8 bytes or 2 bytes)
> > > > > + * @NL802154_COORD_CHANNEL: channel number, related to @NL802154_COORD_PAGE (u8)
> > > > > + * @NL802154_COORD_PAGE: channel page, related to @NL802154_COORD_CHANNEL (u8)
> > > > > + * @NL802154_COORD_PREAMBLE_CODE: Preamble code used when the beacon was received,
> > > > > + *     this is PHY dependent and optional (u8)
> > > > > + * @NL802154_COORD_MEAN_PRF: Mean PRF used when the beacon was received,
> > > > > + *     this is PHY dependent and optional (u8)
> > > > > + * @NL802154_COORD_SUPERFRAME_SPEC: superframe specification of the PAN (u16)
> > > > > + * @NL802154_COORD_LINK_QUALITY: signal quality of beacon in unspecified units,
> > > > > + *     scaled to 0..255 (u8)
> > > > > + * @NL802154_COORD_GTS_PERMIT: set to true if GTS is permitted on this PAN
> > > > > + * @NL802154_COORD_PAYLOAD_DATA: binary data containing the raw data from the
> > > > > + *     frame payload, (only if beacon or probe response had data)
> > > > > + * @NL802154_COORD_PAD: attribute used for padding for 64-bit alignment
> > > > > + * @NL802154_COORD_MAX: highest coordinator attribute
> > > > > + */
> > > > > +enum nl802154_coord {
> > > > > +       __NL802154_COORD_INVALID,
> > > > > +       NL802154_COORD_PANID,
> > > > > +       NL802154_COORD_ADDR,
> > > > > +       NL802154_COORD_CHANNEL,
> > > > > +       NL802154_COORD_PAGE,
> > > > > +       NL802154_COORD_PREAMBLE_CODE,
> > > >
> > > > Interesting, if you do a scan and discover pans and others answers I
> > > > would think you would see only pans on the same preamble. How is this
> > > > working?
> > >
> > > Yes this is how it is working, you only see PANs on one preamble at a
> > > time. That's why we need to tell on which preamble we received the
> > > beacon.
> > >
> >
> > But then I don't know how you want to change the preamble while
> > scanning?
>
> Just to be sure: here we are talking about reporting the beacons that
> were received and the coordinators they advertise. Which means we
> _need_ to tell the user on which preamble code it was, but we don't yet
> consider any preamble code changes here on the PHY.
>

but there is my question, how coordinators can advertise they are
running on a different preamble as they sent on the advertisement. And
what I thought was that the preamble is a non changeable thing, more
specifically 4 octet all zero (preamble) followed by 0xA7 (SFD)). I
know there are transceivers to change at least the SFD value, but then
I was assuming you were running out of spec (some people doing that to
ehm... "fake secure" their 802.15.4 communication as I heard).

> > I know there are registers for changing the preamble and I
> > thought that is a vendor specific option. However I am not an expert
> > to judge if it's needed or not, but somehow curious how it's working.
>
> I guess this is a problem that we must delegate to the drivers, very
> much like channel changes, no?
>

yes.

- Alex

Re: [PATCH wpan-next 1/3] ieee802154: Advertize coordinators discovery

[Miquel Raynal]

Hi Alexander,

aahringo@redhat.com wrote on Mon, 21 Nov 2022 18:54:31 -0500:

> Hi,
> 
> On Mon, Nov 21, 2022 at 4:05 AM Miquel Raynal <miquel.raynal@bootlin.com> wrote:
> >
> > Hi Alexander,
> >
> > aahringo@redhat.com wrote on Sun, 20 Nov 2022 19:57:31 -0500:
> >  
> > > Hi,
> > >
> > > On Fri, Nov 18, 2022 at 5:04 PM Miquel Raynal <miquel.raynal@bootlin.com> wrote:  
> > > >
> > > > Hi Alexander,
> > > >
> > > > aahringo@redhat.com wrote on Sun, 6 Nov 2022 21:01:35 -0500:
> > > >  
> > > > > Hi,
> > > > >
> > > > > On Wed, Nov 2, 2022 at 11:20 AM Miquel Raynal <miquel.raynal@bootlin.com> wrote:  
> > > > > >
> > > > > > Let's introduce the basics for advertizing discovered PANs and
> > > > > > coordinators, which is:
> > > > > > - A new "scan" netlink message group.
> > > > > > - A couple of netlink command/attribute.
> > > > > > - The main netlink helper to send a netlink message with all the
> > > > > >   necessary information to forward the main information to the user.
> > > > > >
> > > > > > Two netlink attributes are proactively added to support future UWB
> > > > > > complex channels, but are not actually used yet.
> > > > > >
> > > > > > 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   |  20 +++++++
> > > > > >  include/net/nl802154.h    |  44 ++++++++++++++
> > > > > >  net/ieee802154/nl802154.c | 121 ++++++++++++++++++++++++++++++++++++++
> > > > > >  net/ieee802154/nl802154.h |   6 ++
> > > > > >  4 files changed, 191 insertions(+)
> > > > > >
> > > > > > diff --git a/include/net/cfg802154.h b/include/net/cfg802154.h
> > > > > > index e1481f9cf049..8d67d9ed438d 100644
> > > > > > --- a/include/net/cfg802154.h
> > > > > > +++ b/include/net/cfg802154.h
> > > > > > @@ -260,6 +260,26 @@ struct ieee802154_addr {
> > > > > >         };
> > > > > >  };
> > > > > >
> > > > > > +/**
> > > > > > + * struct ieee802154_coord_desc - Coordinator descriptor
> > > > > > + * @coord: PAN ID and coordinator address
> > > > > > + * @page: page this coordinator is using
> > > > > > + * @channel: channel this coordinator is using
> > > > > > + * @superframe_spec: SuperFrame specification as received
> > > > > > + * @link_quality: link quality indicator at which the beacon was received
> > > > > > + * @gts_permit: the coordinator accepts GTS requests
> > > > > > + * @node: list item
> > > > > > + */
> > > > > > +struct ieee802154_coord_desc {
> > > > > > +       struct ieee802154_addr *addr;  
> > > > >
> > > > > Why is this a pointer?  
> > > >
> > > > No reason anymore, I've changed this member to be a regular structure.
> > > >  
> > >
> > > ok.
> > >  
> > > > >  
> > > > > > +       u8 page;
> > > > > > +       u8 channel;
> > > > > > +       u16 superframe_spec;
> > > > > > +       u8 link_quality;
> > > > > > +       bool gts_permit;
> > > > > > +       struct list_head node;
> > > > > > +};
> > > > > > +
> > > > > >  struct ieee802154_llsec_key_id {
> > > > > >         u8 mode;
> > > > > >         u8 id;
> > > > > > diff --git a/include/net/nl802154.h b/include/net/nl802154.h
> > > > > > index 145acb8f2509..cfe462288695 100644
> > > > > > --- a/include/net/nl802154.h
> > > > > > +++ b/include/net/nl802154.h
> > > > > > @@ -58,6 +58,9 @@ enum nl802154_commands {
> > > > > >
> > > > > >         NL802154_CMD_SET_WPAN_PHY_NETNS,
> > > > > >
> > > > > > +       NL802154_CMD_NEW_COORDINATOR,
> > > > > > +       NL802154_CMD_KNOWN_COORDINATOR,
> > > > > > +  
> > > > >
> > > > > NEW is something we never saw before and KNOWN we already saw before?
> > > > > I am not getting that when I just want to maintain a list in the user
> > > > > space and keep them updated, but I think we had this discussion
> > > > > already or? Currently they do the same thing, just the command is
> > > > > different. The user can use it to filter NEW and KNOWN? Still I am not
> > > > > getting it why there is not just a start ... event, event, event ....
> > > > > end. and let the user decide if it knows that it's new or old from its
> > > > > perspective.  
> > > >
> > > > Actually we already discussed this once and I personally liked more to
> > > > handle this in the kernel, but you seem to really prefer letting the
> > > > user space device whether or not the beacon is a new one or not, so
> > > > I've updated both the kernel side and the userspace side to act like
> > > > this.
> > > >  
> > >
> > > I thought there was some problem about when the "scan-op" is running
> > > and there could be the case that the discovered PANs are twice there,
> > > but this looks more like handling UAPI features as separate new and
> > > old ones? I can see that there can be a need for the first case?  
> >
> > I don't think there is a problem handling this on one side or the
> > other, both should work identically. I've done the change anyway in v2
> > :)
> >  
> 
> ok.
> 
> > > > > >         /* add new commands above here */
> > > > > >
> > > > > >  #ifdef CONFIG_IEEE802154_NL802154_EXPERIMENTAL
> > > > > > @@ -133,6 +136,8 @@ enum nl802154_attrs {
> > > > > >         NL802154_ATTR_PID,
> > > > > >         NL802154_ATTR_NETNS_FD,
> > > > > >
> > > > > > +       NL802154_ATTR_COORDINATOR,
> > > > > > +
> > > > > >         /* add attributes here, update the policy in nl802154.c */
> > > > > >
> > > > > >  #ifdef CONFIG_IEEE802154_NL802154_EXPERIMENTAL
> > > > > > @@ -218,6 +223,45 @@ enum nl802154_wpan_phy_capability_attr {
> > > > > >         NL802154_CAP_ATTR_MAX = __NL802154_CAP_ATTR_AFTER_LAST - 1
> > > > > >  };
> > > > > >
> > > > > > +/**
> > > > > > + * enum nl802154_coord - Netlink attributes for a coord
> > > > > > + *
> > > > > > + * @__NL802154_COORD_INVALID: invalid
> > > > > > + * @NL802154_COORD_PANID: PANID of the coordinator (2 bytes)
> > > > > > + * @NL802154_COORD_ADDR: coordinator address, (8 bytes or 2 bytes)
> > > > > > + * @NL802154_COORD_CHANNEL: channel number, related to @NL802154_COORD_PAGE (u8)
> > > > > > + * @NL802154_COORD_PAGE: channel page, related to @NL802154_COORD_CHANNEL (u8)
> > > > > > + * @NL802154_COORD_PREAMBLE_CODE: Preamble code used when the beacon was received,
> > > > > > + *     this is PHY dependent and optional (u8)
> > > > > > + * @NL802154_COORD_MEAN_PRF: Mean PRF used when the beacon was received,
> > > > > > + *     this is PHY dependent and optional (u8)
> > > > > > + * @NL802154_COORD_SUPERFRAME_SPEC: superframe specification of the PAN (u16)
> > > > > > + * @NL802154_COORD_LINK_QUALITY: signal quality of beacon in unspecified units,
> > > > > > + *     scaled to 0..255 (u8)
> > > > > > + * @NL802154_COORD_GTS_PERMIT: set to true if GTS is permitted on this PAN
> > > > > > + * @NL802154_COORD_PAYLOAD_DATA: binary data containing the raw data from the
> > > > > > + *     frame payload, (only if beacon or probe response had data)
> > > > > > + * @NL802154_COORD_PAD: attribute used for padding for 64-bit alignment
> > > > > > + * @NL802154_COORD_MAX: highest coordinator attribute
> > > > > > + */
> > > > > > +enum nl802154_coord {
> > > > > > +       __NL802154_COORD_INVALID,
> > > > > > +       NL802154_COORD_PANID,
> > > > > > +       NL802154_COORD_ADDR,
> > > > > > +       NL802154_COORD_CHANNEL,
> > > > > > +       NL802154_COORD_PAGE,
> > > > > > +       NL802154_COORD_PREAMBLE_CODE,  
> > > > >
> > > > > Interesting, if you do a scan and discover pans and others answers I
> > > > > would think you would see only pans on the same preamble. How is this
> > > > > working?  
> > > >
> > > > Yes this is how it is working, you only see PANs on one preamble at a
> > > > time. That's why we need to tell on which preamble we received the
> > > > beacon.
> > > >  
> > >
> > > But then I don't know how you want to change the preamble while
> > > scanning?  
> >
> > Just to be sure: here we are talking about reporting the beacons that
> > were received and the coordinators they advertise. Which means we
> > _need_ to tell the user on which preamble code it was, but we don't yet
> > consider any preamble code changes here on the PHY.
> >  
> 
> but there is my question, how coordinators can advertise they are
> running on a different preamble as they sent on the advertisement.

Well same as a channel change? I don't expect them to constantly
change. But if they do, the next scan will report it.

> And
> what I thought was that the preamble is a non changeable thing, more
> specifically 4 octet all zero (preamble) followed by 0xA7 (SFD)). I
> know there are transceivers to change at least the SFD value, but then
> I was assuming you were running out of spec (some people doing that to
> ehm... "fake secure" their 802.15.4 communication as I heard).

I have not taken into account advanced/out-of-spec preamble code
handling, for now I consider them to be an integer (very much like the
channels actually).

At least for what I can see, it should be enough.

If this field bothers you for now I can drop the field and
we will later add it at the end of the list. To be fully transparent,
it works only in simulation. I haven't yet tested it on UWB hardware but
this is in the pipe. Let me know what you prefer. 

> > > I know there are registers for changing the preamble and I
> > > thought that is a vendor specific option. However I am not an expert
> > > to judge if it's needed or not, but somehow curious how it's working.  
> >
> > I guess this is a problem that we must delegate to the drivers, very
> > much like channel changes, no?
> >  
> 
> yes.
> 
> - Alex
> 


Thanks,
Miquèl

Re: [PATCH wpan-next 1/3] ieee802154: Advertize coordinators discovery

[Alexander Aring]

Hi,

On Wed, Nov 23, 2022 at 12:07 PM Miquel Raynal
<miquel.raynal@bootlin.com> wrote:
>
> Hi Alexander,
>
> aahringo@redhat.com wrote on Mon, 21 Nov 2022 18:54:31 -0500:
>
> > Hi,
> >
> > On Mon, Nov 21, 2022 at 4:05 AM Miquel Raynal <miquel.raynal@bootlin.com> wrote:
> > >
> > > Hi Alexander,
> > >
> > > aahringo@redhat.com wrote on Sun, 20 Nov 2022 19:57:31 -0500:
> > >
> > > > Hi,
> > > >
> > > > On Fri, Nov 18, 2022 at 5:04 PM Miquel Raynal <miquel.raynal@bootlin.com> wrote:
> > > > >
> > > > > Hi Alexander,
> > > > >
> > > > > aahringo@redhat.com wrote on Sun, 6 Nov 2022 21:01:35 -0500:
> > > > >
> > > > > > Hi,
> > > > > >
> > > > > > On Wed, Nov 2, 2022 at 11:20 AM Miquel Raynal <miquel.raynal@bootlin.com> wrote:
> > > > > > >
> > > > > > > Let's introduce the basics for advertizing discovered PANs and
> > > > > > > coordinators, which is:
> > > > > > > - A new "scan" netlink message group.
> > > > > > > - A couple of netlink command/attribute.
> > > > > > > - The main netlink helper to send a netlink message with all the
> > > > > > >   necessary information to forward the main information to the user.
> > > > > > >
> > > > > > > Two netlink attributes are proactively added to support future UWB
> > > > > > > complex channels, but are not actually used yet.
> > > > > > >
> > > > > > > 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   |  20 +++++++
> > > > > > >  include/net/nl802154.h    |  44 ++++++++++++++
> > > > > > >  net/ieee802154/nl802154.c | 121 ++++++++++++++++++++++++++++++++++++++
> > > > > > >  net/ieee802154/nl802154.h |   6 ++
> > > > > > >  4 files changed, 191 insertions(+)
> > > > > > >
> > > > > > > diff --git a/include/net/cfg802154.h b/include/net/cfg802154.h
> > > > > > > index e1481f9cf049..8d67d9ed438d 100644
> > > > > > > --- a/include/net/cfg802154.h
> > > > > > > +++ b/include/net/cfg802154.h
> > > > > > > @@ -260,6 +260,26 @@ struct ieee802154_addr {
> > > > > > >         };
> > > > > > >  };
> > > > > > >
> > > > > > > +/**
> > > > > > > + * struct ieee802154_coord_desc - Coordinator descriptor
> > > > > > > + * @coord: PAN ID and coordinator address
> > > > > > > + * @page: page this coordinator is using
> > > > > > > + * @channel: channel this coordinator is using
> > > > > > > + * @superframe_spec: SuperFrame specification as received
> > > > > > > + * @link_quality: link quality indicator at which the beacon was received
> > > > > > > + * @gts_permit: the coordinator accepts GTS requests
> > > > > > > + * @node: list item
> > > > > > > + */
> > > > > > > +struct ieee802154_coord_desc {
> > > > > > > +       struct ieee802154_addr *addr;
> > > > > >
> > > > > > Why is this a pointer?
> > > > >
> > > > > No reason anymore, I've changed this member to be a regular structure.
> > > > >
> > > >
> > > > ok.
> > > >
> > > > > >
> > > > > > > +       u8 page;
> > > > > > > +       u8 channel;
> > > > > > > +       u16 superframe_spec;
> > > > > > > +       u8 link_quality;
> > > > > > > +       bool gts_permit;
> > > > > > > +       struct list_head node;
> > > > > > > +};
> > > > > > > +
> > > > > > >  struct ieee802154_llsec_key_id {
> > > > > > >         u8 mode;
> > > > > > >         u8 id;
> > > > > > > diff --git a/include/net/nl802154.h b/include/net/nl802154.h
> > > > > > > index 145acb8f2509..cfe462288695 100644
> > > > > > > --- a/include/net/nl802154.h
> > > > > > > +++ b/include/net/nl802154.h
> > > > > > > @@ -58,6 +58,9 @@ enum nl802154_commands {
> > > > > > >
> > > > > > >         NL802154_CMD_SET_WPAN_PHY_NETNS,
> > > > > > >
> > > > > > > +       NL802154_CMD_NEW_COORDINATOR,
> > > > > > > +       NL802154_CMD_KNOWN_COORDINATOR,
> > > > > > > +
> > > > > >
> > > > > > NEW is something we never saw before and KNOWN we already saw before?
> > > > > > I am not getting that when I just want to maintain a list in the user
> > > > > > space and keep them updated, but I think we had this discussion
> > > > > > already or? Currently they do the same thing, just the command is
> > > > > > different. The user can use it to filter NEW and KNOWN? Still I am not
> > > > > > getting it why there is not just a start ... event, event, event ....
> > > > > > end. and let the user decide if it knows that it's new or old from its
> > > > > > perspective.
> > > > >
> > > > > Actually we already discussed this once and I personally liked more to
> > > > > handle this in the kernel, but you seem to really prefer letting the
> > > > > user space device whether or not the beacon is a new one or not, so
> > > > > I've updated both the kernel side and the userspace side to act like
> > > > > this.
> > > > >
> > > >
> > > > I thought there was some problem about when the "scan-op" is running
> > > > and there could be the case that the discovered PANs are twice there,
> > > > but this looks more like handling UAPI features as separate new and
> > > > old ones? I can see that there can be a need for the first case?
> > >
> > > I don't think there is a problem handling this on one side or the
> > > other, both should work identically. I've done the change anyway in v2
> > > :)
> > >
> >
> > ok.
> >
> > > > > > >         /* add new commands above here */
> > > > > > >
> > > > > > >  #ifdef CONFIG_IEEE802154_NL802154_EXPERIMENTAL
> > > > > > > @@ -133,6 +136,8 @@ enum nl802154_attrs {
> > > > > > >         NL802154_ATTR_PID,
> > > > > > >         NL802154_ATTR_NETNS_FD,
> > > > > > >
> > > > > > > +       NL802154_ATTR_COORDINATOR,
> > > > > > > +
> > > > > > >         /* add attributes here, update the policy in nl802154.c */
> > > > > > >
> > > > > > >  #ifdef CONFIG_IEEE802154_NL802154_EXPERIMENTAL
> > > > > > > @@ -218,6 +223,45 @@ enum nl802154_wpan_phy_capability_attr {
> > > > > > >         NL802154_CAP_ATTR_MAX = __NL802154_CAP_ATTR_AFTER_LAST - 1
> > > > > > >  };
> > > > > > >
> > > > > > > +/**
> > > > > > > + * enum nl802154_coord - Netlink attributes for a coord
> > > > > > > + *
> > > > > > > + * @__NL802154_COORD_INVALID: invalid
> > > > > > > + * @NL802154_COORD_PANID: PANID of the coordinator (2 bytes)
> > > > > > > + * @NL802154_COORD_ADDR: coordinator address, (8 bytes or 2 bytes)
> > > > > > > + * @NL802154_COORD_CHANNEL: channel number, related to @NL802154_COORD_PAGE (u8)
> > > > > > > + * @NL802154_COORD_PAGE: channel page, related to @NL802154_COORD_CHANNEL (u8)
> > > > > > > + * @NL802154_COORD_PREAMBLE_CODE: Preamble code used when the beacon was received,
> > > > > > > + *     this is PHY dependent and optional (u8)
> > > > > > > + * @NL802154_COORD_MEAN_PRF: Mean PRF used when the beacon was received,
> > > > > > > + *     this is PHY dependent and optional (u8)
> > > > > > > + * @NL802154_COORD_SUPERFRAME_SPEC: superframe specification of the PAN (u16)
> > > > > > > + * @NL802154_COORD_LINK_QUALITY: signal quality of beacon in unspecified units,
> > > > > > > + *     scaled to 0..255 (u8)
> > > > > > > + * @NL802154_COORD_GTS_PERMIT: set to true if GTS is permitted on this PAN
> > > > > > > + * @NL802154_COORD_PAYLOAD_DATA: binary data containing the raw data from the
> > > > > > > + *     frame payload, (only if beacon or probe response had data)
> > > > > > > + * @NL802154_COORD_PAD: attribute used for padding for 64-bit alignment
> > > > > > > + * @NL802154_COORD_MAX: highest coordinator attribute
> > > > > > > + */
> > > > > > > +enum nl802154_coord {
> > > > > > > +       __NL802154_COORD_INVALID,
> > > > > > > +       NL802154_COORD_PANID,
> > > > > > > +       NL802154_COORD_ADDR,
> > > > > > > +       NL802154_COORD_CHANNEL,
> > > > > > > +       NL802154_COORD_PAGE,
> > > > > > > +       NL802154_COORD_PREAMBLE_CODE,
> > > > > >
> > > > > > Interesting, if you do a scan and discover pans and others answers I
> > > > > > would think you would see only pans on the same preamble. How is this
> > > > > > working?
> > > > >
> > > > > Yes this is how it is working, you only see PANs on one preamble at a
> > > > > time. That's why we need to tell on which preamble we received the
> > > > > beacon.
> > > > >
> > > >
> > > > But then I don't know how you want to change the preamble while
> > > > scanning?
> > >
> > > Just to be sure: here we are talking about reporting the beacons that
> > > were received and the coordinators they advertise. Which means we
> > > _need_ to tell the user on which preamble code it was, but we don't yet
> > > consider any preamble code changes here on the PHY.
> > >
> >
> > but there is my question, how coordinators can advertise they are
> > running on a different preamble as they sent on the advertisement.
>
> Well same as a channel change? I don't expect them to constantly
> change. But if they do, the next scan will report it.
>

ok.

> > And
> > what I thought was that the preamble is a non changeable thing, more
> > specifically 4 octet all zero (preamble) followed by 0xA7 (SFD)). I
> > know there are transceivers to change at least the SFD value, but then
> > I was assuming you were running out of spec (some people doing that to
> > ehm... "fake secure" their 802.15.4 communication as I heard).
>
> I have not taken into account advanced/out-of-spec preamble code
> handling, for now I consider them to be an integer (very much like the
> channels actually).
>

ok.

> At least for what I can see, it should be enough.
>
> If this field bothers you for now I can drop the field and
> we will later add it at the end of the list. To be fully transparent,
> it works only in simulation. I haven't yet tested it on UWB hardware but
> this is in the pipe. Let me know what you prefer.
>

no, it's fine.

- Alex