Message-ID: <CAK-6q+jJKoFy359_Pd4_d+EfqLw4PTdG4F7H4u+URD=UKu9k6w@mail.gmail.com>
Date:   Sun, 20 Nov 2022 19:57:31 -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 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?

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

Powered by blists - more mailing lists