Re: [PATCH wpan-next v2 6/6] mac802154: Handle passive scanning

From: Alexander Aring <aahringo@redhat.com>
To: Miquel Raynal <miquel.raynal@bootlin.com>
Cc: Alexander Aring <alex.aring@gmail.com>,
	Stefan Schmidt <stefan@datenfreihafen.org>,
	linux-wpan@vger.kernel.org,
	David Girault <david.girault@qorvo.com>,
	Romuald Despres <romuald.despres@qorvo.com>,
	Frederic Blain <frederic.blain@qorvo.com>,
	Nicolas Schodet <nico@ni.fr.eu.org>,
	Guilhem Imberton <guilhem.imberton@qorvo.com>,
	Thomas Petazzoni <thomas.petazzoni@bootlin.com>,
	"David S. Miller" <davem@davemloft.net>,
	Jakub Kicinski <kuba@kernel.org>, Paolo Abeni <pabeni@redhat.com>,
	Eric Dumazet <edumazet@google.com>,
	netdev@vger.kernel.org
Subject: Re: [PATCH wpan-next v2 6/6] mac802154: Handle passive scanning
Date: Mon, 2 Jan 2023 20:15:25 -0500	[thread overview]
Message-ID: <CAK-6q+gTQwS5n+YVFDeGTqEnSREt9KjC58zq9r2c8T456zXagQ@mail.gmail.com> (raw)
In-Reply-To: <CAK-6q+hJb-py2sNBGYBQeHLbyM_OWzi78-gOf0LcdTukFDO4MQ@mail.gmail.com>

Hi,

On Mon, Jan 2, 2023 at 8:04 PM Alexander Aring <aahringo@redhat.com> wrote:
>
> Hi,
>
> On Fri, Dec 16, 2022 at 7:04 PM Miquel Raynal <miquel.raynal@bootlin.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.

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.

- Alex