Re: [PATCH wpan-next 09/20] net: mac802154: Introduce a global device lock

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 - ML <linux-wpan@vger.kernel.org>,
	"David S. Miller" <davem@davemloft.net>,
	Jakub Kicinski <kuba@kernel.org>, Paolo Abeni <pabeni@redhat.com>,
	Eric Dumazet <edumazet@google.com>,
	Network Development <netdev@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>,
	Thomas Petazzoni <thomas.petazzoni@bootlin.com>
Subject: Re: [PATCH wpan-next 09/20] net: mac802154: Introduce a global device lock
Date: Sun, 3 Jul 2022 21:12:43 -0400	[thread overview]
Message-ID: <CAK-6q+hu4YGfU9V5EkRiT+Z8MJhOEeVsVv=vEz5fHPkDL99=TQ@mail.gmail.com> (raw)
In-Reply-To: <20220701143052.1267509-10-miquel.raynal@bootlin.com>

Hi,

On Fri, Jul 1, 2022 at 10:36 AM Miquel Raynal <miquel.raynal@bootlin.com> wrote:
>
> The purpose of this device lock is to prevent the removal of the device
> while an asynchronous MLME operation happens. The RTNL works well for
> that but in a later series having the RTNL taken here will be
> problematic and will cause lockdep to warn us about a circular
> dependency. We don't really need the RTNL here, just a serialization
> over this operation.
>
> Replace the RTNL calls with this new lock.

I am unhappy about this solution. Can we not interrupt the ongoing
operation "scan" here and come to an end at a stop?

The RTNL is NOT only to prevent the removal of something... If mostly
all operations are protected by and I know one which makes trouble
here... setting page/channel. I know we don't hold the rtnl lock on
other transmit functionality for phy settings which has other reasons
why we allow it... but here we offer a mac operation which delivers
wrong results if somebody does another setting e.g. set page/channel
while scan is going on and we should prevent this.

Dropping the rtnl lock, yes we can do that... I cannot think about all
the side effects which this change will bring into, one I know is the
channel setting, mostly everything that is interfering with a scan and
then ugly things which we don't want... preparing the code for the
page/channel gives us a direction on how to fix and check the other
cases if we find them. btw: we should do this on another approach
anyway because the rtnl lock is not held during a whole operation and
we don't want that.

We should also take care that we hold some references which we held
during the scan, even if it's protected by stop (just for
correctness).

- Alex