Message-ID: <20230217094945.0f0cdd81@xps-13>
Date:   Fri, 17 Feb 2023 09:49:45 +0100
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@...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 Alexander,

aahringo@...hat.com wrote on Thu, 16 Feb 2023 23:37:01 -0500:

> Hi,
> 
> On Tue, Feb 14, 2023 at 9:46 AM Miquel Raynal <miquel.raynal@...tlin.com> wrote:
> >
> >
> > miquel.raynal@...tlin.com wrote on Tue, 14 Feb 2023 15:06:00 +0100:
> >  
> > > 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?  
> >
> > Actually a beacon requests do not require any kind of source identifier,
> > so what do you mean by source address?
> >  
> 
> Is this more clear now?

Yes, thanks!

> > A regular beacon, however, does. Which means sending beacons would
> > include being able to set an address into a monitor interface. So if we
> > want to play with beacons on monitor interfaces, we should also relax
> > the pan_id/addressing rules.  
> 
> mhhh, this is required for active scans? Then a scan operation cannot
> be run on giving a phy only as a parameter...

No, this is not required for active scans. Scans do not require
anything else than phy parameters, AFAICS.

This is required for sending beacons though. So we cannot send beacons
from monitors if we don't relax the pan_id/addressing rules on these
interfaces. Do you think we should?

Thanks,
Miquèl

Powered by blists - more mailing lists