Message-ID: <CAK-6q+g233giuLd56p0G5TqGF+S-NWSkD2MF5nhP+0HLxwnkCA@mail.gmail.com>
Date:   Thu, 16 Feb 2023 23:34:30 -0500
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 1/6] ieee802154: Add support for user scanning requests

Hi,

On Tue, Feb 14, 2023 at 9:07 AM Miquel Raynal <miquel.raynal@...tlin.com> wrote:
>
> Hi Alexander,
>
> aahringo@...hat.com wrote on Tue, 14 Feb 2023 08:53:57 -0500:
>
> > Hi,
> >
> > On Tue, Feb 14, 2023 at 8:34 AM Alexander Aring <aahringo@...hat.com> wrote:
> > >
> > > Hi,
> > >
> > > On Mon, Feb 13, 2023 at 12:35 PM Miquel Raynal
> > > <miquel.raynal@...tlin.com> wrote:
> > > >
> > > > Hi Alexander,
> > > >
> > > > > > > > > +static int nl802154_trigger_scan(struct sk_buff *skb, struct genl_info *info)
> > > > > > > > > +{
> > > > > > > > > +       struct cfg802154_registered_device *rdev = info->user_ptr[0];
> > > > > > > > > +       struct net_device *dev = info->user_ptr[1];
> > > > > > > > > +       struct wpan_dev *wpan_dev = dev->ieee802154_ptr;
> > > > > > > > > +       struct wpan_phy *wpan_phy = &rdev->wpan_phy;
> > > > > > > > > +       struct cfg802154_scan_request *request;
> > > > > > > > > +       u8 type;
> > > > > > > > > +       int err;
> > > > > > > > > +
> > > > > > > > > +       /* Monitors are not allowed to perform scans */
> > > > > > > > > +       if (wpan_dev->iftype == NL802154_IFTYPE_MONITOR)
> > > > > > > > > +               return -EPERM;
> > > > > > > >
> > > > > > > > btw: why are monitors not allowed?
> > > > > > >
> > > > > > > I guess I had the "active scan" use case in mind which of course does
> > > > > > > not work with monitors. Maybe I can relax this a little bit indeed,
> > > > > > > right now I don't remember why I strongly refused scans on monitors.
> > > > > >
> > > > > > Isn't it that scans really work close to phy level? Means in this case
> > > > > > we disable mostly everything of MAC filtering on the transceiver side.
> > > > > > Then I don't see any reasons why even monitors can't do anything, they
> > > > > > also can send something. But they really don't have any specific
> > > > > > source address set, so long addresses are none for source addresses, I
> > > > > > don't see any problem here. They also don't have AACK handling, but
> > > > > > it's not required for scan anyway...
> > > > > >
> > > > > > If this gets too complicated right now, then I am also fine with
> > > > > > returning an error here, we can enable it later but would it be better
> > > > > > to use ENOTSUPP or something like that in this case? EPERM sounds like
> > > > > > you can do that, but you don't have the permissions.
> > > > > >
> > > > >
> > > > > For me a scan should also be possible from iwpan phy $PHY scan (or
> > > > > whatever the scan command is, or just enable beacon)... to go over the
> > > > > dev is just a shortcut for "I mean whatever the phy is under this dev"
> > > > > ?
> > > >
> > > > Actually only coordinators (in a specific state) should be able to send
> > > > beacons, so I am kind of against allowing that shortcut, because there
> > > > are usually two dev interfaces on top of the phy's, a regular "NODE"
> > > > and a "COORD", so I don't think we should go that way.
> > > >
> > > > For scans however it makes sense, I've added the necessary changes in
> > > > wpan-tools. The TOP_LEVEL(scan) macro however does not support using
> > > > the same command name twice because it creates a macro, so this one
> > > > only supports a device name (the interface command has kind of the same
> > > > situation and uses a HIDDEN() macro which cannot be used here).
> > > >
> > >
> > > Yes, I was thinking about scanning only.
> > >
> > > > So in summary here is what is supported:
> > > > - dev <dev> beacon
> > > > - dev <dev> scan trigger|abort
> > > > - phy <phy> scan trigger|abort
> > > > - dev <dev> scan (the blocking one, which triggers, listens and returns)
> > > >
> > > > Do you agree?
> > > >
> > >
> > > Okay, yes. I trust you.
> >
> > btw: at the point when a scan requires a source address... it cannot
> > be done because then a scan is related to a MAC instance -> an wpan
> > interface and we need to bind to it. But I think it doesn't?
>
> I'm not sure I follow you here. You mean in case of active scan? The
> operation is always tight to a device in the end, even if you provide a
> phy in userspace. So I guess it's not a problem. Or maybe I didn't get
> the question right?

As soon scan requires to put somewhere mib values inside e.g. address
information (which need to compared to source address settings (mib)?)
then it's no longer a phy operation -> wpan_phy, it is binded to a
wpan_dev (mac instance on a phy). But the addresses are set to NONE
address type?
I am not sure where all that data is stored right now for a scan
operation, if it's operating on a phy it should be stored on wpan_phy.

Note: there are also differences between wpan_phy and
ieee802154_local, also wpan_dev and ieee802154_sub_if_data structures.
It has something to do with visibility and SoftMAC vs HardMAC, however
the last one we don't really have an infrastructure for and we
probably need to move something around there. In short
wpan_phy/wpan_dev should be only visible by HardMAC (I think) and the
others are only additional data for the same instances used by
mac802154...

- Alex

Powered by blists - more mailing lists