| Message ID | 20230901170501.1066321-3-miquel.raynal@bootlin.com (mailing list archive) |
|---|---|
| State | Awaiting Upstream |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | ieee802154: Associations between devices | expand |
Hello Miquel. On 01.09.23 19:04, Miquel Raynal wrote: > 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. > + */ > +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? regards Stefan Schmidt
Hi, On Sat, Sep 16, 2023 at 11:39 AM Stefan Schmidt <stefan@datenfreihafen.org> wrote: > > Hello Miquel. > > On 01.09.23 19:04, Miquel Raynal wrote: > > 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. > > > + */ > > +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. 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
Hello, aahringo@redhat.com wrote on Sun, 17 Sep 2023 07:50:55 -0400: > Hi, > > On Sat, Sep 16, 2023 at 11:39 AM Stefan Schmidt > <stefan@datenfreihafen.org> wrote: > > > > Hello Miquel. > > > > On 01.09.23 19:04, Miquel Raynal wrote: > > > 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. > > > > > + */ > > > +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. 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? > 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! Thanks, Miquèl
Hi, On Mon, Sep 18, 2023 at 5:01 AM Miquel Raynal <miquel.raynal@bootlin.com> wrote: > > Hello, > > aahringo@redhat.com wrote on Sun, 17 Sep 2023 07:50:55 -0400: > > > Hi, > > > > On Sat, Sep 16, 2023 at 11:39 AM Stefan Schmidt > > <stefan@datenfreihafen.org> wrote: > > > > > > Hello Miquel. > > > > > > On 01.09.23 19:04, Miquel Raynal wrote: > > > > 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. > > > > > > > + */ > > > > +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? > > 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
Hi Alexander, > > > > > +// 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? I looked more carefully at the code of the association section, we will always know the extended address of the devices which are associated to us, however there may be situations where the second device to compare with this list only comes with a short address and pan ID, so your initial comment needs to be addressed. > > 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. As long as we know the PAN ID, it should be fine. > 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? Yes, this code is used *when* we associate or disassociate. Thanks, Miquèl
Hi, On Mon, Sep 18, 2023 at 10:15 AM Miquel Raynal <miquel.raynal@bootlin.com> wrote: > > Hi Alexander, > > > > > > > > +// 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? > > I looked more carefully at the code of the association section, > we will always know the extended address of the devices which are > associated to us, however there may be situations where the second > device to compare with this list only comes with a short address and pan > ID, so your initial comment needs to be addressed. > > > > 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. > > As long as we know the PAN ID, it should be fine. > yep, so you will add a check of panid when mode is short address type? - Alex
Hi Alexander, aahringo@redhat.com wrote on Mon, 18 Sep 2023 19:01:14 -0400: > Hi, > > On Mon, Sep 18, 2023 at 10:15 AM Miquel Raynal > <miquel.raynal@bootlin.com> wrote: > > > > Hi Alexander, > > > > > > > > > > > +// 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? > > > > I looked more carefully at the code of the association section, > > we will always know the extended address of the devices which are > > associated to us, however there may be situations where the second > > device to compare with this list only comes with a short address and pan > > ID, so your initial comment needs to be addressed. > > > > > > 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. > > > > As long as we know the PAN ID, it should be fine. > > > > yep, so you will add a check of panid when mode is short address type? The above sentence was meant "in the common case". But here we are in a very specific location which does not really apply to the common case. During associations (because this is all what this is about), the stack always saves the extended address, and when it is known, the short address as well. When I need these comparisons, it is because a device (parent or children) has requested an association or a disassociation, and I want to find the local "structure" which matches it. I looked again at the specification, it says: - In the case of an association request, the device will use its extended address only. - In the case of a disassociation notification, the device will also use its extended address only and will address it to the coordinator using the right PAN ID. So actually, on one side, the "we might use the short address" never applies and on the other side, checking the PAN ID *here* is not relevant either as it should be done earlier (and the disassociation canceled if the wrong PAN ID is used). So in v4 I will just error out if one uses short addressing in this function, because it would make no sense at all. I will also check the PAN ID in the disassociation procedure. Thanks, Miquèl
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 + */ +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; + } +} + +bool cfg802154_device_is_associated(struct wpan_dev *wpan_dev) +{ + bool is_assoc; + + mutex_lock(&wpan_dev->association_lock); + is_assoc = !list_empty(&wpan_dev->children) || wpan_dev->parent; + mutex_unlock(&wpan_dev->association_lock); + + return is_assoc; +} + +bool cfg802154_device_is_parent(struct wpan_dev *wpan_dev, + struct ieee802154_addr *target) +{ + lockdep_assert_held(&wpan_dev->association_lock); + + if (cfg802154_same_addr(wpan_dev->parent, target)) + return true; + + return false; +} + +struct ieee802154_pan_device * +cfg802154_device_is_child(struct wpan_dev *wpan_dev, + struct ieee802154_addr *target) +{ + struct ieee802154_pan_device *child; + + lockdep_assert_held(&wpan_dev->association_lock); + + list_for_each_entry(child, &wpan_dev->children, node) + if (cfg802154_same_addr(child, target)) + return child; + + return NULL; +}
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