Message-ID: <CAK-6q+jna-UXWVvTjnJnJ+LADB0oP_WmVJV0N5r00cb1tfhkpA@mail.gmail.com>
Date:   Sun, 23 Oct 2022 19:13:05 -0400
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 S. Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>,
        Paolo Abeni <pabeni@...hat.com>,
        Eric Dumazet <edumazet@...gle.com>, 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>,
        Guilhem Imberton <guilhem.imberton@...vo.com>,
        Thomas Petazzoni <thomas.petazzoni@...tlin.com>
Subject: Re: [PATCH wpan-next] mac802154: Allow the creation of coordinator interfaces

Hi,

On Wed, Oct 19, 2022 at 5:52 AM Miquel Raynal <miquel.raynal@...tlin.com> wrote:
>
> Hi Alexander,
>
> aahringo@...hat.com wrote on Tue, 18 Oct 2022 19:57:19 -0400:
>
> > Hi,
> >
> > On Tue, Oct 18, 2022 at 2:36 PM 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 support will be improved later, in particular regarding
> > > the filtering.
> > >
> > > Signed-off-by: Miquel Raynal <miquel.raynal@...tlin.com>
> > > ---
> > >  net/mac802154/iface.c | 14 ++++++++------
> > >  net/mac802154/main.c  |  2 ++
> > >  net/mac802154/rx.c    | 11 +++++++----
> > >  3 files changed, 17 insertions(+), 10 deletions(-)
> > >
> > > diff --git a/net/mac802154/iface.c b/net/mac802154/iface.c
> > > index d9b50884d34e..682249f3369b 100644
> > > --- a/net/mac802154/iface.c
> > > +++ b/net/mac802154/iface.c
> > > @@ -262,13 +262,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.
> > > @@ -565,6 +565,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);
> > > @@ -624,6 +625,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/main.c b/net/mac802154/main.c
> > > index 40fab08df24b..d03ecb747afc 100644
> > > --- a/net/mac802154/main.c
> > > +++ b/net/mac802154/main.c
> > > @@ -219,6 +219,8 @@ int ieee802154_register_hw(struct ieee802154_hw *hw)
> > >
> > >         if (hw->flags & IEEE802154_HW_PROMISCUOUS)
> > >                 local->phy->supported.iftypes |= BIT(NL802154_IFTYPE_MONITOR);
> > > +       else
> > > +               local->phy->supported.iftypes &= ~BIT(NL802154_IFTYPE_COORD);
> > >
> >
> > So this means if somebody in the driver sets iftype COORD is supported
> > but then IEEE802154_HW_PROMISCUOUS is not supported it will not
> > support COORD?
> >
> > Why is IEEE802154_HW_PROMISCUOUS required for COORD iftype? I thought
> > IEEE802154_HW_PROMISCUOUS is required to do a scan?
>
> You are totally right this is inconsistent, I'll drop the else block
> entirely. The fact that HW_PROMISCUOUS is supported when starting a
> scan is handled by the -EOPNOTSUPP return code from
> drv_set_promiscuous_mode() called by drv_start() in
> mac802154_trigger_scan().
>
> However I need your input on the following topic: in my branch I
> have somewhere a patch adding IFTYPE_COORD to the list of
> phy->supported.iftypes in each individual driver. But right now, if we
> drop the promiscuous constraint as you suggest, I don't see anymore the
> need for setting this as a per-driver value.
>
> Should we make the possibility to create IFTYPE_COORD interfaces the
> default instead, something like this?
>
> --- a/net/mac802154/main.c
> +++ b/net/mac802154/main.c
> @@ -118,7 +118,7 @@ ieee802154_alloc_hw(size_t priv_data_len, const struct ieee802154_ops *ops)
>         phy->supported.lbt = NL802154_SUPPORTED_BOOL_FALSE;
>
>         /* always supported */
> -       phy->supported.iftypes = BIT(NL802154_IFTYPE_NODE);
> +       phy->supported.iftypes = BIT(NL802154_IFTYPE_NODE) | BIT(NL802154_IFTYPE_COORD);
>

okay.

