Date:   Mon, 20 Feb 2023 21:43:51 -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 Fri, Feb 17, 2023 at 4:02 AM Miquel Raynal <miquel.raynal@...tlin.com> wrote:
>
> Hi Alexander,
>
> aahringo@...hat.com wrote on Thu, 16 Feb 2023 23:34:30 -0500:
>
> > 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...
>
> Ok, I got what you meant.
>
> So to be clear, I assume active and passive scans are phy activities,
> they only involve phy parameters. Beaconing however need access to mac
> parameters.
>

ok.

> For now the structure defining user requests in terms of scanning and
> beaconing is stored into ieee802154_local, but we can move it
> away if needed at some point? For now I have no real example of
> hardMAC device so it's a bit hard to anticipate all their
> needs, but do you want me to move it to wpan_dev? (I would like to keep
> both request descriptors aside from each other).

yes, forget about moving things to the right structure... somehow I
think it's good to have them to not get in too much pain when trying
to introduce a hardmac driver...

- Alex

Powered by blists - more mailing lists