Message-ID: <20220107120226.513554db@xps13>
Date:   Fri, 7 Jan 2022 12:02:26 +0100
From:   Miquel Raynal <miquel.raynal@...tlin.com>
To:     Alexander Aring <alex.aring@...il.com>
Cc:     Stefan Schmidt <stefan@...enfreihafen.org>,
        Nicolas Schodet <nico@...fr.eu.org>,
        "David S. Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>,
        "open list:NETWORKING [GENERAL]" <netdev@...r.kernel.org>,
        linux-wpan - ML <linux-wpan@...r.kernel.org>,
        David Girault <david.girault@...vo.com>,
        Romuald Despres <romuald.despres@...vo.com>,
        Frederic Blain <frederic.blain@...vo.com>,
        Thomas Petazzoni <thomas.petazzoni@...tlin.com>,
        kernel list <linux-kernel@...r.kernel.org>
Subject: Re: [net-next 12/18] net: mac802154: Handle scan requests

Hi Alexander,

alex.aring@...il.com wrote on Thu, 6 Jan 2022 20:07:24 -0500:

> Hi,
> 
> On Thu, 6 Jan 2022 at 14:15, Miquel Raynal <miquel.raynal@...tlin.com> wrote:
> >
> > Hi Alexander,
> >
> > alex.aring@...il.com wrote on Wed, 5 Jan 2022 19:38:12 -0500:
> >  
> > > Hi,
> > >
> > >
> > > On Wed, 5 Jan 2022 at 15:55, Miquel Raynal <miquel.raynal@...tlin.com> wrote:
> > > ...  
> > > > > rest in software is a bigger task here...  
> > > >
> > > > On the symbol duration side I feel I'm close to a working PoC.
> > > >  
> > >
> > > oh, ok.  
> >
> > I think it's ready, I'll soon send two series:
> > - the symbol duration update
> > - v2 for this series, which will not apply without the symbol duration
> >   update.
> >  
> 
> ok. Thanks.
> 
> > > > So there is 'only' this item left in my mind. Could you please clarify
> > > > what you expect from me exactly in terms of support for the promiscuous
> > > > filters we discussed so far?
> > > >  
> > >
> > > I think for now it's okay to set the device into promiscuous mode and
> > > enable the flag which checks for bad FCS... we can still implement the
> > > filter modes later (and I think it should work on all supported
> > > transceivers (except that SoftMAC/HardMAC thing)).  
> >
> > I considered the following options in order to do that:
> > 1- Hack all ->set_promiscuous() driver implementations to set
> >    IEEE802154_HW_RX_DROP_BAD_CKSUM as long as it was not already set
> >    initially.
> > 2- Set the above flag at scan level, ie. in
> >    scan.c:mac802154_set_promiscuous_mode(). But this would be a bit
> >    ugly and I'd need to add a persistent field somewhere in the
> >    wpan_dev structure to remember how the flags settings where before
> >    the scan code hacked it.  
> 
> I think there exists two layers of "promiscuous mode": there exists a
> phy level and a mac level. I am not sure at some points what's meant
> now. Whereas phy is regarding the filtering mode whatever will be
> delivered to mac802154, the wpan (mac) level is what 802.15.4 mac says
> it is. The mac promiscuous mode requires the phy promiscuous mode (so
> far I understand).
> 
> > 3- Add more code in hwsim to handle checksum manually instead of
> >    by default setting the above flag to request the core to do the
> >    job. This way no driver would actually set this flag. We can then
> >    consider it "volatile" and would not need to track its state.
> > 4- We know that we are in a scan thanks to a mac802154 internal
> >    variable, we can just assume that all drivers are in promiscuous
> >    mode and that none of them actually checks the FCS. This is
> >    certainly the simplest yet effective solution. In the worst case, we
> >    are just doing the check twice, which I believe does not hurt as
> >    long as the checksum is not cut off. If the checksum is cut, then
> >    the core is buggy because it always remove the two last bytes.
> >
> > I picked 4 for now, but if you think this is unreliable, please
> > tell me what do you prefer otherwise.
> >  
> 
> I think we have some flag to add a calculated checksum
> "IEEE802154_HW_RX_OMIT_CKSUM" which is currently not used by any
> driver. I think your case that the checksum is cut off does not exist
> in 4.? So far I understand we can still move the FCS check to the
> hardware by not breaking anything if the hardware supports it and the
> behavior should be the same.

That is correct.

> So do the 4.?

Done, thanks!

> > > One point to promiscuous mode, currently we have a checking for if a
> > > phy is in promiscuous mode on ifup and it would forbid to ifup a node
> > > interface if the phy is in promiscuous mode (because of the missing
> > > automatic acknowledgement). I see there is a need to turn the phy into
> > > promiscuous mode during runtime... so we need somehow make sure the
> > > constraints are still valid here.  
> >
> > Yes, the code (rx.c) currently drops everything that is not a beacon
> > during a scan.
> >  
> 
> Okay, I will look at this code closely regarding whenever multiple
> wpan_devs are running.

The "scanning" boolean is stored as a wpan_phy member (IIRC) so we
should be good on this regard (now that I have a clearer picture of the
dependencies).

> You should also check for possible stop of all possible wpan dev
> transmit queues, if it's not already done.

I forgot about this path. Indeed I'll add a check in the transmit path
as well, of course.

> I suppose a scan can take a
> long time and we should not send some data frames out. I am thinking
> about the long time scan operation... if we stop the queue for a long
> time I think we will drop a lot, however the scan can only be
> triggered by the right permissions and the user should be aware of the
> side effects. Proper reliable upper layer protocols will care or non
> reliable will not care about this.
> 
> There still exists the driver "ca8210" which is the mentioned HardMAC
> transceiver in SoftMAC. There should somehow be a flag that it cannot
> do a scan and the operation should not be allowed as the xmit callback
> allows dataframes only.

So it cannot do an active scan, but a passive scan would be allowed
(there is no transmission, and the beacons are regular valid frames,
I suppose they should not be filtered out by the hardware).

So we actually need these hooks back :-) Because the right thing to do
here is to use the "FYI here is the scan op that is starting" message
from the mac to the drivers and this driver should return "nope,
-ENOTSUPP". The mac would react in this case by canceling the
operation and returning an error to the caller. Same when sending
beacons if we consider beacons as !dataframes.

Thanks,
Miquèl

Powered by blists - more mailing lists