Date:   Tue, 7 Jun 2022 18:16:08 +0200
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 - ML <linux-wpan@...r.kernel.org>,
        "David S. Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>,
        Paolo Abeni <pabeni@...hat.com>,
        Network Development <netdev@...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>,
        Thomas Petazzoni <thomas.petazzoni@...tlin.com>
Subject: Re: [PATCH wpan-next 1/6] net: ieee802154: Drop coordinator
 interface type

Hi Alex,

aahringo@...hat.com wrote on Mon, 6 Jun 2022 23:04:06 -0400:

> Hi,
> 
> On Mon, Jun 6, 2022 at 11:43 AM Miquel Raynal <miquel.raynal@...tlin.com> wrote:
> >
> > Hi Alexander,
> >
> > aahringo@...hat.com wrote on Fri, 3 Jun 2022 22:01:38 -0400:
> >  
> > > Hi,
> > >
> > > On Fri, Jun 3, 2022 at 2:34 PM Miquel Raynal <miquel.raynal@...tlin.com> wrote:  
> > > >
> > > > The current enum is wrong. A device can either be an RFD, an RFD-RX, an
> > > > RFD-TX or an FFD. If it is an FFD, it can also be a coordinator. While
> > > > defining a node type might make sense from a strict software point of
> > > > view, opposing node and coordinator seems meaningless in the ieee
> > > > 802.15.4 world. As this enumeration is not used anywhere, let's just
> > > > drop it. We will in a second time add a new "node type" enumeration
> > > > which apply only to nodes, and does differentiates the type of devices
> > > > mentioned above.
> > > >  
> > >
> > > First you cannot say if this is not used anywhere else.  
> >
> > Mmmh, that's tricky, I really don't see how that might be a
> > problem because there is literally nowhere in the kernel that uses this
> > type, besides ieee802154_setup_sdata() which would just BUG() if this
> > type was to be used. So I assumed it was safe to be removed.
> >  
> 
> this header is somehow half uapi where we copy it into some other
> software e.g. wpan-tools as you noticed.
> 
> > > Second I have
> > > a different opinion here that you cannot just "switch" the role from
> > > RFD, FFD, whatever.  
> >
> > I agree with this, and that's why I don't understand this enum.
> >
> > A device can either be a NODE (an active device) or a MONITOR (a
> > passive device) at a time. We can certainly switch from one to
> > another at run time.
> >
> > A NODE can be either an RFD or an FFD. That is a static property which
> > cannot change.
> >
> > However being a coordinator is just an additional property of a NODE
> > which is of type FFD, and this can change over time.
> >
> > So I don't get what having a coordinator interface would bring. What
> > was the idea behind its introduction then?
> >  
> 
> There exists arguments which I have in my mind right now:
> 
> 1. frame parsing/address filter (which is somehow missing in your patches)
> 
> The parsing of frames is different from other types (just as monitor
> interfaces). You will notice that setting up the address filter will
> require a parameter if coordinator or not.

I think this is something that I completely missed until now, can you
point me to where/how this is expected to be done? I don't see anything
wpan specific filtering implementation. What is expected on this area
and is there code that I missed already?

> Changing the address
> filterung during runtime of an interface is somehow _not_ supported.
> The reason is that the datasheets tell you to first set up an address
> filter and then switch into receiving mode. Changing the address
> filter during receive mode (start()/stop()) is not a specified
> behaviour. Due to bus communication it also cannot be done atomically.
> This might be avoidable but is a pain to synchronize if you really
> depend on hw address filtering which we might do in future. It should
> end in a different receiving path e.g. node_rx/monitor_rx.

Got it.

> 
> 2. HardMAC transceivers
> 
> The add_interface() callback will be directly forwarded to the driver
> and the driver will during the lifetime of this interface act as a
> coordinator and not a mixed mode which can be disabled and enabled
> anytime. I am not even sure if this can ever be handled in such a way
> from hardmac transceivers, it might depend on the transceiver
> interface but we should assume some strict "static" handling. Static
> handling means here that the transceiver is unable to switch from
> coordinator and vice versa after some initialization state.

Okay. I just completely missed the "interface add" command. So your
advice is to treat the "PAN coordinator" property as a static property
for a given interface, which seems reasonable.

For now I will assume the same treatment when adding the interface is
required compared to a NODE, but if something comes to your mind,
please let me know.

By the way, is there a mechanism limiting the number of interfaces on a
device? Should we prevent the introduction of a coordinator iface if
there is a node iface active?

> 3. coordinator (any $TYPE specific) userspace software
> 
> May the main argument. Some coordinator specific user space daemon
> does specific type handling (e.g. hostapd) maybe because some library
> is required. It is a pain to deal with changing roles during the
> lifetime of an interface and synchronize user space software with it.
> We should keep in mind that some of those handlings will maybe be
> moved to user space instead of doing it in the kernel. I am fine with
> the solution now, but keep in mind to offer such a possibility.
> 
> I think the above arguments are probably the same why wireless is
> doing something similar and I would avoid running into issues or it's
> really difficult to handle because you need to solve other Linux net
> architecture handling at first.

Yep.

> > > You are mixing things here with "role in the network" and what the
> > > transceiver capability (RFD, FFD) is, which are two different things.  
> >
> > I don't think I am, however maybe our vision differ on what an
> > interface should be.
> >  
> > > You should use those defines and the user needs to create a new
> > > interface type and probably have a different extended address to act
> > > as a coordinator.  
> >
> > Can't we just simply switch from coordinator to !coordinator (that's
> > what I currently implemented)? Why would we need the user to create a
> > new interface type *and* to provide a new address?
> >
> > Note that these are real questions that I am asking myself. I'm fine
> > adapting my implementation, as long as I get the main idea.
> >  
> 
> See above.

That's okay for me. I will adapt my implementation to use the interface
thing. In the mean time additional details about what a coordinator
interface should do differently (above question) is welcome because
this is not something I am really comfortable with.

Thanks,
Miquèl

Powered by blists - more mailing lists