diff mbox series

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

Message ID 20221102151915.1007815-2-miquel.raynal@bootlin.com (mailing list archive)
State Awaiting Upstream
Delegated to: Netdev Maintainers
Headers show
Series IEEE 802.15.4 PAN discovery handling | expand

Checks

Context Check Description
netdev/tree_selection success Guessing tree name failed - patch did not apply

Commit Message

Miquel Raynal Nov. 2, 2022, 3:19 p.m. UTC
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(+)

Comments

Alexander Aring Nov. 7, 2022, 2:01 a.m. UTC | #1
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
Miquel Raynal Nov. 18, 2022, 10:04 p.m. UTC | #2
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
Alexander Aring Nov. 21, 2022, 12:57 a.m. UTC | #3
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
Alexander Aring Nov. 21, 2022, 1:01 a.m. UTC | #4
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
Miquel Raynal Nov. 21, 2022, 9:05 a.m. UTC | #5
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
Alexander Aring Nov. 21, 2022, 11:54 p.m. UTC | #6
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
Miquel Raynal Nov. 23, 2022, 5:07 p.m. UTC | #7
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
Alexander Aring Nov. 24, 2022, 1:49 a.m. UTC | #8
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
diff mbox series

Patch

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 */