Message-ID: <CAK-6q+g4v0=FLmTqhXmf-mqORNy69B-AxsftR9Bij-x1UGaqgA@mail.gmail.com>
Date:   Wed, 23 Nov 2022 20:49:51 -0500
From:   Alexander Aring <aahringo@...hat.com>
To:     Miquel Raynal <miquel.raynal@...tlin.com>
Cc:     Alexander Aring <alex.aring@...il.com>,
        Stefan Schmidt <stefan@...enfreihafen.org>,
        linux-wpan@...r.kernel.org,
        David Girault <david.girault@...vo.com>,
        Romuald Despres <romuald.despres@...vo.com>,
        Frederic Blain <frederic.blain@...vo.com>,
        Nicolas Schodet <nico@...fr.eu.org>,
        Guilhem Imberton <guilhem.imberton@...vo.com>,
        Thomas Petazzoni <thomas.petazzoni@...tlin.com>,
        "David S. Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>,
        Paolo Abeni <pabeni@...hat.com>,
        Eric Dumazet <edumazet@...gle.com>, netdev@...r.kernel.org
Subject: Re: [PATCH wpan-next 1/3] ieee802154: Advertize coordinators discovery

Hi,

On Wed, Nov 23, 2022 at 12:07 PM Miquel Raynal
<miquel.raynal@...tlin.com> wrote:
>
> Hi Alexander,
>
> aahringo@...hat.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@...tlin.com> wrote:
> > >
> > > Hi Alexander,
> > >
> > > aahringo@...hat.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@...tlin.com> wrote:
> > > > >
> > > > > Hi Alexander,
> > > > >
> > > > > aahringo@...hat.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@...tlin.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@...vo.com>
> > > > > > > Signed-off-by: David Girault <david.girault@...vo.com>
> > > > > > > Signed-off-by: Miquel Raynal <miquel.raynal@...tlin.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

Powered by blists - more mailing lists