Hi,

On Mon, Sep 18, 2023 at 5:01 AM Miquel Raynal [off-list ref] wrote:

Hello,

aahringo@redhat.com wrote on Sun, 17 Sep 2023 07:50:55 -0400:

quoted

Hi,

On Sat, Sep 16, 2023 at 11:39 AM Stefan Schmidt
[off-list ref] wrote:

quoted

Hello Miquel.

On 01.09.23 19:04, Miquel Raynal wrote:

quoted

Introduce structures to describe peer devices in a PAN as well as a few
related helpers. We basically care about:
- Our unique parent after associating with a coordinator.
- Peer devices, children, which successfully associated with us.

Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
  include/net/cfg802154.h | 46 ++++++++++++++++++++++++++++
  net/ieee802154/Makefile |  2 +-
  net/ieee802154/core.c   |  2 ++
  net/ieee802154/pan.c    | 66 +++++++++++++++++++++++++++++++++++++++++
  4 files changed, 115 insertions(+), 1 deletion(-)
  create mode 100644 net/ieee802154/pan.c

diff --git a/include/net/cfg802154.h b/include/net/cfg802154.h
index f79ce133e51a..6c7193b4873c 100644
--- a/include/net/cfg802154.h
+++ b/include/net/cfg802154.h

@@ -303,6 +303,22 @@ struct ieee802154_coord_desc {
      bool gts_permit;
  };

+/**
+ * struct ieee802154_pan_device - PAN device information
+ * @pan_id: the PAN ID of this device
+ * @mode: the preferred mode to reach the device
+ * @short_addr: the short address of this device
+ * @extended_addr: the extended address of this device
+ * @node: the list node
+ */
+struct ieee802154_pan_device {
+     __le16 pan_id;
+     u8 mode;
+     __le16 short_addr;
+     __le64 extended_addr;
+     struct list_head node;
+};
+
  /**
   * struct cfg802154_scan_request - Scan request
   *

@@ -478,6 +494,11 @@ struct wpan_dev {

      /* fallback for acknowledgment bit setting */
      bool ackreq;
+
+     /* Associations */
+     struct mutex association_lock;
+     struct ieee802154_pan_device *parent;
+     struct list_head children;
  };

  #define to_phy(_dev)        container_of(_dev, struct wpan_phy, dev)

@@ -529,4 +550,29 @@ static inline const char *wpan_phy_name(struct wpan_phy *phy)
  void ieee802154_configure_durations(struct wpan_phy *phy,
                                  unsigned int page, unsigned int channel);

+/**
+ * cfg802154_device_is_associated - Checks whether we are associated to any device
+ * @wpan_dev: the wpan device

Missing return value documentation.

quoted

+ */
+bool cfg802154_device_is_associated(struct wpan_dev *wpan_dev);
+
+/**
+ * cfg802154_device_is_parent - Checks if a device is our coordinator
+ * @wpan_dev: the wpan device
+ * @target: the expected parent
+ * @return: true if @target is our coordinator
+ */
+bool cfg802154_device_is_parent(struct wpan_dev *wpan_dev,
+                             struct ieee802154_addr *target);
+
+/**
+ * cfg802154_device_is_child - Checks whether a device is associated to us
+ * @wpan_dev: the wpan device
+ * @target: the expected child
+ * @return: the PAN device
+ */
+struct ieee802154_pan_device *
+cfg802154_device_is_child(struct wpan_dev *wpan_dev,
+                       struct ieee802154_addr *target);
+
  #endif /* __NET_CFG802154_H */

diff --git a/net/ieee802154/Makefile b/net/ieee802154/Makefile
index f05b7bdae2aa..7bce67673e83 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 trace.o pan.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..cd69bdbfd59f 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++;
+             mutex_init(&wpan_dev->association_lock);
+             INIT_LIST_HEAD(&wpan_dev->children);

              wpan_dev->netdev = dev;
              break;

diff --git a/net/ieee802154/pan.c b/net/ieee802154/pan.c
new file mode 100644
index 000000000000..e2a12a42ba2b
--- /dev/null
+++ b/net/ieee802154/pan.c

@@ -0,0 +1,66 @@
+// 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/kernel.h>
+#include <net/cfg802154.h>
+#include <net/af_ieee802154.h>
+
+static bool cfg802154_same_addr(struct ieee802154_pan_device *a,
+                             struct ieee802154_addr *b)
+{
+     if (!a || !b)
+             return false;
+
+     switch (b->mode) {
+     case IEEE802154_ADDR_SHORT:
+             return a->short_addr == b->short_addr;
+     case IEEE802154_ADDR_LONG:
+             return a->extended_addr == b->extended_addr;
+     default:
+             return false;
+     }
+}

Don't we already have such a helper already?

There must also be a check on (a->mode != b->mode) because short_addr
and extended_addr share memory in this struct.

True.

Actually the ieee802154_addr structure uses an enum to store either
the short address or the extended addres, while at the MAC level I'd
like to compare with what I call a ieee802154_pan_device: the PAN
device is part of a list defining the associated neighbors and contains
both an extended address and a short address once associated.

I do not want to compare the PAN ID here and I do not need to compare
if the modes are different because the device the code is running on
is known to have both an extended address and a short address field
which have been initialized.

I see, so it is guaranteed that the mode value is the same?

With all these constraints, I think it would require more code to
re-use that small function than just writing a slightly different one
here which fully covers the "under association/disassociation" case, no?

I am questioning here currently myself if it's enough to uniquely
identify devices with only short or extended. For extended I would say
yes, for short I would say no. Somehow I don't get it, maybe because
it's on the setup to be associated and we know the panid already but
it is not being set at this point?

quoted

We have something "similar" [0] to this with also checks on panid,
however it depends what Miquel tries to do here. I can imagine that we
extend the helper with an enum opt about what to compare.

- Alex

[0]
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/include/net/ieee802154_netdev.h?h=v6.6-rc1#n232

Thanks for the link!

We have a lot of header dschungels which should be cleaned up at some point. :-)

- Alex