Message-ID: <CAK-6q+hsE5_6ZiZnq7Q8bMni8i0-Osiert=Y4qf=EFUjX44V2w@mail.gmail.com>
Date:   Sun, 25 Sep 2022 14:56:44 -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 - 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 v3 0/9] net: ieee802154: Support scanning/beaconing

Hi,

On Wed, Sep 21, 2022 at 11:46 AM Miquel Raynal
<miquel.raynal@...tlin.com> wrote:
>
> Hi Alexander,
>
> aahringo@...hat.com wrote on Thu, 8 Sep 2022 20:41:14 -0400:
>
> > Hi,
> >
> > On Thu, Sep 8, 2022 at 3:37 AM Miquel Raynal <miquel.raynal@...tlin.com> wrote:
> > >
> > > Hi Alexander,
> > >
> > > aahringo@...hat.com wrote on Wed, 7 Sep 2022 21:40:13 -0400:
> > >
> > > > Hi,
> > > >
> > > > On Mon, Sep 5, 2022 at 4:34 PM Miquel Raynal <miquel.raynal@...tlin.com> wrote:
> > > > >
> > > > > Hello,
> > > > >
> > > > > A third version of this series, dropping the scan patches for now
> > > > > because before we need to settle on the filtering topic and the
> > > > > coordinator interface topic. Here is just the filtering part, I've
> > > > > integrated Alexander's patches, as well as the atusb fix. Once this is
> > > > > merge there are a few coordinator-related patches, and finally the
> > > > > scan.
> > > >
> > > > I think we have a communication problem here and we should talk about
> > > > what the problems are and agree on a way to solve them.
> > > >
> > > > The problems are:
> > > >
> > > > 1. We never supported switching from an operating phy (interfaces are
> > > > up) into another filtering mode.
> > >
> > > In the trigger scan path there is a:
> > >
> > >         mlme_op_pre() // stop Tx
> > >         drv_stop() // stop Rx
> > >         synchronize_net()
> > >         drv_start(params) // restart Rx with another hw filtering level
> > >
> >
> > Okay, that's looking good.
> >
> > > > 2. Scan requires to be in "promiscuous mode" (according to the
> > > > 802.15.4 spec promiscuous mode). We don't support promiscuous mode
> > > > (according to the 802.15.4 spec promiscuous mode). We "can" however
> > > > use the currently supported mode which does not filter anything
> > > > (IEEE802154_FILTERING_NONE) when we do additional filtering in
> > > > mac802154. _But_ this is only required when the phy is scanning, it
> > > > will also deliver anything to the upper layers.
> > > >
> > > > This patch-series tries to do the second thing, okay that's fine. But
> > > > I thought this should only be done while the phy is in "scanning
> > > > mode"?
> > >
> > > I don't understand what's wrong then. We ask for the "scan mode"
> > > filtering level when starting the scan [1] and we ask for the normal
> > > filtering level when it's done/aborted [2] [3].
> > >
> >
> > There is no problem with that. There is for me a problem with the
> > receive path when certain filtering levels are active.
> >
> > > [1] https://github.com/miquelraynal/linux/blob/wpan-next/scan/net/mac802154/scan.c#L326
> > > [2] https://github.com/miquelraynal/linux/blob/wpan-next/scan/net/mac802154/scan.c#L55
> > >
> > > > The other receive path while not in promiscuous mode
> > > > (phy->filtering == IEEE802154_FILTERING_4_FRAME_FIELDS) should never
> > > > require any additional filtering. I somehow miss this point here.
> > >
> > > Maybe the drv_start() function should receive an sdata pointer. This way
> > > instead of changing the PHY filtering level to what has just be asked
> > > blindly, the code should look at the filtering level of all the
> > > interfaces up on the PHY and apply the lowest filtering level by
> > > hardware, knowing that on a per interface basis, the software will
> > > compensate.
> > >
> > > It should work just fine because local->phy->filtering shows the actual
> > > filtering level of the PHY while sdata->requested_filtering shows the
> > > level of filtering that was expected on each interface. If you don't
> > > like the idea of having a mutable sdata->requested_filtering entry, I
> > > can have an sdata->base_filtering which should only be set by
> > > ieee802154_setup_sdata() and an sdata->expected_filtering which would
> > > reflect what the mac expects on this interface at the present moment.
> > >
> >
> > From my view is that if we disable address filters (all filtering
> > modes except IEEE802154_FILTERING_4_FRAME_FIELDS) we never can call
> > netif_receive_skb(). This patch series tries to "compensate" the
> > missing filtering on phy which is fine only to handle things related
> > for the scan operation but nothing else.
> >
> > The reason why we can't call netif_receive_skb() is because we don't
> > have ackknowledge handling, whereas for scanning we ignore ack frames
> > and that's why we don't need it.
>
> I've digested all of that right before the conference so I think I now
> understand all your fears regarding the possible absence of ACK in
> certain situations. I even share them now.
>
> > > > For 1), the driver should change the filtering mode" when we start to
> > > > "listen", this is done by the start() driver callback. They should get
> > > > all receive parameters and set up receiving to whatever mac802154,
> > > > currently there is a bit of chaos there. To move it into drv_start()
> > > > is just a workaround to begin this step that we move it at some point
> > > > to the driver. I mention 1) here because that should be part of the
> > > > picture how everything works together when the phy is switched to a
> > > > different filter level while it's operating (I mean there are running
> > > > interfaces on it which requires IEEE802154_FILTERING_4_FRAME_FIELDS)
> > > > which then activates the different receive path for the use case of
> > > > scanning (something like (phy->state & WPANPHY_SCANING) == true)?
> > >
> > > Scanning is a dedicated filtering level per-se because it must discard
> > > !beacon frames, that's why I request this level of filtering (which
> > > maybe I should do on a per-interface basis instead of using the *local
> > > poiner).
> > >
> >
> > We only can do a per filter level per interface if the hardware has
> > support for such a thing. Currently there is one address filter and if
> > it's disabled we lose ackknowledge handling (as general rule), we
> > can't compensate by doing any additional filtering by software in this
> > mode.
>
> Yes.
>
> >
> > > > I am sorry, but I somehow miss the picture of how those things work
> > > > together. It is not clear for me and I miss those parts to get a whole
> > > > picture of this. For me it's not clear that those patches are going in
> > > > this direction.
> > >
> > > Please tell me if it's more clear and if you agree with this vision. I
> > > don't have time to draft something this week.
> > >
> >
> > That's fine. We should agree on things like compensate lower filter
> > levels by doing additional softmac filtering to reach
> > IEEE802154_FILTERING_4_FRAME_FIELDS filtering from others because we
> > will lose AACK handling. It is only fine to do that in mac802154
> > receive path but don't deliver it to the upper layer.
>
> So actually, if I try to summarize the situation.
>
> I've tried to make several different subif working on a single PHY.
> Unfortunately, today there is only one address filter per PHY, so

correct. But you are assuming hardware which is currently around. As I
said atusb is one candidate to make a more clever hardware because it
has a co-processor which eventually could handle ack handling if done
right.

> disabling the address filter on one interface would also disable it on
> the other, leading to the ACKs not being handled anymore, which we
> cannot afford.
>

For hardware which has such limitation of one address filter yes.

> So overall I guess we should settle on how we want to handle the
> situation. I propose, to move forward, that we continue to assume that
> it is *not* possible to have several different interfaces running at the
> same time on a single PHY. This involves dropping all the "software
> address filtering" which I proposed, but I guess that's fine.

I agree here, but don't remove code which could add such handling and
allow multiple monitors at the same time.

- Alex

Powered by blists - more mailing lists