Subject: Re: [PATCH wpan-next 1/6] ieee802154: Add support for user scanning requests

From: Miquel Raynal <[email-masked]>
Date: Fri, 17 Feb 2023 10:02:20 +0100

Hi Alexander,

[email-masked] wrote on Thu, 16 Feb 2023 23:34:30 -0500:

> Hi,
> 
> On Tue, Feb 14, 2023 at 9:07 AM Miquel Raynal <[email-masked]> wrote:
> >
> > Hi Alexander,
> >
> > [email-masked] wrote on Tue, 14 Feb 2023 08:53:57 -0500:
> >  
> > > Hi,
> > >
> > > On Tue, Feb 14, 2023 at 8:34 AM Alexander Aring <[email-masked]> wrote:  
> > > >
> > > > Hi,
> > > >
> > > > On Mon, Feb 13, 2023 at 12:35 PM Miquel Raynal
> > > > <[email-masked]> 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.

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).

Thanks,
Miquèl