Message-ID: <20221121100539.75e13069@xps-13>
Date:   Mon, 21 Nov 2022 10:05:39 +0100
From:   Miquel Raynal <miquel.raynal@...tlin.com>
To:     Alexander Aring <aahringo@...hat.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 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
:)

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

Powered by blists - more mailing lists