Message-ID: <20220823182950.1c722e13@xps-13>
Date:   Tue, 23 Aug 2022 18:29:50 +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>,
        Eric Dumazet <edumazet@...gle.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 01/20] net: mac802154: Allow the creation of
 coordinator interfaces

Hi Alexander,

aahringo@...hat.com wrote on Tue, 23 Aug 2022 08:33:30 -0400:

> Hi,
> 
> On Fri, Aug 19, 2022 at 1:11 PM Miquel Raynal <miquel.raynal@...tlin.com> wrote:
> >
> > Hi Alexander,
> >
> > aahringo@...hat.com wrote on Tue, 5 Jul 2022 21:51:02 -0400:
> >  
> > > Hi,
> > >
> > > On Fri, Jul 1, 2022 at 10:36 AM Miquel Raynal <miquel.raynal@...tlin.com> wrote:  
> > > >
> > > > As a first strep in introducing proper PAN management and association,
> > > > we need to be able to create coordinator interfaces which might act as
> > > > coordinator or PAN coordinator.
> > > >
> > > > Hence, let's add the minimum support to allow the creation of these
> > > > interfaces. This might be restrained and improved later.
> > > >
> > > > Signed-off-by: Miquel Raynal <miquel.raynal@...tlin.com>
> > > > ---
> > > >  net/mac802154/iface.c | 14 ++++++++------
> > > >  net/mac802154/rx.c    |  2 +-
> > > >  2 files changed, 9 insertions(+), 7 deletions(-)
> > > >
> > > > diff --git a/net/mac802154/iface.c b/net/mac802154/iface.c
> > > > index 500ed1b81250..7ac0c5685d3f 100644
> > > > --- a/net/mac802154/iface.c
> > > > +++ b/net/mac802154/iface.c
> > > > @@ -273,13 +273,13 @@ ieee802154_check_concurrent_iface(struct ieee802154_sub_if_data *sdata,
> > > >                 if (nsdata != sdata && ieee802154_sdata_running(nsdata)) {
> > > >                         int ret;
> > > >
> > > > -                       /* TODO currently we don't support multiple node types
> > > > -                        * we need to run skb_clone at rx path. Check if there
> > > > -                        * exist really an use case if we need to support
> > > > -                        * multiple node types at the same time.
> > > > +                       /* TODO currently we don't support multiple node/coord
> > > > +                        * types we need to run skb_clone at rx path. Check if
> > > > +                        * there exist really an use case if we need to support
> > > > +                        * multiple node/coord types at the same time.
> > > >                          */
> > > > -                       if (wpan_dev->iftype == NL802154_IFTYPE_NODE &&
> > > > -                           nsdata->wpan_dev.iftype == NL802154_IFTYPE_NODE)
> > > > +                       if (wpan_dev->iftype != NL802154_IFTYPE_MONITOR &&
> > > > +                           nsdata->wpan_dev.iftype != NL802154_IFTYPE_MONITOR)
> > > >                                 return -EBUSY;
> > > >
> > > >                         /* check all phy mac sublayer settings are the same.
> > > > @@ -577,6 +577,7 @@ ieee802154_setup_sdata(struct ieee802154_sub_if_data *sdata,
> > > >         wpan_dev->short_addr = cpu_to_le16(IEEE802154_ADDR_BROADCAST);
> > > >
> > > >         switch (type) {
> > > > +       case NL802154_IFTYPE_COORD:
> > > >         case NL802154_IFTYPE_NODE:
> > > >                 ieee802154_be64_to_le64(&wpan_dev->extended_addr,
> > > >                                         sdata->dev->dev_addr);
> > > > @@ -636,6 +637,7 @@ ieee802154_if_add(struct ieee802154_local *local, const char *name,
> > > >         ieee802154_le64_to_be64(ndev->perm_addr,
> > > >                                 &local->hw.phy->perm_extended_addr);
> > > >         switch (type) {
> > > > +       case NL802154_IFTYPE_COORD:
> > > >         case NL802154_IFTYPE_NODE:
> > > >                 ndev->type = ARPHRD_IEEE802154;
> > > >                 if (ieee802154_is_valid_extended_unicast_addr(extended_addr)) {
> > > > diff --git a/net/mac802154/rx.c b/net/mac802154/rx.c
> > > > index b8ce84618a55..39459d8d787a 100644
> > > > --- a/net/mac802154/rx.c
> > > > +++ b/net/mac802154/rx.c
> > > > @@ -203,7 +203,7 @@ __ieee802154_rx_handle_packet(struct ieee802154_local *local,
> > > >         }
> > > >
> > > >         list_for_each_entry_rcu(sdata, &local->interfaces, list) {
> > > > -               if (sdata->wpan_dev.iftype != NL802154_IFTYPE_NODE)
> > > > +               if (sdata->wpan_dev.iftype == NL802154_IFTYPE_MONITOR)
> > > >                         continue;  
> > >
> > > I probably get why you are doing that, but first the overall design is
> > > working differently - means you should add an additional receive path
> > > for the special interface type.
> > >
> > > Also we "discovered" before that the receive path of node vs
> > > coordinator is different... Where is the different handling here? I
> > > don't see it, I see that NODE and COORD are the same now (because that
> > > is _currently_ everything else than monitor). This change is not
> > > enough and does "something" to handle in some way coordinator receive
> > > path but there are things missing.
> > >
> > > 1. Changing the address filters that it signals the transceiver it's
> > > acting as coordinator
> > > 2. We _should_ also have additional handling for whatever the
> > > additional handling what address filters are doing in mac802154
> > > _because_ there is hardware which doesn't have address filtering e.g.
> > > hwsim which depend that this is working in software like other
> > > transceiver hardware address filters.
> > >
> > > For the 2. one, I don't know if we do that even for NODE right or we
> > > just have the bare minimal support there... I don't assume that
> > > everything is working correctly here but what I want to see is a
> > > separate receive path for coordinators that people can send patches to
> > > fix it.  
> >
> > Yes, we do very little differently between the two modes, that's why I
> > took the easy way: just changing the condition. I really don't see what
> > I can currently add here, but I am fine changing the style to easily
> > show people where to add filters for such or such interface, but right
> > now both path will look very "identical", do we agree on that?  
> 
> mostly yes, but there exists a difference and we should at least check
> if the node receive path violates the coordinator receive path and
> vice versa.
> Put it in a receive_path() function and then coord_receive_path(),
> node_receive_path() that calls the receive_path() and do the
> additional filtering for coordinators, etc.
> 
> There should be a part in the standard about "third level filter rule
> if it's a coordinator".
> btw: this is because the address filter on the transceiver needs to
> have the "i am a coordinator" boolean set which is missing in this
> series. However it depends on the transceiver filtering level and the
> mac802154 receive path if we actually need to run such filtering or
> not.

I must be missing some information because I can't find any places
where what you suggest is described in the spec.

I agree there are multiple filtering level so let's go through them one
by one (6.7.2 Reception and rejection):
- first level: is the checksum (FCS) valid?
	yes -> goto second level
	no -> drop
- second level: are we in promiscuous mode?
	yes -> forward to upper layers
	no -> goto second level (bis)
- second level (bis): are we scanning?
	yes -> goto scan filtering
	no -> goto third level
- scan filtering: is it a beacon?
	yes -> process the beacon
	no -> drop
- third level: is the frame valid? (type, source, destination, pan id,
  etc)
	yes -> forward to upper layers
	no -> drop

But none of them, as you said, is dependent on the interface type.
There is no mention of a specific filtering operation to do in all
those cases when running in COORD mode. So I still don't get what
should be included in either node_receive_path() which should be
different than in coord_receive_path() for now.

There are, however, two situations where the interface type has its
importance:
- Enhanced beacon requests with Enhanced beacon filter IE, which asks
  the receiving device to process/drop the request upon certain
  conditions (minimum LQI and/or randomness), as detailed in
  7.4.4.6 Enhanced Beacon Filter IE. But, as mentioned in
  7.5.9 Enhanced Beacon Request command: "The Enhanced Beacon Request
  command is optional for an FFD and an RFD", so this series was only
  targeting basic beaconing for now.
- In relaying mode, the destination address must not be validated
  because the message needs to be re-emitted. Indeed, a receiver in
  relaying mode may not be the recipient. This is also optional and out
  of the scope of this series.

Right now I have the below diff, which clarifies the two path, without
too much changes in the current code because I don't really see why it
would be necessary. Unless you convince me otherwise or read the spec
differently than I do :) What do you think?

Thanks,
Miquèl

---

--- a/net/mac802154/rx.c
+++ b/net/mac802154/rx.c
@@ -194,6 +194,7 @@ __ieee802154_rx_handle_packet(struct ieee802154_local *local,
        int ret;
        struct ieee802154_sub_if_data *sdata;
        struct ieee802154_hdr hdr;
+       bool iface_found = false;
 
        ret = ieee802154_parse_frame_start(skb, &hdr);
        if (ret) {
@@ -203,18 +204,31 @@ __ieee802154_rx_handle_packet(struct ieee802154_local *local,
        }
 
        list_for_each_entry_rcu(sdata, &local->interfaces, list) {
-               if (sdata->wpan_dev.iftype != NL802154_IFTYPE_NODE)
+               if (sdata->wpan_dev.iftype == NL802154_IFTYPE_MONITOR)
                        continue;
 
                if (!ieee802154_sdata_running(sdata))
                        continue;
 
+               iface_found = true;
+               break;
+       }
+
+       if (!iface_found) {
+               kfree_skb(skb);
+               return;
+       }
+
+       /* TBD: Additional filtering is possible on NODEs and/or COORDINATORs */
+       switch (sdata->wpan_dev.iftype) {
+       case NL802154_IFTYPE_COORD:
+       case NL802154_IFTYPE_NODE:
                ieee802154_subif_frame(sdata, skb, &hdr);
-               skb = NULL;
+               break;
+       default:
+               kfree_skb(skb);
                break;
        }
-
-       kfree_skb(skb);
 }
 
 static void

Powered by blists - more mailing lists