Re: netconsole: HARDIRQ-safe -> HARDIRQ-unsafe lock order warning

[Mike Galbraith <efault@gmx.de>]

On Fri, 2025-09-05 at 14:54 +0206, John Ogness wrote:
> 
> On 2025-08-26, Breno Leitao <leitao@debian.org> wrote:
> > On Fri, Aug 22, 2025 at 05:54:28AM +0200, Mike Galbraith wrote:
> > > On Thu, 2025-08-21 at 10:35 -0700, Breno Leitao wrote:
> > > > > On Thu, Aug 21, 2025 at 05:51:59AM +0200, Mike Galbraith wrote:
> > >  
> > > > > > > --- a/drivers/net/netconsole.c
> > > > > > > +++ b/drivers/net/netconsole.c
> > > > > > > @@ -1952,12 +1952,12 @@ static void netcon_write_thread(struct c
> > > > > > >  static void netconsole_device_lock(struct console *con, unsigned long *flags)
> > > > > > >  {
> > > > > > >  	/* protects all the targets at the same time */
> > > > > > > -	spin_lock_irqsave(&target_list_lock, *flags);
> > > > > > > +	spin_lock(&target_list_lock);
> > > > > 
> > > > > I personally think this target_list_lock can be moved to an RCU lock.
> > > > > 
> > > > > If that is doable, then we probably make netconsole_device_lock()
> > > > > to a simple `rcu_read_lock()`, which would solve this problem as well.
> > > 
> > > The bigger issue for the nbcon patch would seem to be the seemingly
> > > required .write_atomic leading to landing here with disabled IRQs.
> 
> Using spin_lock_irqsave()/spin_unlock_irqrestore() within the
> ->device_lock() and ->device->unlock() callbacks is fine. Even with
> PREEMPT_RT this is fine. If you can use RCU to synchronize the target
> list, that is probably a nice optimization, but it is certainly not a
> requirement from the NBCON (and PREEMPT_RT/lockdep) perspective.

I was referring there to !RT+wireless locking meets IRQs disabled, ever
per lockdep, an intolerance shared with RT+spinlock_t.

> > In this case, instead of transmitting through netpoll directly in the
> > .write_atomic context, we could queue the messages for later delivery.
> 
> The ->write_atomic() callback is intended to perform immediate
> transmission. It is called with hardware interrupts disabled and is even
> expected to work from NMI context. If you are not able to implement
> these requirements, do not implement ->write_atomic(). Implementing some
> sort of deferrment mechanism is inappropriate. Such a mechanism already
> exists based on ->write_thread().

Truly atomic packet blasting would be a case of happiness, but barring
that, deferment is way better than the nothing that's available to both
RT and !RT+wireless here/now.  With a .write_atomic that's really just
.write_thread, both RT and !RT+wireless managed to successfully send a
death rattle with the WIP nbcon patch.. a progression for each of them.

> 
	-Mike