Re: [PATCH wpan-next v4 00/11] ieee802154: Synchronous Tx support

[Miquel Raynal <miquel.raynal@bootlin.com>]

Hi Stefan, Alex,

stefan@datenfreihafen.org wrote on Wed, 1 Jun 2022 23:01:51 +0200:

> Hello.
> 
> On 01.06.22 05:30, Alexander Aring wrote:
> > Hi,
> > 
> > On Thu, May 19, 2022 at 11:06 AM Miquel Raynal
> > <miquel.raynal@bootlin.com> wrote:  
> >>
> >> Hello,
> >>
> >> This series brings support for that famous synchronous Tx API for MLME
> >> commands.
> >>
> >> MLME commands will be used during scan operations. In this situation,
> >> we need to be sure that all transfers finished and that no transfer
> >> will be queued for a short moment.
> >>  
> > 
> > Acked-by: Alexander Aring <aahringo@redhat.com>  
> 
> These patches have been applied to the wpan-next tree. Thanks!
> 
> > There will be now functions upstream which will never be used, Stefan
> > should wait until they are getting used before sending it to net-next.  
> 
> Indeed this can wait until we have a consumer of the functions before pushing this forward to net-next. Pretty sure Miquel is happy to finally move on to other pieces of his puzzle and use them. :-)

Next part is coming!

In the mean time I've experienced a new lockdep warning:

All the netlink commands are executed with the rtnl taken.
In my current implementation, when I configure/edit a scan request or a
beacon request I take a scan_lock or a beacons_lock, so they may only
be taken after the rtnl in this case, which leads to this sequence of
events:
- the rtnl is taken (by the net core)
- the beacon's lock is taken

But now in a beacon's work or an active scan work, what happens is:
- work gets woken up
- the beacon/scan lock is taken
- a beacon/beacon-request frame is transmitted
- the rtnl lock is taken during this transmission

Lockdep then detects a possible circular dependency:
[  490.153387]        CPU0                    CPU1
[  490.153391]        ----                    ----
[  490.153394]   lock(&local->beacons_lock);
[  490.153400]                                lock(rtnl_mutex);
[  490.153406]                                lock(&local->beacons_lock);
[  490.153412]   lock(rtnl_mutex);

So in practice, I always need to have the rtnl lock taken when
acquiring these other locks (beacon/scan_lock) which I think is far
from optimal.

1# One solution is to drop the beacons/scan locks because they are not
useful anymore and simply rely on the rtnl.

2# Another solution would be to change the mlme_tx() implementation to
finally not need the rtnl at all.

Note that just calling ASSERT_RTNL() makes no difference in 2#, it
still means that I always need to acquire the rtnl before acquiring the
beacons/scan locks, which greatly reduces their usefulness and leads to
solution 1# in the end.

IIRC I decided to introduce the rtnl to avoid ->ndo_stop() calls during
an MLME transmission. I don't know if it has another use there. If not,
we may perhaps get rid of the rtnl in mlme_tx() by really handling the
stop calls (but I was too lazy so far to do that).

What direction would you advise?

Thanks,
Miquèl