Message-ID: <20230103161047.4efc243c@xps-13>
Date:   Tue, 3 Jan 2023 16:10:47 +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 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>,
        "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
Subject: Re: [PATCH wpan-next v2 6/6] mac802154: Handle passive scanning

Hi Alexander,

aahringo@...hat.com wrote on Mon, 2 Jan 2023 20:15:25 -0500:

> Hi,
> 
> On Mon, Jan 2, 2023 at 8:04 PM Alexander Aring <aahringo@...hat.com> wrote:
> >
> > Hi,
> >
> > On Fri, Dec 16, 2022 at 7:04 PM Miquel Raynal <miquel.raynal@...tlin.com> wrote:
> > ...  
> > > +void mac802154_scan_worker(struct work_struct *work)
> > > +{
> > > +       struct ieee802154_local *local =
> > > +               container_of(work, struct ieee802154_local, scan_work.work);
> > > +       struct cfg802154_scan_request *scan_req;
> > > +       struct ieee802154_sub_if_data *sdata;
> > > +       unsigned int scan_duration = 0;
> > > +       struct wpan_phy* wpan_phy;
> > > +       u8 scan_req_duration;
> > > +       u8 page, channel;
> > > +       int ret;
> > > +
> > > +       /* Ensure the device receiver is turned off when changing channels
> > > +        * because there is no atomic way to change the channel and know on
> > > +        * which one a beacon might have been received.
> > > +        */
> > > +       drv_stop(local);
> > > +       synchronize_net();  
> >
> > Do we do that for every channel switch? I think this is not necessary.
> > It is necessary for bringing the transceiver into scan filtering mode,
> > but we don't need to do that for switching the channel.
> >
> > And there is a difference why we need to do that for filtering. In my
> > mind I had the following reason that the MAC layer is handled in Linux
> > (softMAC) and by offloaded parts on the transceiver, this needs to be
> > synchronized. The PHY layer is completely on the transceiver side,
> > that's why you can switch channels during interface running. There
> > exist some MAC parameters which are offloaded to the hardware and are
> > currently not possible to synchronize while an interface is up,
> > however this could change in future because the new helpers to
> > synchronize softmac/transceiver mac handling.
> >
> > There is maybe a need here to be sure everything is transmitted on the
> > hardware before switching the channel, but this should be done by the
> > new mlme functionality which does a synchronized transmit. However we
> > don't transmit anything here, so there is no need for that yet. We
> > should never stop the transceiver being into receive mode and during
> > scan we should always be into receive mode in
> > IEEE802154_FILTERING_3_SCAN level without never leaving it.
> >
> > ... and happy new year.
> >
> > I wanted to ack this series but this came into my mind. I also wanted
> > to check what exactly happens when a mlme op returns an error like
> > channel access failure? Do we ignore it? Do we do cond_resched() and
> > retry again later? I guess these are questions only if we get into
> > active scanning with more exciting sending of frames, because here we
> > don't transmit anything.  
> 
> Ignore that above about stopping the transceiver being in receive
> mode, you are right... you cannot know on which channel/page
> combination the beacon was received because as the comment says, it is
> not atomic to switch it... sadly the transceiver does not tell us that
> on a per frame basis.

No problem ;)

> Sorry for the noise. Still with my previous comments why it's still
> valid to switch channels while phy is in receive mode but not in scan
> mode, I would say if a user does that... then we don't care. Some
> offloaded parts and softMAC handling still need indeed to be
> synchronized because we don't know how a transceiver reacts on it to
> change registers there while transmitting.

In case of error during an MLME transmission the behavior depends on
which MLME it is. As you said, for passive scanning it does not matter
because we do not really transmit anything yet. However active scans
and beacons sending need this feature and have to transmit frames. I
assumed both actions should not have a very high importance and if the
transmission fails, we will just wait for the next slot and try again.
It was the simplest approach I could come up with (I will send the
beacons part very soon). Should we consider retrying a fixed number of
times immediately after the failure? 1 time? 2 times? Or should we, as
it is implemented, wait for the next slot?

Thanks,
Miquèl

Powered by blists - more mailing lists