> > >         rc = wpan_phy_register(local->phy);
> > >         if (rc < 0)
> > > diff --git a/net/mac802154/rx.c b/net/mac802154/rx.c
> > > index 2ae23a2f4a09..aca348d7834b 100644
> > > --- a/net/mac802154/rx.c
> > > +++ b/net/mac802154/rx.c
> > > @@ -208,6 +208,7 @@ __ieee802154_rx_handle_packet(struct
> > > ieee802154_local *local, int ret;
> > >         struct ieee802154_sub_if_data *sdata;
> > >         struct ieee802154_hdr hdr;
> > > +       struct sk_buff *skb2;
> > >
> > >         ret = ieee802154_parse_frame_start(skb, &hdr);
> > >         if (ret) {
> > > @@ -217,7 +218,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 guess this will work but I would like to see no logic about if it's
> > not MONITOR it's NODE or COORD, because introducing others requires
> > updating those again... however I think it's fine.
>
> Actually I don't get why we would not want this logic, it seem very
> relevant to me. Do you want a helper instead and hide the condition
> inside? Or something else? Or is it just fine for now?
>
> > I would like to see
> > a different receive path for coord_rx() and node_rx(), but yea
> > currently I guess they are mostly the same... in future I think they
> > are required as PACKTE_HOST, etc. will be changed regarding pan
> > coordinator or just coordinator (so far I understood).
>

yes, I think so too.

> I agree it is tempting, but yeah, there is really very little changes
> between the two, for me splitting the rx path would just darken the
> code without bringing much...
>

ok.

> About the way we handle the PAN coordinator role I have a couple of
> questions:
> - shall we consider only the PAN coordinator to be able to accept
>   associations or is any coordinator in the PAN able to do it? (this is
>   not clear to me)

Me either, it sounds for me that coordinators are "leaves" and pan
coordinators are not. It is like in IPv6 level it is a host or router.

> - If a coordinator receives a packet with no destination it should
>   expect it to be targeted at the PAN controller. Should we consider
>   relaying the packet?

I guess it depends what the standard says here?

> - Is relaying a hardware feature or should we do it in software?
>

I think for SoftMAC it is only the address filter which needs to be
changed. The rest is in software. So far what I can see here.

Question is what we are using here in the Linux kernel to provide such
functionality...

e.g. see:

include/net/dst.h

> > >                 if (!ieee802154_sdata_running(sdata))
> > > @@ -230,9 +231,11 @@ __ieee802154_rx_handle_packet(struct
> > > ieee802154_local *local, sdata->required_filtering ==
> > > IEEE802154_FILTERING_4_FRAME_FIELDS) continue;
> > >
> > > -               ieee802154_subif_frame(sdata, skb, &hdr);
> > > -               skb = NULL;
> > > -               break;
> > > +               skb2 = skb_clone(skb, GFP_ATOMIC);
> > > +               if (skb2) {
> > > +                       skb2->dev = sdata->dev;
> > > +                       ieee802154_subif_frame(sdata, skb2, &hdr);
> > > +               }
> > >         }
> > >
> > >         kfree_skb(skb);
> >
> > If we do the clone above this kfree_skb() should be move to
> > ieee802154_rx() right after __ieee802154_rx_handle_packet().
>
> Ok!
>
> > This patch also changes that we deliver one skb to multiple interfaces if
> > there are more than one and I was not aware that we currently do that.
> > :/
>
> Just as a side note: we do that already if we have several MONITOR
> interfaces opened on the same PHY, it is possible to have them all open.
>

yes, I used that feature with hwsim once.

> Regarding the situation where we would have NODE + MONITOR or COORD +
> MONITOR, while the interface creation would work, both could not be
> open at the same time because the following happens:
> mac802154_wpan_open() {
>         ieee802154_check_concurrent_iface() {
>                 ieee802154_check_mac_settings() {
>                         /* prevent the two interface types from being
>                          * open at the same time because the filtering
>                          * needs are not compatible. */
>                 }
>         }
> }
>
> Then, because you asked me to anticipate if we ever want to support more
> than one NODE or COORD interface at the same time, or at least not to
> do anything that would lead to a step back on this regard, I decided I
> would provide all the infrastructure to gracefully handle this
> situation in the Rx path, even though right now it still cannot happen
> because when opening an interface, ieee802154_check_concurrent_iface()
> will also prevent two NODE / COORD interfaces to be opened at the same
> time.

yes, but you are assuming the actual hardware here. A hardware with
multiple address filters can indeed support other interfaces at the
same time. I can also name one, hwsim and a real one - atusb.

>
> TL;DR
> * MONITOR + MONITOR
>   = already supported and working
> * (NODE + MONITOR) || (COORD + MONITOR)
>   = iface creation allowed, but cannot be opened at the same time
>   because of the filtering level incompatibility on a single PHY
> * (NODE + NODE) || (COORD + COORD) || (NODE + COORD)
>   = iface creation allowed, but cannot be opened at the same time
>   because only one PHY available on the device
>
> So for me we are safe and future proof.
>

Yes, this is currently kind of difficult to handle, but I see that we
check such default filtering type thing per phy which depends on what
kind of interface is currently running there? Something like that...

- Alex

Powered by blists - more mailing lists